PartialWildcardRule to detect usage of just account IDs in the princi…

…pal (#194) * Update partial wildcard principal rule to detect usage of just the account ID being specified * Update CHANGELOG Co-authored-by: Oliver Crawford <oliver.crawford@skyscanner.net>
Skyscanner · Sep 30, 2021 · 83ca480 · 83ca480
1 parent 7448e8c
commit 83ca480
Show file tree

Hide file tree

Showing 8 changed files with 53 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,13 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
-## [1.1.0] - 2021-09-XX
+## [1.1.1] - 2021-09-30
+### Fixes
+- Add a fix to the `PartialWildcardPrincipal` rule to be able to detect policies where whole account access is specified via just the account ID.
+- For example, if the Principal was defined as `Principal: AWS: 123456789012` as opposed to `Principal: AWS: arn:aws:iam::123456789012:root`.
+  - These are identical: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html
+
+## [1.1.0] - 2021-09-22
 ### Improvements
 - Add `S3ObjectVersioning` rule
 - Update `pycfmodel` to `0.11.0`

diff --git a/cfripper/__version__.py b/cfripper/__version__.py
@@ -1,3 +1,3 @@
-VERSION = (1, 1, 0)
+VERSION = (1, 1, 1)
 
 __version__ = ".".join(map(str, VERSION))
diff --git a/cfripper/config/regex.py b/cfripper/config/regex.py
@@ -24,6 +24,20 @@
 """
 REGEX_FULL_WILDCARD_PRINCIPAL = re.compile(r"^((\w*:){0,1}\*|arn:aws:iam::\*:.*)$")
 
+"""
+Check for use of wildcard or account-wide principals.
+Valid:
+- arn:aws:iam::123456789012:*
+- arn:aws:iam::123456789012:service-*
+- arn:aws:iam::123456789012:root
+- 123456789012
+Invalid:
+- *
+- potato
+- arn:aws:iam::123456789012:*not-root
+"""
+REGEX_PARTIAL_WILDCARD_PRINCIPAL = re.compile(r"^\d{12}|arn:aws:iam::.*:(.*\*|root)$")
+
 """
 Check for use of wildcard, when applied to the specific elements of an Action.
 For example, sts:AssumeRole* or sts:*. This regex is not checking for use of `*` on its own.

diff --git a/cfripper/rules/wildcard_principals.py b/cfripper/rules/wildcard_principals.py
@@ -13,7 +13,7 @@
 from pycfmodel.model.resources.sns_topic_policy import SNSTopicPolicy
 from pycfmodel.model.resources.sqs_queue_policy import SQSQueuePolicy
 
-from cfripper.config.regex import REGEX_FULL_WILDCARD_PRINCIPAL
+from cfripper.config.regex import REGEX_FULL_WILDCARD_PRINCIPAL, REGEX_PARTIAL_WILDCARD_PRINCIPAL
 from cfripper.model.enums import RuleGranularity, RuleRisk
 from cfripper.model.result import Result
 from cfripper.rules.base_rules import PrincipalCheckingRule
@@ -22,11 +22,12 @@
 
 
 class GenericWildcardPrincipalRule(PrincipalCheckingRule):
-    REASON_WILCARD_PRINCIPAL = "{} should not allow wildcard in principals or account-wide principals (principal: '{}')"
+    REASON_WILDCARD_PRINCIPAL = "{} should not allow wildcards in principals (principal: '{}')"
     GRANULARITY = RuleGranularity.RESOURCE
 
+    AWS_ACCOUNT_ID_PATTERN = re.compile(r"^(\d{12})$")
     IAM_PATTERN = re.compile(r"arn:aws:iam::(\d*|\*):.*")
-    FULL_REGEX = re.compile(r"^((\w*:){0,1}\*|arn:aws:iam::(\d*|\*):.*)$")
+    FULL_REGEX = REGEX_FULL_WILDCARD_PRINCIPAL
 
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
@@ -47,7 +48,7 @@ def check_for_wildcards(
         for statement in resource._statement_as_list():
             if statement.Effect == "Allow" and statement.principals_with(self.FULL_REGEX):
                 for principal in statement.get_principal_list():
-                    account_id_match = self.IAM_PATTERN.match(principal)
+                    account_id_match = self.IAM_PATTERN.match(principal) or self.AWS_ACCOUNT_ID_PATTERN.match(principal)
                     account_id = account_id_match.group(1) if account_id_match else None
 
                     # Check if account ID is allowed. `self._get_allowed_from_config()` used here
@@ -64,7 +65,7 @@ def check_for_wildcards(
                     else:
                         self.add_failure_to_result(
                             result,
-                            self.REASON_WILCARD_PRINCIPAL.format(logical_id, principal),
+                            self.REASON_WILDCARD_PRINCIPAL.format(logical_id, principal),
                             resource_ids={logical_id},
                             context={
                                 "config": self._config,
@@ -103,16 +104,11 @@ class PartialWildcardPrincipalRule(GenericWildcardPrincipalRule):
         |`account_id` | str                | Account ID found in the principal                              |
     """
 
-    REASON_WILCARD_PRINCIPAL = "{} should not allow wildcard in principals or account-wide principals (principal: '{}')"
-
+    REASON_WILDCARD_PRINCIPAL = (
+        "{} should not allow wildcard in principals or account-wide principals (principal: '{}')"
+    )
     RISK_VALUE = RuleRisk.MEDIUM
-    """
-    Will catch:
-
-    - Principal: arn:aws:iam:12345:12345*
-
-    """
-    FULL_REGEX = re.compile(r"^arn:aws:iam::.*:(.*\*|root)$")
+    FULL_REGEX = REGEX_PARTIAL_WILDCARD_PRINCIPAL
 
 
 class FullWildcardPrincipalRule(GenericWildcardPrincipalRule):
@@ -138,8 +134,4 @@ class FullWildcardPrincipalRule(GenericWildcardPrincipalRule):
         |`account_id` | str                | Account ID found in the principal                              |
     """
 
-    REASON_WILCARD_PRINCIPAL = "{} should not allow wildcards in principals (principal: '{}')"
-
     RISK_VALUE = RuleRisk.HIGH
-
-    FULL_REGEX = REGEX_FULL_WILDCARD_PRINCIPAL
diff --git a/tests/config/test_regex.py b/tests/config/test_regex.py
@@ -8,6 +8,7 @@
     REGEX_HAS_STAR_OR_STAR_AFTER_COLON,
     REGEX_IAM_ARN,
     REGEX_IS_STAR,
+    REGEX_PARTIAL_WILDCARD_PRINCIPAL,
     REGEX_STS_ARN,
     REGEX_WILDCARD_ARN,
     REGEX_WILDCARD_POLICY_ACTION,
@@ -69,6 +70,13 @@
         (REGEX_HAS_STAR_OR_STAR_AFTER_COLON, "arn:aws:s3:::*my_corporate_bucket", False),
         (REGEX_HAS_STAR_OR_STAR_AFTER_COLON, "potato", False),
         (REGEX_HAS_STAR_OR_STAR_AFTER_COLON, "sns:Get*", False),
+        (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "arn:aws:iam::123456789012:*", True),
+        (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "arn:aws:iam::123456789012:service-*", True),
+        (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "arn:aws:iam::123456789012:root", True),
+        (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "123456789012", True),
+        (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "*", False),
+        (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "potato", False),
+        (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "arn:aws:iam::123456789012:*not-root", False),
     ],
 )
 def test_regex_cross_account_root(regex, data, valid):

diff --git a/tests/rules/test_GenericWildcardPrincipal.py b/tests/rules/test_GenericWildcardPrincipal.py
@@ -35,7 +35,7 @@ def test_failures_are_raised(bad_template):
         [
             Failure(
                 granularity=RuleGranularity.RESOURCE,
-                reason="PolicyA should not allow wildcard in principals or account-wide principals (principal: 'somewhatrestricted:*')",
+                reason="PolicyA should not allow wildcards in principals (principal: 'somewhatrestricted:*')",
                 risk_value=RuleRisk.MEDIUM,
                 rule="GenericWildcardPrincipalRule",
                 rule_mode=RuleMode.BLOCKING,
@@ -44,7 +44,7 @@ def test_failures_are_raised(bad_template):
             ),
             Failure(
                 granularity=RuleGranularity.RESOURCE,
-                reason="PolicyA should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123445:*')",
+                reason="PolicyA should not allow wildcards in principals (principal: 'arn:aws:iam::123445:*')",
                 risk_value=RuleRisk.MEDIUM,
                 rule="GenericWildcardPrincipalRule",
                 rule_mode=RuleMode.BLOCKING,

diff --git a/tests/rules/test_PartialWildcardPrincipal.py b/tests/rules/test_PartialWildcardPrincipal.py
@@ -83,7 +83,16 @@ def test_failures_for_correct_account_ids(intra_account_root_access):
                 rule_mode=RuleMode.BLOCKING,
                 actions=None,
                 resource_ids={"AccLoadBalancerAccessLogBucketPolicy"},
-            )
+            ),
+            Failure(
+                granularity=RuleGranularity.RESOURCE,
+                reason="AccLoadBalancerAccessLogBucketPolicy should not allow wildcard in principals or account-wide principals (principal: '987654321012')",
+                risk_value=RuleRisk.MEDIUM,
+                rule="PartialWildcardPrincipalRule",
+                rule_mode=RuleMode.BLOCKING,
+                actions=None,
+                resource_ids={"AccLoadBalancerAccessLogBucketPolicy"},
+            ),
         ],
     )
 

diff --git a/tests/test_templates/rules/PartialWildcardPrincipalRule/intra_account_root_access.yml b/tests/test_templates/rules/PartialWildcardPrincipalRule/intra_account_root_access.yml
@@ -13,6 +13,7 @@ Resources:
             Principal:
               AWS:
                 - "arn:aws:iam::123456789012:root"
+                - "987654321012"
             Action: s3:PutObject
             Resource: !Sub
               - "arn:aws:s3:::${BucketName}/*"