From 3873cf55a2c42a32d03cc747e949fe935891b0f2 Mon Sep 17 00:00:00 2001
From: Brian Becker <briandbecker@users.noreply.github.com>
Date: Mon, 22 Mar 2021 10:05:03 -0400
Subject: [PATCH] Removes variables from arn when checking buckets (#185)

* 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 #146
---
 parliament/__init__.py                 | 10 ++++++----
 tests/unit/test_resource_formatting.py |  5 +++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/parliament/__init__.py b/parliament/__init__.py
index 91dd4b3..1596023 100644
--- a/parliament/__init__.py
+++ b/parliament/__init__.py
@@ -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.
@@ -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):
@@ -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(":")
@@ -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
 
diff --git a/tests/unit/test_resource_formatting.py b/tests/unit/test_resource_formatting.py
index 28a409c..0b9e7a6 100644
--- a/tests/unit/test_resource_formatting.py
+++ b/tests/unit/test_resource_formatting.py
@@ -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(