Make create-key optionally deterministic

[mdaniel]

What?

Allow using Tag keys on kms:CreateKey requests so that the backing Key can optionally be deterministic in both its master key material as well as optionally have a known KeyId

This is a port of localstack/localstack#10379

Why?

When testing KMS it can often be helpful to compare expected results, or to be able to use a fixed KeyId in setup scripts. Since there was only one function call to generate the key material, being able to supply it out of band is convenient

[bblommers]

Hi @mdaniel! Thanks for the PR.

I'm not necessarily a fan of this feature as-is, to be honest, as it could lead to an explosion of custom logic to make names or IDs for every service deterministic.

Have you looked into seeding Moto instead? That should result in the same outcome, i.e. a static ID:
https://docs.getmoto.org/en/latest/docs/configuration/recorder/index.html#deterministic-identifiers

[mdaniel]

Hey @bblommers , thanks for taking a look!

I admit currently there are two conflated concerns in this change:

1. deterministic key material
2. deterministic identifiers

What I'm hearing is that the preferred way is to instead have generate_data_key take moto's seed behavior into account, versus always using os.urandom (i.e. push the deterministic behavior down away from models.py so all kms activities benefit from deterministic behavior)

I tried it with

diff --git a/moto/kms/models.py b/moto/kms/models.py
index d2853d585..465a46198 100644
--- a/moto/kms/models.py
+++ b/moto/kms/models.py
@@ -1,5 +1,4 @@
 import json
-import os
 from collections.abc import Iterable
 from copy import copy
 from datetime import datetime, timedelta
@@ -650,7 +649,7 @@ class KmsBackend(BaseBackend):
         else:
             plaintext_len = number_of_bytes
 
-        plaintext = os.urandom(plaintext_len)
+        plaintext = mock_random.randbytes(plaintext_len)
 
         ciphertext_blob, arn = self.encrypt(
             key_id=key_id, plaintext=plaintext, encryption_context=encryption_context
diff --git a/moto/kms/responses.py b/moto/kms/responses.py
index 8ec091b29..e7e712a0c 100644
--- a/moto/kms/responses.py
+++ b/moto/kms/responses.py
@@ -1,11 +1,11 @@
 import base64
 import json
-import os
 import re
 from typing import Any
 
 from moto.core.responses import BaseResponse
 from moto.kms.utils import RESERVED_ALIASE_TARGET_KEY_IDS, RESERVED_ALIASES
+from moto.moto_api._internal import mock_random
 from moto.utilities.utils import get_partition
 
 from .exceptions import (
@@ -632,7 +632,7 @@ class KmsResponse(BaseResponse):
                 "equal to 1024"
             )
 
-        entropy = os.urandom(number_of_bytes)
+        entropy = mock_random.randbytes(number_of_bytes)
 
         response_entropy = base64.b64encode(entropy).decode("utf-8")
 

and it did work better:

1. POST /moto-api/seed?a=42
2. awslocal kms create-key (deterministic ID, as expected)
3. ct=$(awslocal kms encrypt --key-id bdd640fb-0667-4ad1-9c80-317fa3b1799d --plaintext $(echo "hello world" | base64 -w 0) --output text --query CiphertextBlob)
4. POST /moto-api/reset
5. awslocal kms create-key (to re-register the kms key)
6. awslocal kms decrypt --key-id bdd640fb-0667-4ad1-9c80-317fa3b1799d --ciphertext "$ct"

that cycle did what I expected, although I am going to have to file a followup bug because kms decrypt --key-id $(python3 -m uuid -u uuid4) ... still returns 200 due to the way moto carries the original key-id in the ciphertext

---

That last item kind of relates to why just having the seed behavior won't help for the way that we currently use localstack to exercise our code, since in the real AWS account the key-id is essentially fixed, and our ability to load a known key-id with known key material into moto means there is one less deviation between real and local

Let me take this new behavior back to the team and see if it's a compromise they're willing to make to get off of localstack

In the interim, I'll open a separate PR for s/os.urandom/moto_random.randbytes/ to more closely align with the way you were thinking about things, and also open a ticket to talk over whether my experience of kms decrypt returning 200 ok for missing key-id is something you'd be open to fixing

[mdaniel]

I still owe you the PR about moto_random but wanted to report back that the team is using the Tag mechanism to send in the literal key material since for the use case they're testing, there are downstream systems that are going to check signatures and derived outputs based on that specific key material. Thus, it would be very inconvenient to have to regenerate the whole testing suite just because localstack had a rug-pull event

Are there alternatives that you would accept for allowing one to actually set the key material, aside from using a seed to PRNG-it into place?