Fix wildcard rules (#229)

* fix GenericWildcardPrincipalRule and refactor kms_key_wildcard_principal

* fix tests

* update GenericWildcardPrincipalRule message

* update PartialWildcardPrincipalRule message

* update version, changelog and docs

* Fix  to crrectly handle canonical IDs and account IDs

* fix nit

CHANGELOG.md

@@ -1,6 +1,18 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [1.12.0]
+### Improvements
+- Refactored the `KMSKeyWildcardPrincipalRule` rule
+### Updates
+- Update `GenericWildcardPrincipalRule`, `PartialWildcardPrincipalRule` and `GenericResourcePartialWildcardPrincipalRule` message
+- Update docs
+### Fixes
+- Fix `GenericWildcardPrincipalRule` that could add a false-positive
+- Fix `GenericWildcardPrincipalRule` that wasn't handling canonical IDs
+- Fix `REGEX_PARTIAL_WILDCARD_PRINCIPAL` to correctly handle canonical IDs and account IDs
+- Fix unit tests
+
 ## [1.11.0]
 ### Additions
 - New regex `REGEX_CONTAINS_WILDCARD` to check for any wildcard

README.md

@@ -23,7 +23,7 @@ Analysing /tmp/root.yaml...
 Not adding CrossAccountTrustRule failure in rootRole because no AWS Account ID was found in the config.
 Valid: False
 Issues found:
- - FullWildcardPrincipalRule: rootRole should not allow wildcards in principals (principal: '*')
+ - FullWildcardPrincipalRule: rootRole should not allow full wildcard '*', or wildcard in account ID like 'arn:aws:iam::*:12345' at '*'
  - IAMRolesOverprivilegedRule: Role 'rootRole' contains an insecure permission '*' in policy 'root'
 Analysing /tmp/root_bypass.json...
 Valid: True
@@ -37,7 +37,7 @@ Analysing /tmp/root.yaml...
 Not adding CrossAccountTrustRule failure in rootRole because no AWS Account ID was found in the config.
 Valid: False
 Issues found:
- - FullWildcardPrincipalRule: rootRole should not allow wildcards in principals (principal: '*')
+ - FullWildcardPrincipalRule: rootRole should not allow full wildcard '*', or wildcard in account ID like 'arn:aws:iam::*:12345' at '*'
  - IAMRolesOverprivilegedRule: Role 'rootRole' contains an insecure permission '*' in policy 'root'
 Analysing /tmp/root_bypass.json...
 Not adding CrossAccountTrustRule failure in rootRole because no AWS Account ID was found in the config.
@@ -46,8 +46,7 @@ Issues found:
  - IAMRolesOverprivilegedRule: Role 'rootRole' contains an insecure permission '*' in policy 'root'
 Monitored issues found:
  - PartialWildcardPrincipalRule: rootRole contains an unknown principal: 123456789012
- - PartialWildcardPrincipalRule: rootRole should not allow wildcard in principals or account-wide principals
-(principal: 'arn:aws:iam::123456789012:root')
+ - PartialWildcardPrincipalRule: rootRole should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at 'arn:aws:iam::123456789012:root'
 ```
 
 ### Using json format and output-folder argument

cfripper/__version__.py

@@ -1,3 +1,3 @@
-VERSION = (1, 11, 0)
+VERSION = (1, 12, 0)
 
 __version__ = ".".join(map(str, VERSION))

cfripper/config/regex.py

@@ -33,12 +33,16 @@
 - arn:aws:iam::123456789012:root
 - arn:aws:iam::123456789012:*-role
 - 123456789012
+- 79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be
+- eb2fe74dc7e8125d8f8fcae89d90e6dfdecabf896e1a69d55e949b009fd95a97
 Invalid:
 - *
 - potato
 - arn:aws:iam::123456789012:not-root
 """
-REGEX_PARTIAL_WILDCARD_PRINCIPAL = re.compile(r"^(\d+)|(arn:aws:iam::(\d+):(.*[*?]+|[*?]+.*|root))$")
+REGEX_PARTIAL_WILDCARD_PRINCIPAL = re.compile(
+    r"^(\d{12})|([a-fA-F0-9]{64})|(arn:aws:iam::(\d+):(.*[*?]+|[*?]+.*|root))$"
+)
 
 """
 Check for use of wildcard, when applied to the specific elements of an Action.

cfripper/rules/kms_key_wildcard_principal.py

@@ -38,25 +38,25 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         for logical_id, resource in cfmodel.Resources.items():
             if isinstance(resource, KMSKey):
                 for statement in resource.Properties.KeyPolicy._statement_as_list():
-                    if statement.Effect == "Allow" and statement.principals_with(self.CONTAINS_WILDCARD_PATTERN):
-                        for principal in statement.get_principal_list():
-                            if self.CONTAINS_WILDCARD_PATTERN.match(principal):
-                                if statement.Condition and statement.Condition.dict():
-                                    # Ignoring condition checks since they will get reviewed in other
-                                    # rules and future improvements
-                                    pass
-                                else:
-                                    self.add_failure_to_result(
-                                        result,
-                                        self.REASON.format(logical_id),
-                                        resource_ids={logical_id},
-                                        context={
-                                            "config": self._config,
-                                            "extras": extras,
-                                            "logical_id": logical_id,
-                                            "resource": resource,
-                                            "statement": statement,
-                                            "principal": principal,
-                                        },
-                                    )
+                    filtered_principals = statement.principals_with(self.CONTAINS_WILDCARD_PATTERN)
+                    if statement.Effect == "Allow" and filtered_principals:
+                        for principal in filtered_principals:
+                            if statement.Condition and statement.Condition.dict():
+                                # Ignoring condition checks since they will get reviewed in other
+                                # rules and future improvements
+                                pass
+                            else:
+                                self.add_failure_to_result(
+                                    result,
+                                    self.REASON.format(logical_id),
+                                    resource_ids={logical_id},
+                                    context={
+                                        "config": self._config,
+                                        "extras": extras,
+                                        "logical_id": logical_id,
+                                        "resource": resource,
+                                        "statement": statement,
+                                        "principal": principal,
+                                    },
+                                )
         return result

cfripper/rules/wildcard_principals.py

@@ -7,7 +7,6 @@
     "GenericResourceFullWildcardPrincipalRule",
 ]
 import logging
-import re
 from typing import Dict, Optional
 
 from pycfmodel.model.cf_model import CFModel
@@ -24,6 +23,7 @@
 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.model.utils import get_account_id_from_principal
 from cfripper.rules.base_rules import PrincipalCheckingRule
 
 logger = logging.getLogger(__file__)
@@ -56,11 +56,11 @@ class GenericWildcardPrincipalRule(PrincipalCheckingRule):
         |`account_id` | `str`              | Account ID found in the principal                              |
     """
 
-    REASON_WILDCARD_PRINCIPAL = "{} should not allow wildcards in principals (principal: '{}')"
+    REASON_WILDCARD_PRINCIPAL = (
+        "{} should not allow full wildcard '*', or wildcard in account ID like 'arn:aws:iam::*:12345' at '{}'"
+    )
     GRANULARITY = RuleGranularity.RESOURCE
 
-    AWS_ACCOUNT_ID_PATTERN = re.compile(r"^(\d{12})$")
-    IAM_PATTERN = re.compile(r"arn:aws:iam::(\d*|\*):.*")
     FULL_REGEX = REGEX_FULL_WILDCARD_PRINCIPAL
 
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
@@ -104,15 +104,16 @@ def check_for_wildcards(
         extras: Optional[Dict] = None,
     ):
         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) or self.AWS_ACCOUNT_ID_PATTERN.match(principal)
-                    account_id = account_id_match.group(1) if account_id_match else None
+            filtered_principals = statement.principals_with(self.FULL_REGEX)
+            if statement.Effect == "Allow" and filtered_principals:
+                for principal in filtered_principals:
+                    # if we can't find the account ID it might be a canonical ID
+                    identifier = get_account_id_from_principal(principal) or principal
 
-                    # Check if account ID is allowed. `self._get_allowed_from_config()` used here
+                    # Check if account ID / canonical ID is allowed. `self._get_allowed_from_config()` used here
                     # to reduce number of false negatives and only allow exemptions for accounts
                     # which belong to AWS Services (such as ELB and ElastiCache).
-                    if account_id in self._get_allowed_from_config():
+                    if identifier in self._get_allowed_from_config():
                         continue
                     if statement.Condition and statement.Condition.dict():
                         # Ignoring condition checks since they will get reviewed in other rules and future improvements
@@ -130,7 +131,7 @@ def check_for_wildcards(
                                 "resource": resource,
                                 "statement": statement,
                                 "principal": principal,
-                                "account_id": account_id,
+                                "account_id": identifier,
                             },
                         )
 
@@ -161,7 +162,7 @@ class PartialWildcardPrincipalRule(GenericWildcardPrincipalRule):
     """
 
     REASON_WILDCARD_PRINCIPAL = (
-        "{} should not allow wildcard in principals or account-wide principals (principal: '{}')"
+        "{} should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at '{}'"
     )
     RISK_VALUE = RuleRisk.MEDIUM
     FULL_REGEX = REGEX_PARTIAL_WILDCARD_PRINCIPAL
@@ -272,7 +273,7 @@ class GenericResourcePartialWildcardPrincipalRule(GenericResourceWildcardPrincip
     """
 
     REASON_WILDCARD_PRINCIPAL = (
-        "{} should not allow wildcard in principals or account-wide principals (principal: '{}')"
+        "{} should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at '{}'"
     )
     RISK_VALUE = RuleRisk.MEDIUM
     FULL_REGEX = REGEX_PARTIAL_WILDCARD_PRINCIPAL

docs/cli.md

@@ -13,7 +13,7 @@ Analysing /tmp/root.yaml...
 Not adding CrossAccountTrustRule failure in rootRole because no AWS Account ID was found in the config.
 Valid: False
 Issues found:
-	- FullWildcardPrincipalRule: rootRole should not allow wildcards in principals (principal: '*')
+	- FullWildcardPrincipalRule: rootRole should not allow full wildcard '*', or wildcard in account ID like 'arn:aws:iam::*:12345' at '*'
 	- IAMRolesOverprivilegedRule: Role 'rootRole' contains an insecure permission '*' in policy 'root'
 Analysing /tmp/root_bypass.json...
 Valid: True
@@ -26,7 +26,7 @@ Analysing /tmp/root.yaml...
 Not adding CrossAccountTrustRule failure in rootRole because no AWS Account ID was found in the config.
 Valid: False
 Issues found:
-	- FullWildcardPrincipalRule: rootRole should not allow wildcards in principals (principal: '*')
+	- FullWildcardPrincipalRule: rootRole should not allow full wildcard '*', or wildcard in account ID like 'arn:aws:iam::*:12345' at '*'
 	- IAMRolesOverprivilegedRule: Role 'rootRole' contains an insecure permission '*' in policy 'root'
 Analysing /tmp/root_bypass.json...
 Not adding CrossAccountTrustRule failure in rootRole because no AWS Account ID was found in the config.
@@ -68,8 +68,8 @@ Not adding S3CrossAccountTrustRule failure in S3BucketPolicyAccountAccess becaus
 Not adding S3CrossAccountTrustRule failure in S3BucketPolicyAccountAccess because no AWS Account ID was found in the config.
 Valid: False
 Issues found:
-	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role')
-	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::666555444333:root')
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role'
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at 'arn:aws:iam::666555444333:root'
 	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
 	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
 	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
@@ -90,8 +90,8 @@ Analysing ./tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_a
 Using `UNDEFINED_PARAM_S3Bucket` for S3Bucket. Original value wasn't available.
 Valid: False
 Issues found:
-	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role')
-	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::666555444333:root')
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role'
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at 'arn:aws:iam::666555444333:root'
 	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 666555444333
 	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 666555444333
 	- S3BucketPublicReadAclAndListStatementRule: S3 Bucket S3BucketPolicyAccountAccess should not have a public read acl and list bucket statement
@@ -105,8 +105,8 @@ Analysing ./tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_a
 Using `UNDEFINED_PARAM_S3Bucket` for S3Bucket. Original value wasn't available.
 Valid: False
 Issues found:
-	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role')
-	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::666555444333:root')
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role'
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at 'arn:aws:iam::666555444333:root'
 	- S3BucketPublicReadAclAndListStatementRule: S3 Bucket S3BucketPolicyAccountAccess should not have a public read acl and list bucket statement
 	- S3CrossAccountTrustRule: S3BucketPolicyAccountAccess has forbidden cross-account policy allow with arn:aws:iam::666555444333:root for an S3 bucket.
 ```

tests/config/test_config.py

@@ -94,47 +94,7 @@ def test_load_filters_work_with_several_rules(template_two_roles_dict, test_file
             ),
             Failure(
                 granularity=RuleGranularity.RESOURCE,
-                reason="RootRoleTwo should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789:user/someuser@bla.com')",
-                risk_value=RuleRisk.MEDIUM,
-                rule="PartialWildcardPrincipalRule",
-                rule_mode=RuleMode.BLOCKING,
-                actions=None,
-                resource_ids={"RootRoleTwo"},
-                resource_types={"AWS::IAM::Role"},
-            ),
-            Failure(
-                granularity=RuleGranularity.RESOURCE,
-                reason="RootRoleTwo should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789:user/someuser@bla.com')",
-                risk_value=RuleRisk.MEDIUM,
-                rule="PartialWildcardPrincipalRule",
-                rule_mode=RuleMode.BLOCKING,
-                actions=None,
-                resource_ids={"RootRoleTwo"},
-                resource_types={"AWS::IAM::Role"},
-            ),
-            Failure(
-                granularity=RuleGranularity.RESOURCE,
-                reason="RootRoleTwo should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789:root')",
-                risk_value=RuleRisk.MEDIUM,
-                rule="PartialWildcardPrincipalRule",
-                rule_mode=RuleMode.BLOCKING,
-                actions=None,
-                resource_ids={"RootRoleTwo"},
-                resource_types={"AWS::IAM::Role"},
-            ),
-            Failure(
-                granularity=RuleGranularity.RESOURCE,
-                reason="RootRoleTwo should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::999999999:role/someuser@bla.com')",
-                risk_value=RuleRisk.MEDIUM,
-                rule="PartialWildcardPrincipalRule",
-                rule_mode=RuleMode.BLOCKING,
-                actions=None,
-                resource_ids={"RootRoleTwo"},
-                resource_types={"AWS::IAM::Role"},
-            ),
-            Failure(
-                granularity=RuleGranularity.RESOURCE,
-                reason="RootRoleTwo should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789:user/someuser@bla.com')",
+                reason="RootRoleTwo should not allow wildcard, account-wide or root in resource-id like 'arn:aws:iam::12345:root' at 'arn:aws:iam::123456789:root'",
                 risk_value=RuleRisk.MEDIUM,
                 rule="PartialWildcardPrincipalRule",
                 rule_mode=RuleMode.BLOCKING,

tests/config/test_regex.py

@@ -92,6 +92,8 @@
         (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, "79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be", True),
+        (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "eb2fe74dc7e8125d8f8fcae89d90e6dfdecabf896e1a69d55e949b009fd95a97", True),
         (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "*", False),
         (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "potato", False),
         (REGEX_PARTIAL_WILDCARD_PRINCIPAL, "arn:aws:iam::123456789012:*-role", True),

tests/rules/test_FullWildcardPrincipal.py

@@ -35,7 +35,7 @@ def test_failures_are_raised(bad_template):
             Failure(
                 rule_mode=RuleMode.BLOCKING,
                 rule="FullWildcardPrincipalRule",
-                reason="PolicyA should not allow wildcards in principals (principal: '*')",
+                reason="PolicyA should not allow full wildcard '*', or wildcard in account ID like 'arn:aws:iam::*:12345' at '*'",
                 granularity=RuleGranularity.RESOURCE,
                 risk_value=RuleRisk.HIGH,
                 actions=None,