From 77e4371460d4579c8a26c1511220d2bebcd4a1ca Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Fri, 23 Aug 2019 12:38:38 +0200 Subject: [PATCH] aws_kms: Update policy on existing keys (when passed) (#60059) * aws_kms: (integration tests) Use module_defaults to reduce the copy and paste * aws_kms: (integration tests) make sure policy option functions. * aws_kms: (integration tests) Move iam_role creation to start of playbook. iam_roles aren't fully created when iam_role completes, there's a delay on the Amazon side before they're fully recognised. * aws_kms: Update policy on existing keys (when passed) --- .../fragments/60059-aws-kms-update-policy.yml | 2 + .../testing_policies/security-policy.json | 1 + lib/ansible/modules/cloud/amazon/aws_kms.py | 26 ++- .../targets/aws_kms/tasks/main.yml | 155 ++++++------------ .../aws_kms/templates/console-policy.j2 | 72 ++++++++ 5 files changed, 146 insertions(+), 110 deletions(-) create mode 100644 changelogs/fragments/60059-aws-kms-update-policy.yml create mode 100644 test/integration/targets/aws_kms/templates/console-policy.j2 diff --git a/changelogs/fragments/60059-aws-kms-update-policy.yml b/changelogs/fragments/60059-aws-kms-update-policy.yml new file mode 100644 index 0000000000..832f3ea6e0 --- /dev/null +++ b/changelogs/fragments/60059-aws-kms-update-policy.yml @@ -0,0 +1,2 @@ +bugfixes: + - aws_kms - Update key policy when key already exists diff --git a/hacking/aws_config/testing_policies/security-policy.json b/hacking/aws_config/testing_policies/security-policy.json index b7feb6b0b9..8c3f2b914d 100644 --- a/hacking/aws_config/testing_policies/security-policy.json +++ b/hacking/aws_config/testing_policies/security-policy.json @@ -108,6 +108,7 @@ "kms:GenerateRandom", "kms:Get*", "kms:List*", + "kms:PutKeyPolicy", "kms:RetireGrant", "kms:ScheduleKeyDeletion", "kms:TagResource", diff --git a/lib/ansible/modules/cloud/amazon/aws_kms.py b/lib/ansible/modules/cloud/amazon/aws_kms.py index 5339c04983..03a6cc9392 100644 --- a/lib/ansible/modules/cloud/amazon/aws_kms.py +++ b/lib/ansible/modules/cloud/amazon/aws_kms.py @@ -365,7 +365,7 @@ from ansible.module_utils.aws.core import AnsibleAWSModule, is_boto3_error_code from ansible.module_utils.ec2 import ec2_argument_spec from ansible.module_utils.ec2 import AWSRetry, camel_dict_to_snake_dict from ansible.module_utils.ec2 import boto3_tag_list_to_ansible_dict, ansible_dict_to_boto3_tag_list -from ansible.module_utils.ec2 import compare_aws_tags +from ansible.module_utils.ec2 import compare_aws_tags, compare_policies from ansible.module_utils.six import string_types import json @@ -664,6 +664,28 @@ def update_key(connection, module, key): except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: module.fail_json_aws(e, msg="Unable to add tag to key") + # Update existing policy before trying to tweak grants + if module.params.get('policy'): + policy = module.params.get('policy') + try: + keyret = connection.get_key_policy(KeyId=key['key_id'], PolicyName='default') + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + # If we can't fetch the current policy assume we're making a change + # Could occur if we have PutKeyPolicy without GetKeyPolicy + original_policy = {} + original_policy = json.loads(keyret['Policy']) + try: + new_policy = json.loads(policy) + except ValueError as e: + module.fail_json_aws(e, msg="Unable to parse new policy as JSON") + if compare_policies(original_policy, new_policy): + changed = True + if not module.check_mode: + try: + connection.put_key_policy(KeyId=key['key_id'], PolicyName='default', Policy=policy) + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + module.fail_json_aws(e, msg="Unable to update key policy") + desired_grants = module.params.get('grants') existing_grants = key['grants'] @@ -774,7 +796,7 @@ def do_policy_grant(kms, keyarn, role_arn, granttypes, mode='grant', dry_run=Tru policy = json.loads(keyret['Policy']) changes_needed = {} - # assert_policy_shape(policy) + assert_policy_shape(policy) had_invalid_entries = False for statement in policy['Statement']: for granttype in ['role', 'role grant', 'admin']: diff --git a/test/integration/targets/aws_kms/tasks/main.yml b/test/integration/targets/aws_kms/tasks/main.yml index 784796bff2..2e8643a50e 100644 --- a/test/integration/targets/aws_kms/tasks/main.yml +++ b/test/integration/targets/aws_kms/tasks/main.yml @@ -1,21 +1,36 @@ -- block: - - # ============================================================ - - name: See whether key exists and its current state - aws_kms_info: +- module_defaults: + group/aws: region: "{{ aws_region }}" aws_access_key: "{{ aws_access_key }}" aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" + security_token: "{{ security_token | default(omit) }}" + block: + # ============================================================ + # PREPARATION + # + # Get some information about who we are before starting our tests + # we'll need this as soon as we start working on the policies + - name: get ARN of calling user + aws_caller_info: + register: aws_caller_info + + # IAM Roles completes before the Role is fully instantiated, create it here + # to ensure it exists when we need it for updating the policies + - name: create an IAM role that can do nothing + iam_role: + name: "{{ resource_prefix }}-kms-role" + state: present + assume_role_policy_document: '{"Version": "2012-10-17", "Statement": {"Action": "sts:AssumeRole", "Principal": {"Service": "ec2.amazonaws.com"}, "Effect": "Deny"} }' + register: iam_role_result + # ============================================================ + # TESTS + - name: See whether key exists and its current state + aws_kms_info: filters: alias: "{{ resource_prefix }}-kms" - name: create a key aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" tags: Hello: World @@ -31,10 +46,6 @@ - name: find facts about the key aws_kms_info: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" filters: alias: "{{ resource_prefix }}-kms" register: new_key @@ -44,15 +55,27 @@ that: - new_key["keys"]|length == 1 - - name: create an IAM role that can do nothing - iam_role: - name: "{{ resource_prefix }}-kms-role" - state: present - assume_role_policy_document: '{"Version": "2012-10-17", "Statement": {"Action": "sts:AssumeRole", "Principal": {"Service": "ec2.amazonaws.com"}, "Effect": "Deny"} }' - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" - register: iam_role_result + - name: Update Policy on key to match AWS Console generate policy + aws_kms: + key_alias: "alias/{{ resource_prefix }}-kms" + policy: "{{ lookup('template', 'console-policy.j2') | to_json }}" + register: kms_policy_changed + + - name: Policy should have been changed + assert: + that: + - kms_policy_changed is changed + + - name: Attempt to re-assert the same policy + aws_kms: + key_alias: "alias/{{ resource_prefix }}-kms" + policy: "{{ lookup('template', 'console-policy.j2') | to_json }}" + register: kms_policy_changed + + - name: Policy should not have changed since it was last set + assert: + that: + - kms_policy_changed is succeeded - name: grant user-style access to production secrets aws_kms: @@ -60,17 +83,9 @@ key_alias: "alias/{{ resource_prefix }}-kms" role_name: "{{ resource_prefix }}-kms-role" grant_types: "role,role grant" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" - region: "{{ aws_region }}" - name: find facts about the key aws_kms_info: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" filters: alias: "{{ resource_prefix }}-kms" register: new_key @@ -80,44 +95,15 @@ mode: deny key_alias: "alias/{{ resource_prefix }}-kms" role_arn: "{{ iam_role_result.iam_role.arn }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" - region: "{{ aws_region }}" - name: find facts about the key aws_kms_info: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" filters: alias: "{{ resource_prefix }}-kms" register: new_key - - name: set aws environment base fact - set_fact: - aws_environment_base: - AWS_ACCESS_KEY_ID: "{{ aws_access_key }}" - AWS_SECRET_ACCESS_KEY: "{{ aws_secret_key }}" - no_log: True - - - name: set aws environment fact - set_fact: - aws_environment: "{{ aws_environment_base|combine(security_token|ternary({'AWS_SECURITY_TOKEN': security_token}, {})) }}" - no_log: True - - - name: get ARN of calling user - aws_caller_info: - environment: "{{ aws_environment }}" - register: aws_caller_info - - name: Allow the IAM role to use a specific Encryption Context aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present purge_grants: yes @@ -143,10 +129,6 @@ - name: Add a second grant aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present grants: @@ -170,10 +152,6 @@ - name: Add a second grant again aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present grants: @@ -197,10 +175,6 @@ - name: Update the grants with purge_grants set aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present purge_grants: yes @@ -225,10 +199,6 @@ - name: update third grant to change encryption context equals to subset aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present grants: @@ -254,10 +224,6 @@ - name: tag encryption key aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present tags: @@ -275,10 +241,6 @@ - name: add, replace, remove tags aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present purge_tags: yes @@ -298,10 +260,6 @@ - name: make no real tag change aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present register: tag_kms_no_update @@ -317,10 +275,6 @@ - name: update the key's description and disable it aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present description: test key for testing @@ -336,10 +290,6 @@ - name: delete the key aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: absent register: delete_kms @@ -352,10 +302,6 @@ - name: undelete and enable the key aws_kms: - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" state: present enabled: yes @@ -368,15 +314,11 @@ - undelete_kms.changed always: - # ============================================================ + # CLEAN-UP - name: finish off by deleting key aws_kms: state: absent - region: "{{ aws_region }}" - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" alias: "{{ resource_prefix }}-kms" register: destroy_result @@ -384,7 +326,4 @@ iam_role: name: "{{ resource_prefix }}-kms-role" state: absent - aws_access_key: "{{ aws_access_key }}" - aws_secret_key: "{{ aws_secret_key }}" - security_token: "{{ security_token }}" register: iam_role_result diff --git a/test/integration/targets/aws_kms/templates/console-policy.j2 b/test/integration/targets/aws_kms/templates/console-policy.j2 new file mode 100644 index 0000000000..4b60ba5889 --- /dev/null +++ b/test/integration/targets/aws_kms/templates/console-policy.j2 @@ -0,0 +1,72 @@ +{ + "Id": "key-consolepolicy-3", + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": "arn:aws:iam::{{ aws_caller_info.account }}:root" + }, + "Action": "kms:*", + "Resource": "*" + }, + { + "Sid": "Allow access for Key Administrators", + "Effect": "Allow", + "Principal": { + "AWS": "{{ aws_caller_info.arn }}" + }, + "Action": [ + "kms:Create*", + "kms:Describe*", + "kms:Enable*", + "kms:List*", + "kms:Put*", + "kms:Update*", + "kms:Revoke*", + "kms:Disable*", + "kms:Get*", + "kms:Delete*", + "kms:TagResource", + "kms:UntagResource", + "kms:ScheduleKeyDeletion", + "kms:CancelKeyDeletion" + ], + "Resource": "*" + }, + { + "Sid": "Allow use of the key", + "Effect": "Allow", + "Principal": { + "AWS": "{{ aws_caller_info.arn }}" + }, + "Action": [ + "kms:Encrypt", + "kms:Decrypt", + "kms:ReEncrypt*", + "kms:GenerateDataKey*", + "kms:DescribeKey" + ], + "Resource": "*" + }, + { + "Sid": "Allow attachment of persistent resources", + "Effect": "Allow", + "Principal": { + "AWS": "{{ aws_caller_info.arn }}" + }, + "Action": [ + "kms:CreateGrant", + "kms:ListGrants", + "kms:RevokeGrant" + ], + "Resource": "*", + "Condition": { + "Bool": { + "kms:GrantIsForAWSResource": "true" + } + } + } + ] +}