Merge pull request #146 from Skyscanner/partial-wildcard-rule-allow-e…

…lb-accounts

Update to Wildcard Principal rule to ignore wildcards on AWS Service accounts where it is required

CHANGELOG.md

@@ -1,10 +1,15 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
-## [0.23.2] - 2021-02-01
+## [0.23.2] - 2021-02-04
+### Bugfix
+- `GenericWildcardPrincipalRule` to ignore account IDs where full or partial wildcard is required in the Principal.
+These accounts should be AWS Service Accounts defined in the config.
+- Fix CLI flag `--rules-config-file`
 ### Improvements
 - Update `ResourceSpecificRule` to allow for certain resources to be excluded. In particular, the
 `PrivilegeEscalationRule` will now no longer be invoked for `S3BucketPolicy` resources.
+- Add rules config for Kinesis Data Firehose IPs that can be applied
 
 ## [0.23.1] - 2021-01-26
 ### Improvements
@@ -31,7 +36,6 @@ All notable changes to this project will be documented in this file.
 ### Improvements
 - Add SNS actions that only allow wildcards
 
-
 ## [0.21.0] - 2020-11-30
 ### Improvements
 - Upgraded to pycfmodel 0.8.1 (this will improve policy action detection)

cfripper/rules/base_rules.py

