From bb39c0364acf5772eed46ed4fc9ad6388fb49ba7 Mon Sep 17 00:00:00 2001 From: msven Date: Wed, 31 Mar 2021 14:30:50 -0500 Subject: [PATCH] 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 --- ...ways_reporting_changed_with_kms_alias.yaml | 3 + plugins/modules/cloudtrail.py | 50 +++++++++- .../targets/cloudtrail/defaults/main.yml | 1 + .../targets/cloudtrail/tasks/main.yml | 96 +++++++++++++++++-- .../cloudtrail-no-kms-assume-policy.j2 | 11 +++ .../templates/cloudtrail-no-kms-policy.j2 | 11 +++ 6 files changed, 161 insertions(+), 11 deletions(-) create mode 100644 changelogs/fragments/506-cloudtrail_fix_always_reporting_changed_with_kms_alias.yaml create mode 100644 tests/integration/targets/cloudtrail/templates/cloudtrail-no-kms-assume-policy.j2 create mode 100644 tests/integration/targets/cloudtrail/templates/cloudtrail-no-kms-policy.j2 diff --git a/changelogs/fragments/506-cloudtrail_fix_always_reporting_changed_with_kms_alias.yaml b/changelogs/fragments/506-cloudtrail_fix_always_reporting_changed_with_kms_alias.yaml new file mode 100644 index 00000000000..d243c9a5f41 --- /dev/null +++ b/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). diff --git a/plugins/modules/cloudtrail.py b/plugins/modules/cloudtrail.py index 5f8aa5ae03f..a2f2076993f 100644 --- a/plugins/modules/cloudtrail.py +++ b/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) diff --git a/tests/integration/targets/cloudtrail/defaults/main.yml b/tests/integration/targets/cloudtrail/defaults/main.yml index 7338e364da3..93cea45da99 100644 --- a/tests/integration/targets/cloudtrail/defaults/main.yml +++ b/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' diff --git a/tests/integration/targets/cloudtrail/tasks/main.yml b/tests/integration/targets/cloudtrail/tasks/main.yml index 8aa695b95c2..32688315a72 100644 --- a/tests/integration/targets/cloudtrail/tasks/main.yml +++ b/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 diff --git a/tests/integration/targets/cloudtrail/templates/cloudtrail-no-kms-assume-policy.j2 b/tests/integration/targets/cloudtrail/templates/cloudtrail-no-kms-assume-policy.j2 new file mode 100644 index 00000000000..f3bfd14ec6f --- /dev/null +++ b/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" + } + ] +} diff --git a/tests/integration/targets/cloudtrail/templates/cloudtrail-no-kms-policy.j2 b/tests/integration/targets/cloudtrail/templates/cloudtrail-no-kms-policy.j2 new file mode 100644 index 00000000000..d85b650b70f --- /dev/null +++ b/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": [ "*" ] + } + ] +}