Skip to content

Add KMS ReEncrypt Operation #12637

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

victor-martin-santiago
Copy link

Motivation

I'm creating this PR to add the implementation for the KMS ReEncrypt operation.

Changes

Adds an implementation for KMS ReEncrypt operation, based on the already existing Encrypt and Decrypt operations,

Testing

The changes have been covered by integration & AWS parity tests.

Optionally, the ReEncrypt operation can be tested using the below aws cli command

aws kms re-encrypt \
	--endpoint-ur=http://localhost:4566 \
	--ciphertext-blob="$encriptedText" \
	--destination-key-id=$destinationKeyId \
	--source-key-id=$sourceKeyId

@localstack-bot
Copy link
Collaborator

localstack-bot commented May 19, 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.

@victor-martin-santiago victor-martin-santiago changed the title Add KMS Re-encrypt Operation Add KMS ReEncrypt Operation May 19, 2025
@victor-martin-santiago victor-martin-santiago marked this pull request as ready for review May 19, 2025 09:38
@victor-martin-santiago
Copy link
Author

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

@victor-martin-santiago
Copy link
Author

recheck

@viren-nadkarni viren-nadkarni added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label May 19, 2025
@viren-nadkarni viren-nadkarni self-requested a review May 20, 2025 08:05
Copy link
Member

@viren-nadkarni viren-nadkarni left a comment

Choose a reason for hiding this comment

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

Thanks for raising the PR!

It looks good overall, I have only some small points

@@ -954,8 +954,29 @@ def re_encrypt(
dry_run: NullableBooleanType = None,
**kwargs,
) -> ReEncryptResponse:
# TODO: when implementing, ensure cross-account support for source_key_id and destination_key_id
Copy link
Member

Choose a reason for hiding this comment

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

Please leave this TODO comment, it will help future multi-accounts work

Choose a reason for hiding this comment

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

TODO restored

],
)
@markers.aws.validated
def test_re_encript(self, kms_create_key, key_spec, algo, aws_client):
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: typo

Suggested change
def test_re_encript(self, kms_create_key, key_spec, algo, aws_client):
def test_re_encrypt(self, kms_create_key, key_spec, algo, aws_client):

Choose a reason for hiding this comment

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

Fixed

"HTTPStatusCode": 200
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This file is generated by the snapshot fixture, but it's not used in the test.

Could you have mistakenly removed it?

Choose a reason for hiding this comment

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

Yes, I removed the snapshot check by mistake during one of my tests. It's been restored.

Copy link
Contributor

@sannya-singal sannya-singal left a comment

Choose a reason for hiding this comment

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

Great work on implementing the ReEncrypt operation for KMS! 🚀

I just have few suggestions to further enhance the parity with AWS by adding a check for verifying the KeyUsage. Once that’s in place, we can do a quick re-review and move forward with merging the PR 🎉 Thank you 🙌

Comment on lines 203 to 208
"tests/aws/services/kms/test_kms.py::TestKMS::test_re_encript[RSA_2048-RSAES_OAEP_SHA_256]": {
"last_validated_date": "2025-05-19T07:54:40+00:00"
},
"tests/aws/services/kms/test_kms.py::TestKMS::test_re_encript[SYMMETRIC_DEFAULT-SYMMETRIC_DEFAULT]": {
"last_validated_date": "2025-05-19T07:54:39+00:00"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

comment: Probably a leftover from testing, we should remove this.

Choose a reason for hiding this comment

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

You are right, it's been removed.

account_id, region_name, source_key_id = self._parse_key_id(source_key_id, context)
source_key = self._get_kms_key(account_id, region_name, source_key_id)
# Decrypt using source key
decrypt_response = self.decrypt(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: It would be great to add a check to ensure that both the source and destination keys have KeyUsage set to ENCRYPT_DECRYPT.

This aligns with AWS KMS behaviour, which restricts ReEncrypt, Encrypt, and Decrypt operations to keys explicitly configured for encryption and decryption. Attempting to use invalid keys with other usages should raise an exception similar to AWS.

Adding this validation would improve spec compliance and provide clearer feedback to users if they accidentally use a key with an incompatible purpose.

Let me know if you'd like help drafting the validation logic or a corresponding test case!

Choose a reason for hiding this comment

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

Thank you for your review @sannya-singal. That sounds like a good idea 😁. I'll get that pushed throughout the day.

Copy link
Author

@victor-martin-santiago victor-martin-santiago May 21, 2025

Choose a reason for hiding this comment

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

Regarding the validation @sannya-singal

When looking at the code again, I realised the KeyUsage check already happens in both the Encrypt and Decrypt operations. So I think adding the check again before the invocation would be redundant.

On the other hand, I've added tests for these two scenarios as I didn't find any for the already existing Encrypt and Decrypt operations. However, two remarks:

  • test_re_encrypt_incorrect_source_key: Since the cipher text's key is verified before validating the key usage, I could only test for IncorrectKeyException. Unsure if there's another way to test for InvalidKeyUsageException.
  • test_re_encrypt_invalid_destination_key : I had to disable the snapshot check as I'm not quite sure where the ReEncryptTo operation name is coming from.

Thank you!

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