@@ -132,7 +132,20 @@ def resource_invoke(self, resource: Resource, logical_id: str, extras: Optional[
 
 
 class PrincipalCheckingRule(Rule, ABC):
-    """Abstract class for rules that check principals"""
+    """
+    Abstract class for rules that check principals.
+
+    `valid_principals` is a set of the following Account IDs and Canonical IDs:
+      - `aws_principals` set in the user defined config (default = None)
+      - ELB Log Account IDs from AWS
+      - ElastiCache Backup Canonical IDs
+      - (if defined) The AWS Account in the config which CFRipper is executing with
+
+    When using `valid_principals`, make sure the scope of accounts allowed is not too large.
+    It might be the case that the account the stack is being deployed in is in this set.
+    This could raise false negatives in rules. If a rule should only be exempt for AWS Service
+    IDs, such as ELB and ElastiCache, consider using `_get_whitelist_from_config()` directly.
+    """
 
     _valid_principals = None
 

cfripper/rules/wildcard_principals.py

@@ -27,7 +27,6 @@ class GenericWildcardPrincipalRule(PrincipalCheckingRule):
     """
 
     REASON_WILCARD_PRINCIPAL = "{} should not allow wildcard in principals or account-wide principals (principal: '{}')"
-    REASON_NOT_ALLOWED_PRINCIPAL = "{} contains an unknown principal: {}"
     GRANULARITY = RuleGranularity.RESOURCE
 
     IAM_PATTERN = re.compile(r"arn:aws:iam::(\d*|\*):.*")
@@ -50,17 +49,21 @@ def check_for_wildcards(self, result: Result, logical_id: str, resource: PolicyD
         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():
-                    # Check if account ID is allowed
                     account_id_match = self.IAM_PATTERN.match(principal)
-                    if account_id_match:
-                        self.validate_account_id(result, logical_id, account_id_match.group(1))
+                    account_id = account_id_match.group(1) if account_id_match else None
+
+                    # Check if account ID is allowed. `self._get_whitelist_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_whitelist_from_config():
+                        continue
 
                     if statement.Condition and statement.Condition.dict():
                         logger.warning(
                             f"Not adding {type(self).__name__} failure in {logical_id} because there are conditions: "
                             f"{statement.Condition}"
                         )
-                    elif not self.resource_is_whitelisted(logical_id):
+                    elif not self.resource_is_whitelisted(logical_id=logical_id):
                         self.add_failure_to_result(
                             result,
                             self.REASON_WILCARD_PRINCIPAL.format(logical_id, principal),
@@ -70,15 +73,6 @@ def check_for_wildcards(self, result: Result, logical_id: str, resource: PolicyD
     def resource_is_whitelisted(self, logical_id):
         return logical_id in self._config.get_whitelisted_resources(type(self).__name__)
 
-    def validate_account_id(self, result: Result, logical_id: str, account_id: str):
-        if self.should_add_failure(logical_id, account_id):
-            self.add_failure_to_result(result, self.REASON_NOT_ALLOWED_PRINCIPAL.format(logical_id, account_id))
-
-    def should_add_failure(self, logical_id: str, account_id: str) -> bool:
-        if account_id in self.valid_principals:
-            return False
-        return not self.resource_is_whitelisted(logical_id)
-
 
 class PartialWildcardPrincipalRule(GenericWildcardPrincipalRule):
     """

tests/rules/test_GenericWildcardPrincipal.py

@@ -29,18 +29,16 @@ def test_failures_are_raised(bad_template):
     result = rule.invoke(bad_template)
 
     assert not result.valid
-    assert len(result.failed_rules) == 3
+    assert len(result.failed_rules) == 2
     assert len(result.failed_monitored_rules) == 0
     assert result.failed_rules[0].rule == "GenericWildcardPrincipalRule"
     assert (
         result.failed_rules[0].reason == "PolicyA should not allow wildcard in principals or account-wide principals "
         "(principal: 'somewhatrestricted:*')"
     )
     assert result.failed_rules[1].rule == "GenericWildcardPrincipalRule"
-    assert result.failed_rules[1].reason == "PolicyA contains an unknown principal: 123445"
-    assert result.failed_rules[2].rule == "GenericWildcardPrincipalRule"
     assert (
-        result.failed_rules[2].reason == "PolicyA should not allow wildcard in principals or account-wide principals "
+        result.failed_rules[1].reason == "PolicyA should not allow wildcard in principals or account-wide principals "
         "(principal: 'arn:aws:iam::123445:*')"
     )
 

tests/rules/test_PartialWildcardPrincipal.py

@@ -1,5 +1,6 @@
 from pytest import fixture
 
+from cfripper.config.config import Config
 from cfripper.rules import PartialWildcardPrincipalRule
 from tests.utils import get_cfmodel_from
 
@@ -14,6 +15,18 @@ def bad_template():
     return get_cfmodel_from("rules/PartialWildcardPrincipalRule/bad_template.json").resolve()
 
 
+@fixture()
+def intra_account_root_access():
+    return get_cfmodel_from("rules/PartialWildcardPrincipalRule/intra_account_root_access.yml").resolve()
+
+
+@fixture()
+def aws_elb_allow_template():
+    return get_cfmodel_from("rules/PartialWildcardPrincipalRule/aws_elb_template.yml").resolve(
+        extra_params={"AWS::Region": "ap-southeast-1"}
+    )
+
+
 def test_no_failures_are_raised(good_template):
     rule = PartialWildcardPrincipalRule(None)
     result = rule.invoke(good_template)
@@ -28,19 +41,36 @@ def test_failures_are_raised(bad_template):
     result = rule.invoke(bad_template)
 
     assert not result.valid
-    assert len(result.failed_rules) == 4
+    assert len(result.failed_rules) == 2
     assert len(result.failed_monitored_rules) == 0
     assert result.failed_rules[0].rule == "PartialWildcardPrincipalRule"
-    assert result.failed_rules[0].reason == "PolicyA contains an unknown principal: 123445"
-    assert result.failed_rules[1].rule == "PartialWildcardPrincipalRule"
     assert (
-        result.failed_rules[1].reason == "PolicyA should not allow wildcard in principals or account-wide principals "
+        result.failed_rules[0].reason == "PolicyA should not allow wildcard in principals or account-wide principals "
         "(principal: 'arn:aws:iam::123445:12345*')"
     )
-    assert result.failed_rules[2].rule == "PartialWildcardPrincipalRule"
-    assert result.failed_rules[2].reason == "PolicyA contains an unknown principal: 123445"
-    assert result.failed_rules[3].rule == "PartialWildcardPrincipalRule"
+    assert result.failed_rules[1].rule == "PartialWildcardPrincipalRule"
     assert (
-        result.failed_rules[3].reason == "PolicyA should not allow wildcard in principals or account-wide principals "
+        result.failed_rules[1].reason == "PolicyA should not allow wildcard in principals or account-wide principals "
         "(principal: 'arn:aws:iam::123445:root')"
     )
+
+
+def test_failures_for_correct_account_ids(intra_account_root_access):
+    rule = PartialWildcardPrincipalRule(Config(aws_account_id="123456789012"))
+    result = rule.invoke(intra_account_root_access)
+
+    assert not result.valid
+    assert len(result.failed_rules) == 1
+    assert len(result.failed_monitored_rules) == 0
+    assert result.failed_rules[0].rule == "PartialWildcardPrincipalRule"
+    assert (
+        result.failed_rules[0].reason
+        == "AccLoadBalancerAccessLogBucketPolicy should not allow wildcard in principals or account-wide principals "
+        "(principal: 'arn:aws:iam::123456789012:root')"
+    )
+
+
+def test_aws_elb_allow_template(aws_elb_allow_template):
+    rule = PartialWildcardPrincipalRule(None)
+    result = rule.invoke(aws_elb_allow_template)
+    assert result.valid

tests/test_templates/rules/PartialWildcardPrincipalRule/aws_elb_template.yml

@@ -0,0 +1,65 @@
+AWSTemplateFormatVersion: 2010-09-09
+Description: Stack for bucket policy with principal being AWS owned account
+Resources:
+  BrokerLoadBalancerAccessLogBucketPolicy:
+    DependsOn: BrokerLoadBalancerAccessLogBucket
+    Type: "AWS::S3::BucketPolicy"
+    Properties:
+      Bucket:
+        Ref: BrokerLoadBalancerAccessLogBucket
+      PolicyDocument:
+        Statement:
+          - Effect: Allow
+            Principal:
+              AWS: !Sub
+                - "arn:aws:iam::${ELBAccountId}:root"
+                - ELBAccountId:
+                    !FindInMap [
+                      ElasticLoadBalancing,
+                      !Ref "AWS::Region",
+                      AccountId,
+                    ]
+            Action: s3:PutObject
+            Resource: !Sub
+              - "arn:aws:s3:::${BucketName}/*"
+              - BucketName: !Ref BrokerLoadBalancerAccessLogBucket
+Mappings:
+  ElasticLoadBalancing:
+    us-east-1:
+      AccountId: "127311923021"
+    us-east-2:
+      AccountId: "033677994240"
+    us-west-1:
+      AccountId: "027434742980"
+    us-west-2:
+      AccountId: "797873946194"
+    ca-central-1:
+      AccountId: "985666609251"
+    eu-central-1:
+      AccountId: "054676820928"
+    eu-west-1:
+      AccountId: "156460612806"
+    eu-west-2:
+      AccountId: "652711504416"
+    eu-west-3:
+      AccountId: "009996457667"
+    eu-north-1:
+      AccountId: "897822967062"
+    ap-east-1:
+      AccountId: "754344448648"
+    ap-northeast-1:
+      AccountId: "582318560864"
+    ap-northeast-2:
+      AccountId: "600734575887"
+    ap-northeast-3:
+      AccountId: "383597477331"
+    ap-southeast-1:
+      AccountId: "114774131450"
+    ap-southeast-2:
+      AccountId: "783225319266"
+    ap-south-1:
+      AccountId: "718504428378"
+    me-south-1:
+      AccountId: "076674570225"
+    sa-east-1:
+      AccountId: "507241528517"

tests/test_templates/rules/PartialWildcardPrincipalRule/intra_account_root_access.yml

@@ -0,0 +1,19 @@
+AWSTemplateFormatVersion: 2010-09-09
+Description: Stack for bucket policy with principal being the same AWS account as stack.
+Resources:
+  AccLoadBalancerAccessLogBucketPolicy:
+    DependsOn: AccLoadBalancerAccessLogBucket
+    Type: "AWS::S3::BucketPolicy"
+    Properties:
+      Bucket:
+        Ref: AccLoadBalancerAccessLogBucket
+      PolicyDocument:
+        Statement:
+          - Effect: Allow
+            Principal:
+              AWS:
+                - "arn:aws:iam::123456789012:root"
+            Action: s3:PutObject
+            Resource: !Sub
+              - "arn:aws:s3:::${BucketName}/*"
+              - BucketName: !Ref AccLoadBalancerAccessLogBucket