Skip to content

Commit f6e6779

Browse files
authored
Implement IsValueFromSecret and split Refresh into Refresh and RefreshNow (#46931)
### What does this PR do? Implements `IsValueFromSecret` and `resolvedValues` map that keeps track of secrets and allows a caller to find out if a value came from an `ENC[]` field. This will allow callers to intelligently call `Refresh` instead of wasting Re-tries. Refactors the `Refresh` function that had the `updateNow` argument, to become two functions: `Refresh` and `RefreshNow` as a QOL, `Refresh` also checks if `apiKeyFailureRefreshInterval` and `r.backendCommand` is configured and returns a bool that denotes whether an invalid payload should be retried or not ### Motivation ### Describe how you validated your changes CI ### Additional Notes #46704 (comment) Co-authored-by: saad.naji <saad.naji@datadoghq.com>
1 parent fe12c36 commit f6e6779

14 files changed

Lines changed: 167 additions & 60 deletions

File tree

cmd/process-agent/api/secrets.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import (
1010
)
1111

1212
func secretRefreshHandler(deps APIServerDeps, w http.ResponseWriter, _ *http.Request) {
13-
response, err := deps.Secrets.Refresh(true)
13+
response, err := deps.Secrets.RefreshNow()
1414
if err != nil {
1515
deps.Log.Errorf("error while refreshing secrets: %s", err)
1616
writeError(err, http.StatusInternalServerError, w)

cmd/security-agent/api/agent/agent.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ func (a *Agent) makeFlare(w http.ResponseWriter, _ *http.Request) {
146146
}
147147

148148
func (a *Agent) refreshSecrets(w http.ResponseWriter, _ *http.Request) {
149-
res, err := a.secrets.Refresh(true)
149+
res, err := a.secrets.RefreshNow()
150150
if err != nil {
151151
log.Errorf("error while refreshing secrets: %s", err)
152152
w.Header().Set("Content-Type", "application/json")

comp/core/autodiscovery/autodiscoveryimpl/secrets_test.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,18 @@ func (m *MockSecretResolver) SubscribeToChanges(callback secrets.SecretChangeCal
5959
m.subscribers = append(m.subscribers, callback)
6060
}
6161

62-
func (m *MockSecretResolver) Refresh(_ bool) (string, error) {
62+
func (m *MockSecretResolver) Refresh() bool {
63+
return false
64+
}
65+
66+
func (m *MockSecretResolver) RefreshNow() (string, error) {
6367
return "", nil
6468
}
6569

70+
func (m *MockSecretResolver) IsValueFromSecret(_ string) bool {
71+
return false
72+
}
73+
6674
func (m *MockSecretResolver) haveAllScenariosBeenCalled() bool {
6775
for _, scenario := range m.scenarios {
6876
if scenario.called == 0 {

comp/core/secrets/def/component.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,13 @@ type Component interface {
3939
Resolve(data []byte, origin string, imageName string, kubeNamespace string, notify bool) ([]byte, error)
4040
// SubscribeToChanges registers a callback to be invoked whenever secrets are resolved or refreshed
4141
SubscribeToChanges(callback SecretChangeCallback)
42-
// Refresh will resolve secret handles again, notifying any subscribers of changed values.
43-
// If updateNow is true, the function performs the refresh immediately and blocks, returning an informative message suitable for user display.
44-
// If updateNow is false, the function will asynchronously perform a refresh, and may fail to refresh due to throttling. No message is returned, just an empty string.
45-
Refresh(updateNow bool) (string, error)
42+
// Refresh schedules a throttled asynchronous secret refresh. Returns true if the
43+
// secret refresh mechanism is enabled (backend configured and refresh interval set).
44+
Refresh() bool
45+
// RefreshNow performs an immediate blocking secret refresh, returning an informative message suitable for user display.
46+
RefreshNow() (string, error)
47+
// IsValueFromSecret returns true if the given value was ever resolved from a secret handle.
48+
IsValueFromSecret(value string) bool
4649
// RemoveOrigin removes a origin from the internal cache of the secret component. This does not remove secrets
4750
// from the cache but the reference where those secrets are used.
4851
RemoveOrigin(origin string)

comp/core/secrets/impl/secrets.go

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,10 @@ type secretResolver struct {
8787
// list of handles and where they were found
8888
origin handleToContext
8989

90+
// resolvedSecretValues is an append-only set of all secret values ever returned by the backend.
91+
// old values are retained for IsValueFromSecret lookups.
92+
resolvedSecretValues map[string]struct{}
93+
9094
backendType string
9195
backendConfig map[string]interface{}
9296
backendCommand string
@@ -139,6 +143,7 @@ func newEnabledSecretResolver(telemetry telemetry.Component) *secretResolver {
139143
return &secretResolver{
140144
cache: make(map[string]string),
141145
origin: make(handleToContext),
146+
resolvedSecretValues: make(map[string]struct{}),
142147
tlmSecretBackendElapsed: telemetry.NewGauge("secret_backend", "elapsed_ms", []string{"command", "exit_code"}, "Elapsed time of secret backend invocation"),
143148
tlmSecretUnmarshalError: telemetry.NewCounter("secret_backend", "unmarshal_errors_count", []string{}, "Count of errors when unmarshalling the output of the secret binary"),
144149
tlmSecretResolveError: telemetry.NewCounter("secret_backend", "resolve_errors_count", []string{"error_kind", "handle"}, "Count of errors when resolving a secret"),
@@ -212,7 +217,7 @@ func (r *secretResolver) writeDebugInfo(w http.ResponseWriter, _ *http.Request)
212217
}
213218

214219
func (r *secretResolver) handleRefresh(w http.ResponseWriter, _ *http.Request) {
215-
result, err := r.Refresh(true)
220+
result, err := r.RefreshNow()
216221
if err != nil {
217222
log.Infof("could not refresh secrets: %s", err)
218223
setJSONError(w, err, 500)
@@ -667,28 +672,42 @@ func (r *secretResolver) processSecretResponse(secretResponse map[string]string,
667672
}
668673
// add results to the cache
669674
stdmaps.Copy(r.cache, secretResponse)
675+
// track all resolved values in an append-only set for IsValueFromSecret lookups
676+
for _, secretValue := range secretResponse {
677+
r.resolvedSecretValues[secretValue] = struct{}{}
678+
}
670679
// return info about the handles sorted by their name
671680
sort.Slice(handleInfoList, func(i, j int) bool {
672681
return handleInfoList[i].Name < handleInfoList[j].Name
673682
})
674683
return secretRefreshInfo{Handles: handleInfoList}
675684
}
676685

677-
// Refresh will resolve secret handles again, notifying any subscribers of changed values.
678-
// If updateNow is true, the function performs the refresh immediately and blocks, returning an informative message suitable for user display.
679-
// If updateNow is false, the function will asynchronously perform a refresh, and may fail to refresh due to throttling. No message is returned, just an empty string.
680-
func (r *secretResolver) Refresh(updateNow bool) (string, error) {
681-
if updateNow {
682-
// blocking refresh
683-
return r.performRefresh()
686+
// Refresh schedules a throttled asynchronous secret refresh. Returns true if the
687+
// secret refresh mechanism is enabled (backend configured and refresh interval set).
688+
func (r *secretResolver) Refresh() bool {
689+
if r.apiKeyFailureRefreshInterval == 0 || r.backendCommand == "" {
690+
return false
684691
}
685-
686-
// non-blocking refresh, max 1 at a time, others dropped
692+
// non-blocking send, max 1 at a time, others dropped
687693
select {
688694
case r.refreshTrigger <- struct{}{}:
689695
default:
690696
}
691-
return "", nil
697+
return true
698+
}
699+
700+
// RefreshNow performs an immediate blocking secret refresh and returns an informative message suitable for user display.
701+
func (r *secretResolver) RefreshNow() (string, error) {
702+
return r.performRefresh()
703+
}
704+
705+
// IsValueFromSecret returns true if the given value was ever resolved from a secret handle.
706+
func (r *secretResolver) IsValueFromSecret(value string) bool {
707+
r.lock.Lock()
708+
defer r.lock.Unlock()
709+
_, ok := r.resolvedSecretValues[value]
710+
return ok
692711
}
693712

694713
// RemoveOrigin removes a origin from the cache

comp/core/secrets/impl/secrets_test.go

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -591,7 +591,7 @@ func TestResolveThenRefresh(t *testing.T) {
591591

592592
// refresh the secrets and only collect newly updated keys
593593
keysResolved = []string{}
594-
output, err := resolver.Refresh(true)
594+
output, err := resolver.RefreshNow()
595595
require.NoError(t, err)
596596
assert.Equal(t, testConfNestedOriginMultiple, resolver.origin)
597597
assert.Equal(t, []string{"some/second_level"}, keysResolved)
@@ -610,7 +610,7 @@ func TestResolveThenRefresh(t *testing.T) {
610610

611611
// refresh one last time and only those two handles have updated keys
612612
keysResolved = []string{}
613-
_, err = resolver.Refresh(true)
613+
_, err = resolver.RefreshNow()
614614
require.NoError(t, err)
615615
slices.Sort(keysResolved)
616616
assert.Equal(t, testConfNestedOriginMultiple, resolver.origin)
@@ -655,15 +655,15 @@ func TestRefreshAllowlist(t *testing.T) {
655655
allowListPaths = []string{"api_key"}
656656

657657
// Refresh means nothing changes because allowlist doesn't allow it
658-
_, err := resolver.Refresh(true)
658+
_, err := resolver.RefreshNow()
659659
require.NoError(t, err)
660660
assert.Equal(t, changes, []string{})
661661

662662
// now allow the config setting under scrutiny to change
663663
allowListPaths = []string{"setting"}
664664

665665
// Refresh sees the change to the handle
666-
_, err = resolver.Refresh(true)
666+
_, err = resolver.RefreshNow()
667667
require.NoError(t, err)
668668
assert.Equal(t, changes, []string{"second_value"})
669669
}
@@ -712,7 +712,7 @@ func TestRefreshAllowlistFromContainer(t *testing.T) {
712712
})
713713

714714
// Refresh means nothing changes because allowlist doesn't allow it
715-
_, err := resolver.Refresh(true)
715+
_, err := resolver.RefreshNow()
716716
require.NoError(t, err)
717717
slices.Sort(changes)
718718
assert.Equal(t, changes, []string{
@@ -759,7 +759,7 @@ func TestRefreshAllowlistAppliesToEachSettingPath(t *testing.T) {
759759
}
760760

761761
// only 1 setting path got updated
762-
_, err = resolver.Refresh(true)
762+
_, err = resolver.RefreshNow()
763763
require.NoError(t, err)
764764
assert.Equal(t, changedPaths, []string{"instances/0/password"})
765765
}
@@ -795,7 +795,7 @@ func TestRefreshAddsToAuditFile(t *testing.T) {
795795
}
796796

797797
// Refresh the secrets, which will add to the audit file
798-
_, err = resolver.Refresh(true)
798+
_, err = resolver.RefreshNow()
799799
require.NoError(t, err)
800800
assert.Equal(t, auditFileNumRows(tmpfile.Name()), 1)
801801

@@ -806,7 +806,7 @@ func TestRefreshAddsToAuditFile(t *testing.T) {
806806
}
807807

808808
// Refresh secrets again, which will add another row the audit file
809-
_, err = resolver.Refresh(true)
809+
_, err = resolver.RefreshNow()
810810
require.NoError(t, err)
811811
assert.Equal(t, auditFileNumRows(tmpfile.Name()), 2)
812812

@@ -832,9 +832,9 @@ func TestRefreshModes(t *testing.T) {
832832
return map[string]string{"api_key": "test_value"}, nil
833833
}
834834

835-
t.Run("updateNow=true refreshes synchronously", func(t *testing.T) {
835+
t.Run("RefreshNow refreshes synchronously", func(t *testing.T) {
836836
calls.Store(0)
837-
result, err := resolver.Refresh(true)
837+
result, err := resolver.RefreshNow()
838838
require.NoError(t, err)
839839
assert.NotEmpty(t, result)
840840
assert.Equal(t, int32(1), calls.Load())
@@ -848,16 +848,16 @@ func TestRefreshModes(t *testing.T) {
848848
calls.Store(0)
849849

850850
// 1 refresh passes, 2 get dropped
851-
resolver.Refresh(false)
852-
resolver.Refresh(false)
853-
resolver.Refresh(false)
851+
resolver.Refresh()
852+
resolver.Refresh()
853+
resolver.Refresh()
854854
time.Sleep(50 * time.Millisecond)
855855

856856
assert.Equal(t, int32(1), calls.Load(), "only first refresh should process")
857857

858858
// after interval, next should succeed
859859
time.Sleep(100 * time.Millisecond)
860-
resolver.Refresh(false)
860+
resolver.Refresh()
861861
time.Sleep(50 * time.Millisecond)
862862

863863
assert.Equal(t, int32(2), calls.Load(), "refresh after interval should process")
@@ -867,14 +867,46 @@ func TestRefreshModes(t *testing.T) {
867867
resolver.apiKeyFailureRefreshInterval = 0
868868

869869
calls.Store(0)
870-
resolver.Refresh(false)
871-
resolver.Refresh(false)
870+
resolver.Refresh()
871+
resolver.Refresh()
872872
time.Sleep(50 * time.Millisecond)
873873

874874
assert.Equal(t, int32(0), calls.Load(), "no refreshes when disabled")
875875
})
876876
}
877877

878+
func TestIsValueFromSecret(t *testing.T) {
879+
tel := nooptelemetry.GetCompatComponent()
880+
resolver := newEnabledSecretResolver(tel)
881+
resolver.backendCommand = "some_command"
882+
883+
// initially no values are from secrets
884+
assert.False(t, resolver.IsValueFromSecret("password1"))
885+
886+
resolver.fetchHookFunc = func([]string) (map[string]string, error) {
887+
return map[string]string{"pass1": "password1"}, nil
888+
}
889+
_, err := resolver.Resolve(testSimpleConf, "test", "", "", true)
890+
require.NoError(t, err)
891+
892+
assert.True(t, resolver.IsValueFromSecret("password1"))
893+
assert.False(t, resolver.IsValueFromSecret("some_other_value"))
894+
895+
// mock key rotation
896+
resolver.fetchHookFunc = func([]string) (map[string]string, error) {
897+
return map[string]string{"pass1": "password2"}, nil
898+
}
899+
_, err = resolver.RefreshNow()
900+
require.NoError(t, err)
901+
902+
// the new value is recognized
903+
assert.True(t, resolver.IsValueFromSecret("password2"))
904+
// the OLD value is still recognized
905+
assert.True(t, resolver.IsValueFromSecret("password1"))
906+
907+
assert.False(t, resolver.IsValueFromSecret("some_other_hardcoded_key"))
908+
}
909+
878910
func TestStartRefreshRoutineWithScatter(t *testing.T) {
879911
testCases := []struct {
880912
name string
@@ -1318,7 +1350,7 @@ func TestRefreshOutput(t *testing.T) {
13181350

13191351
password = "password2"
13201352

1321-
res, err := resolver.Refresh(true)
1353+
res, err := resolver.RefreshNow()
13221354
require.NoError(t, err)
13231355
res = strings.ReplaceAll(res, "\r", "") // templates use OS line breaks, removes \r line breaks from windows
13241356
assert.Equal(t, "=== Secret stats ===\nNumber of secrets reloaded: 1\nSecrets handle reloaded:\n\n- 'pass1':\n\tused in 'origin1' configuration in entry 'secret_backend_arguments/0'\n", res)

comp/core/secrets/mock/mock.go

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,12 @@ import (
1919

2020
// Mock is a mock of the secret Component useful for testing
2121
type Mock struct {
22-
secretsCache map[string]string
23-
callbacks []secrets.SecretChangeCallback
24-
refreshHook func(bool) (string, error)
22+
secretsCache map[string]string
23+
callbacks []secrets.SecretChangeCallback
24+
refreshHook func() bool
25+
refreshNowHook func() (string, error)
26+
isValueFromSecretHook func(string) bool
27+
isValueFromSecretSet map[string]struct{}
2528
}
2629

2730
var _ secrets.Component = (*Mock)(nil)
@@ -89,17 +92,55 @@ func (m *Mock) SubscribeToChanges(callback secrets.SecretChangeCallback) {
8992
}
9093

9194
// SetRefreshHook sets a hook function that will be called when Refresh is invoked
92-
func (m *Mock) SetRefreshHook(hook func(bool) (string, error)) {
95+
func (m *Mock) SetRefreshHook(hook func() bool) {
9396
m.refreshHook = hook
9497
}
9598

96-
// Refresh will resolve secret handles again, notifying any subscribers of changed values
97-
func (m *Mock) Refresh(updateNow bool) (string, error) {
99+
// SetRefreshNowHook sets a hook function that will be called when RefreshNow is invoked
100+
func (m *Mock) SetRefreshNowHook(hook func() (string, error)) {
101+
m.refreshNowHook = hook
102+
}
103+
104+
// Refresh schedules an asynchronous secret refresh
105+
func (m *Mock) Refresh() bool {
98106
if m.refreshHook != nil {
99-
return m.refreshHook(updateNow)
107+
return m.refreshHook()
108+
}
109+
return false
110+
}
111+
112+
// RefreshNow performs an immediate blocking secret refresh
113+
func (m *Mock) RefreshNow() (string, error) {
114+
if m.refreshNowHook != nil {
115+
return m.refreshNowHook()
100116
}
101117
return "", nil
102118
}
103119

120+
// SetIsValueFromSecretHook sets a hook function that will be called when IsValueFromSecret is invoked
121+
func (m *Mock) SetIsValueFromSecretHook(hook func(string) bool) {
122+
m.isValueFromSecretHook = hook
123+
}
124+
125+
// SetSecretOriginatedValues sets a predefined set of values that IsValueFromSecret will recognize
126+
func (m *Mock) SetSecretOriginatedValues(values []string) {
127+
m.isValueFromSecretSet = make(map[string]struct{}, len(values))
128+
for _, v := range values {
129+
m.isValueFromSecretSet[v] = struct{}{}
130+
}
131+
}
132+
133+
// IsValueFromSecret returns true if the given value was ever resolved from a secret handle
134+
func (m *Mock) IsValueFromSecret(value string) bool {
135+
if m.isValueFromSecretHook != nil {
136+
return m.isValueFromSecretHook(value)
137+
}
138+
if m.isValueFromSecretSet != nil {
139+
_, ok := m.isValueFromSecretSet[value]
140+
return ok
141+
}
142+
return false
143+
}
144+
104145
// RemoveOrigin
105146
func (m *Mock) RemoveOrigin(_ string) {}

comp/core/secrets/noop-impl/types/types.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,20 @@ func (r *SecretNoop) Resolve(data []byte, _ string, _ string, _ string, _ bool)
2424
return data, nil
2525
}
2626

27-
// Refresh does nothing
28-
func (r *SecretNoop) Refresh(_ bool) (string, error) {
27+
// Refresh does nothing and returns false
28+
func (r *SecretNoop) Refresh() bool {
29+
return false
30+
}
31+
32+
// RefreshNow does nothing
33+
func (r *SecretNoop) RefreshNow() (string, error) {
2934
return "", nil
3035
}
3136

37+
// IsValueFromSecret always returns false for noop
38+
func (r *SecretNoop) IsValueFromSecret(_ string) bool {
39+
return false
40+
}
41+
3242
// RemoveOrigin
3343
func (r *SecretNoop) RemoveOrigin(_ string) {}

0 commit comments

Comments
 (0)