test(secrets): failing test for generateDOSpacesKey created_at (PR4a Task 7)#582
Merged
Conversation
…s_key + secret_key
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new (currently failing) regression test in secrets to require generateDOSpacesKey to surface DigitalOcean’s created_at timestamp in its JSON output, which is needed for the upcoming SpacesKeyDriver IaC resource to match/adopt keys by creation time.
Changes:
- Adds
TestGenerateDOSpacesKey_IncludesCreatedAtassertingaccess_key,secret_key, andcreated_atare present in the generator output. - Introduces an httptest stub response that includes a
created_atfield.
Comment on lines
+282
to
+286
| t.Setenv("DIGITALOCEAN_TOKEN", "stub") | ||
| t.Setenv("DIGITALOCEAN_API_URL", srv.URL) // hook used by generateDOSpacesKey for tests | ||
|
|
||
| raw, err := generateDOSpacesKey(context.Background(), map[string]any{"name": "test-key"}) | ||
| if err != nil { |
Comment on lines
+267
to
+269
| if r.URL.Path != "/v2/spaces/keys" || r.Method != http.MethodPost { | ||
| t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) | ||
| } |
…crets Adds TestBootstrapSecrets_StorageFilter_OnlyPersistsSubKeys: with the post-Task-8 generateDOSpacesKey shape (access_key + secret_key + created_at), bootstrapSecrets must store only the canonical sub-keys defined in providerCredentialSubKeys. The created_at sidecar field must NOT leak into GH Secrets, where it would surface as a phantom SPACES_created_at entry and break the audit-keys/prune contract. This is the failing test for Task 9 of the spaces-key-iac-resource plan (PR4a). Task 10 will implement the sub-key allow-list filter in bootstrapSecrets to make it pass. Plan: docs/plans/2026-05-08-spaces-key-iac-resource.md (commit 316559f7), Task 9 of PR4a. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Task 9 stacked — Adds Task 10 (implementer-3) will add the sub-key allow-list filter in |
Comment on lines
+168
to
+175
| // DIGITALOCEAN_API_URL is a test-only hook so unit tests can redirect | ||
| // generateDOSpacesKey at a httptest.Server. In production the env var is | ||
| // unset and we fall through to the canonical DO endpoint. | ||
| apiURL := "https://api.digitalocean.com" | ||
| if v := os.Getenv("DIGITALOCEAN_API_URL"); v != "" { | ||
| apiURL = v | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, apiURL+"/v2/spaces/keys", bytes.NewReader(body)) |
Comment on lines
+171
to
+175
| apiURL := "https://api.digitalocean.com" | ||
| if v := os.Getenv("DIGITALOCEAN_API_URL"); v != "" { | ||
| apiURL = v | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, apiURL+"/v2/spaces/keys", bytes.NewReader(body)) |
Comment on lines
193
to
208
| var result struct { | ||
| Key struct { | ||
| AccessKey string `json:"access_key"` | ||
| SecretKey string `json:"secret_key"` | ||
| CreatedAt string `json:"created_at"` | ||
| } `json:"key"` | ||
| } | ||
| if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { | ||
| return "", fmt.Errorf("secrets: DO spaces key parse: %w", err) | ||
| } | ||
|
|
||
| out, err := json.Marshal(map[string]string{ | ||
| "access_key": result.Key.AccessKey, | ||
| "secret_key": result.Key.SecretKey, | ||
| "created_at": result.Key.CreatedAt, | ||
| }) |
Comment on lines
+188
to
+191
| // This is the failing test for Task 9 of the spaces-key-iac-resource plan. | ||
| // Until Task 10 implements the sub-key allow-list filter in bootstrapSecrets, | ||
| // this test fails at the SPACES_created_at assertion. | ||
| func TestBootstrapSecrets_StorageFilter_OnlyPersistsSubKeys(t *testing.T) { |
…sub-keys (ADR 0020)
bootstrapSecrets now persists only the canonical provider_credential sub-keys
listed in providerCredentialSubKeys[gen.Source]. Sidecar metadata (e.g.
created_at returned by generateDOSpacesKey since Task 8) is captured into a
new RotationResult value AND emitted to stderr as `WFCTL_NEW_KEY_<UPPER>=<v>`
lines for operator observability + audit logs, but is no longer stored as a
GH Secret. This closes the contract gap where every bootstrap of a
digitalocean.spaces credential would produce a phantom SPACES_created_at
secret that audit-keys/prune cannot reconcile against the upstream provider.
Behavior changes:
- bootstrapSecrets is now declared as `var bootstrapSecrets = func(...)` so
Task 20 (rotate-and-prune) can override it without going through the real
provider/network path. Same pattern as the existing `var generateSecret`
test hook.
- New return shape: (map[string]string, []RotationResult, error). The
rotations slice is non-empty only when --force-rotate triggered new
credential creation; nil for idempotent skip-if-exists runs. Eliminates
the subprocess + stderr-parse pattern PR-side that Task 21 would have
needed.
- New exported type RotationResult{Key, Source, AccessKey, CreatedAt}.
- 18 test callsites + 1 production caller updated for the new signature
(rotations slice discarded as `_` everywhere except future Task 21).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Comment on lines
657
to
+660
| if gen.Type == "provider_credential" { | ||
| var subKeyMap map[string]string | ||
| if jsonErr := json.Unmarshal([]byte(value), &subKeyMap); jsonErr == nil { | ||
| allowedSubKeys, ok := providerCredentialSubKeys[gen.Source] |
Comment on lines
+171
to
+175
| apiURL := "https://api.digitalocean.com" | ||
| if v := os.Getenv("DIGITALOCEAN_API_URL"); v != "" { | ||
| apiURL = v | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, apiURL+"/v2/spaces/keys", bytes.NewReader(body)) |
Comment on lines
+260
to
+264
| // TestGenerateDOSpacesKey_IncludesCreatedAt asserts that generateDOSpacesKey | ||
| // surfaces the DO API's `created_at` timestamp alongside access_key+secret_key | ||
| // in its JSON output. This is required by the upcoming SpacesKeyDriver IaC | ||
| // resource (PR4b), which keys observed-key adoption on creation timestamps. | ||
| func TestGenerateDOSpacesKey_IncludesCreatedAt(t *testing.T) { |
Comment on lines
+188
to
+190
| // This is the failing test for Task 9 of the spaces-key-iac-resource plan. | ||
| // Until Task 10 implements the sub-key allow-list filter in bootstrapSecrets, | ||
| // this test fails at the SPACES_created_at assertion. |
…verride security risk Removes the DIGITALOCEAN_API_URL env-var override from generateDOSpacesKey that was added in Task 8 (commit 1f03a7d). The override was honored unconditionally in production: an attacker who could set the env var (malicious .env, hostile CI step, multi-tenant runner, compromised dependency) would silently redirect the `Authorization: Bearer <DIGITALOCEAN_TOKEN>` POST to their own server, exfiltrating the production credential with no detection signal. Switches TestGenerateDOSpacesKey_IncludesCreatedAt (Task 7's failing test, commit 03a9ad7) to the package's existing rewriteTransport helper — defined in secrets/github_provider_test.go and already used by the three sister DOSpaces tests at generators_test.go lines 107-109, 156-158, 211-213. Hermetic in-test mechanism, zero production attack surface. Plan deviation recorded as ADR 0021. The original plan (docs/plans/2026-05-08-spaces-key-iac-resource.md, Tasks 7-8) literally prescribed the env-var hook; security review caught it. Per feedback_proper_fixes_over_workarounds we fix the mechanism, not patch around it. Verification: - `GOWORK=off go test ./secrets -run TestGenerateDOSpacesKey_IncludesCreatedAt -v` → PASS - `GOWORK=off go test ./secrets` → all PASS - `GOWORK=off go build ./...` → clean Thanks to code-reviewer (and Copilot at PR #582 comment 3212559090) for catching this — exactly the class of issue the 2-stage review exists for. Co-Authored-By: Claude Opus 4.7 <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
Adds a failing test for
generateDOSpacesKeythat asserts the function surfaces the DO API'screated_attimestamp alongsideaccess_key+secret_keyin its JSON output.This is Task 7 of PR4a in the
spaces-key-iac-resourceplan. Per ADR 0020 same-commit constraint, PR4a Tasks 7 (failing test) → 8 (impl) → 9 (storage-filter test) → 10 (storage-filter impl) all land on this branch and squash-merge atomically.Why
created_at?The upcoming
SpacesKeyDriverIaC resource (PR4b) keys observed-key adoption on creation timestamps — withoutcreated_atflowing throughgenerateDOSpacesKey, the driver cannot match an in-state key to its remote counterpart.Test failure mode
Running
go test ./secrets -run TestGenerateDOSpacesKey_IncludesCreatedAt -vagainst this commit fails — the current implementation only extractsaccess_key+secret_keyfrom the response. (Failure mode in CI without the env-var hook resolves to aDIGITALOCEAN_API_URLtest stub that Task 8 will honor; on a network-attached runner it fails with HTTP 401 from real DO sinceDIGITALOCEAN_TOKEN=stub.)Test plan
go test ./secrets -run TestGenerateDOSpacesKey_IncludesCreatedAt -vFAILSDIGITALOCEAN_API_URLtest hook → test PASSESbootstrapSecretsstorage-filterPlan reference
docs/plans/2026-05-08-spaces-key-iac-resource.md(commit 316655f7),### Task 7:section.🤖 Generated with Claude Code