Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Poetry conversion #10

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,8 @@ jobs:
uses: actions/setup-python@v5
with:
python-version: '3.x'
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install setuptools wheel twine
- name: Build
run: |
python setup.py sdist bdist_wheel
- name: Build and Inspect a Python Package
uses: hynek/build-and-inspect-python-package@v2.5.0
# retrieve your distributions here
- name: Publish package distribution to PyPI
uses: pypa/gh-action-pypi-publish@release/v1
2 changes: 2 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ jobs:
- name: Install dependencies
run: |
python -m pip install --upgrade pip
python -m pip install pipx
pipx install poetry
make setup
- name: Run test
run: |
Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
setup:
pip install -r requirements.txt
poetry install
test:
bash tests/scripts/unit_tests.sh
bash tests/scripts/run-all-tests.sh
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ This library duplicates (and adds to!) much of the functionality in the web cons

## Installation


```bash
pip install parliament
```
Expand Down Expand Up @@ -264,8 +265,7 @@ The process for community auditors is the same as private auditors, except that:
Setup a testing environment

```bash
python3 -m venv ./venv && source venv/bin/activate
pip3 install -r requirements.txt
poetry install --with dev
```

Run unit tests with:
Expand Down
52 changes: 30 additions & 22 deletions parliamentarian/__init__.py
Original file line number Diff line number Diff line change
@@ -1,23 +1,31 @@
"""
This library is a linter for AWS IAM policies.
"""

__version__ = "1.0.0"

import atexit
import fnmatch
import functools
import importlib.resources
import json
import jsoncfg
import re
from contextlib import ExitStack

import pkg_resources
import jsoncfg
import yaml

file_manager = ExitStack()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not clear to me what we are doing with this. aren't cotnext managers usually used in 'with' blocks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the recommended migration from pkg_resources to importlib. This is the suggested pattern for files that need to persist for longer than a with block should be set up for. See here.

That said, I think I missed the bits needed to close the file. I'll get that set up shortly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see, it looks like you might only need the ExitStack when you need the file to stick around for the life of the program and outside the context of the with. however, we just parse them and store the python objects in memory. so i think we can simply use the 'with' keyword. you might be able to avoid the file(s) and use https://importlib-resources.readthedocs.io/en/latest/migration.html#pkg-resources-resource-stream to pass a stream directly to json.load()/yaml.safe_load() too

atexit.register(file_manager.close)

# On initialization, load the IAM data
iam_definition_path = pkg_resources.resource_filename(__name__, "iam_definition.json")
iam_definition_ref = importlib.resources.files(__name__) / "iam_definition.json"
iam_definition_path = file_manager.enter_context(importlib.resources.as_file(iam_definition_ref))
iam_definition = json.load(open(iam_definition_path, "r"))

# And the config data
config_path = pkg_resources.resource_filename(__name__, "config.yaml")
config_ref = importlib.resources.files(__name__) / "config.yaml"
config_path = file_manager.enter_context(importlib.resources.as_file(config_ref))
config = yaml.safe_load(open(config_path, "r"))


Expand All @@ -38,7 +46,7 @@ def override_config(override_config_path):

