KMS: Fix double hashing during sign/verify operations by viren-nadkarni · Pull Request #7878 · localstack/localstack

viren-nadkarni · 2023-03-15T11:37:43Z

This PR is based on investigation done by @paullallier in #6815 (comment)

LocalStack KMS Sign and Verify operations were double hashing the message payloads. This PR reworks these operations to rely on cryptography to perform hashing if required based on the value of MessageType argument.

The integration tests are also refactored to make the correct assertions.

Also checked that signatures produced by KMS:Sign can be correctly verified with OpenSSL (i.e. outside of the KMS:Verify operation).

$ cat msg.txt
hello world

$ awslocal kms create-key --key-spec RSA_4096 --key-usage SIGN_VERIFY

{
    "KeyMetadata": {
        "AWSAccountId": "000000000000",
        "KeyId": "54ec6e0d-7356-4b08-897c-26eb7b134939",
        "Arn": "arn:aws:kms:us-east-1:000000000000:key/54ec6e0d-7356-4b08-897c-26eb7b134939",
        "CreationDate": "2023-03-15T23:06:05.913720+05:30",
        "Enabled": true,
        "Description": "",
        "KeyUsage": "SIGN_VERIFY",
        "KeyState": "Enabled",
        "Origin": "AWS_KMS",
        "KeyManager": "CUSTOMER",
        "CustomerMasterKeySpec": "RSA_4096",
        "KeySpec": "RSA_4096",
        "SigningAlgorithms": [
            "RSASSA_PKCS1_V1_5_SHA_256",
            "RSASSA_PKCS1_V1_5_SHA_384",
            "RSASSA_PKCS1_V1_5_SHA_512",
            "RSASSA_PSS_SHA_256",
            "RSASSA_PSS_SHA_384",
            "RSASSA_PSS_SHA_512"
        ],
        "MultiRegion": false
    }
}

$ awslocal kms get-public-key --key-id 54ec6e0d-7356-4b08-897c-26eb7b134939 --output text --query PublicKey | base64 -d > pubkey.der

$ openssl rsa -pubin -inform DER     -outform PEM -in pubkey.der     -pubout -out pubkey.pem
writing RSA key

$ awslocal kms sign --key-id 54ec6e0d-7356-4b08-897c-26eb7b134939 --message-type RAW --message fileb://msg.txt --signing-algorithm RSASSA_PKCS1_V1_5_SHA_512 --output text --query Signature | base64 -d > sig.sig

$ openssl dgst -sha512     -verify pubkey.pem     -signature sig.sig msg.txt
Verified OK

Supersedes #7817

Fixes #6815, #7158

viren-nadkarni · 2023-03-15T12:28:59Z

tests/integration/test_kms.py

+    def test_invalid_key_usage(self, kms_client, kms_create_key):
+        key_id = kms_create_key(KeyUsage="ENCRYPT_DECRYPT", KeySpec="RSA_4096")["KeyId"]
+        with pytest.raises(ClientError) as exc:
+            kms_client.sign(
+                MessageType="RAW",
+                Message="hello",
+                KeyId=key_id,
+                SigningAlgorithm="RSASSA_PSS_SHA_256",
+            )
+        assert exc.match("InvalidKeyUsageException")
+
+        key_id = kms_create_key(KeyUsage="SIGN_VERIFY", KeySpec="RSA_4096")["KeyId"]
+        with pytest.raises(ClientError) as exc:
+            kms_client.encrypt(
+                Plaintext="hello",
+                KeyId=key_id,
+                EncryptionAlgorithm="RSAES_OAEP_SHA_256",
+            )
+        assert exc.match("InvalidKeyUsageException")
+
+    @pytest.mark.parametrize(
+        "key_spec,algo",
+        [
+            ("SYMMETRIC_DEFAULT", "SYMMETRIC_DEFAULT"),
+            ("RSA_2048", "RSAES_OAEP_SHA_256"),
+            ("ECC_NIST_P256", "RSAES_OAEP_SHA_1"),
+            ("ECC_SECG_P256K1", "RSAES_OAEP_SHA_256"),
+            # ("HMAC_256", "SYMMETRIC_DEFAULT"),  # currently not supported in LocalStack
+            # ("SM2", "SM2PKE"),  # currently not supported in LocalStack
+        ],
+    )
+    def test_encrypt_decrypt(self, kms_client, kms_create_key, key_spec, algo):
+        key_id = kms_create_key(KeyUsage="ENCRYPT_DECRYPT", KeySpec=key_spec)["KeyId"]
        message = b"test message 123 !%$@ 1234567890"
