Create new GenericResourceWildcardPolicyRule (#209)

* create new GenericResourceWildcardPolicyRule

* merging with master

Co-authored-by: Ramon <ramon.guimera@skyscanner.net>

CHANGELOG.md

@@ -1,6 +1,12 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [1.5.1]
+### Updates
+- Created `GenericResourceWildcardPolicyRule` in order to check for WildcardPolicy issues in generic resources.
+- Added documentation regarding the deprecation of `S3BucketPolicyWildcardActionRule`, `SNSTopicPolicyWildcardActionRule` and `SQSQueuePolicyWildcardActionRule`.
+- Covering cases for already mapped models in rules inherited from `GenericWildcardPolicyRule` with the new `GenericResourceWildcardPolicyRule`.
+
 ## [1.5.0]
 ### Updates
 - Created `GenericCrossAccountTrustRule` in order to check for CrossAccount issues for generic resources.

cfripper/__version__.py

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

cfripper/rules/__init__.py

@@ -38,6 +38,7 @@
     SQSQueuePolicyPublicRule,
 )
 from cfripper.rules.wildcard_policies import (
+    GenericResourceWildcardPolicyRule,
     S3BucketPolicyWildcardActionRule,
     SNSTopicPolicyWildcardActionRule,
     SQSQueuePolicyWildcardActionRule,
@@ -57,6 +58,7 @@
         ElasticsearchDomainCrossAccountTrustRule,
         FullWildcardPrincipalRule,
         GenericCrossAccountTrustRule,
+        GenericResourceWildcardPolicyRule,
         HardcodedRDSPasswordRule,
         IAMRolesOverprivilegedRule,
         IAMRoleWildcardActionOnPolicyRule,

cfripper/rules/wildcard_policies.py

@@ -1,5 +1,6 @@
 __all__ = [
     "GenericWildcardPolicyRule",
+    "GenericResourceWildcardPolicyRule",
     "S3BucketPolicyWildcardActionRule",
     "SNSTopicPolicyWildcardActionRule",
     "SQSQueuePolicyWildcardActionRule",
@@ -57,8 +58,39 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         return result
 
 
+class GenericResourceWildcardPolicyRule(GenericWildcardPolicyRule):
+    """
+    Rule that checks for use of the wildcard `*` character in Actions of Policy Documents of Generic AWS Resources.
+    """
+
+    REASON = "{} should not allow a `*` action"
+    GRANULARITY = RuleGranularity.RESOURCE
+
+    def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
+        result = Result()
+        for logical_id, resource in cfmodel.Resources.items():
+            policy_documents = resource.policy_documents
+            if policy_documents:
+                for document in policy_documents:
+                    if document.policy_document.allowed_actions_with(REGEX_HAS_STAR_OR_STAR_AFTER_COLON):
+                        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,
+                            },
+                        )
+        return result
+
+
 class S3BucketPolicyWildcardActionRule(GenericWildcardPolicyRule):
     """
+    Soon to be replaced by `GenericResourceWildcardPolicyRule`.
+
     Checks for use of the wildcard `*` character in the Actions of Policy Documents of S3 Bucket Policies.
     This rule is a subclass of `GenericWildcardPolicyRule`.
 
@@ -76,6 +108,8 @@ class S3BucketPolicyWildcardActionRule(GenericWildcardPolicyRule):
 
 class SNSTopicPolicyWildcardActionRule(GenericWildcardPolicyRule):
     """
+    Soon to be replaced by `GenericResourceWildcardPolicyRule`.
+
     Checks for use of the wildcard `*` character in the Actions of Policy Documents of SQS Queue Policies.
     This rule is a subclass of `GenericWildcardPolicyRule`.
 
