Fix changed always true when kms key alias used (#506)

Fix output.exists value on create

Fix tags being changed to lower case

add changelog

changelogs/fragments/506-cloudtrail_fix_always_reporting_changed_with_kms_alias.yaml

@@ -0,0 +1,3 @@
+bugfixes:
+  - cloudtrail - fix always reporting changed = true when kms alias used (https://github.com/ansible-collections/community.aws/pull/506).
+  - cloudtrail - fix lower casing of tag keys (https://github.com/ansible-collections/community.aws/pull/506).

plugins/modules/cloudtrail.py

@@ -261,6 +261,24 @@
                                                                      )
 
 
+def get_kms_key_aliases(module, client, keyId):
+    """
+    get list of key aliases
+
+    module : AnsibleAWSModule object
+    client : boto3 client connection object for kms
+    keyId : keyId to get aliases for
+    """
+    try:
+        key_resp = client.list_aliases(KeyId=keyId)
+    except (BotoCoreError, ClientError) as err:
+        # Don't fail here, just return [] to maintain backwards compat
+        # in case user doesn't have kms:ListAliases permissions
+        return []
+
+    return key_resp['Aliases']
+
+
 def create_trail(module, client, ct_params):
     """
     Creates a CloudTrail
@@ -500,6 +518,7 @@ def main():
     # If the trail exists set the result exists variable
     if trail is not None:
         results['exists'] = True
+        initial_kms_key_id = trail.get('KmsKeyId')
 
     if state == 'absent' and results['exists']:
         # If Trail exists go ahead and delete
@@ -524,7 +543,11 @@ def main():
                 val = ct_params.get(key)
             if val != trail.get(tkey):
                 do_update = True
-                results['changed'] = True
+                if tkey != 'KmsKeyId':
+                    # We'll check if the KmsKeyId casues changes later since
+                    # user could've provided a key alias, alias arn, or key id
+                    # and trail['KmsKeyId'] is always a key arn
+                    results['changed'] = True
                 # If we are in check mode copy the changed values to the trail facts in result output to show what would change.
                 if module.check_mode:
                     trail.update({tkey: ct_params.get(key)})
@@ -533,6 +556,26 @@ def main():
             update_trail(module, client, ct_params)
             trail = get_trail_facts(module, client, ct_params['Name'])
 
+        # Determine if KmsKeyId changed
+        if not module.check_mode:
+            if initial_kms_key_id != trail.get('KmsKeyId'):
+                results['changed'] = True
+        else:
+            new_key = ct_params.get('KmsKeyId')
+            if initial_kms_key_id != new_key:
+                # Assume changed for a moment
+                results['changed'] = True
+
+                # However, new_key could be a key id, alias arn, or alias name
+                # that maps back to the key arn in initial_kms_key_id. So check
+                # all aliases for a match.
+                initial_aliases = get_kms_key_aliases(module, module.client('kms'), initial_kms_key_id)
+                for a in initial_aliases:
+                    if(a['AliasName'] == new_key or
+                       a['AliasArn'] == new_key or
+                       a['TargetKeyId'] == new_key):
+                        results['changed'] = False
+
         # Check if we need to start/stop logging
         if enable_logging and not trail['IsLogging']:
             results['changed'] = True
@@ -554,11 +597,12 @@ def main():
             results['changed'] = True
             trail['tags'] = tags
         # Populate trail facts in output
-        results['trail'] = camel_dict_to_snake_dict(trail)
+        results['trail'] = camel_dict_to_snake_dict(trail, ignore_list=['tags'])
 
     elif state == 'present' and not results['exists']:
         # Trail doesn't exist just go create it
         results['changed'] = True
+        results['exists'] = True
         if not module.check_mode:
             # If we aren't in check_mode then actually create it
             created_trail = create_trail(module, client, ct_params)
@@ -598,7 +642,7 @@ def main():
             trail['IsLogging'] = enable_logging
             trail['tags'] = tags
         # Populate trail facts in output
-        results['trail'] = camel_dict_to_snake_dict(trail)
+        results['trail'] = camel_dict_to_snake_dict(trail, ignore_list=['tags'])
 
     module.exit_json(**results)
 

tests/integration/targets/cloudtrail/defaults/main.yml

@@ -5,3 +5,4 @@ sns_topic: '{{ resource_prefix }}-cloudtrail-notifications'
 cloudtrail_prefix: 'test-prefix'
 cloudwatch_log_group: '{{ resource_prefix }}-cloudtrail'
 cloudwatch_role: '{{ resource_prefix }}-cloudtrail'
+cloudwatch_no_kms_role: '{{ resource_prefix }}-cloudtrail2'

tests/integration/targets/cloudtrail/tasks/main.yml

@@ -24,9 +24,6 @@
 # - Using blank string for CloudWatch Log Group / Role doesn't remove them
 #
 # Possible Bugs:
-# - output.exists == false when creating
-# - Changed reports true when using a KMS alias
-# - Tags Keys are being lower-cased
 
 - module_defaults:
     group/aws:
@@ -175,6 +172,27 @@
       policy_name: 'CloudWatch'
       policy_json: "{{ lookup('template', 'cloudwatch-policy.j2') | to_json }}"
 
+  - name: 'Create CloudWatch IAM Role with no kms permissions'
+    iam_role:
+      state: present
+      name: '{{ cloudwatch_no_kms_role }}'
+      assume_role_policy_document: "{{ lookup('template', 'cloudtrail-no-kms-assume-policy.j2') }}"
+      managed_policies:
+        - "arn:aws:iam::aws:policy/AWSCloudTrail_FullAccess"
+    register: output_cloudwatch_no_kms_role
+
+  - name: pause to ensure role exists before attaching policy
+    pause:
+      seconds: 15
+
+  - name: 'Add inline policy to CloudWatch Role'
+    iam_policy:
+      state: present
+      iam_type: role
+      iam_name: '{{ cloudwatch_no_kms_role }}'
+      policy_name: 'CloudWatchNokms'
+      policy_json: "{{ lookup('template', 'cloudtrail-no-kms-policy.j2') }}"
+
   # ============================================================
   #   Tests
   # ============================================================
@@ -197,8 +215,7 @@
   - assert:
       that:
       - output is changed
-      # XXX This appears to be a bug...
-      #- output.exists == True
+      - output.exists == True
       - output.trail.name == cloudtrail_name
 
   - name: 'No-op update to trail'
@@ -451,7 +468,7 @@
       - output.trail.name == cloudtrail_name
       - output.trail.tags | length == 2
       - '("tag2" in output.trail.tags) and (output.trail.tags["tag2"] == "Value2")'
-      #- '("Tag3" in output.trail.tags) and (output.trail.tags["Tag3"] == "Value3")'
+      - '("Tag3" in output.trail.tags) and (output.trail.tags["Tag3"] == "Value3")'
 
   - name: 'Change tags (no change)'
     cloudtrail:
@@ -467,7 +484,7 @@
       - output.trail.name == cloudtrail_name
       - output.trail.tags | length == 2
       - '("tag2" in output.trail.tags) and (output.trail.tags["tag2"] == "Value2")'
-      #- '("Tag3" in output.trail.tags) and (output.trail.tags["Tag3"] == "Value3")'
+      - '("Tag3" in output.trail.tags) and (output.trail.tags["Tag3"] == "Value3")'
 
   - name: 'Remove tags (CHECK MODE)'
     cloudtrail:
@@ -1146,6 +1163,18 @@
       - output is not changed
       - output.trail.kms_key_id == kms_key.key_arn
 
+  - name: 'Enable logging encryption (no change, check mode)'
+    cloudtrail:
+      state: present
+      name: '{{ cloudtrail_name }}'
+      kms_key_id: '{{ kms_key.key_arn }}'
+    check_mode: yes
+    register: output
+  - assert:
+      that:
+      - output is not changed
+      - output.trail.kms_key_id == kms_key.key_arn
+
   - name: 'No-op update to trail'
     cloudtrail:
       state: present
@@ -1219,9 +1248,48 @@
     register: output
   - assert:
       that:
-  #    - output is not changed
+      - output is not changed
       - output.trail.kms_key_id == kms_key.key_arn
 
+  - name: 'Update logging encryption to alias (CHECK MODE, no change)'
+    cloudtrail:
+      state: present
+      name: '{{ cloudtrail_name }}'
+      kms_key_id: '{{ kms_key.key_id }}' # Test when using key id
+    register: output
+    check_mode: yes
+  - assert:
+      that:
+      - output is not changed
+      - output.trail.kms_key_id == kms_key.key_id
+
+  # Assume role to a role with Denied access to KMS
+
+  - community.aws.sts_assume_role:
+      role_arn: '{{ output_cloudwatch_no_kms_role.arn }}'
+      role_session_name: "cloudtrailNoKms"
+      aws_access_key: '{{ aws_access_key }}'
+      aws_secret_key: '{{ aws_secret_key }}'
+      security_token: '{{ security_token | default(omit) }}'
+      region: '{{ aws_region }}'
+    register: noKms_assumed_role
+
+  - name: 'Enable logging encryption w/ alias (no change, no kms permmissions, check mode)'
+    cloudtrail:
+      state: present
+      name: '{{ cloudtrail_name }}'
+      kms_key_id: 'alias/{{ kms_alias }}'
+      aws_access_key: "{{ noKms_assumed_role.sts_creds.access_key }}"
+      aws_secret_key: "{{ noKms_assumed_role.sts_creds.secret_key }}"
+      security_token: "{{ noKms_assumed_role.sts_creds.session_token }}"
+    check_mode: yes
+    register: output
+  - assert:
+      that:
+      - output is changed
+      # when using check_mode, with no kms permissions, and not giving kms_key_id as a key arn
+      # output will always be marked as changed.
+
   #- name: 'Disable logging encryption (CHECK MODE)'
   #  cloudtrail:
   #    state: present
@@ -1423,3 +1491,15 @@
       state: absent
       name: '{{ cloudwatch_role }}'
     ignore_errors: yes
+  - name: 'Remove inline policy to CloudWatch Role'
+    iam_policy:
+      state: absent
+      iam_type: role
+      iam_name: '{{ cloudwatch_no_kms_role }}'
+      policy_name: 'CloudWatchNokms'
+    ignore_errors: yes
+  - name: 'Delete CloudWatch No KMS IAM Role'
+    iam_role:
+      state: absent
+      name: '{{ cloudwatch_no_kms_role }}'
+    ignore_errors: yes

tests/integration/targets/cloudtrail/templates/cloudtrail-no-kms-assume-policy.j2

@@ -0,0 +1,11 @@
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "AssumeRole",
+      "Effect": "Allow",
+      "Principal": { "AWS": "arn:aws:iam::{{ aws_caller_info.account }}:root" },
+      "Action": "sts:AssumeRole"
+    }
+  ]
+}

tests/integration/targets/cloudtrail/templates/cloudtrail-no-kms-policy.j2

@@ -0,0 +1,11 @@
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "kmsDeny",
+      "Effect": "Deny",
+      "Action": [ "kms:*" ],
+      "Resource": [ "*" ]
+    }
+  ]
+}