Skip to content

Commit

Permalink
Removes variables from arn when checking buckets (duo-labs#185)
Browse files Browse the repository at this point in the history
* When checking a bucket arn the presence of a / was flagged as not a
  match.  Because bucket resource arns can contain variables which in
  turn can contain / (e.g. ["arn:aws:s3:::bucket/${aws:PrincipalTag/department}"])
  as seen https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_variables.html#policy-vars-wheretouse
  this is a false flag.
* This change removes the variables from the arn before checking the
  presence of a /
* Also updates variable removal code from is_arn_strictly_valid which
  did not account for a /
* Fixes duo-labs#146
  • Loading branch information
briandbecker authored Mar 22, 2021
1 parent 58b59d4 commit 3873cf5
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 4 deletions.
10 changes: 6 additions & 4 deletions parliament/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ def is_arn_match(resource_type, arn_format, resource):

if "bucket" in resource_type:
# We have to do a special case here for S3 buckets
if "/" in resource:
# and since resources can use variables which contain / need to replace them
if "/" in strip_var_from_arn(resource, "theVar"):
return False

# The ARN has at least 6 parts, separated by a colon. Ensure these exist.
Expand Down Expand Up @@ -144,7 +145,6 @@ def is_arn_match(resource_type, arn_format, resource):

# Some of the arn_id's contain regexes of the form "[key]" so replace those with "*"
resource_id = re.sub(r"\[.+?\]", "*", resource_id)

return is_glob_match(arn_id, resource_id)

def is_arn_strictly_valid(resource_type, arn_format, resource):
Expand All @@ -166,7 +166,6 @@ def is_arn_strictly_valid(resource_type, arn_format, resource):
- 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(":")
Expand All @@ -187,13 +186,16 @@ def is_arn_strictly_valid(resource_type, arn_format, resource):
return False

# replace aws variable and check for other colons
resource_id_no_vars = re.sub(r"\$\{aws.\w+\}", "", resource_id)
resource_id_no_vars = strip_var_from_arn(resource_id)
if ":" in resource_id_no_vars and not ":" in arn_id:
return False

return True
return False

def strip_var_from_arn(arn, replace_with=""):
return re.sub(r"\$\{aws.[\w\/]+\}", replace_with, arn)

def is_glob_match(s1, s2):
# This comes from https://github.com/duo-labs/parliament/issues/36#issuecomment-574001764

Expand Down
5 changes: 5 additions & 0 deletions tests/unit/test_resource_formatting.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ def test_arn_match(self):
"arn:aws:logs:us-east-1:000000000000:/aws/cloudfront/test"
)
)
assert_true(
is_arn_match(
"bucket", "arn:*:s3:::*", "arn:aws:s3:::bucket-for-client-${aws:PrincipalTag/Namespace}-*"
)
)

def test_is_arn_strictly_valid(self):
assert_true(
Expand Down

0 comments on commit 3873cf5

Please sign in to comment.