diff --git a/.github/release-drafter.yml b/.github/release-drafter.yml index fb3da367..1318f25c 100644 --- a/.github/release-drafter.yml +++ b/.github/release-drafter.yml @@ -14,4 +14,6 @@ version-resolver: - 'patch' default: patch template: | - ## Changes \ No newline at end of file + ## Changes + + $CHANGES \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 11e63309..53477741 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/cfripper/__version__.py b/cfripper/__version__.py index df6a2f7f..ba2922a6 100644 --- a/cfripper/__version__.py +++ b/cfripper/__version__.py @@ -1,3 +1,3 @@ -VERSION = (1, 15, 3) +VERSION = (1, 15, 4) __version__ = ".".join(map(str, VERSION)) diff --git a/cfripper/rules/kms_key_wildcard_principal.py b/cfripper/rules/kms_key_wildcard_principal.py index 3934cadf..4f7cc2f9 100644 --- a/cfripper/rules/kms_key_wildcard_principal.py +++ b/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 diff --git a/requirements.txt b/requirements.txt index fa70e777..5639dc25 100644 --- a/requirements.txt +++ b/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 diff --git a/setup.py b/setup.py index cf2c6a66..aa613d04 100644 --- a/setup.py +++ b/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", ] diff --git a/tests/rules/test_CrossAccountTrustRule.py b/tests/rules/test_CrossAccountTrustRule.py index a34fee61..bb7bb890 100644 --- a/tests/rules/test_CrossAccountTrustRule.py +++ b/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", [ diff --git a/tests/rules/test_KMSKeyWildcardPrincipal.py b/tests/rules/test_KMSKeyWildcardPrincipal.py index 72f81953..34d30cfa 100644 --- a/tests/rules/test_KMSKeyWildcardPrincipal.py +++ b/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, []) diff --git a/tests/rules/test_WildcardResourceRule.py b/tests/rules/test_WildcardResourceRule.py index 14541df0..21c3fc42 100644 --- a/tests/rules/test_WildcardResourceRule.py +++ b/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"', diff --git a/tests/test_templates/rules/CrossAccountTrustRule/kms_key_without_policy.yml b/tests/test_templates/rules/CrossAccountTrustRule/kms_key_without_policy.yml new file mode 100644 index 00000000..b3400c53 --- /dev/null +++ b/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 diff --git a/tests/test_templates/rules/WildcardResourceRule/kms_key_with_wildcard_resource.json b/tests/test_templates/rules/KMSKeyWildcardPrincipalRule/kms_key_with_wildcard_resource.json similarity index 96% rename from tests/test_templates/rules/WildcardResourceRule/kms_key_with_wildcard_resource.json rename to tests/test_templates/rules/KMSKeyWildcardPrincipalRule/kms_key_with_wildcard_resource.json index 45d5691a..1599b66b 100644 --- a/tests/test_templates/rules/WildcardResourceRule/kms_key_with_wildcard_resource.json +++ b/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": "*" diff --git a/tests/test_templates/rules/KMSKeyWildcardPrincipalRule/kms_key_without_policy.yml b/tests/test_templates/rules/KMSKeyWildcardPrincipalRule/kms_key_without_policy.yml new file mode 100644 index 00000000..b3400c53 --- /dev/null +++ b/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