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

Ignore KMS keys that are pending deletion in rotation check #462

Closed
wants to merge 2 commits into from
Closed

Ignore KMS keys that are pending deletion in rotation check #462

wants to merge 2 commits into from

Conversation

yorinasub17
Copy link
Contributor

Benchmark where identified

  • CIS v1.4.0

Description

This updates the KMS CMK rotation enabled query to skip any KMS keys that are in the Pending Deletion state. These keys are marked for deletion, and thus it doesn't matter if rotation is enabled or not, so best to skip them to keep the report clean.

If the key is ever restored and put out of pending deletion, then the state will change and should show up in the report the next time it runs.

Sample output

(data has been sanitized to mask real IDs)

| + 3.8 Ensure rotation for customer created CMKs is enabled ............................................................................ 0 /   6 [=         ]
| | |
| | OK   : KEY_ID key rotation enabled. ................................................................. us-east-1 000000000000
| | OK   : KEY_ID key rotation enabled. ................................................................. us-east-1 000000000000
| | OK   : KEY_ID key rotation enabled. ................................................................. us-east-1 000000000000
| | SKIP : KEY_ID is pending deletion. .................................................................. us-east-1 000000000000
| | SKIP : KEY_ID is pending deletion. .................................................................. us-east-1 000000000000
| | SKIP : KEY_ID is pending deletion. .................................................................. us-east-1 000000000000
|

@misraved misraved changed the base branch from main to release/v0.40 July 15, 2022 04:48
@misraved
Copy link
Contributor

Thanks @yorinasub17 for such detailed insight 👍 .

Apart from checking if the key is in PendingDeletion state, I think we could also add a check if the key is in Disabled state or not, since we cannot set rotation for disabled keys.

Relevant AWS docs - https://docs.aws.amazon.com/kms/latest/developerguide/rotate-keys.html

@yorinasub17
Copy link
Contributor Author

Apart from checking if the key is in PendingDeletion state, I think we could also add a check if the key is in Disabled state or not, since we cannot set rotation for disabled keys.

Good point. Updated to also check for disabled keys: 8c53ca0

Here is sample output on the new version:

| + 3.8 Ensure rotation for customer created CMKs is enabled ...........................................................................  0 /   7 [=         ]
| | |
| | OK   : KEY_ID key rotation enabled. ................................................................. us-east-1 000000000000
| | OK   : KEY_ID key rotation enabled. ................................................................. us-east-1 000000000000
| | OK   : KEY_ID key rotation enabled. ................................................................. us-east-1 000000000000
| | SKIP : KEY_ID is pending deletion. .................................................................. us-east-1 000000000000
| | SKIP : KEY_ID is pending deletion. .................................................................. us-east-1 000000000000
| | SKIP : KEY_ID is disabled. .......................................................................... us-east-1 000000000000
| | SKIP : KEY_ID is pending deletion. .................................................................. us-east-1 000000000000

@misraved misraved deleted the branch turbot:release/v0.40 July 15, 2022 14:54
@misraved misraved closed this Jul 15, 2022
@misraved
Copy link
Contributor

I am extremely sorry @yorinasub17 for closing the PR since we released a new version v0.40 of the compliance mod.

Could you please raise the PR again? Sorry for the inconvenience 😢 .

@yorinasub17
Copy link
Contributor Author

No problem! Just opened #466

@misraved
Copy link
Contributor

Thanks a lot @yorinasub17 👍.

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