feat(iac): provider_credential rotation — mint-new-then-revoke-old via ProviderCredentialRevoker#573
Merged
Merged
Conversation
…a ProviderCredentialRevoker - Remove --force-rotate rejection for provider_credential type (was blocked with "must be rotated via upstream provider"); now allowed with mint-new-then-revoke-old ordering (ADR 0012). - Add ProviderCredentialRevoker optional interface to interfaces/iac_provider.go: plugins that issue credentials implement RevokeProviderCredential; callers type-assert and fall through to a warning log if not implemented. - bootstrapSecrets accepts optional variadic ProviderCredentialRevoker; for provider_credential force-rotate: reads OLD access_key_id before deletion, mints+stores new sub-keys, then revokes old credential. Revoke failure is non-fatal — new key is never rolled back (ADR 0012). - ADR 0012: documents ordering guarantee, revoke-failure contract, and alternatives considered. - Tests: ProviderCredentialAllowed, MintsAndRevokes, RevokeFailNonFatal; updated ProviderCredentialRefused → ProviderCredentialAllowed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Enables wfctl infra bootstrap --force-rotate for provider_credential secrets by introducing an optional IaC provider interface to revoke the old upstream credential after minting and storing the new one (per ADR 0012).
Changes:
- Added optional
interfaces.ProviderCredentialRevokerfor provider plugins to support upstream revocation. - Updated
wfctl infra bootstrapto allow force-rotation ofprovider_credentialwith mint-new-then-revoke-old ordering (revocation best-effort / non-fatal). - Added/updated force-rotate tests and introduced ADR 0012 documenting the contract and rationale.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
interfaces/iac_provider.go |
Adds optional ProviderCredentialRevoker interface for upstream credential revocation. |
cmd/wfctl/infra_bootstrap.go |
Allows force-rotate for provider_credential, loads IaC provider for revocation, and implements mint-new-then-revoke-old flow in bootstrapSecrets. |
cmd/wfctl/infra_bootstrap_force_rotate_test.go |
Updates tests to allow provider_credential rotation and adds revocation + revoke-failure tests. |
decisions/0012-provider-credential-rotation.md |
ADR documenting ordering guarantees and revoke-failure behavior. |
| // The new credential will still be minted; the old one will | ||
| // not be explicitly revoked (operator must do so manually). | ||
| fmt.Fprintf(os.Stderr, "warn: could not load IaC provider for credential revocation: %v\n", loadErr) | ||
| fmt.Fprintf(os.Stderr, "warn: old provider credential will NOT be revoked automatically — revoke manually\n") |
| fmt.Fprintf(os.Stderr, "warn: old provider credential will NOT be revoked automatically — revoke manually\n") | ||
| } else { | ||
| if iacProvCloser != nil { | ||
| defer iacProvCloser.Close() //nolint:errcheck |
| // Read old access_key sub-key before deletion. | ||
| accessKeyName := gen.Key + "_access_key" | ||
| if oldVal, getErr := provider.Get(ctx, accessKeyName); getErr == nil { | ||
| oldCredentialID = oldVal |
Comment on lines
+600
to
+601
| } else if credRevoker == nil && oldCredentialID != "" { | ||
| fmt.Fprintf(os.Stderr, "warn: no revoker available — old credential %s (id=%s) must be revoked manually\n", gen.Key, oldCredentialID) |
Comment on lines
187
to
189
| if !isRotatableType(gen.Type) { | ||
| return nil, fmt.Errorf("--force-rotate %q: %s secrets must be rotated via the upstream provider; cannot regenerate locally", name, gen.Type) | ||
| return nil, fmt.Errorf("--force-rotate %q: %s secrets are derived from apply-time state and cannot be regenerated by bootstrap", name, gen.Type) | ||
| } |
Comment on lines
+150
to
+204
| // TestInfraBootstrap_ForceRotate_ProviderCredential_MintsAndRevokes verifies | ||
| // the full mint-new-then-revoke-old flow for provider_credential force-rotate. | ||
| // The revoker captures the old credentialID; the new sub-keys are stored. | ||
| func TestInfraBootstrap_ForceRotate_ProviderCredential_MintsAndRevokes(t *testing.T) { | ||
| // Stub generator returns a new DO Spaces credential JSON. | ||
| withStubGenerator(t, func(_ context.Context, _ string, _ map[string]any) (string, error) { | ||
| return `{"access_key":"NEW_AK","secret_key":"NEW_SK"}`, nil | ||
| }) | ||
|
|
||
| // Store has old sub-keys pre-populated. | ||
| p := newTrackingProvider(map[string]string{ | ||
| "SPACES_access_key": "OLD_AK", | ||
| "SPACES_secret_key": "OLD_SK", | ||
| }) | ||
| cfg := &SecretsConfig{ | ||
| Generate: []SecretGen{ | ||
| {Key: "SPACES", Type: "provider_credential", Source: "digitalocean.spaces"}, | ||
| }, | ||
| } | ||
| forceRotate := map[string]bool{"SPACES": true} | ||
|
|
||
| // Capture revoke calls. | ||
| var revokedSource, revokedID string | ||
| revoker := &stubProviderRevoker{ | ||
| fn: func(_ context.Context, source, credentialID string) error { | ||
| revokedSource = source | ||
| revokedID = credentialID | ||
| return nil | ||
| }, | ||
| } | ||
|
|
||
| result, err := bootstrapSecrets(context.Background(), p, cfg, forceRotate, revoker) | ||
| if err != nil { | ||
| t.Fatalf("bootstrapSecrets: %v", err) | ||
| } | ||
|
|
||
| // New sub-keys should be stored. | ||
| if p.inner["SPACES_access_key"] != "NEW_AK" { | ||
| t.Errorf("SPACES_access_key = %q, want NEW_AK", p.inner["SPACES_access_key"]) | ||
| } | ||
| if p.inner["SPACES_secret_key"] != "NEW_SK" { | ||
| t.Errorf("SPACES_secret_key = %q, want NEW_SK", p.inner["SPACES_secret_key"]) | ||
| } | ||
| if !strings.Contains(err.Error(), "must be rotated via the upstream provider") { | ||
| t.Errorf("error message should mention upstream provider rotation; got: %v", err) | ||
| // Result map should contain new values. | ||
| if result["SPACES_access_key"] != "NEW_AK" { | ||
| t.Errorf("generated[SPACES_access_key] = %q, want NEW_AK", result["SPACES_access_key"]) | ||
| } | ||
| // Revocation should have been called with old access_key_id. | ||
| if revokedSource != "digitalocean.spaces" { | ||
| t.Errorf("revokedSource = %q, want digitalocean.spaces", revokedSource) | ||
| } | ||
| if revokedID != "OLD_AK" { | ||
| t.Errorf("revokedID = %q, want OLD_AK", revokedID) | ||
| } | ||
| } |
…itic violation Moves the IaC provider load + ProviderCredentialRevoker type-assertion out of the for loop (where defer was flagged by gocritic deferInLoop) into a standalone helper resolveCredentialRevoker that returns the revoker + closer. Defer is now at runInfraBootstrap function scope. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…otation - buildForceRotateSet error message: list supported rotatable types instead of claiming the type is infra_output-specific - resolveCredentialRevoker: explicitly check iacProv == nil and emit clear warning (loadIaCProviderFromConfig returns nil when no iac.provider module in config) - runInfraBootstrap: defer iacCloser.Close() with error logging (consistent with bootstrapStateBackend's provider shutdown logging) - bootstrapSecrets: always capture oldCredentialID for provider_credential force-rotate regardless of credRevoker nil-ness — enables the "no revoker available" warning to include the credential ID for manual revocation - bootstrapSecrets: handle ErrUnsupported from Get with explicit warning (write-only stores like GitHub Actions can't expose the old credential ID; operator warned) - Test: TestInfraBootstrap_ForceRotate_ProviderCredential_WriteOnlyStore — verifies that on ErrUnsupported the revoker is NOT called (old ID unknown) and new credential is still stored Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
+291
to
+292
| // provider_credential rotation is requested or the provider doesn't implement | ||
| // ProviderCredentialRevoker (a warning is logged in that case). |
| } else if errors.Is(getErr, secrets.ErrUnsupported) { | ||
| // Write-only provider (e.g. GitHub Actions) — Get is not supported. | ||
| // Revocation will be skipped; warn the operator. | ||
| fmt.Fprintf(os.Stderr, "warn: secrets provider does not support Get — cannot read old %s for revocation; revoke manually after rotation\n", accessKeyName) |
Comment on lines
+530
to
+533
| // --force-rotate path: for provider_credential, read the OLD access_key_id | ||
| // BEFORE deleting so we can revoke it at the upstream provider after minting | ||
| // the new one (mint-new-then-revoke-old; see ADR 0012). | ||
| // For other types, delete the existing value (best-effort) so that the |
Comment on lines
+111
to
+115
| // Load the IaC provider for ProviderCredentialRevoker if any force-rotate | ||
| // target is a provider_credential type. We do this lazily (only when needed) | ||
| // so that runs without --force-rotate on provider_credential don't require | ||
| // the plugin binary to be installed. | ||
| revoker, iacCloser := resolveCredentialRevoker(ctx, cfgFile, secretsCfg, forceRotate) |
| // | ||
| // source is the provider_credential source string (e.g. "digitalocean.spaces"). | ||
| // credentialID is the provider-specific identifier of the OLD credential | ||
| // (e.g. the access_key_id for DO Spaces). |
Comment on lines
+25
to
+29
| Enable `--force-rotate` for `provider_credential` secrets with the following ordering guarantee: | ||
|
|
||
| 1. **Read** the OLD `access_key_id` from the secrets store before any deletion. | ||
| 2. **Delete** all existing sub-keys from the secrets store. | ||
| 3. **Mint** new credentials via `secrets.GenerateSecret` (calls the provider API). |
Comment on lines
+73
to
+78
| - Requires explicit `--force-rotate <name>` flag — never triggered automatically. | ||
| - Validates against `secrets.generate[]` first (fast-fail on typos). | ||
| - `infra_output` types are still rejected (they are derived from apply state, not generated). | ||
| - The OLD `access_key_id` is captured from the secrets store before deletion. If the store | ||
| doesn't expose Get (write-only provider like GitHub Actions), revocation is skipped with a | ||
| warning. |
…ported Get error handling - resolveCredentialRevoker docstring: clarify that (nil, iacCloser) is returned when provider doesn't implement ProviderCredentialRevoker (not (nil, nil) as previously stated) - infra_bootstrap.go: log explicit warning for non-ErrUnsupported/ErrNotFound errors from provider.Get when reading old access_key before rotation (operators previously got no indication that revocation would be skipped due to unexpected Get failures) - Terminology: update all references from "access_key_id" to "access_key" to match the actual sub-key name (<name>_access_key) produced by the provider_credential generator — affects infra_bootstrap.go comment, interfaces/iac_provider.go docstring, and ADR 0012 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
--force-rotatehard-rejection forprovider_credentialtype. Previously wfctl blocked this with "must be rotated via the upstream provider" — now enabled with mint-new-then-revoke-old ordering (ADR 0012).ProviderCredentialRevokerinterface tointerfaces/iac_provider.gofollowing the existing optional-interface pattern (Enumerator, DriftConfigDetector). Provider plugins that can revoke credentials implement it; caller type-asserts and falls through to a warning log if not implemented.bootstrapSecretsaccepts an optional variadicProviderCredentialRevoker. Forprovider_credential+--force-rotate: reads OLDaccess_key_idbefore deletion, mints + stores new sub-keys, then revokes old credential via the interface. Revoke failure is non-fatal — new key is never rolled back (see ADR 0012).Changes
interfaces/iac_provider.goProviderCredentialRevokeroptional interfacecmd/wfctl/infra_bootstrap.goprovider_credential; load IaC provider for revocation; implement mint-new-then-revoke-old inbootstrapSecretscmd/wfctl/infra_bootstrap_force_rotate_test.goProviderCredentialRefused→ProviderCredentialAllowed; addMintsAndRevokes+RevokeFailNonFataltestsdecisions/0012-provider-credential-rotation.mdADR 0012 summary
Ordering: Read OLD key → Delete sub-keys → Mint new → Store new → Revoke old.
Revoke failure: non-fatal, log warning, operator revokes manually. New key is retained.
Alternatives rejected: revoke-then-mint (service disruption window), operator-manual-only (no audit trail).
Test plan
go test ./cmd/wfctl/ -run ForceRotate— all 8 tests pass (3 new, updated 1)go test ./...— full suite greengo build ./cmd/wfctl/— cleanFollow-up
Phase 3:
workflow-plugin-digitaloceanimplementsRevokeProviderCredentialfordigitalocean.spacessource (DELETE /v2/spaces/keys/{access_key_id}).🤖 Generated with Claude Code