exp/services/recoverysigner: add encrypt and decrypt commands #2746
Conversation
| return "", "", errors.Wrap(err, "writing cleartext keyset public") | ||
| } | ||
|
|
||
| return keysetPublic.String(), keysetPrivateEncrypted.String(), nil |
There was a problem hiding this comment.
I don't see any tests for this, is that because of the use of the aws integration here? I think we need to figure out tests for these commands, this code is pretty important. Let's think about this separate to this PR though.
There was a problem hiding this comment.
Exactly. We will have to think about the best way to mock this out. I will create an issue for this.
Speaking of this, I'm curious what you think about having a mock struct in non-test files. i.e. when it is ok to have one and when is not. I remember you mentioned you don't want to have a mock being part of the binary of production code, but we are doing that in the vibrant backend.
There was a problem hiding this comment.
I only had an issue with the type being callable via the CLI, i.e. being able to use the mock in real production code, not having the type live there. If we do have types or functions intended only for testing but that need to be importable, let's keep them in their own package. If it's for the crypto package, put them in cryptotest. This pattern is pretty common I think.
There was a problem hiding this comment.
I only had an issue with the type being callable via the CLI, i.e. being able to use the mock in real production code, not having the type live there.
I see. It does look like it is possible to the use the mock in real production with KMS_PROVIDER being set to mock in the vibrant backend. Maybe we need to address that.
If we do have types or functions intended only for testing but that need to be importable, let's keep them in their own package. If it's for the crypto package, put them in cryptotest. This pattern is pretty common I think.
Yeah I usually realize that I have to do this if I have the import cycle issue.
There was a problem hiding this comment.
Maybe we need to address that.
Maybe we don't. I think there might be some values to start the service with a KMS mock client in vibrant backend. Looks like this is done on purpose so that people can start the service locally and test the endpoints that use it.
I think the difference lies in the fact that the recovery signer can operate without configuring a KMS client because it is only used to encrypt the tink keyset whereas the vibrant backend cannot operate without configuring a KMS client because it is used to encrypt the data directly.
What do you think? Do you agree?
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
This PR adds two sub-commands
encryptanddecrypttoencryption-tink-keysetto encrypt a cleartext keyset private or to decrypt an encrypted keyset private.Why
I was planning on adding a sub-command
reencrypt. However, I realized there's no way for someone who started the recovery signer with a cleartext keyset private without configuringencryption-kms-key-urito encrypt the keyset later on, so I decided to add two sub-commands:encryptanddecrypt.For someone who wants to make the system more secure by configuring the
encryption-kms-key-uriat a later point in time, she can use theencryptcommand to do so; for someone who wants to re-encrypt the keyset after rotating the KMS key, she can first usedecryptto get the cleartext keyset if no backup is available, then useencryptto get a new encrypted keyset.Known limitations
I could theoretically break this PR into two, one for the
encryptcmd and one for thedecryptcmd. But since they are closely related and are not too big in their own, I decided to put them together and tidy up the messages throughout the file.