diff --git a/src/cfnlint/rules/resources/iam/Sid.py b/src/cfnlint/rules/resources/iam/Sid.py new file mode 100644 index 0000000000..3444b9410d --- /dev/null +++ b/src/cfnlint/rules/resources/iam/Sid.py @@ -0,0 +1,81 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" +import re +from cfnlint.rules import CloudFormationLintRule +from cfnlint.rules import RuleMatch + + +class Sid(CloudFormationLintRule): + """Check if IAM Policy Sid is correct""" + id = 'W2512' + shortdesc = 'Check IAM Resource Policies Sid syntax' + description = 'See if the elements inside an IAM Resource policy are configured correctly.' + source_url = 'https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_grammar.html' + tags = ['properties', 'iam'] + + def __init__(self): + """Init""" + super(Sid, self).__init__() + self.idp_and_keys = { + 'AWS::IAM::Group': 'Policies', + 'AWS::IAM::ManagedPolicy': 'PolicyDocument', + 'AWS::IAM::Policy': 'PolicyDocument', + 'AWS::IAM::Role': 'Policies', + 'AWS::IAM::User': 'Policies', + } + for resource_type in self.idp_and_keys: + self.resource_property_types.append(resource_type) + self.sid_regex = re.compile('^[A-Za-z0-9]*$') + + def check_policy_document(self, value, path): + """Check policy document""" + sids = [] # sids must be unique + matches = [] + + if not isinstance(value, dict): + return matches + + for e_v, e_p in value.items_safe(path[:]): + for p_vs, p_p in e_v.items_safe(e_p[:]): + statements = p_vs.get('Statement', []) + if 'get' in dir(statements): + statements = [statements] + for statement in statements: + if 'Sid' in statement: + sid = statement.get('Sid') + if self.sid_regex.search(sid) is None: + message = 'basic alphanumeric characters (A-Z,a-z,0-9) are the only allowed characters in the Sid value' + matches.append(RuleMatch(p_p + ['Sid'], message)) + if sid in sids: + message = 'IAM Sid values should be unique' + matches.append(RuleMatch(p_p + ['Sid'], message)) + else: + sids.append(sid) + + return matches + + def match_resource_properties(self, properties, resourcetype, path, cfn): + """Check CloudFormation Properties""" + matches = [] + + key = self.idp_and_keys.get(resourcetype) + + if key == 'Policies': + for index, policy in enumerate(properties.get(key, [])): + matches.extend( + cfn.check_value( + obj=policy, key='PolicyDocument', + path=path[:] + ['Policies', index], + check_value=self.check_policy_document, + )) + else: + matches.extend( + cfn.check_value( + obj=properties, key=key, + path=path[:], + check_value=self.check_policy_document + )) + + return matches diff --git a/test/fixtures/templates/bad/resources/iam/policy_sid.yaml b/test/fixtures/templates/bad/resources/iam/policy_sid.yaml new file mode 100644 index 0000000000..ad2f0fb7c4 --- /dev/null +++ b/test/fixtures/templates/bad/resources/iam/policy_sid.yaml @@ -0,0 +1,29 @@ +--- +AWSTemplateFormatVersion: "2010-09-09" +Description: > + Test bad Policy Version +Resources: + rIamRole: + Type: AWS::IAM::Role + Properties: + AssumeRolePolicyDocument: {} + Policies: + - PolicyName: String + PolicyDocument: + Version: "2012-10-17" + Statement: + - Sid: "invalid regex" + Effect: "Allow" + Action: + - "s3:ListBucket" + Resource: '*' + - Effect: "Allow" + Sid: "Sid1" + Action: + - "s3:GetBucketLocation" + Resource: '*' + - Effect: "Allow" + Sid: "Sid1" + Action: + - "s3:GetObject" + Resource: '*' diff --git a/test/fixtures/templates/bad/resources/iam/policy_sid_2.yaml b/test/fixtures/templates/bad/resources/iam/policy_sid_2.yaml new file mode 100644 index 0000000000..46fae38816 --- /dev/null +++ b/test/fixtures/templates/bad/resources/iam/policy_sid_2.yaml @@ -0,0 +1,19 @@ +--- +AWSTemplateFormatVersion: "2010-09-09" +Description: > + Test bad Policy Version +Resources: + rIamRole: + Type: AWS::IAM::Role + Properties: + AssumeRolePolicyDocument: {} + Policies: + - PolicyName: String + PolicyDocument: + Version: "2012-10-17" + Statement: + Sid: "invalid regex" + Effect: "Allow" + Action: + - "s3:ListBucket" + Resource: '*' diff --git a/test/fixtures/templates/good/resources/iam/policy.yaml b/test/fixtures/templates/good/resources/iam/policy.yaml index 052cd64ae7..e64cf54082 100644 --- a/test/fixtures/templates/good/resources/iam/policy.yaml +++ b/test/fixtures/templates/good/resources/iam/policy.yaml @@ -20,6 +20,11 @@ Resources: - "s3:ListBucket" - "s3:GetObject" Resource: '*' + - Effect: "Allow" + Sid: "FindMyBucket" + Action: + - "s3:GetBucketLocation" + Resource: '*' - Fn::If: - SomeCondition - Effect: "Allow" diff --git a/test/fixtures/templates/quickstart/nist_logging.yaml b/test/fixtures/templates/quickstart/nist_logging.yaml index 58544ccf4b..9acb2bfe3a 100644 --- a/test/fixtures/templates/quickstart/nist_logging.yaml +++ b/test/fixtures/templates/quickstart/nist_logging.yaml @@ -81,7 +81,7 @@ Resources: - ':s3:::' - Ref: rArchiveLogsBucket - /* - Sid: Enforce HTTPS Connections + Sid: EnforceHTTPSConnections - Action: s3:Delete* Effect: Deny Principal: '*' @@ -96,7 +96,7 @@ Resources: - ':s3:::' - Ref: rArchiveLogsBucket - /* - Sid: Restrict Delete* Actions + Sid: RestrictDeleteActions - Action: s3:PutObject Condition: StringNotEquals: @@ -286,7 +286,7 @@ Resources: - ':s3:::' - Ref: rCloudTrailBucket - /* - Sid: Enforce HTTPS Connections + Sid: EnforceHTTPSConnections - Action: s3:Delete* Effect: Deny Principal: '*' @@ -301,7 +301,7 @@ Resources: - ':s3:::' - Ref: rCloudTrailBucket - /* - Sid: Restrict Delete* Actions + Sid: RestrictDeleteActions - Action: s3:PutObject Condition: StringNotEquals: diff --git a/test/unit/module/config/test_config_mixin.py b/test/unit/module/config/test_config_mixin.py index dbe84bc0a8..6bc4fd92b7 100644 --- a/test/unit/module/config/test_config_mixin.py +++ b/test/unit/module/config/test_config_mixin.py @@ -158,4 +158,4 @@ def test_config_expand_ignore_templates(self, yaml_mock): # test defaults self.assertNotIn( 'test/fixtures/templates/bad/resources/iam/resource_policy.yaml', config.templates) - self.assertEqual(len(config.templates), 4) + self.assertEqual(len(config.templates), 6) diff --git a/test/unit/rules/resources/iam/test_iam_sid.py b/test/unit/rules/resources/iam/test_iam_sid.py new file mode 100644 index 0000000000..6e79442ad0 --- /dev/null +++ b/test/unit/rules/resources/iam/test_iam_sid.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 +from cfnlint.rules.resources.iam.Sid import Sid # pylint: disable=E0401 + + +class TestSid(BaseRuleTestCase): + """Test IAM Resource Policies""" + + def setUp(self): + """Setup""" + super(TestSid, self).setUp() + self.collection.register(Sid()) + self.success_templates = [ + #'test/fixtures/templates/good/resources/iam/resource_policy.yaml', + 'test/fixtures/templates/good/resources/iam/policy.yaml', + ] + + def test_file_positive(self): + """Test Positive""" + self.helper_file_positive() + + def test_file_negative(self): + """Test failure""" + self.helper_file_negative( + 'test/fixtures/templates/bad/resources/iam/policy_sid.yaml', 2) + self.helper_file_negative( + 'test/fixtures/templates/bad/resources/iam/policy_sid_2.yaml', 1)