From e9e4df1f67edef5583e891ee764e40cd94b51183 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 18:31:53 -0400 Subject: [PATCH 1/5] =?UTF-8?q?feat(secrets):=20fail-safe=20Reachability?= =?UTF-8?q?=20gate=20f(backend=20=C3=97=20exec-env)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 4.6 --- secrets/reachability.go | 98 +++++++++++++ secrets/reachability_test.go | 277 +++++++++++++++++++++++++++++++++++ 2 files changed, 375 insertions(+) create mode 100644 secrets/reachability.go create mode 100644 secrets/reachability_test.go diff --git a/secrets/reachability.go b/secrets/reachability.go new file mode 100644 index 000000000..fb919956a --- /dev/null +++ b/secrets/reachability.go @@ -0,0 +1,98 @@ +package secrets + +import ( + "context" + "fmt" +) + +// Result holds the outcome of a Reachability check. +// +// Reachable is true when the secrets provider can be reached from the given +// execution environment. Reason is empty when Reachable is true, and contains a +// human-readable (non-credential-leaking) explanation when Reachable is false. +type Result struct { + Reachable bool + Reason string +} + +// isLocalExecEnv returns true for execution environments that are considered +// local — i.e., the same host where the engine/agent runs. Local secrets +// backends (env, file, keychain) are always reachable from these environments. +func isLocalExecEnv(execEnv string) bool { + return execEnv == "" || execEnv == "local" || execEnv == "local-docker" +} + +// Reachability determines whether the secrets referenced by a plan can be read +// from the given execution environment (execEnv). It is designed to be +// fail-safe: when the answer is uncertain and the exec-env is remote, it +// returns unreachable rather than failing open. +// +// Classification uses a concrete type-switch rather than Name() string-matching. +// Name() is a config-assigned identifier that operators may override; the +// concrete type is the authoritative signal for capability detection. +// +// Rules (in order): +// +// 1. *EnvProvider, *FileProvider, *KeychainProvider — local backends. Always +// reachable regardless of execEnv. For KeychainProvider specifically: the +// OS keychain is only meaningful on the engine host, but for the gate's +// purpose we treat it as local-reachable because the engine (wherever it +// runs) is the process that started the pipeline. If a remote exec-env truly +// cannot access the keychain, the runtime will surface an error when the +// secret is actually read; the gate does not second-guess that. +// +// 2. *GitHubSecretsProvider — short-circuited to unreachable BEFORE any +// AccessChecker call. GitHub secrets are write-only: Get() returns +// ErrUnsupported and values are only injected at CI-job startup. No +// exec-env can read them at runtime. CheckAccess on the GitHub provider +// would succeed (it probes the public key endpoint, not Get), so probing +// it would give a false "reachable" verdict. +// +// 3. All other providers that implement AccessChecker — call CheckAccess. +// Reachable iff CheckAccess returns nil. +// +// 4. Providers that do NOT implement AccessChecker + remote execEnv — fail-safe +// unreachable ("reachability unknown for remote exec-env; assuming unreachable"). +// We never fail open for an unknown/unclassified backend in a remote context. +// +// 5. Providers that do NOT implement AccessChecker + local execEnv — treat as +// reachable (local operation; the runtime will surface access errors if any). +func Reachability(p Provider, execEnv string) Result { + switch p.(type) { + case *EnvProvider, *FileProvider, *KeychainProvider: + // Local backends — always reachable from any exec-env. + return Result{Reachable: true} + + case *GitHubSecretsProvider: + // Write-only short-circuit: GitHub secrets inject at CI time only. + // CheckAccess is intentionally NOT called — it would return a false + // positive (probing the public key endpoint, not read capability). + return Result{ + Reachable: false, + Reason: "write-only — secrets inject at CI time only; no exec-env can read them", + } + + default: + // For all remaining providers: check whether they implement AccessChecker. + if ac, ok := p.(AccessChecker); ok { + if err := ac.CheckAccess(context.Background()); err != nil { + return Result{ + Reachable: false, + Reason: fmt.Sprintf("access check failed: %s", err.Error()), + } + } + return Result{Reachable: true} + } + + // No AccessChecker implementation: + // - Local exec-env: treat as reachable (the runtime will catch errors). + // - Remote exec-env: fail-safe unreachable. + if isLocalExecEnv(execEnv) { + return Result{Reachable: true} + } + return Result{ + Reachable: false, + Reason: "reachability unknown for remote exec-env; assuming unreachable", + } + } +} diff --git a/secrets/reachability_test.go b/secrets/reachability_test.go new file mode 100644 index 000000000..7ea013b29 --- /dev/null +++ b/secrets/reachability_test.go @@ -0,0 +1,277 @@ +package secrets_test + +import ( + "context" + "errors" + "testing" + + "github.com/GoCodeAlone/workflow/secrets" +) + +// ─── stub types for Reachability tests ─────────────────────────────────────── + +// stubRemoteWithAccess implements Provider + AccessChecker and returns nil from +// CheckAccess (simulates a reachable remote backend). +type stubRemoteWithAccess struct { + checkAccessErr error + checkCalled bool +} + +func (s *stubRemoteWithAccess) Name() string { return "stub-remote-access" } +func (s *stubRemoteWithAccess) Get(_ context.Context, _ string) (string, error) { return "", nil } +func (s *stubRemoteWithAccess) Set(_ context.Context, _, _ string) error { return nil } +func (s *stubRemoteWithAccess) Delete(_ context.Context, _ string) error { return nil } +func (s *stubRemoteWithAccess) List(_ context.Context) ([]string, error) { return nil, nil } +func (s *stubRemoteWithAccess) CheckAccess(_ context.Context) error { + s.checkCalled = true + return s.checkAccessErr +} + +// stubRemoteNoAccess implements Provider only (no AccessChecker). +type stubRemoteNoAccess struct{} + +func (s *stubRemoteNoAccess) Name() string { return "stub-remote-no-access" } +func (s *stubRemoteNoAccess) Get(_ context.Context, _ string) (string, error) { return "", nil } +func (s *stubRemoteNoAccess) Set(_ context.Context, _, _ string) error { return nil } +func (s *stubRemoteNoAccess) Delete(_ context.Context, _ string) error { return nil } +func (s *stubRemoteNoAccess) List(_ context.Context) ([]string, error) { return nil, nil } + +// stubGitHubLikeWithAccess is a provider that behaves like GitHub: +// Get returns ErrUnsupported but ALSO implements AccessChecker (returns nil). +// This tests that the GitHub short-circuit fires before CheckAccess is consulted. +type stubGitHubLikeWithAccess struct { + checkCalled bool +} + +func (s *stubGitHubLikeWithAccess) Name() string { return "stub-github-like" } +func (s *stubGitHubLikeWithAccess) Get(_ context.Context, _ string) (string, error) { + return "", secrets.ErrUnsupported +} +func (s *stubGitHubLikeWithAccess) Set(_ context.Context, _, _ string) error { return nil } +func (s *stubGitHubLikeWithAccess) Delete(_ context.Context, _ string) error { return nil } +func (s *stubGitHubLikeWithAccess) List(_ context.Context) ([]string, error) { return nil, nil } +func (s *stubGitHubLikeWithAccess) CheckAccess(_ context.Context) error { + s.checkCalled = true + return nil +} + +// compile-time interface assertions +var _ secrets.Provider = (*stubRemoteWithAccess)(nil) +var _ secrets.AccessChecker = (*stubRemoteWithAccess)(nil) +var _ secrets.Provider = (*stubRemoteNoAccess)(nil) +var _ secrets.Provider = (*stubGitHubLikeWithAccess)(nil) +var _ secrets.AccessChecker = (*stubGitHubLikeWithAccess)(nil) + +// ─── TestReachability ───────────────────────────────────────────────────────── + +func TestReachability(t *testing.T) { + cases := []struct { + name string + provider secrets.Provider + execEnv string + wantReachable bool + wantReasonSub string // non-empty: the reason must contain this substring + }{ + // ── Local backends — always reachable ───────────────────────────────── + { + name: "EnvProvider local-exec", + provider: secrets.NewEnvProvider(""), + execEnv: "local", + wantReachable: true, + }, + { + name: "EnvProvider remote-exec", + provider: secrets.NewEnvProvider("APP_"), + execEnv: "remote", + wantReachable: true, + }, + { + name: "FileProvider local-exec", + provider: secrets.NewFileProvider("/tmp"), + execEnv: "", + wantReachable: true, + }, + { + name: "FileProvider remote-exec", + provider: secrets.NewFileProvider("/tmp"), + execEnv: "k8s-prod", + wantReachable: true, + }, + + // ── GitHubSecretsProvider — always unreachable (write-only short-circuit) ─ + // The stub has CheckAccess returning nil to prove it is NOT consulted. + { + name: "GitHubSecretsProvider write-only short-circuit (local exec)", + provider: newTestGitHubProvider(t), + execEnv: "local", + wantReachable: false, + wantReasonSub: "write-only", + }, + { + name: "GitHubSecretsProvider write-only short-circuit (remote exec)", + provider: newTestGitHubProvider(t), + execEnv: "remote", + wantReachable: false, + wantReasonSub: "write-only", + }, + + // ── Remote backends WITH AccessChecker ──────────────────────────────── + { + name: "remote with AccessChecker nil → reachable", + provider: &stubRemoteWithAccess{checkAccessErr: nil}, + execEnv: "remote", + wantReachable: true, + }, + { + name: "remote with AccessChecker err → unreachable", + provider: &stubRemoteWithAccess{checkAccessErr: errors.New("connection refused")}, + execEnv: "remote", + wantReachable: false, + wantReasonSub: "connection refused", + }, + + // ── Remote backends WITHOUT AccessChecker ───────────────────────────── + { + name: "remote WITHOUT AccessChecker + remote execEnv → fail-safe unreachable", + provider: &stubRemoteNoAccess{}, + execEnv: "remote", + wantReachable: false, + wantReasonSub: "reachability unknown", + }, + { + name: "remote WITHOUT AccessChecker + empty execEnv → reachable (local assumed)", + provider: &stubRemoteNoAccess{}, + execEnv: "", + wantReachable: true, + }, + { + name: "remote WITHOUT AccessChecker + 'local' execEnv → reachable", + provider: &stubRemoteNoAccess{}, + execEnv: "local", + wantReachable: true, + }, + { + name: "remote WITHOUT AccessChecker + 'local-docker' execEnv → reachable", + provider: &stubRemoteNoAccess{}, + execEnv: "local-docker", + wantReachable: true, + }, + + // ── Unknown provider type + remote execEnv → fail-safe ──────────────── + { + name: "unknown provider + remote execEnv → fail-safe unreachable", + provider: &stubRemoteNoAccess{}, + execEnv: "prod-k8s", + wantReachable: false, + wantReasonSub: "reachability unknown", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + result := secrets.Reachability(tc.provider, tc.execEnv) + if result.Reachable != tc.wantReachable { + t.Errorf("Reachable = %v, want %v (reason: %q)", result.Reachable, tc.wantReachable, result.Reason) + } + if tc.wantReasonSub != "" { + if result.Reason == "" { + t.Errorf("expected Reason containing %q, got empty", tc.wantReasonSub) + } else if !containsSubstr(result.Reason, tc.wantReasonSub) { + t.Errorf("expected Reason containing %q, got %q", tc.wantReasonSub, result.Reason) + } + } + if tc.wantReachable && result.Reason != "" { + t.Errorf("expected empty Reason when reachable, got %q", result.Reason) + } + }) + } +} + +// TestReachability_GitHubShortCircuit_CheckAccessNotConsulted proves that the +// GitHub write-only short-circuit fires BEFORE any AccessChecker call. We use a +// real *secrets.GitHubSecretsProvider for the type-switch, but we also verify the +// pattern with our stub whose CheckAccess is instrumented. +// +// For the stub: the key invariant is that a provider whose Get returns ErrUnsupported +// should be short-circuited — the stub is NOT a *GitHubSecretsProvider, so this +// test specifically covers the *GitHubSecretsProvider path via the real type. +func TestReachability_GitHubShortCircuit_CheckAccessNotConsulted(t *testing.T) { + p := newTestGitHubProvider(t) + + result := secrets.Reachability(p, "remote") + if result.Reachable { + t.Error("GitHubSecretsProvider must be unreachable (write-only)") + } + if !containsSubstr(result.Reason, "write-only") { + t.Errorf("expected reason to mention write-only, got %q", result.Reason) + } + // The short-circuit must fire without making any network call. + // We can't instrument the real GitHubSecretsProvider's CheckAccess, but we + // verify the stubGitHubLikeWithAccess (same shape) is NOT consulted: + stub := &stubGitHubLikeWithAccess{} + // stubGitHubLikeWithAccess is NOT *GitHubSecretsProvider, so the type-switch + // won't catch it — it will fall through to the generic remote path. + // This is intentional: the type-switch is explicitly for *GitHubSecretsProvider. + // Verify the stub does get its CheckAccess called (it's a regular remote-with-access). + result2 := secrets.Reachability(stub, "remote") + if !result2.Reachable { + t.Errorf("stub with CheckAccess→nil should be reachable, got reason: %q", result2.Reason) + } + if !stub.checkCalled { + t.Error("expected CheckAccess to be called on stub remote provider") + } +} + +// TestReachability_VaultProvider tests the real *VaultProvider path (remote with +// AccessChecker). Since VaultProvider requires a real vault.Client, we use the +// stubRemoteWithAccess to cover the functional branch; the actual *VaultProvider +// type-switch is confirmed by compilation. +func TestReachability_VaultAndAWS_WithAccessChecker(t *testing.T) { + // stubRemoteWithAccess covers the same code path as *VaultProvider and + // *AWSSecretsManagerProvider: it implements AccessChecker. + stub := &stubRemoteWithAccess{checkAccessErr: nil} + result := secrets.Reachability(stub, "prod-env") + if !result.Reachable { + t.Errorf("remote with CheckAccess→nil should be reachable, got: %q", result.Reason) + } + if !stub.checkCalled { + t.Error("expected CheckAccess to be called") + } + + stubErr := &stubRemoteWithAccess{checkAccessErr: errors.New("vault: token expired")} + result2 := secrets.Reachability(stubErr, "prod-env") + if result2.Reachable { + t.Error("remote with CheckAccess returning err should be unreachable") + } + if !containsSubstr(result2.Reason, "token expired") { + t.Errorf("expected reason to contain error text, got %q", result2.Reason) + } +} + +// ─── helpers ────────────────────────────────────────────────────────────────── + +// newTestGitHubProvider creates a real *secrets.GitHubSecretsProvider using a +// fake token so the type-switch in Reachability fires correctly. +// It sets the GITHUB_TOKEN env var temporarily so NewGitHubSecretsProvider +// doesn't reject the empty token. +func newTestGitHubProvider(t *testing.T) *secrets.GitHubSecretsProvider { + t.Helper() + t.Setenv("WORKFLOW_TEST_GH_TOKEN", "ghp_fake_token_for_type_switch_test") + p, err := secrets.NewGitHubSecretsProvider("owner/repo", "WORKFLOW_TEST_GH_TOKEN") + if err != nil { + t.Fatalf("NewGitHubSecretsProvider: %v", err) + } + return p +} + +func containsSubstr(s, sub string) bool { + if len(sub) == 0 { + return true + } + for i := 0; i <= len(s)-len(sub); i++ { + if s[i:i+len(sub)] == sub { + return true + } + } + return false +} From f7d5e67805d1d4916162eac2d09872370207524c Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 18:35:29 -0400 Subject: [PATCH 2/5] feat(iac): step.iac_secret_reachability pre-flight gate Co-Authored-By: Claude Sonnet 4.6 --- DOCUMENTATION.md | 1 + .../pipeline_step_iac_secret_reachability.go | 201 +++++++++ ...eline_step_iac_secret_reachability_test.go | 384 ++++++++++++++++++ plugins/platform/plugin.go | 5 +- 4 files changed, 590 insertions(+), 1 deletion(-) create mode 100644 module/pipeline_step_iac_secret_reachability.go create mode 100644 module/pipeline_step_iac_secret_reachability_test.go diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 2f8b80d56..9813f5dfa 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -283,6 +283,7 @@ flowchart TD | `step.iac_provider_apply` | Applies a plan after recomputing + validating `desired_hash` (stateless two-phase TOCTOU guard) | platform | | `step.iac_provider_destroy` | Destroys resources via an `iac.provider` plugin | platform | | `step.iac_provider_drift` | Detects drift via an `iac.provider` (optional `IaCProviderDriftDetector`; `supported:false` fallback) | platform | +| `step.iac_secret_reachability` | Pre-flight gate: checks whether `secret://` refs in plan specs are reachable from the chosen exec-env; returns per-ref verdict + `all_reachable` bool (fail-safe for remote exec-envs) | platform | | `step.tofu_init` | Initializes an OpenTofu working directory | platform | | `step.tofu_plan` | Creates an OpenTofu execution plan | platform | | `step.tofu_apply` | Applies OpenTofu changes to infrastructure | platform | diff --git a/module/pipeline_step_iac_secret_reachability.go b/module/pipeline_step_iac_secret_reachability.go new file mode 100644 index 000000000..fab35f633 --- /dev/null +++ b/module/pipeline_step_iac_secret_reachability.go @@ -0,0 +1,201 @@ +package module + +import ( + "context" + "fmt" + "sort" + "strings" + + "github.com/GoCodeAlone/modular" + "github.com/GoCodeAlone/workflow/iac/specparse" + "github.com/GoCodeAlone/workflow/interfaces" + "github.com/GoCodeAlone/workflow/secrets" +) + +// ─── step.iac_secret_reachability ──────────────────────────────────────────── + +// IaCSecretReachabilityStep pre-flights whether the secrets referenced in a set +// of IaC resource specs can be read from the chosen execution environment. +// +// It resolves the secrets provider from the app service registry (mirrors the +// resolveProvider pattern in SecretSetStep), gathers all distinct secret:// +// references from the spec configs (including nested maps and slices), calls +// secrets.Reachability once for the provider, and reports per-ref results. +// +// Output shape: +// +// { +// "secrets": [ {ref, reachable, reason}, ... ], +// "all_reachable": bool, +// } +// +// When there are zero secret:// refs, all_reachable is true with an empty list. +type IaCSecretReachabilityStep struct { + name string + provider string // secrets service name in the app registry + execEnv string + specs []interfaces.ResourceSpec + specsFrom string // dotted context path; mutually exclusive with specs + app modular.Application +} + +// NewIaCSecretReachabilityStepFactory returns a StepFactory for +// step.iac_secret_reachability. +func NewIaCSecretReachabilityStepFactory() StepFactory { + return func(name string, cfg map[string]any, app modular.Application) (PipelineStep, error) { + providerName, _ := cfg["provider"].(string) + if providerName == "" { + return nil, fmt.Errorf("iac_secret_reachability step %q: 'provider' is required", name) + } + + execEnv, _ := cfg["exec_env"].(string) + + specsFrom, _ := cfg["specs_from"].(string) + _, hasStaticSpecs := cfg["specs"] + if specsFrom != "" && hasStaticSpecs { + return nil, fmt.Errorf("iac_secret_reachability step %q: 'specs' and 'specs_from' are mutually exclusive", name) + } + + var specs []interfaces.ResourceSpec + if hasStaticSpecs { + var err error + specs, err = specparse.ParseResourceSpecs(cfg["specs"]) + if err != nil { + return nil, fmt.Errorf("iac_secret_reachability step %q: parse specs: %w", name, err) + } + } + + return &IaCSecretReachabilityStep{ + name: name, + provider: providerName, + execEnv: execEnv, + specs: specs, + specsFrom: specsFrom, + app: app, + }, nil + } +} + +func (s *IaCSecretReachabilityStep) Name() string { return s.name } + +func (s *IaCSecretReachabilityStep) Execute(ctx context.Context, pc *PipelineContext) (*StepResult, error) { + // Resolve specs: dynamic path takes precedence when specsFrom is configured. + specs := s.specs + if s.specsFrom != "" { + raw := resolveBodyFrom(s.specsFrom, pc) + var err error + specs, err = specparse.ParseResourceSpecs(raw) + if err != nil { + return nil, fmt.Errorf("iac_secret_reachability step %q: resolve specs_from %q: %w", s.name, s.specsFrom, err) + } + } + + // Resolve the secrets provider from the service registry. + p, err := resolveSecretsProvider(s.app, s.provider, s.name) + if err != nil { + return nil, err + } + + // Gather distinct secret:// refs across all spec configs. + refs := collectSecretRefs(specs) + + // Short-path: no secret refs → trivially reachable. + if len(refs) == 0 { + return &StepResult{Output: map[string]any{ + "secrets": []map[string]any{}, + "all_reachable": true, + }}, nil + } + + // Call Reachability once — the verdict is provider-level, not per-ref. + verdict := secrets.Reachability(p, s.execEnv) + + secretEntries := make([]map[string]any, 0, len(refs)) + for _, ref := range refs { + entry := map[string]any{ + "ref": ref, + "reachable": verdict.Reachable, + } + if !verdict.Reachable { + entry["reason"] = verdict.Reason + } + secretEntries = append(secretEntries, entry) + } + + allReachable := verdict.Reachable + + return &StepResult{Output: map[string]any{ + "secrets": secretEntries, + "all_reachable": allReachable, + }}, nil +} + +// resolveSecretsProvider looks up a secrets.Provider from the application +// service registry by name. It mirrors the resolveProvider pattern in +// SecretSetStep: first checks if the service directly implements secrets.Provider; +// if not, checks for a Provider() accessor (used by SecretsAWSModule, +// SecretsVaultModule, etc.). +func resolveSecretsProvider(app modular.Application, providerName, stepName string) (secrets.Provider, error) { + if app == nil { + return nil, fmt.Errorf("iac_secret_reachability step %q: no application context", stepName) + } + svc, ok := app.SvcRegistry()[providerName] + if !ok { + return nil, fmt.Errorf("iac_secret_reachability step %q: secrets service %q not found in registry", stepName, providerName) + } + + // Direct: service itself implements secrets.Provider. + if p, ok := svc.(secrets.Provider); ok { + return p, nil + } + + // Indirect: service exposes a Provider() accessor (e.g. SecretsAWSModule, + // SecretsVaultModule). + type providerAccessor interface { + Provider() secrets.Provider + } + if acc, ok := svc.(providerAccessor); ok { + p := acc.Provider() + if p == nil { + return nil, fmt.Errorf("iac_secret_reachability step %q: service %q exposes Provider() accessor but returned nil; module may not be started", stepName, providerName) + } + return p, nil + } + + return nil, fmt.Errorf("iac_secret_reachability step %q: service %q does not implement secrets.Provider directly or via Provider() accessor", stepName, providerName) +} + +// collectSecretRefs walks the Config map of each ResourceSpec and returns a +// sorted, deduplicated slice of all string values that start with secrets.SecretPrefix. +// It recurses into nested map[string]any and []any values so nested config +// trees are fully scanned. +func collectSecretRefs(specs []interfaces.ResourceSpec) []string { + seen := make(map[string]struct{}) + for _, spec := range specs { + collectFromValue(spec.Config, seen) + } + refs := make([]string, 0, len(seen)) + for ref := range seen { + refs = append(refs, ref) + } + sort.Strings(refs) + return refs +} + +// collectFromValue recursively extracts secret:// refs from an arbitrary value. +func collectFromValue(v any, seen map[string]struct{}) { + switch val := v.(type) { + case string: + if strings.HasPrefix(val, secrets.SecretPrefix) { + seen[val] = struct{}{} + } + case map[string]any: + for _, child := range val { + collectFromValue(child, seen) + } + case []any: + for _, item := range val { + collectFromValue(item, seen) + } + } +} diff --git a/module/pipeline_step_iac_secret_reachability_test.go b/module/pipeline_step_iac_secret_reachability_test.go new file mode 100644 index 000000000..8adab391e --- /dev/null +++ b/module/pipeline_step_iac_secret_reachability_test.go @@ -0,0 +1,384 @@ +package module_test + +import ( + "context" + "errors" + "testing" + + "github.com/GoCodeAlone/workflow/module" + "github.com/GoCodeAlone/workflow/secrets" +) + +// ─── secrets provider stubs for reachability step tests ────────────────────── + +// stubSecretsProviderWithAccess implements secrets.Provider + secrets.AccessChecker. +// It simulates a reachable (or unreachable, via checkErr) remote backend. +type stubSecretsProviderWithAccess struct { + checkErr error + checkCalled bool +} + +func (s *stubSecretsProviderWithAccess) Name() string { return "stub-vault" } +func (s *stubSecretsProviderWithAccess) Get(_ context.Context, _ string) (string, error) { + return "", nil +} +func (s *stubSecretsProviderWithAccess) Set(_ context.Context, _, _ string) error { return nil } +func (s *stubSecretsProviderWithAccess) Delete(_ context.Context, _ string) error { return nil } +func (s *stubSecretsProviderWithAccess) List(_ context.Context) ([]string, error) { return nil, nil } +func (s *stubSecretsProviderWithAccess) CheckAccess(_ context.Context) error { + s.checkCalled = true + return s.checkErr +} + +// stubSecretsProviderNoAccess implements secrets.Provider only (no AccessChecker). +type stubSecretsProviderNoAccess struct{} + +func (s *stubSecretsProviderNoAccess) Name() string { return "stub-no-access" } +func (s *stubSecretsProviderNoAccess) Get(_ context.Context, _ string) (string, error) { + return "", nil +} +func (s *stubSecretsProviderNoAccess) Set(_ context.Context, _, _ string) error { return nil } +func (s *stubSecretsProviderNoAccess) Delete(_ context.Context, _ string) error { return nil } +func (s *stubSecretsProviderNoAccess) List(_ context.Context) ([]string, error) { return nil, nil } + +// stubSecretsModuleAccessor wraps a secrets.Provider behind a Provider() accessor, +// mirroring how SecretsVaultModule / SecretsAWSModule work in production. +type stubSecretsModuleAccessor struct { + underlying secrets.Provider +} + +func (m *stubSecretsModuleAccessor) Provider() secrets.Provider { return m.underlying } + +// compile-time assertions +var _ secrets.Provider = (*stubSecretsProviderWithAccess)(nil) +var _ secrets.AccessChecker = (*stubSecretsProviderWithAccess)(nil) +var _ secrets.Provider = (*stubSecretsProviderNoAccess)(nil) + +// ─── TestSecretReachabilityStep ─────────────────────────────────────────────── + +// TestSecretReachabilityStep_VaultWithAccess verifies case (a): +// specs with a vault secret:// ref + stub vault provider whose CheckAccess→nil +// + execEnv "remote" → all_reachable true. +func TestSecretReachabilityStep_VaultWithAccess(t *testing.T) { + app := module.NewMockApplication() + stub := &stubSecretsProviderWithAccess{checkErr: nil} + // Register via Provider() accessor (mirrors production SecretsVaultModule wiring). + if err := app.RegisterService("my-vault", &stubSecretsModuleAccessor{underlying: stub}); err != nil { + t.Fatal(err) + } + + cfg := map[string]any{ + "provider": "my-vault", + "exec_env": "remote", + "specs": []any{ + map[string]any{ + "name": "my-db", + "type": "infra.database", + "config": map[string]any{ + "password": "secret://vault/my-db-password", + }, + }, + }, + } + + factory := module.NewIaCSecretReachabilityStepFactory() + step, err := factory("reach-step", cfg, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + result, err := step.Execute(context.Background(), &module.PipelineContext{}) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + allReachable, ok := result.Output["all_reachable"].(bool) + if !ok { + t.Fatalf("expected bool all_reachable, got %T: %v", result.Output["all_reachable"], result.Output["all_reachable"]) + } + if !allReachable { + t.Errorf("expected all_reachable=true, got false") + } + + secretsList, ok := result.Output["secrets"].([]map[string]any) + if !ok { + t.Fatalf("expected []map[string]any secrets, got %T", result.Output["secrets"]) + } + if len(secretsList) != 1 { + t.Errorf("expected 1 secret entry, got %d", len(secretsList)) + } + if secretsList[0]["ref"] != "secret://vault/my-db-password" { + t.Errorf("expected ref=secret://vault/my-db-password, got %v", secretsList[0]["ref"]) + } + if secretsList[0]["reachable"] != true { + t.Errorf("expected reachable=true for secret entry, got %v", secretsList[0]["reachable"]) + } +} + +// TestSecretReachabilityStep_GitHubProvider verifies case (b): +// a github-backed provider → all_reachable false with the write-only reason. +func TestSecretReachabilityStep_GitHubProvider(t *testing.T) { + app := module.NewMockApplication() + t.Setenv("WORKFLOW_TEST_GH_TOKEN2", "ghp_fake_token_step_test") + ghProvider, err := secrets.NewGitHubSecretsProvider("owner/repo", "WORKFLOW_TEST_GH_TOKEN2") + if err != nil { + t.Fatalf("NewGitHubSecretsProvider: %v", err) + } + if err := app.RegisterService("my-gh", &stubSecretsModuleAccessor{underlying: ghProvider}); err != nil { + t.Fatal(err) + } + + cfg := map[string]any{ + "provider": "my-gh", + "exec_env": "remote", + "specs": []any{ + map[string]any{ + "name": "deploy-key", + "type": "infra.secret", + "config": map[string]any{ + "token": "secret://gh/DEPLOY_TOKEN", + }, + }, + }, + } + + factory := module.NewIaCSecretReachabilityStepFactory() + step, err := factory("reach-step", cfg, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + result, err := step.Execute(context.Background(), &module.PipelineContext{}) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + allReachable, _ := result.Output["all_reachable"].(bool) + if allReachable { + t.Error("expected all_reachable=false for github-backed provider") + } + + secretsList, ok := result.Output["secrets"].([]map[string]any) + if !ok { + t.Fatalf("expected []map[string]any secrets, got %T", result.Output["secrets"]) + } + if len(secretsList) != 1 { + t.Errorf("expected 1 secret entry, got %d", len(secretsList)) + } + reason, _ := secretsList[0]["reason"].(string) + if !containsString(reason, "write-only") { + t.Errorf("expected reason to contain 'write-only', got %q", reason) + } +} + +// TestSecretReachabilityStep_VaultNoAccessChecker_Remote verifies case (c): +// a vault provider without AccessChecker + execEnv "remote" → all_reachable false (fail-safe). +func TestSecretReachabilityStep_VaultNoAccessChecker_Remote(t *testing.T) { + app := module.NewMockApplication() + stub := &stubSecretsProviderNoAccess{} + if err := app.RegisterService("my-vault-noaccess", &stubSecretsModuleAccessor{underlying: stub}); err != nil { + t.Fatal(err) + } + + cfg := map[string]any{ + "provider": "my-vault-noaccess", + "exec_env": "remote", + "specs": []any{ + map[string]any{ + "name": "infra-key", + "type": "infra.database", + "config": map[string]any{ + "key": "secret://unknown/backend-key", + }, + }, + }, + } + + factory := module.NewIaCSecretReachabilityStepFactory() + step, err := factory("reach-step", cfg, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + result, err := step.Execute(context.Background(), &module.PipelineContext{}) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + allReachable, _ := result.Output["all_reachable"].(bool) + if allReachable { + t.Error("expected all_reachable=false for fail-safe (no AccessChecker + remote env)") + } + + secretsList, ok := result.Output["secrets"].([]map[string]any) + if !ok { + t.Fatalf("expected []map[string]any secrets, got %T", result.Output["secrets"]) + } + if len(secretsList) != 1 { + t.Errorf("expected 1 secret entry, got %d", len(secretsList)) + } + reason, _ := secretsList[0]["reason"].(string) + if !containsString(reason, "reachability unknown") { + t.Errorf("expected reason to contain 'reachability unknown', got %q", reason) + } +} + +// TestSecretReachabilityStep_NoSecretRefs verifies case (d): +// specs with NO secret refs → all_reachable true, empty list. +func TestSecretReachabilityStep_NoSecretRefs(t *testing.T) { + app := module.NewMockApplication() + stub := &stubSecretsProviderWithAccess{checkErr: nil} + if err := app.RegisterService("my-vault", &stubSecretsModuleAccessor{underlying: stub}); err != nil { + t.Fatal(err) + } + + cfg := map[string]any{ + "provider": "my-vault", + "exec_env": "remote", + "specs": []any{ + map[string]any{ + "name": "plain-resource", + "type": "infra.database", + "config": map[string]any{ + "size": "small", + "region": "us-east-1", + }, + }, + }, + } + + factory := module.NewIaCSecretReachabilityStepFactory() + step, err := factory("reach-step", cfg, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + result, err := step.Execute(context.Background(), &module.PipelineContext{}) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + allReachable, ok := result.Output["all_reachable"].(bool) + if !ok { + t.Fatalf("expected bool all_reachable, got %T", result.Output["all_reachable"]) + } + if !allReachable { + t.Error("expected all_reachable=true when no secret refs present") + } + + secretsList, ok := result.Output["secrets"].([]map[string]any) + if !ok { + t.Fatalf("expected []map[string]any secrets, got %T", result.Output["secrets"]) + } + if len(secretsList) != 0 { + t.Errorf("expected empty secrets list, got %d entries", len(secretsList)) + } +} + +// TestSecretReachabilityStep_MultipleRefs_SameProvider verifies that multiple +// distinct secret:// refs in specs are all reported individually but the provider +// check fires only once (provider-level verdict). +func TestSecretReachabilityStep_MultipleRefs_SameProvider(t *testing.T) { + app := module.NewMockApplication() + stub := &stubSecretsProviderWithAccess{checkErr: errors.New("unreachable vault")} + if err := app.RegisterService("my-vault", &stubSecretsModuleAccessor{underlying: stub}); err != nil { + t.Fatal(err) + } + + cfg := map[string]any{ + "provider": "my-vault", + "exec_env": "remote", + "specs": []any{ + map[string]any{ + "name": "db", + "type": "infra.database", + "config": map[string]any{ + "password": "secret://vault/db-pass", + "username": "secret://vault/db-user", + }, + }, + }, + } + + factory := module.NewIaCSecretReachabilityStepFactory() + step, err := factory("reach-step", cfg, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + result, err := step.Execute(context.Background(), &module.PipelineContext{}) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + allReachable, _ := result.Output["all_reachable"].(bool) + if allReachable { + t.Error("expected all_reachable=false when vault is unreachable") + } + + secretsList, ok := result.Output["secrets"].([]map[string]any) + if !ok { + t.Fatalf("expected []map[string]any secrets, got %T", result.Output["secrets"]) + } + if len(secretsList) != 2 { + t.Errorf("expected 2 secret entries (one per distinct ref), got %d", len(secretsList)) + } + for _, entry := range secretsList { + if entry["reachable"] != false { + t.Errorf("expected all entries unreachable when vault is down, got %v", entry["reachable"]) + } + } +} + +// TestSecretReachabilityStep_Factory_RequiresProvider verifies factory validation. +func TestSecretReachabilityStep_Factory_RequiresProvider(t *testing.T) { + factory := module.NewIaCSecretReachabilityStepFactory() + _, err := factory("reach-step", map[string]any{}, nil) + if err == nil { + t.Fatal("expected factory error when 'provider' missing") + } + if !containsString(err.Error(), "provider") { + t.Errorf("expected error to mention 'provider', got: %v", err) + } +} + +// TestSecretReachabilityStep_DirectProvider verifies that the step works when +// the service directly implements secrets.Provider (not via Provider() accessor). +func TestSecretReachabilityStep_DirectProvider(t *testing.T) { + app := module.NewMockApplication() + stub := &stubSecretsProviderWithAccess{checkErr: nil} + // Register the provider directly (not wrapped). + if err := app.RegisterService("direct-provider", stub); err != nil { + t.Fatal(err) + } + + cfg := map[string]any{ + "provider": "direct-provider", + "exec_env": "remote", + "specs": []any{ + map[string]any{ + "name": "my-service", + "type": "infra.service", + "config": map[string]any{ + "api_key": "secret://direct/api-key", + }, + }, + }, + } + + factory := module.NewIaCSecretReachabilityStepFactory() + step, err := factory("reach-step", cfg, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + result, err := step.Execute(context.Background(), &module.PipelineContext{}) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + allReachable, _ := result.Output["all_reachable"].(bool) + if !allReachable { + t.Error("expected all_reachable=true for direct provider with CheckAccess→nil") + } +} diff --git a/plugins/platform/plugin.go b/plugins/platform/plugin.go index 072c1fb5e..9884ea015 100644 --- a/plugins/platform/plugin.go +++ b/plugins/platform/plugin.go @@ -44,7 +44,7 @@ func New() *Plugin { Description: "Platform infrastructure modules, workflow handler, reconciliation trigger, and template step", Tier: plugin.TierCore, ModuleTypes: []string{"platform.provider", "platform.resource", "platform.context", "platform.kubernetes", "platform.dns", "platform.region", "platform.region_router", "iac.state", "app.container", "argo.workflows"}, - StepTypes: []string{"step.platform_template", "step.k8s_plan", "step.k8s_apply", "step.k8s_status", "step.k8s_destroy", "step.iac_plan", "step.iac_apply", "step.iac_status", "step.iac_destroy", "step.iac_drift_detect", "step.iac_provider_list", "step.iac_provider_catalog", "step.iac_provider_plan", "step.iac_provider_apply", "step.iac_provider_destroy", "step.iac_provider_drift", "step.dns_plan", "step.dns_apply", "step.dns_status", "step.app_deploy", "step.app_status", "step.app_rollback", "step.region_deploy", "step.region_promote", "step.region_failover", "step.region_status", "step.region_weight", "step.region_sync", "step.argo_submit", "step.argo_status", "step.argo_logs", "step.argo_delete", "step.argo_list"}, + StepTypes: []string{"step.platform_template", "step.k8s_plan", "step.k8s_apply", "step.k8s_status", "step.k8s_destroy", "step.iac_plan", "step.iac_apply", "step.iac_status", "step.iac_destroy", "step.iac_drift_detect", "step.iac_provider_list", "step.iac_provider_catalog", "step.iac_provider_plan", "step.iac_provider_apply", "step.iac_provider_destroy", "step.iac_provider_drift", "step.iac_secret_reachability", "step.dns_plan", "step.dns_apply", "step.dns_status", "step.app_deploy", "step.app_status", "step.app_rollback", "step.region_deploy", "step.region_promote", "step.region_failover", "step.region_status", "step.region_weight", "step.region_sync", "step.argo_submit", "step.argo_status", "step.argo_logs", "step.argo_delete", "step.argo_list"}, TriggerTypes: []string{"reconciliation"}, WorkflowTypes: []string{"platform"}, }, @@ -152,6 +152,9 @@ func (p *Plugin) StepFactories() map[string]plugin.StepFactory { "step.iac_provider_drift": func(name string, cfg map[string]any, app modular.Application) (any, error) { return module.NewIaCProviderDriftStepFactory()(name, cfg, app) }, + "step.iac_secret_reachability": func(name string, cfg map[string]any, app modular.Application) (any, error) { + return module.NewIaCSecretReachabilityStepFactory()(name, cfg, app) + }, "step.dns_plan": func(name string, cfg map[string]any, app modular.Application) (any, error) { return module.NewDNSPlanStepFactory()(name, cfg, app) }, From 7a6d11da549689d48ef595cd86682fc9cf143c4f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 18:48:02 -0400 Subject: [PATCH 3/5] fix(secrets,iac): ctx propagation + host-local fail-safe for remote + recursion coverage (review) PR4 review fixes: - CRITICAL: Reachability now takes ctx and passes it into AccessChecker.CheckAccess so a slow/unreachable vault/aws probe is bounded by the route/pipeline deadline instead of hanging the pre-flight. step.iac_secret_reachability propagates its Execute ctx. - IMPORTANT: host-local backends (env/file/keychain) are now reachable ONLY for a local exec-env. For a remote exec-env they are fail-safe unreachable (ADR 0017: the remote agent resolves its own secrets; engine-host env/file/keychain are not verifiable on a remote runner). Closes a fail-open hole. - IMPORTANT: collectFromValue now also recurses typed []string (programmatically- built specs), with a dedicated TestCollectSecretRefs covering flat/map/[]any-of- strings/[]any-of-maps/[]string/double-nesting + dedup. - MINORS: assert CheckAccess called (VaultWithAccess) and called exactly once (MultipleRefs, proving provider-level verdict); add specs_from + specs/specs_from mutual-exclusion tests; DOCUMENTATION row notes provider-level verdict. Co-Authored-By: Claude Opus 4.8 (1M context) --- DOCUMENTATION.md | 2 +- .../pipeline_step_iac_secret_reachability.go | 17 ++- ...p_iac_secret_reachability_internal_test.go | 93 +++++++++++++++ ...eline_step_iac_secret_reachability_test.go | 91 +++++++++++++++ secrets/reachability.go | 63 ++++++++--- secrets/reachability_test.go | 107 ++++++++++++++++-- 6 files changed, 341 insertions(+), 32 deletions(-) create mode 100644 module/pipeline_step_iac_secret_reachability_internal_test.go diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 9813f5dfa..da60d0c1f 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -283,7 +283,7 @@ flowchart TD | `step.iac_provider_apply` | Applies a plan after recomputing + validating `desired_hash` (stateless two-phase TOCTOU guard) | platform | | `step.iac_provider_destroy` | Destroys resources via an `iac.provider` plugin | platform | | `step.iac_provider_drift` | Detects drift via an `iac.provider` (optional `IaCProviderDriftDetector`; `supported:false` fallback) | platform | -| `step.iac_secret_reachability` | Pre-flight gate: checks whether `secret://` refs in plan specs are reachable from the chosen exec-env; returns per-ref verdict + `all_reachable` bool (fail-safe for remote exec-envs) | platform | +| `step.iac_secret_reachability` | Pre-flight gate: checks whether `secret://` refs in plan specs are reachable from the chosen exec-env. The verdict is provider-level (one `CheckAccess` probe; the same result is reported per distinct ref); returns `all_reachable` bool. Fail-safe for remote exec-envs (host-local backends unverifiable per ADR 0017, unknown backends, and probe failure → unreachable) | platform | | `step.tofu_init` | Initializes an OpenTofu working directory | platform | | `step.tofu_plan` | Creates an OpenTofu execution plan | platform | | `step.tofu_apply` | Applies OpenTofu changes to infrastructure | platform | diff --git a/module/pipeline_step_iac_secret_reachability.go b/module/pipeline_step_iac_secret_reachability.go index fab35f633..e94c741c9 100644 --- a/module/pipeline_step_iac_secret_reachability.go +++ b/module/pipeline_step_iac_secret_reachability.go @@ -108,7 +108,9 @@ func (s *IaCSecretReachabilityStep) Execute(ctx context.Context, pc *PipelineCon } // Call Reachability once — the verdict is provider-level, not per-ref. - verdict := secrets.Reachability(p, s.execEnv) + // Propagate the Execute ctx so a slow/unreachable backend probe is bounded + // by the pipeline/route deadline rather than hanging the pre-flight. + verdict := secrets.Reachability(ctx, p, s.execEnv) secretEntries := make([]map[string]any, 0, len(refs)) for _, ref := range refs { @@ -167,8 +169,8 @@ func resolveSecretsProvider(app modular.Application, providerName, stepName stri // collectSecretRefs walks the Config map of each ResourceSpec and returns a // sorted, deduplicated slice of all string values that start with secrets.SecretPrefix. -// It recurses into nested map[string]any and []any values so nested config -// trees are fully scanned. +// It recurses into nested map[string]any, []any, and typed []string values so +// both YAML-decoded and programmatically-built spec configs are fully scanned. func collectSecretRefs(specs []interfaces.ResourceSpec) []string { seen := make(map[string]struct{}) for _, spec := range specs { @@ -183,12 +185,21 @@ func collectSecretRefs(specs []interfaces.ResourceSpec) []string { } // collectFromValue recursively extracts secret:// refs from an arbitrary value. +// It handles both YAML-decoded shapes (map[string]any, []any) and typed Go +// shapes built programmatically (notably []string), since ResourceSpec.Config +// may carry either when specs are constructed in code rather than parsed. func collectFromValue(v any, seen map[string]struct{}) { switch val := v.(type) { case string: if strings.HasPrefix(val, secrets.SecretPrefix) { seen[val] = struct{}{} } + case []string: + for _, item := range val { + if strings.HasPrefix(item, secrets.SecretPrefix) { + seen[item] = struct{}{} + } + } case map[string]any: for _, child := range val { collectFromValue(child, seen) diff --git a/module/pipeline_step_iac_secret_reachability_internal_test.go b/module/pipeline_step_iac_secret_reachability_internal_test.go new file mode 100644 index 000000000..03a1bb37a --- /dev/null +++ b/module/pipeline_step_iac_secret_reachability_internal_test.go @@ -0,0 +1,93 @@ +package module + +import ( + "reflect" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestCollectSecretRefs exercises collectSecretRefs/collectFromValue across every +// container shape a ResourceSpec.Config can carry: a flat string ref, a ref +// nested inside a map[string]any, a ref inside an []any of strings, a ref inside +// an []any of maps, a ref inside a typed []string (programmatically-built specs), +// and double-nesting. It also asserts dedup + sorted order. +func TestCollectSecretRefs(t *testing.T) { + specs := []interfaces.ResourceSpec{ + { + Name: "r1", + Type: "infra.database", + Config: map[string]any{ + // flat string ref + "password": "secret://vault/db-pass", + // non-ref string (ignored) + "region": "us-east-1", + // ref nested in a map[string]any + "nested": map[string]any{ + "token": "secret://vault/nested-token", + }, + // ref in an []any of strings + "list_any_strings": []any{ + "plain", + "secret://vault/from-anylist", + }, + // ref in an []any of maps + "list_any_maps": []any{ + map[string]any{"key": "secret://vault/from-anymap"}, + }, + // ref in a typed []string (programmatically-built) + "list_typed_strings": []string{ + "not-a-ref", + "secret://vault/from-typedlist", + }, + // double-nesting: map → slice → map → string + "deep": map[string]any{ + "items": []any{ + map[string]any{ + "deeper": map[string]any{ + "v": "secret://vault/deep-ref", + }, + }, + }, + }, + }, + }, + { + Name: "r2", + Type: "infra.database", + Config: map[string]any{ + // duplicate of r1's password — must be deduped + "password": "secret://vault/db-pass", + // a unique ref in r2 + "api": "secret://vault/r2-api", + }, + }, + } + + got := collectSecretRefs(specs) + + want := []string{ + "secret://vault/db-pass", + "secret://vault/deep-ref", + "secret://vault/from-anylist", + "secret://vault/from-anymap", + "secret://vault/from-typedlist", + "secret://vault/nested-token", + "secret://vault/r2-api", + } + + if !reflect.DeepEqual(got, want) { + t.Errorf("collectSecretRefs mismatch:\n got: %#v\n want: %#v", got, want) + } +} + +// TestCollectSecretRefs_NoRefs asserts an empty (non-nil) slice when no refs exist. +func TestCollectSecretRefs_NoRefs(t *testing.T) { + specs := []interfaces.ResourceSpec{ + {Name: "r1", Type: "infra.database", Config: map[string]any{"size": "small"}}, + } + got := collectSecretRefs(specs) + if len(got) != 0 { + t.Errorf("expected no refs, got %#v", got) + } +} diff --git a/module/pipeline_step_iac_secret_reachability_test.go b/module/pipeline_step_iac_secret_reachability_test.go index 8adab391e..24648989f 100644 --- a/module/pipeline_step_iac_secret_reachability_test.go +++ b/module/pipeline_step_iac_secret_reachability_test.go @@ -16,6 +16,7 @@ import ( type stubSecretsProviderWithAccess struct { checkErr error checkCalled bool + checkCount int } func (s *stubSecretsProviderWithAccess) Name() string { return "stub-vault" } @@ -27,6 +28,7 @@ func (s *stubSecretsProviderWithAccess) Delete(_ context.Context, _ string) erro func (s *stubSecretsProviderWithAccess) List(_ context.Context) ([]string, error) { return nil, nil } func (s *stubSecretsProviderWithAccess) CheckAccess(_ context.Context) error { s.checkCalled = true + s.checkCount++ return s.checkErr } @@ -92,6 +94,11 @@ func TestSecretReachabilityStep_VaultWithAccess(t *testing.T) { t.Fatalf("Execute error: %v", err) } + // The AccessChecker probe must have been consulted (remote backend path). + if !stub.checkCalled { + t.Error("expected CheckAccess to be called on the vault provider") + } + allReachable, ok := result.Output["all_reachable"].(bool) if !ok { t.Fatalf("expected bool all_reachable, got %T: %v", result.Output["all_reachable"], result.Output["all_reachable"]) @@ -316,6 +323,12 @@ func TestSecretReachabilityStep_MultipleRefs_SameProvider(t *testing.T) { t.Error("expected all_reachable=false when vault is unreachable") } + // The verdict is provider-level: CheckAccess must fire EXACTLY ONCE even + // though two distinct refs are reported. + if stub.checkCount != 1 { + t.Errorf("expected CheckAccess called exactly once (provider-level verdict), got %d", stub.checkCount) + } + secretsList, ok := result.Output["secrets"].([]map[string]any) if !ok { t.Fatalf("expected []map[string]any secrets, got %T", result.Output["secrets"]) @@ -382,3 +395,81 @@ func TestSecretReachabilityStep_DirectProvider(t *testing.T) { t.Error("expected all_reachable=true for direct provider with CheckAccess→nil") } } + +// TestSecretReachabilityStep_SpecsFromContext verifies the dynamic specs_from +// path: specs are resolved from the pipeline context at Execute time, and their +// secret:// refs are collected and checked. +func TestSecretReachabilityStep_SpecsFromContext(t *testing.T) { + app := module.NewMockApplication() + stub := &stubSecretsProviderWithAccess{checkErr: nil} + if err := app.RegisterService("my-vault", &stubSecretsModuleAccessor{underlying: stub}); err != nil { + t.Fatal(err) + } + + factory := module.NewIaCSecretReachabilityStepFactory() + step, err := factory("reach-step", map[string]any{ + "provider": "my-vault", + "exec_env": "remote", + "specs_from": "steps.parse-request.body.specs", + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := &module.PipelineContext{ + StepOutputs: map[string]map[string]any{ + "parse-request": { + "body": map[string]any{ + "specs": []any{ + map[string]any{ + "name": "vault-db", + "type": "infra.database", + "config": map[string]any{ + "password": "secret://vault/dynamic-pass", + }, + }, + }, + }, + }, + }, + } + + result, err := step.Execute(context.Background(), pc) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + if !stub.checkCalled { + t.Error("expected CheckAccess to be called for specs_from path") + } + allReachable, _ := result.Output["all_reachable"].(bool) + if !allReachable { + t.Error("expected all_reachable=true for reachable vault via specs_from") + } + secretsList, ok := result.Output["secrets"].([]map[string]any) + if !ok { + t.Fatalf("expected []map[string]any secrets, got %T", result.Output["secrets"]) + } + if len(secretsList) != 1 || secretsList[0]["ref"] != "secret://vault/dynamic-pass" { + t.Errorf("expected one ref secret://vault/dynamic-pass from specs_from, got %#v", secretsList) + } +} + +// TestSecretReachabilityStep_Factory_SpecsAndSpecsFromMutualExclusion verifies +// that setting both 'specs' and 'specs_from' is rejected at factory time. +func TestSecretReachabilityStep_Factory_SpecsAndSpecsFromMutualExclusion(t *testing.T) { + factory := module.NewIaCSecretReachabilityStepFactory() + _, err := factory("reach-step", map[string]any{ + "provider": "my-vault", + "specs_from": "steps.parse-request.body.specs", + "specs": []any{ + map[string]any{"name": "x", "type": "infra.database"}, + }, + }, nil) + if err == nil { + t.Fatal("expected factory error when both specs and specs_from are set") + } + if !containsString(err.Error(), "mutually exclusive") { + t.Errorf("expected 'mutually exclusive' error, got: %v", err) + } +} diff --git a/secrets/reachability.go b/secrets/reachability.go index fb919956a..30bc3aa4f 100644 --- a/secrets/reachability.go +++ b/secrets/reachability.go @@ -16,30 +16,54 @@ type Result struct { } // isLocalExecEnv returns true for execution environments that are considered -// local — i.e., the same host where the engine/agent runs. Local secrets -// backends (env, file, keychain) are always reachable from these environments. +// local — i.e., the same host where the engine (and its configured secrets +// providers) run. Host-local secrets backends (env, file, keychain) are only +// verifiably reachable from these environments. func isLocalExecEnv(execEnv string) bool { return execEnv == "" || execEnv == "local" || execEnv == "local-docker" } +// hostLocalKind returns a short label for a host-local backend type, or "" if p +// is not a host-local backend. Used to classify and to build the fail-safe +// reason string. +func hostLocalKind(p Provider) string { + switch p.(type) { + case *EnvProvider: + return "env" + case *FileProvider: + return "file" + case *KeychainProvider: + return "keychain" + default: + return "" + } +} + // Reachability determines whether the secrets referenced by a plan can be read // from the given execution environment (execEnv). It is designed to be // fail-safe: when the answer is uncertain and the exec-env is remote, it // returns unreachable rather than failing open. // +// ctx bounds any backend probe (AccessChecker.CheckAccess). Callers MUST pass a +// deadline-bearing context (e.g. the pipeline/route ctx) so a slow or +// unreachable remote backend cannot hang the pre-flight (vault default ~60s, +// aws ~10s) past the request deadline. +// // Classification uses a concrete type-switch rather than Name() string-matching. // Name() is a config-assigned identifier that operators may override; the // concrete type is the authoritative signal for capability detection. // // Rules (in order): // -// 1. *EnvProvider, *FileProvider, *KeychainProvider — local backends. Always -// reachable regardless of execEnv. For KeychainProvider specifically: the -// OS keychain is only meaningful on the engine host, but for the gate's -// purpose we treat it as local-reachable because the engine (wherever it -// runs) is the process that started the pipeline. If a remote exec-env truly -// cannot access the keychain, the runtime will surface an error when the -// secret is actually read; the gate does not second-guess that. +// 1. *EnvProvider, *FileProvider, *KeychainProvider — host-local backends. +// Reachable ONLY when execEnv is local ("" | "local" | "local-docker"). +// For a REMOTE exec-env these are fail-safe UNREACHABLE: under the +// agent-side-resolution model (ADR 0017), a remote exec-env resolves +// secrets with the remote agent's OWN provider, so the engine cannot vouch +// that the engine-host's env vars / files / OS keychain exist on the remote +// runner (e.g. a macOS keychain entry is not present on a remote Linux +// runner). This is intentionally conservative: it is the safe default until +// the agent-probe hardening filed in ADR 0017 lands. // // 2. *GitHubSecretsProvider — short-circuited to unreachable BEFORE any // AccessChecker call. GitHub secrets are write-only: Get() returns @@ -48,7 +72,7 @@ func isLocalExecEnv(execEnv string) bool { // would succeed (it probes the public key endpoint, not Get), so probing // it would give a false "reachable" verdict. // -// 3. All other providers that implement AccessChecker — call CheckAccess. +// 3. All other providers that implement AccessChecker — call CheckAccess(ctx). // Reachable iff CheckAccess returns nil. // // 4. Providers that do NOT implement AccessChecker + remote execEnv — fail-safe @@ -57,12 +81,19 @@ func isLocalExecEnv(execEnv string) bool { // // 5. Providers that do NOT implement AccessChecker + local execEnv — treat as // reachable (local operation; the runtime will surface access errors if any). -func Reachability(p Provider, execEnv string) Result { - switch p.(type) { - case *EnvProvider, *FileProvider, *KeychainProvider: - // Local backends — always reachable from any exec-env. - return Result{Reachable: true} +func Reachability(ctx context.Context, p Provider, execEnv string) Result { + if kind := hostLocalKind(p); kind != "" { + // Host-local backend — verifiable only from a local exec-env. + if isLocalExecEnv(execEnv) { + return Result{Reachable: true} + } + return Result{ + Reachable: false, + Reason: fmt.Sprintf("host-local backend (%s) is not verifiable from a remote exec-env; the remote agent resolves its own secrets (ADR 0017)", kind), + } + } + switch p.(type) { case *GitHubSecretsProvider: // Write-only short-circuit: GitHub secrets inject at CI time only. // CheckAccess is intentionally NOT called — it would return a false @@ -75,7 +106,7 @@ func Reachability(p Provider, execEnv string) Result { default: // For all remaining providers: check whether they implement AccessChecker. if ac, ok := p.(AccessChecker); ok { - if err := ac.CheckAccess(context.Background()); err != nil { + if err := ac.CheckAccess(ctx); err != nil { return Result{ Reachable: false, Reason: fmt.Sprintf("access check failed: %s", err.Error()), diff --git a/secrets/reachability_test.go b/secrets/reachability_test.go index 7ea013b29..3b83157f2 100644 --- a/secrets/reachability_test.go +++ b/secrets/reachability_test.go @@ -72,7 +72,7 @@ func TestReachability(t *testing.T) { wantReachable bool wantReasonSub string // non-empty: the reason must contain this substring }{ - // ── Local backends — always reachable ───────────────────────────────── + // ── Host-local backends + LOCAL exec-env — reachable ────────────────── { name: "EnvProvider local-exec", provider: secrets.NewEnvProvider(""), @@ -80,22 +80,47 @@ func TestReachability(t *testing.T) { wantReachable: true, }, { - name: "EnvProvider remote-exec", + name: "EnvProvider empty (local) exec", provider: secrets.NewEnvProvider("APP_"), - execEnv: "remote", + execEnv: "", wantReachable: true, }, { - name: "FileProvider local-exec", + name: "FileProvider local-docker exec", provider: secrets.NewFileProvider("/tmp"), - execEnv: "", + execEnv: "local-docker", wantReachable: true, }, { - name: "FileProvider remote-exec", + name: "KeychainProvider local exec", + provider: newTestKeychainProvider(t), + execEnv: "local", + wantReachable: true, + }, + + // ── Host-local backends + REMOTE exec-env — fail-safe UNREACHABLE ────── + // Under ADR 0017 the remote agent resolves its own secrets; the engine + // cannot vouch for engine-host env/file/keychain on a remote runner. + { + name: "EnvProvider remote-exec → fail-safe unreachable", + provider: secrets.NewEnvProvider("APP_"), + execEnv: "remote", + wantReachable: false, + wantReasonSub: "host-local backend (env)", + }, + { + name: "FileProvider remote-exec → fail-safe unreachable", provider: secrets.NewFileProvider("/tmp"), execEnv: "k8s-prod", - wantReachable: true, + wantReachable: false, + wantReasonSub: "host-local backend (file)", + }, + { + name: "KeychainProvider remote-exec → fail-safe unreachable", + provider: newTestKeychainProvider(t), + execEnv: "remote", + wantReachable: false, + wantReasonSub: "host-local backend (keychain)", }, // ── GitHubSecretsProvider — always unreachable (write-only short-circuit) ─ @@ -169,7 +194,7 @@ func TestReachability(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - result := secrets.Reachability(tc.provider, tc.execEnv) + result := secrets.Reachability(context.Background(), tc.provider, tc.execEnv) if result.Reachable != tc.wantReachable { t.Errorf("Reachable = %v, want %v (reason: %q)", result.Reachable, tc.wantReachable, result.Reason) } @@ -198,7 +223,7 @@ func TestReachability(t *testing.T) { func TestReachability_GitHubShortCircuit_CheckAccessNotConsulted(t *testing.T) { p := newTestGitHubProvider(t) - result := secrets.Reachability(p, "remote") + result := secrets.Reachability(context.Background(), p, "remote") if result.Reachable { t.Error("GitHubSecretsProvider must be unreachable (write-only)") } @@ -213,7 +238,7 @@ func TestReachability_GitHubShortCircuit_CheckAccessNotConsulted(t *testing.T) { // won't catch it — it will fall through to the generic remote path. // This is intentional: the type-switch is explicitly for *GitHubSecretsProvider. // Verify the stub does get its CheckAccess called (it's a regular remote-with-access). - result2 := secrets.Reachability(stub, "remote") + result2 := secrets.Reachability(context.Background(), stub, "remote") if !result2.Reachable { t.Errorf("stub with CheckAccess→nil should be reachable, got reason: %q", result2.Reason) } @@ -230,7 +255,7 @@ func TestReachability_VaultAndAWS_WithAccessChecker(t *testing.T) { // stubRemoteWithAccess covers the same code path as *VaultProvider and // *AWSSecretsManagerProvider: it implements AccessChecker. stub := &stubRemoteWithAccess{checkAccessErr: nil} - result := secrets.Reachability(stub, "prod-env") + result := secrets.Reachability(context.Background(), stub, "prod-env") if !result.Reachable { t.Errorf("remote with CheckAccess→nil should be reachable, got: %q", result.Reason) } @@ -239,7 +264,7 @@ func TestReachability_VaultAndAWS_WithAccessChecker(t *testing.T) { } stubErr := &stubRemoteWithAccess{checkAccessErr: errors.New("vault: token expired")} - result2 := secrets.Reachability(stubErr, "prod-env") + result2 := secrets.Reachability(context.Background(), stubErr, "prod-env") if result2.Reachable { t.Error("remote with CheckAccess returning err should be unreachable") } @@ -248,6 +273,52 @@ func TestReachability_VaultAndAWS_WithAccessChecker(t *testing.T) { } } +// stubCtxObservingProvider records the context it received in CheckAccess so a +// test can prove ctx is propagated (not context.Background()). +type stubCtxObservingProvider struct { + gotCtx context.Context +} + +func (s *stubCtxObservingProvider) Name() string { return "stub-ctx" } +func (s *stubCtxObservingProvider) Get(_ context.Context, _ string) (string, error) { return "", nil } +func (s *stubCtxObservingProvider) Set(_ context.Context, _, _ string) error { return nil } +func (s *stubCtxObservingProvider) Delete(_ context.Context, _ string) error { return nil } +func (s *stubCtxObservingProvider) List(_ context.Context) ([]string, error) { return nil, nil } +func (s *stubCtxObservingProvider) CheckAccess(ctx context.Context) error { + s.gotCtx = ctx + return ctx.Err() +} + +var _ secrets.AccessChecker = (*stubCtxObservingProvider)(nil) + +// TestReachability_PropagatesContext proves Reachability passes the caller's ctx +// (not a fresh context.Background) into AccessChecker.CheckAccess, so a route / +// pipeline deadline bounds the probe. An already-cancelled context must surface +// as an unreachable verdict. +func TestReachability_PropagatesContext(t *testing.T) { + stub := &stubCtxObservingProvider{} + + type ctxKey string + const k ctxKey = "marker" + ctx, cancel := context.WithCancel(context.WithValue(context.Background(), k, "v")) + cancel() // already cancelled — CheckAccess returns ctx.Err() + + result := secrets.Reachability(ctx, stub, "remote") + + if stub.gotCtx == nil { + t.Fatal("CheckAccess did not receive a context") + } + if stub.gotCtx.Value(k) != "v" { + t.Error("Reachability did not propagate the caller's context into CheckAccess (got a different context)") + } + if result.Reachable { + t.Error("expected unreachable when the propagated context is already cancelled") + } + if !containsSubstr(result.Reason, "access check failed") { + t.Errorf("expected access-check-failed reason, got %q", result.Reason) + } +} + // ─── helpers ────────────────────────────────────────────────────────────────── // newTestGitHubProvider creates a real *secrets.GitHubSecretsProvider using a @@ -264,6 +335,18 @@ func newTestGitHubProvider(t *testing.T) *secrets.GitHubSecretsProvider { return p } +// newTestKeychainProvider creates a real *secrets.KeychainProvider so the +// host-local type-switch in Reachability fires correctly. It performs no OS +// keychain I/O (Reachability classifies by type, it does not probe keychain). +func newTestKeychainProvider(t *testing.T) *secrets.KeychainProvider { + t.Helper() + p, err := secrets.NewKeychainProvider("workflow-test-reachability") + if err != nil { + t.Fatalf("NewKeychainProvider: %v", err) + } + return p +} + func containsSubstr(s, sub string) bool { if len(sub) == 0 { return true From f0b898c0ac3350f3430b045ae1c17a13fd086b7e Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 19:07:39 -0400 Subject: [PATCH 4/5] fix(secrets,iac): register step schema + nil-provider + specs_from fail-safe (review/CI) - CI consistency gate: register step.iac_secret_reachability in coreModuleTypes (schema/schema.go) + ModuleSchema registry (module_schema.go) + StepSchema builtins (step_schema_builtins.go) + regenerate editor golden. (TestRegistryConsistency + TestCoreStepTypesHaveSchemas + TestModuleSchemaRegistry_* require this for every built-in step type.) - Copilot: Reachability rejects nil/typed-nil providers (fail-safe, was a potential panic/fail-open) via isNilProvider (reflect). - Copilot: step's specs_from resolving to empty/missing now ERRORS instead of silently returning all_reachable=true (was a fail-open gate bypass), matching iac_provider_plan/apply. - Copilot: corrected two inaccurate test comments. - Tests: TestReachability_NilProvider + TestSecretReachabilityStep_SpecsFromEmpty_Errors. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../pipeline_step_iac_secret_reachability.go | 6 ++++ ...eline_step_iac_secret_reachability_test.go | 28 +++++++++++++++++ schema/module_schema.go | 1 + schema/schema.go | 1 + schema/step_schema_builtins.go | 18 +++++++++++ schema/testdata/editor-schemas.golden.json | 7 +++++ secrets/reachability.go | 22 +++++++++++++ secrets/reachability_test.go | 31 +++++++++++++++---- 8 files changed, 108 insertions(+), 6 deletions(-) diff --git a/module/pipeline_step_iac_secret_reachability.go b/module/pipeline_step_iac_secret_reachability.go index e94c741c9..3b178bd9f 100644 --- a/module/pipeline_step_iac_secret_reachability.go +++ b/module/pipeline_step_iac_secret_reachability.go @@ -88,6 +88,12 @@ func (s *IaCSecretReachabilityStep) Execute(ctx context.Context, pc *PipelineCon if err != nil { return nil, fmt.Errorf("iac_secret_reachability step %q: resolve specs_from %q: %w", s.name, s.specsFrom, err) } + // Fail-safe: a specs_from that resolves to nil/empty (missing/misspelled + // context path, or a body lacking specs) must NOT silently bypass the + // gate with all_reachable=true. Error, matching iac_provider_plan/apply. + if len(specs) == 0 { + return nil, fmt.Errorf("iac_secret_reachability step %q: specs_from %q resolved to empty/zero specs", s.name, s.specsFrom) + } } // Resolve the secrets provider from the service registry. diff --git a/module/pipeline_step_iac_secret_reachability_test.go b/module/pipeline_step_iac_secret_reachability_test.go index 24648989f..5a3e5af93 100644 --- a/module/pipeline_step_iac_secret_reachability_test.go +++ b/module/pipeline_step_iac_secret_reachability_test.go @@ -473,3 +473,31 @@ func TestSecretReachabilityStep_Factory_SpecsAndSpecsFromMutualExclusion(t *test t.Errorf("expected 'mutually exclusive' error, got: %v", err) } } + +// TestSecretReachabilityStep_SpecsFromEmpty_Errors verifies the fail-safe: a +// specs_from that resolves to a missing/empty context path errors instead of +// silently returning all_reachable=true (which would bypass the gate). +func TestSecretReachabilityStep_SpecsFromEmpty_Errors(t *testing.T) { + app := module.NewMockApplication() + stub := &stubSecretsProviderWithAccess{checkErr: nil} + if err := app.RegisterService("my-vault", &stubSecretsModuleAccessor{underlying: stub}); err != nil { + t.Fatal(err) + } + + factory := module.NewIaCSecretReachabilityStepFactory() + step, err := factory("reach-step", map[string]any{ + "provider": "my-vault", + "exec_env": "remote", + "specs_from": "steps.parse-request.body.specs", + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + // PipelineContext WITHOUT the parse-request output → specs_from resolves nil. + pc := &module.PipelineContext{StepOutputs: map[string]map[string]any{}} + + if _, err := step.Execute(context.Background(), pc); err == nil { + t.Fatal("expected an error when specs_from resolves to empty/missing, got nil (fail-open!)") + } +} diff --git a/schema/module_schema.go b/schema/module_schema.go index 80120ed52..cabdd1c4d 100644 --- a/schema/module_schema.go +++ b/schema/module_schema.go @@ -2858,6 +2858,7 @@ func (r *ModuleSchemaRegistry) registerBuiltins() { {"step.iac_provider_drift", "IaC Provider Drift", "Detects drift via IaCProvider service"}, {"step.iac_provider_list", "IaC Provider List", "Lists resource statuses from IaCProvider service"}, {"step.iac_provider_plan", "IaC Provider Plan", "Plans infrastructure changes via IaCProvider service"}, + {"step.iac_secret_reachability", "IaC Secret Reachability", "Pre-flight gate: reports whether a plan's secret:// refs are reachable from the chosen exec-env"}, {"step.iac_status", "IaC Status", "Gets IaC provisioning status"}, {"step.k8s_apply", "K8s Apply", "Applies Kubernetes manifests"}, {"step.k8s_destroy", "K8s Destroy", "Deletes Kubernetes resources"}, diff --git a/schema/schema.go b/schema/schema.go index 0ef8316af..06a7f7af0 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -318,6 +318,7 @@ var coreModuleTypes = []string{ "step.iac_provider_drift", "step.iac_provider_list", "step.iac_provider_plan", + "step.iac_secret_reachability", "step.iac_status", "step.jq", "step.json_parse", diff --git a/schema/step_schema_builtins.go b/schema/step_schema_builtins.go index 45b23a677..7689e228c 100644 --- a/schema/step_schema_builtins.go +++ b/schema/step_schema_builtins.go @@ -2215,6 +2215,24 @@ func (r *StepSchemaRegistry) registerBuiltins() { }, }) + // ---- IaC Secret Reachability ---- + + r.Register(&StepSchema{ + Type: "step.iac_secret_reachability", + Plugin: "platform", + Description: "Pre-flight gate: collects the secret:// refs in a plan's specs and reports whether they are reachable from the chosen exec-env (fail-safe; the direct-apply route 409s when any are unreachable). The verdict is provider-level (one AccessChecker probe), reported per ref.", + ConfigFields: []ConfigFieldDef{ + {Key: "provider", Type: FieldTypeString, Description: "Name of the registered secrets.Provider service", Required: true}, + {Key: "exec_env", Type: FieldTypeString, Description: "Execution environment the apply will run in (empty/local/local-docker = local; anything else = remote)"}, + {Key: "specs", Type: FieldTypeArray, Description: "Static resource specs (mutually exclusive with specs_from)"}, + {Key: "specs_from", Type: FieldTypeString, Description: "Pipeline-context path to the specs (e.g. steps.parse-request.body.specs); mutually exclusive with specs"}, + }, + Outputs: []StepOutputDef{ + {Key: "secrets", Type: "[]any", Description: "Per-ref verdicts: {ref, reachable, reason}"}, + {Key: "all_reachable", Type: "boolean", Description: "True iff every referenced secret is reachable from the exec-env"}, + }, + }) + // ---- Kubernetes Apply ---- r.Register(&StepSchema{ diff --git a/schema/testdata/editor-schemas.golden.json b/schema/testdata/editor-schemas.golden.json index 10fb3c71d..fab7b3e06 100644 --- a/schema/testdata/editor-schemas.golden.json +++ b/schema/testdata/editor-schemas.golden.json @@ -6055,6 +6055,13 @@ "description": "Plans infrastructure changes via IaCProvider service", "configFields": [] }, + "step.iac_secret_reachability": { + "type": "step.iac_secret_reachability", + "label": "IaC Secret Reachability", + "category": "pipeline", + "description": "Pre-flight gate: reports whether a plan's secret:// refs are reachable from the chosen exec-env", + "configFields": [] + }, "step.iac_status": { "type": "step.iac_status", "label": "IaC Status", diff --git a/secrets/reachability.go b/secrets/reachability.go index 30bc3aa4f..73fe1f1e6 100644 --- a/secrets/reachability.go +++ b/secrets/reachability.go @@ -3,6 +3,7 @@ package secrets import ( "context" "fmt" + "reflect" ) // Result holds the outcome of a Reachability check. @@ -82,6 +83,12 @@ func hostLocalKind(p Provider) string { // 5. Providers that do NOT implement AccessChecker + local execEnv — treat as // reachable (local operation; the runtime will surface access errors if any). func Reachability(ctx context.Context, p Provider, execEnv string) Result { + // Fail-safe: a nil provider — including a typed-nil pointer stored in the + // Provider interface (e.g. (*VaultProvider)(nil)) — is never reachable, and + // calling into it could panic on a nil receiver. Reject before any dispatch. + if isNilProvider(p) { + return Result{Reachable: false, Reason: "no secrets provider configured (nil provider)"} + } if kind := hostLocalKind(p); kind != "" { // Host-local backend — verifiable only from a local exec-env. if isLocalExecEnv(execEnv) { @@ -127,3 +134,18 @@ func Reachability(ctx context.Context, p Provider, execEnv string) Result { } } } + +// isNilProvider reports whether p is nil or a typed-nil pointer/interface stored +// in the Provider interface — both of which must be treated as unreachable. +func isNilProvider(p Provider) bool { + if p == nil { + return true + } + v := reflect.ValueOf(p) + switch v.Kind() { + case reflect.Ptr, reflect.Interface, reflect.Map, reflect.Slice, reflect.Func, reflect.Chan: + return v.IsNil() + default: + return false + } +} diff --git a/secrets/reachability_test.go b/secrets/reachability_test.go index 3b83157f2..b99a67116 100644 --- a/secrets/reachability_test.go +++ b/secrets/reachability_test.go @@ -247,10 +247,11 @@ func TestReachability_GitHubShortCircuit_CheckAccessNotConsulted(t *testing.T) { } } -// TestReachability_VaultProvider tests the real *VaultProvider path (remote with -// AccessChecker). Since VaultProvider requires a real vault.Client, we use the -// stubRemoteWithAccess to cover the functional branch; the actual *VaultProvider -// type-switch is confirmed by compilation. +// TestReachability_VaultAndAWS_WithAccessChecker covers the AccessChecker branch +// that *VaultProvider and *AWSSecretsManagerProvider hit (rule 3). Those concrete +// providers require live clients, so this test uses stubRemoteWithAccess — a stub +// implementing AccessChecker — to exercise the same default-case probe path. It +// does NOT instantiate the real provider types. func TestReachability_VaultAndAWS_WithAccessChecker(t *testing.T) { // stubRemoteWithAccess covers the same code path as *VaultProvider and // *AWSSecretsManagerProvider: it implements AccessChecker. @@ -323,8 +324,8 @@ func TestReachability_PropagatesContext(t *testing.T) { // newTestGitHubProvider creates a real *secrets.GitHubSecretsProvider using a // fake token so the type-switch in Reachability fires correctly. -// It sets the GITHUB_TOKEN env var temporarily so NewGitHubSecretsProvider -// doesn't reject the empty token. +// It sets the WORKFLOW_TEST_GH_TOKEN env var temporarily (the name passed into +// NewGitHubSecretsProvider) so the provider doesn't reject an empty token. func newTestGitHubProvider(t *testing.T) *secrets.GitHubSecretsProvider { t.Helper() t.Setenv("WORKFLOW_TEST_GH_TOKEN", "ghp_fake_token_for_type_switch_test") @@ -358,3 +359,21 @@ func containsSubstr(s, sub string) bool { } return false } + +// TestReachability_NilProvider verifies a nil or typed-nil provider is fail-safe +// unreachable (never panics, never reports reachable). +func TestReachability_NilProvider(t *testing.T) { + // Untyped nil. + if r := secrets.Reachability(context.Background(), nil, "local"); r.Reachable { + t.Errorf("nil provider should be unreachable, got reachable") + } + // Typed-nil pointer stored in the Provider interface. + var typedNil *secrets.VaultProvider + r := secrets.Reachability(context.Background(), typedNil, "local") + if r.Reachable { + t.Errorf("typed-nil *VaultProvider should be unreachable, got reachable") + } + if r.Reason == "" { + t.Errorf("expected a non-empty reason for nil provider") + } +} From eaf8dadbbbdb851e1e55188b02f76b205abdf936 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 19:12:13 -0400 Subject: [PATCH 5/5] test(platform): add step.iac_secret_reachability to expectedSteps factory list Co-Authored-By: Claude Opus 4.8 (1M context) --- plugins/platform/plugin_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/platform/plugin_test.go b/plugins/platform/plugin_test.go index 8a6182a1c..8d29559b5 100644 --- a/plugins/platform/plugin_test.go +++ b/plugins/platform/plugin_test.go @@ -47,6 +47,7 @@ func TestStepFactories(t *testing.T) { "step.iac_provider_apply", "step.iac_provider_destroy", "step.iac_provider_drift", + "step.iac_secret_reachability", "step.dns_plan", "step.dns_apply", "step.dns_status",