KMS: RotateKeyOnDemand api update #12806
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
There was a problem hiding this comment.
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 the article 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
|
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. |
There was a problem hiding this comment.
Nice work 🚀 Thanks for addressing the review comments 🙂
I have some more suggestions related to adding a test case to validate the DeleteImportedKeyMaterial 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 @@ | |||
| } | |||
There was a problem hiding this comment.
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"] = ( |
There was a problem hiding this comment.
Great catch here! 🚀 🎉
I suggest we rather add a fix for this in our handler and do not modify the AWS response for rotate_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": | |||
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
For the response ImportKeyMaterialResponse(), 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 to https://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": | |||
There was a problem hiding this comment.
As discussed in our call yesterday, I think we could also check if this works against different key types?
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
RotateKeyOnDemandto acceptEXTERNALType keysTests
test_key_before_and_after_rotations_on_imported_keyto fully test the import and rotation of anEXTERNALkeyresolves #12801