diff --git a/cmd/wfctl/infra_bootstrap.go b/cmd/wfctl/infra_bootstrap.go index 44e38224..66d3a348 100644 --- a/cmd/wfctl/infra_bootstrap.go +++ b/cmd/wfctl/infra_bootstrap.go @@ -132,8 +132,78 @@ func bootstrapDOSpacesBucket(ctx context.Context, bucket, region string) error { return nil } +// providerCredentialSubKeys lists the sub-key names produced by each +// provider_credential source. Existence checks must verify ALL of them so a +// partial prior write (e.g. one sub-key manually deleted) triggers a full +// regeneration rather than an incorrect skip. +var providerCredentialSubKeys = map[string][]string{ + "digitalocean.spaces": {"access_key", "secret_key"}, +} + +// generateSecret is the package-level hook used by bootstrapSecrets. Tests +// override it to exercise provider_credential code paths without reaching +// out to cloud APIs. +var generateSecret = secrets.GenerateSecret + +// expectedStoredKeys returns every key name that a completed generation of +// gen would have stored in the provider. For provider_credential with a +// known source, that is "_" for each subkey; for simple +// generators it is just gen.Key. +func expectedStoredKeys(gen SecretGen) []string { + if gen.Type == "provider_credential" { + if subs, ok := providerCredentialSubKeys[gen.Source]; ok { + out := make([]string, len(subs)) + for i, s := range subs { + out[i] = gen.Key + "_" + s + } + return out + } + // Unknown source — best-effort single probe. + return []string{gen.Key + "_access_key"} + } + return []string{gen.Key} +} + // bootstrapSecrets generates and stores secrets that don't already exist. func bootstrapSecrets(ctx context.Context, provider secrets.Provider, cfg *SecretsConfig) error { + // Cache List() as a set so repeated probes in a bootstrap run only hit + // the provider once and subsequent lookups are O(1). Resolved lazily on + // the first write-only Get. + var listSet map[string]struct{} + var listErr error + var listDone bool + lookupViaList := func(key string) (bool, error) { + if !listDone { + names, err := provider.List(ctx) + listErr = err + if err == nil { + listSet = make(map[string]struct{}, len(names)) + for _, n := range names { + listSet[n] = struct{}{} + } + } + listDone = true + } + if listErr != nil && !errors.Is(listErr, secrets.ErrUnsupported) { + return false, fmt.Errorf("list secrets to check %q: %w", key, listErr) + } + _, ok := listSet[key] + return ok, nil + } + secretExists := func(key string) (bool, error) { + _, err := provider.Get(ctx, key) + switch { + case err == nil: + return true, nil + case errors.Is(err, secrets.ErrNotFound): + return false, nil + case errors.Is(err, secrets.ErrUnsupported): + return lookupViaList(key) + default: + return false, fmt.Errorf("check secret %q: %w", key, err) + } + } + for _, gen := range cfg.Generate { // Build generator config from SecretGen fields. genConfig := map[string]any{} @@ -144,18 +214,31 @@ func bootstrapSecrets(ctx context.Context, provider secrets.Provider, cfg *Secre genConfig["source"] = gen.Source } - // Check if already set (skip if supported, ignore ErrUnsupported). - _, err := provider.Get(ctx, gen.Key) - if err == nil { + // Check that EVERY expected stored key is already present before + // skipping. provider_credential writes multiple sub-keys; if a prior + // write was partial or one sub-key was manually removed, we must + // regenerate to produce a usable credential. Without this loop, + // every run regenerates for write-only providers (GH Actions), and + // provider_credential regeneration orphans upstream credentials. + expected := expectedStoredKeys(gen) + allExist := true + for _, key := range expected { + present, err := secretExists(key) + if err != nil { + return err + } + if !present { + allExist = false + break + } + } + if allExist { fmt.Printf(" secret %q: already exists — skipped\n", gen.Key) continue } - if !errors.Is(err, secrets.ErrNotFound) && !errors.Is(err, secrets.ErrUnsupported) { - return fmt.Errorf("check secret %q: %w", gen.Key, err) - } // Generate the secret value. - value, err := secrets.GenerateSecret(ctx, gen.Type, genConfig) + value, err := generateSecret(ctx, gen.Type, genConfig) if err != nil { return fmt.Errorf("generate secret %q: %w", gen.Key, err) } diff --git a/cmd/wfctl/infra_bootstrap_secrets_test.go b/cmd/wfctl/infra_bootstrap_secrets_test.go new file mode 100644 index 00000000..416b7785 --- /dev/null +++ b/cmd/wfctl/infra_bootstrap_secrets_test.go @@ -0,0 +1,208 @@ +package main + +import ( + "context" + "encoding/json" + "testing" + + "github.com/GoCodeAlone/workflow/secrets" +) + +// writeOnlyProvider simulates a GitHub-style provider where Get is not +// supported but List returns known names. +type writeOnlyProvider struct { + existing []string + stored map[string]string + getCalls int + listCalls int + listOK bool +} + +func (p *writeOnlyProvider) Name() string { return "write-only-fake" } + +func (p *writeOnlyProvider) Get(_ context.Context, _ string) (string, error) { + p.getCalls++ + return "", secrets.ErrUnsupported +} + +func (p *writeOnlyProvider) Set(_ context.Context, key, value string) error { + if p.stored == nil { + p.stored = map[string]string{} + } + p.stored[key] = value + return nil +} + +func (p *writeOnlyProvider) Delete(_ context.Context, _ string) error { + return nil +} + +func (p *writeOnlyProvider) List(_ context.Context) ([]string, error) { + p.listCalls++ + if !p.listOK { + return nil, secrets.ErrUnsupported + } + return append([]string(nil), p.existing...), nil +} + +// withStubGenerator swaps the package-level generateSecret for the duration +// of the test, so provider_credential paths don't reach out to cloud APIs. +func withStubGenerator(t *testing.T, fn func(ctx context.Context, genType string, cfg map[string]any) (string, error)) { + t.Helper() + prev := generateSecret + generateSecret = fn + t.Cleanup(func() { generateSecret = prev }) +} + +// TestBootstrapSecrets_WriteOnlyProviderSkipsExisting verifies that when the +// provider is write-only (GitHub Actions), bootstrapSecrets consults List() +// and skips regeneration if the secret name already exists. Without this, +// every bootstrap run regenerates, and for provider_credential that orphans +// upstream credentials (e.g. DO Spaces access keys). +func TestBootstrapSecrets_WriteOnlyProviderSkipsExisting(t *testing.T) { + p := &writeOnlyProvider{ + existing: []string{"JWT_SECRET", "SPACES_access_key", "SPACES_secret_key"}, + listOK: true, + } + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "JWT_SECRET", Type: "random_hex", Length: 32}, + {Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces"}, + }, + } + if err := bootstrapSecrets(context.Background(), p, cfg); err != nil { + t.Fatalf("bootstrapSecrets: %v", err) + } + if len(p.stored) != 0 { + t.Fatalf("stored = %v, want empty (all secrets already exist)", p.stored) + } + if p.listCalls != 1 { + t.Fatalf("List called %d times, want 1 (should be cached)", p.listCalls) + } +} + +// TestBootstrapSecrets_WriteOnlyProviderGeneratesWhenMissing verifies the +// fallback still generates when List shows the name is absent. +func TestBootstrapSecrets_WriteOnlyProviderGeneratesWhenMissing(t *testing.T) { + p := &writeOnlyProvider{ + existing: []string{"UNRELATED"}, + listOK: true, + } + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "JWT_SECRET", Type: "random_hex", Length: 8}, + }, + } + if err := bootstrapSecrets(context.Background(), p, cfg); err != nil { + t.Fatalf("bootstrapSecrets: %v", err) + } + if _, ok := p.stored["JWT_SECRET"]; !ok { + t.Fatalf("JWT_SECRET was not stored; stored=%v", p.stored) + } +} + +// TestBootstrapSecrets_WriteOnlyProviderListUnsupported verifies that when +// both Get and List return ErrUnsupported, bootstrap regenerates (preserves +// prior behaviour for providers with no introspection at all). +func TestBootstrapSecrets_WriteOnlyProviderListUnsupported(t *testing.T) { + p := &writeOnlyProvider{listOK: false} + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "JWT_SECRET", Type: "random_hex", Length: 8}, + }, + } + if err := bootstrapSecrets(context.Background(), p, cfg); err != nil { + t.Fatalf("bootstrapSecrets: %v", err) + } + if len(p.stored) != 1 { + t.Fatalf("stored = %v, want 1 entry (List unsupported → regenerate)", p.stored) + } +} + +// TestBootstrapSecrets_ProviderCredentialAllSubKeysPresent verifies the +// provider_credential skip path: both access_key and secret_key sub-keys +// must exist before the generator is skipped. +func TestBootstrapSecrets_ProviderCredentialAllSubKeysPresent(t *testing.T) { + withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { + t.Fatal("generator must not be called when both sub-keys already exist") + return "", nil + }) + p := &writeOnlyProvider{ + existing: []string{"SPACES_access_key", "SPACES_secret_key"}, + listOK: true, + } + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces"}, + }, + } + if err := bootstrapSecrets(context.Background(), p, cfg); err != nil { + t.Fatalf("bootstrapSecrets: %v", err) + } + if len(p.stored) != 0 { + t.Fatalf("stored = %v, want empty", p.stored) + } +} + +// TestBootstrapSecrets_ProviderCredentialPartialRegenerates verifies that a +// partial prior write (one sub-key missing) triggers regeneration. +func TestBootstrapSecrets_ProviderCredentialPartialRegenerates(t *testing.T) { + withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { + out, _ := json.Marshal(map[string]string{ + "access_key": "new-access", + "secret_key": "new-secret", + }) + return string(out), nil + }) + // Only the access_key is present — the secret_key is missing, so the + // stored credential is unusable and bootstrap must regenerate. + p := &writeOnlyProvider{ + existing: []string{"SPACES_access_key"}, + listOK: true, + } + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces"}, + }, + } + if err := bootstrapSecrets(context.Background(), p, cfg); err != nil { + t.Fatalf("bootstrapSecrets: %v", err) + } + if got := p.stored["SPACES_access_key"]; got != "new-access" { + t.Errorf("SPACES_access_key = %q, want %q", got, "new-access") + } + if got := p.stored["SPACES_secret_key"]; got != "new-secret" { + t.Errorf("SPACES_secret_key = %q, want %q", got, "new-secret") + } +} + +// TestBootstrapSecrets_ProviderCredentialProbeIgnoresBareKey verifies that a +// plain secret named the same as the provider_credential key (without the +// _access_key / _secret_key suffixes) does not cause a false skip. +func TestBootstrapSecrets_ProviderCredentialProbeIgnoresBareKey(t *testing.T) { + generateCalls := 0 + withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { + generateCalls++ + out, _ := json.Marshal(map[string]string{ + "access_key": "a", + "secret_key": "b", + }) + return string(out), nil + }) + // "SPACES" is present, but the real sub-keys are not — must regenerate. + p := &writeOnlyProvider{ + existing: []string{"SPACES"}, + listOK: true, + } + cfg := &SecretsConfig{ + Generate: []SecretGen{ + {Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces"}, + }, + } + if err := bootstrapSecrets(context.Background(), p, cfg); err != nil { + t.Fatalf("bootstrapSecrets: %v", err) + } + if generateCalls != 1 { + t.Fatalf("generator called %d times, want 1", generateCalls) + } +}