KMS: Fixup ImportKeyMaterial for non-symmetric keys #10116
Conversation
There was a problem hiding this comment.
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.
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hi @uubk, thanks for contributing to localstack 🚀. I have requested a few changes, kindly also make sure that the CI checks are green. 🙂 |
sannya-singal
added
semver: minor
uubk
force-pushed
the
fixup-kms-ecc-import
branch
2 times, most recently
from
e7868a1
to
e4fe42b
Compare
Hey - thanks for the review! |
localstack/services/kms/models.py
Outdated
| key = load_der_private_key(material, password=None) | ||
| self.public_key = key.public_key().public_bytes( | ||
| crypto_serialization.Encoding.DER, | ||
| crypto_serialization.PublicFormat.SubjectPublicKeyInfo, | ||
| ) | ||
| self.private_key = key.private_bytes( | ||
| crypto_serialization.Encoding.DER, | ||
| crypto_serialization.PrivateFormat.PKCS8, | ||
| crypto_serialization.NoEncryption(), | ||
| ) |
There was a problem hiding this comment.
I see similar logic used in
localstack/localstack/services/kms/models.py
Lines 194 to 201 in 7ac868b
Can we optimise this code?
There was a problem hiding this comment.
I've moved it to its own function
This should be fixed by #10459, kindly rebase the PR. |
AWS KMS allows importing keys for any of the supported KMS operations, not just encryption/decryption. This was already fixed for GetParametersForImport (#9111), however, the check for ImportKeyMaterial remained - lets drop it.
When calling ImportKeyMaterial, we cannot assume that we always import a symmetric key. Check for this case in KmsCryptoKey and make sure we set the correct member. Also note that not all asymmetric keys are RSA.
RSAES_PKCS1_V1_5 was dropped from the API on October 10th 2023 (https://docs.aws.amazon.com/kms/latest/developerguide/importing-keys-get-public-key-and-token.html#select-wrapping-algorithm)
Unfortunately, the _name_ of the third argument to key.sign() appears to be _platform dependant_. Split up the argument construction so that we can avoid referring to it by name.
Much like the case for symmetric keys, this test generates a sample key and imports it, but uses it for sign/verify instead of encrypt/decrypt.
uubk
force-pushed
the
fixup-kms-ecc-import
branch
from
e4fe42b
to
e4dc0ef
Compare
uubk
force-pushed
the
fixup-kms-ecc-import
branch
from
e4dc0ef
to
229f324
Compare
Yes, can confirm! The pipeline is now green, thanks! |
|
Thanks @uubk for these changes and fixing reviews patiently! 🎉 LGTM! |
Motivation
KMS allows you to import key material, that is, create KMS keys with "private keys" you already have. In the context of Localstack this is fairly important now that the old "local-kms" backend was removed since the new backend cannot be preseeded at the moment and controlling key material can be very important in testing.
Changes
This PR drops the restriction on encrypt/decrypt for the KeyUsage in ImportKeyMaterial (the restriction for GetParametersForImport was already dropped as part of the fix for #9111) and should therefore fix #10115. It then fixes the issues that crop up when trying to use this with existing code - notably the hardcoded assumption that all imported key material is a symmetric key. Finally, it adds a new test to track this usecase and fixes a few other small issues you bump into when you update the KMS test snapshots.