@@ -93,6 +127,8 @@ class SNSTopicPolicyWildcardActionRule(GenericWildcardPolicyRule):
 
 class SQSQueuePolicyWildcardActionRule(GenericWildcardPolicyRule):
     """
+    Soon to be replaced by `GenericResourceWildcardPolicyRule`.
+
     Checks for use of the wildcard `*` character in the Actions of Policy Documents of SQS Queue Policies.
     This rule is a subclass of `GenericWildcardPolicyRule`.
 

tests/rules/test_GenericResourceWildcardPolicyRule.py

@@ -0,0 +1,151 @@
+import pytest
+
+from cfripper.model.enums import RuleGranularity, RuleMode, RuleRisk
+from cfripper.model.result import Failure
+from cfripper.rules.wildcard_policies import GenericResourceWildcardPolicyRule
+from tests.utils import compare_lists_of_failures, get_cfmodel_from
+
+
+def s3_bucket_with_wildcards():
+    return get_cfmodel_from("rules/WildcardPoliciesRule/s3_bucket_with_wildcards.json").resolve()
+
+
+def sqs_queue_with_wildcards():
+    return get_cfmodel_from("rules/WildcardPoliciesRule/sqs_queue_with_wildcards.json").resolve()
+
+
+def sns_topic_with_wildcards():
+    return get_cfmodel_from("rules/WildcardPoliciesRule/sns_topic_with_wildcards.json").resolve()
+
+
+def generic_with_wildcards():
+    return get_cfmodel_from("rules/WildcardPoliciesRule/generic_with_wildcards.json").resolve()
+
+
+@pytest.fixture()
+def sns_topic_with_wildcards_fixture():
+    return sns_topic_with_wildcards()
+
+
+@pytest.mark.parametrize(
+    "template, is_valid, failures",
+    [
+        (
+            sns_topic_with_wildcards(),
+            False,
+            [
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="mysnspolicy1 should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"mysnspolicy1"},
+                )
+            ],
+        ),
+        (
+            sqs_queue_with_wildcards(),
+            False,
+            [
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="mysqspolicy1 should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"mysqspolicy1"},
+                ),
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="mysqspolicy1b should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"mysqspolicy1b"},
+                ),
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="mysqspolicy1c should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"mysqspolicy1c"},
+                ),
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="mysqspolicy1d should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"mysqspolicy1d"},
+                ),
+            ],
+        ),
+        (
+            s3_bucket_with_wildcards(),
+            False,
+            [
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="S3BucketPolicy should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"S3BucketPolicy"},
+                ),
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="S3BucketPolicy2 should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"S3BucketPolicy2"},
+                ),
+            ],
+        ),
+        (
+            generic_with_wildcards(),
+            False,
+            [
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="NotMapped4 should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"NotMapped4"},
+                ),
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="NotMapped5 should not allow a `*` action",
+                    risk_value=RuleRisk.MEDIUM,
+                    rule="GenericResourceWildcardPolicyRule",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"NotMapped5"},
+                ),
+            ],
+        ),
+    ],
+)
+def test_generic_rule_with_already_mapped_resources(template, is_valid, failures):
+    rule = GenericResourceWildcardPolicyRule(None)
+    result = rule.invoke(template)
+    assert result.valid == is_valid
+    assert compare_lists_of_failures(result.failures, failures,)
+
+
+def test_rule_supports_filter_config(sns_topic_with_wildcards_fixture, default_allow_all_config):
+    rule = GenericResourceWildcardPolicyRule(default_allow_all_config)
+    result = rule.invoke(sns_topic_with_wildcards_fixture)
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])

tests/test_templates/rules/WildcardPoliciesRule/generic_with_wildcards.json

@@ -0,0 +1,93 @@
+{
+  "Resources": {
+    "NotMapped1": {
+      "Type": "AWS::Not::Mapped",
+      "Properties": {
+        "BucketName": "fakebucketfakebucket"
+      }
+    },
+    "NotMapped2": {
+      "Type": "AWS::Not::Mapped",
+      "Properties": {
+        "BucketName": "fakebucketfakebucket2"
+      }
+    },
+    "NotMapped3": {
+      "Type": "AWS::Not::Mapped",
+      "Properties": {
+        "BucketName": "fakebucketfakebucket3"
+      }
+    },
+    "NotMapped4": {
+      "Type": "AWS::Not::Mapped",
+      "Properties": {
+        "Bucket": {
+          "Ref": "S3Bucket"
+        },
+        "PolicyDocument": {
+          "Statement": [
+            {
+              "Action": [
+                "*"
+              ],
+              "Effect": "Allow",
+              "Resource": "arn:aws:s3:::fakebucketfakebucket/*",
+              "Principal": {
+                "AWS": [
+                  "156460612806"
+                ]
+              }
+            }
+          ]
+        }
+      }
+    },
+    "NotMapped5": {
+      "Type": "AWS::Not::Mapped",
+      "Properties": {
+        "Bucket": {
+          "Ref": "S3Bucket2"
+        },
+        "PolicyDocument": {
+          "Statement": [
+            {
+              "Action": [
+                "s3:*"
+              ],
+              "Effect": "Allow",
+              "Resource": "arn:aws:s3:::fakebucketfakebucket2/*",
+              "Principal": {
+                "AWS": "*"
+              }
+            }
+          ]
+        }
+      }
+    },
+    "NotMapped6": {
+      "Type": "AWS::Not::Mapped",
+      "Properties": {
+        "Bucket": {
+          "Ref": "S3Bucket3"
+        },
+        "PolicyDocument": {
+          "Statement": [
+            {
+              "Action": [
+                "s3:PutObject"
+              ],
+              "Effect": "Deny",
+              "Resource": "arn:aws:s3:::fakebucketfakebucket3/*",
+              "Principal": "*",
+              "Condition": {
+                "StringNotEquals": {
+                  "s3:x-amz-server-side-encryption": "AES256"
+                }
+              }
+            }
+          ]
+        }
+      }
+    }
+  }
+}