Skip to content
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

exp/services/recoverysigner: add encrypt and decrypt commands #2746

Merged
merged 11 commits into from
Jun 26, 2020
Merged

exp/services/recoverysigner: add encrypt and decrypt commands #2746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2e24e69
add decrypt cmd and make other messages consistent
howardtw Jun 25, 2020
010ef54
add encrypt cmd
howardtw Jun 25, 2020
d9adfae
output pubkeys
howardtw Jun 25, 2020
db21ae6
make error messages better
howardtw Jun 25, 2020
22f624f
fix shadow
howardtw Jun 25, 2020
598d26e
update README
howardtw Jun 25, 2020
adcf23d
update usage
howardtw Jun 25, 2020
a142e95
add a period to README
howardtw Jun 25, 2020
12f6dd2
add a JSON suffix
howardtw Jun 25, 2020
b694651
Merge branch 'recoverysigner-unique-signing-keys' into reencrypt
howardtw Jun 26, 2020
8bb682c
remove the word existing
howardtw Jun 26, 2020
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
29 changes: 27 additions & 2 deletions exp/services/recoverysigner/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ Usage:
recoverysigner [command]

Available Commands:
db Run database operations
serve Run the SEP-30 Recovery Signer server
db Run database operations
encryption-tink-keyset Run Tink keyset operations
serve Run the SEP-30 Recovery Signer server

Use "recoverysigner [command] --help" for more information about a command.
```
Expand Down Expand Up @@ -74,5 +75,29 @@ Flags:
Use "recoverysigner db [command] --help" for more information about a command.
```

## Usage: encryption-tink-keyset

The format of the --encryption-kms-key-uri configuration option should conform to the format stated in [Google Tink](https://github.com/google/tink/blob/040ac621b3e9ff7a240b1e596a423a30d32f9013/docs/KEY-MANAGEMENT.md#key-management-systems).
```
$ recoverysigner encryption-tink-keyset --help
Run Tink keyset operations

Usage:
recoverysigner encryption-tink-keyset [flags]
recoverysigner encryption-tink-keyset [command]

Available Commands:
create Create a new Tink keyset
decrypt Decrypt the Tink keyset specified in encryption-tink-keyset with the KMS key specified in encryption-kms-key-uri
encrypt Encrypt the Tink keyset specified in encryption-tink-keyset with the KMS key specified in encryption-kms-key-uri
rotate Rotate the Tink keyset specified in encryption-tink-keyset by generating a new key, adding it to the keyset, and making it the primary key in the keyset

Flags:
--encryption-kms-key-uri string URI for a remote KMS key used to encrypt Tink keyset (ENCRYPTION_KMS_KEY_URI)
--encryption-tink-keyset string Tink keyset to rotate/encrypt/decrypt (ENCRYPTION_TINK_KEYSET)

Use "recoverysigner encryption-tink-keyset [command] --help" for more information about a command.
```

[SEP-30]: https://github.com/stellar/stellar-protocol/blob/600c326b210d71ee031d7f3a40ca88191b4cdf9c/ecosystem/sep-0030.md
[README-Firebase.md]: README-Firebase.md
160 changes: 140 additions & 20 deletions exp/services/recoverysigner/cmd/keyset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import (
)

type KeysetCommand struct {
Logger *supportlog.Entry
EncryptionKMSKeyURI string
EncryptionTinkKeyset string
Logger *supportlog.Entry
EncryptionKMSKeyURI string
EncryptionTinkKeysetJSON string
}

func (c *KeysetCommand) Command() *cobra.Command {
Expand All @@ -34,9 +34,9 @@ func (c *KeysetCommand) Command() *cobra.Command {
},
{
Name: "encryption-tink-keyset",
Usage: "Existing Tink keyset to rotate",
Usage: "Tink keyset to rotate/encrypt/decrypt",
OptType: types.String,
ConfigKey: &c.EncryptionTinkKeyset,
ConfigKey: &c.EncryptionTinkKeysetJSON,
FlagDefault: "",
Required: false,
},
Expand Down Expand Up @@ -67,9 +67,25 @@ func (c *KeysetCommand) Command() *cobra.Command {
c.Rotate()
},
}
decryptCmd := &cobra.Command{
Use: "decrypt",
Short: "Decrypt the Tink keyset specified in encryption-tink-keyset with the KMS key specified in encryption-kms-key-uri",
Run: func(_ *cobra.Command, _ []string) {
c.Decrypt()
},
}
encryptCmd := &cobra.Command{
Use: "encrypt",
Short: "Encrypt the Tink keyset specified in encryption-tink-keyset with the KMS key specified in encryption-kms-key-uri",
howardtw marked this conversation as resolved.
Show resolved Hide resolved
Run: func(_ *cobra.Command, _ []string) {
c.Encrypt()
},
}

cmd.AddCommand(createCmd)
cmd.AddCommand(rotateCmd)
cmd.AddCommand(decryptCmd)
cmd.AddCommand(encryptCmd)

return cmd
}
Expand Down Expand Up @@ -116,30 +132,30 @@ func createKeyset(kmsKeyURI string, keyTemplate *tinkpb.KeyTemplate) (publicClea

err = khPriv.Write(keyset.NewJSONWriter(&keysetPrivateEncrypted), aead)
if err != nil {
return "", "", "", errors.Wrap(err, "writing encrypted keyset containing private key")
return "", "", "", errors.Wrap(err, "writing encrypted keyset private")
}
}

