-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
KMS: on-demand key rotation #12342
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
KMS: on-demand key rotation #12342
Conversation
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
I have read the CLA Document and I hereby sign the CLA |
…n automatic rotation is enabled and disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations on your first contribution! Great work, and thank you for adding integration tests validated against AWS🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! This is great, thanks for adding a new operation and improving parity! Great set of snapshot tests.
I have a few comments about the new attribute on the key that could maybe be on the store itself, another about returning None
values from the provider, and maybe we could add a bit more snapshot match in order to reduce the manual assert
.
Thanks again for the very thorough testing! 🚀
…n `OnDemandRotationStartDate` only while it's "in progress"
@@ -284,6 +284,7 @@ def __init__( | |||
self.crypto_key = KmsCryptoKey(self.metadata.get("KeySpec"), custom_key_material) | |||
self.rotation_period_in_days = 365 | |||
self.next_rotation_date = None | |||
self.on_demand_rotation_start_date = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For simplicity current implementation is limited to tracking a single on demand rotation.
As part of implementing ListKeyRotations
, the model can be evolved to track many.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discussion we've decided not to replicate AWS behavior for on_demand_rotation_start_date
, which means LocalStack will not return this field. Since we don't consider it essential to emulate and it's only used in the get_key_rotation_status
function, please remove support for it from your code, along with all related tests.
To fix tests you can use poll_condition
to wait until OnDemandRotationStartDate
is no longer in the response, just like you did in the test_rotate_key_on_demand_should_return_rotation_start_date_only_while_its_in_progress
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, along with @bentsku comment, are the last two things we need to address. After that, we're good to merge the PR 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both issues fixed 🚀
…ecessary to do it explicitly in `rotate_key_on_demand()`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for addressing the comments so quickly 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! 🚀
Motivation
Motivated by this enhancement request: #10723
Changes
Implementation of the on-demand key rotation feature in KMS.
As per AWS's specs, on-demand key rotation is only allowed for:
TODO
What's left to do:
ListKeyRotations
may be required too, because it'd allow to properly test this functionality)