From 16859e3b55fcb496d23bffd69b41e03a46adcb8b Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 20 May 2026 21:47:52 -0400 Subject: [PATCH 1/2] fix: preserve required secrets in infra plans --- cmd/wfctl/infra.go | 73 +++++++++++++++---- .../infra_plan_env_vars_preserve_test.go | 52 +++++++++++++ cmd/wfctl/infra_resolve_state.go | 58 +++++++++++---- cmd/wfctl/infra_resolve_state_test.go | 65 ++++++++++++++++- .../testdata/infra-with-env-var-refs.yaml | 6 ++ 5 files changed, 226 insertions(+), 28 deletions(-) diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 000b39ed..ee92c98d 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -7,6 +7,7 @@ import ( "flag" "fmt" "os" + "sort" "strings" "time" @@ -444,22 +445,66 @@ func resourceSpecFromResolvedModule(r *config.ResolvedModule) interfaces.Resourc return spec } -// secretGenKeys returns the variable names declared in cfg.Secrets.Generate. +// declaredSecretKeys returns the variable names declared as workflow secrets. // These keys are preserved as literal ${VAR} references during plan-time // config expansion so that desiredStateHash produces the same result // regardless of whether the variable is currently set in the process -// environment. This fixes the "plan stale: config hash mismatch" error that -// occurs when a generated secret (e.g. STAGING_PG_PASSWORD) is referenced -// outside env_vars — for example in a Droplet user_data cloud-init script — -// where the variable is absent at plan time but present at apply time. -func secretGenKeys(cfg *config.WorkflowConfig) []string { - if cfg == nil || cfg.Secrets == nil { +// environment. This covers generated secrets and externally supplied required +// secrets declared through either the top-level secrets block or secrets.* +// modules. +func declaredSecretKeys(cfg *config.WorkflowConfig) []string { + if cfg == nil { return nil } - keys := make([]string, 0, len(cfg.Secrets.Generate)) - for _, g := range cfg.Secrets.Generate { - if g.Key != "" { - keys = append(keys, g.Key) + keys := map[string]struct{}{} + if cfg.Secrets != nil { + for _, g := range cfg.Secrets.Generate { + if g.Key != "" { + keys[g.Key] = struct{}{} + } + } + for _, entry := range cfg.Secrets.Entries { + if entry.Name != "" { + keys[entry.Name] = struct{}{} + } + } + } + for _, m := range cfg.Modules { + if m.Type != "secrets.generate" && m.Type != "secrets.requires" { + continue + } + for _, field := range []string{"generate", "requires"} { + for _, key := range secretModuleKeys(m.Config, field) { + keys[key] = struct{}{} + } + } + } + out := make([]string, 0, len(keys)) + for key := range keys { + out = append(out, key) + } + sort.Strings(out) + return out +} + +func secretModuleKeys(moduleCfg map[string]any, field string) []string { + raw, ok := moduleCfg[field] + if !ok { + return nil + } + items, ok := raw.([]any) + if !ok { + return nil + } + keys := make([]string, 0, len(items)) + for _, item := range items { + m, ok := item.(map[string]any) + if !ok { + continue + } + key, ok := m["key"].(string) + if ok && key != "" { + keys = append(keys, key) } } return keys @@ -472,7 +517,7 @@ func parseInfraResourceSpecs(cfgFile string) ([]interfaces.ResourceSpec, error) if err != nil { return nil, fmt.Errorf("load %s: %w", cfgFile, err) } - secretVars := secretGenKeys(cfg) + secretVars := declaredSecretKeys(cfg) var specs []interfaces.ResourceSpec for _, m := range cfg.Modules { if !isInfraType(m.Type) { @@ -517,7 +562,7 @@ func planResourcesForEnv(path, envName string) ([]*config.ResolvedModule, error) if envName != "" && cfg.Environments != nil { topEnv = cfg.Environments[envName] } - secretVars := secretGenKeys(cfg) + secretVars := declaredSecretKeys(cfg) var out []*config.ResolvedModule for i := range cfg.Modules { m := &cfg.Modules[i] @@ -574,7 +619,7 @@ func planResourcesForEnv(path, envName string) ([]*config.ResolvedModule, error) // ${VAR} literals through plan serialization (apply-time injection // resolves them when the plugin creates/updates the resource). // secretVars are also preserved so that fields like user_data that - // reference generated secrets produce the same hash at plan time + // reference declared secrets produce the same hash at plan time // (variable unset) and apply time (variable set). resolved.Config = config.ExpandEnvInMapPreservingVars(resolved.Config, infraPreserveKeys, secretVars) out = append(out, resolved) diff --git a/cmd/wfctl/infra_plan_env_vars_preserve_test.go b/cmd/wfctl/infra_plan_env_vars_preserve_test.go index e5b1a930..1fff992b 100644 --- a/cmd/wfctl/infra_plan_env_vars_preserve_test.go +++ b/cmd/wfctl/infra_plan_env_vars_preserve_test.go @@ -13,6 +13,7 @@ func TestParseInfraResourceSpecs_PreservesEnvVarRefs(t *testing.T) { t.Setenv("IMAGE_REF", "registry.example.com/api:abc123") t.Setenv("AUTH_TOKEN", "would-be-resolved-secret") t.Setenv("DATABASE_URL", "postgres://would-be-resolved") + t.Setenv("EXTERNAL_API_TOKEN", "would-be-resolved-required-secret") // POSTGRES_PASSWORD is in secrets.generate — leave it unset to simulate plan time. specs, err := parseInfraResourceSpecs("testdata/infra-with-env-var-refs.yaml") @@ -142,6 +143,54 @@ func TestParseInfraResourceSpecs_PreservesSecretGenVarsInUserData(t *testing.T) } } +func TestParseInfraResourceSpecs_PreservesRequiredSecretVarsInUserData(t *testing.T) { + t.Setenv("DIGITALOCEAN_TOKEN", "actual-do-token") + t.Setenv("IMAGE_REF", "registry.example.com/api:abc123") + t.Setenv("AUTH_TOKEN", "would-be-resolved-secret") + t.Setenv("DATABASE_URL", "postgres://would-be-resolved") + t.Setenv("POSTGRES_PASSWORD", "deadbeef1234567890abcdef12345678") + + t.Setenv("EXTERNAL_API_TOKEN", "") + specsAtPlan, err := parseInfraResourceSpecs("testdata/infra-with-env-var-refs.yaml") + if err != nil { + t.Fatalf("parseInfraResourceSpecs (plan): %v", err) + } + hashAtPlan := desiredStateHash(specsAtPlan) + + t.Setenv("EXTERNAL_API_TOKEN", "required-secret-value") + specsAtApply, err := parseInfraResourceSpecs("testdata/infra-with-env-var-refs.yaml") + if err != nil { + t.Fatalf("parseInfraResourceSpecs (apply): %v", err) + } + hashAtApply := desiredStateHash(specsAtApply) + + if hashAtPlan != hashAtApply { + planJSON, _ := json.MarshalIndent(specsAtPlan, "", " ") + applyJSON, _ := json.MarshalIndent(specsAtApply, "", " ") + t.Errorf("desiredStateHash mismatch between plan and apply:\n"+ + " plan hash: %s\n apply hash: %s\n\nplan specs:\n%s\n\napply specs:\n%s", + hashAtPlan, hashAtApply, planJSON, applyJSON) + } + + var dropletCfg map[string]any + for _, s := range specsAtApply { + if s.Name == "example-droplet" { + dropletCfg = s.Config + break + } + } + if dropletCfg == nil { + t.Fatal("example-droplet spec not found in parsed specs") + } + ud, _ := dropletCfg["user_data"].(string) + if !strings.Contains(ud, "${EXTERNAL_API_TOKEN}") { + t.Errorf("user_data should contain literal ${EXTERNAL_API_TOKEN}, got:\n%s", ud) + } + if strings.Contains(ud, "required-secret-value") { + t.Errorf("user_data should NOT contain the resolved required secret value, got:\n%s", ud) + } +} + // TestPlanEnvVarPreserveTestdataExists ensures the fixture file exists and // has the env_vars_secret block required for the preservation test. func TestPlanEnvVarPreserveTestdataExists(t *testing.T) { @@ -162,4 +211,7 @@ func TestPlanEnvVarPreserveTestdataExists(t *testing.T) { if !strings.Contains(string(b), "secrets:") { t.Errorf("fixture missing secrets: section — needed for secret-gen preservation test") } + if !strings.Contains(string(b), "secrets.requires") { + t.Errorf("fixture missing secrets.requires module — needed for required-secret preservation test") + } } diff --git a/cmd/wfctl/infra_resolve_state.go b/cmd/wfctl/infra_resolve_state.go index 26c6d79c..c389559c 100644 --- a/cmd/wfctl/infra_resolve_state.go +++ b/cmd/wfctl/infra_resolve_state.go @@ -132,23 +132,55 @@ func buildResolvedSecretsFromState( return out } -// buildRuntimeOnlySecretKeys returns the set of SecretGen keys whose Type -// is NOT "infra_output". These secrets (random_hex, random_base64, -// random_alphanumeric, provider_credential, etc.) are runtime-resolved: the -// runtime JIT resolver substitutes them from env at apply time. They MUST -// NOT be substituted at plan time — even if the value is present in the -// process environment — because doing so would embed a literal secret value -// in the plan, which security-check R4 flags as a potential secret literal -// in env_vars. See ADR 0014. +// buildRuntimeOnlySecretKeys returns the set of declared secret keys that must +// resolve only at apply/runtime. Generated non-infra_output secrets and +// externally supplied required secrets MUST NOT be substituted at plan time — +// even if present in the process environment — because doing so would embed a +// literal secret value in the plan, which security-check R4 flags as a +// potential secret literal in env_vars. See ADR 0014. func buildRuntimeOnlySecretKeys(cfg *config.WorkflowConfig) map[string]struct{} { - if cfg == nil || cfg.Secrets == nil || len(cfg.Secrets.Generate) == 0 { + if cfg == nil { return nil } - out := make(map[string]struct{}, len(cfg.Secrets.Generate)) - for _, gen := range cfg.Secrets.Generate { - if gen.Type != "infra_output" { - out[gen.Key] = struct{}{} + out := make(map[string]struct{}) + if cfg.Secrets != nil { + for _, gen := range cfg.Secrets.Generate { + if gen.Type != "infra_output" && gen.Key != "" { + out[gen.Key] = struct{}{} + } } + for _, entry := range cfg.Secrets.Entries { + if entry.Name != "" { + out[entry.Name] = struct{}{} + } + } + } + for _, m := range cfg.Modules { + switch m.Type { + case "secrets.requires": + for _, key := range secretModuleKeys(m.Config, "requires") { + out[key] = struct{}{} + } + case "secrets.generate": + raw, ok := m.Config["generate"].([]any) + if !ok { + continue + } + for _, item := range raw { + gen, ok := item.(map[string]any) + if !ok { + continue + } + key, _ := gen["key"].(string) + typ, _ := gen["type"].(string) + if key != "" && typ != "infra_output" { + out[key] = struct{}{} + } + } + } + } + if len(out) == 0 { + return nil } return out } diff --git a/cmd/wfctl/infra_resolve_state_test.go b/cmd/wfctl/infra_resolve_state_test.go index 96c75a69..da6ec0ce 100644 --- a/cmd/wfctl/infra_resolve_state_test.go +++ b/cmd/wfctl/infra_resolve_state_test.go @@ -152,6 +152,30 @@ func TestBuildRuntimeOnlySecretKeys(t *testing.T) { {Key: "SESSION_SECRET", Type: "random_base64", Length: 48}, {Key: "SPACES_access_key", Type: "provider_credential", Source: "digitalocean.spaces"}, }, + Entries: []config.SecretEntry{ + {Name: "STRIPE_KEY"}, + }, + }, + Modules: []config.ModuleConfig{ + { + Name: "required-secrets", + Type: "secrets.requires", + Config: map[string]any{ + "requires": []any{ + map[string]any{"key": "WFCOMPUTE_VALIDATION_TOKEN"}, + }, + }, + }, + { + Name: "generated-secrets", + Type: "secrets.generate", + Config: map[string]any{ + "generate": []any{ + map[string]any{"key": "MODULE_RANDOM", "type": "random_hex"}, + map[string]any{"key": "MODULE_INFRA_OUTPUT", "type": "infra_output"}, + }, + }, + }, }, } keys := buildRuntimeOnlySecretKeys(cfg) @@ -159,8 +183,11 @@ func TestBuildRuntimeOnlySecretKeys(t *testing.T) { if _, ok := keys["STAGING_VPC_UUID"]; ok { t.Errorf("STAGING_VPC_UUID (infra_output) must NOT be in runtime-only keys") } + if _, ok := keys["MODULE_INFRA_OUTPUT"]; ok { + t.Errorf("MODULE_INFRA_OUTPUT (infra_output) must NOT be in runtime-only keys") + } // Non-infra_output types must be in the blocklist. - for _, wantKey := range []string{"NATS_AUTH_TOKEN", "SESSION_SECRET", "SPACES_access_key"} { + for _, wantKey := range []string{"NATS_AUTH_TOKEN", "SESSION_SECRET", "SPACES_access_key", "STRIPE_KEY", "WFCOMPUTE_VALIDATION_TOKEN", "MODULE_RANDOM"} { if _, ok := keys[wantKey]; !ok { t.Errorf("%s must be in runtime-only keys", wantKey) } @@ -226,3 +253,39 @@ func TestResolveSpecsAgainstState_RuntimeOnlySecretNotSubstituted(t *testing.T) t.Errorf("diags: got %+v, want one entry for NATS_AUTH_TOKEN", diags) } } + +func TestResolveSpecsAgainstState_RequiredSecretNotSubstituted(t *testing.T) { + t.Setenv("WFCOMPUTE_VALIDATION_TOKEN", "literal-token-that-must-not-enter-plan") + specs := []interfaces.ResourceSpec{{ + Name: "bmw-staging", + Type: "infra.container_service", + Config: map[string]any{ + "env_vars": map[string]any{ + "WFCOMPUTE_VALIDATION_TOKEN": "${WFCOMPUTE_VALIDATION_TOKEN}", + }, + }, + }} + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{{ + Name: "bmw-required-secrets", + Type: "secrets.requires", + Config: map[string]any{ + "requires": []any{ + map[string]any{"key": "WFCOMPUTE_VALIDATION_TOKEN"}, + }, + }, + }}, + } + + out, diags, err := resolveSpecsAgainstState(specs, nil, cfg, "staging") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + envVars, _ := out[0].Config["env_vars"].(map[string]any) + if val := envVars["WFCOMPUTE_VALIDATION_TOKEN"]; val != "${WFCOMPUTE_VALIDATION_TOKEN}" { + t.Errorf("WFCOMPUTE_VALIDATION_TOKEN: got %q, want preserved template", val) + } + if len(diags) != 1 || diags[0].Ref != "WFCOMPUTE_VALIDATION_TOKEN" { + t.Errorf("diags: got %+v, want one entry for WFCOMPUTE_VALIDATION_TOKEN", diags) + } +} diff --git a/cmd/wfctl/testdata/infra-with-env-var-refs.yaml b/cmd/wfctl/testdata/infra-with-env-var-refs.yaml index 9b569f13..f3dea7f8 100644 --- a/cmd/wfctl/testdata/infra-with-env-var-refs.yaml +++ b/cmd/wfctl/testdata/infra-with-env-var-refs.yaml @@ -6,6 +6,11 @@ secrets: length: 32 modules: + - name: required-secrets + type: secrets.requires + config: + requires: + - key: EXTERNAL_API_TOKEN - name: do-provider type: iac.provider config: @@ -35,3 +40,4 @@ modules: postgres: environment: POSTGRES_PASSWORD: '${POSTGRES_PASSWORD}' + EXTERNAL_API_TOKEN: '${EXTERNAL_API_TOKEN}' From 2b41cc47a45a4f6bc0f79ff08a2363a3813bffe5 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 20 May 2026 21:56:01 -0400 Subject: [PATCH 2/2] test: cover secret entry preservation --- cmd/wfctl/infra_align_rules.go | 34 ++---------- .../infra_plan_env_vars_preserve_test.go | 52 +++++++++++++++++++ .../testdata/infra-with-env-var-refs.yaml | 3 ++ 3 files changed, 59 insertions(+), 30 deletions(-) diff --git a/cmd/wfctl/infra_align_rules.go b/cmd/wfctl/infra_align_rules.go index 5f22636c..d36e3536 100644 --- a/cmd/wfctl/infra_align_rules.go +++ b/cmd/wfctl/infra_align_rules.go @@ -68,43 +68,17 @@ func buildAlignContext(cfgFile string) (*alignContext, error) { case m.Type == "infra.database": ctx.databases = append(ctx.databases, m) case m.Type == "secrets.generate" || m.Type == "secrets.requires": - if gen, ok := extractSecretKeys(m.Config, "generate"); ok { - for _, k := range gen { - ctx.secretKeys[k] = struct{}{} - } + for _, k := range secretModuleKeys(m.Config, "generate") { + ctx.secretKeys[k] = struct{}{} } - if req, ok := extractSecretKeys(m.Config, "requires"); ok { - for _, k := range req { - ctx.secretKeys[k] = struct{}{} - } + for _, k := range secretModuleKeys(m.Config, "requires") { + ctx.secretKeys[k] = struct{}{} } } } return ctx, nil } -// extractSecretKeys extracts key names from config[field] which is expected -// to be []any where each element is map[string]any with a "key" field. -func extractSecretKeys(cfg map[string]any, field string) ([]string, bool) { - raw, ok := cfg[field] - if !ok { - return nil, false - } - items, ok := raw.([]any) - if !ok { - return nil, false - } - var keys []string - for _, item := range items { - if m, ok := item.(map[string]any); ok { - if k, ok := m["key"].(string); ok && k != "" { - keys = append(keys, k) - } - } - } - return keys, true -} - // ── R-A1: container/runtime alignment ────────────────────────────────────── func checkRA1(ctx *alignContext) []AlignFinding { diff --git a/cmd/wfctl/infra_plan_env_vars_preserve_test.go b/cmd/wfctl/infra_plan_env_vars_preserve_test.go index 1fff992b..437b72fa 100644 --- a/cmd/wfctl/infra_plan_env_vars_preserve_test.go +++ b/cmd/wfctl/infra_plan_env_vars_preserve_test.go @@ -191,6 +191,55 @@ func TestParseInfraResourceSpecs_PreservesRequiredSecretVarsInUserData(t *testin } } +func TestParseInfraResourceSpecs_PreservesSecretEntriesInUserData(t *testing.T) { + t.Setenv("DIGITALOCEAN_TOKEN", "actual-do-token") + t.Setenv("IMAGE_REF", "registry.example.com/api:abc123") + t.Setenv("AUTH_TOKEN", "would-be-resolved-secret") + t.Setenv("DATABASE_URL", "postgres://would-be-resolved") + t.Setenv("POSTGRES_PASSWORD", "deadbeef1234567890abcdef12345678") + t.Setenv("EXTERNAL_API_TOKEN", "required-secret-value") + + t.Setenv("MANUAL_API_TOKEN", "") + specsAtPlan, err := parseInfraResourceSpecs("testdata/infra-with-env-var-refs.yaml") + if err != nil { + t.Fatalf("parseInfraResourceSpecs (plan): %v", err) + } + hashAtPlan := desiredStateHash(specsAtPlan) + + t.Setenv("MANUAL_API_TOKEN", "manual-secret-value") + specsAtApply, err := parseInfraResourceSpecs("testdata/infra-with-env-var-refs.yaml") + if err != nil { + t.Fatalf("parseInfraResourceSpecs (apply): %v", err) + } + hashAtApply := desiredStateHash(specsAtApply) + + if hashAtPlan != hashAtApply { + planJSON, _ := json.MarshalIndent(specsAtPlan, "", " ") + applyJSON, _ := json.MarshalIndent(specsAtApply, "", " ") + t.Errorf("desiredStateHash mismatch between plan and apply:\n"+ + " plan hash: %s\n apply hash: %s\n\nplan specs:\n%s\n\napply specs:\n%s", + hashAtPlan, hashAtApply, planJSON, applyJSON) + } + + var dropletCfg map[string]any + for _, s := range specsAtApply { + if s.Name == "example-droplet" { + dropletCfg = s.Config + break + } + } + if dropletCfg == nil { + t.Fatal("example-droplet spec not found in parsed specs") + } + ud, _ := dropletCfg["user_data"].(string) + if !strings.Contains(ud, "${MANUAL_API_TOKEN}") { + t.Errorf("user_data should contain literal ${MANUAL_API_TOKEN}, got:\n%s", ud) + } + if strings.Contains(ud, "manual-secret-value") { + t.Errorf("user_data should NOT contain the resolved secret entry value, got:\n%s", ud) + } +} + // TestPlanEnvVarPreserveTestdataExists ensures the fixture file exists and // has the env_vars_secret block required for the preservation test. func TestPlanEnvVarPreserveTestdataExists(t *testing.T) { @@ -211,6 +260,9 @@ func TestPlanEnvVarPreserveTestdataExists(t *testing.T) { if !strings.Contains(string(b), "secrets:") { t.Errorf("fixture missing secrets: section — needed for secret-gen preservation test") } + if !strings.Contains(string(b), "entries:") { + t.Errorf("fixture missing secrets.entries section — needed for secret-entry preservation test") + } if !strings.Contains(string(b), "secrets.requires") { t.Errorf("fixture missing secrets.requires module — needed for required-secret preservation test") } diff --git a/cmd/wfctl/testdata/infra-with-env-var-refs.yaml b/cmd/wfctl/testdata/infra-with-env-var-refs.yaml index f3dea7f8..8040a5ce 100644 --- a/cmd/wfctl/testdata/infra-with-env-var-refs.yaml +++ b/cmd/wfctl/testdata/infra-with-env-var-refs.yaml @@ -1,5 +1,7 @@ secrets: provider: github + entries: + - name: MANUAL_API_TOKEN generate: - key: POSTGRES_PASSWORD type: random_hex @@ -41,3 +43,4 @@ modules: environment: POSTGRES_PASSWORD: '${POSTGRES_PASSWORD}' EXTERNAL_API_TOKEN: '${EXTERNAL_API_TOKEN}' + MANUAL_API_TOKEN: '${MANUAL_API_TOKEN}'