KMS #12530: Error handling for invalid blob issue #12809
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.
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
pbansal2
changed the title
alexrashed
added
aws:kms
There was a problem hiding this comment.
question: when I re-run the tests against AWS, I find that a KeyMaterialId field in the decrypt_response_with_encryption_context block of this snapshot is added, which is not captured in this PR. Can you explain why? Did you execute these tests against AWS, or manually update the snapshot file?
You can see more details about our parity testing approach here: https://github.com/localstack/localstack/tree/master/docs/testing/parity-testing
There was a problem hiding this comment.
I executed the tests against AWS that generated the new block (decrypt_response_with_invalid_ciphertext) in test_kms.snapshot.json. I don't expect any changes in response for decrypt_response_with_encryption_context with my changes in test.
Let me test this again to double check.
There was a problem hiding this comment.
question: do you think it's worth logging an error message on line 1047 where we catch and silence any exceptions?
There was a problem hiding this comment.
Yes, agree. We should log the error before line 1047 to avoid hiding potential bugs caused by catching all exceptions without logging.
Motivation
User reported a issue #12530 that causes a attribute error in Localstack. The error doesn't guide or provide clue to user on the problem. User is trying to decrypt the ciphertext blob by providing KeyId using Lambda function.
This PR fixes how LocalStack handles corrupted blobs during decryption when KeyId is provided by user. The fix ensures LocalStack's response matches AWS behavior.
Changes
Localstack will mimic AWS behavior i.e. it will generate same response that AWS would generate. It will help user to validate the blob text that need to be decrypted using Localstack.
Testing
Add a snaphshot test case in existing test method test_encrypt_decrypt_encryption_context() because AWS response is same. Newl response from AWS is added to test_kms.snapshot.json.