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

Enhance aws_kms_info to support ignoring denied keys #206

Closed
jokajak opened this issue Aug 27, 2020 · 5 comments · Fixed by #1332
Closed

Enhance aws_kms_info to support ignoring denied keys #206

jokajak opened this issue Aug 27, 2020 · 5 comments · Fixed by #1332
Labels
affects_2.10 feature This issue/PR relates to a feature request module module plugins plugin (any type)

Comments

@jokajak
Copy link
Contributor

jokajak commented Aug 27, 2020

SUMMARY

Enhance aws_kms_info to allow searching for keys KMS even if one or more keys cannot be accessed due to an explicit deny.

ISSUE TYPE
  • Feature Idea
COMPONENT NAME

aws_kms_info.py

ADDITIONAL INFORMATION

This feature would allow obtaining facts for all available keys even if one key is inaccessible. A key could be inaccessible because there is an explicit deny for accessing the key.

One method to accomplish this would be to add a parameter to the module that allows ignoring these errors.

- name: Get all accessible KMS keys
  aws_kms_info:
    ignore_deny: yes
@jokajak
Copy link
Contributor Author

jokajak commented Aug 27, 2020

Leaving this here. I plan on writing test cases and trying to get this incorporated but it will take a bit of effort for me to write the test cases due to needing to set up appropriate permissions.

--- plugins/modules/aws_kms_info.py     2020-08-27 13:00:07.545602595 +0000
+++ ansible_collections/community/aws/plugins/modules/aws_kms_info.py   2020-08-27 12:56:05.108964305 +0000
@@ -27,10 +27,6 @@
     description: Whether to get full details (tags, grants etc.) of keys pending deletion
     default: False
     type: bool
-  ignore_deny:
-    description: Whether to continue if a key cannot be accessed.
-    default: False
-    type: bool
 extends_documentation_fragment:
 - amazon.aws.aws
 - amazon.aws.ec2
@@ -362,9 +358,6 @@
     try:
         result = get_kms_metadata_with_backoff(connection, key_id)['KeyMetadata']
     except botocore.exceptions.ClientError as e:
-        if e.response['Error']['Code'] == "AccessDeniedException" and module.params.get('ignore_deny'):
-            module.warn("Failed to obtain key metadata for key-id {key_id}, ignoring.".format(key_id=key_id))
-            return None
         module.fail_json(msg="Failed to obtain key metadata",
                          exception=traceback.format_exc(),
                          **camel_dict_to_snake_dict(e.response))
@@ -404,17 +397,13 @@
                          exception=traceback.format_exc(),
                          **camel_dict_to_snake_dict(e.response))
 
-    key_details = [get_key_details(connection, module, key['KeyId']) for key in keys]
-    # Remove any None entries from key details
-    # None is returned if access to a key is deniied
-    return [key for key in key_details if key]
+    return [get_key_details(connection, module, key['KeyId']) for key in keys]
 
 
 def main():
     argument_spec = dict(
         filters=dict(type='dict'),
         pending_deletion=dict(type='bool', default=False),
-        ignore_deny=dict(type='bool', default=False),
     )
 
     module = AnsibleAWSModule(argument_spec=argument_spec,

@tremble
Copy link
Contributor

tremble commented Aug 27, 2020

FYI There is #200 (and mattclay/aws-terminator#106) which would enable running the current integration tests in the CI account and includes a minor bugfix in the tests

@ansibullbot
Copy link

@ansibullbot ansibullbot added affects_2.10 feature This issue/PR relates to a feature request module module plugins plugin (any type) labels Aug 27, 2020
@phene
Copy link
Contributor

phene commented Feb 10, 2022

I just ran into this issue -- This would be an excellent enhancement.

@ansibullbot
Copy link

alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this issue May 25, 2022
Update module name deprecation messages to be more helpful
softwarefactory-project-zuul bot pushed a commit that referenced this issue Jul 9, 2022
kms_key_info - improve AccessDeniedException handing

SUMMARY
fixes: #206
Because KMS doesn't support server-side filtering of keys we have to pull full metadata for all KMS keys unless querying a specific key.  This can result in additional permission denied errors, even though we may have permissions to read many of the keys.  Try to handle AccessDeniedException more liberally.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
kms_key_info
ADDITIONAL INFORMATION

Reviewed-by: Joseph Torcasso <None>
patchback bot pushed a commit that referenced this issue Jul 9, 2022
kms_key_info - improve AccessDeniedException handing

SUMMARY
fixes: #206
Because KMS doesn't support server-side filtering of keys we have to pull full metadata for all KMS keys unless querying a specific key.  This can result in additional permission denied errors, even though we may have permissions to read many of the keys.  Try to handle AccessDeniedException more liberally.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
kms_key_info
ADDITIONAL INFORMATION

Reviewed-by: Joseph Torcasso <None>
(cherry picked from commit 5e1466e)
patchback bot pushed a commit that referenced this issue Jul 9, 2022
kms_key_info - improve AccessDeniedException handing

SUMMARY
fixes: #206
Because KMS doesn't support server-side filtering of keys we have to pull full metadata for all KMS keys unless querying a specific key.  This can result in additional permission denied errors, even though we may have permissions to read many of the keys.  Try to handle AccessDeniedException more liberally.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
kms_key_info
ADDITIONAL INFORMATION

Reviewed-by: Joseph Torcasso <None>
(cherry picked from commit 5e1466e)
softwarefactory-project-zuul bot pushed a commit that referenced this issue Jul 10, 2022
[PR #1332/5e1466e9 backport][stable-4] kms_key_info - improve AccessDeniedException handing

This is a backport of PR #1332 as merged into main (5e1466e).
SUMMARY
fixes: #206
Because KMS doesn't support server-side filtering of keys we have to pull full metadata for all KMS keys unless querying a specific key.  This can result in additional permission denied errors, even though we may have permissions to read many of the keys.  Try to handle AccessDeniedException more liberally.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
kms_key_info
ADDITIONAL INFORMATION

Reviewed-by: Mark Chappell <None>
softwarefactory-project-zuul bot pushed a commit that referenced this issue Jul 10, 2022
[PR #1332/5e1466e9 backport][stable-3] kms_key_info - improve AccessDeniedException handing

This is a backport of PR #1332 as merged into main (5e1466e).
SUMMARY
fixes: #206
Because KMS doesn't support server-side filtering of keys we have to pull full metadata for all KMS keys unless querying a specific key.  This can result in additional permission denied errors, even though we may have permissions to read many of the keys.  Try to handle AccessDeniedException more liberally.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
kms_key_info
ADDITIONAL INFORMATION

Reviewed-by: Mark Chappell <None>
abikouo pushed a commit to abikouo/community.aws that referenced this issue Oct 24, 2023
…ions#1332)

kms_key_info - improve AccessDeniedException handing

SUMMARY
fixes: ansible-collections#206
Because KMS doesn't support server-side filtering of keys we have to pull full metadata for all KMS keys unless querying a specific key.  This can result in additional permission denied errors, even though we may have permissions to read many of the keys.  Try to handle AccessDeniedException more liberally.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
kms_key_info
ADDITIONAL INFORMATION

Reviewed-by: Joseph Torcasso <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@5e1466e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 feature This issue/PR relates to a feature request module module plugins plugin (any type)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants