New wildcard principal rules to understand generic resources (#212)

* created new wildcard principal rules to understand generic resources

* PR suggestions - update version

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

CHANGELOG.md

@@ -1,6 +1,13 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [1.6.0]
+### Updates
+- Created `GenericResourceWildcardPrincipalRule` to be an abstract for wildcard principals for Generic resources.
+- Created `GenericResourcePartialWildcardPrincipalRule` and `GenericResourceFullWildcardPrincipalRule` to evaluate Generic resources.
+### Fixes
+- Rollback `GenericWildcardPrincipalRule` as it was in `1.5.2`.
+
 ## [1.5.3]
 ### Updates
 - Updates `GenericWildcardPrincipalRule` to understand the `GenericResource`.

cfripper/__version__.py

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

cfripper/rules/__init__.py

@@ -43,7 +43,12 @@
     SNSTopicPolicyWildcardActionRule,
     SQSQueuePolicyWildcardActionRule,
 )
-from cfripper.rules.wildcard_principals import FullWildcardPrincipalRule, PartialWildcardPrincipalRule
+from cfripper.rules.wildcard_principals import (
+    FullWildcardPrincipalRule,
+    GenericResourceFullWildcardPrincipalRule,
+    GenericResourcePartialWildcardPrincipalRule,
+    PartialWildcardPrincipalRule,
+)
 from cfripper.rules.wildcard_resource_rule import WildcardResourceRule
 
 DEFAULT_RULES = {
@@ -58,6 +63,8 @@
         ElasticsearchDomainCrossAccountTrustRule,
         FullWildcardPrincipalRule,
         GenericCrossAccountTrustRule,
+        GenericResourceFullWildcardPrincipalRule,
+        GenericResourcePartialWildcardPrincipalRule,
         GenericResourceWildcardPolicyRule,
         HardcodedRDSPasswordRule,
         IAMRolesOverprivilegedRule,

cfripper/rules/wildcard_principals.py

@@ -1,12 +1,25 @@
-__all__ = ["GenericWildcardPrincipalRule", "PartialWildcardPrincipalRule", "FullWildcardPrincipalRule"]
+__all__ = [
+    "GenericWildcardPrincipalRule",
+    "PartialWildcardPrincipalRule",
+    "FullWildcardPrincipalRule",
+    "GenericResourceWildcardPrincipalRule",
+    "GenericResourcePartialWildcardPrincipalRule",
+    "GenericResourceFullWildcardPrincipalRule",
+]
 import logging
 import re
 from typing import Dict, Optional
 
 from pycfmodel.model.cf_model import CFModel
+from pycfmodel.model.resources.iam_managed_policy import IAMManagedPolicy
+from pycfmodel.model.resources.iam_policy import IAMPolicy
 from pycfmodel.model.resources.iam_role import IAMRole
+from pycfmodel.model.resources.iam_user import IAMUser
 from pycfmodel.model.resources.kms_key import KMSKey
 from pycfmodel.model.resources.properties.policy_document import PolicyDocument
+from pycfmodel.model.resources.s3_bucket_policy import S3BucketPolicy
+from pycfmodel.model.resources.sns_topic_policy import SNSTopicPolicy
+from pycfmodel.model.resources.sqs_queue_policy import SQSQueuePolicy
 
 from cfripper.config.regex import REGEX_FULL_WILDCARD_PRINCIPAL, REGEX_PARTIAL_WILDCARD_PRINCIPAL
 from cfripper.model.enums import RuleGranularity, RuleRisk
@@ -19,6 +32,7 @@
 class GenericWildcardPrincipalRule(PrincipalCheckingRule):
     """
     Checks for any wildcard principal defined in any statement.
+    Only for: IAMManagedPolicy, IAMPolicy, S3BucketPolicy, SNSTopicPolicy and SQSQueuePolicy
     To be inherited into more precise rules.
     Ignores KMS Keys, since they have `KMSKeyWildcardPrincipalRule`.
     For IAM Roles, it also checks `AssumeRolePolicyDocument`.
@@ -52,14 +66,14 @@ class GenericWildcardPrincipalRule(PrincipalCheckingRule):
     def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
         result = Result()
         for logical_id, resource in cfmodel.Resources.items():
-            if isinstance(resource, KMSKey):
-                # Ignoring KMSKey because there's already a rule for it `KMSKeyWildcardPrincipalRule`
-                continue
-            if isinstance(resource, IAMRole):
-                # Checking the `AssumeRolePolicyDocument` for IAM Roles
-                self.check_for_wildcards(result, logical_id, resource.Properties.AssumeRolePolicyDocument, extras)
-            for policy in resource.policy_documents:
-                self.check_for_wildcards(result, logical_id, policy.policy_document, extras)
+            if isinstance(resource, (IAMManagedPolicy, IAMPolicy, S3BucketPolicy, SNSTopicPolicy, SQSQueuePolicy)):
+                self.check_for_wildcards(result, logical_id, resource.Properties.PolicyDocument, extras)
+            elif isinstance(resource, (IAMRole, IAMUser)):
+                if isinstance(resource, IAMRole):
+                    self.check_for_wildcards(result, logical_id, resource.Properties.AssumeRolePolicyDocument, extras)
+                if resource.Properties and resource.Properties.Policies:
+                    for policy in resource.Properties.Policies:
+                        self.check_for_wildcards(result, logical_id, policy.PolicyDocument, extras)
 
         return result
 
@@ -153,3 +167,102 @@ class FullWildcardPrincipalRule(GenericWildcardPrincipalRule):
     """
 
     RISK_VALUE = RuleRisk.HIGH
+
+
+class GenericResourceWildcardPrincipalRule(GenericWildcardPrincipalRule):
+    """
+    Checks for any wildcard principal defined in any statement for any type of resource.
+    To be inherited into more precise rules.
+    Ignores KMS Keys, since they have `KMSKeyWildcardPrincipalRule`.
+    For IAM Roles, it also checks `AssumeRolePolicyDocument`.
+
+    Risk:
+        It might allow other AWS identities to escalate privileges.
+
+    Fix:
+        Where possible, restrict the access to only the required resources.
+        For example, instead of `Principal: "*"`, include a list of the roles that need access.
+
+    Filters context:
+        | Parameter   | Type               | Description                                                    |
+        |:-----------:|:------------------:|:--------------------------------------------------------------:|
+        |`config`     | `str`              | `config` variable available inside the rule                    |
+        |`extras`     | `str`              | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`              | ID used in CloudFormation to refer the resource being analysed |
+        |`resource`   | `S3BucketPolicy`   | Resource that is being addressed                               |
+        |`statement`  | `Statement`        | Statement being checked found in the Resource                  |
+        |`principal`  | `str`              | AWS Principal being checked found in the statement             |
+        |`account_id` | `str`              | Account ID found in the principal                              |
+    """
+
+    def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
+        result = Result()
+        for logical_id, resource in cfmodel.Resources.items():
+            if isinstance(resource, KMSKey):
+                # Ignoring KMSKey because there's already a rule for it `KMSKeyWildcardPrincipalRule`
+                continue
+            if isinstance(resource, IAMRole):
+                # Checking the `AssumeRolePolicyDocument` for IAM Roles
+                self.check_for_wildcards(result, logical_id, resource.Properties.AssumeRolePolicyDocument, extras)
+            for policy in resource.policy_documents:
+                self.check_for_wildcards(result, logical_id, policy.policy_document, extras)
+
+        return result
+
+
+class GenericResourcePartialWildcardPrincipalRule(GenericResourceWildcardPrincipalRule):
+    """
+    Checks for any wildcard or account-wide principals defined in any statements. This rule will flag
+    as non-compliant any principals where `root` or `*` are included at the end of the value, for
+    example, `arn:aws:iam:12345:12345*`.
+
+    Risk:
+        It might allow other AWS identities or the root access of the account to escalate privileges.
+
+    Fix:
+        Where possible, restrict the access to only the required resources.
+        For example, instead of `Principal: "*"`, include a list of the roles that need access.
+
+    Filters context:
+        | Parameter   | Type               | Description                                                    |
+        |:-----------:|:------------------:|:--------------------------------------------------------------:|
+        |`config`     | `str`              | `config` variable available inside the rule                    |
+        |`extras`     | `str`              | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`              | ID used in CloudFormation to refer the resource being analysed |
+        |`resource`   | `S3BucketPolicy`   | Resource that is being addressed                               |
+        |`statement`  | `Statement`        | Statement being checked found in the Resource                  |
+        |`principal`  | `str`              | AWS Principal being checked found in the statement             |
+        |`account_id` | `str`              | Account ID found in the principal                              |
+    """
+
+    REASON_WILDCARD_PRINCIPAL = (
+        "{} should not allow wildcard in principals or account-wide principals (principal: '{}')"
+    )
+    RISK_VALUE = RuleRisk.MEDIUM
+    FULL_REGEX = REGEX_PARTIAL_WILDCARD_PRINCIPAL
+
+
+class GenericResourceFullWildcardPrincipalRule(GenericResourceWildcardPrincipalRule):
+    """
+    Checks for any wildcard principal defined in any statement.
+
+    Risk:
+        It might allow other AWS identities to escalate privileges.
+
+    Fix:
+        Where possible, restrict the access to only the required resources.
+        For example, instead of `Principal: "*"`, include a list of the roles that need access.
+
+    Filters context:
+        | Parameter   | Type               | Description                                                    |
+        |:-----------:|:------------------:|:--------------------------------------------------------------:|
+        |`config`     | `str`              | `config` variable available inside the rule                    |
+        |`extras`     | `str`              | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`              | ID used in CloudFormation to refer the resource being analysed |
+        |`resource`   | `S3BucketPolicy`   | Resource that is being addressed                               |
+        |`statement`  | `Statement`        | Statement being checked found in the Resource                  |
+        |`principal`  | `str`              | AWS Principal being checked found in the statement             |
+        |`account_id` | `str`              | Account ID found in the principal                              |
+    """
+
+    RISK_VALUE = RuleRisk.HIGH

tests/rules/test_GenericResourceFullWildcardPrincipal.py

@@ -0,0 +1,53 @@
+import pytest
+
+from cfripper.model.enums import RuleGranularity, RuleMode, RuleRisk
+from cfripper.model.result import Failure
+from cfripper.rules import GenericResourceFullWildcardPrincipalRule
+from tests.utils import compare_lists_of_failures, get_cfmodel_from
+
+
+@pytest.fixture()
+def good_template():
+    return get_cfmodel_from("rules/FullWilcardPrincipalRule/good_template.json").resolve()
+
+
+@pytest.fixture()
+def bad_template():
+    return get_cfmodel_from("rules/FullWilcardPrincipalRule/bad_template.json").resolve()
+
+
+def test_no_failures_are_raised(good_template):
+    rule = GenericResourceFullWildcardPrincipalRule(None)
+    result = rule.invoke(good_template)
+
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])
+
+
+def test_failures_are_raised(bad_template):
+    rule = GenericResourceFullWildcardPrincipalRule(None)
+    result = rule.invoke(bad_template)
+
+    assert not result.valid
+    assert compare_lists_of_failures(
+        result.failures,
+        [
+            Failure(
+                rule_mode=RuleMode.BLOCKING,
+                rule="GenericResourceFullWildcardPrincipalRule",
+                reason="PolicyA should not allow wildcards in principals (principal: '*')",
+                granularity=RuleGranularity.RESOURCE,
+                risk_value=RuleRisk.HIGH,
+                actions=None,
+                resource_ids={"PolicyA"},
+            )
+        ],
+    )
+
+
+def test_rule_supports_filter_config(bad_template, default_allow_all_config):
+    rule = GenericResourceFullWildcardPrincipalRule(default_allow_all_config)
+    result = rule.invoke(bad_template)
+
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])