def enhance_finding(finding):
if finding.issue not in config:
raise Exception("Uknown finding issue: {}".format(finding.issue))
raise Exception(f"Unknown finding issue: {finding.issue}")
config_settings = config[finding.issue]
finding.severity = config_settings["severity"]
finding.title = config_settings["title"]
Expand All @@ -58,7 +66,8 @@ def analyze_policy_string(
"""Given a string reperesenting a policy, convert it to a Policy object with findings"""

try:
# TODO Need to write my own json parser so I can track line numbers. See https://stackoverflow.com/questions/7225056/python-json-decoding-library-which-can-associate-decoded-items-with-original-li
# TODO Need to write my own json parser so I can track line numbers. See
# https://stackoverflow.com/questions/7225056/python-json-decoding-library-which-can-associate-decoded-items-with-original-li
policy_json = jsoncfg.loads_config(policy_str)
except jsoncfg.parser.JSONConfigParserException as e:
policy = Policy(None)
Expand Down Expand Up @@ -102,9 +111,9 @@ def is_arn_match(resource_type, arn_format, resource):
- resource: ARN regex from IAM policy


We can cheat some because after the first sections of the arn match, meaning until the 5th colon (with some
rules there to allow empty or asterisk sections), we only need to match the ID part.
So the above is simplified to "*/*" and "*personalize*".
We can cheat some because after the first sections of the arn match, meaning until the 5th
colon (with some rules there to allow empty or asterisk sections), we only need to match the
ID part. So the above is simplified to "*/*" and "*personalize*".

Let's look at some examples and if these should be marked as a match:
"*/*" and "*personalize*" -> True
Expand Down Expand Up @@ -132,7 +141,8 @@ def is_arn_match(resource_type, arn_format, resource):
raise Exception("Unexpected format for resource: {}".format(resource))

# For the first 5 parts (ex. arn:aws:SERVICE:REGION:ACCOUNT:), ensure these match appropriately
# We do this because we don't want "arn:*:s3:::*/*" and "arn:aws:logs:*:*:/aws/cloudfront/*" to return True
# We do this because we don't want "arn:*:s3:::*/*" and "arn:aws:logs:*:*:/aws/cloudfront/*"
# to return True
for position in range(0, 5):
if arn_parts[position] == "*" and resource_parts[position] != "":
continue
Expand All @@ -143,7 +153,8 @@ def is_arn_match(resource_type, arn_format, resource):
else:
return False

# Everything up to and including the account ID section matches, so now try to match the remainder
# Everything up to and including the account ID section matches,
# so now try to match the remainder
arn_id = ":".join(arn_parts[5:])
resource_id = ":".join(resource_parts[5:])

Expand All @@ -162,8 +173,9 @@ def is_arn_strictly_valid(resource_type, arn_format, resource):

That should return true because you could have "arn:aws:s3:::personalize/" which matches both.

However when not using *, must include the resource type in the resource arn and wildcards
are not valid for the resource type portion (https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#genref-aws-service-namesspaces)
However, when not using *, must include the resource type in the resource arn and wildcards
are not valid for the resource type portion
(https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#genref-aws-service-namesspaces)

Input:
- resource_type: Example "bucket", this is only used to identify special cases.
Expand All @@ -183,7 +195,7 @@ def is_arn_strictly_valid(resource_type, arn_format, resource):
# : or / and then anything else excluding the resource type string starting with a *
arn_id_resource_type = re.match(r"(^[^\*][\w-]+)[\/\:].+", arn_id)

if arn_id_resource_type != None and resource_id != "*":
if arn_id_resource_type is not None and resource_id != "*":

# https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#genref-aws-service-namesspaces
# The following is not allowed: arn:aws:iam::123456789012:u*
Expand All @@ -192,7 +204,7 @@ def is_arn_strictly_valid(resource_type, arn_format, resource):

# replace aws variable and check for other colons
resource_id_no_vars = strip_var_from_arn(resource_id)
if ":" in resource_id_no_vars and not ":" in arn_id:
if ":" in resource_id_no_vars and ":" not in arn_id:
return False

return True
Expand Down Expand Up @@ -260,9 +272,7 @@ def expand_action(action, raise_exceptions=True):
return []

for privilege in service["privileges"]:
if fnmatch.fnmatchcase(
privilege["privilege"].lower(), unexpanded_action.lower()
):
if fnmatch.fnmatchcase(privilege["privilege"].lower(), unexpanded_action.lower()):
actions.append(
{
"service": service_match["prefix"],
Expand All @@ -274,9 +284,7 @@ def expand_action(action, raise_exceptions=True):
raise UnknownPrefixException("Unknown prefix {}".format(prefix))

if len(actions) == 0 and raise_exceptions:
raise UnknownActionException(
"Unknown action {}:{}".format(prefix, unexpanded_action)
)
raise UnknownActionException("Unknown action {}:{}".format(prefix, unexpanded_action))

return actions

Expand Down Expand Up @@ -326,4 +334,4 @@ def get_privilege_matches_for_resource_type(resource_type_matches):


# Import moved here to deal with cyclic dependency
from .policy import Policy
from .policy import Policy # noqa: E402
38 changes: 14 additions & 24 deletions parliamentarian/cli.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
#!/usr/bin/env python3
import argparse
import json
import jsoncfg
import logging
import re
import sys
from os import walk
from os.path import abspath
from os.path import join
from os.path import abspath, join
from pathlib import Path

import jsoncfg

from parliamentarian import (
__version__,
analyze_policy_string,
config,
enhance_finding,
override_config,
config,
__version__,
)
from parliamentarian.misc import make_list

Expand All @@ -26,9 +26,7 @@ def is_finding_filtered(finding, minimum_severity="LOW"):
# Return True if the finding should be filtered (ie. return False if it should be displayed)
minimum_severity = minimum_severity.upper()
severity_choices = ["MUTE", "INFO", "LOW", "MEDIUM", "HIGH", "CRITICAL"]
if severity_choices.index(finding.severity) < severity_choices.index(
minimum_severity
):
if severity_choices.index(finding.severity) < severity_choices.index(minimum_severity):
return True

if finding.ignore_locations:
Expand Down Expand Up @@ -134,7 +132,8 @@ def main():
)
parser.add_argument(
"--string",
help='Provide a string such as \'{"Version": "2012-10-17","Statement": {"Effect": "Allow","Action": ["s3:GetObject", "s3:PutBucketPolicy"],"Resource": ["arn:aws:s3:::bucket1", "arn:aws:s3:::bucket2/*"]}}\'',
help='Provide a string such as \'{"Version": "2012-10-17","Statement": {"Effect": "Allow","Action": ['
'"s3:GetObject", "s3:PutBucketPolicy"],"Resource": ["arn:aws:s3:::bucket1", "arn:aws:s3:::bucket2/*"]}}\'',
type=str,
)
parser.add_argument(
Expand All @@ -157,12 +156,8 @@ def main():
help='File name regex pattern to exclude (ex. ".*venv.*")',
type=str,
)
parser.add_argument(
"--minimal", help="Minimal output", default=False, action="store_true"
)
parser.add_argument(
"--json", help="json output", default=False, action="store_true"
)
parser.add_argument("--minimal", help="Minimal output", default=False, action="store_true")
parser.add_argument("--json", help="json output", default=False, action="store_true")
parser.add_argument(
"--minimum_severity",
help="Minimum severity to display. Options: CRITICAL, HIGH, MEDIUM, LOW, INFO",
Expand All @@ -171,12 +166,11 @@ def main():
)
parser.add_argument(
"--private_auditors",
help="Directory of the private auditors. Defaults to looking in private_auditors in the same directory as the iam_definition.json file.",
help="Directory of the private auditors. Defaults to looking in private_auditors in the same directory as the "
"iam_definition.json file.",
default=None,
)
parser.add_argument(
"--config", help="Custom config file for over-riding values", type=str
)
parser.add_argument("--config", help="Custom config file for over-riding values", type=str)
parser.add_argument(
"--include-community-auditors",
help="Use this flag to enable community-provided auditors",
Expand Down Expand Up @@ -218,16 +212,12 @@ def main():
if not sys.stdin.isatty() and args.file.name != "<stdin>":
parser.error("You cannot pass a file with --file and use stdin together")

# Change the exit status if there are errors
exit_status = 0
findings = []

if args.include_community_auditors:
community_auditors_directory = "community_auditors"
community_auditors_override_file = (
Path(abspath(__file__)).parent
/ community_auditors_directory
/ "config_override.yaml"
Path(abspath(__file__)).parent / community_auditors_directory / "config_override.yaml"
)
override_config(community_auditors_override_file)
override_config(args.config)
Expand Down
20 changes: 9 additions & 11 deletions parliamentarian/community_auditors/advanced_policy_elements.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
"""
"""
For AWS resource policies, check whether they use discouraged constructions.
See the [AWS Policy Troubleshooting Guide](https://docs.aws.amazon.com/IAM/latest/UserGuide/troubleshoot_policies.html).

AWS documentation discourages the use of NotPrincipal, NotAction and
NotResource, particularly with Allow. These constructs, by default, grant
permissions, then Deny the ones explicitly listed. Instead, use an explicit
NotResource, particularly with Allow. These constructs, by default, grant
permissions, then Deny the ones explicitly listed. Instead, use an explicit
Resource, Action or Principal in your Allow list.
"""

Expand All @@ -16,26 +16,24 @@


def get_stmts(policy: Policy) -> Iterable:
if "jsoncfg.config_classes.ConfigJSONObject" in str(
type(policy.policy_json.Statement)
):
if "jsoncfg.config_classes.ConfigJSONObject" in str(type(policy.policy_json.Statement)):
return [policy.policy_json.Statement]
elif "jsoncfg.config_classes.ConfigJSONArray" in str(
type(policy.policy_json.Statement)
):
elif "jsoncfg.config_classes.ConfigJSONArray" in str(type(policy.policy_json.Statement)):
return policy.policy_json.Statement

return []


def audit(policy: Policy) -> None:
for stmt in get_stmts(policy):
if stmt.Effect.value == "Allow" and jsoncfg.node_exists(stmt["NotPrincipal"]):
# See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_notprincipal.html#specifying-notprincipal-allow
# https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_notprincipal.html#specifying-notprincipal-allow
policy.add_finding(
"NOTPRINCIPAL_WITH_ALLOW",
location=("NotPrincipal", stmt["NotPrincipal"]),
)
elif stmt.Effect.value == "Allow" and jsoncfg.node_exists(stmt["NotResource"]):
# See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_notresource.html#notresource-element-combinations
# https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_notresource.html#notresource-element-combinations
policy.add_finding(
"NOTRESOURCE_WITH_ALLOW",
location=("NotResource", stmt["NotResource"]),
Expand Down
6 changes: 2 additions & 4 deletions parliamentarian/community_auditors/sensitive_access.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from collections import defaultdict

from parliamentarian import is_arn_match, expand_action
from parliamentarian import expand_action, is_arn_match


def _expand_action(operation):
Expand Down Expand Up @@ -29,9 +29,7 @@ def audit(policy):
for action in allowed_actions:
expanded_action = _expand_action(action)
service, operation = expanded_action.split(":")
action_resources[expanded_action] = policy.get_allowed_resources(
service, operation
)
action_resources[expanded_action] = policy.get_allowed_resources(service, operation)

for action in action_resources:
for action_resource in action_resources[action]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
designed for multi-value condition keys results in "overly permissive policies"
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_single-vs-multi-valued-condition-keys.html
"""

import re

from parliamentarian import Policy
from parliamentarian.misc import make_list


def audit(policy: Policy) -> None:
Expand Down Expand Up @@ -60,5 +61,6 @@ def audit(policy: Policy) -> None:
):
policy.add_finding(
"SINGLE_VALUE_CONDITION_TOO_PERMISSIVE",
detail="Checking a single value conditional key against a set of values results in overly permissive policies.",
detail="Checking a single value conditional key against a set of values results in overly "
"permissive policies.",
)
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ def test_credentials_management(self):
]
}
"""
policy = analyze_policy_string(
example_policy_string, include_community_auditors=True
)
policy = analyze_policy_string(example_policy_string, include_community_auditors=True)

assert_equal(
policy.finding_ids,
Expand Down
Loading