feat(iac): engine-side sensitive-output routing through secrets.Provider#587
Closed
intel352 wants to merge 12 commits into
Closed
feat(iac): engine-side sensitive-output routing through secrets.Provider#587intel352 wants to merge 12 commits into
intel352 wants to merge 12 commits into
Conversation
…ovider Captures the design for v0.27.0 engine layer that routes ResourceOutput fields flagged Sensitive (or declared via SensitiveKeys()) through the configured secrets.Provider before state persistence — keeping plugins platform-agnostic per ADR 0015 / user mandate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…indings Addresses adversarial-design-review cycle 1 findings: - Critical: drop non-existent audit-secrets recovery; add audit-state-secrets - Critical: drop Restore API (incompatible with write-only GitHub provider); use in-memory hydrated map for same-process hand-off - Critical: explicit resourceName parameter on Route (not from out.Name) - Important: enumerate all 5 state-write call sites + per-site routing policy - Important: SensitiveKeys() stays display-masking-only; routing uses per-call Sensitive map exclusively - Important: Read/Adoption/Refresh paths sanitize-only (no Set), preventing cache pollution - Important: compensating Delete on Set-success/Save-failure (no orphan cloud resources) - Important: MaskSensitiveForDiff prevents drift false-positives Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5-task plan covering: iac/sensitive package, persistResourceWithSecretRouting helper + Apply call-site rewires (sites 1+2), sanitize-only Read paths (sites 3+4+5), audit-state-secrets command + drift masking, docs. Single PR; ~1.6k lines net. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dings Addresses adversarial-design-review (plan phase) cycle 1 findings: Critical: - Type-system mismatch: helper takes infraStateStore (wfctl-internal), not interfaces.IaCStateStore. Matches existing call-site discipline. - Removed dead currentInfraConfigFile() reference; cfgFile threaded as explicit new parameter through applyWithProviderAndStore + applyPrecomputedPlanWithStore. - Tasks 2 + 3 add Step N.7.bis runtime-launch-validation smoke (build artifact + exercise without/with secrets cfg, capture transcripts). Important: - Task 4 split into Task 4 (audit-state-secrets only) + Task 5 (drift masking enumeration with explicit no-op clause). Plan now has 6 tasks. - Step 2.5.1 adds explicit existing-test-update sub-step for syncInfraOutputSecrets/resolveInfraOutput signature ripple. - Step 2.4 #5 preserves validateOutputProviderID call (regression guard). - Step 2.4 #6 adds pre-flight detection of sensitive-emitting drivers before persistence loop (no partial-apply mid-stream). - Step 2.4 #7 isolates compensation context (fresh 30s timeout). - Task 6 §6.4 validates rollback claim against v0.26.x consumer. - Public API contract section locks exported names from Task 1. - Scope Manifest excludes infra.go:1101 import path with reasoning. Minor: - Documents string-only sensitive value limitation as v0.27.0 constraint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Alignment-check identified ## Task → ### Task normalization required by the writing-plans skill format. Six tasks now match alignment-check's forward + reverse trace and the Scope Manifest counts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Manifest section sha256: f7a8e190ca8bc264252d2609ba856bdd979b3a5eaa3265d0d33f1dc85055c361 Locked at: 2026-05-09T08:11:07Z PR Count: 1 Tasks: 6 Branch: feat/engine-sensitive-output-routing Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…helpers New iac/sensitive package for engine-side routing of ResourceOutput sensitive fields through secrets.Provider. Free functions, no struct. Exported API (locked at v0.27.0): - Route(ctx, provider, resourceName, *out) (sanitized, hydrated, err) - Revoke(ctx, provider, resourceName, mergedKeys) error - IsPlaceholder(v any) bool - MaskSensitiveForDiff(driverKeys, desired, current) (map, map) - Placeholder(resourceName, outputKey) string - PlaceholderPrefix string - SecretKey(resourceName, outputKey) string Placeholder format "secret_ref://<resource>_<key>" is distinct from the existing user-supplied "secret://<key>" config-reference convention. Routing trigger is exclusively out.Sensitive[k]==true; SensitiveKeys() remains a display-masking-only signal per design rev1. 23 unit tests cover happy/error/edge cases per plan §7. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces persistResourceWithSecretRouting helper that funnels both state-write call sites in applyWithProviderAndStore + in-process apply. Routes per-call Sensitive ResourceOutput fields through secrets.Provider into "secret_ref://" placeholders before SaveResource. Threading: - applyWithProviderAndStore + applyPrecomputedPlanWithStore each gain cfgFile string + hydratedOut map[string]string parameters. - applyInfraModules now returns (map[string]string, error) where the map contains routed-secret values for in-process hand-off to syncInfraOutputSecrets. - syncInfraOutputSecrets + resolveInfraOutput accept a hydrated map to rehydrate sensitive.PlaceholderPrefix entries from same-process apply (works on write-only GitHub Actions provider). Failure modes: - Pre-flight detection: if any driver emits Sensitive outputs but no secrets.Provider is configured, hard-fail BEFORE per-resource persistence loop (no partial-apply). - On SaveResource failure post-Set, compensateAfterSaveFailure with a fresh 30s context calls driver.Delete + provider.Delete to prevent orphan cloud resources + routed secrets. Test ripple: every existing call site updated to pass nil/empty for the new parameters. New test file infra_apply_sensitive_routing_test.go covers Route happy path, NoProvider hard-fail, SaveFailure compensation, NoSensitive pass-through, idempotent re-apply, pre-flight detection, read-mode placeholder preservation + new-key drop. Rollback: revert this commit; helper + call sites revert to prior literal Outputs: r.Outputs shape in one diff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Read paths (adoptExistingResources, runInfraRefreshOutputs) now route state writes through persistResourceWithSecretRouting in persistModeRead. The helper inherits placeholders from prior state (via ListResources) and drops newly-declared sensitive keys — never calls provider.Set from a Read path (prevents cache pollution per design §4.4). Refresh path uses driver.SensitiveKeys() as the per-call Sensitive map source (refresh has no per-call Sensitive declaration; SensitiveKeys is the static driver-declared signal that approximates intent). Site 4 (resourceStateFromLiveOutput) is a builder, not a save site; no change needed there. Sites 3+5 wired through the helper. Rollback: revert this commit; sites revert to direct store.SaveResource. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds wfctl infra audit-state-secrets command per design §4.7. Distinct from existing audit-secrets (which audits the secrets.generate config block for anti-patterns). Audit-state-secrets walks state.Outputs vs. secrets.Provider to detect: - placeholder-without-routed-value findings (state has secret_ref:// placeholder but provider does not have the secret) - legacy plaintext findings (state value matches DefaultSensitiveKeys heuristic AND is plaintext rather than a placeholder) - mistaken secret://... config-reference findings (user-config syntax leaked into a persisted state field) - orphan secret findings (provider has a name matching <res>_<key> pattern; no state resource named <res>) --prune deletes confirmed orphan secrets idempotently (safe to rerun). Exit codes: 0 = no findings; 1 = findings; 2 = audit error. For write-only providers (GitHub Actions Get returns ErrUnsupported), the command emits structured ADVISORY lines for each placeholder it cannot verify, but does not exit non-zero on those alone. Rollback: revert this commit; command is additive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-the design's drift-masking enumeration task (Task 5 of the engine-sensitive-output-routing plan): grep enumeration of driver.Diff/d.Diff call sites in cmd/wfctl/ + iac/ shows zero production sites that see state.Outputs flowing into Diff. Only iac/conformance/scenario_grpc_roundtrip.go matches and that's a conformance-test tool that synthesizes its own desired/current. This commit ships the documented no-op outcome: a comment block at the top of cmd/wfctl/infra_apply_refresh.go (the closest applicable file) explains the enumeration outcome and how to wire sensitive.MaskSensitiveForDiff if a future call site lands. sensitive.MaskSensitiveForDiff stays exported as the future hook. Rollback: revert this commit; comment is additive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- DOCUMENTATION.md: new "Sensitive output routing" section explaining Sensitive flag, secret_ref:// placeholder format, failure modes, recovery path, read-path semantics, drift masking, cold-start consumers, and v0.27.0 string-only limitation. - docs/WFCTL.md: command-tree update + new "infra audit-state-secrets" command reference. - CHANGELOG.md: comprehensive Unreleased entry covering Added (engine routing, iac/sensitive package, audit-state-secrets command), Changed (signature ripple for applyWithProviderAndStore + applyPrecomputedPlanWithStore + applyInfraModules + syncInfraOutputSecrets + resolveInfraOutput, sanitize-only routing for adoption + refresh), Migration (greenfield + legacy guidance), Rollback (v0.26.x runbook with validation-deferred note). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces engine-side handling for IaC resource outputs marked as sensitive: during Create/Update, sensitive output values are routed into the configured secrets.Provider and replaced in persisted state with secret_ref://... placeholders, keeping state backends free of plaintext secrets. It also adds an operational audit command to detect issues between state and the secrets provider, plus documentation for the new model.
Changes:
- Added new
iac/sensitivehelpers (Route,Revoke, placeholder utilities, diff masking) with unit tests. - Rewired wfctl infra apply/adoption/refresh persistence to sanitize/route sensitive outputs and plumb an in-process “hydrated” map to post-apply secret generation.
- Added
wfctl infra audit-state-secretscommand and updated docs/changelog for v0.27.0 behavior.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
iac/sensitive/route.go |
New routing/sanitization primitives and placeholder conventions. |
iac/sensitive/route_test.go |
Unit tests for routing, revoke, placeholder detection, and diff masking. |
cmd/wfctl/infra_apply.go |
Central routing/sanitize helper, apply-path integration, preflight check, compensation logic; signature changes for apply helpers. |
cmd/wfctl/infra.go |
Threads apply’s hydrated routed-secret map to syncInfraOutputSecrets; adds audit-state-secrets dispatch. |
cmd/wfctl/infra_output_secrets.go |
Rehydrates placeholders via same-process hydrated map during infra_output secret generation; signature changes. |
cmd/wfctl/infra_resolve_state.go |
Passes nil hydrated map for offline state resolution (documented skip behavior). |
cmd/wfctl/infra_refresh_outputs.go |
Refresh persistence now sanitize-only via persistResourceWithSecretRouting read mode. |
cmd/wfctl/infra_apply_refresh.go |
Documents current lack of in-tree Diff call sites needing masking. |
cmd/wfctl/infra_audit_state_secrets.go |
New infra audit-state-secrets implementation. |
cmd/wfctl/infra_audit_state_secrets_test.go |
Tests for audit findings and write-only provider behavior. |
cmd/wfctl/infra_apply_sensitive_routing_test.go |
Integration tests covering routing, preflight, compensation, and read-mode sanitization. |
cmd/wfctl/infra_output_secrets_test.go |
Updated for new hydrated parameter. |
cmd/wfctl/infra_*_test.go (various) |
Updated call sites for signature changes (applyInfraModules, apply helpers, resolveInfraOutput). |
DOCUMENTATION.md |
Adds “Sensitive output routing (v0.27.0+)” section. |
docs/WFCTL.md |
Documents infra audit-state-secrets and updates command tree. |
CHANGELOG.md |
Adds v0.27.0 entries for routing, new command, signature changes, migration/rollback notes. |
docs/plans/2026-05-09-engine-sensitive-output-routing*.md* |
Adds design + implementation plan and scope lock artifact. |
Comment on lines
+982
to
+996
| // requireSecretsProviderForSensitiveOutputs returns an error if any of the | ||
| // resources in result has sensitive outputs but provider is nil. Surfaced | ||
| // before the per-resource persistence loop so an apply with sensitive | ||
| // drivers and no provider fails fast with a complete diagnostic instead | ||
| // of partial state writes. | ||
| func requireSecretsProviderForSensitiveOutputs(provider secrets.Provider, result *interfaces.ApplyResult) error { | ||
| if provider != nil || result == nil { | ||
| return nil | ||
| } | ||
| for i := range result.Resources { | ||
| if hasSensitiveOutputs(&result.Resources[i]) { | ||
| return fmt.Errorf( | ||
| "secrets.Provider not configured but driver emitted sensitive outputs (resource %q has Sensitive keys %v); add `secrets:` block to your config or use `secrets: { provider: env }`", | ||
| result.Resources[i].Name, sensitiveKeysFor(&result.Resources[i])) | ||
| } |
Comment on lines
1136
to
1142
| provider, closer, err := resolveIaCProvider(ctx, g.provType, g.provCfg) | ||
| if err != nil { | ||
| return fmt.Errorf("provider %q (%s): load provider: %w", moduleRef, g.provType, err) | ||
| } | ||
| applyErr := applyPrecomputedPlanWithStore(ctx, groupPlan, provider, g.provType, store, os.Stderr, envName) | ||
| applyErr := applyPrecomputedPlanWithStore(ctx, groupPlan, provider, g.provType, store, os.Stderr, envName, cfgFile, nil) | ||
| if closer != nil { | ||
| if cerr := closer.Close(); cerr != nil { |
Comment on lines
+142
to
+163
| // Walk provider for orphan secrets. | ||
| names, err := prov.List(ctx) | ||
| switch { | ||
| case err == nil: | ||
| sort.Strings(names) | ||
| for _, name := range names { | ||
| res := stripKnownSensitiveSuffix(name) | ||
| if _, ok := stateNames[res]; ok { | ||
| continue | ||
| } | ||
| if prune { | ||
| if delErr := prov.Delete(ctx, name); delErr != nil { | ||
| fmt.Fprintf(w, "PRUNE FAILED: %q: %v\n", name, delErr) | ||
| findings++ | ||
| } else { | ||
| fmt.Fprintf(w, "pruned orphan secret %q\n", name) | ||
| } | ||
| continue | ||
| } | ||
| fmt.Fprintf(w, "FINDING (orphan secret): %q has no matching state resource; rerun with --prune to delete\n", name) | ||
| findings++ | ||
| } |
Comment on lines
+802
to
+805
| // In read mode, the helper does NOT call provider.Set; instead it consults | ||
| // the prior state via store.GetResource and inherits any | ||
| // PlaceholderPrefix entries; new sensitive keys (declared by the driver | ||
| // at Read time but not previously routed) are dropped from sanitized. |
Comment on lines
+116
to
+129
| case sensitive.IsPlaceholder(v): | ||
| secretName := strings.TrimPrefix(s, sensitive.PlaceholderPrefix) | ||
| _, getErr := prov.Get(ctx, secretName) | ||
| if getErr == nil { | ||
| continue | ||
| } | ||
| if errors.Is(getErr, secrets.ErrUnsupported) { | ||
| fmt.Fprintf(w, "ADVISORY (Get unsupported): cannot verify routed value for %s/%s -> %q on this provider\n", st.Name, k, secretName) | ||
| continue | ||
| } | ||
| if errors.Is(getErr, secrets.ErrNotFound) { | ||
| fmt.Fprintf(w, "FINDING (missing routed value): %s/%s expects routed secret %q but provider does not have it\n", st.Name, k, secretName) | ||
| findings++ | ||
| } |
Contributor
Author
|
Recreating branch after main moved (avoiding force-push per workspace convention). New PR will follow shortly. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Engine routes per-call
Sensitive-flagged outputs fromResourceDriver.Create/Updatethrough the configuredsecrets.Provider, replacing values in state withsecret_ref://<resource>_<key>placeholders. Plugins remain platform-agnostic — a plugin compiled into a wfctl-from-CI run, a wfctl-from-CLI run, or a workflow-cloud server transparently gets sensitive-output handling without each host writing its own routing.Read paths (adoption, refresh) are sanitize-only — no
provider.Setcalls from a Read path (prevents cache pollution). OnSaveResourcefailure afterprovider.Setsucceeded, the engine compensates withdriver.Delete+provider.Deleteto prevent orphan cloud resources.Targets workflow
v0.27.0. Per ADR 0015 (Approach B Hybrid split-storage).Design
docs/plans/2026-05-09-engine-sensitive-output-routing-design.mdImplementation Plan
docs/plans/2026-05-09-engine-sensitive-output-routing.mdScope Manifest
f7a8e190ca8bc264252d2609ba856bdd979b3a5eaa3265d0d33f1dc85055c361)feat/engine-sensitive-output-routingChanges (per task)
Task 1 — New
iac/sensitivepackage:Route(ctx, provider, resourceName, *out) (sanitized, hydrated, error),Revoke,IsPlaceholder,MaskSensitiveForDiff,Placeholder,PlaceholderPrefix,SecretKey. Free functions, no struct. Routing trigger is exclusively per-callout.Sensitive[k]==true;ResourceDriver.SensitiveKeys()remains a display-masking-only signal. 23 unit tests cover happy/error/edge cases.Task 2 —
persistResourceWithSecretRoutinghelper + Apply call-site rewires (sites 1+2:applyWithProviderAndStoreandapplyPrecomputedPlanWithStore). Helper funnels both apply paths; pre-flight detection of sensitive-emitting drivers when no provider configured (no partial-apply); compensatingdriver.Delete+provider.DeleteonSaveResourcefailure post-Set(with isolated 30s context); hydrated-map plumbing throughrunInfraApplytosyncInfraOutputSecretsfor in-process hand-off (works on write-only GitHub provider). 9 new integration tests.Task 3 — Sanitize-only routing for adoption + refresh paths (sites 3+5). Read paths inherit placeholders from prior state (via
ListResources) and drop newly-declared sensitive keys. Refresh path usesdriver.SensitiveKeys()as the static Sensitive map source.Task 4 — New
wfctl infra audit-state-secretscommand with--prune. Walksstate.Outputsvs.secrets.Providerto detect orphans, missing routed values, legacy plaintext, and mistakensecret://...config-references in state. Distinct from existingaudit-secrets(which auditssecrets.generateconfig block). Exit codes: 0/1/2. Write-only-provider-aware (emits ADVISORY for unverifiable placeholders without exit-1). 8 unit tests.Task 5 — Drift masking enumeration: documented no-op. Verified
grep -rn "driver\.Diff(\|d\.Diff(" cmd/wfctl/ iac/returns only conformance-test scenarios.iac/sensitive.MaskSensitiveForDiffis exported as a helper for future in-tree consumers; comment incmd/wfctl/infra_apply_refresh.goexplains the enumeration outcome.Task 6 — Documentation:
DOCUMENTATION.md"Sensitive output routing (v0.27.0+)" section,docs/WFCTL.mdcommand-tree update +infra audit-state-secretscommand reference,CHANGELOG.mdUnreleased entry covering Added / Changed / Migration / Rollback. Rollback claim against v0.26.0 documented as mechanical-analysis-validated; live runtime smoke deferred to first-rollback-event.Verification (local)
go test ./...— all packages PASS (full workflow test suite)go test -race ./cmd/wfctl/... ./iac/sensitive/... ./secrets/...— PASSgo vet ./...— cleangolangci-lint run ./cmd/wfctl/... ./iac/sensitive/...— clean (only pre-existingG115insecrets_detect.gounrelated to this PR)go build -o /tmp/wfctl-v027 ./cmd/wfctl— PASSwfctl infra --helpandwfctl infra audit-state-secrets --help— both render correctlyRuntime smoke against a stub IaC provider is deferred per the plan's note (no in-tree stub fixture in
cmd/wfctl/testdata/); the helper's behavior is exercised by 14 integration tests covering apply mode, read mode, no-provider-hard-fail, save-failure compensation, idempotent re-apply, pre-flight detection, placeholder preservation, and new-sensitive-key drop.Test plan (reviewer)
iac/sensitive/route.goandroute_test.go— 23 tests cover the public API contract.cmd/wfctl/infra_apply.gopersistResourceWithSecretRoutingblock (~200 LoC) — verify mode dispatch + compensating-Delete logic + read-mode placeholder inheritance.cmd/wfctl/infra_apply_sensitive_routing_test.go— 9 integration tests pin behaviour.cmd/wfctl/infra_audit_state_secrets.go+_test.go— 8 tests cover all 4 finding types + prune + write-only advisory.Rollback
Pin
setup-wfctl@v0.26.xand rebuild. State records written under v0.27.0 containsecret_ref://...placeholders that v0.26.0 treats as literal strings; rotate affected secrets viawfctl infra bootstrap --force-rotate <name>running v0.27.0 first to regenerate plaintext state. Full runbook inCHANGELOG.mdUnreleased / Rollback section.🤖 Generated with Claude Code