From 33ec3403a6ef34066cac6a4e577e861f015aa161 Mon Sep 17 00:00:00 2001 From: John Newbigin Date: Tue, 1 Dec 2020 23:44:51 +1100 Subject: [PATCH 1/3] Add Sid validation --- src/cfnlint/rules/resources/iam/Sid.py | 97 +++++++++++++++++++ .../bad/resources/iam/policy_sid.yaml | 29 ++++++ .../bad/resources/iam/policy_sid_2.yaml | 19 ++++ .../templates/good/resources/iam/policy.yaml | 5 + .../templates/quickstart/nist_logging.yaml | 8 +- test/unit/module/config/test_config_mixin.py | 2 +- test/unit/rules/resources/iam/test_iam_sid.py | 30 ++++++ 7 files changed, 185 insertions(+), 5 deletions(-) create mode 100644 src/cfnlint/rules/resources/iam/Sid.py create mode 100644 test/fixtures/templates/bad/resources/iam/policy_sid.yaml create mode 100644 test/fixtures/templates/bad/resources/iam/policy_sid_2.yaml create mode 100644 test/unit/rules/resources/iam/test_iam_sid.py diff --git a/src/cfnlint/rules/resources/iam/Sid.py b/src/cfnlint/rules/resources/iam/Sid.py new file mode 100644 index 0000000000..1def5a7abe --- /dev/null +++ b/src/cfnlint/rules/resources/iam/Sid.py @@ -0,0 +1,97 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" +import re +from datetime import date +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.resources_and_keys = { + 'AWS::ECR::Repository': 'RepositoryPolicyText', + 'AWS::Elasticsearch::Domain': 'AccessPolicies', + 'AWS::KMS::Key': 'KeyPolicy', + 'AWS::S3::BucketPolicy': 'PolicyDocument', + 'AWS::SNS::TopicPolicy': 'PolicyDocument', + 'AWS::SQS::QueuePolicy': 'PolicyDocument', + } + 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.resources_and_keys: + self.resource_property_types.append(resource_type) + 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 = None + if resourcetype in self.resources_and_keys: + key = self.resources_and_keys.get(resourcetype) + else: + 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) From 05d350f3843465020c633a42698b1743206a986b Mon Sep 17 00:00:00 2001 From: John Newbigin Date: Wed, 2 Dec 2020 00:00:58 +1100 Subject: [PATCH 2/3] fix linting issues --- src/cfnlint/rules/resources/iam/Sid.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/cfnlint/rules/resources/iam/Sid.py b/src/cfnlint/rules/resources/iam/Sid.py index 1def5a7abe..3c2e8eb03f 100644 --- a/src/cfnlint/rules/resources/iam/Sid.py +++ b/src/cfnlint/rules/resources/iam/Sid.py @@ -3,7 +3,6 @@ SPDX-License-Identifier: MIT-0 """ import re -from datetime import date from cfnlint.rules import CloudFormationLintRule from cfnlint.rules import RuleMatch @@ -64,7 +63,6 @@ def check_policy_document(self, value, path): matches.append(RuleMatch(p_p + ['Sid'], message)) else: sids.append(sid) - return matches From 1fde08de75fd33f27409613843891d416658040f Mon Sep 17 00:00:00 2001 From: John Newbigin Date: Thu, 24 Dec 2020 00:03:03 +1100 Subject: [PATCH 3/3] Remove non-IAM resources which have different rules --- src/cfnlint/rules/resources/iam/Sid.py | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/cfnlint/rules/resources/iam/Sid.py b/src/cfnlint/rules/resources/iam/Sid.py index 3c2e8eb03f..3444b9410d 100644 --- a/src/cfnlint/rules/resources/iam/Sid.py +++ b/src/cfnlint/rules/resources/iam/Sid.py @@ -18,14 +18,6 @@ class Sid(CloudFormationLintRule): def __init__(self): """Init""" super(Sid, self).__init__() - self.resources_and_keys = { - 'AWS::ECR::Repository': 'RepositoryPolicyText', - 'AWS::Elasticsearch::Domain': 'AccessPolicies', - 'AWS::KMS::Key': 'KeyPolicy', - 'AWS::S3::BucketPolicy': 'PolicyDocument', - 'AWS::SNS::TopicPolicy': 'PolicyDocument', - 'AWS::SQS::QueuePolicy': 'PolicyDocument', - } self.idp_and_keys = { 'AWS::IAM::Group': 'Policies', 'AWS::IAM::ManagedPolicy': 'PolicyDocument', @@ -33,8 +25,6 @@ def __init__(self): 'AWS::IAM::Role': 'Policies', 'AWS::IAM::User': 'Policies', } - for resource_type in self.resources_and_keys: - self.resource_property_types.append(resource_type) for resource_type in self.idp_and_keys: self.resource_property_types.append(resource_type) self.sid_regex = re.compile('^[A-Za-z0-9]*$') @@ -70,11 +60,7 @@ def match_resource_properties(self, properties, resourcetype, path, cfn): """Check CloudFormation Properties""" matches = [] - key = None - if resourcetype in self.resources_and_keys: - key = self.resources_and_keys.get(resourcetype) - else: - key = self.idp_and_keys.get(resourcetype) + key = self.idp_and_keys.get(resourcetype) if key == 'Policies': for index, policy in enumerate(properties.get(key, [])):