tests/rules/test_GenericResourcePartialWildcardPrincipal.py

@@ -0,0 +1,113 @@
+from pytest import fixture
+
+from cfripper.config.config import Config
+from cfripper.model.enums import RuleGranularity, RuleMode, RuleRisk
+from cfripper.model.result import Failure
+from cfripper.rules import GenericResourcePartialWildcardPrincipalRule
+from tests.utils import compare_lists_of_failures, get_cfmodel_from
+
+
+@fixture()
+def good_template():
+    return get_cfmodel_from("rules/PartialWildcardPrincipalRule/good_template.json").resolve()
+
+
+@fixture()
+def bad_template():
+    return get_cfmodel_from("rules/PartialWildcardPrincipalRule/bad_template.json").resolve()
+
+
+@fixture()
+def intra_account_root_access():
+    return get_cfmodel_from("rules/PartialWildcardPrincipalRule/intra_account_root_access.yml").resolve()
+
+
+@fixture()
+def aws_elb_allow_template():
+    return get_cfmodel_from("rules/PartialWildcardPrincipalRule/aws_elb_template.yml").resolve(
+        extra_params={"AWS::Region": "ap-southeast-1"}
+    )
+
+
+def test_no_failures_are_raised(good_template):
+    rule = GenericResourcePartialWildcardPrincipalRule(None)
+    result = rule.invoke(good_template)
+
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])
+
+
+def test_failures_are_raised(bad_template):
+    rule = GenericResourcePartialWildcardPrincipalRule(None)
+    result = rule.invoke(bad_template)
+
+    assert not result.valid
+    assert compare_lists_of_failures(
+        result.failures,
+        [
+            Failure(
+                granularity=RuleGranularity.RESOURCE,
+                reason="PolicyA should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123445:12345*')",
+                risk_value=RuleRisk.MEDIUM,
+                rule="GenericResourcePartialWildcardPrincipalRule",
+                rule_mode=RuleMode.BLOCKING,
+                actions=None,
+                resource_ids={"PolicyA"},
+            ),
+            Failure(
+                granularity=RuleGranularity.RESOURCE,
+                reason="PolicyA should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123445:root')",
+                risk_value=RuleRisk.MEDIUM,
+                rule="GenericResourcePartialWildcardPrincipalRule",
+                rule_mode=RuleMode.BLOCKING,
+                actions=None,
+                resource_ids={"PolicyA"},
+            ),
+        ],
+    )
+
+
+def test_failures_for_correct_account_ids(intra_account_root_access):
+    rule = GenericResourcePartialWildcardPrincipalRule(Config(aws_account_id="123456789012"))
+    result = rule.invoke(intra_account_root_access)
+
+    assert not result.valid
+    assert compare_lists_of_failures(
+        result.failures,
+        [
+            Failure(
+                granularity=RuleGranularity.RESOURCE,
+                reason="AccLoadBalancerAccessLogBucketPolicy should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789012:root')",
+                risk_value=RuleRisk.MEDIUM,
+                rule="GenericResourcePartialWildcardPrincipalRule",
+                rule_mode=RuleMode.BLOCKING,
+                actions=None,
+                resource_ids={"AccLoadBalancerAccessLogBucketPolicy"},
+            ),
+            Failure(
+                granularity=RuleGranularity.RESOURCE,
+                reason="AccLoadBalancerAccessLogBucketPolicy should not allow wildcard in principals or account-wide principals (principal: '987654321012')",
+                risk_value=RuleRisk.MEDIUM,
+                rule="GenericResourcePartialWildcardPrincipalRule",
+                rule_mode=RuleMode.BLOCKING,
+                actions=None,
+                resource_ids={"AccLoadBalancerAccessLogBucketPolicy"},
+            ),
+        ],
+    )
+
+
+def test_aws_elb_allow_template(aws_elb_allow_template):
+    rule = GenericResourcePartialWildcardPrincipalRule(None)
+    result = rule.invoke(aws_elb_allow_template)
+
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])
+
+
+def test_rule_supports_filter_config(bad_template, default_allow_all_config):
+    rule = GenericResourcePartialWildcardPrincipalRule(default_allow_all_config)
+    result = rule.invoke(bad_template)
+
+    assert result.valid
+    assert compare_lists_of_failures(result.failures, [])