8000 KMS: on-demand key rotation by agseco · Pull Request #12342 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 27 commits into from
Mar 13, 2025
Merged

Conversation

agseco
Copy link
Contributor
@agseco agseco commented Mar 5, 2025

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:

  • Symmetric keys
  • Keys which DO NOT import custom key material

TODO

What's left to do:

  • Preserve old key material, as specified in AWS's docs (implementing ListKeyRotations may be required too, because it'd allow to properly test this functionality)
  • Consider multi-region keys

@localstack-bot
Copy link
Collaborator
localstack-bot commented Mar 5, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

Copy link
Collaborator
@localstack-bot localstack-bot left a 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.

@agseco
Copy link
Contributor Author
agseco commented Mar 5, 2025

I have read the CLA Document and I hereby sign the CLA

localstack-bot added a commit that referenced this pull request Mar 5, 2025
@k-a-il k-a-il requested review from bentsku and k-a-il March 5, 2025 11:39
@bentsku bentsku added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Mar 5, 2025
@agseco agseco marked this pull request as ready for review March 6, 2025 10:55
@agseco agseco requested a review from sannya-singal as a code owner March 6, 2025 10:55
Copy link
Contributor
@k-a-il k-a-il left a 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🚀

Copy link
Contributor
@bentsku bentsku left a 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! 🚀

@@ -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
Copy link
Contributor Author
@agseco agseco Mar 11, 2025

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.

Copy link
Contributor

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.

Copy link
Contributor

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 🚢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both issues fixed 🚀

@k-a-il k-a-il removed the request for review from sannya-singal March 12, 2025 17:01
Copy link
Contributor
@bentsku bentsku left a 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 🚀

@k-a-il k-a-il self-requested a review March 12, 2025 20:35
Copy link
Contributor
@k-a-il k-a-il left a 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! 🚀

@k-a-il k-a-il merged commit d6fbc57 into localstack:master Mar 13, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0