From 3fcbd08a62e5d15e9a66e3125260686556cc46d0 Mon Sep 17 00:00:00 2001 From: Zach Bernstein Date: Mon, 27 Jul 2026 11:57:07 -0500 Subject: [PATCH] fix: match AWS Secrets Manager ARNs against ExternalSecret/PushSecret friendly names AwsSqs notification source events carry event.Detail.RequestParameters.SecretId verbatim from CloudTrail, which for PutSecretValue calls is the secret's full ARN (including the random 6-character suffix Secrets Manager appends), not its friendly name. Both AWS's own built-in rotation Lambdas and manual PutSecretValue calls made via the console/CLI with the ARN as --secret-id populate this field with the ARN. The ExternalSecret/PushSecret destination handlers' default References() implementations compared this raw identifier against spec.dataFrom[].extract.key / spec.data[].remoteRef.key / spec.data[].match.remoteRef.remoteKey / spec.selector.secret.name using strict string equality. Since those fields conventionally hold the secret's friendly name (the common, documented way to configure an ExternalSecret against a SecretsManager ClusterSecretStore), the comparison never succeeded for AWS-sourced events. The destination was silently skipped ("skipping object as its not referenced") even though the listener, auth and event delivery pipeline all worked correctly - so, in practice, any ExternalSecret backed by AWS Secrets Manager and referenced by friendly name never got reloaded. Add internal/util.SecretIdentifierMatches (backed by SecretIdentifierAliases), which expands an AWS Secrets Manager ARN into its canonical friendly name before comparing, while still matching a raw identifier by strict equality otherwise (including when a spec already uses the full ARN, preserving existing behavior). Wire this into both the ExternalSecret and PushSecret handlers' _references implementations. Covered by new unit tests in internal/util and the externalsecret/pushsecret handler packages, plus the existing internal/controller envtest suite (still green). --- internal/handler/externalsecret/handler.go | 4 +- .../handler/externalsecret/handler_test.go | 39 ++++++ internal/handler/pushsecret/handler.go | 4 +- internal/handler/pushsecret/handler_test.go | 122 +++++++++++++++++ internal/util/secretid.go | 62 +++++++++ internal/util/secretid_test.go | 123 ++++++++++++++++++ 6 files changed, 350 insertions(+), 4 deletions(-) create mode 100644 internal/handler/pushsecret/handler_test.go create mode 100644 internal/util/secretid.go create mode 100644 internal/util/secretid_test.go diff --git a/internal/handler/externalsecret/handler.go b/internal/handler/externalsecret/handler.go index 21bbc66..f450310 100644 --- a/internal/handler/externalsecret/handler.go +++ b/internal/handler/externalsecret/handler.go @@ -144,14 +144,14 @@ func (h *Handler) _references(obj client.Object, secretIdentifier string) (bool, } // Check Data field for _, data := range es.Spec.Data { - if data.RemoteRef.Key == secretIdentifier { + if util.SecretIdentifierMatches(data.RemoteRef.Key, secretIdentifier) { return true, nil } } // Check DataFrom field for _, dataFrom := range es.Spec.DataFrom { - if dataFrom.Extract != nil && dataFrom.Extract.Key == secretIdentifier { + if dataFrom.Extract != nil && util.SecretIdentifierMatches(dataFrom.Extract.Key, secretIdentifier) { return true, nil } // Handle RegExp matching if needed diff --git a/internal/handler/externalsecret/handler_test.go b/internal/handler/externalsecret/handler_test.go index 678c0fa..55be1b5 100644 --- a/internal/handler/externalsecret/handler_test.go +++ b/internal/handler/externalsecret/handler_test.go @@ -203,6 +203,45 @@ func TestHandler_References_RemoteRefKey(t *testing.T) { } } +// TestHandler_References_DataFromExtractAWSARN verifies that an ExternalSecret +// referencing a secret by its AWS Secrets Manager friendly name (the +// conventional way to configure dataFrom.extract.key) is still considered +// referenced when the event's secretIdentifier is the full ARN that AWS +// notification sources (e.g. AwsSqs reading CloudTrail's +// requestParameters.secretId) actually deliver. +func TestHandler_References_DataFromExtractAWSARN(t *testing.T) { + h := newHandlerWithDefaults() + es := &esov1.ExternalSecret{ + ObjectMeta: metav1.ObjectMeta{Name: "ai-gateway", Namespace: "ai-gateway"}, + Spec: esov1.ExternalSecretSpec{ + DataFrom: []esov1.ExternalSecretDataFromRemoteRef{ + {Extract: &esov1.ExternalSecretDataRemoteRef{Key: "platform/ai-gateway/service-secrets"}}, + }, + }, + } + ref, err := h.References(es, "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj") + if err != nil { + t.Fatalf("References: %v", err) + } + if !ref { + t.Error("expected References to return true when dataFrom.extract.key matches the friendly name embedded in an AWS Secrets Manager ARN") + } +} + +// TestHandler_References_RemoteRefKeyAWSARN is the same as +// TestHandler_References_DataFromExtractAWSARN but for spec.data[].remoteRef.key. +func TestHandler_References_RemoteRefKeyAWSARN(t *testing.T) { + h := newHandlerWithDefaults() + es := externalSecretWithRemoteRefKey("es", "platform/ai-gateway/service-secrets") + ref, err := h.References(es, "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj") + if err != nil { + t.Fatalf("References: %v", err) + } + if !ref { + t.Error("expected References to return true when remoteRef.key matches the friendly name embedded in an AWS Secrets Manager ARN") + } +} + // TestHandler_References_NotExternalSecret verifies that passing a non-ExternalSecret // object returns an error. func TestHandler_References_NotExternalSecret(t *testing.T) { diff --git a/internal/handler/pushsecret/handler.go b/internal/handler/pushsecret/handler.go index eba3529..89553f8 100644 --- a/internal/handler/pushsecret/handler.go +++ b/internal/handler/pushsecret/handler.go @@ -142,11 +142,11 @@ func (h *Handler) _references(obj client.Object, secretIdentifier string) (bool, return false, errors.New("obj isn't type PushSecret") } // Check selector - if ps.Spec.Selector.Secret != nil && ps.Spec.Selector.Secret.Name == secretIdentifier { + if ps.Spec.Selector.Secret != nil && util.SecretIdentifierMatches(ps.Spec.Selector.Secret.Name, secretIdentifier) { return true, nil } for _, data := range ps.Spec.Data { - if data.Match.RemoteRef.RemoteKey == secretIdentifier { + if util.SecretIdentifierMatches(data.Match.RemoteRef.RemoteKey, secretIdentifier) { return true, nil } } diff --git a/internal/handler/pushsecret/handler_test.go b/internal/handler/pushsecret/handler_test.go new file mode 100644 index 0000000..7e4bf35 --- /dev/null +++ b/internal/handler/pushsecret/handler_test.go @@ -0,0 +1,122 @@ +package pushsecret + +import ( + "context" + "testing" + + esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/external-secrets/reloader/api/v1alpha1" +) + +func newHandlerWithDefaults() *Handler { + ctx := context.Background() + scheme := newScheme() + c := fake.NewClientBuilder().WithScheme(scheme).Build() + dest := v1alpha1.DestinationToWatch{ + Type: "PushSecret", + PushSecret: &v1alpha1.PushSecretDestination{}, + } + h := &Handler{ + ctx: ctx, + client: c, + destinationCache: dest, + } + h.referenceFn = h._references + return h +} + +func newScheme() *runtime.Scheme { + scheme := runtime.NewScheme() + _ = esv1alpha1.AddToScheme(scheme) + return scheme +} + +// TestHandler_References_RemoteKey verifies existing behavior: matching +// spec.data[].match.remoteRef.remoteKey returns true. +func TestHandler_References_RemoteKey(t *testing.T) { + h := newHandlerWithDefaults() + ps := &esv1alpha1.PushSecret{ + ObjectMeta: metav1.ObjectMeta{Name: "ps", Namespace: "default"}, + Spec: esv1alpha1.PushSecretSpec{ + Data: []esv1alpha1.PushSecretData{ + {Match: esv1alpha1.PushSecretMatch{RemoteRef: esv1alpha1.PushSecretRemoteRef{RemoteKey: "my-remote-key"}}}, + }, + }, + } + ref, err := h.References(ps, "my-remote-key") + if err != nil { + t.Fatalf("References: %v", err) + } + if !ref { + t.Error("expected References to return true when remoteRef.remoteKey matches") + } +} + +// TestHandler_References_RemoteKeyAWSARN verifies that a PushSecret +// referencing a secret by its AWS Secrets Manager friendly name is still +// considered referenced when the event's secretIdentifier is the full ARN +// that AWS notification sources actually deliver. +func TestHandler_References_RemoteKeyAWSARN(t *testing.T) { + h := newHandlerWithDefaults() + ps := &esv1alpha1.PushSecret{ + ObjectMeta: metav1.ObjectMeta{Name: "ps", Namespace: "default"}, + Spec: esv1alpha1.PushSecretSpec{ + Data: []esv1alpha1.PushSecretData{ + {Match: esv1alpha1.PushSecretMatch{RemoteRef: esv1alpha1.PushSecretRemoteRef{RemoteKey: "platform/ai-gateway/service-secrets"}}}, + }, + }, + } + ref, err := h.References(ps, "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj") + if err != nil { + t.Fatalf("References: %v", err) + } + if !ref { + t.Error("expected References to return true when remoteKey matches the friendly name embedded in an AWS Secrets Manager ARN") + } +} + +// TestHandler_References_SelectorSecretName verifies existing behavior: +// matching spec.selector.secret.name returns true. +func TestHandler_References_SelectorSecretName(t *testing.T) { + h := newHandlerWithDefaults() + ps := &esv1alpha1.PushSecret{ + ObjectMeta: metav1.ObjectMeta{Name: "ps", Namespace: "default"}, + Spec: esv1alpha1.PushSecretSpec{ + Selector: esv1alpha1.PushSecretSelector{ + Secret: &esv1alpha1.PushSecretSecret{Name: "local-secret"}, + }, + }, + } + ref, err := h.References(ps, "local-secret") + if err != nil { + t.Fatalf("References: %v", err) + } + if !ref { + t.Error("expected References to return true when selector.secret.name matches") + } +} + +// TestHandler_References_NoMatch verifies that an unrelated identifier does +// not match. +func TestHandler_References_NoMatch(t *testing.T) { + h := newHandlerWithDefaults() + ps := &esv1alpha1.PushSecret{ + ObjectMeta: metav1.ObjectMeta{Name: "ps", Namespace: "default"}, + Spec: esv1alpha1.PushSecretSpec{ + Data: []esv1alpha1.PushSecretData{ + {Match: esv1alpha1.PushSecretMatch{RemoteRef: esv1alpha1.PushSecretRemoteRef{RemoteKey: "my-remote-key"}}}, + }, + }, + } + ref, err := h.References(ps, "unrelated") + if err != nil { + t.Fatalf("References: %v", err) + } + if ref { + t.Error("expected References to return false for an unrelated identifier") + } +} diff --git a/internal/util/secretid.go b/internal/util/secretid.go new file mode 100644 index 0000000..b099310 --- /dev/null +++ b/internal/util/secretid.go @@ -0,0 +1,62 @@ +package util + +import "regexp" + +// awsSecretsManagerARNPattern matches AWS Secrets Manager secret ARNs, e.g. +// +// arn:aws:secretsmanager:us-east-1:123456789012:secret:platform/ai-gateway/service-secrets-78YXTj +// +// Secrets Manager always appends a hyphen followed by a random 6-character +// alphanumeric suffix to a secret's friendly name when it generates the ARN +// for that secret - see: +// https://docs.aws.amazon.com/secretsmanager/latest/userguide/troubleshoot.html#ARN_secretnamehyphen +// +// Group 1 captures the friendly name (everything between "secret:" and the +// AWS-appended suffix). The partition segment allows for aws, aws-cn and +// aws-us-gov ARNs. +var awsSecretsManagerARNPattern = regexp.MustCompile(`^arn:aws[a-zA-Z0-9-]*:secretsmanager:[^:]*:[^:]*:secret:(.+)-[A-Za-z0-9]{6}$`) + +// SecretIdentifierAliases returns every identifier form that should be +// considered equivalent to identifier when checking whether a destination +// resource (ExternalSecret, PushSecret, ...) references the secret an event +// is about. +// +// Notification sources report the identifier exactly as the upstream +// provider's API surfaced it. For AWS Secrets Manager, sources that derive +// the identifier from CloudTrail request parameters (e.g. the AwsSqs source +// reading requestParameters.secretId from a PutSecretValue/RotateSecret +// event) get the full secret ARN, including the random suffix AWS appends - +// not the friendly name. Both manual PutSecretValue calls made via the AWS +// console/CLI with the ARN as --secret-id and Secrets Manager's own built-in +// rotation Lambdas invoke PutSecretValue with the ARN as SecretId, so this is +// the common case, not an edge case. +// +// Meanwhile, ExternalSecret/PushSecret specs conventionally reference +// secrets by friendly name (e.g. spec.dataFrom[].extract.key: +// "platform/ai-gateway/service-secrets"), since that's what most SecretStore +// provider configs expect and what ties the manifest to a stable value +// across secret recreation. Comparing the two forms with strict equality +// means the reference check never succeeds for AWS-sourced events, so +// reload destinations are silently skipped even though the pipeline +// (listener, auth, event delivery) is working correctly. +func SecretIdentifierAliases(identifier string) []string { + aliases := []string{identifier} + if m := awsSecretsManagerARNPattern.FindStringSubmatch(identifier); m != nil { + aliases = append(aliases, m[1]) + } + return aliases +} + +// SecretIdentifierMatches reports whether candidate - a value taken from a +// destination resource's spec, such as an ExternalSecret's +// spec.dataFrom[].extract.key or a PushSecret's spec.data[].match.remoteRef.remoteKey - +// refers to the same secret as identifier, the raw identifier carried on a +// SecretRotationEvent. +func SecretIdentifierMatches(candidate, identifier string) bool { + for _, alias := range SecretIdentifierAliases(identifier) { + if candidate == alias { + return true + } + } + return false +} diff --git a/internal/util/secretid_test.go b/internal/util/secretid_test.go new file mode 100644 index 0000000..63a1981 --- /dev/null +++ b/internal/util/secretid_test.go @@ -0,0 +1,123 @@ +package util + +import "testing" + +func TestSecretIdentifierAliases(t *testing.T) { + tests := []struct { + name string + identifier string + want []string + }{ + { + name: "plain friendly name is left untouched", + identifier: "platform/ai-gateway/service-secrets", + want: []string{"platform/ai-gateway/service-secrets"}, + }, + { + name: "standard partition ARN expands to friendly name", + identifier: "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj", + want: []string{ + "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj", + "platform/ai-gateway/service-secrets", + }, + }, + { + name: "gov cloud partition ARN expands to friendly name", + identifier: "arn:aws-us-gov:secretsmanager:us-gov-west-1:051826739313:secret:my-secret-AbC123", + want: []string{ + "arn:aws-us-gov:secretsmanager:us-gov-west-1:051826739313:secret:my-secret-AbC123", + "my-secret", + }, + }, + { + name: "china partition ARN expands to friendly name", + identifier: "arn:aws-cn:secretsmanager:cn-north-1:051826739313:secret:my-secret-AbC123", + want: []string{ + "arn:aws-cn:secretsmanager:cn-north-1:051826739313:secret:my-secret-AbC123", + "my-secret", + }, + }, + { + name: "friendly name that itself ends in a 6-char hyphenated segment only strips the AWS-appended suffix", + identifier: "arn:aws:secretsmanager:us-east-1:051826739313:secret:my-secret-abc123-XyZ789", + want: []string{ + "arn:aws:secretsmanager:us-east-1:051826739313:secret:my-secret-abc123-XyZ789", + "my-secret-abc123", + }, + }, + { + // Real Secrets Manager ARNs always carry the AWS-appended suffix, so an + // ARN-shaped identifier that is too short to plausibly contain one (no + // hyphen at all before "secret:") is left untouched rather than mangled. + name: "arn with a friendly name too short to contain a suffix is left untouched", + identifier: "arn:aws:secretsmanager:us-east-1:051826739313:secret:x", + want: []string{"arn:aws:secretsmanager:us-east-1:051826739313:secret:x"}, + }, + { + name: "non-secretsmanager arn is left untouched", + identifier: "arn:aws:sqs:us-east-1:051826739313:my-queue", + want: []string{"arn:aws:sqs:us-east-1:051826739313:my-queue"}, + }, + { + name: "empty identifier", + identifier: "", + want: []string{""}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SecretIdentifierAliases(tt.identifier) + if len(got) != len(tt.want) { + t.Fatalf("SecretIdentifierAliases(%q) = %v, want %v", tt.identifier, got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("SecretIdentifierAliases(%q)[%d] = %q, want %q", tt.identifier, i, got[i], tt.want[i]) + } + } + }) + } +} + +func TestSecretIdentifierMatches(t *testing.T) { + tests := []struct { + name string + candidate string + identifier string + want bool + }{ + { + name: "exact match, no ARN involved", + candidate: "platform/ai-gateway/service-secrets", + identifier: "platform/ai-gateway/service-secrets", + want: true, + }, + { + name: "friendly name in spec matches AWS ARN from event", + candidate: "platform/ai-gateway/service-secrets", + identifier: "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj", + want: true, + }, + { + name: "full ARN in spec still matches identical ARN from event", + candidate: "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj", + identifier: "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj", + want: true, + }, + { + name: "unrelated secret does not match", + candidate: "platform/other-service/service-secrets", + identifier: "arn:aws:secretsmanager:us-east-1:051826739313:secret:platform/ai-gateway/service-secrets-78YXTj", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := SecretIdentifierMatches(tt.candidate, tt.identifier); got != tt.want { + t.Errorf("SecretIdentifierMatches(%q, %q) = %v, want %v", tt.candidate, tt.identifier, got, tt.want) + } + }) + } +}