Merge pull request #268 from Skyscanner/bump_pycfmodel

Bump pycfmodel and fix KMSKeyWildcardPrincipalRule

.github/release-drafter.yml

@@ -14,4 +14,6 @@ version-resolver:
       - 'patch'
   default: patch
 template: |
-  ## Changes
+  ## Changes
+  
+  $CHANGES

CHANGELOG.md

@@ -1,6 +1,13 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [1.15.4]
+## Fixes
+- Fix `KMSKeyWildcardPrincipalRule` to work without a KMS policy
+- Fix release drafter template to show PR titles
+### Updates
+- Bumped minimum `pycfmodel` version to `0.22.0`
+
 ## [1.15.3]
 ## Changes
 - Update invalid_role_inline_policy_fn_if.json 

cfripper/__version__.py

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

cfripper/rules/kms_key_wildcard_principal.py

@@ -37,26 +37,27 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
         for logical_id, resource in cfmodel.Resources.items():
             if isinstance(resource, KMSKey):
-                for statement in resource.Properties.KeyPolicy._statement_as_list():
-                    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,
-                                    },
-                                )
+                if resource.Properties.KeyPolicy:
+                    for statement in resource.Properties.KeyPolicy._statement_as_list():
+                        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

requirements.txt

@@ -10,7 +10,7 @@ cfn-flip==1.3.0
 click==8.1.2
 jmespath==1.0.0
 pluggy==0.13.1
-pycfmodel==0.20.0
+pycfmodel==0.22.0
 pydantic==1.9.0
 pydash==6.0.0
 python-dateutil==2.8.2

setup.py

@@ -11,7 +11,7 @@
     "cfn_flip>=1.2.0",
     "click>=8.0.0",
     "pluggy~=0.13.1",
-    "pycfmodel>=0.20.0",
+    "pycfmodel>=0.22.0",
     "pydash>=4.7.6",
     "PyYAML>=4.2b1",
 ]

tests/rules/test_CrossAccountTrustRule.py

@@ -301,6 +301,14 @@ def test_kms_key_cross_account_sts(template, is_valid, failures):
     assert compare_lists_of_failures(result.failures, failures)
 
 
+def test_kms_key__without_policy():
+    rule = KMSKeyCrossAccountTrustRule(Config(aws_account_id="123456789", aws_principals=["999999999"]))
+    model = get_cfmodel_from("rules/CrossAccountTrustRule/kms_key_without_policy.yml")
+    result = rule.invoke(model)
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])
+
+
 @pytest.mark.parametrize(
     "principal",
     [

tests/rules/test_KMSKeyWildcardPrincipal.py

@@ -1,22 +1,49 @@
-# import pytest
-#
-# from cfripper.rules.KMSKeyWildcardPrincipal import KMSKeyWildcardPrincipal
-# from cfripper.model.result import Result
-# from tests.utils import get_cfmodel_from
-
-# TODO Implement check if this is needed as GenericWildcardPrincipal rule seems to include this one
-# @pytest.fixture()
-# def abcdef():
-#     return get_cfmodel_from("rules/KMSKeyWildcardPrincipal/abcdef.json").resolve()
-#
-#
-# def test_abcdef(abcdef):
-#     result = Result()
-#     rule = KMSKeyWildcardPrincipal(None, result)
-#     rule.invoke(abcdef)
-#
-#     assert not result.valid
-#     assert len(result.failed_rules) == 1
-#     assert len(result.failed_monitored_rules) == 0
-#     assert result.failed_rules[0].rule == "KMSKeyWildcardPrincipal"
-#     assert result.failed_rules[0].reason == "KMS Key policy {} should not allow wildcard principals"
+import pytest
+
+from cfripper.model.result import Failure
+from cfripper.rules import KMSKeyWildcardPrincipalRule
+from tests.utils import compare_lists_of_failures, get_cfmodel_from
+
+
+@pytest.fixture()
+def kms_key_with_wildcard_policy():
+    return get_cfmodel_from("rules/KMSKeyWildcardPrincipalRule/kms_key_with_wildcard_resource.json").resolve()
+
+
+@pytest.fixture()
+def kms_key_without_policy():
+    return get_cfmodel_from("rules/KMSKeyWildcardPrincipalRule/kms_key_without_policy.yml").resolve()
+
+
+def test_kms_key_with_wildcard_resource_not_allowed_is_flagged(kms_key_with_wildcard_policy):
+    rule = KMSKeyWildcardPrincipalRule(None)
+    rule._config.stack_name = "stack3"
+    rule.all_cf_actions = set()
+    result = rule.invoke(kms_key_with_wildcard_policy)
+
+    assert result.valid is False
+    assert compare_lists_of_failures(
+        result.failures,
+        [
+            Failure(
+                granularity="RESOURCE",
+                reason="KMS Key policy myKey should not allow wildcard principals",
+                risk_value="MEDIUM",
+                rule="KMSKeyWildcardPrincipalRule",
+                rule_mode="BLOCKING",
+                actions=None,
+                resource_ids={"myKey"},
+                resource_types=None,
+            )
+        ],
+    )
+
+
+def test_kms_key_without_policy_is_not_flagged(kms_key_without_policy):
+    rule = KMSKeyWildcardPrincipalRule(None)
+    rule._config.stack_name = "stack3"
+    rule.all_cf_actions = set()
+    result = rule.invoke(kms_key_without_policy)
+
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])

