From d1cfeb2bd9d5e8b2a7f80cde32d9130e9c079d0e Mon Sep 17 00:00:00 2001 From: Kevin DeJong Date: Sat, 20 Jan 2024 08:48:27 -0800 Subject: [PATCH] Add rule E3019 to validate primaryIdentifiers are unique (#3023) --- .../rules/resources/PrimaryIdentifiers.py | 156 ++++++++++++++++++ .../bad/resources/primary_identifiers.yaml | 154 +++++++++++++++++ .../good/resources/primary_identifiers.yaml | 99 +++++++++++ .../resources/test_primary_identifiers.py | 30 ++++ 4 files changed, 439 insertions(+) create mode 100644 src/cfnlint/rules/resources/PrimaryIdentifiers.py create mode 100644 test/fixtures/templates/bad/resources/primary_identifiers.yaml create mode 100644 test/fixtures/templates/good/resources/primary_identifiers.yaml create mode 100644 test/unit/rules/resources/test_primary_identifiers.py diff --git a/src/cfnlint/rules/resources/PrimaryIdentifiers.py b/src/cfnlint/rules/resources/PrimaryIdentifiers.py new file mode 100644 index 0000000000..7bc91501af --- /dev/null +++ b/src/cfnlint/rules/resources/PrimaryIdentifiers.py @@ -0,0 +1,156 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" +from copy import copy +from dataclasses import dataclass, field +from typing import Any, Dict, List, Set + +from cfnlint.jsonschema._utils import equal +from cfnlint.rules import CloudFormationLintRule, RuleMatch +from cfnlint.schema import PROVIDER_SCHEMA_MANAGER +from cfnlint.schema.exceptions import ResourceNotFoundError + + +@dataclass +class _Seen: + conditions: Dict[str, Set[bool]] = field(init=True, default_factory=dict) + resources: Set[str] = field(init=True, default_factory=set) + instance: Dict[str, Any] = field(init=True, default_factory=dict) + + +class PrimaryIdentifiers(CloudFormationLintRule): + id = "E3019" + shortdesc = "Unique resource and parameter names" + description = "All resources and parameters must have unique names" + source_url = "https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html" + tags = ["parameters", "resources"] + + def _merge_conditions( + self, conditions1: Dict[str, Set[bool]], conditions2: Dict[str, Set[bool]] + ): + for k2, v2 in conditions2.items(): + if k2 in conditions1: + if v2 != conditions1[k2]: + raise ValueError("Condition mismatch") + conditions1[k2] = v2 + + return conditions1 + + def _validate_resource_type_uniqueness(self, cfn, resource_type, ids): + matches = [] + seens: List[_Seen] = [] + for resource_name, resource_attributes in cfn.get_resources( + resource_type + ).items(): + condition = resource_attributes.get("Condition") + properties = resource_attributes.get("Properties", {}) + path = ["Resources", resource_name, "Properties"] + for scenario in cfn.get_object_without_conditions(properties): + object = scenario.get("Object") + conditions = scenario.get("Scenario") + if conditions is None: + conditions = {} + # we need to change this to a set for implies to work + conditions = {k: set([v]) for k, v in conditions.items()} + + # if there is a resource level condition lets see if its valid + # and add it to the scenario as needed + if condition: + if scenario: + if not cfn.conditions.check_implies(scenario, condition): + continue + conditions[condition] = set([True]) + + # validate object is an object just in case it was emptied out + # from the condition (Ref: AWS::NoValue) + if not isinstance(object, dict): + continue + + # get_object_without_conditions will return all elements + # so we are going to filter down to the ids that we care about + identifiers = {id: object.get(id) for id in ids} + # if any of the identifiers are None that means the service + # will define it and we can skip this test + if any(identifier is None for identifier in identifiers.values()): + continue + + # build the element + element = _Seen(conditions, set([resource_name]), identifiers) + + found = False + for seen in seens: + if equal(seen.instance, element.instance): + try: + merged_conditions = self._merge_conditions( + copy(seen.conditions), copy(element.conditions) + ) + # if conditions are empty we found it + if not merged_conditions: + found = True + seen.resources.add(resource_name) + for _ in cfn.conditions.build_scenarios(merged_conditions): + seen.resources.add(resource_name) + # because the conditions could be unique + # we will validate conditions being equal + # and only say found if they match + if equal(element.conditions, seen.conditions): + found = True + break + + except ValueError: + continue + + # add anything not already Found to the seens element + if not found: + seens.append(element) + + for seen in seens: + # build errors for any seen items with more than 1 resource + if len(seen.resources) > 1: + for resource in seen.resources: + path = ["Resources", resource, "Properties"] + if len(seen.instance) == 1: + key = list(seen.instance.keys())[0] + path.append(key) + + message = ( + f"Primary identifiers {seen.instance!r} should " + "have unique values across the resources " + f"{seen.resources!r}" + ) + + matches.append(RuleMatch(path, message)) + + return matches + + def match(self, cfn): + tS = set() + for _, resource_properties in cfn.get_resources().items(): + tS.add(resource_properties.get("Type")) + + matches = [] + for t in tS: + try: + schema = PROVIDER_SCHEMA_MANAGER.get_resource_schema(cfn.regions[0], t) + + # we are worried about primary identifiers that can be set + # by the customer so if any primary identifiers are read + # only we have to skip evaluation + primary_ids = schema.schema.get("primaryIdentifier", []) + read_only_ids = schema.schema.get("readOnlyProperties", []) + + if any(id in read_only_ids for id in primary_ids): + continue + + primary_ids = [id.replace("/properties/", "") for id in primary_ids] + # most primaryIdentifiers are at the root level. At this time + # we can only validate if they are at the root of Properties + if any("/" in id for id in primary_ids): + continue + matches.extend( + self._validate_resource_type_uniqueness(cfn, t, primary_ids) + ) + except ResourceNotFoundError: + continue + return matches diff --git a/test/fixtures/templates/bad/resources/primary_identifiers.yaml b/test/fixtures/templates/bad/resources/primary_identifiers.yaml new file mode 100644 index 0000000000..e6cbbdd82b --- /dev/null +++ b/test/fixtures/templates/bad/resources/primary_identifiers.yaml @@ -0,0 +1,154 @@ +Conditions: + IsUsEast1: !Equals [!Ref 'AWS::Region', 'us-east-1'] + NotUsEast1: !Not [!Condition IsUsEast1] +Resources: + RootRole: + Type: 'AWS::IAM::Role' + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: root + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + RootRole2: + Type: 'AWS::IAM::Role' + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: root + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + RootRole3: + Type: 'AWS::IAM::Role' + Condition: IsUsEast1 + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: useast1 + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + RootRole4: + Type: 'AWS::IAM::Role' + Condition: NotUsEast1 + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: useast1 + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + + RootRole5: + Type: 'AWS::IAM::Role' + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: !If [IsUsEast1, useast1, !Ref 'AWS::NoValue'] + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + RootRole6: + Type: 'AWS::IAM::Role' + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: !If [IsUsEast1, !Ref 'AWS::NoValue', useast1] + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + Bucket1: + Type: 'AWS::S3::Bucket' + Properties: + Fn::If: + - IsUsEast1 + - BucketName: us-east-1 + - !Ref 'AWS::NoValue' + Bucket2: + Type: 'AWS::S3::Bucket' + Properties: + BucketName: + Fn::If: + - IsUsEast1 + - us-east-1 + - !Ref 'AWS::NoValue' diff --git a/test/fixtures/templates/good/resources/primary_identifiers.yaml b/test/fixtures/templates/good/resources/primary_identifiers.yaml new file mode 100644 index 0000000000..23d952e640 --- /dev/null +++ b/test/fixtures/templates/good/resources/primary_identifiers.yaml @@ -0,0 +1,99 @@ +Conditions: + IsUsEast1: !Equals [!Ref 'AWS::Region', 'us-east-1'] + NotUsEast1: !Not [!Condition IsUsEast1] +Resources: + RootRole: + Type: 'AWS::IAM::Role' + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: root + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + RootRole3: + Type: 'AWS::IAM::Role' + Condition: IsUsEast1 + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: useast1 + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + RootRole4: + Type: 'AWS::IAM::Role' + Condition: NotUsEast1 + Properties: + AssumeRolePolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Principal: + Service: + - ec2.amazonaws.com + Action: + - 'sts:AssumeRole' + Path: / + RoleName: useast1 + Policies: + - PolicyName: root + PolicyDocument: + Version: "2012-10-17" + Statement: + - Effect: Allow + Action: '*' + Resource: '*' + + Bucket1: + Type: 'AWS::S3::Bucket' + Properties: + Fn::If: + - IsUsEast1 + - BucketName: us-east-1 + - !Ref 'AWS::NoValue' + Bucket2: + Type: 'AWS::S3::Bucket' + + # primaryIdentifier. ID is read only so FunctionName + # being the same should pass + # "/properties/FunctionName", + # "/properties/Id" + FunctionPermission1: + Type: 'AWS::Lambda::Permission' + Properties: + FunctionName: !Ref AWS::Region + Action: lambda:InvokeFunction + Principal: sns.amazonaws.com + FunctionPermission2: + Type: 'AWS::Lambda::Permission' + Properties: + FunctionName: !Ref AWS::Region + Action: lambda:InvokeFunction + Principal: sns.amazonaws.com diff --git a/test/unit/rules/resources/test_primary_identifiers.py b/test/unit/rules/resources/test_primary_identifiers.py new file mode 100644 index 0000000000..1bc3dfd1b7 --- /dev/null +++ b/test/unit/rules/resources/test_primary_identifiers.py @@ -0,0 +1,30 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" +from test.unit.rules import BaseRuleTestCase + +# ruff: noqa: E501 +from cfnlint.rules.resources.PrimaryIdentifiers import PrimaryIdentifiers + + +class TestPrimaryIdentifiers(BaseRuleTestCase): + """Test base template""" + + def setUp(self): + """Setup""" + super(TestPrimaryIdentifiers, self).setUp() + self.collection.register(PrimaryIdentifiers()) + self.success_templates = [ + "test/fixtures/templates/good/resources/primary_identifiers.yaml", + ] + + def test_file_positive(self): + """Test Positive""" + self.helper_file_positive() + + def test_file_negative_alias(self): + """Test failure""" + self.helper_file_negative( + "test/fixtures/templates/bad/resources/primary_identifiers.yaml", 8 + )