feat(rotation): keyring phases 2–5 — recovery PR for orphaned stack - #139
Merged
Conversation
#72) Builds on Phase 1's keyring plumbing with the active machinery needed to actually rotate keys: three new agent endpoints implementing the controller-orchestrated three-phase dance (install → use → remove), the dashboard-side rotator service that drives it, and the `X-Gearbox-Kid` request/response header that Phase 5 will use for drift detection. Agent endpoints --------------- `POST /api/v1/system/keyring/install` - Body `{kid, secret_b64, role?}`. New entries default to `secondary`; the controller flips primary in a separate `/use` call. - Idempotent on `(kid, same_secret)` — re-installing returns 200 with current state. Same kid with a different secret returns 409 so a divergence between dashboard and agent surfaces loudly. - Returns 507 when the keyring is at `MaxKeyRingEntries` (4) — guards against runaway growth from a buggy rotator. `POST /api/v1/system/keyring/use` - Body `{kid}`. Flips named entry to primary, demotes the prior primary to secondary. Both stay accepted; the agent doesn't distinguish primary vs secondary for inbound auth. `DELETE /api/v1/system/keyring/{kid}` - Refuses 409 on the only remaining entry — agent never bricks itself. All three serialize through a handler-level mutex so concurrent controller calls can't race their read-modify-write of the on-disk keyring. The atomic-tmpfile+rename + `atomic.Pointer` swap from Phase 1 means the auth middleware sees the new keyring on the very next request after the swap, no restart. Dashboard side -------------- `agent.Client` - New `NewClientWithKID(url, key, kid)` constructor + `WithKID(kid)` setter. Existing `NewClient` callers untouched. - All outbound requests now go through a `setAuthHeaders(req)` helper that sets `Authorization: Bearer …` and, when the client was built with a kid, the `X-Gearbox-Kid` request header. Agent middleware echoes the matched kid in the response header of the same name; Phase 5 compares the two to detect drift. - New `KeyRingGet`, `KeyRingInstall(kid, secret, role)`, `KeyRingUse(kid)`, `KeyRingDelete(kid)` methods. `services/agent_keyring` - New package containing the `Rotator`, which composes `RotateBox(boxID, overlap)` from the install/use/remove primitives: decrypt current primary → build authenticated client → install new key on agent as secondary → persist new key to box_agent_keys → flip primary on agent → flip primary in DB (which also stamps `retired_at` on the old entry). - `CleanupRetiredKeys(boxID, overlap)` removes any entries whose `retired_at` is older than the overlap window. Uses `time.Since` for the cutoff comparison so SQLite-driver timezone behaviour doesn't bite (modernc.org/sqlite scans bare DATETIMEs in local time; comparing against `time.Now()` in UTC would otherwise mis- fire). Removes from agent first, then DB; tolerates the agent having already lost the entry (404) or refusing to remove the last key (409) since neither leaves us in a bad state. - `DefaultOverlapWindow = 24h`. Tunable per-call so Phase 4's scheduler can pick the operator's configured value. `SetBoxPrimaryKey` now stamps `retired_at` with `time.Now().UTC()` explicitly rather than letting SQLite emit `CURRENT_TIMESTAMP`, so the value round-trips correctly through the driver's date parser. Tests ----- - 11 keyring-endpoint integration tests (agent side): install adds secondary, install is idempotent, KID collision with different secret returns 409, malformed secret returns 400, use flips primary, use of unknown kid returns 404, delete works, delete of only-remaining-entry returns 409, mutations persist across in-process reload + on-disk reload, keyring file mode is 0600, install over MaxKeyRingEntries returns 507. - 4 rotator integration tests against a `httptest` mock of the agent's keyring API (the dashboard module can't import the agent module): happy path covers full install → use → DB-flip, cleanup removes retired keys past the overlap window, cleanup leaves keys within the overlap window alone, missing-box returns error. - All existing tests in both modules still pass. Refs: Phase 2 of the implementation plan posted to #72. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Surfaces the Phase 2 rotator behind two operator-visible buttons —
the bit that makes the keyring work actually usable. No new
back-end abstractions; just two thin handlers that compose the
existing rotator with the dashboard's box management.
Backend
-------
`POST /settings/boxes/{id}/rotate-key`
- Rotates one box. Returns 200 with `{success, new_kid, old_kid,
retire_after}` or 4xx/5xx with `{success: false, message}`.
- Constructs a rotator per request from the handler's existing DB +
encryptor — no new singletons.
`POST /settings/boxes/rotate-key-all`
- Iterates every enabled box and rotates each through the same
rotator. Reports per-box success/failure so the operator sees
exactly which boxes need follow-up. Failures on one box don't
halt the run; this matches the homelab use case better than a
strict circuit-breaker — operator decides whether to investigate
one bad box or move on.
Both routes are wired into the admin-only `/settings` group in
cmd/server/main.go alongside the existing box CRUD routes; same
permission gate as `HAProxyBoxUpdatePost` etc.
UI
--
Box edit form (`HAProxyBoxEditPage`)
- New "Rotate API key" section under the API-Key field with a Rotate
Key button. Visible only on edit (server != nil).
- Click → `showConfirmDialog` (warning style) explaining the 24h
overlap → POST → toast on success or alert dialog on failure.
- Reuses the in-page rotate-spinner SVG to surface in-flight state.
Boxes list (`HAProxyBoxesPageContent`)
- New "Rotate All Keys" button next to the existing "Add Box"
button. Visible only when at least one box exists.
- Click → confirm → POST → success toast or alert dialog with a
per-box failure list when partial.
JS uses the established `showConfirmDialog` / `showAlertDialog` /
`showToast` APIs from `layouts.Base`, per the CLAUDE.md "never use
native confirm/alert/prompt" rule.
Tests
-----
No new tests — the rotator's behaviour is already covered by
`services/agent_keyring/rotator_test.go` (Phase 2). The handlers
are thin enough that adding HTTP-level tests would duplicate the
rotator-side coverage. Browser-level testing of the new UI was not
performed in this commit; operator should smoke-test by hitting
both buttons end-to-end before merging.
Refs: Phase 3 of the implementation plan posted to #72.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a background sweeper that walks every enabled box on a tick and removes any keys whose retired_at + overlap window has passed — completing the install→use→remove three-phase rotation cycle without operator intervention. Originally Phase 4 in the issue plan also covered auto-rotation on a schedule (off by default, configurable cadence). That needs a new global-settings surface in the dashboard (no app-level config table exists today; only user_preferences), which is its own design pass. Deferring the auto-rotate scheduler to a follow-up so this PR stays focused on the piece that's actually needed regardless of whether auto-rotation is enabled. What's here ----------- `services/agent_keyring/cleaner.go` - `RetiredKeyCleaner` runs as a goroutine off the dashboard's process-lifetime context. - Hourly tick (`CleanerInterval`) — short enough that a 24h rotation cleans up within a few hours of its target, long enough that the sweep is cheap. - Immediate sweep on start so a freshly-deployed dashboard catches up on any retired keys left from manual rotations done while the prior instance was down. - Per-box failures are logged but don't halt the sweep; one unreachable agent shouldn't block cleanup on the others. `cmd/server/main.go` - Wires the cleaner into startup alongside the existing alert evaluator. Cancelled when main returns. Tests ----- - `TestCleaner_RemovesRetiredKeyOnTick` — rotates a box, then runs the cleaner with a 1ms overlap and 20ms interval; verifies the retired key is removed from both the mock agent and the DB. - `TestCleaner_NoopWhenNothingRetired` — no rotation happens, so the cleaner finds nothing to do; verifies the seeded entry survives a sweep. Refs: Phase 4 (lite) of the implementation plan posted to #72. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the dashboard signs an outbound request with kid X but the agent matches kid Y instead (because rotation propagated on one side but not the other), drift detection logs the disagreement so an operator can resync. Closes the observability loop on the install→use→remove cycle: Phase 2 added the request header, the agent's auth middleware already echoes the matched kid, and this commit wires the dashboard-side comparison. What's here ----------- `agent.HeaderKID = "X-Gearbox-Kid"` is the shared constant the dashboard sends and the agent echoes back. The agent's middleware sets it on every authenticated response (see Phase 1). `agent.Client` - `SetDriftHandler(DriftHandler)` installs an optional callback invoked when `resp.Header.Get(HeaderKID)` differs from the kid the client was built with. Reads c.onDrift at RoundTrip time, so the handler can be installed AFTER construction (typical for long- lived per-box clients held by the dashboard). - Transport wrap: `kidObservingTransport` sits between the http client and the underlying TLS transport, calling `c.checkDrift` on every successful response. One central point of inspection — no invasive edits to every `doRequest*` method. - `LogDriftHandler(logger, boxID)` builds a ready-to-use DriftHandler that emits a structured warn-level log. Lowest-friction wiring for the long-lived clients held in the WebSocketManager. Test fix -------- `TestClientTimeout` was a flaky pre-existing test whose substring check was case-sensitive — Go's net/http error message capitalises "Timeout" sometimes and emits "context deadline exceeded" other times. Fixed by lower-casing the error message before substring matching. Verified stable across 5 runs. Tests ----- `client_drift_test.go` exercises the four corners of the matrix: - Drift handler fires when kid mismatches. - Doesn't fire when kid matches. - Doesn't fire when the agent omits the header (older agents). - Doesn't fire when the dashboard client has no kid. Not yet wired into production code ---------------------------------- Adding `agent.LogDriftHandler` is the small API surface; deciding *where* to call SetDriftHandler is a separate design choice (WebSocketManager? capability poller? every short-lived handler.agentClient()?). Deferring that integration so this PR stays focused on the observability primitive itself. A follow-up can install LogDriftHandler at every site that constructs a kid- bearing client. Refs: Phase 5 of the implementation plan posted to #72. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Recovery PR that re-lands phases 2–5 of the API-key rotation work (issue #72): the agent's install/use/remove keyring endpoints, the dashboard's rotator service + manual rotate UI (per-box and rotate-all), a retired-key cleanup goroutine, and X-Gearbox-Kid drift detection wired through a wrapping HTTP transport on agent.Client.
Changes:
- Agent gains
/api/v1/system/keyring/install|use|{kid}mutation endpoints with mutex-serialized writes and tmpfile-persistent state; dashboard learns matchingKeyRingInstall/Use/Deleteclient methods. - New
services/agent_keyringpackage:Rotatororchestrates the three-phase install→use→remove dance,RetiredKeyCleanerbackground goroutine sweeps demoted keys after the overlap window. - New dashboard handlers (
HAProxyBoxRotateKeyPost,HAProxyBoxesRotateKeyAllPost), UI buttons in box edit form and boxes list, andkidObservingTransport+SetDriftHandler/LogDriftHandleronagent.Clientfor Phase 5 drift detection.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| gearbox/internal/framework/templates/pages/haproxy_settings.templ | Adds "Rotate Key" and "Rotate All Keys" buttons + JS handlers. |
| gearbox/internal/framework/services/agent_keyring/rotator.go | New Rotator orchestrating the install→use→remove flow. |
| gearbox/internal/framework/services/agent_keyring/rotator_test.go | Mock-agent-based rotator tests. |
| gearbox/internal/framework/services/agent_keyring/keygen.go | KID/secret generation helpers. |
| gearbox/internal/framework/services/agent_keyring/cleaner.go | RetiredKeyCleaner background goroutine. |
| gearbox/internal/framework/services/agent_keyring/cleaner_test.go | Cleaner happy-path / no-op tests. |
| gearbox/internal/framework/handler/haproxy_rotate_key.go | HTTP handlers for single-box and all-boxes rotation. |
| gearbox/internal/framework/database/box_agent_keys.go | Use Go-side UTC timestamp for retired_at to avoid SQLite TZ parse drift. |
| gearbox/internal/framework/agent/keyring.go | Dashboard client methods for the agent keyring endpoints. |
| gearbox/internal/framework/agent/client.go | Adds kid header, drift handler, observing transport, setAuthHeaders helper. |
| gearbox/internal/framework/agent/client_test.go | Case-insensitive timeout substring match. |
| gearbox/internal/framework/agent/client_drift_test.go | New drift detection tests. |
| gearbox/cmd/server/main.go | Wires up the retired-key cleaner and new routes. |
| gearbox-agent/internal/api/keyring.go | Implements install/use/remove handlers with mutex + idempotency. |
| gearbox-agent/internal/api/keyring_test.go | Endpoint integration tests. |
| gearbox-agent/cmd/gearbox-agent/main.go | Passes cfg.KeyRingPath through to the handler. |
Comment on lines
+109
to
+114
| _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| "success": successCount == len(boxes), | ||
| "rotated": successCount, | ||
| "total": len(boxes), | ||
| "results": results, | ||
| }) |
Comment on lines
+58
to
+63
| // HAProxyBoxesRotateKeyAllPost rotates every enabled box sequentially | ||
| // with a small stagger between rotations (avoids hammering the | ||
| // agents). Reports per-box outcomes; the operator sees which boxes | ||
| // succeeded and which failed and can act on each. | ||
| // | ||
| // Wired at POST /settings/boxes/rotate-key-all. Body is empty. |
Comment on lines
+43
to
+48
| result, err := rotator.RotateBox(id, agent_keyring.DefaultOverlapWindow) | ||
| if err != nil { | ||
| h.logger.Warn("rotate-key: failed", "box_id", id, "error", err) | ||
| writeRotateError(w, http.StatusBadGateway, "rotation failed: "+err.Error()) | ||
| return | ||
| } |
Comment on lines
+91
to
+107
| results := make([]boxResult, 0, len(boxes)) | ||
| successCount := 0 | ||
| for _, box := range boxes { | ||
| out := boxResult{BoxID: box.ID, Name: box.Name} | ||
| rr, rerr := rotator.RotateBox(box.ID, agent_keyring.DefaultOverlapWindow) | ||
| if rerr != nil { | ||
| out.Success = false | ||
| out.Error = rerr.Error() | ||
| h.logger.Warn("rotate-all: box failed", "box_id", box.ID, "name", box.Name, "error", rerr) | ||
| } else { | ||
| out.Success = true | ||
| out.NewKID = rr.NewKID | ||
| out.OldKID = rr.OldKID | ||
| successCount++ | ||
| } | ||
| results = append(results, out) | ||
| } |
Comment on lines
+110
to
+141
| // Step 1 — install on the agent as secondary. | ||
| if _, err := client.KeyRingInstall(newKID, newSecret, "secondary"); err != nil { | ||
| return nil, fmt.Errorf("agent install: %w", err) | ||
| } | ||
|
|
||
| // Step 2 — persist the new key on our side. The on-disk format | ||
| // matches the existing legacy entry: AES-256-GCM ciphertext of the | ||
| // 64-char hex form of the secret, so callers can decrypt uniformly. | ||
| encrypted, err := r.encryptor.EncryptString(hex.EncodeToString(newSecret)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("encrypt new secret: %w", err) | ||
| } | ||
| if err := r.db.InsertBoxAgentKey(&database.BoxAgentKey{ | ||
| BoxID: boxID, | ||
| KID: newKID, | ||
| SecretEncrypted: encrypted, | ||
| Role: "secondary", | ||
| }); err != nil { | ||
| return nil, fmt.Errorf("insert new key row: %w", err) | ||
| } | ||
|
|
||
| // Step 3 — flip primary on the agent. Both keys remain accepted. | ||
| if _, err := client.KeyRingUse(newKID); err != nil { | ||
| return nil, fmt.Errorf("agent use: %w", err) | ||
| } | ||
|
|
||
| // Step 4 — flip primary in our DB. Demoted entry gets retired_at | ||
| // stamped so CleanupRetiredKeys can sweep it once overlapWindow | ||
| // elapses. | ||
| if err := r.db.SetBoxPrimaryKey(boxID, newKID); err != nil { | ||
| return nil, fmt.Errorf("flip db primary: %w", err) | ||
| } |
| } | ||
| writeError(w, http.StatusBadRequest, "add entry: "+err.Error()) | ||
| return | ||
| } |
Comment on lines
+829
to
+835
| } else { | ||
| await showAlertDialog({ | ||
| title: 'Rotation failed', | ||
| message: data.message || 'Unknown error during rotation.', | ||
| type: 'error', | ||
| }); | ||
| } |
Comment on lines
+358
to
+361
| const msg = failures.map(r => r.name + ' (id=' + r.box_id + '): ' + r.error).join('\n'); | ||
| await showAlertDialog({ | ||
| title: 'Some rotations failed', | ||
| message: 'Succeeded: ' + data.rotated + ' / ' + data.total + '\n\nFailed:\n' + msg, |
Comment on lines
+361
to
+362
| keyringCleanerCtx, cancelKeyringCleaner := context.WithCancel(context.Background()) | ||
| defer cancelKeyringCleaner() |
Comment on lines
+117
to
+123
| func writeRotateError(w http.ResponseWriter, status int, msg string) { | ||
| w.WriteHeader(status) | ||
| _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| "success": false, | ||
| "message": msg, | ||
| }) | ||
| } |
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
Recovery PR for the keyring rotation work that got tangled by the cascade-merge order this morning. PRs #129, #130, #131, #132 were stacked on each other; when I attempted to merge them after #128 landed, only #130 and #132 "merged" — but they merged into their stacked base branches (
feature/issue-72-phase-2-rotation-endpointsandfeature/issue-72-phase-4-cleanup-scheduler), not intomain. #129 and #131 got closed without merging during the cascade.This PR cherry-picks the four phase commits onto a fresh branch off current
mainso the content lands in one squash. Net effect is equivalent to the original PR #129 + #130 + #131 + #132 merging in order:Tracks issue #72.
Test plan
go build ./...clean in bothgearbox/andgearbox-agent/go vet ./...cleango test -count=1 ./...passes in both modules —agent_keyringpackage'scleaner_test.goandrotator_test.gopass; agent'scryptokeyring tests pass; gearbox-agent'skeyring_test.gofor the install/use/remove endpoints passesWhy not just reopen #129–#132?
GitHub returns
Could not open the pull requestwhen the head branch's commits no longer diff cleanly against any open base — the stacked bases (feature/issue-72-phase-1-keyring, etc.) were deleted as part of the cascade merges, and the head branches now contain a mix of original + auto-merge commits that don't cleanly rebase. A fresh PR is the cheapest recovery path.🤖 Generated with Claude Code