Skip to content

KMS #12530: Error handling for invalid blob issue #12809

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 18, 2025
Merged

KMS #12530: Error handling for invalid blob issue #12809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion localstack-core/localstack/services/kms/provider.py
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

question: do you think it's worth logging an error message on line 1047 where we catch and silence any exceptions?

Copy link
Contributor Author

@pbansal2 pbansal2 Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, agree. I should have done it. We should log the error before line 1047 to avoid hiding potential bugs caused by catching all exceptions without logging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this change should be part of this PR or a separate one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made this change in this PR only.

Original file line number Diff line number Diff line change
Expand Up @@ -1043,7 +1043,8 @@ def decrypt(
account_id, region_name, key_id = self._parse_key_id(key_id, context)
try:
ciphertext = deserialize_ciphertext_blob(ciphertext_blob=ciphertext_blob)
except Exception:
except Exception as e:
logging.error("Error deserializing ciphertext blob: %s", e)
ciphertext = None
pass
else:
Expand Down Expand Up @@ -1072,6 +1073,9 @@ def decrypt(
if self._is_rsa_spec(key.crypto_key.key_spec) and not ciphertext:
plaintext = key.decrypt_rsa(ciphertext_blob)
else:
# if symmetric encryption then ciphertext must not be None
if ciphertext is None:
raise InvalidCiphertextException()
plaintext = key.decrypt(ciphertext, encryption_context)
except InvalidTag:
raise InvalidCiphertextException()
Expand Down
12 changes: 11 additions & 1 deletion tests/aws/services/kms/test_kms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from cryptography.hazmat.primitives.serialization import load_der_public_key

from localstack.services.kms.models import (
HEADER_LEN,
IV_LEN,
ON_DEMAND_ROTATION_LIMIT,
Ciphertext,
Expand Down Expand Up @@ -1787,7 +1788,7 @@ def test_plaintext_size_for_encrypt(self, kms_create_key, snapshot, aws_client):
snapshot.match("invalid-plaintext-size-encrypt", e.value.response)

@markers.aws.validated
@markers.snapshot.skip_snapshot_verify(paths=["$..message"])
@markers.snapshot.skip_snapshot_verify(paths=["$..message", "$..KeyMaterialId"])
def test_encrypt_decrypt_encryption_context(self, kms_create_key, snapshot, aws_client):
key_id = kms_create_key()["KeyId"]
message = b"test message 123 !%$@ 1234567890"
Expand Down Expand Up @@ -1819,6 +1820,15 @@ def test_encrypt_decrypt_encryption_context(self, kms_create_key, snapshot, aws_
)
snapshot.match("decrypt_response_without_encryption_context", e.value.response)

with pytest.raises(ClientError) as e:
aws_client.kms.decrypt(
KeyId=key_id,
CiphertextBlob=ciphertext[HEADER_LEN:],
EncryptionAlgorithm=algo,
EncryptionContext=encryption_context,
)
snapshot.match("decrypt_response_with_invalid_ciphertext", e.value.response)

@markers.aws.validated
def test_get_parameters_for_import(self, kms_create_key, snapshot, aws_client):
sign_verify_key = kms_create_key(
Expand Down
13 changes: 12 additions & 1 deletion tests/aws/services/kms/test_kms.snapshot.json
View file
Open in desktop
Copy link
Contributor

@simonrw simonrw Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@pbansal2 pbansal2 Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonrw I am not seeing KeyMaterialId or any other changes in block decrypt_response_with_encryption_context in test_kms_snapshot.json
I am testing with following command after setting required env variables to update the snapshot and to execute the against the AWS. I can also see keys created in my AWS account.
TEST_PATH="tests/aws/services/kms/test_kms.py::TestKMS::test_encrypt_decrypt_encryption_context" make test
This is updated snapshot block after running above test:

diff --git a/tests/aws/services/kms/test_kms.snapshot.json b/tests/aws/services/kms/test_kms.snapshot.json
index 17ebf79f2..e33911e7e 100644
--- a/tests/aws/services/kms/test_kms.snapshot.json
+++ b/tests/aws/services/kms/test_kms.snapshot.json
@@ -1565,7 +1565,7 @@
     }
   },
   "tests/aws/services/kms/test_kms.py::TestKMS::test_encrypt_decrypt_encryption_context": {
-    "recorded-date": "11-05-2023, 22:46:49",
+    "recorded-date": "01-07-2025, 17:01:36",
     "recorded-content": {
       "encrypt_response": {
         "CiphertextBlob": "ciphertext-blob",
@@ -1594,6 +1594,16 @@
           "HTTPHeaders": {},
           "HTTPStatusCode": 400
         }
+      },
+      "decrypt_response_with_invalid_ciphertext": {
+        "Error": {
+          "Code": "InvalidCiphertextException",
+          "Message": ""
+        },
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 400
+        }
       }
     }
   },

could you please suggest anything wrong with these steps test against AWS?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the How to write parity tests section of our docs, you should find the instructions on how to run the tests against AWS.

Specifically:

Run the test against AWS: use the parameter --snapshot-update (or the environment variable SNAPSHOT_UPDATE=1) and set the environment variable as TEST_TARGET=AWS_CLOUD.

You have run the tests with your AWS credentials, however you are not configuring the LocalStack test suite to target AWS.

I am testing with following command after setting required env variables to update the snapshot and to execute the against the AWS. I can also see keys created in my AWS account.
TEST_PATH="tests/aws/services/kms/test_kms.py::TestKMS::test_encrypt_decrypt_encryption_context" make test

