Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

KMS: RotateKeyOnDemand api update#12806

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
willdefig wants to merge20 commits intolocalstack:main
base:main
Choose a base branch
Loading
fromwilldefig:master

Conversation

willdefig
Copy link

@willdefigwilldefig commentedJun 26, 2025•
edited
Loading

Motivation

In response to the latest changes to KMS that AWS have made it is now possible to rotate keys on demand if they are and external key,
https://aws.amazon.com/blogs/security/how-to-use-on-demand-rotation-for-aws-kms-imported-keys/

Also there is a new item in the metadata for the key to show the CurrentKeyMaterialId this should update when the key is rotated.

Changes

  • AllowRotateKeyOnDemand to acceptEXTERNAL Type keys

Tests

  • Addedtest_key_before_and_after_rotations_on_imported_key to fully test the import and rotation of anEXTERNAL key

resolves#12801

sannya-singal and k-a-il reacted with hooray emoji
@localstack-bot
Copy link
Contributor

localstack-bot commentedJun 26, 2025•
edited
Loading

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

@sannya-singalsannya-singal added this to thePlayground milestoneJun 26, 2025
@sannya-singalsannya-singal added semver: minorNon-breaking changes which can be included in minor releases, but not in patch releases aws:kmsAWS Key Management Service labelsJun 26, 2025
@willdefig
Copy link
Author

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

@willdefig
Copy link
Author

recheck

localstack-bot added a commit that referenced this pull requestJun 26, 2025
Copy link
Contributor

@k-a-ilk-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.

Hi@willdefig thank you for your contribution 🚀
As mentioned in my comment below, I believe it’s important to add the test case described in thearticle to check that this implementation aligns with AWS behavior. Without this, the behavior may not fully match AWS KMS behavior.

- Creates Key- Imports custom Key Material- Creates Encrypted data key- Tests it can use it to decrypt- Creates new key material- Imports new material- Rotates new material- Creates new encrypted key data- Tests it can decrypt that- Tests it can still decrypt old key data
 - updated test to include checking rotated keys are still usable. - Tidied up import_key_material importing for EXTERNAL keys -  rotate_key_on_demand stopped key being recreated if using EXTERNAL keys
 - Fix for TestKMS::test_key_rotations_limits failing due to the last change
@localstack-bot
Copy link
Contributor

Currently, only patch changes are allowed on master. Your PR labels (aws:kms, semver: minor) indicate that it cannot be merged into the master at this time.

Copy link
Contributor

@sannya-singalsannya-singal 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 🚀 Thanks for addressing the review comments 🙂

I have some more suggestions related to adding a test case to validate theDeleteImportedKeyMaterial changes, adding validations for different key types during the key import material and modifying the snapshotted AWS response. Please let me know if you require more context or help here 🙂

@@ -1208,7 +1208,7 @@
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for removing the unrelated snapshot validation changes! 🚀

ImportType="NEW_KEY_MATERIAL",
)
response = aws_client.kms.rotate_key_on_demand(KeyId=key_id)
response["KeyId"] = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch here! 🚀 🎉

I suggest we rather add a fix for this in our handler and do not modify the AWS response forrotate_key_on_demand or alternatively leave a TODO for it and this could be fixed in future 🙂

@@ -1216,6 +1225,19 @@ def delete_imported_key_material(
key.metadata["Enabled"] = False
key.metadata["KeyState"] = KeyState.PendingImport
key.metadata.pop("ExpirationModel", None)
if key.metadata["Origin"] == "EXTERNAL":
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if we additionally add a test case to verify the implementation against AWS here 🙂

return ImportKeyMaterialResponse(
KeyId=key_to_import_material_to.metadata["KeyId"],
KeyMaterialId=key_material.hex(),
)
return ImportKeyMaterialResponse()
Copy link
Contributor

Choose a reason for hiding this comment

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

For the responseImportKeyMaterialResponse(), do we have a potential use case where it could be returned?

According to AWS documentation:https://docs.aws.amazon.com/kms/latest/APIReference/API_ImportKeyMaterial.html the response syntax should be:

{
"KeyId": "string",
"KeyMaterialId": "string"
}

Similar tohttps://github.com/localstack/localstack/pull/12806/files#diff-e8657a5d40e0464d11c6289bcc150c964813e3a9efdc6b21c7c513cec8e4f73eR1204-R1206 in your implementation.

@@ -1193,8 +1193,18 @@ def import_key_material(
# TODO actually set validTo and make the key expire
key_to_import_material_to.metadata["Enabled"] = True
key_to_import_material_to.metadata["KeyState"] = KeyState.Enabled
key_to_import_material_to.crypto_key.load_key_material(key_material)

if key_to_import_material_to.metadata["Origin"] == "EXTERNAL":
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in our call yesterday, I think we could also check if this works against different key types?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@k-a-ilk-a-ilk-a-il left review comments

@sannya-singalsannya-singalsannya-singal requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@k-a-ilk-a-il

Labels
aws:kmsAWS Key Management Servicesemver: minorNon-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
Playground
Development

Successfully merging this pull request may close these issues.

bug: KMS RotateKeyOnDemand now supports imported keys
4 participants
@willdefig@localstack-bot@sannya-singal@k-a-il
[8]ページ先頭
©2009-2025 Movatter.jp