aws_kms: Update policy on existing keys (when passed)

[tremble]

SUMMARY

Update policy on existing keys when policy is passed as an option.

Fixes #59987

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

lib/ansible/modules/cloud/amazon/aws_kms.py

ADDITIONAL INFORMATION

@willthames PR as promised on Friday

[ansibot]

@tremble, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

[ansibot]

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_kms.py:654:97: bad-whitespace No space allowed before :         if json.dumps(original_policy, sort_keys=True) != json.dumps(new_policy, sort_keys=True) :                                                                                                  ^

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_kms.py:654:97: E203 whitespace before ':'

click here for bot help

[willthames]

@tremble this change looks reasonable to me but needs the test suite updated to match (so that it would have caught this problem)

[ansibot]

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_kms.py:654:97: bad-whitespace No space allowed before :         if json.dumps(original_policy, sort_keys=True) != json.dumps(new_policy, sort_keys=True) :                                                                                                  ^

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_kms.py:654:97: E203 whitespace before ':'

click here for bot help

[tremble]

@willthames the integration tests appear to be broken. This may be due to the older version of Boto3 I'm running (rhel 7), however, I'm seeing things like

The full traceback is:
Traceback (most recent call last):
  File "/home/mchappel/.ansible/tmp/ansible-tmp-1565005313.24-218693380161805/AnsiballZ_aws_kms.py", line 139, in <module>
    _ansiballz_main()
  File "/home/mchappel/.ansible/tmp/ansible-tmp-1565005313.24-218693380161805/AnsiballZ_aws_kms.py", line 131, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File "/home/mchappel/.ansible/tmp/ansible-tmp-1565005313.24-218693380161805/AnsiballZ_aws_kms.py", line 68, in invoke_module
    imp.load_module('__main__', mod, module, MOD_DESC)
  File "/tmp/ansible_aws_kms_payload_JEyiic/__main__.py", line 937, in <module>
  File "/tmp/ansible_aws_kms_payload_JEyiic/__main__.py", line 903, in main
KeyError: 'key_arn'

Which looks like some refactoring around aliases hasn't done what's expected.

[tremble]

See also #60206 for initial test fixes

[omonnig]

This PR looks reasonable, but do we care if the policy matches? I wasted way too much time trying to debug my policy before checking the module source and finding the root cause. Just write the policy and be done with it.

How do we handle the BypassPolicyLockoutSafetyCheck? It should default to False, only set to True by the user. This behavior can be dangerous, but allowed IMHO.

[willthames]

We do care if the policy matches, otherwise how do we know whether to report changed correctly?

[tremble]

@omonnig Am I understanding correctly that this PR falsely flagged your new policy as not changing anything? Or are you just worried about the additional complexity?

As @willthames mentioned it's there so that "changed" can be correctly reported. Personally I try to ensure that my playbooks are idempotent and "changed" only shows up when something's really changed.

re BypassPolicyLockoutSafetyCheck that would likely want to be an additional option, and for the sake of keeping PRs simple I'd rather not pull it into this PR.

[tremble]

@willthames Should I rebase this change on top of #60206 and see if I can test the shape / policy pieces as a part of this PR built on top of #60206 ?

[omonnig]

Sorry, @willthames, I was not thinking straight. The Changed functionality is very important and should be included in this PR.

I will put the BypassPolicyLockoutSafetyCheck in another PR.

[tremble]

@willthames @omonnig https://github.com/tremble/ansible/tree/aws_kms_keyid_only now uses compare_policies instead of json.dumps I'll update this PR once @jillr or @willthames can give me an idea which order we're going to try and merge everything in.

[tremble]

And just for the curious, the reason a simple json dump doesn't work is that it would appear Amazon mangle the Principal lists. My basic testing seems to show that when you add a new principal to the list then where-ever in the list you add it Amazon will add it to the end of the list.

[jillr]

Test suite passes for me, change looks good. Thanks @tremble.

[willthames]

Looks like security-policy.json is missing a permission:

The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_aws_kms_payload_gnmjc6sa/__main__.py", line 685, in update_key
    connection.put_key_policy(KeyId=key['key_id'], PolicyName='default', Policy=policy)
  File "/usr/local/lib/python3.6/dist-packages/botocore/client.py", line 357, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/usr/local/lib/python3.6/dist-packages/botocore/client.py", line 661, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (AccessDeniedException) when calling the PutKeyPolicy operation: User: arn:aws:iam::864163117881:user/AnsibleTestUser is not authorized to perform: kms:PutKeyPolicy on resource: arn:aws:kms:us-west-2:864163117881:key/57be8964-e790-44fb-afe1-2307e8febccd

[tremble]

@willthames added

[willthames]

Tests pass for me locally now, thanks @tremble!