fix(secrets): prevent duplicate DO Spaces key creation#732
Merged
Conversation
DO does not enforce name uniqueness on Spaces keys. The provider_credential bootstrap path POSTed blindly every run; if the bootstrap layer failed to persist the access_key+secret_key pair to the configured store (or skipped existsInProvider for any reason), the next run created another key with the same name. Repeated runs accreted orphaned keys until the account hit DO's 200-key quota. Each orphan is unrecoverable — DO returns secret_key once at creation time, and List returns access_key only. Changes: - generateDOSpacesKey() now does a list-then-create. Pre-create call to https://api.digitalocean.com/v2/spaces/keys checks for a matching name; if found, returns an error naming the existing access_key with explicit recovery guidance (delete via console OR --force-rotate). The POST is never attempted in the conflict case. - New lookupExistingSpacesKey() helper paginates through Spaces keys (10 × 100 cap) matching by exact name. Used by the pre-check AND by the 403 error branch (account-quota vs name-conflict disambiguation hint). - Improved error wrapping: every "DO spaces key create" error now includes name=%q so the operator can correlate logs with DO console entries. - stderr trace line on every create attempt: name + bucket + grant permission, so the failure mode is visible without re-running with extra flags. Tests: - TestGenerateDOSpacesKey_RejectsDuplicateNameBeforeCreate verifies the pre-check short-circuits and POST is never attempted when a duplicate exists. (postCalls == 0 asserted.) - TestGenerateDOSpacesKey_403QuotaWithoutNameConflict covers the fallback hint when no name match but DO still returns 403. - TestLookupExistingSpacesKey_PaginatedHit + _Miss cover the pager helper. - Existing TestGenerateSecret_ProviderCredential_DOSpaces and _NoBucket / _WithBucket tests pass without changes — their GET handlers return 404 (no existing key) so the create path proceeds. - TestGenerateDOSpacesKey_IncludesCreatedAt updated to stub both the new GET and the existing POST. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rning Two follow-ups to the duplicate-creation fix: 1. Refuse the default name fallback. The prior default \`workflow-spaces-key\` was project-shared; multiple workflow- managed projects in one DO account would all try to create or adopt the same key. Force the caller to set a project-unique slug (e.g. \`multisite-deploy-key\`, \`wfcompute-deploy-key\`). 2. Emit a stderr WARN when permission=fullaccess is granted (i.e. no \`bucket:\` configured). Bootstrap necessarily uses fullaccess because the IaC state bucket does not exist yet, but once it does the operator should rotate to a bucket- scoped key via \`wfctl secrets rotate --target SPACES --bucket <state-bucket>\` to limit blast radius. The warning includes the exact rotate command. Test updates: - WithBucket / IncludesCreatedAt / NoBucket tests now pass an explicit name (was relying on the default). - New TestGenerateDOSpacesKey_RequiresName covers the refuse- default-name path. 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 hardens the DigitalOcean Spaces provider_credential secret generator to avoid creating duplicate-named Spaces keys (and thereby orphaning unrecoverable secret_key values) by adding a list-before-create check and better diagnostics.
Changes:
- Added
lookupExistingSpacesKey(paged list) and a pre-create duplicate-name guard ingenerateDOSpacesKey. - Expanded DO Spaces key creation errors with disambiguation hints for the “key quota exceeded” 403 case.
- Added/updated tests to cover duplicate-name rejection, pagination, and 403 hint behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| secrets/generators.go | Adds a DO Spaces key name existence lookup + pre-check and enhances error messaging/hints. |
| secrets/generators_test.go | Updates existing stubs for list+create flow and adds new unit tests for the new behaviors. |
Comment on lines
220
to
+224
| } else { | ||
| // fullaccess is required for bootstrap (the IaC state bucket | ||
| // does not exist yet, so the key cannot be scoped to it). | ||
| // Once the bucket is created, the operator should rotate to a | ||
| // bucket-scoped key via the rotate-and-prune flow. |
Comment on lines
+241
to
+243
| // secret store, but a single save-failure leaves us with an | ||
| // orphaned DO key whose secret_key is no longer retrievable. | ||
| // Each subsequent run then creates *another* orphan, accreting |
Comment on lines
274
to
277
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK { |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…ntial
Round 2 of the DO Spaces orphan fix (workflow#732 follow-up).
Round 1 (already in this PR) added a list-before-create pre-check so
the generator refuses to POST a duplicate name. That prevents NEW
orphans whose root cause is a save-failure-after-mint loop.
This round closes the other half: the orphan-creation event itself.
cmd/wfctl/infra_bootstrap.go — bootstrapSecrets provider_credential
path now:
1. Extracts access_key + created_at BEFORE the Set loop (was inside
the loop, so a first-iteration Set failure left the rollback path
blind to the just-minted credential).
2. On ANY Set failure inside the loop, invokes credRevoker.
RevokeProviderCredential with the just-minted access_key. If no
revoker is registered, emits a loud ORPHANED-CREDENTIAL warning
to stderr with the wfctl list-orphans recovery command pre-filled.
3. Rollback is best-effort: failure does not mask the original Set
error.
cmd/wfctl/secrets_orphans.go — new \`wfctl secrets list-orphans\`
subcommand:
wfctl secrets list-orphans --source digitalocean.spaces \
--name workflow-spaces-key # dry-run
wfctl secrets list-orphans --source digitalocean.spaces \
--name workflow-spaces-key --delete # delete all matches
Lists every upstream credential whose name field equals --name
(DO Spaces does not enforce name uniqueness, so a single name can
map to many access_keys after the orphan loop). Bounded pager
(100 × 100 = 10 000 keys). Per-orphan delete continues on failure
to surface every recoverable orphan.
Currently supports digitalocean.spaces; extending to other sources
adds one switch arm.
Tests:
- TestBootstrapSecrets_ProviderCredential_RollbackOnSetFailure —
Set(SPACES_secret_key) returns error; assert revoker invoked with
AK_ORPHAN; assert original error surfaces.
- TestBootstrapSecrets_ProviderCredential_RollbackOnFirstSetFailure —
Set(SPACES_access_key) (the very first call) returns error;
asserts the access_key extraction reorder is correct (revoker
must receive AK_FIRST even though we never even reached the
access_key iteration).
These two tests reproduce the pre-fix orphan-creation bug
(generator succeeds + Set fails → DO key remains, no rollback
attempted). Without the rollback wiring they fail with
"expected 1 rollback-revoke call; got 0".
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-existing G204 false positive on a fixed-binary + url-escaped endpoint subprocess. Documented inline; no behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
199
to
208
| name, _ := config["name"].(string) | ||
| if name == "" { | ||
| name = "workflow-spaces-key" | ||
| // Refuse the default fallback. A shared default name causes | ||
| // cross-project collisions in any account that runs more than | ||
| // one workflow-managed deploy — every project would try to | ||
| // create or adopt the same `workflow-spaces-key`. Force the | ||
| // operator to name the key per-project (e.g. | ||
| // `multisite-deploy-key`, `wfcompute-deploy-key`). | ||
| return "", fmt.Errorf("secrets: provider_credential digitalocean.spaces: `name` is required (use a project-unique slug like \"<project>-deploy-key\"; a shared default would collide across projects in the same DO account)") | ||
| } |
| // does not exist yet, so the key cannot be scoped to it). | ||
| // Once the bucket is created, the operator should rotate to a | ||
| // bucket-scoped key via the rotate-and-prune flow. | ||
| fmt.Fprintf(os.Stderr, "secrets: WARN provider_credential digitalocean.spaces name=%q is being granted permission=fullaccess (no `bucket:` configured); rotate to a bucket-scoped key after first apply via `wfctl secrets rotate --target SPACES --bucket <state-bucket>` to limit blast radius.\n", name) |
Comment on lines
+740
to
+741
| fmt.Fprintf(os.Stderr, "warn: provider_credential %q minted access_key=%s but no revoker available; the upstream credential is now ORPHANED and unrecoverable. Run `wfctl secrets list-orphans --source %s --name %s` to clean up. Original error: %v\n", | ||
| gen.Key, newAccessKey, gen.Source, gen.Key, reason) |
| fmt.Printf(" secret %q: created\n", fullKey) | ||
| } | ||
| } | ||
| _ = newCreatedAt // captured into RotationResult below |
Comment on lines
+88
to
+92
| // | ||
| // #nosec G204 -- the binary is the fixed string "gh" and `endpoint` is | ||
| // constructed from a literal-prefix + URL-escaped query inside this | ||
| // package (urlQueryEscape sanitises). No user-controlled shell metachars | ||
| // flow into the subprocess. |
list-orphans was using local page-counter increments. Earlier runs truncated at 100 because per_page was 100 + the locally-bounded loop returned after page 1. DO's Spaces Keys API allows per_page up to 200 and returns an absolute next-page URL in links.pages.next; we now follow that URL rather than incrementing locally. Also emits a stderr scan summary so debugging is observable from the CI log without re-running with extra flags. 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.
DO doesn't enforce name uniqueness on Spaces keys. The provider_credential bootstrap was POSTing blindly each run, accreting orphaned keys whenever the save-to-store path failed (each orphan unrecoverable because DO only returns secret_key at create-time). This adds a list-then-create pre-check that names the conflicting key + suggests delete-via-console OR
--force-rotate. 5 new tests; existing tests pass.