Merge branch 'master' into add-dependabot

.github/release-drafter.yml

@@ -0,0 +1,19 @@
+name-template: 'v$RESOLVED_VERSION'
+tag-template: 'v$RESOLVED_VERSION'
+change-template: '- $TITLE @$AUTHOR (#$NUMBER)'
+change-title-escapes: '\<*_&' # You can add # and @ to disable mentions, and add ` to disable code blocks.
+version-resolver:
+  major:
+    labels:
+      - 'major'
+  minor:
+    labels:
+      - 'minor'
+  patch:
+    labels:
+      - 'patch'
+  default: patch
+template: |
+  ## Changes
+  
+  $CHANGES

.github/workflows/release-drafter.yml

@@ -0,0 +1,32 @@
+name: Release Drafter
+
+on:
+  push:
+    # branches to consider in the event; optional, defaults to all
+    branches:
+      - master
+  # pull_request event is required only for autolabeler
+  pull_request:
+    # Only following types are handled by the action, but one can default to all as well
+    types: [opened, reopened, synchronize]
+  # pull_request_target event is required for autolabeler to support PRs from forks
+  pull_request_target:
+    types: [opened, reopened, synchronize]
+
+permissions:
+  contents: read
+
+jobs:
+  update_release_draft:
+    permissions:
+      # write permission is required to create a github release
+      contents: write
+      # write permission is required for autolabeler
+      # otherwise, read permission is required at least
+      pull-requests: write
+    runs-on: ubuntu-latest
+    steps:
+      # Drafts your next Release notes as Pull Requests are merged into "master"
+      - uses: release-drafter/release-drafter@v5
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

CHANGELOG.md

@@ -1,9 +1,26 @@
 # 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 
+- Improve logging for the exception when applying rule filters
+- Add release drafter 
+
+## [1.15.2]
+### Fixes
+- Fixes https://github.com/Skyscanner/cfripper/issues/260
+
 ## [1.15.1]
 ### Fixes
-- Fix documentation.
+- Fix docs generation
 
 ## [1.15.0]
 ### Additions

README.md

@@ -1,11 +1,12 @@
 <p align="center">
-<img src="docs/img/logo.png" width="200">
+<img src="docs/img/logo.png" width="200" alt="cfripper logo">
 </p>
 
 # CFRipper
 
 ![Build Status](https://github.com/Skyscanner/cfripper/workflows/PyPI%20release/badge.svg)
 [![PyPI version](https://badge.fury.io/py/cfripper.svg)](https://badge.fury.io/py/cfripper)
+[![homebrew version](https://img.shields.io/homebrew/v/cfripper)](https://formulae.brew.sh/formula/cfripper)
 ![License](https://img.shields.io/github/license/skyscanner/cfripper)
 
 CFRipper is a Library and CLI security analyzer for AWS CloudFormation templates. You can use CFRipper to prevent deploying insecure AWS resources into your Cloud environment. You can write your own compliance checks by adding new custom plugins.

cfripper/__version__.py

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

cfripper/rules/base_rules.py

@@ -66,7 +66,12 @@ def add_failure_to_result(
                     if self._config.metrics_logger:
                         self._config.metrics_logger(rule=self.__class__.__name__, filter_reason=rule_filter.reason)
             except Exception:
-                logger.exception(f"Exception raised while evaluating filter for `{rule_filter.reason}`", extra=context)
+                logger.exception(
+                    f"Exception raised while evaluating rule {self.__class__.__name__} "
+                    f"with filter for `{rule_filter.reason}`. "
+                    f"Stack: {self._config.stack_name} Account: {self._config.aws_account_id}",
+                    extra=context,
+                )
 
         if rule_mode != RuleMode.ALLOWED:
             result.add_failure(

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

cfripper/rules/stack_name_matches_regex.py

@@ -28,11 +28,11 @@ def _stack_name_matches_regex(self, stack_name: str) -> bool:
 
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
+        if not extras:
+            extras = {}
         stack_name = self._config.stack_name or extras.get("stack_name", "")
         if not stack_name:
             return result
-        if not extras:
-            extras = {}
 
         if not self._stack_name_matches_regex(stack_name):
             self.add_failure_to_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/IAMRolesOverprivilegedRule/invalid_role_inline_policy_fn_if.json

@@ -35,7 +35,7 @@
                     {
                       "Action": "sts:AssumeRole",
                       "Effect": "Allow",
-                      "Resource": "arn:aws:iam::325714046698:role/sandbox-secrets-access"
+                      "Resource": "arn:aws:iam::123456789012:role/test-role"
                     }
                   ],
                   "Version": "2012-10-17"
@@ -65,4 +65,4 @@
       }
     }
   }
-}
+}

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