Merge pull request #147 from Skyscanner/resource-specific-rule-exclud…

…e-resource-types

Update `ResourceSpecificRule` to allow for certain resources to be excluded

CHANGELOG.md

@@ -1,6 +1,11 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [0.23.2] - 2021-02-01
+### Improvements
+- Update `ResourceSpecificRule` to allow for certain resources to be excluded. In particular, the
+`PrivilegeEscalationRule` will now no longer be invoked for `S3BucketPolicy` resources.
+
 ## [0.23.1] - 2021-01-26
 ### Improvements
 - Add more X-Ray permissions that accept wildcard resource only

cfripper/__version__.py

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

cfripper/rules/base_rules.py

@@ -101,15 +101,28 @@ def add_warning_to_result(
 
 class ResourceSpecificRule(Rule):
     """
-    Base class for rules that only apply to a subset of resource types
+    Base class for rules that only apply to a subset of resource types.
+
+    RESOURCE_TYPES: Resources to invoke the rule for.
+    EXCLUDED_RESOURCE_TYPES: Resources to explicitly not run the rule for.
+
+    Both fields are included to allow for more granular rule definitions. For example,
+    you may want to allow all resources except S3BucketPolicies, in which case you
+    would define these variables as:
+
+    EXCLUDED_RESOURCE_TYPES = (S3BucketPolicy,)
+    RESOURCE_TYPES = (Resource,)
+
+    Where the `S3BucketPolicy` Resource inherits from the base `Resource` class.
     """
 
+    EXCLUDED_RESOURCE_TYPES: Tuple[Type] = tuple()
     RESOURCE_TYPES: Tuple[Type] = tuple()
 
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
         for logical_id, resource in cfmodel.Resources.items():
-            if isinstance(resource, self.RESOURCE_TYPES):
+            if isinstance(resource, self.RESOURCE_TYPES) and not isinstance(resource, self.EXCLUDED_RESOURCE_TYPES):
                 result += self.resource_invoke(resource=resource, logical_id=logical_id, extras=extras)
         return result
 

cfripper/rules/privilege_escalation.py

@@ -1,6 +1,7 @@
 __all__ = ["PrivilegeEscalationRule"]
 
 from pycfmodel.model.resources.resource import Resource
+from pycfmodel.model.resources.s3_bucket_policy import S3BucketPolicy
 
 from cfripper.model.enums import RuleGranularity
 from cfripper.rules.base_rules import BaseDangerousPolicyActions
@@ -22,6 +23,7 @@ class PrivilegeEscalationRule(BaseDangerousPolicyActions):
 
     GRANULARITY = RuleGranularity.ACTION
     REASON = "{} has blacklisted IAM actions: {}"
+    EXCLUDED_RESOURCE_TYPES = (S3BucketPolicy,)
     RESOURCE_TYPES = (Resource,)
     DANGEROUS_ACTIONS = [
         "iam:AddUserToGroup",

tests/rules/test_PrivilegeEscalationRule.py

@@ -14,6 +14,11 @@ def privilege_escalation_role_cf():
     return get_cfmodel_from("rules/PrivilegeEscalationRule/privilege_escalation_role.yaml").resolve()
 
 
+@fixture()
+def valid_privilege_escalation_on_s3_bucket_policy():
+    return get_cfmodel_from("rules/PrivilegeEscalationRule/privilege_escalation_s3_bucket_policy.yaml").resolve()
+
+
 def test_valid_role_inline_policy(valid_role_inline_policy):
     rule = PrivilegeEscalationRule(None)
     result = rule.invoke(valid_role_inline_policy)
@@ -40,3 +45,9 @@ def test_privilege_escalation_using_role(privilege_escalation_role_cf):
         result.failed_rules[0].reason
         == "PrivilegeInjectorRole has blacklisted IAM actions: ['iam:UpdateAssumeRolePolicy']"
     )
+
+
+def test_valid_privilege_escalation_on_s3_bucket_policy(valid_privilege_escalation_on_s3_bucket_policy):
+    rule = PrivilegeEscalationRule(None)
+    result = rule.invoke(valid_privilege_escalation_on_s3_bucket_policy)
+    assert result.valid

tests/test_templates/rules/PrivilegeEscalationRule/privilege_escalation_s3_bucket_policy.yaml

@@ -0,0 +1,29 @@
+AWSTemplateFormatVersion: 2010-09-09
+Description: "Bucket policy with Action:* in place"
+
+Resources:
+  TestBucketPolicy:
+    Type: AWS::S3::BucketPolicy
+    Properties:
+      Bucket: !Ref SomeS3Bucket
+      PolicyDocument:
+        Version: 2008-10-17
+        Statement:
+          - Sid: Stmt123456789012
+            Effect: Allow
+            Principal:
+              {
+                CanonicalUser: 1234abcd5678,
+                AWS: "arn:aws:iam::123456789012:role/s3-bucket-foo-bar-role",
+              }
+            Action: "*"
+            Resource: "arn:aws:s3:::my-bucket-of-potatoes"
+          - Sid: Stmt987654321345
+            Effect: Allow
+            Principal:
+              {
+                CanonicalUser: 1234abcd5678,
+                AWS: "arn:aws:iam::123456789012:role/s3-bucket-foo-bar-role",
+              }
+            Action: "*"
+            Resource: "arn:aws:s3:::my-bucket-of-potatoes/*"