tests/rules/test_WildcardResourceRule.py

@@ -17,7 +17,7 @@ def user_with_wildcard_resource():
 
 @pytest.fixture()
 def kms_key_with_wildcard_policy():
-    return get_cfmodel_from("rules/WildcardResourceRule/kms_key_with_wildcard_resource.json").resolve()
+    return get_cfmodel_from("rules/KMSKeyWildcardPrincipalRule/kms_key_with_wildcard_resource.json").resolve()
 
 
 @pytest.fixture()
@@ -434,6 +434,28 @@ def test_multiple_resources_with_wildcard_resources_are_detected(user_and_policy
                 resource_ids={"RolePolicy"},
                 resource_types={"AWS::IAM::Policy"},
             ),
+            Failure(
+                granularity="ACTION",
+                reason='"RolePolicy" is using a wildcard resource in "TheExtremePolicy" for "dynamodb:GetResourcePolicy"',
+                risk_value="MEDIUM",
+                rule="WildcardResourceRule",
+                rule_mode="BLOCKING",
+                actions={
+                    "dynamodb:CreateTable",
+                    "dynamodb:BatchGet*",
+                    "dynamodb:Scan",
+                    "dynamodb:Update*",
+                    "dynamodb:Query",
+                    "dynamodb:Delete*",
+                    "dynamodb:PutItem",
+                    "dynamodb:DescribeStream",
+                    "dynamodb:DescribeTable",
+                    "dynamodb:BatchWrite*",
+                    "dynamodb:Get*",
+                },
+                resource_ids={"RolePolicy"},
+                resource_types={"AWS::IAM::Policy"},
+            ),
             Failure(
                 granularity="ACTION",
                 reason='"RolePolicy" is using a wildcard resource in "TheExtremePolicy" for "dynamodb:GetShardIterator"',
@@ -610,6 +632,28 @@ def test_multiple_resources_with_wildcard_resources_are_detected(user_and_policy
                 resource_ids={"RolePolicy"},
                 resource_types={"AWS::IAM::Policy"},
             ),
+            Failure(
+                granularity="ACTION",
+                reason='"RolePolicy" is using a wildcard resource in "TheExtremePolicy" for "dynamodb:UpdateGlobalTableVersion"',
+                risk_value="MEDIUM",
+                rule="WildcardResourceRule",
+                rule_mode="BLOCKING",
+                actions={
+                    "dynamodb:CreateTable",
+                    "dynamodb:BatchGet*",
+                    "dynamodb:Scan",
+                    "dynamodb:Update*",
+                    "dynamodb:Query",
+                    "dynamodb:Delete*",
+                    "dynamodb:PutItem",
+                    "dynamodb:DescribeStream",
+                    "dynamodb:DescribeTable",
+                    "dynamodb:BatchWrite*",
+                    "dynamodb:Get*",
+                },
+                resource_ids={"RolePolicy"},
+                resource_types={"AWS::IAM::Policy"},
+            ),
             Failure(
                 granularity="ACTION",
                 reason='"RolePolicy" is using a wildcard resource in "TheExtremePolicy" for "dynamodb:UpdateItem"',

tests/test_templates/rules/CrossAccountTrustRule/kms_key_without_policy.yml

@@ -0,0 +1,9 @@
+---
+AWSTemplateFormatVersion: "2010-09-09"
+
+Resources:
+  MyKey:
+    Type: "AWS::KMS::Key"
+    Properties:
+      EnableKeyRotation: true
+      Enabled: true

tests/test_templates/rules/WildcardResourceRule/kms_key_with_wildcard_resource.json → tests/test_templates/rules/KMSKeyWildcardPrincipalRule/kms_key_with_wildcard_resource.json

@@ -12,7 +12,7 @@
               "Sid": "Enable IAM User Permissions",
               "Effect": "Allow",
               "Principal": {
-                "AWS": "arn:aws:iam::111122223333:root"
+                "AWS": "*"
               },
               "Action": "kms:*",
               "Resource": "*"

tests/test_templates/rules/KMSKeyWildcardPrincipalRule/kms_key_without_policy.yml

@@ -0,0 +1,9 @@
+---
+AWSTemplateFormatVersion: "2010-09-09"
+
+Resources:
+  MyKey:
+    Type: "AWS::KMS::Key"
+    Properties:
+      EnableKeyRotation: true
+      Enabled: true