From 1f38a1205774a80ae60fd73298a3a9c4f1c6d9c9 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Mon, 9 Sep 2019 17:08:25 +0100 Subject: [PATCH] Fix behaviour of module_utils/ec2 compare_policies when dealing with bare bools and ints. (#61115) * module_utils/ec2: (unit tests) Move unit tests for module_utils/ec2.py into test/units/module_utils - compare_policies was refactored from s3_bucket - "ec2_utils" doesn't seem to have ever existed * module_utils/ec2: (unit tests) Add unit test for comparing quoted and unquoted bools and numbers within policies As per https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_grammar.html "Values are enclosed in quotation marks. Quotation marks are optional for numeric and Boolean values." * module_utils/ec2: Explicitly convert bools and ints to strings when comparing policies See also: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_grammar.html --- ...004-compare_policies-convert-to-string.yml | 2 + lib/ansible/module_utils/ec2.py | 6 + test/sanity/ignore.txt | 6 +- test/units/module_utils/test_ec2.py | 244 ++++++++++++++++++ .../modules/cloud/amazon/test_ec2_utils.py | 12 - .../modules/cloud/amazon/test_s3_bucket.py | 164 ------------ 6 files changed, 254 insertions(+), 180 deletions(-) create mode 100644 changelogs/fragments/61004-compare_policies-convert-to-string.yml create mode 100644 test/units/module_utils/test_ec2.py delete mode 100644 test/units/modules/cloud/amazon/test_ec2_utils.py delete mode 100644 test/units/modules/cloud/amazon/test_s3_bucket.py diff --git a/changelogs/fragments/61004-compare_policies-convert-to-string.yml b/changelogs/fragments/61004-compare_policies-convert-to-string.yml new file mode 100644 index 00000000000..5d45f76fa92 --- /dev/null +++ b/changelogs/fragments/61004-compare_policies-convert-to-string.yml @@ -0,0 +1,2 @@ +bugfixes: +- compare_policies() Explicitly convert bools and ints to strings when comparing AWS IAM policies diff --git a/lib/ansible/module_utils/ec2.py b/lib/ansible/module_utils/ec2.py index d8d986dc903..0efacd08b27 100644 --- a/lib/ansible/module_utils/ec2.py +++ b/lib/ansible/module_utils/ec2.py @@ -554,6 +554,12 @@ def _hashable_policy(policy, policy_list): ('Version', (u'2012-10-17',)))] """ + # Amazon will automatically convert bool and int to strings for us + if isinstance(policy, bool): + return tuple([str(policy).lower()]) + elif isinstance(policy, int): + return tuple([str(policy)]) + if isinstance(policy, list): for each in policy: tupleified = _hashable_policy(each, []) diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index 24d31f0d8e3..517c74efb51 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -6191,6 +6191,8 @@ test/units/module_utils/test_database.py future-import-boilerplate test/units/module_utils/test_database.py metaclass-boilerplate test/units/module_utils/test_distro.py future-import-boilerplate test/units/module_utils/test_distro.py metaclass-boilerplate +test/units/module_utils/test_ec2.py future-import-boilerplate +test/units/module_utils/test_ec2.py metaclass-boilerplate test/units/module_utils/test_hetzner.py future-import-boilerplate test/units/module_utils/test_hetzner.py metaclass-boilerplate test/units/module_utils/test_kubevirt.py future-import-boilerplate @@ -6218,8 +6220,6 @@ test/units/modules/cloud/amazon/test_data_pipeline.py future-import-boilerplate test/units/modules/cloud/amazon/test_data_pipeline.py metaclass-boilerplate test/units/modules/cloud/amazon/test_ec2_group.py future-import-boilerplate test/units/modules/cloud/amazon/test_ec2_group.py metaclass-boilerplate -test/units/modules/cloud/amazon/test_ec2_utils.py future-import-boilerplate -test/units/modules/cloud/amazon/test_ec2_utils.py metaclass-boilerplate test/units/modules/cloud/amazon/test_ec2_vpc_nat_gateway.py future-import-boilerplate test/units/modules/cloud/amazon/test_ec2_vpc_nat_gateway.py metaclass-boilerplate test/units/modules/cloud/amazon/test_ec2_vpc_nat_gateway.py pylint:blacklisted-name @@ -6236,8 +6236,6 @@ test/units/modules/cloud/amazon/test_redshift_cross_region_snapshots.py future-i test/units/modules/cloud/amazon/test_redshift_cross_region_snapshots.py metaclass-boilerplate test/units/modules/cloud/amazon/test_route53_zone.py future-import-boilerplate test/units/modules/cloud/amazon/test_route53_zone.py metaclass-boilerplate -test/units/modules/cloud/amazon/test_s3_bucket.py future-import-boilerplate -test/units/modules/cloud/amazon/test_s3_bucket.py metaclass-boilerplate test/units/modules/cloud/amazon/test_s3_bucket_notification.py future-import-boilerplate test/units/modules/cloud/amazon/test_s3_bucket_notification.py metaclass-boilerplate test/units/modules/cloud/cloudstack/test_cs_traffic_type.py future-import-boilerplate diff --git a/test/units/module_utils/test_ec2.py b/test/units/module_utils/test_ec2.py new file mode 100644 index 00000000000..eb6e956288c --- /dev/null +++ b/test/units/module_utils/test_ec2.py @@ -0,0 +1,244 @@ +# (c) 2017 Red Hat Inc. +# +# This file is part of Ansible +# +# Ansible is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# Ansible is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Ansible. If not, see . + +# (c) 2017 Red Hat Inc. +import unittest + +from ansible.module_utils.ec2 import map_complex_type, compare_policies + + +class Ec2Utils(unittest.TestCase): + + def setUp(self): + # A pair of simple IAM Trust relationships using bools, the first a + # native bool the second a quoted string + self.bool_policy_bool = { + 'Version': '2012-10-17', + 'Statement': [ + { + "Action": "sts:AssumeRole", + "Condition": { + "Bool": {"aws:MultiFactorAuthPresent": True} + }, + "Effect": "Allow", + "Principal": {"AWS": "arn:aws:iam::XXXXXXXXXXXX:root"}, + "Sid": "AssumeRoleWithBoolean" + } + ] + } + + self.bool_policy_string = { + 'Version': '2012-10-17', + 'Statement': [ + { + "Action": "sts:AssumeRole", + "Condition": { + "Bool": {"aws:MultiFactorAuthPresent": "true"} + }, + "Effect": "Allow", + "Principal": {"AWS": "arn:aws:iam::XXXXXXXXXXXX:root"}, + "Sid": "AssumeRoleWithBoolean" + } + ] + } + + # A pair of simple bucket policies using numbers, the first a + # native int the second a quoted string + self.numeric_policy_number = { + 'Version': '2012-10-17', + 'Statement': [ + { + "Action": "s3:ListBucket", + "Condition": { + "NumericLessThanEquals": {"s3:max-keys": 15} + }, + "Effect": "Allow", + "Resource": "arn:aws:s3:::examplebucket", + "Sid": "s3ListBucketWithNumericLimit" + } + ] + } + + self.numeric_policy_string = { + 'Version': '2012-10-17', + 'Statement': [ + { + "Action": "s3:ListBucket", + "Condition": { + "NumericLessThanEquals": {"s3:max-keys": "15"} + }, + "Effect": "Allow", + "Resource": "arn:aws:s3:::examplebucket", + "Sid": "s3ListBucketWithNumericLimit" + } + ] + } + + self.small_policy_one = { + 'Version': '2012-10-17', + 'Statement': [ + { + 'Action': 's3:PutObjectAcl', + 'Sid': 'AddCannedAcl2', + 'Resource': 'arn:aws:s3:::test_policy/*', + 'Effect': 'Allow', + 'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']} + } + ] + } + + # The same as small_policy_one, except the single resource is in a list and the contents of Statement are jumbled + self.small_policy_two = { + 'Version': '2012-10-17', + 'Statement': [ + { + 'Effect': 'Allow', + 'Action': 's3:PutObjectAcl', + 'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']}, + 'Resource': ['arn:aws:s3:::test_policy/*'], + 'Sid': 'AddCannedAcl2' + } + ] + } + + self.larger_policy_one = { + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "Test", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::XXXXXXXXXXXX:user/testuser1", + "arn:aws:iam::XXXXXXXXXXXX:user/testuser2" + ] + }, + "Action": "s3:PutObjectAcl", + "Resource": "arn:aws:s3:::test_policy/*" + }, + { + "Effect": "Allow", + "Principal": { + "AWS": "arn:aws:iam::XXXXXXXXXXXX:user/testuser2" + }, + "Action": [ + "s3:PutObject", + "s3:PutObjectAcl" + ], + "Resource": "arn:aws:s3:::test_policy/*" + } + ] + } + + # The same as larger_policy_one, except having a list of length 1 and jumbled contents + self.larger_policy_two = { + "Version": "2012-10-17", + "Statement": [ + { + "Principal": { + "AWS": ["arn:aws:iam::XXXXXXXXXXXX:user/testuser2"] + }, + "Effect": "Allow", + "Resource": "arn:aws:s3:::test_policy/*", + "Action": [ + "s3:PutObject", + "s3:PutObjectAcl" + ] + }, + { + "Action": "s3:PutObjectAcl", + "Principal": { + "AWS": [ + "arn:aws:iam::XXXXXXXXXXXX:user/testuser1", + "arn:aws:iam::XXXXXXXXXXXX:user/testuser2" + ] + }, + "Sid": "Test", + "Resource": "arn:aws:s3:::test_policy/*", + "Effect": "Allow" + } + ] + } + + # Different than larger_policy_two: a different principal is given + self.larger_policy_three = { + "Version": "2012-10-17", + "Statement": [ + { + "Principal": { + "AWS": ["arn:aws:iam::XXXXXXXXXXXX:user/testuser2"] + }, + "Effect": "Allow", + "Resource": "arn:aws:s3:::test_policy/*", + "Action": [ + "s3:PutObject", + "s3:PutObjectAcl"] + }, + { + "Action": "s3:PutObjectAcl", + "Principal": { + "AWS": [ + "arn:aws:iam::XXXXXXXXXXXX:user/testuser1", + "arn:aws:iam::XXXXXXXXXXXX:user/testuser3" + ] + }, + "Sid": "Test", + "Resource": "arn:aws:s3:::test_policy/*", + "Effect": "Allow" + } + ] + } + + def test_map_complex_type_over_dict(self): + complex_type = {'minimum_healthy_percent': "75", 'maximum_percent': "150"} + type_map = {'minimum_healthy_percent': 'int', 'maximum_percent': 'int'} + complex_type_mapped = map_complex_type(complex_type, type_map) + complex_type_expected = {'minimum_healthy_percent': 75, 'maximum_percent': 150} + self.assertEqual(complex_type_mapped, complex_type_expected) + + def test_compare_small_policies_without_differences(self): + """ Testing two small policies which are identical except for: + * The contents of the statement are in different orders + * The second policy contains a list of length one whereas in the first it is a string + """ + self.assertFalse(compare_policies(self.small_policy_one, self.small_policy_two)) + + def test_compare_large_policies_without_differences(self): + """ Testing two larger policies which are identical except for: + * The statements are in different orders + * The contents of the statements are also in different orders + * The second contains a list of length one for the Principal whereas in the first it is a string + """ + self.assertFalse(compare_policies(self.larger_policy_one, self.larger_policy_two)) + + def test_compare_larger_policies_with_difference(self): + """ Testing two larger policies which are identical except for: + * one different principal + """ + self.assertTrue(compare_policies(self.larger_policy_two, self.larger_policy_three)) + + def test_compare_smaller_policy_with_larger(self): + """ Testing two policies of different sizes """ + self.assertTrue(compare_policies(self.larger_policy_one, self.small_policy_one)) + + def test_compare_boolean_policy_bool_and_string_are_equal(self): + """ Testing two policies one using a quoted boolean, the other a bool """ + self.assertFalse(compare_policies(self.bool_policy_string, self.bool_policy_bool)) + + def test_compare_numeric_policy_number_and_string_are_equal(self): + """ Testing two policies one using a quoted number, the other an int """ + self.assertFalse(compare_policies(self.numeric_policy_string, self.numeric_policy_number)) diff --git a/test/units/modules/cloud/amazon/test_ec2_utils.py b/test/units/modules/cloud/amazon/test_ec2_utils.py deleted file mode 100644 index bceb12126e2..00000000000 --- a/test/units/modules/cloud/amazon/test_ec2_utils.py +++ /dev/null @@ -1,12 +0,0 @@ -import unittest - -from ansible.module_utils.ec2 import map_complex_type - - -class Ec2Utils(unittest.TestCase): - def test_map_complex_type_over_dict(self): - complex_type = {'minimum_healthy_percent': "75", 'maximum_percent': "150"} - type_map = {'minimum_healthy_percent': 'int', 'maximum_percent': 'int'} - complex_type_mapped = map_complex_type(complex_type, type_map) - complex_type_expected = {'minimum_healthy_percent': 75, 'maximum_percent': 150} - self.assertEqual(complex_type_mapped, complex_type_expected) diff --git a/test/units/modules/cloud/amazon/test_s3_bucket.py b/test/units/modules/cloud/amazon/test_s3_bucket.py deleted file mode 100644 index 53975e3c7fc..00000000000 --- a/test/units/modules/cloud/amazon/test_s3_bucket.py +++ /dev/null @@ -1,164 +0,0 @@ -# (c) 2017 Red Hat Inc. -# -# This file is part of Ansible -# -# Ansible is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Ansible is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Ansible. If not, see . - -# (c) 2017 Red Hat Inc. - -from ansible.modules.cloud.amazon.s3_bucket import compare_policies - -small_policy_one = { - 'Version': '2012-10-17', - 'Statement': [ - { - 'Action': 's3:PutObjectAcl', - 'Sid': 'AddCannedAcl2', - 'Resource': 'arn:aws:s3:::test_policy/*', - 'Effect': 'Allow', - 'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']} - } - ] -} - -# The same as small_policy_one, except the single resource is in a list and the contents of Statement are jumbled -small_policy_two = { - 'Version': '2012-10-17', - 'Statement': [ - { - 'Effect': 'Allow', - 'Action': 's3:PutObjectAcl', - 'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']}, - 'Resource': ['arn:aws:s3:::test_policy/*'], - 'Sid': 'AddCannedAcl2' - } - ] -} - -larger_policy_one = { - "Version": "2012-10-17", - "Statement": [ - { - "Sid": "Test", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::XXXXXXXXXXXX:user/testuser1", - "arn:aws:iam::XXXXXXXXXXXX:user/testuser2" - ] - }, - "Action": "s3:PutObjectAcl", - "Resource": "arn:aws:s3:::test_policy/*" - }, - { - "Effect": "Allow", - "Principal": { - "AWS": "arn:aws:iam::XXXXXXXXXXXX:user/testuser2" - }, - "Action": [ - "s3:PutObject", - "s3:PutObjectAcl" - ], - "Resource": "arn:aws:s3:::test_policy/*" - } - ] -} - -# The same as larger_policy_one, except having a list of length 1 and jumbled contents -larger_policy_two = { - "Version": "2012-10-17", - "Statement": [ - { - "Principal": { - "AWS": ["arn:aws:iam::XXXXXXXXXXXX:user/testuser2"] - }, - "Effect": "Allow", - "Resource": "arn:aws:s3:::test_policy/*", - "Action": [ - "s3:PutObject", - "s3:PutObjectAcl" - ] - }, - { - "Action": "s3:PutObjectAcl", - "Principal": { - "AWS": [ - "arn:aws:iam::XXXXXXXXXXXX:user/testuser1", - "arn:aws:iam::XXXXXXXXXXXX:user/testuser2" - ] - }, - "Sid": "Test", - "Resource": "arn:aws:s3:::test_policy/*", - "Effect": "Allow" - } - ] -} - -# Different than larger_policy_two: a different principal is given -larger_policy_three = { - "Version": "2012-10-17", - "Statement": [ - { - "Principal": { - "AWS": ["arn:aws:iam::XXXXXXXXXXXX:user/testuser2"] - }, - "Effect": "Allow", - "Resource": "arn:aws:s3:::test_policy/*", - "Action": [ - "s3:PutObject", - "s3:PutObjectAcl"] - }, - { - "Action": "s3:PutObjectAcl", - "Principal": { - "AWS": [ - "arn:aws:iam::XXXXXXXXXXXX:user/testuser1", - "arn:aws:iam::XXXXXXXXXXXX:user/testuser3" - ] - }, - "Sid": "Test", - "Resource": "arn:aws:s3:::test_policy/*", - "Effect": "Allow" - } - ] -} - - -def test_compare_small_policies_without_differences(): - """ Testing two small policies which are identical except for: - * The contents of the statement are in different orders - * The second policy contains a list of length one whereas in the first it is a string - """ - assert compare_policies(small_policy_one, small_policy_two) is False - - -def test_compare_large_policies_without_differences(): - """ Testing two larger policies which are identical except for: - * The statements are in different orders - * The contents of the statements are also in different orders - * The second contains a list of length one for the Principal whereas in the first it is a string - """ - assert compare_policies(larger_policy_one, larger_policy_two) is False - - -def test_compare_larger_policies_with_difference(): - """ Testing two larger policies which are identical except for: - * one different principal - """ - assert compare_policies(larger_policy_two, larger_policy_three) - - -def test_compare_smaller_policy_with_larger(): - """ Testing two policies of different sizes """ - assert compare_policies(larger_policy_one, small_policy_one)