From 34d7cff4b34a2e1c58beb5b96436639f8fd5bfd6 Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Mon, 4 Nov 2024 15:41:00 +1100 Subject: [PATCH] refactor: remove obfuscator I think in practice this makes life more difficult, as it becomes much more painful to debug secrets/config that are obfuscated. --- internal/configuration/api.go | 4 - internal/configuration/manager/manager.go | 31 ++----- internal/configuration/obfuscator.go | 80 ------------------- internal/configuration/obfuscator_test.go | 72 ----------------- .../configuration/providers/asm_follower.go | 18 +---- 5 files changed, 9 insertions(+), 196 deletions(-) delete mode 100644 internal/configuration/obfuscator.go delete mode 100644 internal/configuration/obfuscator_test.go diff --git a/internal/configuration/api.go b/internal/configuration/api.go index 71a5d3ca15..121f2482c2 100644 --- a/internal/configuration/api.go +++ b/internal/configuration/api.go @@ -80,10 +80,6 @@ type Secrets struct{} func (Secrets) String() string { return "secrets" } -func (Secrets) Obfuscator() Obfuscator { - return NewObfuscator([]byte("obfuscatesecrets")) // 16 characters (AES-128), not meant to provide security -} - type Configuration struct{} func (Configuration) String() string { return "configuration" } diff --git a/internal/configuration/manager/manager.go b/internal/configuration/manager/manager.go index 399b9910d4..e44db89512 100644 --- a/internal/configuration/manager/manager.go +++ b/internal/configuration/manager/manager.go @@ -21,10 +21,9 @@ import ( // Manager is a high-level configuration manager that abstracts the details of // the Router and Provider interfaces. type Manager[R configuration.Role] struct { - provider configuration.Provider[R] - router configuration.Router[R] - obfuscator optional.Option[configuration.Obfuscator] - cache *cache[R] + provider configuration.Provider[R] + router configuration.Router[R] + cache *cache[R] } func ConfigFromEnvironment() []string { @@ -59,9 +58,6 @@ func NewDefaultConfigurationManagerFromConfig(ctx context.Context, registry *pro // New configuration manager. func New[R configuration.Role](ctx context.Context, router configuration.Router[R], provider configuration.Provider[R]) (*Manager[R], error) { m := &Manager[R]{provider: provider} - if provider, ok := any(new(R)).(configuration.ObfuscatorProvider); ok { - m.obfuscator = optional.Some(provider.Obfuscator()) - } m.router = router var asyncProvider optional.Option[configuration.AsynchronousProvider[R]] @@ -110,12 +106,6 @@ func (m *Manager[R]) getData(ctx context.Context, ref configuration.Ref) ([]byte return nil, fmt.Errorf("provider for %s does not support on demand access or syncing", ref) } } - if obfuscator, ok := m.obfuscator.Get(); ok { - data, err = obfuscator.Reveal(data) - if err != nil { - return nil, fmt.Errorf("could not reveal obfuscated value: %w", err) - } - } return data, nil } @@ -153,22 +143,11 @@ func (m *Manager[R]) SetJSON(ctx context.Context, ref configuration.Ref, value j if err := checkJSON(value); err != nil { return fmt.Errorf("invalid value for %s, must be JSON: %w", m.router.Role(), err) } - var bytes []byte - if obfuscator, ok := m.obfuscator.Get(); ok { - var err error - bytes, err = obfuscator.Obfuscate(value) - if err != nil { - return fmt.Errorf("could not obfuscate: %w", err) - } - } else { - bytes = value - } - - key, err := m.provider.Store(ctx, ref, bytes) + key, err := m.provider.Store(ctx, ref, value) if err != nil { return err } - m.cache.updatedValue(ref, bytes, key) + m.cache.updatedValue(ref, value, key) return m.router.Set(ctx, ref, key) } diff --git a/internal/configuration/obfuscator.go b/internal/configuration/obfuscator.go deleted file mode 100644 index 1f085610be..0000000000 --- a/internal/configuration/obfuscator.go +++ /dev/null @@ -1,80 +0,0 @@ -package configuration - -import ( - "crypto/aes" - "crypto/cipher" - "crypto/rand" - "encoding/base64" - "errors" - "fmt" - "io" - "strings" -) - -type ObfuscatorProvider interface { - Obfuscator() Obfuscator -} - -// Obfuscator hides and reveals a value, but does not provide real security. -// -// # Instead, the aim of this Obfuscator is to make values not easily human-readable -// -// Obfuscation is done by XOR-ing the input with the AES key. Length of key must be 16, 24 or 32 bytes (corresponding to AES-128, AES-192 or AES-256 keys). -type Obfuscator struct { - key []byte -} - -func NewObfuscator(key []byte) Obfuscator { - return Obfuscator{key: key} -} - -// Obfuscate takes a value and returns an obfuscated value (encoded in base64) -func (o Obfuscator) Obfuscate(input []byte) ([]byte, error) { - block, err := aes.NewCipher(o.key) - if err != nil { - return nil, fmt.Errorf("could not create cypher for obfuscation: %w", err) - } - if len(input) > 64*1024*1024 { - return nil, errors.New("value too large") - } - ciphertext := make([]byte, aes.BlockSize+len(input)) - iv := ciphertext[:aes.BlockSize] - if _, err := io.ReadFull(rand.Reader, iv); err != nil { - return nil, fmt.Errorf("could not generate IV for obfuscation: %w", err) - } - cfb := cipher.NewCFBEncrypter(block, iv) - cfb.XORKeyStream(ciphertext[aes.BlockSize:], input) - return []byte(base64.StdEncoding.EncodeToString(ciphertext)), nil -} - -// Reveal takes an obfuscated value and de-obfuscates the base64 encoded value -func (o Obfuscator) Reveal(input []byte) ([]byte, error) { - if len(input) == 0 { - return nil, nil - } - // check if the input looks like it was obfuscated - if !strings.ContainsRune("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ+/=", rune(input[0])) { - // known issue: an unobfuscated value which is just a number will be considered obfuscated - return input, nil - } - - obfuscated, err := base64.StdEncoding.DecodeString(string(input)) - if err != nil { - return nil, fmt.Errorf("expected base64 string: %w", err) - } - block, err := aes.NewCipher(o.key) - if err != nil { - return nil, fmt.Errorf("could not create cypher for decoding obfuscation: %w", err) - } - if len(obfuscated) < aes.BlockSize { - return nil, errors.New("obfuscated value too short to decode") - } - iv := obfuscated[:aes.BlockSize] - obfuscated = obfuscated[aes.BlockSize:] - cfb := cipher.NewCFBDecrypter(block, iv) - - var output = make([]byte, len(obfuscated)) - cfb.XORKeyStream(output, obfuscated) - - return output, nil -} diff --git a/internal/configuration/obfuscator_test.go b/internal/configuration/obfuscator_test.go deleted file mode 100644 index 7d33584a02..0000000000 --- a/internal/configuration/obfuscator_test.go +++ /dev/null @@ -1,72 +0,0 @@ -package configuration - -import ( - "testing" - - "github.com/alecthomas/assert/v2" - "github.com/alecthomas/types/optional" -) - -func TestObfuscator(t *testing.T) { - defaultKey := []byte("1234567890123456") // 32 characters - for _, tt := range []struct { - input string - key []byte - expectedError optional.Option[string] - backwardsCompatible bool - }{ - { - input: "test input can be anything", - key: defaultKey, - backwardsCompatible: false, - }, - { - input: `"test input can be anything"`, - key: defaultKey, - backwardsCompatible: true, - }, - { - input: `"{\n "key": "value"\n}`, - key: defaultKey, - backwardsCompatible: true, - }, - { - input: `1.2345`, - key: defaultKey, - backwardsCompatible: false, - }, - { - input: "key is too short", - key: []byte("too-short"), - expectedError: optional.Some("could not create cypher for obfuscation: crypto/aes: invalid key size 9"), - }, - } { - t.Run(tt.input, func(t *testing.T) { - o := Obfuscator{ - key: tt.key, - } - // obfuscate - obfuscated, err := o.Obfuscate([]byte(tt.input)) - if expectedError, ok := tt.expectedError.Get(); ok { - assert.EqualError(t, err, expectedError) - return - } - assert.NoError(t, err) - - // reveal obfuscated value - revealed, err := o.Reveal(obfuscated) - assert.NoError(t, err) - assert.Equal(t, tt.input, string(revealed)) - - // obfuscated value should not include the input we are trying to obfuscate - assert.NotContains(t, string(obfuscated), tt.input) - - // reveal unobfuscated value to check backwards compatibility - if tt.backwardsCompatible { - revealed, err = o.Reveal([]byte(tt.input)) - assert.NoError(t, err) - assert.Equal(t, tt.input, string(revealed)) - } - }) - } -} diff --git a/internal/configuration/providers/asm_follower.go b/internal/configuration/providers/asm_follower.go index bdee3e7019..a9119e9134 100644 --- a/internal/configuration/providers/asm_follower.go +++ b/internal/configuration/providers/asm_follower.go @@ -47,7 +47,6 @@ func (f *asmFollower) syncInterval() time.Duration { func (f *asmFollower) sync(ctx context.Context, values *xsync.MapOf[configuration.Ref, configuration.SyncedValue]) error { // values must store obfuscated values, but f.client gives unobfuscated values logger := log.FromContext(ctx) - obfuscator := configuration.Secrets{}.Obfuscator() module := "" includeValues := true resp, err := f.client.SecretsList(ctx, connect.NewRequest(&ftlv1.ListSecretsRequest{ @@ -72,13 +71,9 @@ func (f *asmFollower) sync(ctx context.Context, values *xsync.MapOf[configuratio if err != nil { return fmt.Errorf("invalid ref %q: %w", s.RefPath, err) } - obfuscatedValue, err := obfuscator.Obfuscate(s.Value) - if err != nil { - return fmt.Errorf("asm follower could not obfuscate value for ref %q: %w", s.RefPath, err) - } visited[ref] = true values.Store(ref, configuration.SyncedValue{ - Value: obfuscatedValue, + Value: s.Value, }) } // delete old values @@ -91,18 +86,13 @@ func (f *asmFollower) sync(ctx context.Context, values *xsync.MapOf[configuratio return nil } -func (f *asmFollower) store(ctx context.Context, ref configuration.Ref, obfuscatedValue []byte) (*url.URL, error) { - obfuscator := configuration.Secrets{}.Obfuscator() - unobfuscatedValue, err := obfuscator.Reveal(obfuscatedValue) - if err != nil { - return nil, fmt.Errorf("asm follower could not unobfuscate: %w", err) - } - _, err = f.client.SecretSet(ctx, connect.NewRequest(&ftlv1.SetSecretRequest{ +func (f *asmFollower) store(ctx context.Context, ref configuration.Ref, value []byte) (*url.URL, error) { + _, err := f.client.SecretSet(ctx, connect.NewRequest(&ftlv1.SetSecretRequest{ Ref: &ftlv1.ConfigRef{ Module: ref.Module.Ptr(), Name: ref.Name, }, - Value: unobfuscatedValue, + Value: value, })) if err != nil { return nil, err