Fix kms key rotation rule (#195)

* Fix KMS key rotation rule * Add test * Update changelog and version
Skyscanner · Oct 7, 2021 · 1bc3ff4 · 1bc3ff4
1 parent 83ca480
commit 1bc3ff4
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,10 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [1.1.2] - 2021-10-06
+### Fixes
+- Add a fix to the `KMSKeyEnabledKeyRotation` rule to be able to detect the `EnableKeyRotation` property properly.
+
 ## [1.1.1] - 2021-09-30
 ### Fixes
 - Add a fix to the `PartialWildcardPrincipal` rule to be able to detect policies where whole account access is specified via just the account ID.

diff --git a/cfripper/__version__.py b/cfripper/__version__.py
@@ -1,3 +1,3 @@
-VERSION = (1, 1, 1)
+VERSION = (1, 1, 2)
 
 __version__ = ".".join(map(str, VERSION))
diff --git a/cfripper/rules/kms_key_rotation_enabled.py b/cfripper/rules/kms_key_rotation_enabled.py
@@ -34,9 +34,9 @@ class KMSKeyEnabledKeyRotation(Rule):
 
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
-        for logical_id, resource in cfmodel.resources_filtered_by_type(("AWS::KMS::Key")).items():
-            if not hasattr(resource, "KeySpec") or resource.Properties.get("KeySpec") == "SYMMETRIC_DEFAULT":
-                if not hasattr(resource, "EnableKeyRotation") or resource.Properties.get("EnableKeyRotation") is False:
+        for logical_id, resource in cfmodel.resources_filtered_by_type("AWS::KMS::Key").items():
+            if not resource.Properties.KeySpec or resource.Properties.KeySpec == "SYMMETRIC_DEFAULT":
+                if not resource.Properties.EnableKeyRotation or resource.Properties.EnableKeyRotation is False:
                     self.add_failure_to_result(
                         result,
                         self.REASON.format(logical_id),

diff --git a/tests/rules/test_KMSEnabledKeyRotationRule.py b/tests/rules/test_KMSEnabledKeyRotationRule.py
@@ -14,32 +14,37 @@ def bad_template():
 
 
 @pytest.mark.parametrize(
-    "bad_template_path",
+    "template_path, success",
     [
-        "rules/KMSEnabledKeyRotation/bad_template_symmetric_keyspec_property.yaml",
-        "rules/KMSEnabledKeyRotation/bad_template_symmetric_no_property.yaml",
-        "rules/KMSEnabledKeyRotation/bad_template_symmetric_property.yaml",
+        ("rules/KMSEnabledKeyRotation/bad_template_symmetric_keyspec_property.yaml", False),
+        ("rules/KMSEnabledKeyRotation/bad_template_symmetric_no_property.yaml", False),
+        ("rules/KMSEnabledKeyRotation/bad_template_symmetric_property.yaml", False),
+        ("rules/KMSEnabledKeyRotation/good_template.yaml", True),
     ],
 )
-def test_failures_are_raised(bad_template_path):
+def test_failures_are_raised(template_path, success):
     rule = KMSKeyEnabledKeyRotation(Config())
-    result = rule.invoke(get_cfmodel_from(bad_template_path).resolve())
-
-    assert not result.valid
-    assert compare_lists_of_failures(
-        result.failures,
-        [
-            Failure(
-                granularity=RuleGranularity.RESOURCE,
-                reason="KMS Key KMSKey should have the key rotation enabled for symmetric keys",
-                risk_value=RuleRisk.HIGH,
-                rule="KMSKeyEnabledKeyRotation",
-                rule_mode=RuleMode.BLOCKING,
-                actions=None,
-                resource_ids={"KMSKey"},
-            )
-        ],
-    )
+    result = rule.invoke(get_cfmodel_from(template_path).resolve())
+
+    if success:
+        assert result.valid
+        assert compare_lists_of_failures(result.failures, [])
+    else:
+        assert not result.valid
+        assert compare_lists_of_failures(
+            result.failures,
+            [
+                Failure(
+                    granularity=RuleGranularity.RESOURCE,
+                    reason="KMS Key KMSKey should have the key rotation enabled for symmetric keys",
+                    risk_value=RuleRisk.HIGH,
+                    rule="KMSKeyEnabledKeyRotation",
+                    rule_mode=RuleMode.BLOCKING,
+                    actions=None,
+                    resource_ids={"KMSKey"},
+                )
+            ],
+        )
 
 
 def test_rule_supports_filter_config(bad_template, default_allow_all_config):

diff --git a/tests/test_templates/rules/KMSEnabledKeyRotation/good_template.yaml b/tests/test_templates/rules/KMSEnabledKeyRotation/good_template.yaml
@@ -0,0 +1,16 @@
+Resources:
+  KMSKey:
+    Type: AWS::KMS::Key
+    Properties:
+      Description: An example multi-Region primary key
+      EnableKeyRotation: True
+      KeyPolicy:
+        Version: '2012-10-17'
+        Id: key-default-1
+        Statement:
+          - Sid: Enable IAM User Permissions
+            Effect: Allow
+            Principal:
+              AWS: arn:aws:iam::111122223333:root
+            Action: kms:*
+            Resource: '*'