From a0de5b695559af622f263af15bf231fbf613c81f Mon Sep 17 00:00:00 2001 From: Peter Jiang Date: Fri, 8 May 2026 13:37:37 -0700 Subject: [PATCH] fix: apply HideSecretData to server-side diff results for Secrets (cherry-pick argoproj/argo-cd#27598) Signed-off-by: Peter Jiang --- pkg/diff/diff.go | 23 +++++++- pkg/diff/diff_test.go | 120 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 2 deletions(-) diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 8513eb03d..1b0405b80 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -189,13 +189,23 @@ func serverSideDiff(config, live *unstructured.Unstructured, opts ...Option) (*D Normalize(predictedLive, opts...) unstructured.RemoveNestedField(predictedLive.Object, "metadata", "managedFields") + Normalize(live, opts...) + unstructured.RemoveNestedField(live.Object, "metadata", "managedFields") + + if isCoreSecret(config) { + // Mask Secret data symmetrically before comparison. + // Equal values get equal placeholders, different values get different placeholders. + predictedLive, live, err = HideSecretData(predictedLive, live, nil) + if err != nil { + return nil, fmt.Errorf("error hiding secret data for resource %s/%s: %w", config.GetKind(), config.GetName(), err) + } + } + predictedLiveBytes, err := json.Marshal(predictedLive) if err != nil { return nil, fmt.Errorf("error marshaling predicted live for resource %s/%s: %w", config.GetKind(), config.GetName(), err) } - Normalize(live, opts...) - unstructured.RemoveNestedField(live.Object, "metadata", "managedFields") liveBytes, err := json.Marshal(live) if err != nil { return nil, fmt.Errorf("error marshaling live resource %s/%s: %w", config.GetKind(), config.GetName(), err) @@ -355,6 +365,15 @@ func jsonStrToUnstructured(jsonString string) (*unstructured.Unstructured, error return &unstructured.Unstructured{Object: res}, nil } +// isCoreSecret reports whether obj is a core/v1 Secret (Group="" and Kind="Secret"). +func isCoreSecret(obj *unstructured.Unstructured) bool { + if obj == nil { + return false + } + gvk := obj.GroupVersionKind() + return gvk.Group == "" && gvk.Kind == "Secret" +} + // StructuredMergeDiff will calculate the diff using the structured-merge-diff // k8s library (https://github.com/kubernetes-sigs/structured-merge-diff). func StructuredMergeDiff(config, live *unstructured.Unstructured, gvkParser *managedfields.GvkParser, manager string) (*DiffResult, error) { diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index 69d0b1d58..b05b4d51b 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -1166,6 +1166,126 @@ func TestServerSideDiff(t *testing.T) { assert.Empty(t, predictedDeploy.Annotations[AnnotationLastAppliedConfig]) assert.Empty(t, liveDeploy.Annotations[AnnotationLastAppliedConfig]) }) + + t.Run("will mask Secret data symmetrically so identical values do not produce a spurious diff", func(t *testing.T) { + t.Parallel() + + desired := buildSecret("test-secret", "default", map[string]string{"password": "vault:secret/foo"}, nil) + live := buildSecret("test-secret", "default", map[string]string{"password": "injected-by-webhook"}, nil) + predictedLiveJSON := mustMarshalJSON(t, buildSecret("test-secret", "default", map[string]string{"password": "injected-by-webhook"}, nil)) + + opts := append(buildOpts(predictedLiveJSON), WithIgnoreMutationWebhook(false)) + result, err := serverSideDiff(desired, live, opts...) + require.NoError(t, err) + require.NotNil(t, result) + + assert.False(t, result.Modified, "identical secret values on both sides must not be flagged as modified after masking") + + predictedData := mustGetSecretData(t, result.PredictedLive) + liveData := mustGetSecretData(t, result.NormalizedLive) + assert.Equal(t, "++++++++", predictedData["password"], "predicted data must be masked, not raw") + assert.Equal(t, "++++++++", liveData["password"], "live data must be masked, not raw") + }) + + t.Run("will keep Secret data masked but still detect genuine value differences", func(t *testing.T) { + t.Parallel() + + desired := buildSecret("test-secret", "default", map[string]string{"password": "vault:secret/foo"}, nil) + live := buildSecret("test-secret", "default", map[string]string{"password": "old-value"}, nil) + predictedLiveJSON := mustMarshalJSON(t, buildSecret("test-secret", "default", map[string]string{"password": "new-value"}, nil)) + + opts := append(buildOpts(predictedLiveJSON), WithIgnoreMutationWebhook(false)) + result, err := serverSideDiff(desired, live, opts...) + require.NoError(t, err) + require.NotNil(t, result) + + assert.True(t, result.Modified, "different secret values must still be flagged as modified") + + predictedData := mustGetSecretData(t, result.PredictedLive) + liveData := mustGetSecretData(t, result.NormalizedLive) + // HideSecretData yields different placeholder lengths for different values, so the + // data field is masked on both sides and the two placeholders differ. + assert.NotEqual(t, "new-value", predictedData["password"], "raw new value must not leak into PredictedLive") + assert.NotEqual(t, "old-value", liveData["password"], "raw old value must not leak into NormalizedLive") + assert.NotEqual(t, predictedData["password"], liveData["password"], "differing values must yield differing placeholders") + }) + + t.Run("will detect Secret key additions and removals", func(t *testing.T) { + t.Parallel() + + desired := buildSecret("test-secret", "default", map[string]string{"password": "x", "token": "y"}, nil) + live := buildSecret("test-secret", "default", map[string]string{"password": "x"}, nil) + predictedLiveJSON := mustMarshalJSON(t, buildSecret("test-secret", "default", map[string]string{"password": "x", "token": "y"}, nil)) + + opts := append(buildOpts(predictedLiveJSON), WithIgnoreMutationWebhook(false)) + result, err := serverSideDiff(desired, live, opts...) + require.NoError(t, err) + require.NotNil(t, result) + + assert.True(t, result.Modified, "added Secret keys must still be flagged as modified after masking") + }) + + t.Run("will not mask non-core Secret resources", func(t *testing.T) { + // Resources whose Kind is "Secret" but whose Group is non-empty (e.g. CRDs) + // must not be touched by the core/v1 Secret masking path. + t.Parallel() + + desired := buildSecret("test-secret", "default", map[string]string{"password": "raw-value"}, nil) + desired.SetAPIVersion("custom.io/v1") + live := buildSecret("test-secret", "default", map[string]string{"password": "raw-value"}, nil) + live.SetAPIVersion("custom.io/v1") + predictedLiveJSON := mustMarshalJSON(t, desired) + + opts := append(buildOpts(predictedLiveJSON), WithIgnoreMutationWebhook(false)) + result, err := serverSideDiff(desired, live, opts...) + require.NoError(t, err) + require.NotNil(t, result) + + predictedData := mustGetSecretData(t, result.PredictedLive) + assert.Equal(t, "raw-value", predictedData["password"], "non-core Secret data must be left untouched") + }) +} + +// buildSecret returns a core/v1 Secret as an *unstructured.Unstructured. +func buildSecret(name, namespace string, data map[string]string, annotations map[string]string) *unstructured.Unstructured { + dataField := make(map[string]any, len(data)) + for k, v := range data { + dataField[k] = v + } + metadata := map[string]any{ + "name": name, + "namespace": namespace, + } + if len(annotations) > 0 { + annField := make(map[string]any, len(annotations)) + for k, v := range annotations { + annField[k] = v + } + metadata["annotations"] = annField + } + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": metadata, + "type": "Opaque", + "data": dataField, + }} +} + +func mustMarshalJSON(t *testing.T, obj *unstructured.Unstructured) string { + t.Helper() + bytes, err := json.Marshal(obj) + require.NoError(t, err) + return string(bytes) +} + +func mustGetSecretData(t *testing.T, secretBytes []byte) map[string]any { + t.Helper() + var obj map[string]any + require.NoError(t, json.Unmarshal(secretBytes, &obj)) + data, ok := obj["data"].(map[string]any) + require.True(t, ok, "expected data field to be a map") + return data } // testIgnoreDifferencesNormalizer implements a simple normalizer that removes specified fields