From 22f6d6fcfbd3803a8448d05c129462db7f8fee5e Mon Sep 17 00:00:00 2001 From: Brian Becker Date: Mon, 15 Mar 2021 09:44:23 -0400 Subject: [PATCH] Add stricter checking to resource attributes (#177) * Add stricter checking to resource attributes * Certain resource strings were incorrectly accepted as valid * Adds is_arn_strictly_valid function to do tighter checks on resources against their documented requirements * Appears to fix #167 * Delete erroneous file * Remove extra spacing * Resource types can have hyphens --- parliament/__init__.py | 46 +++++++++++++++++++++++++ parliament/statement.py | 3 +- tests/unit/test_resource_formatting.py | 47 +++++++++++++++++++++++++- 3 files changed, 94 insertions(+), 2 deletions(-) diff --git a/parliament/__init__.py b/parliament/__init__.py index 324e528..91dd4b3 100644 --- a/parliament/__init__.py +++ b/parliament/__init__.py @@ -147,6 +147,52 @@ def is_arn_match(resource_type, arn_format, resource): return is_glob_match(arn_id, resource_id) +def is_arn_strictly_valid(resource_type, arn_format, resource): + """ + Strictly validate the arn_format specified in the docs, with the resource + given in the IAM policy. These can each be strings with globbing. For example, we + want to match the following two strings: + - arn:*:s3:::*/* + - arn:aws:s3:::*personalize* + + 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) + + Input: + - resource_type: Example "bucket", this is only used to identify special cases. + - arn_format: ARN regex from the docs + - resource: ARN regex from IAM policy + + """ + + if is_arn_match(resource_type, arn_format, resource): + # this would have already raised exception + arn_parts = arn_format.split(":") + resource_parts = resource.split(":") + arn_id = ":".join(arn_parts[5:]) + resource_id = ":".join(resource_parts[5:]) + + # Does the resource contain a resource type component + # regex looks for a resource type word like "user" or "cluster-endpoint" followed by a + # : 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 != "*": + + # 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* + if not (resource_id.startswith(arn_id_resource_type[1])): + return False + + # replace aws variable and check for other colons + resource_id_no_vars = re.sub(r"\$\{aws.\w+\}", "", resource_id) + if ":" in resource_id_no_vars and not ":" in arn_id: + return False + + return True + return False def is_glob_match(s1, s2): # This comes from https://github.com/duo-labs/parliament/issues/36#issuecomment-574001764 diff --git a/parliament/statement.py b/parliament/statement.py index 56ca84a..fc7db73 100644 --- a/parliament/statement.py +++ b/parliament/statement.py @@ -4,6 +4,7 @@ from . import ( iam_definition, is_arn_match, + is_arn_strictly_valid, expand_action, UnknownActionException, UnknownPrefixException, @@ -921,7 +922,7 @@ def analyze_statement(self): self.resource_star[action_key] += 1 match_found = True continue - if is_arn_match(resource_type, arn_format, resource.value): + if is_arn_strictly_valid(resource_type, arn_format, resource.value): match_found = True continue diff --git a/tests/unit/test_resource_formatting.py b/tests/unit/test_resource_formatting.py index 2388faa..28a409c 100644 --- a/tests/unit/test_resource_formatting.py +++ b/tests/unit/test_resource_formatting.py @@ -2,7 +2,7 @@ from nose.tools import raises, assert_equal, assert_true, assert_false # import parliament -from parliament import analyze_policy_string, is_arn_match, is_glob_match +from parliament import analyze_policy_string, is_arn_match, is_arn_strictly_valid, is_glob_match from parliament.statement import is_valid_region, is_valid_account_id @@ -91,6 +91,51 @@ def test_arn_match(self): ) ) + def test_is_arn_strictly_valid(self): + assert_true( + is_arn_strictly_valid( + "user", "arn:*:iam::*:user/*", "arn:aws:iam::123456789012:user/Development/product_1234/*" + ) + ) + + assert_true( + is_arn_strictly_valid( + "user", "arn:*:iam::*:user/*", "arn:aws:iam::123456789012:*" + ) + ) + + assert_true( + is_arn_strictly_valid( + "ssm", "arn:*:ssm::*:resource-data-sync/*", "arn:aws:ssm::123456789012:resource-data-sync/*" + ) + ) + + assert_false( + is_arn_strictly_valid( + "ssm", "arn:*:ssm::*:resource-data-sync/*", "arn:aws:ssm::123456789012:resource-data-*/*" + ) + ) + + assert_false( + is_arn_strictly_valid( + "user", "arn:*:iam::*:user/*", "arn:aws:iam::123456789012:*/*" + ) + ) + + assert_false( + is_arn_strictly_valid( + "user", "arn:*:iam::*:user/*", "arn:aws:iam::123456789012:u*" + ) + ) + + assert_false( + is_arn_strictly_valid( + "dbuser", "arn:*:redshift:*:*:dbuser:*/*", "arn:aws:redshift:us-west-2:123456789012:db*:the_cluster/the_user" + ) + ) + + + def test_arn_match_cloudtrail_emptysegments(self): assert_false( is_arn_match(