I can't really explain what is going on here unfortunately. You are clearly

  • updating the snapshot with SNAPSHOT_UPDATE=1 since it has changed, and
  • targeting AWS with TEST_TARGET=AWS_CLOUD since you say the keys are created (but they should only be temporary, they should be scheduled for deletion after the test completes, right?)

but when I re-run the test with the same configuration, I find the KeyMaterialId property has been added

diff --git i/tests/aws/services/kms/test_kms.snapshot.json w/tests/aws/services/kms/test_kms.snapshot.json
index e97a3097e6d7..33e987b371d7 100644
--- i/tests/aws/services/kms/test_kms.snapshot.json
+++ w/tests/aws/services/kms/test_kms.snapshot.json
@@ -1565,7 +1565,7 @@
     }
   },
   "tests/aws/services/kms/test_kms.py::TestKMS::test_encrypt_decrypt_encryption_context": {
-    "recorded-date": "27-06-2025, 08:31:49",
+    "recorded-date": "04-07-2025, 10:40:02",
     "recorded-content": {
       "encrypt_response": {
         "CiphertextBlob": "ciphertext-blob",
@@ -1579,6 +1579,7 @@
       "decrypt_response_with_encryption_context": {
         "EncryptionAlgorithm": "SYMMETRIC_DEFAULT",
         "KeyId": "<key-id:1>",
+        "KeyMaterialId": "4ba2a196f27b13af15cce4730e67b632eff7b53b68f987b7331d209fddc0b103",
         "Plaintext": "plaintext",
         "ResponseMetadata": {
           "HTTPHeaders": {},

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.

If you are executing the test against AWS and capturing snapshots, then it captures all snapshot.match values, not just the one that you have changed. So it's not unexpected that even though you have not changed the code that contributes to the decrypt_response_with_encryption_context snapshot content, that it may change.

This is a challenge we face regularly, you see the last time this snapshot was executed was 2023! We regularly find the AWS responses have changed since the last time the snapshot was captured.

When I re-run the entire kms test suite against AWS I see

  • other snapshot responses have additional keys (also relating to key material so likely related)
  • some tests fail which is interesting, they need revisiting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

targeting AWS with TEST_TARGET=AWS_CLOUD since you say the keys are created (but they should only be temporary, they should be scheduled for deletion after the test completes, right?)

Yes, they were scheduled for deletion right after test was finished.

If you are executing the test against AWS and capturing snapshots, then it captures all snapshot.match values, not just the one that you have changed. So it's not unexpected that even though you have not changed the code that contributes to the decrypt_response_with_encryption_context snapshot content, that it may change.

Understood. Thanks!
I also tried running the entire KMS test suite but couldn't see keymaterialId in decrypt_response_with_encryption_context or any other test function in KMS suite.
I can also see few tests failing in KMS suite.

Copy link
Contributor Author

@pbansal2 pbansal2 Jul 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I tested two other setups as well to repro it. It seems some issue in my Localstack set up probably.

  1. Outside Localstack: I can seekeymaterialId in the response of decrypt() from AWS.
  2. Within Localstack using awslocal CLI: Same issue as KMS test suite where there is no keymaterialId from decrypt with encryption context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonrw It seems that earlier I added the fix and test the changes on Localstack version 4.3 where the issue was raised by user. However, probably I didn't test it again on master before commiting my changes.

I have tested again on master and able to see keyMaterialId in response for decrypt_response_with_encryption_context. I have made this change in this PR now. Please review it once. Thank you!

Original file line number Diff line number Diff line change
Expand Up @@ -1565,7 +1565,7 @@
}
},
"tests/aws/services/kms/test_kms.py::TestKMS::test_encrypt_decrypt_encryption_context": {
"recorded-date": "11-05-2023, 22:46:49",
"recorded-date": "08-07-2025, 05:53:27",
"recorded-content": {
"encrypt_response": {
"CiphertextBlob": "ciphertext-blob",
Expand All @@ -1579,6 +1579,7 @@
"decrypt_response_with_encryption_context": {
"EncryptionAlgorithm": "SYMMETRIC_DEFAULT",
"KeyId": "<key-id:1>",
"KeyMaterialId": "e2333676b9bf055cb0caa2bec3957d7f3e60b7545a3706314e397746cd26122e",
"Plaintext": "plaintext",
"ResponseMetadata": {
"HTTPHeaders": {},
Expand All @@ -1594,6 +1595,16 @@
"HTTPHeaders": {},
"HTTPStatusCode": 400
}
},
"decrypt_response_with_invalid_ciphertext": {
"Error": {
"Code": "InvalidCiphertextException",
"Message": ""
},
"ResponseMetadata": {
"HTTPHeaders": {},
"HTTPStatusCode": 400
}
}
}
},
Expand Down
8 changes: 7 additions & 1 deletion tests/aws/services/kms/test_kms.validation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,13 @@
"last_validated_date": "2024-04-11T15:53:18+00:00"
},
"tests/aws/services/kms/test_kms.py::TestKMS::test_encrypt_decrypt_encryption_context": {
"last_validated_date": "2024-04-11T15:54:22+00:00"
"last_validated_date": "2025-07-08T05:53:27+00:00",
"durations_in_seconds": {
"setup": 0.74,
"call": 1.08,
"teardown": 0.15,
"total": 1.97
}
},
"tests/aws/services/kms/test_kms.py::TestKMS::test_encrypt_validate_plaintext_size_per_key_type[RSA_2048-RSAES_OAEP_SHA_1]": {
"last_validated_date": "2024-04-11T15:53:20+00:00"
Expand Down
Loading