Special case for "Principal": "*" in IAM policy comparison (ansible-c…

…ollections#153)

* Add test cases for ansible-collections/amazon.aws#149

* Treat {Principal: *} == {Principal: {AWS: *}}

* Make sure we don't update the policy we're working with

plugins/module_utils/ec2.py

@@ -633,7 +633,12 @@ def _hashable_policy(policy, policy_list):
         sorted_keys = list(policy.keys())
         sorted_keys.sort()
         for key in sorted_keys:
-            tupleified = _hashable_policy(policy[key], [])
+            element = policy[key]
+            # Special case defined in
+            # https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html
+            if key in ["NotPrincipal", "Principal"] and policy[key] == "*":
+                element = {"AWS": "*"}
+            tupleified = _hashable_policy(element, [])
             if isinstance(tupleified, list):
                 tupleified = tuple(tupleified)
             policy_list.append((key, tupleified))
@@ -675,6 +680,7 @@ def compare_policies(current_policy, new_policy, default_version="2008-10-17"):
     """ Compares the existing policy and the updated policy
         Returns True if there is a difference between policies.
     """
+    # https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_version.html
     if default_version:
         if isinstance(current_policy, dict):
             current_policy = current_policy.copy()

tests/integration/targets/s3_bucket/roles/s3_bucket/tasks/complex.yml

@@ -79,7 +79,7 @@
           - output is changed
           - output.policy.Statement[0].Action == 's3:GetObject'
           - output.policy.Statement[0].Effect == 'Deny'
-          - output.policy.Statement[0].Principal == '*'
+          - output.policy.Statement[0].Principal.AWS == '*'
           - output.policy.Statement[0].Resource == 'arn:aws:s3:::{{ bucket_name }}/*'
           - output.policy.Statement[0].Sid == 'AddPerm'
 

tests/integration/targets/s3_bucket/roles/s3_bucket/templates/policy-updated.json

@@ -4,7 +4,7 @@
     {
       "Sid":"AddPerm",
       "Effect":"Deny",
-      "Principal": "*",
+      "Principal": {"AWS": "*"},
       "Action":["s3:GetObject"],
       "Resource":["arn:aws:s3:::{{bucket_name}}/*"]
     }

tests/unit/module_utils/ec2/test_compare_policies.py

@@ -0,0 +1,341 @@
+# (c) 2017 Red Hat Inc.
+#
+# This file is part of Ansible
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import unittest
+
+from ansible_collections.amazon.aws.plugins.module_utils.ec2 import compare_policies
+
+
+class Ec2UtilsComparePolicies(unittest.TestCase):
+
+    # ========================================================
+    # Setup some initial data that we can use within our tests
+    # ========================================================
+    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.version_policy_missing = {
+            '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']}
+                }
+            ]
+        }
+
+        self.version_policy_old = {
+            'Version': '2008-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']}
+                }
+            ]
+        }
+
+        self.version_policy_new = {
+            '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']}
+                }
+            ]
+        }
+
+        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"
+                }
+            ]
+        }
+
+        # Minimal policy using wildcarded Principal
+        self.wildcard_policy_one = {
+            "Version": "2012-10-17",
+            "Statement": [
+                {
+                    "Principal": {
+                        "AWS": ["*"]
+                    },
+                    "Effect": "Allow",
+                    "Resource": "arn:aws:s3:::test_policy/*",
+                    "Action": [
+                        "s3:PutObject",
+                        "s3:PutObjectAcl"]
+                }
+            ]
+        }
+
+        # Minimal policy using wildcarded Principal
+        self.wildcard_policy_two = {
+            "Version": "2012-10-17",
+            "Statement": [
+                {
+                    "Principal": "*",
+                    "Effect": "Allow",
+                    "Resource": "arn:aws:s3:::test_policy/*",
+                    "Action": [
+                        "s3:PutObject",
+                        "s3:PutObjectAcl"]
+                }
+            ]
+        }
+
+    # ========================================================
+    #   ec2.compare_policies
+    # ========================================================
+
+    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))
+
+    def test_compare_version_policies_defaults_old(self):
+        """ Testing that a policy without Version is considered identical to one
+        with the 'old' Version (by default)
+        """
+        self.assertFalse(compare_policies(self.version_policy_old, self.version_policy_missing))
+        self.assertTrue(compare_policies(self.version_policy_new, self.version_policy_missing))
+
+    def test_compare_version_policies_default_disabled(self):
+        """ Testing that a policy without Version not considered identical when default_version=None
+        """
+        self.assertFalse(compare_policies(self.version_policy_missing, self.version_policy_missing, default_version=None))
+        self.assertTrue(compare_policies(self.version_policy_old, self.version_policy_missing, default_version=None))
+        self.assertTrue(compare_policies(self.version_policy_new, self.version_policy_missing, default_version=None))
+
+    def test_compare_version_policies_default_set(self):
+        """ Testing that a policy without Version is only considered identical
+        when default_version="2008-10-17"
+        """
+        self.assertFalse(compare_policies(self.version_policy_missing, self.version_policy_missing, default_version="2012-10-17"))
+        self.assertTrue(compare_policies(self.version_policy_old, self.version_policy_missing, default_version="2012-10-17"))
+        self.assertFalse(compare_policies(self.version_policy_old, self.version_policy_missing, default_version="2008-10-17"))
+        self.assertFalse(compare_policies(self.version_policy_new, self.version_policy_missing, default_version="2012-10-17"))
+        self.assertTrue(compare_policies(self.version_policy_new, self.version_policy_missing, default_version="2008-10-17"))
+
+    def test_compare_version_policies_with_none(self):
+        """ Testing that comparing with no policy works
+        """
+        self.assertTrue(compare_policies(self.small_policy_one, None))
+        self.assertTrue(compare_policies(None, self.small_policy_one))
+        self.assertFalse(compare_policies(None, None))
+
+    def test_compare_wildcard_policies_without_differences(self):
+        """ Testing two small wildcard policies which are identical except for:
+                * Principal: "*" vs Principal: ["AWS": "*"]
+        """
+        self.assertFalse(compare_policies(self.wildcard_policy_one, self.wildcard_policy_two))