Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws_kms_info - Gracefully Handle Keys That Don't Allow kms:GetKeyRotationStatus API Calls #199

Merged
merged 7 commits into from
Aug 24, 2020
Merged

aws_kms_info - Gracefully Handle Keys That Don't Allow kms:GetKeyRotationStatus API Calls #199

merged 7 commits into from
Aug 24, 2020

Conversation

ichekaldin
Copy link
Contributor

SUMMARY

Some AWS KMS keys (e.g. aws/acm) do not allow permissions to call the API
kms:GetKeyRotationStatus. As a result, module execution fails, even if the
user executing it has full admin privileges (example).

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

aws_kms_info.py

ADDITIONAL INFORMATION

The following module execution:

- community.aws.aws_kms_info:
  filter:
    alias: aws/ebs

will fail with the following error:

An exception occurred during task execution. To see the full traceback, use -vvv. The error was: botocore.exceptions.ClientError: An error occurred (AccessDeniedException) when calling the GetKeyRotationStatus operation: User: arn:aws:sts::123412341234:assumed-role/MyRole/i-1234abcd1234 is not authorized to perform: kms:GetKeyRotationStatus on resource: arn:aws:kms:us-east-1:123412341234:key/3b5bbd74-1234-abcd-1234-1234abcd1234

Key ID 3b5bbd74-1234-abcd-1234-1234abcd1234 in the example above is aws/acm key.

…calls

Some AWS KMS keys (e.g. aws/acm) do not allow permissions to call the API
kms:GetKeyRotationStatus. As a result, module execution fails, even if the
user execuing it has full admin privileges.

Example: https://forums.aws.amazon.com/thread.jspa?threadID=312992
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for your contribution.

We have some test cases (they care currently only run 'manually') do you think you could update the integration test to account for this issue and help avoid a regression?

plugins/modules/aws_kms_info.py Outdated Show resolved Hide resolved
Update documentation to reflect this use case.

Use helper to track the exception.
@ichekaldin
Copy link
Contributor Author

Trying to think of a good integration test to cover this situation.

Would something like this work:

- module_defaults:
    group/aws:
        region: "{{ aws_region }}"
        aws_access_key: "{{ aws_access_key }}"
        aws_secret_key: "{{ aws_secret_key }}"
        security_token: "{{ security_token | default(omit) }}"
  collections:
    - amazon.aws

  block:
    #   TESTS
    - name: Retrive information about aws/kms key 
      aws_kms_info:
        filters:
          alias: aws/kms
      register: check_key

    - name: assert that key rotation state is None
      assert:
        that:
          - check_key.enable_key_rotation == None

@tremble
Copy link
Contributor

tremble commented Aug 24, 2020

tests/integration/targets/aws_kms/ already contains a suite of tests.

My suggestion would be that as one of the last tests before the key is scheduled for deletion you set a new policy on the key that includes an explicit deny (for everyone) on kms:GetKeyRotationStatus. You then attempt to fetch the info for that key.

@ichekaldin
Copy link
Contributor Author

That makes sense. I added an integration test that (I think) covers this situation.

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing for the future, and possibly for your playbooks, the templating engine used by Ansible doesn't like "== None", it needs "is undefined" for a situation like this.

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and local tests show the expected behaviour.

@tremble tremble merged commit 2d16b54 into ansible-collections:main Aug 24, 2020
@tremble
Copy link
Contributor

tremble commented Aug 24, 2020

Many thanks for your contribution. Now for me to fix the aws_kms issue and some minor bugs in the test suite :)

@ichekaldin
Copy link
Contributor Author

Thank you!

@ichekaldin ichekaldin deleted the ichekaldin/bugfix_aws_kms_info_rotation_fix branch May 6, 2021 23:24
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
…tionStatus API Calls (ansible-collections#199)

* Gracefully handle keys that don't allow kms:GetKeyRotationStatus API calls

Some AWS KMS keys (e.g. aws/acm) do not allow permissions to call the API
kms:GetKeyRotationStatus. As a result, module execution fails, even if the
user execuing it has full admin privileges.

Example: https://forums.aws.amazon.com/thread.jspa?threadID=312992

* change log fragment

* Return None if key rotation status can't be determined

Update documentation to reflect this use case.

Use helper to track the exception.

* Add integration tests
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
…tionStatus API Calls (ansible-collections#199)

* Gracefully handle keys that don't allow kms:GetKeyRotationStatus API calls

Some AWS KMS keys (e.g. aws/acm) do not allow permissions to call the API
kms:GetKeyRotationStatus. As a result, module execution fails, even if the
user execuing it has full admin privileges.

Example: https://forums.aws.amazon.com/thread.jspa?threadID=312992

* change log fragment

* Return None if key rotation status can't be determined

Update documentation to reflect this use case.

Use helper to track the exception.

* Add integration tests
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
* Add AWSRetries to standard ec2_vol boto3 calls

* changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants