feat(rotation): Phase 1 — multi-key keyring plumbing (#72) - #128
Merged
Conversation
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Implements Phase 1 of API-key rotation plumbing across gearbox-agent and gearbox by introducing an N-entry keyring model (agent) and per-box key storage (dashboard), without changing operator-visible behavior yet.
Changes:
- Agent: adds
crypto.KeyRing+KeyRingPointer, updates auth middleware to accept both newgbx_<kid>_<b64>tokens and legacy 64-hex tokens, and echoesX-Gearbox-Kidon authenticated responses. - Agent: adds
GET /api/v1/system/keyringmetadata endpoint and updates CLI flags to operate on the keyring. - Dashboard: adds
box_agent_keystable + storage primitives for multi-key per-box persistence.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| gearbox/internal/framework/database/migrations/files/000002_add_box_agent_keys.up.sql | Adds box_agent_keys table, indexes, and legacy backfill from boxes.api_key_encrypted. |
| gearbox/internal/framework/database/migrations/files/000002_add_box_agent_keys.down.sql | Drops the new table and indexes. |
| gearbox/internal/framework/database/box_agent_keys.go | DB primitives for keyring rows (get/insert/set-primary/delete/touch). |
| gearbox/internal/framework/database/box_agent_keys_test.go | Adds storage tests and documents current FK-cascade behavior. |
| gearbox-agent/internal/framework/middleware/auth.go | Reworks auth to validate against a keyring and emit X-Gearbox-Kid. |
| gearbox-agent/internal/framework/middleware/auth_test.go | Adds integration tests for both token formats and hot-swap behavior. |
| gearbox-agent/internal/framework/crypto/keyring.go | Introduces keyring type, on-disk persistence (atomic write), migration from legacy api-key file, and token matching. |
| gearbox-agent/internal/framework/crypto/keyring_test.go | Unit tests for token parsing/matching, persistence, migration, and pointer swapping. |
| gearbox-agent/internal/framework/config/config.go | Adds KeyRingPath config and env var support. |
| gearbox-agent/internal/api/server.go | Plumbs keyring pointer through server config for middleware usage. |
| gearbox-agent/internal/api/keyring.go | Adds keyring metadata endpoint handler. |
| gearbox-agent/cmd/gearbox-agent/main.go | Updates startup + CLI flows to load/create and print/rotate keyring-based API keys. |
sarg3nt
added a commit
that referenced
this pull request
May 17, 2026
Nine findings from the Copilot review on PR #128, all valid or worth addressing. Fixed in this commit; replies + thread-resolves go with the push. 1. LoadOrCreateKeyRing fall-through (keyring.go:131-167) Was: any error reading the legacy api-key file (incl. ErrKeyRequired from a missing encryption-key env, or a permission error) silently fell through to generating a fresh keyring — would rotate every dashboard out for a transient operator mistake. Now: distinguish "file doesn't exist" (proceed to fresh-gen) from "file exists but errored / malformed" (return the error to the caller). os.Stat + os.IsNotExist gates the choice explicitly. 2. MatchToken constant-time guarantee (keyring.go ~195) Was: the prefixed-token path returned early on the first kid match, making total runtime depend on which kid the request claimed — kid enumeration via timing. The doc said "All comparisons are constant-time" but the prefixed branch broke that promise. Now: walk every entry, compare both kid and secret with subtle.ConstantTimeCompare, AND the two results. Match is recorded without short-circuit; runtime is uniform regardless of which kid (if any) matches. Doc updated to reflect the actual guarantee. 3. writeKeyRingFile mutates input (keyring.go ~415) Was: the function populated SecretHex on each entry of the passed- in keyring before marshaling. KeyRing values are shared via atomic.Pointer and treated as immutable; mutating in-place risks races with concurrent middleware readers. Now: marshal off a local snapshot whose entries have SecretHex backfilled from Secret where needed. Input is never written to. 4. --rotate-api-key zero CreatedAt (main.go ~155) Was: the fresh KeyRingEntry built for the CLI rotate command omitted CreatedAt, so the keyring file got 0001-01-01T00:00:00Z and the /api/v1/system/keyring metadata exposed the same. Now: CreatedAt: time.Now().UTC(). 5. handleGet nil-guard (api/keyring.go ~50) Was: h.keyring.Load() was dereferenced unconditionally; a future wiring bug that left the pointer nil would panic the agent on every keyring request. Now: nil check + 500 + log line. Fails loud rather than crashing. 6. At-most-one-primary-per-box constraint (migration 000002) Was: nothing in the schema stopped two rows with role='primary' for the same box. SetBoxPrimaryKey's transactional flip is correct, but a buggy code path or a manual DB edit could produce the invalid state and GetBoxPrimaryKey would return an arbitrary row. Now: partial unique index on box_agent_keys(box_id) WHERE role='primary'. SQLite supports this directly; index is dropped in the down migration too. 7. Test naming clarity (box_agent_keys_test.go) Was: TestBoxAgentKeys_MigrationBackfillsLegacyEntry was named as if it validated migration behaviour but actually only exercised InsertBoxAgentKey + GetBoxPrimaryKey roundtrip; the comment also misled. Now: split into two clearly-named tests — InsertAndLookup covers the roundtrip, and a new MigrationBackfillStatementWorks test wipes the migrated rows for a single box, re-executes the migration's INSERT-FROM-boxes statement, and asserts the row appears + reruns are idempotent. 8. DeleteBox cascade gap (servers.go DeleteBox) Was: the schema declared ON DELETE CASCADE but PRAGMA foreign_keys is off in this codebase, so deleting a box left orphaned box_agent_keys rows holding encrypted secrets. Phase 1 docs flagged this as a deferred gap; Copilot pushed back, and fairly — it's a small, contained fix. Now: DeleteBox runs inside a transaction that wipes box_agent_keys WHERE box_id = ? before deleting from boxes. Both succeed or neither does. Test re-added: TestBoxAgentKeys_DeleteBoxClearsDependentKeys. 9. APIKeyAuth nil-guard (middleware/auth.go) Was: keyring.Load() was called without first checking the pointer itself for nil. A miswired ServerConfig would panic on every authenticated request. Now: fail-closed nil check at the top of the request handler — returns 401 + logs at error level. Same defensive treatment as fix #5. Tests ----- All 3 dashboard-side suites pass (`database` package, 7 new tests including the new DeleteBox cascade test). All 3 agent-side suites pass (`crypto`, `middleware`, `api`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Closing + reopening to retrigger CI — synchronize event was silently dropped on the fe3c762 fix commit. |
sarg3nt
added a commit
that referenced
this pull request
May 17, 2026
Add a note that subtle.ConstantTimeCompare's length-dependent fast-fail is fine here because every kid in the system is exactly 6 chars long (kidLength = 6 hex chars; the legacy entry uses 'legacy' which is also 6 chars by deliberate convention). Custom kids of a different length would naturally hash-mismatch — which is the intended failure mode. Also serves to force a synchronize event so PR #128's CI re-runs on the fix commit; the prior synchronize from fe3c762 didn't trigger workflows (still unclear why; not blocking the work). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Foundation for issue #72's rotation work. Adds the data structures and storage required for N-entry keyrings on both the agent and dashboard sides, with no operator-visible behaviour change yet — rotation endpoints and UI follow in Phase 2. Agent side ---------- - `internal/framework/crypto/keyring.go` — `KeyRing` type with up to `MaxKeyRingEntries = 4` accepted keys, atomic tmpfile+rename on disk, AES-256-GCM (GBE1) encryption when `GEARBOX_AGENT_ENCRYPTION_KEY` is set. Wire token format: `gbx_<6-hex-kid>_<base64url(32 random bytes)>`, with legacy 64-hex tokens still accepted for one release cycle. - `LoadOrCreateKeyRing(keyringPath, legacyAPIKeyPath)` migrates an existing `/var/lib/gearbox-agent/api-key` file into a single keyring entry tagged `kid="legacy"`, role=primary. Legacy file stays on disk as a read-only fallback. - `KeyRingPointer` wraps `atomic.Pointer[KeyRing]` so Phase 2's install/use/remove endpoints can swap the live keyring without middleware restart. Verified by the new auth-middleware test `TestAPIKeyAuth_HotSwapVisibleImmediately`. - `internal/framework/middleware/auth.go` rewritten to take a keyring pointer instead of a static key. Accepts both prefixed and legacy token formats; matched `kid` echoed back as `X-Gearbox-Kid:` header on every authenticated response so the dashboard can detect drift (consumed in Phase 5). Auth with a secondary key logs at INFO so the audit log can later flag "old key still in use after rotation". - New endpoint `GET /api/v1/system/keyring` (authenticated) returns metadata only — kids, roles, created_at, sha256-prefix fingerprint for diagnostic equality checks — never the secret bytes themselves. - `--show-api-key` and `--rotate-api-key` CLI flags work against the keyring; the printed key uses the new `gbx_<kid>_<b64>` wire format the dashboard can paste verbatim. - `GEARBOX_AGENT_KEYRING_PATH` env var (default `<DataDir>/keyring.json`) is now a config field alongside the legacy `HAPROXY_AGENT_API_KEY_PATH`. Dashboard side -------------- - Migration `000002_add_box_agent_keys` adds the `(box_id, kid)`-keyed `box_agent_keys` table and idempotently backfills one `kid='legacy'` row per existing box from `boxes.api_key_encrypted`. The legacy column stays for one release. - `database/box_agent_keys.go` exposes Get/Insert/SetPrimary/Delete/ TouchLastUsed — the storage primitives Phase 2's rotator service composes into the install→use→remove dance. Tests ----- - 19 keyring unit tests covering token parsing (prefixed + legacy + malformed), keyring mutation, file round-trip with and without encryption, legacy api-key migration, and pointer hot-swap. - 8 auth-middleware integration tests covering bearer parsing, kid header echo, secondary-key acceptance, and the live hot-swap path Phase 2 depends on. - 5 storage tests covering primary-key lookup, atomic role flip, delete-refuses-last guard, and last_used_at touch. Carry-overs to Phase 2 (intentional gaps surfaced from this PR) --------------------------------------------------------------- - `DeleteBox` does not yet cascade to `box_agent_keys` (SQLite `PRAGMA foreign_keys` is off in this codebase; enabling it is a broader change). Phase 2's box-delete path will clean dependent rows explicitly. Documented in box_agent_keys_test.go. - The dashboard's `agent.Client` does not yet send `X-Gearbox-Kid` on outbound requests — there's no kid to send while every box's keyring contains only the legacy entry. Phase 2 wires this when the rotator starts mutating keyrings. Refs: research summary and implementation plan posted to #72. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Nine findings from the Copilot review on PR #128, all valid or worth addressing. Fixed in this commit; replies + thread-resolves go with the push. 1. LoadOrCreateKeyRing fall-through (keyring.go:131-167) Was: any error reading the legacy api-key file (incl. ErrKeyRequired from a missing encryption-key env, or a permission error) silently fell through to generating a fresh keyring — would rotate every dashboard out for a transient operator mistake. Now: distinguish "file doesn't exist" (proceed to fresh-gen) from "file exists but errored / malformed" (return the error to the caller). os.Stat + os.IsNotExist gates the choice explicitly. 2. MatchToken constant-time guarantee (keyring.go ~195) Was: the prefixed-token path returned early on the first kid match, making total runtime depend on which kid the request claimed — kid enumeration via timing. The doc said "All comparisons are constant-time" but the prefixed branch broke that promise. Now: walk every entry, compare both kid and secret with subtle.ConstantTimeCompare, AND the two results. Match is recorded without short-circuit; runtime is uniform regardless of which kid (if any) matches. Doc updated to reflect the actual guarantee. 3. writeKeyRingFile mutates input (keyring.go ~415) Was: the function populated SecretHex on each entry of the passed- in keyring before marshaling. KeyRing values are shared via atomic.Pointer and treated as immutable; mutating in-place risks races with concurrent middleware readers. Now: marshal off a local snapshot whose entries have SecretHex backfilled from Secret where needed. Input is never written to. 4. --rotate-api-key zero CreatedAt (main.go ~155) Was: the fresh KeyRingEntry built for the CLI rotate command omitted CreatedAt, so the keyring file got 0001-01-01T00:00:00Z and the /api/v1/system/keyring metadata exposed the same. Now: CreatedAt: time.Now().UTC(). 5. handleGet nil-guard (api/keyring.go ~50) Was: h.keyring.Load() was dereferenced unconditionally; a future wiring bug that left the pointer nil would panic the agent on every keyring request. Now: nil check + 500 + log line. Fails loud rather than crashing. 6. At-most-one-primary-per-box constraint (migration 000002) Was: nothing in the schema stopped two rows with role='primary' for the same box. SetBoxPrimaryKey's transactional flip is correct, but a buggy code path or a manual DB edit could produce the invalid state and GetBoxPrimaryKey would return an arbitrary row. Now: partial unique index on box_agent_keys(box_id) WHERE role='primary'. SQLite supports this directly; index is dropped in the down migration too. 7. Test naming clarity (box_agent_keys_test.go) Was: TestBoxAgentKeys_MigrationBackfillsLegacyEntry was named as if it validated migration behaviour but actually only exercised InsertBoxAgentKey + GetBoxPrimaryKey roundtrip; the comment also misled. Now: split into two clearly-named tests — InsertAndLookup covers the roundtrip, and a new MigrationBackfillStatementWorks test wipes the migrated rows for a single box, re-executes the migration's INSERT-FROM-boxes statement, and asserts the row appears + reruns are idempotent. 8. DeleteBox cascade gap (servers.go DeleteBox) Was: the schema declared ON DELETE CASCADE but PRAGMA foreign_keys is off in this codebase, so deleting a box left orphaned box_agent_keys rows holding encrypted secrets. Phase 1 docs flagged this as a deferred gap; Copilot pushed back, and fairly — it's a small, contained fix. Now: DeleteBox runs inside a transaction that wipes box_agent_keys WHERE box_id = ? before deleting from boxes. Both succeed or neither does. Test re-added: TestBoxAgentKeys_DeleteBoxClearsDependentKeys. 9. APIKeyAuth nil-guard (middleware/auth.go) Was: keyring.Load() was called without first checking the pointer itself for nil. A miswired ServerConfig would panic on every authenticated request. Now: fail-closed nil check at the top of the request handler — returns 401 + logs at error level. Same defensive treatment as fix #5. Tests ----- All 3 dashboard-side suites pass (`database` package, 7 new tests including the new DeleteBox cascade test). All 3 agent-side suites pass (`crypto`, `middleware`, `api`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a note that subtle.ConstantTimeCompare's length-dependent fast-fail is fine here because every kid in the system is exactly 6 chars long (kidLength = 6 hex chars; the legacy entry uses 'legacy' which is also 6 chars by deliberate convention). Custom kids of a different length would naturally hash-mismatch — which is the intended failure mode. Also serves to force a synchronize event so PR #128's CI re-runs on the fix commit; the prior synchronize from fe3c762 didn't trigger workflows (still unclear why; not blocking the work). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two changes required by main moving forward (PRs #127, #134, #137): 1. internal/api/server_test.go was added in PR #127 (remote console) after Phase 1 branched. It uses the old ServerConfig.APIKey field that Phase 1 replaced with KeyRing. Updated the test to construct a one-entry KeyRing and send the legacy 64-hex bearer token. 2. PR #127 also added migration 000002_add_box_console_enabled, colliding with Phase 1's 000002_add_box_agent_keys. Renumbered Phase 1's migration to 000003. Migrations are content-addressed by the embedded iofs, so the rename is mechanical — no schema change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sarg3nt
force-pushed
the
feature/issue-72-phase-1-keyring
branch
from
May 17, 2026 19:03
67dd424 to
9fa9e27
Compare
4 tasks
sarg3nt
added a commit
that referenced
this pull request
May 17, 2026
Six new findings from Copilot's second pass after the initial nine
were resolved.
1. Migration header comment (000003_add_box_agent_keys.up.sql)
Was: said 'New writes go to this table'.
Now: scopes the comment honestly — table creation + one-shot
backfill of EXISTING boxes only. CreateBox / UpdateBox still
write to boxes.api_key_encrypted exclusively in this phase; the
in-app rotator (Phase 2) is the only path that writes to
box_agent_keys after migration. Aligning CreateBox / UpdateBox to
mirror writes here is a planned follow-up so freshly-created
boxes get a keyring entry without a manual rotate.
2. DeleteBox dependent-table cleanup (servers.go DeleteBox)
Was: explicitly wiped box_agent_keys (first-pass fix) but other
tables with FK→boxes ON DELETE CASCADE were still orphaned
because SQLite foreign keys aren't enabled in this codebase.
Now: DeleteBox runs every wipe inside a single transaction.
Tables covered: box_agent_keys, log_source_settings,
box_git_config, config_changes. Comment lists them so adding a
new FK→boxes table forces an update here.
3. TouchBoxAgentKeyLastUsed error wrap (box_agent_keys.go)
Was: returned the raw Exec error.
Now: wraps with 'touch last_used_at:' to match every other
helper in this file.
4. MatchToken DoS via oversized header (crypto/keyring.go)
Was: base64-decoded the secret BEFORE checking encoded length.
A maliciously large Authorization header could force a sizeable
allocation before being rejected.
Now: enforces len(secB64) == roundedSecLen as the very first
thing in the prefixed-token branch — DecodeString never sees
oversized input.
5. handleGet nil-check on h.keyring itself (api/keyring.go)
First-pass guarded against h.keyring.Load() returning nil, but
didn't guard h.keyring itself being nil — calling Load() on a
nil pointer would panic.
Now: checks h.keyring != nil before calling Load(). Same
defensive treatment as APIKeyAuth's middleware-level guard.
6. hydrateSecrets persisted-invariant validation (crypto/keyring.go)
Was: only validated each entry's hex-decoded secret length.
Now: also enforces on load — entry count in [1, MaxKeyRingEntries],
role ∈ {primary, secondary}, at most one primary, KID non-empty
and either the literal 'legacy' or exactly kidLength hex chars,
no duplicate KIDs. Catches a manually-edited or corrupted
keyring.json at the boundary rather than letting it surprise the
auth path. The KID length invariant also makes the constant-
time MatchToken comparisons sound (subtle.ConstantTimeCompare
leaks length-mismatch fast-fail by design; uniform-length kids
eliminate the leak).
Tests
-----
All agent + dashboard test suites pass after the changes. The
stricter hydrateSecrets validation surfaced two-character fake
kids ('v2', 'shared', 'ok', 'overflow') in Phase 2's
api/keyring_test.go that don't conform to the new contract; that
fix lives on phase-2 along with the rest of the test file.
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
First of five stacked PRs implementing API-key rotation per the plan posted to #72. Adds the data structures + storage required for an N-entry keyring on both the agent and the dashboard, with no operator-visible behaviour change yet — rotation endpoints (Phase 2), UI (Phase 3), cleanup scheduler (Phase 4) and drift detection (Phase 5) all build on this.
Agent side
crypto.KeyRingtype with up to 4 accepted keys per agent, atomic tmpfile+rename on disk, AES-256-GCM (GBE1) encryption whenGEARBOX_AGENT_ENCRYPTION_KEYis setgbx_<6-hex-kid>_<base64url(32 random bytes)>; legacy 64-hex bare tokens still accepted for one release cycleLoadOrCreateKeyRing(keyringPath, legacyAPIKeyPath)migrates an existingapi-keyfile to a keyring entry taggedkid="legacy", role=primary; legacy file stays on disk as read-only fallbackKeyRingPointerwrapsatomic.Pointer[KeyRing]so Phase 2's mutation endpoints can swap the live keyring without middleware restartX-Gearbox-Kid:header on every authenticated response (consumed in Phase 5)GET /api/v1/system/keyringreturns metadata only — kids, roles, created_at, sha256-prefix fingerprints — never the secret bytes themselves--show-api-keyand--rotate-api-keyCLI flags work against the keyringDashboard side
000002_add_box_agent_keysadds the(box_id, kid)-keyedbox_agent_keystable and idempotently backfills onekid='legacy'row per existing box fromboxes.api_key_encrypted. Legacy column stays for one release.database/box_agent_keys.goexposes Get/Insert/SetPrimary/Delete/TouchLastUsed storage primitives Phase 2's rotator composes into the install→use→remove danceTest plan
keyring.jsonwith one entry; logs show "API Key: gbx_..."api-keyfile → keyring.json withkid="legacy"Stack
This is Phase 1 of 5. Subsequent phases stack on top:
Each subsequent PR's base branch is the previous phase's branch; they merge in order. Auto-rotation on a schedule (originally Phase 4 in the plan) is deferred — it needs a new global-settings surface that doesn't exist yet in the dashboard.
Closes part of #72.
🤖 Generated with Claude Code