From 52816a0911918bb3f52cf3b07ebc6b702275e3c2 Mon Sep 17 00:00:00 2001 From: Maxime mouial Date: Fri, 24 Nov 2023 15:04:30 +0100 Subject: [PATCH] Adding a callback based method to the secrets component The secrets component can now notify the caller when resolving a secret. This allows the config package to only overwrite the setting using secrets instead of the entire configuration. --- comp/core/secrets/component.go | 7 +- comp/core/secrets/secretsimpl/fetch_secret.go | 6 +- .../secrets/secretsimpl/fetch_secret_test.go | 8 +- comp/core/secrets/secretsimpl/info.tmpl | 4 +- .../core/secrets/secretsimpl/info_nix_test.go | 16 +- comp/core/secrets/secretsimpl/secrets.go | 140 +++++--------- comp/core/secrets/secretsimpl/secrets_mock.go | 28 ++- comp/core/secrets/secretsimpl/secrets_test.go | 171 ++++++++++-------- comp/core/secrets/secretsimpl/walker.go | 114 ++++++++++++ comp/core/secrets/secretsimpl/walker_test.go | 134 ++++++++++++++ comp/core/secrets/type.go | 3 + pkg/autodiscovery/secrets.go | 8 +- pkg/autodiscovery/secrets_test.go | 26 +-- pkg/config/config.go | 14 +- pkg/config/config_secret_test.go | 21 +-- 15 files changed, 468 insertions(+), 232 deletions(-) create mode 100644 comp/core/secrets/secretsimpl/walker.go create mode 100644 comp/core/secrets/secretsimpl/walker_test.go diff --git a/comp/core/secrets/component.go b/comp/core/secrets/component.go index f0c1dc81e3985..8e021e3659bb0 100644 --- a/comp/core/secrets/component.go +++ b/comp/core/secrets/component.go @@ -18,6 +18,9 @@ type Component interface { Configure(command string, arguments []string, timeout, maxSize int, groupExecPerm, removeLinebreak bool) // Get debug information and write it to the parameter GetDebugInfo(w io.Writer) - // Decrypt the given handle and return the corresponding secret value - Decrypt(data []byte, origin string) ([]byte, error) + // Resolve resolves the secrets in the given yaml data by replacing secrets handles by their corresponding secret value + Resolve(data []byte, origin string) ([]byte, error) + // ResolveWithCallback resolves the secrets in the given yaml data calling the callback with the YAML path of + // the secret handle and its value + ResolveWithCallback(data []byte, origin string, callback ResolveCallback) error } diff --git a/comp/core/secrets/secretsimpl/fetch_secret.go b/comp/core/secrets/secretsimpl/fetch_secret.go index c3841608be909..e140ad29534d9 100644 --- a/comp/core/secrets/secretsimpl/fetch_secret.go +++ b/comp/core/secrets/secretsimpl/fetch_secret.go @@ -133,11 +133,11 @@ func (r *secretResolver) fetchSecret(secretsHandle []string) (map[string]string, for _, sec := range secretsHandle { v, ok := secrets[sec] if !ok { - return nil, fmt.Errorf("secret handle '%s' was not decrypted by the secret_backend_command", sec) + return nil, fmt.Errorf("secret handle '%s' was not resolved by the secret_backend_command", sec) } if v.ErrorMsg != "" { - return nil, fmt.Errorf("an error occurred while decrypting '%s': %s", sec, v.ErrorMsg) + return nil, fmt.Errorf("an error occurred while resolving '%s': %s", sec, v.ErrorMsg) } if r.removeTrailingLinebreak { @@ -145,7 +145,7 @@ func (r *secretResolver) fetchSecret(secretsHandle []string) (map[string]string, } if v.Value == "" { - return nil, fmt.Errorf("decrypted secret for '%s' is empty", sec) + return nil, fmt.Errorf("resolved secret for '%s' is empty", sec) } // add it to the cache diff --git a/comp/core/secrets/secretsimpl/fetch_secret_test.go b/comp/core/secrets/secretsimpl/fetch_secret_test.go index bbd23a699b495..0212fd64d5af0 100644 --- a/comp/core/secrets/secretsimpl/fetch_secret_test.go +++ b/comp/core/secrets/secretsimpl/fetch_secret_test.go @@ -201,7 +201,7 @@ func TestFetchSecretMissingSecret(t *testing.T) { resolver.commandHookFunc = func(string) ([]byte, error) { return []byte("{}"), nil } _, err := resolver.fetchSecret(secrets) assert.NotNil(t, err) - assert.Equal(t, "secret handle 'handle1' was not decrypted by the secret_backend_command", err.Error()) + assert.Equal(t, "secret handle 'handle1' was not resolved by the secret_backend_command", err.Error()) } func TestFetchSecretErrorForHandle(t *testing.T) { @@ -211,7 +211,7 @@ func TestFetchSecretErrorForHandle(t *testing.T) { } _, err := resolver.fetchSecret([]string{"handle1"}) assert.NotNil(t, err) - assert.Equal(t, "an error occurred while decrypting 'handle1': some error", err.Error()) + assert.Equal(t, "an error occurred while resolving 'handle1': some error", err.Error()) } func TestFetchSecretEmptyValue(t *testing.T) { @@ -221,14 +221,14 @@ func TestFetchSecretEmptyValue(t *testing.T) { } _, err := resolver.fetchSecret([]string{"handle1"}) assert.NotNil(t, err) - assert.Equal(t, "decrypted secret for 'handle1' is empty", err.Error()) + assert.Equal(t, "resolved secret for 'handle1' is empty", err.Error()) resolver.commandHookFunc = func(string) ([]byte, error) { return []byte("{\"handle1\":{\"value\": \"\"}}"), nil } _, err = resolver.fetchSecret([]string{"handle1"}) assert.NotNil(t, err) - assert.Equal(t, "decrypted secret for 'handle1' is empty", err.Error()) + assert.Equal(t, "resolved secret for 'handle1' is empty", err.Error()) } func TestFetchSecret(t *testing.T) { diff --git a/comp/core/secrets/secretsimpl/info.tmpl b/comp/core/secrets/secretsimpl/info.tmpl index 2c56659e88806..53f30f3ce3851 100644 --- a/comp/core/secrets/secretsimpl/info.tmpl +++ b/comp/core/secrets/secretsimpl/info.tmpl @@ -10,8 +10,8 @@ Permissions Detail: {{- end }} === Secrets stats === -Number of secrets decrypted: {{ len .Handles }} -Secrets handle decrypted: +Number of secrets resolved: {{ len .Handles }} +Secrets handle resolved: {{ range $handle, $places := .Handles }} - '{{ $handle }}': {{- range $place := $places }} diff --git a/comp/core/secrets/secretsimpl/info_nix_test.go b/comp/core/secrets/secretsimpl/info_nix_test.go index 7deb57ef2be47..1b170077d15a7 100644 --- a/comp/core/secrets/secretsimpl/info_nix_test.go +++ b/comp/core/secrets/secretsimpl/info_nix_test.go @@ -77,9 +77,9 @@ func TestDebugInfo(t *testing.T) { return res, nil } - _, err := resolver.Decrypt(testConf, "test") + _, err := resolver.Resolve(testConf, "test") require.NoError(t, err) - _, err = resolver.Decrypt(testConfInfo, "test2") + _, err = resolver.Resolve(testConfInfo, "test2") require.NoError(t, err) var buffer bytes.Buffer @@ -95,8 +95,8 @@ Owner: ` + currentUser + ` Group: ` + currentGroup + ` === Secrets stats === -Number of secrets decrypted: 3 -Secrets handle decrypted: +Number of secrets resolved: 3 +Secrets handle resolved: - 'pass1': used in 'test' configuration in entry 'instances/password' @@ -121,9 +121,9 @@ func TestDebugInfoError(t *testing.T) { return res, nil } - _, err := resolver.Decrypt(testConf, "test") + _, err := resolver.Resolve(testConf, "test") require.NoError(t, err) - _, err = resolver.Decrypt(testConfInfo, "test2") + _, err = resolver.Resolve(testConfInfo, "test2") require.NoError(t, err) var buffer bytes.Buffer @@ -137,8 +137,8 @@ Permissions Detail: Could not stat some_command: no such file or directory === Secrets stats === -Number of secrets decrypted: 3 -Secrets handle decrypted: +Number of secrets resolved: 3 +Secrets handle resolved: - 'pass1': used in 'test' configuration in entry 'instances/password' diff --git a/comp/core/secrets/secretsimpl/secrets.go b/comp/core/secrets/secretsimpl/secrets.go index 787e050b7ec14..2f436c380186f 100644 --- a/comp/core/secrets/secretsimpl/secrets.go +++ b/comp/core/secrets/secretsimpl/secrets.go @@ -186,75 +186,6 @@ func (r *secretResolver) Configure(command string, arguments []string, timeout, } } -type walkerCallback func([]string, string) (string, error) - -func walkSlice(data []interface{}, yamlPath []string, callback walkerCallback) error { - for idx, k := range data { - switch v := k.(type) { - case string: - newValue, err := callback(yamlPath, v) - if err != nil { - return err - } - data[idx] = newValue - case map[interface{}]interface{}: - if err := walkHash(v, yamlPath, callback); err != nil { - return err - } - case []interface{}: - if err := walkSlice(v, yamlPath, callback); err != nil { - return err - } - } - } - return nil -} - -func walkHash(data map[interface{}]interface{}, yamlPath []string, callback walkerCallback) error { - for k := range data { - path := yamlPath - if newkey, ok := k.(string); ok { - path = append(path, newkey) - } - - switch v := data[k].(type) { - case string: - newValue, err := callback(path, v) - if err != nil { - return err - } - data[k] = newValue - case map[interface{}]interface{}: - if err := walkHash(v, path, callback); err != nil { - return err - } - case []interface{}: - if err := walkSlice(v, path, callback); err != nil { - return err - } - } - } - return nil -} - -// walk will go through loaded yaml and call callback on every strings allowing -// the callback to overwrite the string value -func walk(data *interface{}, yamlPath []string, callback walkerCallback) error { - switch v := (*data).(type) { - case string: - newValue, err := callback(yamlPath, v) - if err != nil { - return err - } - *data = newValue - case map[interface{}]interface{}: - return walkHash(v, yamlPath, callback) - case []interface{}: - return walkSlice(v, yamlPath, callback) - } - return nil -} - func isEnc(str string) (bool, string) { // trimming space and tabs str = strings.Trim(str, " ") @@ -264,9 +195,20 @@ func isEnc(str string) (bool, string) { return false, "" } -// Decrypt replaces all encrypted secrets in data by executing -// "secret_backend_command" once if all secrets aren't present in the cache. -func (r *secretResolver) Decrypt(data []byte, origin string) ([]byte, error) { +// Resolve replaces all encrypted secrets in data by executing "secret_backend_command" once if all secrets aren't +// present in the cache. +func (r *secretResolver) Resolve(data []byte, origin string) ([]byte, error) { + return r.resolve(data, origin, nil) +} + +// ResolveWithCallback resolves the secrets in the given yaml data calling the callback with the YAML path of +// the secret handle and its value +func (r *secretResolver) ResolveWithCallback(data []byte, origin string, cb secrets.ResolveCallback) error { + _, err := r.resolve(data, origin, cb) + return err +} + +func (r *secretResolver) resolve(data []byte, origin string, notifyCb secrets.ResolveCallback) ([]byte, error) { if !r.enabled { log.Infof("Agent secrets is disabled by caller") return nil, nil @@ -284,11 +226,10 @@ func (r *secretResolver) Decrypt(data []byte, origin string) ([]byte, error) { // First we collect all new handles in the config newHandles := []string{} haveSecret := false - err = walk( - &config, - nil, - func(yamlPath []string, str string) (string, error) { - if ok, handle := isEnc(str); ok { + + w := &walker{ + resolver: func(yamlPath []string, value string) (string, error) { + if ok, handle := isEnc(value); ok { haveSecret = true // Check if we already know this secret if secret, ok := r.cache[handle]; ok { @@ -298,10 +239,14 @@ func (r *secretResolver) Decrypt(data []byte, origin string) ([]byte, error) { return secret, nil } newHandles = append(newHandles, handle) + return value, nil } - return str, nil - }) - if err != nil { + return value, nil + }, + notifier: notifyCb, + } + + if err := w.walk(&config); err != nil { return nil, err } @@ -324,25 +269,24 @@ func (r *secretResolver) Decrypt(data []byte, origin string) ([]byte, error) { return nil, err } - // Replace all new encrypted secrets in the config - err = walk( - &config, - nil, - func(yamlPath []string, str string) (string, error) { - if ok, handle := isEnc(str); ok { - if secret, ok := secrets[handle]; ok { - log.Debugf("Secret '%s' was retrieved from executable", handle) - // keep track of place where a handle was found - r.registerSecretOrigin(handle, origin, yamlPath) - return secret, nil - } - // This should never happen since fetchSecret will return an error - // if not every handles have been fetched. - return str, fmt.Errorf("unknown secret '%s'", handle) + w.resolver = func(yamlPath []string, value string) (string, error) { + if ok, handle := isEnc(value); ok { + if secret, ok := secrets[handle]; ok { + log.Debugf("Secret '%s' was successfully resolved", handle) + // keep track of place where a handle was found + r.registerSecretOrigin(handle, origin, yamlPath) + return secret, nil } - return str, nil - }) - if err != nil { + + // This should never happen since fetchSecret will return an error if not every handle have + // been fetched. + return "", fmt.Errorf("unknown secret '%s'", handle) + } + return value, nil + } + + // Replace all newly resolved secrets in the config + if err := w.walk(&config); err != nil { return nil, err } } diff --git a/comp/core/secrets/secretsimpl/secrets_mock.go b/comp/core/secrets/secretsimpl/secrets_mock.go index 45bf33d9d276a..cfa456e6bef4e 100644 --- a/comp/core/secrets/secretsimpl/secrets_mock.go +++ b/comp/core/secrets/secretsimpl/secrets_mock.go @@ -16,9 +16,15 @@ import ( "go.uber.org/fx" ) +type callbackArgs struct { + yamlPath []string + value any +} + // MockSecretResolver is a mock of the secret Component useful for testing type MockSecretResolver struct { - resolve map[string]string + resolve map[string]string + callbacks []callbackArgs } var _ secrets.Component = (*MockSecretResolver)(nil) @@ -29,13 +35,19 @@ func (m *MockSecretResolver) Configure(_ string, _ []string, _, _ int, _, _ bool // GetDebugInfo is not implemented func (m *MockSecretResolver) GetDebugInfo(_ io.Writer) {} -// Inject adds data to be decrypted, by returning the value for the given key +// Inject adds data to be resolved, by returning the value for the given key func (m *MockSecretResolver) Inject(key, value string) { m.resolve[key] = value } -// Decrypt returns the secret value based upon the injected data -func (m *MockSecretResolver) Decrypt(data []byte, _ string) ([]byte, error) { +// InjectCallback adds to the list of callbacks that will be used to mock ResolveWithCallback. Each injected callback +// will equal a call to the callback givent to 'ResolveWithCallback' +func (m *MockSecretResolver) InjectCallback(yamlPath []string, value any) { + m.callbacks = append(m.callbacks, callbackArgs{yamlPath: yamlPath, value: value}) +} + +// Resolve returns the secret value based upon the injected data +func (m *MockSecretResolver) Resolve(data []byte, _ string) ([]byte, error) { re := regexp.MustCompile(`ENC\[(.*?)\]`) result := re.ReplaceAllStringFunc(string(data), func(in string) string { key := in[4 : len(in)-1] @@ -44,6 +56,14 @@ func (m *MockSecretResolver) Decrypt(data []byte, _ string) ([]byte, error) { return []byte(result), nil } +// ResolveWithCallback mocks the ResolveWithCallback method of the secrets Component +func (m *MockSecretResolver) ResolveWithCallback(_ []byte, _ string, cb secrets.ResolveCallback) error { + for _, call := range m.callbacks { + cb(call.yamlPath, call.value) + } + return nil +} + // NewMockSecretResolver constructs a MockSecretResolver func NewMockSecretResolver() *MockSecretResolver { return &MockSecretResolver{resolve: make(map[string]string)} diff --git a/comp/core/secrets/secretsimpl/secrets_test.go b/comp/core/secrets/secretsimpl/secrets_test.go index 81e37cd7cd829..56ef9557c73de 100644 --- a/comp/core/secrets/secretsimpl/secrets_test.go +++ b/comp/core/secrets/secretsimpl/secrets_test.go @@ -8,11 +8,11 @@ package secretsimpl import ( "fmt" "sort" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - yaml "gopkg.in/yaml.v2" ) var ( @@ -52,7 +52,7 @@ instances: user: test2 `) - testConfDecrypted = `instances: + testConfResolveed = `instances: - password: password1 user: test - password: password2 @@ -80,7 +80,7 @@ keys_with_dash_string_value: foo: "-" `) - testConfDecryptedDash = `keys_with_dash_string_value: + testConfResolveedDash = `keys_with_dash_string_value: foo: '-' some_encoded_password: password1 ` @@ -97,7 +97,7 @@ some_encoded_password: password1 some_encoded_password: ENC[pass1] `) - testConfDecryptedMultiline = `some_encoded_password: | + testConfResolveedMultiline = `some_encoded_password: | password1 ` testConfMultilineOrigin = handleToContext{ @@ -115,7 +115,7 @@ some: data: ENC[pass1] `) - testConfDecryptedNested = `some: + testConfResolveedNested = `some: encoded: data: password1 ` @@ -127,6 +127,35 @@ some: }, }, } + + testConfNestedMultiple = []byte(`--- +top_level: ENC[pass1] +some: + second_level: ENC[pass2] + encoded: + third_level: ENC[pass3] +`) + + testConfNestedOriginMultiple = handleToContext{ + "pass1": []secretContext{ + { + origin: "test", + yamlPath: "top_level", + }, + }, + "pass2": []secretContext{ + { + origin: "test", + yamlPath: "some/second_level", + }, + }, + "pass3": []secretContext{ + { + origin: "test", + yamlPath: "some/encoded/third_level", + }, + }, + } ) func TestIsEnc(t *testing.T) { @@ -157,77 +186,19 @@ func TestIsEnc(t *testing.T) { assert.Equal(t, "test", secret) } -func TestWalkerError(t *testing.T) { - var config interface{} - err := yaml.Unmarshal(testYamlHash, &config) - require.NoError(t, err) - - err = walk(&config, nil, func([]string, string) (string, error) { - return "", fmt.Errorf("some error") - }) - assert.NotNil(t, err) -} - -func TestWalkerSimple(t *testing.T) { - var config interface{} - err := yaml.Unmarshal([]byte("test"), &config) - require.NoError(t, err) - - stringsCollected := []string{} - err = walk(&config, nil, func(_ []string, str string) (string, error) { - stringsCollected = append(stringsCollected, str) - return str + "_verified", nil - }) - require.NoError(t, err) - - assert.Equal(t, []string{"test"}, stringsCollected) - - updatedConf, err := yaml.Marshal(config) - require.NoError(t, err) - assert.Equal(t, string("test_verified\n"), string(updatedConf)) -} - -func TestWalkerComplex(t *testing.T) { - var config interface{} - err := yaml.Unmarshal(testYamlHash, &config) - require.NoError(t, err) - - stringsCollected := []string{} - err = walk(&config, nil, func(_ []string, str string) (string, error) { - stringsCollected = append(stringsCollected, str) - return str + "_verified", nil - }) - require.NoError(t, err) - - sort.Strings(stringsCollected) - assert.Equal(t, []string{ - "1", - "2", - "test1", - "test2", - "test3", - "test4", - "test5", - }, stringsCollected) - - updatedConf, err := yaml.Marshal(config) - require.NoError(t, err) - assert.Equal(t, string(testYamlHashUpdated), string(updatedConf)) -} - -func TestDecryptNoCommand(t *testing.T) { +func TestResolveNoCommand(t *testing.T) { resolver := newEnabledSecretResolver() resolver.fetchHookFunc = func(secrets []string) (map[string]string, error) { return nil, fmt.Errorf("some error") } // since we didn't set any command this should return without any error - resConf, err := resolver.Decrypt(testConf, "test") + resConf, err := resolver.Resolve(testConf, "test") require.NoError(t, err) assert.Equal(t, testConf, resConf) } -func TestDecryptSecretError(t *testing.T) { +func TestResolveSecretError(t *testing.T) { resolver := newEnabledSecretResolver() resolver.backendCommand = "some_command" @@ -235,15 +206,15 @@ func TestDecryptSecretError(t *testing.T) { return nil, fmt.Errorf("some error") } - _, err := resolver.Decrypt(testConf, "test") + _, err := resolver.Resolve(testConf, "test") require.NotNil(t, err) } -func TestDecrypt(t *testing.T) { +func TestResolve(t *testing.T) { type testCase struct { name string testConf []byte - decryptedConf string + resolveedConf string expectedSecretOrigin handleToContext expectedScrubbedKey []string secretFetchCB func([]string) (map[string]string, error) @@ -253,13 +224,13 @@ func TestDecrypt(t *testing.T) { currentTest := t testCases := []testCase{ { - // TestDecryptSecretStringMapStringWithDashValue checks that a nested string config value + // TestResolveSecretStringMapStringWithDashValue checks that a nested string config value // that can be interpreted as YAML (such as a "-") is not interpreted as YAML by the secrets // decryption logic, but is left unchanged as a string instead. // See https://github.com/DataDog/datadog-agent/pull/6586 for details. name: "map with dash value", testConf: testConfDash, - decryptedConf: testConfDecryptedDash, + resolveedConf: testConfResolveedDash, expectedSecretOrigin: testConfDashOrigin, expectedScrubbedKey: []string{"some_encoded_password"}, secretFetchCB: func(secrets []string) (map[string]string, error) { @@ -275,7 +246,7 @@ func TestDecrypt(t *testing.T) { { name: "multiline", testConf: testConfMultiline, - decryptedConf: testConfDecryptedMultiline, + resolveedConf: testConfResolveedMultiline, expectedSecretOrigin: testConfMultilineOrigin, expectedScrubbedKey: []string{"some_encoded_password"}, secretFetchCB: func(secrets []string) (map[string]string, error) { @@ -291,7 +262,7 @@ func TestDecrypt(t *testing.T) { { name: "nested", testConf: testConfNested, - decryptedConf: testConfDecryptedNested, + resolveedConf: testConfResolveedNested, expectedSecretOrigin: testConfNestedOrigin, expectedScrubbedKey: []string{"data"}, secretFetchCB: func(secrets []string) (map[string]string, error) { @@ -307,7 +278,7 @@ func TestDecrypt(t *testing.T) { { name: "no cache", testConf: testConf, - decryptedConf: testConfDecrypted, + resolveedConf: testConfResolveed, expectedSecretOrigin: testConfOrigin, expectedScrubbedKey: []string{"password", "password"}, secretFetchCB: func(secrets []string) (map[string]string, error) { @@ -326,7 +297,7 @@ func TestDecrypt(t *testing.T) { { name: "partial cache", testConf: testConf, - decryptedConf: testConfDecrypted, + resolveedConf: testConfResolveed, expectedSecretOrigin: testConfOrigin, expectedScrubbedKey: []string{"password", "password"}, secretCache: map[string]string{"pass1": "password1"}, @@ -344,7 +315,7 @@ func TestDecrypt(t *testing.T) { { name: "full cache", testConf: testConf, - decryptedConf: testConfDecrypted, + resolveedConf: testConfResolveed, expectedSecretOrigin: testConfOrigin, expectedScrubbedKey: []string{"password", "password"}, secretCache: map[string]string{"pass1": "password1", "pass2": "password2"}, @@ -368,12 +339,56 @@ func TestDecrypt(t *testing.T) { scrubbedKey := []string{} resolver.scrubHookFunc = func(k []string) { scrubbedKey = append(scrubbedKey, k[0]) } - newConf, err := resolver.Decrypt(tc.testConf, "test") + newConf, err := resolver.Resolve(tc.testConf, "test") require.NoError(t, err) - assert.Equal(t, tc.decryptedConf, string(newConf)) + assert.Equal(t, tc.resolveedConf, string(newConf)) assert.Equal(t, tc.expectedSecretOrigin, resolver.origin) assert.Equal(t, tc.expectedScrubbedKey, scrubbedKey) }) } } + +func TestResolveWithCallback(t *testing.T) { + testConf := testConfNestedMultiple + + resolver := newEnabledSecretResolver() + resolver.backendCommand = "some_command" + resolver.cache = map[string]string{"pass3": "password3"} + + resolver.fetchHookFunc = func(secrets []string) (map[string]string, error) { + return map[string]string{ + "pass1": "password1", + "pass2": "password2", + }, nil + } + + topLevelResolved := 0 + secondLevelResolved := 0 + thirdLevelResolved := 0 + err := resolver.ResolveWithCallback( + testConf, + "test", + func(yamlPath []string, value any) { + switch strings.Join(yamlPath, "/") { + case "top_level": + assert.Equal(t, "password1", value) + topLevelResolved++ + case "some/second_level": + assert.Equal(t, "password2", value) + secondLevelResolved++ + case "some/encoded/third_level": + assert.Equal(t, "password3", value) + thirdLevelResolved++ + default: + assert.Fail(t, "unknown yaml path: %s", yamlPath) + } + }, + ) + require.NoError(t, err) + assert.Equal(t, 1, topLevelResolved, "'top_level' secret was not resolved or resolved multiple times") + assert.Equal(t, 1, secondLevelResolved, "'second_level' secret was not resolved or resolved multiple times") + assert.Equal(t, 1, thirdLevelResolved, "'third_level' secret was not resolved or resolved multiple times") + + assert.Equal(t, testConfNestedOriginMultiple, resolver.origin) +} diff --git a/comp/core/secrets/secretsimpl/walker.go b/comp/core/secrets/secretsimpl/walker.go new file mode 100644 index 0000000000000..49a39584de09d --- /dev/null +++ b/comp/core/secrets/secretsimpl/walker.go @@ -0,0 +1,114 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2016-present Datadog, Inc. + +package secretsimpl + +import ( + "github.com/DataDog/datadog-agent/comp/core/secrets" +) + +type resolverCallback func([]string, string) (string, error) + +type walker struct { + // resolver is called to fetch the value of a handle + resolver resolverCallback + // notifier is called each time a key from the YAML is updated. This is used by ResolveWithCallback. + // + // When a slice is updated, this will be called once with the final slice content. + notifier secrets.ResolveCallback +} + +func (w *walker) notify(yamlPath []string, value any) { + if w.notifier != nil { + w.notifier(yamlPath, value) + } +} + +func (w *walker) string(yamlPath []string, value string, shouldNotify bool) (string, error) { + newValue, err := w.resolver(yamlPath, value) + if err != nil { + return value, err + } + + if shouldNotify && value != newValue { + w.notify(yamlPath, newValue) + } + return newValue, err +} + +func (w *walker) slice(data []interface{}, yamlPath []string) error { + var shouldNotify bool + for idx, k := range data { + switch v := k.(type) { + case string: + if newValue, err := w.string(yamlPath, v, false); err == nil { + if v != newValue { + data[idx] = newValue + shouldNotify = true + } + } else { + return err + } + case map[interface{}]interface{}: + if err := w.hash(v, yamlPath); err != nil { + return err + } + case []interface{}: + if err := w.slice(v, yamlPath); err != nil { + return err + } + } + } + // for slice we notify once with the final values + if shouldNotify { + w.notify(yamlPath, data) + } + return nil +} + +func (w *walker) hash(data map[interface{}]interface{}, yamlPath []string) error { + for k := range data { + path := yamlPath + if newkey, ok := k.(string); ok { + path = append(path, newkey) + } + + switch v := data[k].(type) { + case string: + if newValue, err := w.string(path, v, true); err == nil { + data[k] = newValue + } else { + return err + } + case map[interface{}]interface{}: + if err := w.hash(v, path); err != nil { + return err + } + case []interface{}: + if err := w.slice(v, path); err != nil { + return err + } + } + } + return nil +} + +// walk will go through loaded yaml and invoke the callback on every string node allowing +// the callback to overwrite the string value +func (w *walker) walk(data *interface{}) error { + switch v := (*data).(type) { + case string: + if newValue, err := w.string(nil, v, true); err == nil { + *data = newValue + } else { + return err + } + case map[interface{}]interface{}: + return w.hash(v, nil) + case []interface{}: + return w.slice(v, nil) + } + return nil +} diff --git a/comp/core/secrets/secretsimpl/walker_test.go b/comp/core/secrets/secretsimpl/walker_test.go new file mode 100644 index 0000000000000..e347bf0360b0d --- /dev/null +++ b/comp/core/secrets/secretsimpl/walker_test.go @@ -0,0 +1,134 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2016-present Datadog, Inc. + +package secretsimpl + +import ( + "fmt" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v2" +) + +func TestWalkerError(t *testing.T) { + var config interface{} + err := yaml.Unmarshal(testYamlHash, &config) + require.NoError(t, err) + + w := walker{ + resolver: func([]string, string) (string, error) { + return "", fmt.Errorf("some error") + }, + } + + err = w.walk(&config) + assert.NotNil(t, err) +} + +func TestWalkerSimple(t *testing.T) { + var config interface{} + err := yaml.Unmarshal([]byte("test"), &config) + require.NoError(t, err) + + stringsCollected := []string{} + + w := walker{ + resolver: func(_ []string, str string) (string, error) { + stringsCollected = append(stringsCollected, str) + return str + "_verified", nil + }, + } + err = w.walk(&config) + require.NoError(t, err) + + assert.Equal(t, []string{"test"}, stringsCollected) + + updatedConf, err := yaml.Marshal(config) + require.NoError(t, err) + assert.Equal(t, string("test_verified\n"), string(updatedConf)) +} + +func TestWalkerComplex(t *testing.T) { + var config interface{} + err := yaml.Unmarshal(testYamlHash, &config) + require.NoError(t, err) + + stringsCollected := []string{} + w := walker{ + resolver: func(_ []string, str string) (string, error) { + stringsCollected = append(stringsCollected, str) + return str + "_verified", nil + }, + } + err = w.walk(&config) + require.NoError(t, err) + + sort.Strings(stringsCollected) + assert.Equal(t, []string{ + "1", + "2", + "test1", + "test2", + "test3", + "test4", + "test5", + }, stringsCollected) + + updatedConf, err := yaml.Marshal(config) + require.NoError(t, err) + assert.Equal(t, string(testYamlHashUpdated), string(updatedConf)) +} + +func TestWalkerNotify(t *testing.T) { + yamlConf := []byte(` +slice: + - "ENC 1" + - ["ENC test1", test2] + - 123 +hash: + a: test3 + b: "ENC 2" + c: 456 + slice: + - ENC test4 + - test5 +`) + + notification := map[string]any{} + w := walker{ + resolver: func(_ []string, value string) (string, error) { + if strings.HasPrefix(value, "ENC ") { + return value[4:] + "_verified", nil + } + return value, nil + }, + notifier: func(yamlPath []string, value any) { + notification[strings.Join(yamlPath, ".")] = value + }, + } + + var config interface{} + err := yaml.Unmarshal(yamlConf, &config) + require.NoError(t, err) + + err = w.walk(&config) + require.NoError(t, err) + + // we expect to be notified once for each updated value (especially a single call per slice) + expected := map[string]any{ + "hash.b": "2_verified", + "hash.slice": []any{"test4_verified", "test5"}, + "slice": []any{ + "1_verified", + []any{"test1_verified", "test2"}, + 123, + }, + } + assert.Equal(t, expected, notification) +} diff --git a/comp/core/secrets/type.go b/comp/core/secrets/type.go index 72f07c8389b72..7602578e9c390 100644 --- a/comp/core/secrets/type.go +++ b/comp/core/secrets/type.go @@ -11,5 +11,8 @@ type SecretVal struct { ErrorMsg string `json:"error,omitempty"` } +// ResolveCallback is the callback type used by the ResolveWithCallback method +type ResolveCallback func(key []string, value any) + // PayloadVersion defines the current payload version sent to a secret backend const PayloadVersion = "1.0" diff --git a/pkg/autodiscovery/secrets.go b/pkg/autodiscovery/secrets.go index 3f6735f107922..6db09d295809f 100644 --- a/pkg/autodiscovery/secrets.go +++ b/pkg/autodiscovery/secrets.go @@ -23,7 +23,7 @@ func decryptConfig(conf integration.Config, secretResolver secrets.Component) (i var err error // init_config - conf.InitConfig, err = secretResolver.Decrypt(conf.InitConfig, conf.Name) + conf.InitConfig, err = secretResolver.Resolve(conf.InitConfig, conf.Name) if err != nil { return conf, fmt.Errorf("error while decrypting secrets in 'init_config': %s", err) } @@ -32,7 +32,7 @@ func decryptConfig(conf integration.Config, secretResolver secrets.Component) (i // we cannot update in place as, being a slice, it would modify the input config as well instances := make([]integration.Data, 0, len(conf.Instances)) for _, inputInstance := range conf.Instances { - decryptedInstance, err := secretResolver.Decrypt(inputInstance, conf.Name) + decryptedInstance, err := secretResolver.Resolve(inputInstance, conf.Name) if err != nil { return conf, fmt.Errorf("error while decrypting secrets in an instance: %s", err) } @@ -41,13 +41,13 @@ func decryptConfig(conf integration.Config, secretResolver secrets.Component) (i conf.Instances = instances // metrics - conf.MetricConfig, err = secretResolver.Decrypt(conf.MetricConfig, conf.Name) + conf.MetricConfig, err = secretResolver.Resolve(conf.MetricConfig, conf.Name) if err != nil { return conf, fmt.Errorf("error while decrypting secrets in 'metrics': %s", err) } // logs - conf.LogsConfig, err = secretResolver.Decrypt(conf.LogsConfig, conf.Name) + conf.LogsConfig, err = secretResolver.Resolve(conf.LogsConfig, conf.Name) if err != nil { return conf, fmt.Errorf("error while decrypting secrets 'logs': %s", err) } diff --git a/pkg/autodiscovery/secrets_test.go b/pkg/autodiscovery/secrets_test.go index b6590e9cdd2e0..e1eb4dc01c741 100644 --- a/pkg/autodiscovery/secrets_test.go +++ b/pkg/autodiscovery/secrets_test.go @@ -38,7 +38,7 @@ func (m *MockSecretResolver) Configure(_ string, _ []string, _, _ int, _, _ bool func (m *MockSecretResolver) GetDebugInfo(_ io.Writer) {} -func (m *MockSecretResolver) Decrypt(data []byte, origin string) ([]byte, error) { +func (m *MockSecretResolver) Resolve(data []byte, origin string) ([]byte, error) { if m.scenarios == nil { return data, nil } @@ -48,8 +48,12 @@ func (m *MockSecretResolver) Decrypt(data []byte, origin string) ([]byte, error) return scenario.returnedData, scenario.returnedError } } - m.t.Errorf("Decrypt called with unexpected arguments: data=%s, origin=%s", string(data), origin) - return nil, fmt.Errorf("Decrypt called with unexpected arguments: data=%s, origin=%s", string(data), origin) + m.t.Errorf("Resolve called with unexpected arguments: data=%s, origin=%s", string(data), origin) + return nil, fmt.Errorf("Resolve called with unexpected arguments: data=%s, origin=%s", string(data), origin) +} + +func (m *MockSecretResolver) ResolveWithCallback(_ []byte, _ string, _ secrets.ResolveCallback) error { + return nil } func (m *MockSecretResolver) haveAllScenariosBeenCalled() bool { @@ -111,29 +115,29 @@ var makeSharedScenarios = func() []mockSecretScenario { } } -func TestSecretDecrypt(t *testing.T) { - mockDecrypt := &MockSecretResolver{t, makeSharedScenarios()} +func TestSecretResolve(t *testing.T) { + mockResolve := &MockSecretResolver{t, makeSharedScenarios()} - newConfig, err := decryptConfig(sharedTpl, mockDecrypt) + newConfig, err := decryptConfig(sharedTpl, mockResolve) require.NoError(t, err) assert.NotEqual(t, newConfig.Instances, sharedTpl.Instances) - assert.True(t, mockDecrypt.haveAllScenariosBeenCalled()) + assert.True(t, mockResolve.haveAllScenariosBeenCalled()) } -func TestSkipSecretDecrypt(t *testing.T) { - mockDecrypt := &MockSecretResolver{t, makeSharedScenarios()} +func TestSkipSecretResolve(t *testing.T) { + mockResolve := &MockSecretResolver{t, makeSharedScenarios()} cfg := config.Mock(t) cfg.SetWithoutSource("secret_backend_skip_checks", true) defer cfg.SetWithoutSource("secret_backend_skip_checks", false) - c, err := decryptConfig(sharedTpl, mockDecrypt) + c, err := decryptConfig(sharedTpl, mockResolve) require.NoError(t, err) assert.Equal(t, sharedTpl.Instances, c.Instances) assert.Equal(t, sharedTpl.InitConfig, c.InitConfig) - assert.True(t, mockDecrypt.haveAllScenariosNotCalled()) + assert.True(t, mockResolve.haveAllScenariosNotCalled()) } diff --git a/pkg/config/config.go b/pkg/config/config.go index 48f3bf34e3b19..2bc47036604d6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -7,7 +7,6 @@ package config import ( - "bytes" "context" "encoding/json" "errors" @@ -1774,14 +1773,17 @@ func ResolveSecrets(config Config, secretResolver secrets.Component, origin stri return fmt.Errorf("unable to marshal configuration to YAML to decrypt secrets: %v", err) } - finalYamlConf, err := secretResolver.Decrypt(yamlConf, origin) + err = secretResolver.ResolveWithCallback( + yamlConf, + origin, + func(yamlPath []string, value any) { + settingName := strings.Join(yamlPath, ".") + log.Debugf("replacing handle for setting '%s' with secret value", settingName) + config.Set(settingName, value, pkgconfigmodel.SourceAgentRuntime) + }) if err != nil { return fmt.Errorf("unable to decrypt secret from datadog.yaml: %v", err) } - r := bytes.NewReader(finalYamlConf) - if err = config.MergeConfigOverride(r); err != nil { - return fmt.Errorf("could not update main configuration after decrypting secrets: %v", err) - } } return nil } diff --git a/pkg/config/config_secret_test.go b/pkg/config/config_secret_test.go index e178d662f36fa..d010371076b2c 100644 --- a/pkg/config/config_secret_test.go +++ b/pkg/config/config_secret_test.go @@ -28,10 +28,9 @@ func TestProxyWithSecret(t *testing.T) { { name: "secrets from configuration for proxy", setup: func(t *testing.T, config Config, resolver *secretsimpl.MockSecretResolver) { - resolver.Inject("http_handle", "http_url") - resolver.Inject("https_handle", "https_url") - resolver.Inject("no_proxy_1_handle", "no_proxy_1") - resolver.Inject("no_proxy_2_handle", "no_proxy_2") + resolver.InjectCallback([]string{"proxy", "http"}, "http_url") + resolver.InjectCallback([]string{"proxy", "https"}, "https_url") + resolver.InjectCallback([]string{"proxy", "no_proxy"}, []string{"no_proxy_1", "no_proxy_2"}) config.SetWithoutSource("secret_backend_command", "some_command") config.SetWithoutSource("proxy.http", "ENC[http_handle]") @@ -51,10 +50,9 @@ func TestProxyWithSecret(t *testing.T) { { name: "secrets fron DD env vars for proxy", setup: func(t *testing.T, config Config, resolver *secretsimpl.MockSecretResolver) { - resolver.Inject("http_handle", "http_url") - resolver.Inject("https_handle", "https_url") - resolver.Inject("no_proxy_1_handle", "no_proxy_1") - resolver.Inject("no_proxy_2_handle", "no_proxy_2") + resolver.InjectCallback([]string{"proxy", "http"}, "http_url") + resolver.InjectCallback([]string{"proxy", "https"}, "https_url") + resolver.InjectCallback([]string{"proxy", "no_proxy"}, []string{"no_proxy_1", "no_proxy_2"}) config.SetWithoutSource("secret_backend_command", "some_command") t.Setenv("DD_PROXY_HTTP", "ENC[http_handle]") @@ -74,10 +72,9 @@ func TestProxyWithSecret(t *testing.T) { { name: "secrets fron UNIX env vars for proxy", setup: func(t *testing.T, config Config, resolver *secretsimpl.MockSecretResolver) { - resolver.Inject("http_handle", "http_url") - resolver.Inject("https_handle", "https_url") - resolver.Inject("no_proxy_1_handle", "no_proxy_1") - resolver.Inject("no_proxy_2_handle", "no_proxy_2") + resolver.InjectCallback([]string{"proxy", "http"}, "http_url") + resolver.InjectCallback([]string{"proxy", "https"}, "https_url") + resolver.InjectCallback([]string{"proxy", "no_proxy"}, []string{"no_proxy_1", "no_proxy_2"}) config.SetWithoutSource("secret_backend_command", "some_command") t.Setenv("HTTP_PROXY", "ENC[http_handle]")