err = insecurecleartextkeyset.Write(khPriv, keyset.NewJSONWriter(&keysetPrivateCleartext))
if err != nil {
return "", "", "", errors.Wrap(err, "writing cleartext keyset containing private key")
return "", "", "", errors.Wrap(err, "writing cleartext keyset private")
}

khPub, err := khPriv.Public()
if err != nil {
return "", "", "", errors.Wrap(err, "getting keyhandle for public key")
return "", "", "", errors.Wrap(err, "getting key handle for keyset public")
}

err = khPub.WriteWithNoSecrets(keyset.NewJSONWriter(&keysetPublic))
if err != nil {
return "", "", "", errors.Wrap(err, "writing cleartext keyset containing public key")
return "", "", "", errors.Wrap(err, "writing cleartext keyset public")
}

return keysetPublic.String(), keysetPrivateCleartext.String(), keysetPrivateEncrypted.String(), nil
}

func (c *KeysetCommand) Rotate() {
keysetPublic, keysetPrivateCleartext, keysetPrivateEncrypted, err := rotateKeyset(c.EncryptionKMSKeyURI, c.EncryptionTinkKeyset, c.keyTemplate())
keysetPublic, keysetPrivateCleartext, keysetPrivateEncrypted, err := rotateKeyset(c.EncryptionKMSKeyURI, c.EncryptionTinkKeysetJSON, c.keyTemplate())
if err != nil {
c.Logger.Errorf("Error rotating keyset: %v", err)
return
Expand All @@ -153,7 +169,7 @@ func (c *KeysetCommand) Rotate() {
}
}

func rotateKeyset(kmsKeyURI, currentTinkKeyset string, keyTemplate *tinkpb.KeyTemplate) (publicCleartext string, privateCleartext string, privateEncrypted string, err error) {
func rotateKeyset(kmsKeyURI, keysetJSON string, keyTemplate *tinkpb.KeyTemplate) (publicCleartext string, privateCleartext string, privateEncrypted string, err error) {
var (
khPriv *keyset.Handle
aead tink.AEAD
Expand All @@ -170,14 +186,14 @@ func rotateKeyset(kmsKeyURI, currentTinkKeyset string, keyTemplate *tinkpb.KeyTe
return "", "", "", errors.Wrap(kmsErr, "getting AEAD primitive from KMS")
}

khPriv, err = keyset.Read(keyset.NewJSONReader(strings.NewReader(currentTinkKeyset)), aead)
khPriv, err = keyset.Read(keyset.NewJSONReader(strings.NewReader(keysetJSON)), aead)
if err != nil {
return "", "", "", errors.Wrap(err, "reading encrypted keyset")
return "", "", "", errors.Wrap(err, "getting key handle for keyset private by reading an encrypted keyset")
}
} else {
khPriv, err = insecurecleartextkeyset.Read(keyset.NewJSONReader(strings.NewReader(currentTinkKeyset)))
khPriv, err = insecurecleartextkeyset.Read(keyset.NewJSONReader(strings.NewReader(keysetJSON)))
if err != nil {
return "", "", "", errors.Wrap(err, "getting key handle for private key")
return "", "", "", errors.Wrap(err, "getting key handle for keyset private by reading a cleartext keyset")
}
}

Expand All @@ -189,7 +205,7 @@ func rotateKeyset(kmsKeyURI, currentTinkKeyset string, keyTemplate *tinkpb.KeyTe

khPriv, err = m.Handle()
if err != nil {
return "", "", "", errors.Wrap(err, "creating handle for the new keyset")
return "", "", "", errors.Wrap(err, "creating key handle for the rotated keyset private")
}

keysetPrivateEncrypted := strings.Builder{}
Expand All @@ -199,24 +215,128 @@ func rotateKeyset(kmsKeyURI, currentTinkKeyset string, keyTemplate *tinkpb.KeyTe
if kmsKeyURI != "" {
err = khPriv.Write(keyset.NewJSONWriter(&keysetPrivateEncrypted), aead)
if err != nil {
return "", "", "", errors.Wrap(err, "writing encrypted keyset containing private keys")
return "", "", "", errors.Wrap(err, "writing encrypted keyset private")
}
}

err = insecurecleartextkeyset.Write(khPriv, keyset.NewJSONWriter(&keysetPrivateCleartext))
if err != nil {
return "", "", "", errors.Wrap(err, "writing cleartext keyset containing private keys")
return "", "", "", errors.Wrap(err, "writing cleartext keyset private")
}

khPub, err := khPriv.Public()
if err != nil {
return "", "", "", errors.Wrap(err, "getting keyhandle for public keys")
return "", "", "", errors.Wrap(err, "getting key handle for keyset public")
}

err = khPub.WriteWithNoSecrets(keyset.NewJSONWriter(&keysetPublic))
if err != nil {
return "", "", "", errors.Wrap(err, "writing cleartext keyset containing public keys")
return "", "", "", errors.Wrap(err, "writing cleartext keyset public")
}

return keysetPublic.String(), keysetPrivateCleartext.String(), keysetPrivateEncrypted.String(), nil
}

var errNoKMSKeyURI = errors.New("KMS Key URI is not configured")

func (c *KeysetCommand) Decrypt() {
keysetPublic, keysetPrivateCleartext, err := decryptKeyset(c.EncryptionKMSKeyURI, c.EncryptionTinkKeysetJSON)
if err != nil {
c.Logger.Errorf("Error decrypting keyset: %v", err)
return
}

c.Logger.Print("Cleartext keyset public:", keysetPublic)
c.Logger.Print("Cleartext keyset private:", keysetPrivateCleartext)
}

func decryptKeyset(kmsKeyURI, keysetJSON string) (publicCleartext string, privateCleartext string, err error) {
if kmsKeyURI == "" {
return "", "", errNoKMSKeyURI
}

kmsClient, err := awskms.NewClient(kmsKeyURI)
if err != nil {
return "", "", errors.Wrap(err, "initializing AWS KMS client")
}

aead, err := kmsClient.GetAEAD(kmsKeyURI)
if err != nil {
return "", "", errors.Wrap(err, "getting AEAD primitive from KMS")
}

khPriv, err := keyset.Read(keyset.NewJSONReader(strings.NewReader(keysetJSON)), aead)
if err != nil {
return "", "", errors.Wrap(err, "getting key handle for keyset private by reading an encrypted keyset")
}

keysetPrivateCleartext := strings.Builder{}
err = insecurecleartextkeyset.Write(khPriv, keyset.NewJSONWriter(&keysetPrivateCleartext))
if err != nil {
return "", "", errors.Wrap(err, "writing cleartext keyset private")
}

khPub, err := khPriv.Public()
if err != nil {
return "", "", errors.Wrap(err, "getting key handle for keyset public")
}

keysetPublic := strings.Builder{}
err = khPub.WriteWithNoSecrets(keyset.NewJSONWriter(&keysetPublic))
if err != nil {
return "", "", errors.Wrap(err, "writing cleartext keyset public")
}

return keysetPublic.String(), keysetPrivateCleartext.String(), nil
}

func (c *KeysetCommand) Encrypt() {
keysetPublic, keysetPrivateEncrypted, err := encryptKeyset(c.EncryptionKMSKeyURI, c.EncryptionTinkKeysetJSON)
if err != nil {
c.Logger.Errorf("Error encrypting keyset: %v", err)
return
}

c.Logger.Print("Cleartext keyset public:", keysetPublic)
c.Logger.Print("Encrypted keyset private:", keysetPrivateEncrypted)
}

func encryptKeyset(kmsKeyURI, keysetJSON string) (publicCleartext string, privateEncrypted string, err error) {
if kmsKeyURI == "" {
return "", "", errNoKMSKeyURI
}

kmsClient, err := awskms.NewClient(kmsKeyURI)
if err != nil {
return "", "", errors.Wrap(err, "initializing AWS KMS client")
}

aead, err := kmsClient.GetAEAD(kmsKeyURI)
if err != nil {
return "", "", errors.Wrap(err, "getting AEAD primitive from KMS")
}

khPriv, err := insecurecleartextkeyset.Read(keyset.NewJSONReader(strings.NewReader(keysetJSON)))
if err != nil {
return "", "", errors.Wrap(err, "getting key handle for keyset private by reading a cleartext keyset")
}

keysetPrivateEncrypted := strings.Builder{}
err = khPriv.Write(keyset.NewJSONWriter(&keysetPrivateEncrypted), aead)
if err != nil {
return "", "", errors.Wrap(err, "writing encrypted keyset private")
}

khPub, err := khPriv.Public()
if err != nil {
return "", "", errors.Wrap(err, "getting key handle for keyset public")
}

keysetPublic := strings.Builder{}
err = khPub.WriteWithNoSecrets(keyset.NewJSONWriter(&keysetPublic))
if err != nil {
return "", "", errors.Wrap(err, "writing cleartext keyset public")
}

return keysetPublic.String(), keysetPrivateEncrypted.String(), nil
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2750

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

}
26 changes: 24 additions & 2 deletions exp/services/recoverysigner/cmd/keyset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,30 @@ func TestRotateKeyset_invalidKMSKeyURI(t *testing.T) {
assert.Contains(t, err.Error(), "initializing AWS KMS client")
}

func TestRotateKeyset_noCurrentKeyset(t *testing.T) {
func TestRotateKeyset_noEncryptionTinkKeyset(t *testing.T) {
_, _, _, err := rotateKeyset("", "", hybrid.ECIESHKDFAES128GCMKeyTemplate())
require.Error(t, err)
assert.Contains(t, err.Error(), "getting key handle for private key")
assert.Contains(t, err.Error(), "getting key handle for keyset private by reading a cleartext keyset")
}

func TestDecryptKeyset_invalidKMSKeyURI(t *testing.T) {
// encrption-kms-key-uri is not configured
_, _, err := decryptKeyset("", "keysetJSON")
require.Error(t, err)
assert.Equal(t, err, errNoKMSKeyURI)

_, _, err = decryptKeyset("invalid-uri", "keysetJSON")
require.Error(t, err)
assert.Contains(t, err.Error(), "initializing AWS KMS client")
}

func TestEncryptKeyset_invalidKMSKeyURI(t *testing.T) {
// encrption-kms-key-uri is not configured
_, _, err := encryptKeyset("", "keysetJSON")
require.Error(t, err)
assert.Equal(t, err, errNoKMSKeyURI)

_, _, err = encryptKeyset("invalid-uri", "keysetJSON")
require.Error(t, err)
assert.Contains(t, err.Error(), "initializing AWS KMS client")
}
2 changes: 1 addition & 1 deletion exp/services/recoverysigner/cmd/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (c *ServeCommand) Command() *cobra.Command {
},
{
Name: "encryption-kms-key-uri",
Usage: "URI for a remote KMS key used to decrypt the Tink keyset provided in encryption-tink-keyset",
Usage: "URI for a remote KMS key used to decrypt the Tink keyset specified in encryption-tink-keyset",
OptType: types.String,
ConfigKey: &opts.EncryptionKMSKeyURI,
FlagDefault: "",
Expand Down