-        algo = "RSASSA_PSS_SHA_256" if key_type == "rsa" else "ECDSA_SHA_256"
-        kwargs = {"KeyId": key_id, "Message": message, "SigningAlgorithm": algo}
-
-        signature_for_plaintext = kms_client.sign(MessageType="RAW", **kwargs)["Signature"]
-        signature_for_digest = kms_client.sign(MessageType="DIGEST", **kwargs)["Signature"]
-        assert signature_for_plaintext != signature_for_digest
-
-        # These following blocks basically test that MessageType="DIGEST" results in a different outcome
-        # from the default MessageType="RAW". As long as both Sign and Verify are called with the same MessageType,
-        # everything should work, while if this parameter mismatches between such two calls - the verifications is
-        # supposed to fail.
-        #
-        # There is a possibility that I do not understand how digests work, so can't write better tests. If we have
-        # any issues with the digests - know that the current approach is not a calculated design, but rather a guess.
-        signature = kms_client.sign(MessageType="RAW", **kwargs)["Signature"]
-        assert kms_client.verify(MessageType="RAW", Signature=signature, **kwargs)["SignatureValid"]
-
-        signature = kms_client.sign(MessageType="RAW", **kwargs)["Signature"]
-        with pytest.raises(kms_client.exceptions.KMSInvalidSignatureException):
-            assert not kms_client.verify(MessageType="DIGEST", Signature=signature, **kwargs)[
-                "SignatureValid"
-            ]
-
-        signature = kms_client.sign(MessageType="DIGEST", **kwargs)["Signature"]
-        with pytest.raises(kms_client.exceptions.KMSInvalidSignatureException):
-            assert not kms_client.verify(MessageType="RAW", Signature=signature, **kwargs)[
-                "SignatureValid"
-            ]
-
-        signature = kms_client.sign(MessageType="DIGEST", **kwargs)["Signature"]
-        assert kms_client.verify(MessageType="DIGEST", Signature=signature, **kwargs)[
-            "SignatureValid"
-        ]
+        ciphertext = kms_client.encrypt(
+            KeyId=key_id, Plaintext=base64.b64encode(message), EncryptionAlgorithm=algo
+        )["CiphertextBlob"]
+        plaintext = kms_client.decrypt(
+            KeyId=key_id, CiphertextBlob=ciphertext, EncryptionAlgorithm=algo
+        )["Plaintext"]
+        assert base64.b64decode(plaintext) == message


New tests to cover Encrypt and Decrypt

coveralls · 2023-03-15T13:14:08Z

Coverage: 85.121% (+0.01%) from 85.109% when pulling 8392dfd on kms-sign-verify into 3cb5069 on master.

whummer

Awesome set of changes @viren-nadkarni ! 🚀 Also big kudos to @paullallier for the detailed analysis of the issue and the initial implementation 🙌

Changes LGTM, only one small comment related to error handling.. In a future iteration, we could maybe introduce a few snapshots.

localstack/services/kms/models.py

whummer · 2023-03-15T13:48:49Z

tests/integration/test_kms.py

+        # Ensure bad digest raises during signing
+        with pytest.raises(ClientError) as exc:
+            kms_client.sign(MessageType="DIGEST", Message=plaintext, **kwargs)
+        assert exc.match("ValidationException")


small nit: Since the tests are already AWS validated, we could also do a snapshot.match(..) here, which would also assert the other elements of the error response. (no need to change now, though, we could tackle the snapshots in a follow-up iteration 👍 ).

github-actions · 2023-03-15T14:18:49Z

LocalStack integration with Pro

      3 files ±0       3 suites ±0 1h 40m 9s ⏱️ - 2m 11s
1 805 tests +7 1 424 ✔️ +8 381 💤 - 1 0 ❌ ±0
2 523 runs - 1 1 790 ✔️ ±0 733 💤 - 1 0 ❌ ±0

Results for commit 8392dfd. ± Comparison against base commit 3cb5069.

♻️ This comment has been updated with latest results.

paullallier · 2023-03-17T09:35:07Z

Thanks for getting this one into the codebase, @viren-nadkarni.

Fix double hashing during sign/verify operations

d8e2dff

viren-nadkarni self-assigned this Mar 15, 2023

viren-nadkarni temporarily deployed to localstack-ext-tests March 15, 2023 11:37 — with GitHub Actions Inactive

viren-nadkarni marked this pull request as ready for review March 15, 2023 12:10

viren-nadkarni mentioned this pull request Mar 15, 2023

KMS sign: Avoid rehashing the already hashed message #7817

Closed

viren-nadkarni requested a review from whummer March 15, 2023 12:16

Fix bad argument

a6ace50

viren-nadkarni temporarily deployed to localstack-ext-tests March 15, 2023 12:28 — with GitHub Actions Inactive

viren-nadkarni commented Mar 15, 2023

View reviewed changes

whummer approved these changes Mar 15, 2023

View reviewed changes

Raise exception for invalid hashing algo

cddd12a

viren-nadkarni temporarily deployed to localstack-ext-tests March 16, 2023 12:10 — with GitHub Actions Inactive

Enable snapshot testing

8392dfd

viren-nadkarni temporarily deployed to localstack-ext-tests March 16, 2023 14:00 — with GitHub Actions Inactive

viren-nadkarni merged commit 1dfc637 into master Mar 17, 2023

viren-nadkarni deleted the kms-sign-verify branch March 17, 2023 06:44

sannya-singal mentioned this pull request Jul 3, 2023

[KMS] For Sign / Verify - confirm that it is necessary and implement digest calculation based on SigningAlgorithm. #7158

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

KMS: Fix double hashing during sign/verify operations#7878

KMS: Fix double hashing during sign/verify operations#7878
viren-nadkarni merged 4 commits intomasterfrom
kms-sign-verify

viren-nadkarni commented Mar 15, 2023 •

edited

Loading

Uh oh!

viren-nadkarni Mar 15, 2023

Uh oh!

coveralls commented Mar 15, 2023 •

edited

Loading

Uh oh!

whummer left a comment

Uh oh!

Uh oh!

whummer Mar 15, 2023 •

edited

Loading

Uh oh!

viren-nadkarni Mar 17, 2023

Uh oh!

github-actions bot commented Mar 15, 2023 •

edited

Loading

Uh oh!

paullallier commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

viren-nadkarni commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viren-nadkarni Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whummer Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viren-nadkarni Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack integration with Pro

Uh oh!

paullallier commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viren-nadkarni commented Mar 15, 2023 •

edited

Loading

coveralls commented Mar 15, 2023 •

edited

Loading

whummer Mar 15, 2023 •

edited

Loading

github-actions bot commented Mar 15, 2023 •

edited

Loading