-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nice quality-of-life change would be to take the KMS key ID as a command-line argument. Typer/Click already support setting optional CLI arguments via env vars. Then we could raise an exception if it's not provided as we currently do.
That said, what we have here is good enough until we switch away from kms-ext
. Users won't be doing any decryption themselves with it after all, unless it is used for things other than Meltano Cloud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this change, actually. Is there a good reason to not put the key into the secrets file? (For context, we're having the same discussion over on the encrypted config spec.)
I want to make sure we have clear erroring when keys are rotated - and also that we leave the door open to the same container potentially having 2 or more available keys at one time. Can we instead do both - put the key ID in the file so it is explicit rather than implicit - and throw an error (or at least a warning) if the key that is mentioned in the file is not the same Key ID as the one passed in the env var?
My initial thoughts around this was to simplify the encryption process (not requiring a key id). But with expectations of key rotation or multiple kms keys as an option in the future, probably worth keeping in that case. I'll refactor based on this. |
Oh, got it! Yeah, that makes sense. 👍
|
@aaronsteers If we want to store it in the yaml, we need it to be provided during the Providing it via env var is useful for the But don't we plan to move away from this approach towards secrets relatively soon anyway? I don't think we'll ever get around to supporting key rotation with |
Yes! Thank you. I was switching context from the other issue, where there is a known explicit key at encrypt time and any number of potential decrypt keys at decrypt time. This is also true here except we're starting from the public key and not from a Key ID. Okay, I'm caught up now. Presumably the closest 'ID' we have at encrypt time is a public key fingerprint, yes? Because we don't want the user to have to concern themselves with the Key ID and also the public key while encrypting - which makes sense. I will reread with the improved context and reply again shortly. Thanks, both. |
Logged for follow-up: It's probably not a ton of work to add a fingerprint (MD5 or SHA-based hash of the key text), but it isn't strictly needed as of now.
That's an important question, and it does play into my thinking here. I don't expect this will fully go away with meltano/meltano#6999. Rather than slow down this PR, I've put some thoughts into a new discussion here in this repo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for the discussion here. I'm happy for this to merge as it is. 👍
The KMS key ID is now passed to the container via an env var
KMS_KEY_ID
automatically so we don't need to providekms_key_id
when encrypting anymore.Decrypting should retrieve this value from the environment.
Closes: #3