From c32027dc8dc4c68d1ef7a1ce0285673100852b4a Mon Sep 17 00:00:00 2001 From: Alex Russell-Saw Date: Tue, 6 Apr 2021 12:31:30 +0100 Subject: [PATCH] rename encryption.Key.ID() to Version() & return structured version data (#19706) * add Version to mounted key config * rename encryption.Key.ID() to Version() & return structured version data * update external services & accounts to use Key.Version() * add TODO for json schema generation of KeyVersion.Type * fix string conversion lint error --- internal/database/external_accounts.go | 3 +- internal/database/external_services.go | 9 +-- internal/database/external_services_test.go | 4 +- internal/database/oob_migrate.go | 7 +- internal/database/oob_migrate_test.go | 12 ++-- internal/encryption/cloudkms/cloud_kms.go | 13 ++-- internal/encryption/key.go | 21 +++++- internal/encryption/mounted/key.go | 71 +++++++++++---------- internal/encryption/noop.go | 12 +++- schema/schema.go | 1 + schema/site.schema.json | 3 + 11 files changed, 98 insertions(+), 58 deletions(-) diff --git a/internal/database/external_accounts.go b/internal/database/external_accounts.go index 5c486d8d1d0..102bfa57771 100644 --- a/internal/database/external_accounts.go +++ b/internal/database/external_accounts.go @@ -475,10 +475,11 @@ func MaybeEncrypt(ctx context.Context, key encryption.Key, data string) (maybeEn return "", "", err } data = string(encrypted) - keyIdent, err = key.ID(ctx) + version, err := key.Version(ctx) if err != nil { return "", "", err } + keyIdent = version.JSON() } return data, keyIdent, nil diff --git a/internal/database/external_services.go b/internal/database/external_services.go index 952a26aac9a..0ee6bed7282 100644 --- a/internal/database/external_services.go +++ b/internal/database/external_services.go @@ -567,8 +567,8 @@ func (e *ExternalServiceStore) Create(ctx context.Context, confGet func() *conf. func (e *ExternalServiceStore) maybeEncryptConfig(ctx context.Context, config string) (string, string, error) { // encrypt the config before writing if we have a key configured var ( - keyID string - key = keyring.Default().ExternalServiceKey + keyVersion string + key = keyring.Default().ExternalServiceKey ) if e.key != nil { key = e.key @@ -579,12 +579,13 @@ func (e *ExternalServiceStore) maybeEncryptConfig(ctx context.Context, config st return "", "", err } config = string(encrypted) - keyID, err = key.ID(ctx) + version, err := key.Version(ctx) if err != nil { return "", "", err } + keyVersion = version.JSON() } - return config, keyID, nil + return config, keyVersion, nil } func (e *ExternalServiceStore) maybeDecryptConfig(ctx context.Context, config string, keyID string) (string, error) { diff --git a/internal/database/external_services_test.go b/internal/database/external_services_test.go index 16d5f111e12..a05dc23b344 100644 --- a/internal/database/external_services_test.go +++ b/internal/database/external_services_test.go @@ -1476,6 +1476,6 @@ func (k testKey) Decrypt(ctx context.Context, ciphertext []byte) (*encryption.Se return &s, err } -func (k testKey) ID(ctx context.Context) (string, error) { - return "testkey", nil +func (k testKey) Version(ctx context.Context) (encryption.KeyVersion, error) { + return encryption.KeyVersion{Type: "testkey"}, nil } diff --git a/internal/database/oob_migrate.go b/internal/database/oob_migrate.go index 942254a7db5..d29b01474f2 100644 --- a/internal/database/oob_migrate.go +++ b/internal/database/oob_migrate.go @@ -82,10 +82,11 @@ func (m *ExternalServiceConfigMigrator) Up(ctx context.Context) (err error) { return err } - keyIdent, err := key.ID(ctx) + version, err := key.Version(ctx) if err != nil { return err } + keyIdent := version.JSON() // ensure encryption round-trip is valid with keyIdent decrypted, err := key.Decrypt(ctx, encryptedCfg) @@ -231,11 +232,13 @@ func (m *ExternalAccountsMigrator) Up(ctx context.Context) (err error) { return nil } - keyIdent, err := key.ID(ctx) + version, err := key.Version(ctx) if err != nil { return err } + keyIdent := version.JSON() + tx, err := m.store.Transact(ctx) if err != nil { return err diff --git a/internal/database/oob_migrate_test.go b/internal/database/oob_migrate_test.go index 672a59a855a..f8703ed2436 100644 --- a/internal/database/oob_migrate_test.go +++ b/internal/database/oob_migrate_test.go @@ -168,8 +168,8 @@ func TestExternalServiceConfigMigrator(t *testing.T) { t.Fatalf("decrypted config is different from the original one") } - if id, _ := key.ID(ctx); keyID != id { - t.Fatalf("wrong encryption_key_id, want %s, got %s", id, keyID) + if version, _ := key.Version(ctx); keyID != version.JSON() { + t.Fatalf("wrong encryption_key_id, want %s, got %s", version.JSON(), keyID) } i++ @@ -285,8 +285,8 @@ func (k invalidKey) Decrypt(ctx context.Context, ciphertext []byte) (*encryption return &s, nil } -func (k invalidKey) ID(ctx context.Context) (string, error) { - return "invalidKey", nil +func (k invalidKey) Version(ctx context.Context) (encryption.KeyVersion, error) { + return encryption.KeyVersion{Type: "invalidkey"}, nil } func TestExternalAccountsMigrator(t *testing.T) { @@ -452,8 +452,8 @@ func TestExternalAccountsMigrator(t *testing.T) { t.Fatalf("decrypted data is different from the original one") } - if id, _ := key.ID(ctx); keyID != id { - t.Fatalf("wrong encryption_key_id, want %s, got %s", id, keyID) + if version, _ := key.Version(ctx); keyID != version.JSON() { + t.Fatalf("wrong encryption_key_id, want %s, got %s", version.JSON(), keyID) } i++ diff --git a/internal/encryption/cloudkms/cloud_kms.go b/internal/encryption/cloudkms/cloud_kms.go index f9369707c31..8d3c01c1465 100644 --- a/internal/encryption/cloudkms/cloud_kms.go +++ b/internal/encryption/cloudkms/cloud_kms.go @@ -4,7 +4,6 @@ import ( "context" "encoding/base64" "encoding/json" - "fmt" "hash/crc32" "strings" @@ -25,7 +24,7 @@ func NewKey(ctx context.Context, keyName string) (encryption.Key, error) { name: keyName, client: client, } - _, err = k.ID(ctx) + _, err = k.Version(ctx) return k, err } @@ -34,16 +33,20 @@ type Key struct { client *kms.KeyManagementClient } -func (k *Key) ID(ctx context.Context) (string, error) { +func (k *Key) Version(ctx context.Context) (encryption.KeyVersion, error) { key, err := k.client.GetCryptoKey(ctx, &kmspb.GetCryptoKeyRequest{ Name: k.name, }) if err != nil { - return "", errors.Wrap(err, "getting key ident") + return encryption.KeyVersion{}, errors.Wrap(err, "getting key version") } // return the primary key version name, as that will include which key // revision is currently in use - return fmt.Sprintf("cloudkms:%s", key.Primary.Name), nil + return encryption.KeyVersion{ + Type: "cloudkms", + Version: key.Primary.Name, + Name: key.Name, + }, nil } // Decrypt a secret, it must have been encrypted with the same Key diff --git a/internal/encryption/key.go b/internal/encryption/key.go index 4ac08e97af6..9a6b82cc62e 100644 --- a/internal/encryption/key.go +++ b/internal/encryption/key.go @@ -1,15 +1,30 @@ package encryption -import "context" +import ( + "context" + "encoding/json" +) // Key combines the Encrypter & Decrypter interfaces. type Key interface { Encrypter Decrypter - // ID returns an identifier string containing anything to concretely identify + // Version returns info containing to concretely identify // the underlying key, eg: key type, name, & version. - ID(ctx context.Context) (string, error) + Version(ctx context.Context) (KeyVersion, error) +} + +type KeyVersion struct { + // TODO: generate this as an enum from JSONSchema + Type string + Name string + Version string +} + +func (v KeyVersion) JSON() string { + buf, _ := json.Marshal(v) + return string(buf) } // Encrypter is anything that can encrypt a value diff --git a/internal/encryption/mounted/key.go b/internal/encryption/mounted/key.go index aec38beb903..773cdc028f2 100644 --- a/internal/encryption/mounted/key.go +++ b/internal/encryption/mounted/key.go @@ -13,21 +13,57 @@ import ( "os" "strings" + "github.com/sourcegraph/sourcegraph/schema" + "github.com/cockroachdb/errors" "github.com/sourcegraph/sourcegraph/internal/encryption" - "github.com/sourcegraph/sourcegraph/schema" ) +func NewKey(ctx context.Context, k schema.MountedEncryptionKey) (*Key, error) { + var secret []byte + if k.EnvVarName != "" && k.Filepath == "" { + secret = []byte(os.Getenv(k.EnvVarName)) + + } else if k.Filepath != "" && k.EnvVarName == "" { + keyBytes, err := os.ReadFile(k.Filepath) + if err != nil { + return nil, errors.Errorf("error reading secret file for %q: %v", k.Keyname, err) + } + secret = keyBytes + } else { + // Either the user has set none of EnvVarName or Filepath or both in their config. Either way we return an error. + return nil, errors.Errorf( + "must use only one of EnvVarName and Filepath, EnvVarName: %q, Filepath: %q", + k.EnvVarName, k.Filepath, + ) + } + + if len(secret) != 32 { + return nil, fmt.Errorf("invalid key length: %d, expected 32 bytes", len(secret)) + } + + return &Key{ + keyname: k.Keyname, + version: k.Version, + secret: secret, + }, nil +} + // Key is an encryption.Key implementation that uses AES GCM encryption, using a // secret loaded either from an env var or a file type Key struct { keyname string secret []byte + version string } -func (k *Key) ID(ctx context.Context) (string, error) { - return k.keyname, nil +func (k *Key) Version(ctx context.Context) (encryption.KeyVersion, error) { + return encryption.KeyVersion{ + Type: "mounted", + Name: k.keyname, + Version: k.version, + }, nil } func (k *Key) Encrypt(ctx context.Context, plaintext []byte) ([]byte, error) { @@ -103,35 +139,6 @@ func (k *Key) Decrypt(ctx context.Context, ciphertext []byte) (*encryption.Secre return &s, nil } -func NewKey(ctx context.Context, k schema.MountedEncryptionKey) (*Key, error) { - var secret []byte - if k.EnvVarName != "" && k.Filepath == "" { - secret = []byte(os.Getenv(k.EnvVarName)) - - } else if k.Filepath != "" && k.EnvVarName == "" { - keyBytes, err := os.ReadFile(k.Filepath) - if err != nil { - return nil, errors.Errorf("error reading secret file for %q: %v", k.Keyname, err) - } - secret = keyBytes - } else { - // Either the user has set none of EnvVarName or Filepath or both in their config. Either way we return an error. - return nil, errors.Errorf( - "must use only one of EnvVarName and Filepath, EnvVarName: %q, Filepath: %q", - k.EnvVarName, k.Filepath, - ) - } - - if len(secret) != 32 { - return nil, fmt.Errorf("invalid key length: %d, expected 32 bytes", len(secret)) - } - - return &Key{ - keyname: k.Keyname, - secret: secret, - }, nil -} - type encryptedValue struct { KeyName string Ciphertext []byte diff --git a/internal/encryption/noop.go b/internal/encryption/noop.go index e68d5d0cfe4..06d654b5094 100644 --- a/internal/encryption/noop.go +++ b/internal/encryption/noop.go @@ -1,13 +1,19 @@ package encryption -import "context" +import ( + "context" +) var _ Key = &NoopKey{} type NoopKey struct{} -func (k *NoopKey) ID(ctx context.Context) (string, error) { - return "no-op", nil +func (k *NoopKey) Version(ctx context.Context) (KeyVersion, error) { + return KeyVersion{ + Type: "noop", + Name: "noop", + Version: "", + }, nil } func (k *NoopKey) Encrypt(ctx context.Context, plaintext []byte) ([]byte, error) { diff --git a/schema/schema.go b/schema/schema.go index a3190d93acd..86d41823ba1 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -839,6 +839,7 @@ type MountedEncryptionKey struct { Filepath string `json:"filepath,omitempty"` Keyname string `json:"keyname"` Type string `json:"type"` + Version string `json:"version,omitempty"` } // NoOpEncryptionKey description: This encryption key is a no op, leaving your data in plaintext (not recommended). diff --git a/schema/site.schema.json b/schema/site.schema.json index 326006a1a03..e9131d57916 100644 --- a/schema/site.schema.json +++ b/schema/site.schema.json @@ -1405,6 +1405,9 @@ }, "envVarName": { "type": "string" + }, + "version": { + "type": "string" } } },