diff --git a/cmd/wfctl/infra_bootstrap.go b/cmd/wfctl/infra_bootstrap.go index cef7b3da..1d062463 100644 --- a/cmd/wfctl/infra_bootstrap.go +++ b/cmd/wfctl/infra_bootstrap.go @@ -509,6 +509,26 @@ var bootstrapSecrets = func(ctx context.Context, provider secrets.Provider, cfg if len(revoker) > 0 { credRevoker = revoker[0] } + // Strict-contract guard: every forceRotate entry MUST match exactly one + // secrets.generate[].Key. A mismatch (e.g. caller passed a cloud-side + // resource Name without translating to gen.Key) means the force-rotate + // code path will be silently bypassed for that entry — the loop below + // keys forceRotate[gen.Key], so the rotation never runs and the rotations + // slice stays empty. This produced the staging-dispatch false-negative + // "expected 1 rotation result, got 0" surfaced 2026-05-09 (run 25616807427). + // Caller MUST translate name→key (see buildForceRotateSet in this file + // and buildRotateAndPruneForceRotateSet in infra_rotate_and_prune.go). + if len(forceRotate) > 0 && cfg != nil { + known := make(map[string]bool, len(cfg.Generate)) + for _, gen := range cfg.Generate { + known[gen.Key] = true + } + for k := range forceRotate { + if !known[k] { + return nil, nil, fmt.Errorf("forceRotate entry %q does not match any secrets.generate[].key — caller must translate cloud-side names to canonical keys before calling bootstrapSecrets", k) + } + } + } generated := map[string]string{} var rotations []RotationResult diff --git a/cmd/wfctl/infra_bootstrap_rotation_count_test.go b/cmd/wfctl/infra_bootstrap_rotation_count_test.go new file mode 100644 index 00000000..c84f561d --- /dev/null +++ b/cmd/wfctl/infra_bootstrap_rotation_count_test.go @@ -0,0 +1,195 @@ +package main + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestRotateAndPrune_KeyNameMismatch_ReturnsRotationResult +// pins the contract that surfaced as a real-world bug on 2026-05-09 in +// core-dump rotate-spaces-key staging dispatch run 25616807427: +// +// Step 1: rotating credential "coredump-deploy-key" (type infra.spaces_key)... +// secret "SPACES_access_key": created +// WFCTL_NEW_KEY_CREATED_AT=2026-05-10T01:41:58Z +// [external-plugins] shutting down plugin "digitalocean" +// rotate-and-prune: expected 1 rotation result, got 0 +// error: rotate-and-prune exited with code 1 +// +// Root cause: `wfctl infra rotate-and-prune --name ` builds +// `forceRotate := map[string]bool{name: true}` keyed by the cloud-side +// resource name (per ADR 0023 + docs/runbooks/spaces-key-prune.md), but +// `bootstrapSecrets` checks `forceRotate[gen.Key]` keyed by the canonical +// generator key. Mismatch → force-rotate code path silently bypassed → +// rotations slice never appended to → "expected 1 rotation result, got 0" +// AFTER the cloud and GH Secrets side effects already committed. +// +// This test exercises the full runInfraRotateAndPrune path with a config +// where `key != name` — the exact shape of core-dump's infra.yaml on the +// failing run. Without the fix, runInfraRotateAndPrune returns code=1 with +// "expected 1 rotation result, got 0" even though bootstrapSecrets minted +// the new credential. With the fix, the CLI translates --name→gen.Key +// at the buildRotateAndPruneForceRotateSet boundary; bootstrapSecrets +// strictly requires forceRotate keyed by gen.Key and rejects unknown +// entries (no name-fallback on the bootstrap side). +func TestRotateAndPrune_KeyNameMismatch_ReturnsRotationResult(t *testing.T) { + t.Setenv("WFCTL_CONFIRM_PRUNE", "1") + tmpDir := t.TempDir() + t.Setenv("WFCTL_STATE_DIR", tmpDir) + + // Write a config matching core-dump's shape: secrets.generate[].key=SPACES, + // secrets.generate[].name=coredump-deploy-key. The CLI takes + // --name coredump-deploy-key (cloud-side resource name). + cfgPath := filepath.Join(tmpDir, "infra.yaml") + body := `secrets: + provider: env + config: + prefix: WFCTL_TEST_ + generate: + - key: SPACES + type: provider_credential + source: digitalocean.spaces + name: coredump-deploy-key +` + if err := os.WriteFile(cfgPath, []byte(body), 0600); err != nil { + t.Fatalf("write fixture infra.yaml: %v", err) + } + + // Stub the underlying generator so we don't reach DO. Returns a JSON + // blob shaped like generateDOSpacesKey's real output. The stub also + // proves the rotation HAPPENED (it was called) so any "got 0" failure + // is purely the keying-mismatch bug, not a missing call. + withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { + return `{"access_key":"AK_NEW_FROM_STUB","secret_key":"SK_NEW_FROM_STUB","created_at":"2026-05-10T01:41:58Z"}`, nil + }) + + // Pre-populate the env-provider's backing values so the force-rotate + // path (which reads OLD access_key for revocation) finds a value to + // "revoke" without being a hard requirement. Env provider reads from + // process env via prefix WFCTL_TEST_. + t.Setenv("WFCTL_TEST_SPACES_access_key", "AK_OLD") + t.Setenv("WFCTL_TEST_SPACES_secret_key", "SK_OLD") + + // fakeProviderEnumerableDriver returns the OLD canonical key as an + // enumerable resource so the prune step has something to look at and + // runs without the EnumerateAll-empty escape. + fakeProv := &fakeProviderEnumerableDriver{ + outputs: []*interfaces.ResourceOutput{ + { + Name: "coredump-deploy-key", + Type: "infra.spaces_key", + ProviderID: "AK_OLD", + Outputs: map[string]any{ + "access_key": "AK_OLD", + "created_at": "2026-05-01T00:00:00Z", + "name": "coredump-deploy-key", + }, + }, + { + Name: "coredump-deploy-key", + Type: "infra.spaces_key", + ProviderID: "AK_NEW_FROM_STUB", + Outputs: map[string]any{ + "access_key": "AK_NEW_FROM_STUB", + "created_at": "2026-05-10T01:41:58Z", + "name": "coredump-deploy-key", + }, + }, + }, + } + + var out bytes.Buffer + code := runInfraRotateAndPrune([]string{ + "--type", "infra.spaces_key", + "--name", "coredump-deploy-key", + "--config", cfgPath, + "--confirm", "--non-interactive", + // --prune-first=false to keep the test focused on the rotate step's + // rotation-result contract; pre-prune is exercised by other tests. + "--prune-first=false", + }, fakeProv, &out) + + if code != 0 { + t.Fatalf("rotate-and-prune failed: code=%d, out=%q\n\nThis is the regression: the rotation succeeded (stub generator was called and minted AK_NEW_FROM_STUB) but bootstrapSecrets returned an empty rotations slice because forceRotate was keyed by --name (\"coredump-deploy-key\") while bootstrapSecrets checks forceRotate[gen.Key] (\"SPACES\"). The CLI must translate --name→gen.Key.", code, out.String()) + } + + // Sanity: output should reflect rotation success (not skipped/created path). + got := out.String() + if !strings.Contains(got, "AK_NEW_FROM_STUB") { + t.Errorf("output should report new access_key from rotation; got: %s", got) + } +} + +// TestBootstrapSecrets_ForceRotate_NameOnlyMatch_StillRotatesAndAppends +// is the unit-level lock for the keying contract: when force-rotate +// is keyed by secrets.generate[].Key for a successful provider_credential +// force-rotate that hits an empty store, rotation must run and a +// RotationResult must be appended. Without this, the rotate-and-prune +// integration is fragile to internal CLI changes. +// +// Strict contract enforced by this PR: +// - forceRotate MUST be keyed by secrets.generate[].Key (canonical key). +// - bootstrapSecrets ignores entries that don't match a known generator +// Key (it does NOT accept generator .Name as a fallback key). +// - Name→Key translation is the CLI layer's responsibility — see +// buildRotateAndPruneForceRotateSet in infra_rotate_and_prune.go, +// which also enforces single-match + provider_credential constraints +// so this contract is unviolable from the operator side. +// +// Invariant pinned by this test: a successful force-rotate where Set +// committed MUST produce a RotationResult (not the false-negative the +// staging run hit on 2026-05-09). +func TestBootstrapSecrets_ForceRotate_AppendsRotationResultEvenWhenStoreEmpty(t *testing.T) { + withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { + return `{"access_key":"AK_NEW","secret_key":"SK_NEW","created_at":"2026-05-10T01:41:58Z"}`, nil + }) + + // Empty store — same shape as the staging-dispatch case where the OLD + // canonical sub-keys had been wiped before the dispatch (or never + // existed under the canonical-key shape because the prior config used + // the misconfigured suffixed-key shape). The bug-side surface is: + // even with an empty store, forceRotate[gen.Key]=true MUST drive the + // rotation appendpath, NOT the "doesn't exist → mint fresh" path that + // bypasses the rotations slice append. + p := newTrackingProvider(map[string]string{}) + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces", Name: "coredump-deploy-key"}, + }, + } + forceRotate := map[string]bool{"SPACES": true} + + _, rotations, err := bootstrapSecrets(context.Background(), p, cfg, forceRotate) + if err != nil { + t.Fatalf("bootstrapSecrets: %v", err) + } + + // The contract: force-rotate that successfully Set sub-keys MUST + // surface a RotationResult. Side effects committed without a + // RotationResult is the false-negative the staging run hit. + if len(rotations) != 1 { + t.Fatalf("expected len(rotations)==1 after successful force-rotate; got %d. Side effects committed (Set calls=%v) but rotations slice is empty — same false-negative as staging run 25616807427.", len(rotations), p.setCalls) + } + if rotations[0].Key != "SPACES" { + t.Errorf("rotations[0].Key = %q, want \"SPACES\"", rotations[0].Key) + } + if rotations[0].AccessKey != "AK_NEW" { + t.Errorf("rotations[0].AccessKey = %q, want \"AK_NEW\"", rotations[0].AccessKey) + } + if rotations[0].CreatedAt != "2026-05-10T01:41:58Z" { + t.Errorf("rotations[0].CreatedAt = %q, want \"2026-05-10T01:41:58Z\"", rotations[0].CreatedAt) + } + // And the side effects must have happened (proves the rotation ran). + if !containsSlice(p.setCalls, "SPACES_access_key") { + t.Errorf("Set(SPACES_access_key) must be called; setCalls=%v", p.setCalls) + } + if !containsSlice(p.setCalls, "SPACES_secret_key") { + t.Errorf("Set(SPACES_secret_key) must be called; setCalls=%v", p.setCalls) + } +} diff --git a/cmd/wfctl/infra_rotate_and_prune.go b/cmd/wfctl/infra_rotate_and_prune.go index 4d1461c4..32868878 100644 --- a/cmd/wfctl/infra_rotate_and_prune.go +++ b/cmd/wfctl/infra_rotate_and_prune.go @@ -156,6 +156,98 @@ func runInfraRotateAndPruneCmd(args []string) error { return fmt.Errorf("rotate-and-prune: no loaded provider implements EnumeratorAll") } +// buildRotateAndPruneForceRotateSet translates the cloud-side resource name +// passed via `wfctl infra rotate-and-prune --name ` into the canonical +// secrets.generate[].Key values that bootstrapSecrets keys its forceRotate +// map by. +// +// Per ADR 0023 + docs/runbooks/spaces-key-prune.md, `--name` is the +// cloud-side resource Name (e.g. "coredump-deploy-key"), matching the +// `secrets.generate[].name` field. bootstrapSecrets keys forceRotate by +// `gen.Key` (e.g. "SPACES"), so the CLI must translate or the force-rotate +// path is silently skipped — the false-negative that surfaced as staging +// run 25616807427 ("rotate-and-prune: expected 1 rotation result, got 0" +// AFTER side effects committed). +// +// Match precedence: +// 1. secrets.generate[].name == name → take that gen's Key +// 2. secrets.generate[].key == name → fallback for configs that omit Name +// +// Returns an error when no entry matches (typo guard, mirrors the +// buildForceRotateSet validation in infra_bootstrap.go) so the operator +// gets a fast-fail before bootstrap touches the store. +// +// Strict-contract enforcement (per Copilot review on PR 594): +// - Exactly ONE matching gen entry. Zero or two-plus matches reject. +// - Match MUST be type=provider_credential. Other rotatable types +// (random_hex, random_base64, random_alphanumeric) are intentionally +// rejected here because rotate-and-prune's downstream invariants +// (len(rotations)==1, prune step expects an old credential to revoke) +// are coherent only for provider_credential. Operators rotating +// non-provider_credential generators must use `wfctl infra bootstrap +// --force-rotate` directly. +// +// Without these guards a multi-Name config or a non-provider_credential +// --name target would either rotate multiple keys then fail count check +// (side effects committed but errored) OR rotate without appending a +// RotationResult (the false-negative class this PR was opened to fix). +func buildRotateAndPruneForceRotateSet(name string, cfg *SecretsConfig) (map[string]bool, error) { + if name == "" { + return nil, fmt.Errorf("--name is required") + } + if cfg == nil || len(cfg.Generate) == 0 { + return nil, fmt.Errorf("config has no secrets.generate entries; nothing to rotate for --name %q", name) + } + // Match precedence: secrets.generate[].name first (canonical per ADR 0023), + // then .key fallback for older configs / unit-test fixtures. + var matched []SecretGen + for _, gen := range cfg.Generate { + if gen.Name == name { + matched = append(matched, gen) + } + } + if len(matched) == 0 { + for _, gen := range cfg.Generate { + if gen.Key == name { + matched = append(matched, gen) + } + } + } + if len(matched) == 0 { + return nil, fmt.Errorf("no secrets.generate entry matches --name %q (matched neither .name nor .key)", name) + } + if len(matched) > 1 { + keys := make([]string, len(matched)) + for i, g := range matched { + keys[i] = g.Key + } + return nil, fmt.Errorf("--name %q matches multiple secrets.generate entries (keys: %v); rotate-and-prune supports exactly one provider_credential per dispatch — use `wfctl infra bootstrap --force-rotate` for multi-entry rotation", name, keys) + } + if matched[0].Type != "provider_credential" { + return nil, fmt.Errorf("--name %q matches secrets.generate entry of type %q; rotate-and-prune only operates on provider_credential entries (use `wfctl infra bootstrap --force-rotate` for other rotatable types)", name, matched[0].Type) + } + // Defense-in-depth: ensure the matched Key is unique across all + // secrets.generate[] entries. Without this, a config with two gens + // sharing the same Key (different Names) would set forceRotate[key]=true + // → bootstrapSecrets rotates BOTH → fails len(rotations)==1 after side + // effects committed (the same false-negative class this PR was opened + // to fix, just via Key collision instead of Name collision). + keyCount := 0 + var keyCollisionNames []string + for _, gen := range cfg.Generate { + if gen.Key == matched[0].Key { + keyCount++ + if gen.Name != "" { + keyCollisionNames = append(keyCollisionNames, gen.Name) + } + } + } + if keyCount > 1 { + return nil, fmt.Errorf("secrets.generate has %d entries with .key=%q (names: %v); rotate-and-prune requires Key uniqueness so the rotation count invariant holds", keyCount, matched[0].Key, keyCollisionNames) + } + return map[string]bool{matched[0].Key: true}, nil +} + // recoveryRecord is persisted to ${WFCTL_STATE_DIR:-$HOME/.wfctl}/last-rotation.json // after a successful rotate step, BEFORE the prune step. Used by the // `wfctl infra prune --recovery-from-last-rotation` flow (Task 19) to recover @@ -316,7 +408,18 @@ func runInfraRotateAndPrune(args []string, provider pruneProvider, w io.Writer) fmt.Fprintf(w, "rotate-and-prune: resolve secrets provider: %v\n", err) return 1 } - forceRotate := map[string]bool{name: true} + // Translate the cloud-side resource name (--name; per ADR 0023 + + // docs/runbooks/spaces-key-prune.md) into the canonical secrets.generate[].Key + // values that bootstrapSecrets keys forceRotate by. Without this translation + // forceRotate[gen.Key] is false for every generator and bootstrapSecrets + // silently bypasses the force-rotate code path (rotations slice stays empty + // even when the underlying generator + Set side effects committed) — the + // staging-dispatch false-negative surfaced 2026-05-09 (run 25616807427). + forceRotate, err := buildRotateAndPruneForceRotateSet(name, cfg) + if err != nil { + fmt.Fprintf(w, "rotate-and-prune: %v\n", err) + return 1 + } revoker, closer := resolveCredentialRevoker(ctx, configFile, cfg, forceRotate) if closer != nil { defer closer.Close() diff --git a/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go b/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go new file mode 100644 index 00000000..b3a66a69 --- /dev/null +++ b/cmd/wfctl/infra_rotate_and_prune_force_rotate_set_test.go @@ -0,0 +1,127 @@ +package main + +import ( + "strings" + "testing" +) + +// TestBuildRotateAndPruneForceRotateSet exercises the strict-contract +// validation added per Copilot review on PR #594: +// - Exactly one matching gen entry (zero or two-plus reject) +// - Match must be type=provider_credential +// - Returns map[Key]bool with exactly one entry +// +// Without these guards, the staging-rotation false-negative class returns: +// multi-match → multi-rotate → fails len(rotations)==1 after side effects; +// non-provider_credential match → rotates without appending RotationResult. +func TestBuildRotateAndPruneForceRotateSet(t *testing.T) { + cases := []struct { + name string + argName string + gens []SecretGen + wantKeys []string // empty = expect error + wantErrFrag string // substring expected in error + }{ + { + name: "happy_path_match_by_name", + argName: "coredump-deploy-key", + gens: []SecretGen{ + {Key: "SPACES", Type: "provider_credential", Name: "coredump-deploy-key"}, + }, + wantKeys: []string{"SPACES"}, + }, + { + name: "happy_path_fallback_match_by_key", + argName: "SPACES", + gens: []SecretGen{ + {Key: "SPACES", Type: "provider_credential"}, + }, + wantKeys: []string{"SPACES"}, + }, + { + name: "empty_name_rejected", + argName: "", + gens: []SecretGen{{Key: "SPACES", Type: "provider_credential"}}, + wantErrFrag: "--name is required", + }, + { + name: "no_generate_entries", + argName: "anything", + gens: nil, + wantErrFrag: "no secrets.generate entries", + }, + { + name: "no_match", + argName: "ghost-name", + gens: []SecretGen{{Key: "SPACES", Type: "provider_credential", Name: "coredump-deploy-key"}}, + wantErrFrag: "no secrets.generate entry matches", + }, + { + name: "multi_match_by_name_rejected", + argName: "shared-name", + gens: []SecretGen{ + {Key: "SPACES_A", Type: "provider_credential", Name: "shared-name"}, + {Key: "SPACES_B", Type: "provider_credential", Name: "shared-name"}, + }, + wantErrFrag: "matches multiple secrets.generate entries", + }, + { + name: "non_provider_credential_rejected", + argName: "session-secret", + gens: []SecretGen{ + {Key: "SESSION_SECRET", Type: "random_hex", Name: "session-secret"}, + }, + wantErrFrag: "rotate-and-prune only operates on provider_credential", + }, + { + name: "name_match_takes_precedence_over_key_match", + argName: "SPACES", + gens: []SecretGen{ + {Key: "OTHER", Type: "provider_credential", Name: "SPACES"}, // matched (Pass 1 by Name) + {Key: "SPACES", Type: "provider_credential"}, // not matched (Pass 1 hit, Pass 2 skipped) + }, + wantKeys: []string{"OTHER"}, + }, + { + // Defense-in-depth: even when --name picks a single gen, if the + // matched gen's Key is shared with another gen, forceRotate[key] + // would rotate both → fails len(rotations)==1. Reject up-front. + name: "matched_key_shared_with_another_gen_rejected", + argName: "primary-key", + gens: []SecretGen{ + {Key: "SHARED_KEY", Type: "provider_credential", Name: "primary-key"}, // matched + {Key: "SHARED_KEY", Type: "provider_credential", Name: "secondary-key"}, // collides on Key + }, + wantErrFrag: "rotate-and-prune requires Key uniqueness", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := &SecretsConfig{Generate: tc.gens} + got, err := buildRotateAndPruneForceRotateSet(tc.argName, cfg) + + if tc.wantErrFrag != "" { + if err == nil { + t.Fatalf("expected error containing %q; got nil (got=%v)", tc.wantErrFrag, got) + } + if !strings.Contains(err.Error(), tc.wantErrFrag) { + t.Fatalf("expected error containing %q; got %q", tc.wantErrFrag, err.Error()) + } + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != len(tc.wantKeys) { + t.Fatalf("expected %d keys, got %d (%v)", len(tc.wantKeys), len(got), got) + } + for _, k := range tc.wantKeys { + if !got[k] { + t.Fatalf("expected key %q in result, got %v", k, got) + } + } + }) + } +} diff --git a/cmd/wfctl/infra_rotate_and_prune_test.go b/cmd/wfctl/infra_rotate_and_prune_test.go index c47b5757..df79a857 100644 --- a/cmd/wfctl/infra_rotate_and_prune_test.go +++ b/cmd/wfctl/infra_rotate_and_prune_test.go @@ -57,8 +57,15 @@ func rotateAndPruneContains(haystack []string, needle string) bool { // for runInfraRotateAndPrune to load without erroring before reaching the // stubbed `bootstrapSecrets` hook. The config provides a `secrets:` block // (so parseSecretsConfig returns non-nil) with provider=env so -// resolveSecretsProvider builds without external dependencies. Returns the -// fixture's full path for use as `--config` arg. +// resolveSecretsProvider builds without external dependencies. +// +// Two generate entries are declared so callers can use either --name +// "test-key" (key + name both) or --name "canonical-name" (cloud-side +// name; key="canonical-key") — the latter exercises the name→key +// translation in buildRotateAndPruneForceRotateSet introduced to fix +// the staging-dispatch false-negative (run 25616807427, 2026-05-09). +// +// Returns the fixture's full path for use as `--config` arg. func writeMinimalRotationConfig(t *testing.T, tmpDir string) string { t.Helper() cfgPath := filepath.Join(tmpDir, "infra.yaml") @@ -71,6 +78,10 @@ func writeMinimalRotationConfig(t *testing.T, tmpDir string) string { type: provider_credential source: digitalocean.spaces name: test-key + - key: canonical-key + type: provider_credential + source: digitalocean.spaces + name: canonical-name ` if err := os.WriteFile(cfgPath, []byte(body), 0600); err != nil { t.Fatalf("write fixture infra.yaml: %v", err)