Skip to content

Commit

Permalink
Fix changed always true when kms key alias used (ansible-collections#506
Browse files Browse the repository at this point in the history
)

Fix output.exists value on create

Fix tags being changed to lower case

add changelog
  • Loading branch information
msven authored and danielcotton committed Nov 23, 2021
1 parent e795f81 commit bb39c03
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 11 deletions.
Original file line number Diff line number Diff line change
@@ -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).
50 changes: 47 additions & 3 deletions plugins/modules/cloudtrail.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)})
Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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)

Expand Down
1 change: 1 addition & 0 deletions tests/integration/targets/cloudtrail/defaults/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
96 changes: 88 additions & 8 deletions tests/integration/targets/cloudtrail/tasks/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
# ============================================================
Expand All @@ -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'
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Original file line number Diff line number Diff line change
@@ -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"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"Version": "2012-10-17",
"Statement": [
{
"Sid": "kmsDeny",
"Effect": "Deny",
"Action": [ "kms:*" ],
"Resource": [ "*" ]
}
]
}

0 comments on commit bb39c03

Please sign in to comment.