diff --git a/cmd/secret/enumerate.go b/cmd/secret/enumerate.go index e5b688f727..81b7ed4937 100644 --- a/cmd/secret/enumerate.go +++ b/cmd/secret/enumerate.go @@ -18,9 +18,10 @@ import ( // scopeEntry is a single (stack, component) instance that declares one or more secrets, paired // with its resolved component section (declarations carry their derived scope after stack merge). type scopeEntry struct { - Stack string - Component string - Section map[string]any + Stack string + Component string + ComponentType string + Section map[string]any } // enumerateScopesFn is a seam so tests can inject scope entries without real stack processing. @@ -66,7 +67,8 @@ func enumerateSecretScopes(facet secretScope) ([]scopeEntry, *schema.AtmosConfig // collectSecretScopeEntries traverses the describe-stacks map // (stack -> components -> -> component -> section) and keeps the instances that declare -// secrets, optionally narrowed to a single component. Entries are sorted by stack then component. +// secrets, optionally narrowed to a single component. Entries are sorted by stack, component, +// then component type so a name shared across component types has deterministic ordering. func collectSecretScopeEntries(stacksMap map[string]any, componentFilter string) []scopeEntry { var entries []scopeEntry for stackName, raw := range stacksMap { @@ -81,7 +83,10 @@ func collectSecretScopeEntries(stacksMap map[string]any, componentFilter string) if entries[i].Stack != entries[j].Stack { return entries[i].Stack < entries[j].Stack } - return entries[i].Component < entries[j].Component + if entries[i].Component != entries[j].Component { + return entries[i].Component < entries[j].Component + } + return entries[i].ComponentType < entries[j].ComponentType }) return entries } @@ -94,7 +99,7 @@ func secretEntriesInStack(stackName string, stackMap map[string]any, componentFi return nil } var entries []scopeEntry - for _, typeRaw := range comps { + for componentType, typeRaw := range comps { typeMap, ok := typeRaw.(map[string]any) if !ok { continue @@ -110,7 +115,7 @@ func secretEntriesInStack(stackName string, stackMap map[string]any, componentFi if len(secrets.ExtractDeclarations(section)) == 0 { continue } - entries = append(entries, scopeEntry{Stack: stackName, Component: compName, Section: section}) + entries = append(entries, scopeEntry{Stack: stackName, Component: compName, ComponentType: componentType, Section: section}) } } return entries diff --git a/cmd/secret/enumerate_test.go b/cmd/secret/enumerate_test.go index d184945e2c..0bbe2eb3b9 100644 --- a/cmd/secret/enumerate_test.go +++ b/cmd/secret/enumerate_test.go @@ -99,6 +99,21 @@ func TestCollectSecretScopeEntries_ComponentFilter(t *testing.T) { assert.Equal(t, "api", entries[0].Component) } +func TestCollectSecretScopeEntries_SortsSharedNameByComponentType(t *testing.T) { + stacksMap := map[string]any{ + "dev": map[string]any{ + cfg.ComponentsSectionName: map[string]any{ + "terraform": map[string]any{"example-service": declaringSection("SHARED_TOKEN")}, + "helm": map[string]any{"example-service": declaringSection("SHARED_TOKEN")}, + }, + }, + } + + entries := collectSecretScopeEntries(stacksMap, "") + require.Len(t, entries, 2) + assert.Equal(t, []string{"helm", "terraform"}, []string{entries[0].ComponentType, entries[1].ComponentType}) +} + // TestSecretEntriesInStack covers the per-stack edge cases: a missing components section, a // non-map component-type node, and a section that is not a map. func TestSecretEntriesInStack(t *testing.T) { diff --git a/cmd/secret/set.go b/cmd/secret/set.go index 3e5793676b..296c446dd1 100644 --- a/cmd/secret/set.go +++ b/cmd/secret/set.go @@ -41,7 +41,7 @@ func init() { func runSecretSet(cmd *cobra.Command, args []string) error { defer perf.Track(nil, "secret.runSecretSet")() - scope, err := parseScope(cmd, args) + scope, err := parseSetScope(cmd, args) if err != nil { return err } @@ -70,6 +70,86 @@ func runSecretSet(cmd *cobra.Command, args []string) error { return nil } +// parseSetScope permits --component to be omitted only when a positional name resolves to one +// consistent global declaration in the selected stack. A component is still used internally to +// load the inherited declaration, but it cannot affect the resulting global backend coordinate. +func parseSetScope(cmd *cobra.Command, args []string) (secretScope, error) { + scope, err := parseScopeStack(cmd, args) + if err != nil { + return scope, err + } + if scope.Component != "" || len(args) == 0 { + return requireScopeComponent(scope, cmd, args) + } + target, err := setTargetFromArg(args[0]) + if err != nil { + return scope, err + } + component, componentType, err := findGlobalSetContext(scope, target.name) + if err != nil { + return scope, err + } + scope.Component = component + if scope.ComponentType == "" { + scope.ComponentType = componentType + } + return scope, nil +} + +func findGlobalSetContext(scope secretScope, name string) (string, string, error) { + entries, _, err := enumerateScopesFn(secretScope{Stack: scope.Stack, ComponentType: scope.ComponentType}) + if err != nil { + return "", "", componentRequiredForSet(name, fmt.Sprintf("the global declaration could not be verified: %v", err)) + } + var selected *secrets.Declaration + var component, componentType string + for _, entry := range entries { + if entry.Stack != "" && entry.Stack != scope.Stack { + continue + } + if scope.ComponentType != "" && entry.ComponentType != "" && entry.ComponentType != scope.ComponentType { + continue + } + decl, ok := secrets.ExtractDeclarations(entry.Section)[name] + if !ok { + continue + } + if decl.Scope != secrets.ScopeGlobal { + return "", "", componentRequiredForSet(name, "the declaration is not global") + } + // Component-less writes must never select one component's backend address arbitrarily. + // Enumeration normally renders templates per component, so different results are caught by + // the declaration equality check below. Reject an unresolved component template as well so + // an identical raw declaration cannot bypass that guarantee. + if componentDependentReference(decl.Reference) { + return "", "", componentRequiredForSet(name, "the global declaration reference depends on the component context") + } + if selected != nil && decl != *selected { + return "", "", componentRequiredForSet(name, "global declarations differ between components") + } + copy := decl + selected = © + if component == "" { + component, componentType = entry.Component, entry.ComponentType + } + } + if selected == nil { + return "", "", componentRequiredForSet(name, "no global declaration was found in the stack") + } + return component, componentType, nil +} + +func componentDependentReference(reference string) bool { + return strings.Contains(reference, "{{") && strings.Contains(reference, "atmos_component") +} + +func componentRequiredForSet(name, reason string) error { + return errUtils.Build(errUtils.ErrRequiredFlagNotProvided). + WithExplanationf("--component is required to set secret %q: %s", name, reason). + WithHint("Omit --component only for a secret declared with `scope: global`; otherwise specify --component or -c"). + Err() +} + // setSuccessMessage describes where the value was written: shared scopes (stack, global) name the // shared location so the user knows every consumer sees the new value. func setSuccessMessage(svc secretService, scope secretScope, name string) string { @@ -96,13 +176,7 @@ type setTarget struct { // TTY, and falls back to the standard "NAME required" error in non-interactive contexts. func resolveSetName(svc secretService, args []string) (setTarget, error) { if len(args) > 0 { - name, value, hasValue := strings.Cut(args[0], "=") - name = strings.TrimSpace(name) - if name == "" { - return setTarget{}, errUtils.Build(errUtils.ErrRequiredFlagNotProvided). - WithExplanation("secret NAME is required").Err() - } - return setTarget{name: name, value: value, hasValue: hasValue}, nil + return setTargetFromArg(args[0]) } names := declaredNames(svc) @@ -118,6 +192,16 @@ func resolveSetName(svc secretService, args []string) (setTarget, error) { return setTarget{name: chosen}, nil } +func setTargetFromArg(arg string) (setTarget, error) { + name, value, hasValue := strings.Cut(arg, "=") + name = strings.TrimSpace(name) + if name == "" { + return setTarget{}, errUtils.Build(errUtils.ErrRequiredFlagNotProvided). + WithExplanation("secret NAME is required").Err() + } + return setTarget{name: name, value: value, hasValue: hasValue}, nil +} + // declaredNames returns the sorted declared secret names for the service's scope. func declaredNames(svc secretService) []string { decls := svc.Declarations() diff --git a/cmd/secret/set_test.go b/cmd/secret/set_test.go index 9594e17cdb..f9b392fbc6 100644 --- a/cmd/secret/set_test.go +++ b/cmd/secret/set_test.go @@ -80,6 +80,189 @@ func TestRunSecretSet_SharedScopes(t *testing.T) { assert.Equal(t, "GLOBAL", svc2.setCalls[0].name) } +func TestRunSecretSet_GlobalScopeWithoutComponent(t *testing.T) { + svc := newFakeSecretService() + svc.scopes = map[string]secrets.Scope{"SHARED_TOKEN": secrets.ScopeGlobal} + installService(t, svc, nil) + overrideEnumerateScopes(t, []scopeEntry{ + { + Stack: "dev", + Component: "example-service", + ComponentType: "helm", + Section: secretDeclarationSection("SHARED_TOKEN", map[string]any{ + "store": "example-secrets", + "scope": "global", + }), + }, + }, nil) + + err := runSecretSubcommand(t, "set", "SHARED_TOKEN=v1", "--stack", "dev") + require.NoError(t, err) + require.Len(t, svc.setCalls, 1) + assert.Equal(t, "SHARED_TOKEN", svc.setCalls[0].name) + assert.Equal(t, "v1", svc.setCalls[0].value) +} + +func TestRunSecretSet_GlobalScopeWithoutComponentPreservesType(t *testing.T) { + svc := newFakeSecretService() + svc.scopes = map[string]secrets.Scope{"SHARED_TOKEN": secrets.ScopeGlobal} + installService(t, svc, nil) + originalLoadService := loadServiceFn + var loadedScope secretScope + loadServiceFn = func(scope secretScope) (secretService, error) { + loadedScope = scope + return originalLoadService(scope) + } + t.Cleanup(func() { loadServiceFn = originalLoadService }) + overrideEnumerateScopes(t, []scopeEntry{ + { + Stack: "dev", + Component: "example-service", + ComponentType: "helm", + Section: secretDeclarationSection("SHARED_TOKEN", map[string]any{ + "store": "example-secrets", + "scope": "global", + }), + }, + }, nil) + + err := runSecretSubcommand(t, "set", "SHARED_TOKEN=v1", "--stack", "dev", "--type", "helm") + require.NoError(t, err) + require.Len(t, svc.setCalls, 1) + assert.Equal(t, "helm", loadedScope.ComponentType) +} + +func TestFindGlobalSetContext(t *testing.T) { + sharedSection := secretDeclarationSection("SHARED_TOKEN", map[string]any{"store": "example-secrets", "scope": "global"}) + componentReferenceSection := secretDeclarationSection("SHARED_TOKEN", map[string]any{ + "store": "example-secrets", + "scope": "global", + "reference": "op://shared/{{ .atmos_component }}/password", + }) + tests := []struct { + name string + entries []scopeEntry + enumerationErr error + scope secretScope + expectedComponent string + expectedType string + expectedErr error + }{ + { + name: "enumeration error", + enumerationErr: errors.New("stack enumeration failed"), + scope: secretScope{Stack: "dev"}, + expectedErr: errUtils.ErrRequiredFlagNotProvided, + }, + { + name: "no matching declaration", + entries: []scopeEntry{ + { + Stack: "prod", + Component: "other-stack-service", + ComponentType: "helm", + Section: secretDeclarationSection("SHARED_TOKEN", map[string]any{"store": "example-secrets", "scope": "global"}), + }, + { + Stack: "dev", + Component: "other-type-service", + ComponentType: "terraform", + Section: secretDeclarationSection("SHARED_TOKEN", map[string]any{"store": "example-secrets", "scope": "global"}), + }, + { + Stack: "dev", + Component: "example-service", + ComponentType: "helm", + Section: secretDeclarationSection("OTHER_TOKEN", map[string]any{"store": "example-secrets", "scope": "global"}), + }, + }, + scope: secretScope{Stack: "dev", ComponentType: "helm"}, + expectedErr: errUtils.ErrRequiredFlagNotProvided, + }, + { + name: "inconsistent declarations", + entries: []scopeEntry{ + { + Stack: "dev", + Component: "example-service-a", + ComponentType: "helm", + Section: secretDeclarationSection("SHARED_TOKEN", map[string]any{"store": "example-secrets-a", "scope": "global"}), + }, + { + Stack: "dev", + Component: "example-service-b", + ComponentType: "helm", + Section: secretDeclarationSection("SHARED_TOKEN", map[string]any{"store": "example-secrets-b", "scope": "global"}), + }, + }, + scope: secretScope{Stack: "dev"}, + expectedErr: errUtils.ErrRequiredFlagNotProvided, + }, + { + name: "identical component-dependent references require component", + entries: []scopeEntry{ + {Stack: "dev", Component: "example-service-a", ComponentType: "helm", Section: componentReferenceSection}, + {Stack: "dev", Component: "example-service-b", ComponentType: "helm", Section: componentReferenceSection}, + }, + scope: secretScope{Stack: "dev"}, + expectedErr: errUtils.ErrRequiredFlagNotProvided, + }, + { + name: "identical declarations select first component", + entries: []scopeEntry{ + {Stack: "dev", Component: "example-service-a", ComponentType: "helm", Section: sharedSection}, + {Stack: "dev", Component: "example-service-b", ComponentType: "helm", Section: sharedSection}, + }, + scope: secretScope{Stack: "dev"}, + expectedComponent: "example-service-a", + expectedType: "helm", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + overrideEnumerateScopes(t, tt.entries, tt.enumerationErr) + + component, componentType, err := findGlobalSetContext(tt.scope, "SHARED_TOKEN") + if tt.expectedErr != nil { + require.ErrorIs(t, err, tt.expectedErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expectedComponent, component) + assert.Equal(t, tt.expectedType, componentType) + }) + } +} + +func TestRunSecretSet_NonGlobalScopeStillRequiresComponent(t *testing.T) { + svc := newFakeSecretService() + installService(t, svc, nil) + overrideEnumerateScopes(t, []scopeEntry{ + { + Stack: "dev", + Component: "example-service", + ComponentType: "helm", + Section: secretDeclarationSection("API_KEY", map[string]any{ + "store": "example-secrets", + "scope": "instance", + }), + }, + }, nil) + + err := runSecretSubcommand(t, "set", "API_KEY=v1", "--stack", "dev") + require.ErrorIs(t, err, errUtils.ErrRequiredFlagNotProvided) + assert.Empty(t, svc.setCalls) +} + +func secretDeclarationSection(name string, spec map[string]any) map[string]any { + return map[string]any{ + "secrets": map[string]any{ + "vars": map[string]any{name: spec}, + }, + } +} + func TestRunSecretSet_Prompt(t *testing.T) { svc := newFakeSecretService() installService(t, svc, nil) @@ -115,6 +298,15 @@ func TestRunSecretSet_EmptyName(t *testing.T) { assert.Empty(t, svc.setCalls) } +func TestRunSecretSet_EmptyNameWithoutComponent(t *testing.T) { + svc := newFakeSecretService() + installService(t, svc, nil) + + err := runSecretSubcommand(t, "set", "=v1", "--stack", "dev") + require.ErrorIs(t, err, errUtils.ErrRequiredFlagNotProvided) + assert.Empty(t, svc.setCalls) +} + func TestRunSecretSet_SetError(t *testing.T) { svc := newFakeSecretService() svc.setErr = errors.New("backend write failed") diff --git a/cmd/secret/shared.go b/cmd/secret/shared.go index f3c10bc562..bbbbbfceef 100644 --- a/cmd/secret/shared.go +++ b/cmd/secret/shared.go @@ -74,6 +74,17 @@ func parseFacets(cmd *cobra.Command) (secretScope, error) { // non-interactive shells). In a non-interactive context a missing flag falls back to the standard // "required flag not provided" error, preserving today's pipeline behavior. func parseScope(cmd *cobra.Command, args []string) (secretScope, error) { + scope, err := parseScopeStack(cmd, args) + if err != nil { + return scope, err + } + return requireScopeComponent(scope, cmd, args) +} + +// parseScopeStack resolves the common facets and requires only a stack. Commands that can prove +// a component is irrelevant (for example, setting a uniquely global secret) use this narrower +// helper and discover a declaration-bearing component context afterward. +func parseScopeStack(cmd *cobra.Command, args []string) (secretScope, error) { v := viper.GetViper() if err := secretParser.BindFlagsToViper(cmd, v); err != nil { return secretScope{}, err @@ -100,7 +111,10 @@ func parseScope(cmd *cobra.Command, args []string) (secretScope, error) { } // Make the chosen stack visible to the component completion (it filters by --stack). v.Set(cfg.StackStr, scope.Stack) + return scope, nil +} +func requireScopeComponent(scope secretScope, cmd *cobra.Command, args []string) (secretScope, error) { if scope.Component == "" { chosen, err := flags.PromptForMissingRequired("component", "Choose a component", componentCompletion, cmd, args) if err != nil { diff --git a/pkg/component/helm/secret_values_integration_test.go b/pkg/component/helm/secret_values_integration_test.go new file mode 100644 index 0000000000..3cfe3360b9 --- /dev/null +++ b/pkg/component/helm/secret_values_integration_test.go @@ -0,0 +1,171 @@ +package helm + +import ( + "context" + "encoding/json" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + e "github.com/cloudposse/atmos/internal/exec" + "github.com/cloudposse/atmos/pkg/auth" + cfg "github.com/cloudposse/atmos/pkg/config" + iolib "github.com/cloudposse/atmos/pkg/io" + "github.com/cloudposse/atmos/pkg/keyring" + "github.com/cloudposse/atmos/pkg/manifest" + "github.com/cloudposse/atmos/pkg/schema" + "github.com/cloudposse/atmos/pkg/store" + storeproviders "github.com/cloudposse/atmos/pkg/store/providers" +) + +const ( + serviceAccountJSON = `{"type":"service_account","private_key":"-----BEGIN PRIVATE KEY-----\nAAAA\nBBBB\n-----END PRIVATE KEY-----\n","client_email":"service@example.iam.gserviceaccount.com"}` + plainSecret = "single-line-example-token" + privateKey = "-----BEGIN PRIVATE KEY-----\nAAAA\nBBBB\n-----END PRIVATE KEY-----\n" +) + +// jsonDecodingStore reproduces the structured Get behavior of cloud secret stores while using +// the in-memory keychain backend. GetRaw preserves the opaque payload for !secret ... | raw. +type jsonDecodingStore struct { + store.Store +} + +func (s *jsonDecodingStore) Get(stack, component, key string) (any, error) { + raw, err := s.GetRaw(stack, component, key) + if err != nil { + return nil, err + } + var decoded any + if err := json.Unmarshal([]byte(raw), &decoded); err == nil { + return decoded, nil + } + return raw, nil +} + +func (s *jsonDecodingStore) GetRaw(stack, component, key string) (string, error) { + return s.Store.(store.RawStore).GetRaw(stack, component, key) +} + +func TestNativeHelmSecretRawAndStructuredValuesMaskIndentedMultilineValues(t *testing.T) { + fixture, atmosConfig, info := prepareHelmSecretValuesFixture(t) + assertResolvedHelmSecretValues(t, info) + rendered, renderedEnv := renderHelmSecretValuesFixture(t, fixture, atmosConfig, info) + assert.Equal(t, serviceAccountJSON, envValue(t, renderedEnv, "SERVICE_ACCOUNT_JSON"), "Helm env.value must remain a scalar string") + assertMaskedHelmSecretValues(t, rendered) +} + +func prepareHelmSecretValuesFixture(t *testing.T) (string, *schema.AtmosConfiguration, schema.ConfigAndStacksInfo) { + t.Helper() + + fixture, err := filepath.Abs(filepath.Join("..", "..", "..", "tests", "fixtures", "scenarios", "helm-secret-values")) + require.NoError(t, err) + t.Chdir(fixture) + t.Setenv("ATMOS_CLI_CONFIG_PATH", ".") + t.Setenv("ATMOS_BASE_PATH", ".") + + info := schema.ConfigAndStacksInfo{ + ComponentFromArg: "secret-values", + ComponentType: cfg.HelmComponentType, + Stack: "dev", + SubCommand: "template", + SecretsMaskOnly: true, + } + atmosConfig, err := cfg.InitCliConfig(info, true) + require.NoError(t, err) + require.Equal(t, store.KindGCPSecret, atmosConfig.StoresConfig["gcp-secrets"].Kind) + + memoryStore, err := storeproviders.NewKeychainStore(&storeproviders.KeychainStoreOptions{Backend: keyring.TypeMemory}) + require.NoError(t, err) + offlineStore := &jsonDecodingStore{Store: memoryStore} + atmosConfig.Stores["gcp-secrets"] = offlineStore + + // Top-level declarations are stack scoped, so the component segment is intentionally empty. + require.NoError(t, offlineStore.Set("dev", "", "service-account-json", serviceAccountJSON)) + require.NoError(t, offlineStore.Set("dev", "", "plain-token", plainSecret)) + require.NoError(t, offlineStore.Set("dev", "", "signing-key", privateKey)) + + info.SecretsMaskOnly = false + info, err = e.ProcessStacks(&atmosConfig, info, true, true, true, nil, auth.AuthManager(nil)) + require.NoError(t, err) + return fixture, &atmosConfig, info +} + +func assertResolvedHelmSecretValues(t *testing.T, info schema.ConfigAndStacksInfo) { + t.Helper() + + values, ok := info.ComponentSection[cfg.ValuesSectionName].(map[string]any) + require.True(t, ok) + structured, ok := values["structured_service_account"].(map[string]any) + require.True(t, ok, "bare !secret must preserve the existing structured store contract") + assert.Equal(t, "service_account", structured["type"]) + env, ok := values["env"].([]any) + require.True(t, ok) + require.Len(t, env, 4) + assert.Equal(t, serviceAccountJSON, envValue(t, env, "SERVICE_ACCOUNT_JSON")) + assert.Equal(t, "service@example.iam.gserviceaccount.com", envValue(t, env, "CLIENT_EMAIL")) + assert.Equal(t, plainSecret, envValue(t, env, "PLAIN_TOKEN")) + assert.Equal(t, privateKey, envValue(t, env, "SIGNING_KEY")) +} + +func renderHelmSecretValuesFixture( + t *testing.T, + fixture string, + atmosConfig *schema.AtmosConfiguration, + info schema.ConfigAndStacksInfo, +) (string, []any) { + t.Helper() + + componentPath := filepath.Join(fixture, "components", "helm", "secret-values") + spec, err := buildChartSpec(atmosConfig, &info, componentPath) + require.NoError(t, err) + rendered, err := renderManifest(context.Background(), spec) + require.NoError(t, err) + + objects, err := manifest.DecodeObjects([]byte(rendered)) + require.NoError(t, err) + require.Len(t, objects, 1) + containers, found, err := unstructured.NestedSlice(objects[0].Object, "spec", "template", "spec", "containers") + require.NoError(t, err) + require.True(t, found) + require.NotEmpty(t, containers) + require.IsType(t, map[string]any{}, containers[0]) + container := containers[0].(map[string]any) + require.IsType(t, []any{}, container["env"]) + return rendered, container["env"].([]any) +} + +func assertMaskedHelmSecretValues(t *testing.T, rendered string) { + t.Helper() + + masker := iolib.GetContext().Masker() + t.Cleanup(func() { + masker.Clear() + masker.SetEnabled(true) + }) + masked := masker.Mask(rendered) + assert.NotContains(t, masked, "BEGIN PRIVATE KEY") + assert.NotContains(t, masked, "AAAA") + assert.NotContains(t, masked, "BBBB") + assert.NotContains(t, masked, plainSecret) + assert.Contains(t, masked, masker.Replacement()) + + masker.SetEnabled(false) + unmasked := masker.Mask(rendered) + assert.Contains(t, unmasked, "BEGIN PRIVATE KEY") + assert.Contains(t, unmasked, plainSecret) +} + +func envValue(t *testing.T, env []any, name string) any { + t.Helper() + for _, item := range env { + entry, ok := item.(map[string]any) + if ok && entry["name"] == name { + return entry["value"] + } + } + t.Fatalf("environment entry %q not found", name) + return nil +} diff --git a/pkg/function/parser/parser.go b/pkg/function/parser/parser.go index 23f564bcb2..fe47a481df 100644 --- a/pkg/function/parser/parser.go +++ b/pkg/function/parser/parser.go @@ -69,6 +69,14 @@ type StoreGetArgs struct { Query string } +// SecretArgs contains a declared secret name and its optional modifiers. +type SecretArgs struct { + Name string + Path string + Raw bool + Default *string +} + const ( tokenWhitespace = "Whitespace" tokenQuoted = "Quoted" @@ -329,6 +337,62 @@ func ParseStoreGet(input string) (StoreGetArgs, error) { return StoreGetArgs{Store: words[0], Key: words[1], Default: options.defaultValue, Query: options.query}, nil } +// ParseSecret parses `name [| path expression | raw] [| default value]`. +func ParseSecret(input string) (SecretArgs, error) { + hasPath := false + tokens, err := tokenize(input) + if err != nil { + return SecretArgs{}, err + } + if len(tokens) == 0 { + return SecretArgs{}, emptyError() + } + if tokens[0].typeName == tokenPipe { + return SecretArgs{}, parseError(tokens[0], "secret name must not be empty") + } + + result := SecretArgs{Name: unquote(tokens[0].value)} + for index := 1; index < len(tokens); { + if tokens[index].typeName != tokenPipe || index+1 >= len(tokens) { + return SecretArgs{}, parseError(tokens[index], "expected option delimiter") + } + keyword := unquote(tokens[index+1].value) + switch keyword { + case "raw": + result.Raw = true + index += 2 + case "path": + hasPath = true + if index+2 >= len(tokens) || tokens[index+2].typeName == tokenPipe { + return SecretArgs{}, parseError(tokens[index+1], "expected option value") + } + next := index + 3 + for next < len(tokens) && tokens[next].typeName != tokenPipe { + next++ + } + end := len(input) + if next < len(tokens) { + end = tokens[next].position.Offset + } + result.Path = unquote(strings.TrimSpace(input[tokens[index+2].position.Offset:end])) + index = next + case "default": + if index+2 >= len(tokens) || tokens[index+2].typeName == tokenPipe { + return SecretArgs{}, parseError(tokens[index+1], "expected option value") + } + value := unquote(tokens[index+2].value) + result.Default = &value + index += 3 + default: + return SecretArgs{}, parseError(tokens[index+1], "expected path, raw, or default option") + } + } + if result.Raw && hasPath { + return SecretArgs{}, &Error{Position: Position{Line: 1, Column: 1}, Message: "raw and path options are mutually exclusive"} + } + return result, nil +} + func words(input string) ([]string, error) { tokens, err := tokenize(input) if err != nil { diff --git a/pkg/function/parser/parser_test.go b/pkg/function/parser/parser_test.go index 005df7e0e7..80a4d49462 100644 --- a/pkg/function/parser/parser_test.go +++ b/pkg/function/parser/parser_test.go @@ -193,6 +193,51 @@ func TestParseStoreGet(t *testing.T) { require.Error(t, err) } +func TestParseSecret(t *testing.T) { + tests := []struct { + name string + input string + expected SecretArgs + wantErr bool + }{ + { + name: "path with default", + input: `SERVICE_CONFIG | path ".credentials.token" | default "not set"`, + expected: SecretArgs{Name: "SERVICE_CONFIG", Path: ".credentials.token", Default: stringPtr("not set")}, + }, + { + name: "path expression with spaces", + input: `SERVICE_CONFIG | path .credentials.token // "not set"`, + expected: SecretArgs{Name: "SERVICE_CONFIG", Path: `.credentials.token // "not set"`}, + }, + { + name: "compact raw with empty default", + input: `SERVICE_CREDENTIALS |raw | default ""`, + expected: SecretArgs{Name: "SERVICE_CREDENTIALS", Raw: true, Default: stringPtr("")}, + }, + {name: "empty input", input: "", wantErr: true}, + {name: "empty name", input: "| raw", wantErr: true}, + {name: "missing delimiter", input: "SERVICE_CONFIG raw", wantErr: true}, + {name: "raw with value", input: "SERVICE_CONFIG | raw value", wantErr: true}, + {name: "path without value", input: "SERVICE_CONFIG | path", wantErr: true}, + {name: "unknown option", input: "SERVICE_CONFIG | unknown value", wantErr: true}, + {name: "raw path conflict", input: `SERVICE_CONFIG | raw | path ".token"`, wantErr: true}, + {name: "empty path conflicts with raw", input: `SERVICE_CONFIG | path "" | raw`, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actual, err := ParseSecret(tt.input) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expected, actual) + }) + } +} + func TestParseStoreRejectsMissingOptionValue(t *testing.T) { _, err := ParseStore("ssm vpc id | default") require.Error(t, err) diff --git a/pkg/function/secret.go b/pkg/function/secret.go index f647cd5e84..8a7cb6f32f 100644 --- a/pkg/function/secret.go +++ b/pkg/function/secret.go @@ -34,6 +34,7 @@ func NewSecretFunction() *SecretFunction { // // !secret NAME - Resolve a declared secret value. // !secret NAME | path ".a.b" - Extract a nested value from a structured secret. +// !secret NAME | raw - Retrieve the original textual payload. // !secret NAME | default "dev-key" - Fall back to a default when the secret is missing. func (f *SecretFunction) Execute(ctx context.Context, args string, execCtx *ExecutionContext) (any, error) { defer perf.Track(nil, "function.SecretFunction.Execute")() diff --git a/pkg/io/global.go b/pkg/io/global.go index 1c398ea655..88c48b9451 100644 --- a/pkg/io/global.go +++ b/pkg/io/global.go @@ -1,6 +1,7 @@ package io import ( + "encoding/json" "fmt" stdio "io" "os" @@ -395,6 +396,13 @@ func RegisterSecretValue(v any) { return case string: RegisterSecret(t) + var structured any + if json.Unmarshal([]byte(t), &structured) == nil { + switch structured.(type) { + case map[string]any, []any: + RegisterSecretValue(structured) + } + } case map[string]any: for _, child := range t { RegisterSecretValue(child) diff --git a/pkg/secrets/providers/provider.go b/pkg/secrets/providers/provider.go index 272fe9e55d..e0000a8445 100644 --- a/pkg/secrets/providers/provider.go +++ b/pkg/secrets/providers/provider.go @@ -54,6 +54,12 @@ type Provider interface { SupportsScope(scope Scope) bool } +// RawGetter is an optional capability for providers that can retrieve the original textual +// secret without structured decoding. `!secret ... | raw` explicitly requests this capability. +type RawGetter interface { + GetRaw(coord Coordinate) (string, error) +} + // Provider-construction errors. var ( // ErrStoreNotFound indicates the referenced store is not configured. @@ -64,6 +70,8 @@ var ( ErrProviderNotFound = errors.New("referenced secrets provider is not configured") // ErrDeleteNotSupported indicates the backend cannot delete values. ErrDeleteNotSupported = errors.New("backend does not support delete") + // ErrRawNotSupported indicates the backend cannot return an original textual payload. + ErrRawNotSupported = errors.New("backend does not support raw secret retrieval") // ErrKeygenNotSupported indicates a provider implements the keygen capability but cannot // generate for this particular vault/kind (e.g. a KMS/GPG-backed SOPS vault). Callers should // surface it as a friendly "not implemented" message, not a hard failure. diff --git a/pkg/secrets/providers/store.go b/pkg/secrets/providers/store.go index 77f84e2d2b..467cc9e5b8 100644 --- a/pkg/secrets/providers/store.go +++ b/pkg/secrets/providers/store.go @@ -76,6 +76,23 @@ func (p *storeProvider) Get(coord Coordinate) (any, error) { return p.store.Get(coord.Stack, coord.Component, coord.Key) } +func (p *storeProvider) GetRaw(coord Coordinate) (string, error) { + defer perf.Track(nil, "providers.storeProvider.GetRaw")() + + if rawStore, ok := p.store.(store.RawStore); ok { + return rawStore.GetRaw(coord.Stack, coord.Component, coord.Key) + } + value, err := p.store.Get(coord.Stack, coord.Component, coord.Key) + if err != nil { + return "", err + } + text, ok := value.(string) + if !ok { + return "", fmt.Errorf("%w: store %q (%s)", ErrRawNotSupported, p.name, p.kind) + } + return text, nil +} + func (p *storeProvider) Delete(coord Coordinate) error { defer perf.Track(nil, "providers.storeProvider.Delete")() diff --git a/pkg/secrets/providers/store_test.go b/pkg/secrets/providers/store_test.go index 9d2a7bb16f..25f45dea0a 100644 --- a/pkg/secrets/providers/store_test.go +++ b/pkg/secrets/providers/store_test.go @@ -103,6 +103,59 @@ func TestStoreProvider_SetGet(t *testing.T) { assert.Equal(t, "v1", got) } +func TestStoreProvider_GetRawFallsBackForTextOnly(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT().Get("prod", "api", "API_KEY").Return("v1", nil) + p := &storeProvider{name: "app", kind: "example/text", store: mockStore} + + got, err := p.GetRaw(Coordinate{Stack: "prod", Component: "api", Key: "API_KEY"}) + require.NoError(t, err) + assert.Equal(t, "v1", got) +} + +func TestStoreProvider_GetRawRejectsStructuredFallback(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT().Get("prod", "api", "CONFIG").Return(map[string]any{"enabled": true}, nil) + p := &storeProvider{name: "app", kind: "example/structured", store: mockStore} + + _, err := p.GetRaw(Coordinate{Stack: "prod", Component: "api", Key: "CONFIG"}) + require.ErrorIs(t, err, ErrRawNotSupported) +} + +type rawFakeStore struct { + rawPayload string + getCalls int + rawCalls int +} + +func (s *rawFakeStore) Set(_, _, _ string, _ any) error { return nil } +func (s *rawFakeStore) Get(_, _, _ string) (any, error) { + s.getCalls++ + return "decoded", nil +} +func (s *rawFakeStore) GetKey(_ string) (any, error) { return nil, nil } +func (s *rawFakeStore) GetRaw(_, _, _ string) (string, error) { + s.rawCalls++ + return s.rawPayload, nil +} + +func TestStoreProvider_GetRawDelegatesToNativeRawStore(t *testing.T) { + rawStore := &rawFakeStore{rawPayload: `{"enabled":true}`} + p := &storeProvider{name: "app", kind: "example/raw", store: rawStore} + + got, err := p.GetRaw(Coordinate{Stack: "prod", Component: "api", Key: "CONFIG"}) + require.NoError(t, err) + assert.Equal(t, `{"enabled":true}`, got) + assert.Equal(t, 1, rawStore.rawCalls) + assert.Zero(t, rawStore.getCalls) +} + func TestStoreProvider_DeleteUnsupported(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() diff --git a/pkg/secrets/resolver.go b/pkg/secrets/resolver.go index 945ce497aa..f8c4629fa8 100644 --- a/pkg/secrets/resolver.go +++ b/pkg/secrets/resolver.go @@ -1,9 +1,11 @@ package secrets import ( + "errors" "fmt" "strings" + fnparser "github.com/cloudposse/atmos/pkg/function/parser" "github.com/cloudposse/atmos/pkg/io" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/schema" @@ -14,7 +16,7 @@ import ( // secretTag is the YAML tag (with leading bang) for the secret function. const secretTag = "!secret" -// Resolve resolves a `!secret NAME [| path ...] [| default ...]` expression to a value. +// Resolve resolves a `!secret NAME [| path ...] [| raw] [| default ...]` expression to a value. // // Behavior (in order): // 1. It validates that the secret is declared in the component section. @@ -64,9 +66,27 @@ func Resolve(atmosConfig *schema.AtmosConfiguration, input, currentStack string, func retrieveAndMask(atmosConfig *schema.AtmosConfiguration, provider providers.Provider, coord providers.Coordinate, name string, opts ResolveOptions) (any, error) { defer perf.Track(atmosConfig, "secrets.retrieveAndMask")() - value, err := provider.Get(coord) + var value any + var err error + if opts.Raw { + if rawProvider, ok := provider.(providers.RawGetter); ok { + value, err = rawProvider.GetRaw(coord) + } else { + value, err = provider.Get(coord) + if err == nil { + if _, ok := value.(string); !ok { + err = providers.ErrRawNotSupported + } + } + } + } else { + value, err = provider.Get(coord) + } if err != nil { - if opts.Default != nil { + // A default replaces a missing value, not an unsupported retrieval capability. In + // particular, `raw | default` must not silently turn a structured-only backend into a + // successful lookup. + if opts.Default != nil && !errors.Is(err, providers.ErrRawNotSupported) { return *opts.Default, nil } return nil, fmt.Errorf("%w: %q: %w", ErrSecretMissing, name, err) @@ -99,37 +119,18 @@ func componentName(stackInfo *schema.ConfigAndStacksInfo) string { return stackInfo.FinalComponent } -// parseSecretArgs parses `!secret NAME [| path "x"] [| default "y"]` (or the same without the -// leading tag) into the secret name and modifiers. +// parseSecretArgs parses `!secret NAME [| path "x"] [| raw] [| default "y"]` (or the same +// without the leading tag) into the secret name and modifiers. func parseSecretArgs(input string) (string, ResolveOptions, error) { defer perf.Track(nil, "secrets.parseSecretArgs")() s := strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(input), secretTag)) - - parts := strings.Split(s, "|") - name := strings.TrimSpace(parts[0]) - if name == "" { - return "", ResolveOptions{}, ErrEmptyName - } - - opts := ResolveOptions{} - for _, p := range parts[1:] { - segs := strings.SplitN(strings.TrimSpace(p), " ", 2) - if len(segs) != 2 { - return "", ResolveOptions{}, fmt.Errorf("%w: invalid modifier %q", ErrInvalidSecretArgs, p) - } - key := strings.Trim(segs[0], `"'`) - val := strings.Trim(strings.TrimSpace(segs[1]), `"'`) - switch key { - case "path": - opts.Path = val - case "default": - v := val - opts.Default = &v - default: - return "", ResolveOptions{}, fmt.Errorf("%w: unknown modifier %q", ErrInvalidSecretArgs, key) + parsed, err := fnparser.ParseSecret(s) + if err != nil { + if strings.TrimSpace(s) == "" { + return "", ResolveOptions{}, ErrEmptyName } + return "", ResolveOptions{}, fmt.Errorf("%w: %w", ErrInvalidSecretArgs, err) } - - return name, opts, nil + return parsed.Name, ResolveOptions{Path: parsed.Path, Raw: parsed.Raw, Default: parsed.Default}, nil } diff --git a/pkg/secrets/resolver_test.go b/pkg/secrets/resolver_test.go index 739cc47a67..c26d4ca705 100644 --- a/pkg/secrets/resolver_test.go +++ b/pkg/secrets/resolver_test.go @@ -9,6 +9,7 @@ import ( iolib "github.com/cloudposse/atmos/pkg/io" "github.com/cloudposse/atmos/pkg/schema" + "github.com/cloudposse/atmos/pkg/secrets/providers" "github.com/cloudposse/atmos/pkg/store" ) @@ -150,6 +151,49 @@ func TestResolve_DefaultOnMissing(t *testing.T) { assert.Equal(t, "dev-key", got) } +func TestResolve_RawDefaultDoesNotHideUnsupportedCapability(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT(). + Get("prod", "api", "DATADOG_API_KEY"). + Return(map[string]any{"enabled": true}, nil). + Times(1) + + cfg, componentSection := newSecretTestConfig(mockStore) + info := &schema.ConfigAndStacksInfo{ + Stack: "prod", + Component: "api", + ComponentSection: componentSection, + } + + _, err := Resolve(cfg, `!secret DATADOG_API_KEY | raw | default "fallback"`, "prod", info) + require.ErrorIs(t, err, providers.ErrRawNotSupported) +} + +func TestResolve_RawDefaultUsesFallbackOnMissing(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT(). + Get("prod", "api", "DATADOG_API_KEY"). + Return(nil, store.ErrResourceNotFound). + Times(1) + + cfg, componentSection := newSecretTestConfig(mockStore) + info := &schema.ConfigAndStacksInfo{ + Stack: "prod", + Component: "api", + ComponentSection: componentSection, + } + + got, err := Resolve(cfg, `!secret DATADOG_API_KEY | raw | default "fallback"`, "prod", info) + require.NoError(t, err) + assert.Equal(t, "fallback", got) +} + // assertErr is a trivial error used to simulate a backend miss. type assertErr struct{} @@ -157,13 +201,38 @@ func (assertErr) Error() string { return "not found" } // TestParseSecretArgs covers name + modifier parsing. func TestParseSecretArgs(t *testing.T) { - name, opts, err := parseSecretArgs(`!secret DB_CONFIG | path ".host" | default "localhost"`) - require.NoError(t, err) - assert.Equal(t, "DB_CONFIG", name) - assert.Equal(t, ".host", opts.Path) - require.NotNil(t, opts.Default) - assert.Equal(t, "localhost", *opts.Default) + defaultValue := "localhost" + tests := []struct { + name string + input string + expectedName string + expectedRaw bool + expectedDefault *string + expectedErr error + }{ + { + name: "raw with default", + input: `!secret DB_CONFIG | raw | default "localhost"`, + expectedName: "DB_CONFIG", + expectedRaw: true, + expectedDefault: &defaultValue, + }, + {name: "compact raw", input: `!secret DB_CONFIG |raw`, expectedName: "DB_CONFIG", expectedRaw: true}, + {name: "raw path conflict", input: `!secret DB_CONFIG | raw | path ".host"`, expectedErr: ErrInvalidSecretArgs}, + {name: "empty name", input: "!secret ", expectedErr: ErrEmptyName}, + } - _, _, err = parseSecretArgs("!secret ") - require.ErrorIs(t, err, ErrEmptyName) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + name, opts, err := parseSecretArgs(tt.input) + if tt.expectedErr != nil { + require.ErrorIs(t, err, tt.expectedErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expectedName, name) + assert.Equal(t, tt.expectedRaw, opts.Raw) + assert.Equal(t, tt.expectedDefault, opts.Default) + }) + } } diff --git a/pkg/secrets/scope_test.go b/pkg/secrets/scope_test.go index 2f457373c6..af15852573 100644 --- a/pkg/secrets/scope_test.go +++ b/pkg/secrets/scope_test.go @@ -38,6 +38,20 @@ func TestCoordinateForDeclaration_Scope(t *testing.T) { coordOther := coordinateForDeclaration(globalDecl, "dev", "web") assert.Equal(t, coord, coordOther, "every resolving scope must converge on the same global coordinate") + + // Provider-specific reference templates receive the resolved coordinate, not the original + // component context. Global scope therefore cannot produce a different backend key per + // component even when a declaration contains an atmos_component expression. + globalReference := &Declaration{ + Name: "SHARED_CLIENT_SECRET", + Reference: "op://shared/{{ .atmos_component }}/password", + Scope: ScopeGlobal, + } + referenceCoord := coordinateForDeclaration(globalReference, "prod", "api") + referenceCoordOther := coordinateForDeclaration(globalReference, "dev", "web") + assert.Equal(t, referenceCoord, referenceCoordOther) + assert.Empty(t, referenceCoord.Stack) + assert.Empty(t, referenceCoord.Component) } // TestTagScope_StampsAndResolvesOverride proves position-derived scope tagging plus the standard diff --git a/pkg/secrets/types.go b/pkg/secrets/types.go index fd047b67ed..71f2b2f419 100644 --- a/pkg/secrets/types.go +++ b/pkg/secrets/types.go @@ -77,6 +77,8 @@ type Status struct { type ResolveOptions struct { // Path is an optional YQ-style path expression applied to a structured secret value. Path string + // Raw requests the original textual payload instead of the store's structured value. + Raw bool // Default is an optional fallback value used when the secret is missing. Default *string } diff --git a/pkg/store/providers/aws_secrets_manager_store.go b/pkg/store/providers/aws_secrets_manager_store.go index 0da44b2288..b1bee4d631 100644 --- a/pkg/store/providers/aws_secrets_manager_store.go +++ b/pkg/store/providers/aws_secrets_manager_store.go @@ -13,6 +13,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/secretsmanager" smtypes "github.com/aws/aws-sdk-go-v2/service/secretsmanager/types" + "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/store" ) @@ -25,6 +26,7 @@ type SecretsManagerStore struct { stackDelimiter *string region string endpoint string + secret bool // Identity-based authentication fields. identityName string @@ -59,6 +61,7 @@ var ( _ store.DeletableStore = (*SecretsManagerStore)(nil) _ store.StatusStore = (*SecretsManagerStore)(nil) _ store.ListableStore = (*SecretsManagerStore)(nil) + _ store.SecretAwareStore = (*SecretsManagerStore)(nil) ) func init() { @@ -214,7 +217,7 @@ func (s *SecretsManagerStore) Set(stack string, component string, key string, va ctx := context.TODO() - jsonValue, err := marshalSecretsManagerValue(value) + jsonValue, err := marshalSecretsManagerValue(value, s.secret) if err != nil { return fmt.Errorf(errWrapFormat, store.ErrSerializeJSON, err) } @@ -232,8 +235,11 @@ func (s *SecretsManagerStore) Set(stack string, component string, key string, va // A string that already holds valid JSON object or array is passed through verbatim // to avoid double-encoding it as a quoted JSON string; everything else is marshaled // to JSON. -func marshalSecretsManagerValue(value any) ([]byte, error) { +func marshalSecretsManagerValue(value any, rawStrings bool) ([]byte, error) { if str, ok := value.(string); ok { + if rawStrings { + return []byte(str), nil + } trimmed := strings.TrimSpace(str) if len(trimmed) > 0 && (trimmed[0] == '{' || trimmed[0] == '[') && json.Valid([]byte(trimmed)) { return []byte(trimmed), nil @@ -242,6 +248,12 @@ func marshalSecretsManagerValue(value any) ([]byte, error) { return json.Marshal(value) } +// SetSecret implements SecretAwareStore. Secret string values are written verbatim so +// `atmos secret set` round-trips through `!secret ... | raw`; structured values remain JSON. +func (s *SecretsManagerStore) SetSecret(secret bool) { + s.secret = secret +} + // putOrCreate updates an existing secret value, creating the secret if it does not yet exist. func (s *SecretsManagerStore) putOrCreate(ctx context.Context, secretID, strValue string) error { _, err := s.client.PutSecretValue(ctx, &secretsmanager.PutSecretValueInput{ @@ -270,20 +282,36 @@ func (s *SecretsManagerStore) putOrCreate(ctx context.Context, secretID, strValu // Get retrieves a value for an Atmos component in a stack. An empty stack and/or component is // permitted: scoped secret coordinates (stack/global scope) omit those path segments. func (s *SecretsManagerStore) Get(stack string, component string, key string) (any, error) { + raw, err := s.GetRaw(stack, component, key) + if err != nil { + return nil, err + } + var result any + //nolint:nilerr // Non-JSON secrets are returned as the raw string. + if err := json.Unmarshal([]byte(raw), &result); err != nil { + return raw, nil + } + return result, nil +} + +// GetRaw retrieves the original Secrets Manager string without JSON decoding. +func (s *SecretsManagerStore) GetRaw(stack string, component string, key string) (string, error) { + defer perf.Track(nil, "providers.SecretsManagerStore.GetRaw")() + if key == "" { - return nil, store.ErrEmptyKey + return "", store.ErrEmptyKey } if err := s.ensureClient(); err != nil { - return nil, err + return "", err } secretID, err := s.getKey(stack, component, key) if err != nil { - return nil, fmt.Errorf(errWrapFormat, store.ErrGetKey, err) + return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err) } - return s.getByID(secretID) + return s.getRawByID(secretID) } // GetKey retrieves a value by its raw secret id (optionally prefixed). @@ -303,24 +331,31 @@ func (s *SecretsManagerStore) GetKey(key string) (any, error) { } func (s *SecretsManagerStore) getByID(secretID string) (any, error) { + raw, err := s.getRawByID(secretID) + if err != nil { + return nil, err + } + var result any + //nolint:nilerr // Non-JSON secrets are returned as the raw string. + if err := json.Unmarshal([]byte(raw), &result); err != nil { + return raw, nil + } + return result, nil +} + +func (s *SecretsManagerStore) getRawByID(secretID string) (string, error) { ctx := context.TODO() output, err := s.client.GetSecretValue(ctx, &secretsmanager.GetSecretValueInput{ SecretId: aws.String(secretID), }) if err != nil { // Use %w for the underlying error so callers (e.g. Has) can detect ResourceNotFound. - return nil, fmt.Errorf("%w '%s': %w", store.ErrGetSecret, secretID, err) + return "", fmt.Errorf("%w '%s': %w", store.ErrGetSecret, secretID, err) } if output.SecretString == nil { - return nil, fmt.Errorf("%w '%s': empty secret string", store.ErrGetSecret, secretID) + return "", fmt.Errorf("%w '%s': empty secret string", store.ErrGetSecret, secretID) } - - var result any - //nolint:nilerr // Non-JSON secrets are returned as the raw string. - if err := json.Unmarshal([]byte(*output.SecretString), &result); err != nil { - return *output.SecretString, nil - } - return result, nil + return *output.SecretString, nil } // Delete removes a secret (with no recovery window so the name can be reused immediately). diff --git a/pkg/store/providers/aws_secrets_manager_store_endpoint_test.go b/pkg/store/providers/aws_secrets_manager_store_endpoint_test.go index a131fb542d..b20887e591 100644 --- a/pkg/store/providers/aws_secrets_manager_store_endpoint_test.go +++ b/pkg/store/providers/aws_secrets_manager_store_endpoint_test.go @@ -14,9 +14,10 @@ var errTestSetAuth = errors.New("primed init error") func TestMarshalSecretsManagerValue(t *testing.T) { tests := []struct { - name string - value any - want string + name string + value any + secret bool + want string }{ { name: "json object string passes through verbatim", @@ -33,6 +34,12 @@ func TestMarshalSecretsManagerValue(t *testing.T) { value: " {\"key\":\"value\"}\n", want: `{"key":"value"}`, }, + { + name: "secret string is stored verbatim", + value: "hello", + secret: true, + want: `hello`, + }, { name: "invalid json string is encoded as quoted json", value: `{not valid json`, @@ -57,7 +64,7 @@ func TestMarshalSecretsManagerValue(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := marshalSecretsManagerValue(tt.value) + got, err := marshalSecretsManagerValue(tt.value, tt.secret) require.NoError(t, err) assert.Equal(t, tt.want, string(got)) }) diff --git a/pkg/store/providers/aws_ssm_param_store.go b/pkg/store/providers/aws_ssm_param_store.go index 074368aacf..d14e6a0183 100644 --- a/pkg/store/providers/aws_ssm_param_store.go +++ b/pkg/store/providers/aws_ssm_param_store.go @@ -363,7 +363,7 @@ func (s *SSMStore) Set(stack string, component string, key string, value any) er return err } - // Construct the full parameter name using getKey + // Construct the full parameter name using getKey. paramName, err := s.getKey(stack, component, key) if err != nil { return fmt.Errorf(errWrapFormat, store.ErrGetKey, err) @@ -404,12 +404,21 @@ func (s *SSMStore) Set(stack string, component string, key string, value any) er // An empty stack and/or component is permitted: scoped secret coordinates (stack/global scope) // omit those path segments. func (s *SSMStore) Get(stack string, component string, key string) (any, error) { + raw, err := s.GetRaw(stack, component, key) + if err != nil { + return nil, err + } + return s.decodeParameterValue(raw), nil +} + +// GetRaw retrieves the exact decrypted parameter string without JSON decoding. +func (s *SSMStore) GetRaw(stack string, component string, key string) (string, error) { if key == "" { - return nil, store.ErrEmptyKey + return "", store.ErrEmptyKey } if err := s.ensureClient(); err != nil { - return nil, err + return "", err } ctx := context.TODO() @@ -417,13 +426,13 @@ func (s *SSMStore) Get(stack string, component string, key string) (any, error) // Construct the full parameter name using getKey paramName, err := s.getKey(stack, component, key) if err != nil { - return nil, fmt.Errorf(errWrapFormat, store.ErrGetKey, err) + return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err) } - // Assume the read role if specified + // Assume the read role if specified. cfg, err := s.assumeRole(ctx, s.readRoleArn) if err != nil { - return nil, fmt.Errorf(errWrapFormat, store.ErrAssumeRole, err) + return "", fmt.Errorf(errWrapFormat, store.ErrAssumeRole, err) } // Use the same client if no role was assumed @@ -443,10 +452,10 @@ func (s *SSMStore) Get(stack string, component string, key string) (any, error) WithDecryption: aws.Bool(true), }) if err != nil { - return nil, fmt.Errorf(errWrapFormatWithID, store.ErrGetParameter, paramName, err) + return "", fmt.Errorf(errWrapFormatWithID, store.ErrGetParameter, paramName, err) } - return s.decodeParameterValue(*output.Parameter.Value), nil + return *output.Parameter.Value, nil } // GetKey retrieves a value by key from AWS SSM Parameter store.Store. diff --git a/pkg/store/providers/azure_keyvault_store.go b/pkg/store/providers/azure_keyvault_store.go index e200e31f25..8eec055e3b 100644 --- a/pkg/store/providers/azure_keyvault_store.go +++ b/pkg/store/providers/azure_keyvault_store.go @@ -54,6 +54,7 @@ type AzureKeyVaultStore struct { stackDelimiter *string clientOptions *azsecrets.ClientOptions withoutAuth bool + secret bool // Identity-based authentication fields. identityName string @@ -81,6 +82,7 @@ var ( _ store.DeletableStore = (*AzureKeyVaultStore)(nil) _ store.StatusStore = (*AzureKeyVaultStore)(nil) _ store.ListableStore = (*AzureKeyVaultStore)(nil) + _ store.SecretAwareStore = (*AzureKeyVaultStore)(nil) ) // NewAzureKeyVaultStore creates a new Azure Key Vault store. @@ -287,13 +289,9 @@ func (s *AzureKeyVaultStore) getKey(stack string, component string, key string) return s.normalizeSecretName(baseKey), nil } +// Set writes a value for an Atmos secret coordinate. Empty stack and component segments are +// valid for stack-scoped and global secrets and are omitted by getKey. func (s *AzureKeyVaultStore) Set(stack string, component string, key string, value interface{}) error { - if stack == "" { - return store.ErrEmptyStack - } - if component == "" { - return store.ErrEmptyComponent - } if key == "" { return store.ErrEmptyKey } @@ -310,12 +308,10 @@ func (s *AzureKeyVaultStore) Set(stack string, component string, key string, val return fmt.Errorf(errWrapFormat, store.ErrGetKey, err) } - // Convert value to JSON string like other stores. - jsonValue, err := json.Marshal(value) + strValue, err := marshalAzureSecretValue(value, s.secret) if err != nil { return fmt.Errorf(errWrapFormat, store.ErrSerializeJSON, err) } - strValue := string(jsonValue) params := azsecrets.SetSecretParameters{ Value: &strValue, @@ -333,61 +329,81 @@ func (s *AzureKeyVaultStore) Set(stack string, component string, key string, val return nil } +// SetSecret implements SecretAwareStore. Secret string values are written verbatim so +// `atmos secret set` round-trips through `!secret ... | raw`; structured values remain JSON. +func (s *AzureKeyVaultStore) SetSecret(secret bool) { + s.secret = secret +} + +func marshalAzureSecretValue(value any, rawStrings bool) (string, error) { + if rawStrings { + if text, ok := value.(string); ok { + return text, nil + } + } + encoded, err := json.Marshal(value) + if err != nil { + return "", err + } + return string(encoded), nil +} + func (s *AzureKeyVaultStore) Get(stack string, component string, key string) (interface{}, error) { - if stack == "" { - return nil, store.ErrEmptyStack + raw, err := s.GetRaw(stack, component, key) + if err != nil { + return nil, err } - if component == "" { - return nil, store.ErrEmptyComponent + + var result interface{} + if jsonErr := json.Unmarshal([]byte(raw), &result); jsonErr != nil { + return raw, nil } + return result, nil +} + +// GetRaw retrieves the original Key Vault secret string without JSON decoding. Empty stack and +// component segments are valid for stack-scoped and global secrets and are omitted by getKey. +func (s *AzureKeyVaultStore) GetRaw(stack string, component string, key string) (string, error) { if key == "" { - return nil, store.ErrEmptyKey + return "", store.ErrEmptyKey } if err := s.ensureClient(); err != nil { - return nil, err + return "", err } secretName, err := s.getKey(stack, component, key) if err != nil { - return nil, fmt.Errorf(errWrapFormat, store.ErrGetKey, err) + return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err) } + return s.getRawByName(secretName) +} +func (s *AzureKeyVaultStore) getRawByName(secretName string) (string, error) { resp, err := s.client.GetSecret(context.Background(), secretName, "", nil) if err != nil { var respErr *azcore.ResponseError if errors.As(err, &respErr) { switch respErr.StatusCode { case statusCodeNotFound: - return nil, fmt.Errorf(errWrapFormatWithID, store.ErrResourceNotFound, secretName, err) + return "", fmt.Errorf(errWrapFormatWithID, store.ErrResourceNotFound, secretName, err) case statusCodeForbidden: - return nil, fmt.Errorf(errWrapFormatWithID, store.ErrPermissionDenied, fmt.Sprintf(secretIDFormat, secretName), err) + return "", fmt.Errorf(errWrapFormatWithID, store.ErrPermissionDenied, fmt.Sprintf(secretIDFormat, secretName), err) } } - return nil, fmt.Errorf(errWrapFormat, store.ErrAccessSecret, err) + return "", fmt.Errorf(errWrapFormat, store.ErrAccessSecret, err) } if resp.Value == nil { return "", nil } - // Try to unmarshal as JSON first, fallback to string if it fails. - var result interface{} - if jsonErr := json.Unmarshal([]byte(*resp.Value), &result); jsonErr != nil { - // If JSON unmarshaling fails, return as string. - return *resp.Value, nil - } - return result, nil + return *resp.Value, nil } -// Delete removes a secret from Azure Key Vault for the given stack, component, and key. +// Delete removes a secret from Azure Key Vault for the given stack, component, and key. Empty +// stack and component segments are valid for stack-scoped and global secrets. func (s *AzureKeyVaultStore) Delete(stack string, component string, key string) error { - if stack == "" { - return store.ErrEmptyStack - } - if component == "" { - return store.ErrEmptyComponent - } if key == "" { return store.ErrEmptyKey } @@ -427,12 +443,6 @@ func (s *AzureKeyVaultStore) Delete(stack string, component string, key string) // secrets have no separate "decrypt" permission distinct from "get"; the versions listing relies // on the "list" permission and is the lightest existence check that avoids reading the value. func (s *AzureKeyVaultStore) Has(stack string, component string, key string) (bool, error) { - if stack == "" { - return false, store.ErrEmptyStack - } - if component == "" { - return false, store.ErrEmptyComponent - } if key == "" { return false, store.ErrEmptyKey } diff --git a/pkg/store/providers/azure_keyvault_store_test.go b/pkg/store/providers/azure_keyvault_store_test.go index 33dd93a6d5..eb181ac35b 100644 --- a/pkg/store/providers/azure_keyvault_store_test.go +++ b/pkg/store/providers/azure_keyvault_store_test.go @@ -10,6 +10,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets" storepkg "github.com/cloudposse/atmos/pkg/store" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // errTestBackend is a generic (non-*azcore.ResponseError) backend error used to exercise the @@ -96,20 +97,26 @@ func TestAzureKeyVaultStore_Set(t *testing.T) { }, }, { - name: "empty stack", + name: "global scope", stack: "", - component: "app", + component: "", key: "secret", value: "value", - wantErr: storepkg.ErrEmptyStack, + mockFunc: func(_ context.Context, name string, _ azsecrets.SetSecretParameters, _ *azsecrets.SetSecretOptions) (azsecrets.SetSecretResponse, error) { + assert.Equal(t, "secret", name) + return azsecrets.SetSecretResponse{}, nil + }, }, { - name: "empty component", + name: "stack scope", stack: "dev", component: "", key: "secret", value: "value", - wantErr: storepkg.ErrEmptyComponent, + mockFunc: func(_ context.Context, name string, _ azsecrets.SetSecretParameters, _ *azsecrets.SetSecretOptions) (azsecrets.SetSecretResponse, error) { + assert.Equal(t, "dev-secret", name) + return azsecrets.SetSecretResponse{}, nil + }, }, { name: "empty key", @@ -153,6 +160,22 @@ func TestAzureKeyVaultStore_Set(t *testing.T) { } } +func TestAzureKeyVaultStore_SetSecretStringVerbatim(t *testing.T) { + var stored string + store := &AzureKeyVaultStore{ + client: &mockClient{setSecretFunc: func(_ context.Context, _ string, parameters azsecrets.SetSecretParameters, _ *azsecrets.SetSecretOptions) (azsecrets.SetSecretResponse, error) { + stored = *parameters.Value + return azsecrets.SetSecretResponse{}, nil + }}, + vaultURL: "https://example.vault.azure.net", + stackDelimiter: stringPtr("-"), + } + store.SetSecret(true) + + require.NoError(t, store.Set("dev", "app", "credential", "example-token")) + assert.Equal(t, "example-token", stored) +} + func TestAzureKeyVaultStore_Get(t *testing.T) { tests := []struct { name string @@ -194,18 +217,28 @@ func TestAzureKeyVaultStore_Get(t *testing.T) { want: map[string]interface{}{"key": "value", "number": float64(123)}, }, { - name: "empty stack", + name: "global scope", stack: "", - component: "app", + component: "", key: "secret", - wantErr: storepkg.ErrEmptyStack, + mockFunc: func(_ context.Context, name string, _ string, _ *azsecrets.GetSecretOptions) (azsecrets.GetSecretResponse, error) { + assert.Equal(t, "secret", name) + value := "global-value" + return azsecrets.GetSecretResponse{Secret: azsecrets.Secret{Value: &value}}, nil + }, + want: "global-value", }, { - name: "empty component", + name: "stack scope", stack: "dev", component: "", key: "secret", - wantErr: storepkg.ErrEmptyComponent, + mockFunc: func(_ context.Context, name string, _ string, _ *azsecrets.GetSecretOptions) (azsecrets.GetSecretResponse, error) { + assert.Equal(t, "dev-secret", name) + value := "stack-value" + return azsecrets.GetSecretResponse{Secret: azsecrets.Secret{Value: &value}}, nil + }, + want: "stack-value", }, { name: "empty key", @@ -277,18 +310,24 @@ func TestAzureKeyVaultStore_Delete(t *testing.T) { }, }, { - name: "empty stack", + name: "global scope", stack: "", - component: "app", + component: "", key: "secret", - wantErr: storepkg.ErrEmptyStack, + mockFunc: func(_ context.Context, name string, _ *azsecrets.DeleteSecretOptions) (azsecrets.DeleteSecretResponse, error) { + assert.Equal(t, "secret", name) + return azsecrets.DeleteSecretResponse{}, nil + }, }, { - name: "empty component", + name: "stack scope", stack: "dev", component: "", key: "secret", - wantErr: storepkg.ErrEmptyComponent, + mockFunc: func(_ context.Context, name string, _ *azsecrets.DeleteSecretOptions) (azsecrets.DeleteSecretResponse, error) { + assert.Equal(t, "dev-secret", name) + return azsecrets.DeleteSecretResponse{}, nil + }, }, { name: "empty key", @@ -422,8 +461,8 @@ func TestAzureKeyVaultStore_Has(t *testing.T) { } } -// TestAzureKeyVaultStore_HasValidatesInput confirms Has() validates its arguments before touching -// the client, matching Get/Set/Delete. +// TestAzureKeyVaultStore_HasValidatesInput confirms Has() rejects an empty key before touching +// the client while permitting omitted scope segments. func TestAzureKeyVaultStore_HasValidatesInput(t *testing.T) { store := &AzureKeyVaultStore{ client: &mockClient{}, @@ -431,23 +470,36 @@ func TestAzureKeyVaultStore_HasValidatesInput(t *testing.T) { stackDelimiter: stringPtr("-"), } + got, err := store.Has("dev", "app", "") + assert.False(t, got) + assert.ErrorIs(t, err, storepkg.ErrEmptyKey) +} + +func TestAzureKeyVaultStore_HasSharedScopes(t *testing.T) { tests := []struct { - name string - stack string - component string - key string - wantErr error + name string + stack string + component string + expectedName string }{ - {name: "empty stack", stack: "", component: "app", key: "secret", wantErr: storepkg.ErrEmptyStack}, - {name: "empty component", stack: "dev", component: "", key: "secret", wantErr: storepkg.ErrEmptyComponent}, - {name: "empty key", stack: "dev", component: "app", key: "", wantErr: storepkg.ErrEmptyKey}, + {name: "stack scope", stack: "dev", expectedName: "dev-secret"}, + {name: "global scope", expectedName: "secret"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := store.Has(tt.stack, tt.component, tt.key) - assert.False(t, got) - assert.ErrorIs(t, err, tt.wantErr) + store := &AzureKeyVaultStore{ + client: &mockClient{listVersionsFunc: func(name string) (azsecrets.ListSecretPropertiesVersionsResponse, error) { + assert.Equal(t, tt.expectedName, name) + return azsecrets.ListSecretPropertiesVersionsResponse{}, nil + }}, + vaultURL: "https://test.vault.azure.net", + stackDelimiter: stringPtr("-"), + } + + got, err := store.Has(tt.stack, tt.component, "secret") + require.NoError(t, err) + assert.True(t, got) }) } } diff --git a/pkg/store/providers/github_actions_store.go b/pkg/store/providers/github_actions_store.go index b3fd67d798..7181fe59cb 100644 --- a/pkg/store/providers/github_actions_store.go +++ b/pkg/store/providers/github_actions_store.go @@ -164,6 +164,11 @@ func (s *GitHubActionsStore) Get(_ string, _ string, key string) (any, error) { return s.getByKey(key) } +// GetRaw returns the exact environment value injected by GitHub Actions. +func (s *GitHubActionsStore) GetRaw(_ string, _ string, key string) (string, error) { + return s.getRawByKey(key) +} + // GetKey returns the secret value for a raw key without stack/component context (same env-read // semantics as Get). func (s *GitHubActionsStore) GetKey(key string) (any, error) { @@ -171,20 +176,28 @@ func (s *GitHubActionsStore) GetKey(key string) (any, error) { } func (s *GitHubActionsStore) getByKey(key string) (any, error) { - name, err := githubSecretName(s.prefix, key) + raw, err := s.getRawByKey(key) if err != nil { return nil, err } + return decodeGitHubSecretValue(raw), nil +} + +func (s *GitHubActionsStore) getRawByKey(key string) (string, error) { + name, err := githubSecretName(s.prefix, key) + if err != nil { + return "", err + } if !s.readAllowed() { - return nil, fmt.Errorf("%w: %q is only readable inside a GitHub Actions runner — %s, or set options.ci.enabled to override", + return "", fmt.Errorf("%w: %q is only readable inside a GitHub Actions runner — %s, or set options.ci.enabled to override", store.ErrGitHubSecretValueCIOnly, name, s.envHint(name)) } s.alignOnce.Do(s.verifyAlignment) raw, ok := lookupGitHubSecretEnv(name) if !ok { - return nil, fmt.Errorf("%w: %q — %s", store.ErrGitHubSecretNotInEnv, name, s.envHint(name)) + return "", fmt.Errorf("%w: %q — %s", store.ErrGitHubSecretNotInEnv, name, s.envHint(name)) } - return decodeGitHubSecretValue(raw), nil + return raw, nil } // envHint describes how to make the secret available in the runner environment, naming the diff --git a/pkg/store/providers/google_secret_manager_store.go b/pkg/store/providers/google_secret_manager_store.go index 6ba0a1a44a..e4d9f00e93 100644 --- a/pkg/store/providers/google_secret_manager_store.go +++ b/pkg/store/providers/google_secret_manager_store.go @@ -18,6 +18,7 @@ import ( "github.com/cloudposse/atmos/internal/gcp" log "github.com/cloudposse/atmos/pkg/logger" + "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/store" ) @@ -73,6 +74,7 @@ type GSMStore struct { endpoint string endpointInsecure bool withoutAuthentication bool + secret bool // Identity-based authentication fields. identityName string @@ -102,6 +104,7 @@ var ( _ store.DeletableStore = (*GSMStore)(nil) _ store.StatusStore = (*GSMStore)(nil) _ store.ListableStore = (*GSMStore)(nil) + _ store.SecretAwareStore = (*GSMStore)(nil) ) // NewGSMStore initializes a new Google Secret Manager store.Store. @@ -374,12 +377,10 @@ func (s *GSMStore) Set(stack string, component string, key string, value any) er ctx, cancel := context.WithTimeout(context.Background(), gsmOperationTimeout) defer cancel() - // Convert value to JSON string - jsonValue, err := json.Marshal(value) + strValue, err := marshalGSMValue(value, s.secret) if err != nil { return fmt.Errorf(errWrapFormat, store.ErrSerializeJSON, err) } - strValue := string(jsonValue) // Get the secret ID using getKey secretID, err := s.getKey(stack, component, key) @@ -399,15 +400,49 @@ func (s *GSMStore) Set(stack string, component string, key string, value any) er return nil } +// SetSecret implements SecretAwareStore. Secret string values are written verbatim so +// `atmos secret set` round-trips through `!secret ... | raw`; structured values remain JSON. +func (s *GSMStore) SetSecret(secret bool) { + s.secret = secret +} + +func marshalGSMValue(value any, rawStrings bool) (string, error) { + if rawStrings { + if text, ok := value.(string); ok { + return text, nil + } + } + encoded, err := json.Marshal(value) + if err != nil { + return "", err + } + return string(encoded), nil +} + // Get retrieves a value by key from Google Secret Manager. An empty stack and/or component is // permitted: scoped secret coordinates (stack/global scope) omit those path segments. func (s *GSMStore) Get(stack string, component string, key string) (any, error) { + payload, err := s.getRaw(stack, component, key) + if err != nil { + return nil, err + } + + var unmarshalled interface{} + // Intentionally ignoring JSON unmarshal error to handle legacy or 3rd-party secrets that might not be JSON-encoded + if err := json.Unmarshal([]byte(payload), &unmarshalled); err != nil { + // If it's not valid JSON, return the raw string value + return payload, nil + } + return unmarshalled, nil +} + +func (s *GSMStore) getRaw(stack string, component string, key string) (string, error) { if key == "" { - return nil, store.ErrEmptyKey + return "", store.ErrEmptyKey } if err := s.ensureClient(); err != nil { - return nil, err + return "", err } ctx, cancel := context.WithTimeout(context.Background(), gsmOperationTimeout) @@ -416,7 +451,7 @@ func (s *GSMStore) Get(stack string, component string, key string) (any, error) // Get the secret ID using getKey secretID, err := s.getKey(stack, component, key) if err != nil { - return nil, fmt.Errorf(errWrapFormat, store.ErrGetKey, err) + return "", fmt.Errorf(errWrapFormat, store.ErrGetKey, err) } // Build the resource name for the latest version @@ -431,21 +466,25 @@ func (s *GSMStore) Get(stack string, component string, key string) (any, error) if ok { switch st.Code() { case codes.NotFound: - return nil, fmt.Errorf(errWrapFormatWithID, store.ErrResourceNotFound, secretID, err) + return "", fmt.Errorf(errWrapFormatWithID, store.ErrResourceNotFound, secretID, err) case codes.PermissionDenied: - return nil, fmt.Errorf(errWrapFormatWithID, store.ErrPermissionDenied, fmt.Sprintf("secret %s", secretID), err) + return "", fmt.Errorf(errWrapFormatWithID, store.ErrPermissionDenied, fmt.Sprintf("secret %s", secretID), err) } } - return nil, fmt.Errorf(errWrapFormat, store.ErrAccessSecret, err) + return "", fmt.Errorf(errWrapFormat, store.ErrAccessSecret, err) } + return string(result.Payload.Data), nil +} - var unmarshalled interface{} - // Intentionally ignoring JSON unmarshal error to handle legacy or 3rd-party secrets that might not be JSON-encoded - if err := json.Unmarshal(result.Payload.Data, &unmarshalled); err != nil { - // If it's not valid JSON, return the raw string value - return string(result.Payload.Data), nil +// GetRaw retrieves the original Secret Manager payload without JSON decoding. +func (s *GSMStore) GetRaw(stack string, component string, key string) (string, error) { + defer perf.Track(nil, "providers.GSMStore.GetRaw")() + + value, err := s.getRaw(stack, component, key) + if err != nil { + return "", err } - return unmarshalled, nil + return value, nil } // Delete removes a secret (and all its versions) from Google Secret Manager for the given diff --git a/pkg/store/providers/google_secret_manager_store_test.go b/pkg/store/providers/google_secret_manager_store_test.go index 401ffac980..2ca99d3734 100644 --- a/pkg/store/providers/google_secret_manager_store_test.go +++ b/pkg/store/providers/google_secret_manager_store_test.go @@ -193,9 +193,19 @@ func TestGSMStore_Set(t *testing.T) { key string value any locations []string + secret bool mockFn func(*MockGSMClient) wantErr bool }{ + { + name: "secret string is stored verbatim", + stack: "dev-usw2", + component: "app/service", + key: "config-key", + value: "test-value", + secret: true, + mockFn: gsmClientSecretCreationMock("test-prefix_dev_usw2_app_service_config-key", `test-value`, nil, nil, nil), + }, { name: "successful set", stack: "dev-usw2", @@ -339,6 +349,7 @@ func TestGSMStore_Set(t *testing.T) { StackDelimiter: &testDelimiter, Locations: &tt.locations, }) + store.SetSecret(tt.secret) err := store.Set(tt.stack, tt.component, tt.key, tt.value) if tt.wantErr { @@ -506,6 +517,40 @@ func TestGSMStore_Get(t *testing.T) { } } +func TestGSMStore_GetRawPreservesPayload(t *testing.T) { + const versionName = "projects/test-project/secrets/test-prefix_dev_usw2_example_service_config-key/versions/latest" + testPrefix := "test-prefix" + testDelimiter := "-" + + for _, tt := range []struct { + name string + payload string + }{ + {name: "JSON object", payload: `{"enabled":true,"port":8080}`}, + {name: "plain text", payload: "example-token"}, + } { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + mockClient := NewMockGSMClient(ctrl) + mockClient.EXPECT().AccessSecretVersion(gomock.Any(), gomock.Cond(func(req *secretmanagerpb.AccessSecretVersionRequest) bool { + return req.Name == versionName + })).Return(&secretmanagerpb.AccessSecretVersionResponse{ + Payload: &secretmanagerpb.SecretPayload{Data: []byte(tt.payload)}, + }, nil) + + s := newGSMStoreWithClient(mockClient, GSMStoreOptions{ + ProjectID: "test-project", + Prefix: &testPrefix, + StackDelimiter: &testDelimiter, + }) + + got, err := s.GetRaw("dev-usw2", "example/service", "config-key") + assert.NoError(t, err) + assert.Equal(t, tt.payload, got) + }) + } +} + func TestGSMStore_Delete(t *testing.T) { testPrefix := "test-prefix" testDelimiter := "-" diff --git a/pkg/store/providers/keychain_store.go b/pkg/store/providers/keychain_store.go index 65284e0f9f..bf9e02b03a 100644 --- a/pkg/store/providers/keychain_store.go +++ b/pkg/store/providers/keychain_store.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/cloudposse/atmos/pkg/keyring" + "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/store" ) @@ -146,6 +147,29 @@ func (s *KeychainStore) Get(stack string, component string, key string) (any, er return s.get(composed) } +// GetRaw returns the original textual value represented by the keychain entry. Values written +// as strings are unquoted; structured values remain in their JSON representation. +func (s *KeychainStore) GetRaw(stack string, component string, key string) (string, error) { + defer perf.Track(nil, "providers.KeychainStore.GetRaw")() + + composed, err := s.composeKey(stack, component, key) + if err != nil { + return "", err + } + raw, err := s.kr.Get(composed) + if err != nil { + if errors.Is(err, keyring.ErrNotFound) { + return "", fmt.Errorf(errWrapFormatWithID, store.ErrKeychainNotFound, composed, err) + } + return "", fmt.Errorf(errWrapFormatWithID, store.ErrKeychainRead, composed, err) + } + var stringValue *string + if err := json.Unmarshal([]byte(raw), &stringValue); err == nil && stringValue != nil { + return *stringValue, nil + } + return raw, nil +} + // GetKey retrieves a value directly by its composed key, without stack/component context. func (s *KeychainStore) GetKey(key string) (any, error) { if key == "" { diff --git a/pkg/store/providers/keychain_store_test.go b/pkg/store/providers/keychain_store_test.go index 170b8721ab..2ac61c1f7a 100644 --- a/pkg/store/providers/keychain_store_test.go +++ b/pkg/store/providers/keychain_store_test.go @@ -1,6 +1,7 @@ package providers import ( + "errors" "testing" "github.com/stretchr/testify/assert" @@ -91,6 +92,61 @@ func TestKeychainStore_SetGetStructuredValue(t *testing.T) { assert.Equal(t, map[string]any{"a": "1", "b": "2"}, got) } +func TestKeychainStore_GetRaw(t *testing.T) { + t.Run("string value is unquoted", func(t *testing.T) { + s := newTestKeychainStore(t) + require.NoError(t, s.Set("dev", "example-service", "token", "example-token")) + + rawStore, ok := s.(store.RawStore) + require.True(t, ok) + got, err := rawStore.GetRaw("dev", "example-service", "token") + require.NoError(t, err) + assert.Equal(t, "example-token", got) + }) + + t.Run("structured value remains JSON", func(t *testing.T) { + s := newTestKeychainStore(t) + require.NoError(t, s.Set("dev", "example-service", "config", map[string]any{"enabled": true})) + + got, err := s.(store.RawStore).GetRaw("dev", "example-service", "config") + require.NoError(t, err) + assert.JSONEq(t, `{"enabled":true}`, got) + }) + + t.Run("JSON null remains raw", func(t *testing.T) { + ctrl := gomock.NewController(t) + kr := keyring.NewMockKeyring(ctrl) + kr.EXPECT().Get("atmos/dev/example-service/token").Return("null", nil) + s := &KeychainStore{kr: kr, prefix: "atmos", stackDelimiter: "-"} + + got, err := s.GetRaw("dev", "example-service", "token") + require.NoError(t, err) + assert.Equal(t, "null", got) + }) + + t.Run("empty key is rejected", func(t *testing.T) { + s := newTestKeychainStore(t) + _, err := s.(store.RawStore).GetRaw("dev", "example-service", "") + assert.ErrorIs(t, err, store.ErrEmptyKey) + }) + + t.Run("missing value is classified", func(t *testing.T) { + s := newTestKeychainStore(t) + _, err := s.(store.RawStore).GetRaw("dev", "example-service", "missing") + assert.ErrorIs(t, err, store.ErrKeychainNotFound) + }) + + t.Run("backend read error is classified", func(t *testing.T) { + ctrl := gomock.NewController(t) + kr := keyring.NewMockKeyring(ctrl) + kr.EXPECT().Get("atmos/dev/example-service/token").Return("", errors.New("backend unavailable")) + s := &KeychainStore{kr: kr, prefix: "atmos", stackDelimiter: "-"} + + _, err := s.GetRaw("dev", "example-service", "token") + assert.ErrorIs(t, err, store.ErrKeychainRead) + }) +} + func TestKeychainStore_GetMissing(t *testing.T) { s := newTestKeychainStore(t) diff --git a/pkg/store/store.go b/pkg/store/store.go index 0ae541dd18..1ef403888b 100644 --- a/pkg/store/store.go +++ b/pkg/store/store.go @@ -12,6 +12,15 @@ type Store interface { GetKey(key string) (any, error) } +// RawStore is an optional capability for stores that decode structured values in Get. +// `!secret ... | raw` uses this capability so an opaque JSON, PEM, or other textual secret is +// returned byte-for-byte. Bare `!secret` and `!secret ... | path` continue to use Get and +// preserve the store's structured-value contract. +type RawStore interface { + Store + GetRaw(stack string, component string, key string) (string, error) +} + // DeletableStore extends Store with the ability to remove a value. Backends that support // deletion (SSM, ASM, Vault, Azure Key Vault, GCP Secret Manager) implement this; backends // that don't may return ErrDeleteNotSupported. The secrets CLI (`atmos secret delete`) diff --git a/tests/fixtures/scenarios/helm-secret-values/atmos.yaml b/tests/fixtures/scenarios/helm-secret-values/atmos.yaml new file mode 100644 index 0000000000..ce7aa1d5f9 --- /dev/null +++ b/tests/fixtures/scenarios/helm-secret-values/atmos.yaml @@ -0,0 +1,18 @@ +base_path: "." + +components: + helm: + base_path: "components/helm" + +stacks: + base_path: "stacks" + included_paths: + - "deploy/**/*" + name_template: "{{ .vars.stage }}" + +stores: + gcp-secrets: + kind: gcp/secretmanager + secret: true + options: + project_id: example-project diff --git a/tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/Chart.yaml b/tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/Chart.yaml new file mode 100644 index 0000000000..6d2555aba8 --- /dev/null +++ b/tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/Chart.yaml @@ -0,0 +1,6 @@ +apiVersion: v2 +name: secret-values +description: Generic fixture for native Helm secret rendering +type: application +version: 0.1.0 +appVersion: "1.0" diff --git a/tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/templates/deployment.yaml b/tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/templates/deployment.yaml new file mode 100644 index 0000000000..c5ce63f286 --- /dev/null +++ b/tests/fixtures/scenarios/helm-secret-values/components/helm/secret-values/templates/deployment.yaml @@ -0,0 +1,18 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: {{ .Release.Name }} +spec: + selector: + matchLabels: + app: {{ .Release.Name }} + template: + metadata: + labels: + app: {{ .Release.Name }} + spec: + containers: + - name: example + image: example.invalid/application:1.0 + env: +{{ toYaml .Values.env | nindent 12 }} diff --git a/tests/fixtures/scenarios/helm-secret-values/stacks/deploy/dev.yaml b/tests/fixtures/scenarios/helm-secret-values/stacks/deploy/dev.yaml new file mode 100644 index 0000000000..4ecaae571d --- /dev/null +++ b/tests/fixtures/scenarios/helm-secret-values/stacks/deploy/dev.yaml @@ -0,0 +1,30 @@ +vars: + stage: dev + +secrets: + vars: + service-account-json: + store: gcp-secrets + plain-token: + store: gcp-secrets + signing-key: + store: gcp-secrets + +components: + helm: + secret-values: + metadata: + component: secret-values + chart: "." + namespace: example + values: + structured_service_account: !secret service-account-json + env: + - name: SERVICE_ACCOUNT_JSON + value: !secret service-account-json |raw + - name: CLIENT_EMAIL + value: !secret service-account-json | path ".client_email" + - name: PLAIN_TOKEN + value: !secret plain-token + - name: SIGNING_KEY + value: !secret signing-key diff --git a/website/docs/cli/commands/secret/set.mdx b/website/docs/cli/commands/secret/set.mdx index eba4cfde36..c7951c4894 100644 --- a/website/docs/cli/commands/secret/set.mdx +++ b/website/docs/cli/commands/secret/set.mdx @@ -20,11 +20,17 @@ Set the value of a declared secret in its configured backend (create or update). atmos secret set [NAME[=VALUE]] [flags] ``` -Setting a secret is **scope-aware**: a value set with `--component` is an *instance override* and is -only allowed if that component declares the secret (otherwise it is a hard error — declare it under -the component first, or omit `--component` to set the shared stack value). See +Setting a secret is **scope-aware**. Atmos normally requires the component that declares the secret. +For a named secret declared with `scope: global`, you may omit `--component`; Atmos discovers a +declaration-bearing component in the selected stack because the component does not affect the global +backend coordinate. Instance- and stack-scoped secrets still require `--component`. See [Secret scopes](/cli/commands/secret/usage#secret-scopes). +String values are written verbatim to secret stores. This makes values written by `atmos secret set` +round-trip through `!secret NAME | raw` without JSON quote characters being added. Structured maps and +lists supplied through other store APIs remain JSON encoded. Values written by an older Atmos release +may already contain JSON quotes; set those values again with the current release to normalize them. + ## Examples ```shell @@ -32,19 +38,22 @@ the component first, or omit `--component` to set the shared stack value). See atmos secret set # Set a value inline -atmos secret set DATADOG_API_KEY=abc123 --stack=prod --component=api +atmos secret set SERVICE_API_KEY=abc123 --stack=prod --component=example-service # Set a value interactively with a masked prompt (terminal only) -atmos secret set DATADOG_API_KEY --stack=prod --component=api +atmos secret set SERVICE_API_KEY --stack=prod --component=example-service # Read a multi-line value (such as a PEM key) from standard input -cat key.pem | atmos secret set GITHUB_APP_KEY --stdin --stack=prod --component=api +cat key.pem | atmos secret set APP_PRIVATE_KEY --stdin --stack=prod --component=example-service + +# A global declaration does not require a component +atmos secret set SHARED_API_KEY=abc123 --stack=prod # Overwrite an existing value without confirmation -atmos secret set DATADOG_API_KEY=newvalue --stack=prod --component=api --force +atmos secret set SERVICE_API_KEY=newvalue --stack=prod --component=example-service --force # Use the `add` alias -atmos secret add DATADOG_API_KEY=abc123 --stack=prod --component=api +atmos secret add SERVICE_API_KEY=abc123 --stack=prod --component=example-service ``` ## Arguments @@ -68,7 +77,7 @@ atmos secret add DATADOG_API_KEY=abc123 --stack=prod --component=api
`--component` (alias `-c`)
- The Atmos component that declares the secret. Required for instance-scoped secrets — prompted interactively on a TTY when omitted. + The Atmos component that declares the secret. Required except when a positional secret name resolves to a consistent `scope: global` declaration in the selected stack. Prompted interactively on a TTY when no name is supplied. **Environment variable:** `ATMOS_COMPONENT`
diff --git a/website/docs/functions/yaml/secret.mdx b/website/docs/functions/yaml/secret.mdx index bef5642ea1..9346df64fd 100644 --- a/website/docs/functions/yaml/secret.mdx +++ b/website/docs/functions/yaml/secret.mdx @@ -18,7 +18,7 @@ values are automatically registered with the I/O masker, so they are redacted in ## Usage ```yaml - !secret | path | default + !secret [| path | raw] [| default ] ``` The secret `` must be declared under the component's [`secrets.vars`](/cli/configuration/secrets) before it @@ -40,11 +40,28 @@ Usage examples: db_host: !secret DATABASE_CONFIG | path ".host" db_password: !secret DATABASE_CONFIG | path ".credentials.password" + # Preserve the exact backend payload when valid JSON must remain an opaque string + vars: + service_credentials: !secret SERVICE_CREDENTIALS | raw + # Combine path and default vars: db_host: !secret DATABASE_CONFIG | path ".host" | default "localhost" ``` +Bare `!secret` preserves the store's existing value contract, including structured maps and lists. +Use `| raw` when the original textual payload is required byte-for-byte. The `raw` and `path` +modifiers are mutually exclusive. + +Some store backends detect structured values by attempting to decode the payload as JSON. Use +`| raw` whenever a secret that must remain a string might be valid JSON—including objects, arrays, +bare numbers, booleans, `null`, and JSON-quoted strings. This prevents a future secret rotation from +silently changing the resolved value's type. + +`atmos secret set` writes string values verbatim, so new values round-trip through `| raw`. Secrets +written by older Atmos releases may contain JSON quote characters in the stored payload; set them +again with the current release before switching those references to `| raw`. + ## Declaration A secret must be declared before use. Declarations live in stack/component config and reference a backend by name: