refactor(#89): drop HAPROXY_AGENT_CONSOLE_ENABLED — per-box toggle is the sole gate - #137
Merged
Merged
Conversation
… the sole gate The two-layer "agent env var AND dashboard per-box flag" gate was friction without proportionate security benefit for the single-operator homelab case this is built for. The agent's API key already grants full administrative control of the box (logs, systemd, restarts, package management); the marginal exposure of also exposing the console endpoints by default is small. Simplifies the enable path to one click: flip the "Remote console" checkbox on the box edit page. No agent-side restart, no env-var plumbing, no per-host configuration management work. Agent changes: - internal/framework/config/config.go: drop ConsoleEnabled field + HAPROXY_AGENT_CONSOLE_ENABLED env-var read - internal/api/server.go: drop ConsoleEnabled from ServerConfig; always construct the console handler and mount its routes - cmd/gearbox-agent/main.go: drop the conditional startup log; replace with a single Info line noting the surface is mounted and that the per-box opt-in is dashboard-side - internal/api/server_test.go: replace TestNewServer_ConsoleDisabled_RoutesReturn404 (premise gone) with TestNewServer_ConsoleRoutesAlwaysMounted, which pins that all three console routes exist and that each is behind its appropriate auth (API key for token + capabilities, single-use token for WS — verified by 401 on token-less call) - internal/api/console/capabilities.go: drop stale env-var reference in the Enabled-field comment Dashboard changes: - internal/framework/templates/pages/haproxy_settings.templ: trim the "Requires HAPROXY_AGENT_CONSOLE_ENABLED=true on the agent" clause from the toggle's help text; keep the box_console:connect mention with "non-admin users" framing (admins get the permission for free via the IsAdmin shortcut) - internal/framework/database/servers.go: rewrite ConsoleEnabled field doc to drop the now-misleading "belt and suspenders" framing - internal/framework/handler/api_console.go: rewrite comment on the per-box check for the same reason Docs: - docs/console-setup.md: replace the per-mode "set env var, restart" instruction with a single "Enable for a box" section pointing at the dashboard toggle. Adds an IMPORTANT note that the API key alone is sufficient to use the console — matching the existing trust model, surfaced explicitly. Removes HAPROXY_AGENT_CONSOLE_ENABLED from the Mode A/B.1/B.2 examples. Troubleshooting table updates: "/api/v1/console/* returns 404" now means the agent build predates this feature, not a flipped flag. Adds a new row for "capabilities returns 404 from the dashboard" → "flip the per-box toggle." - docs/security-review/console-threat-model.md: rewrite the "Stolen agent API key" section to be honest about the new posture — API key alone is enough to open a session directly against the agent; the dashboard's per-box toggle only gates the dashboard path. Adds a residual-risk entry making this explicit. Drops two stale residuals (InsecureSkipVerify on the WS dialer + fixed idle timeout — both resolved in #127). Tests: full agent suite + dashboard handler/database/agent suites race-clean; markdownlint clean on both docs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the agent-side HAPROXY_AGENT_CONSOLE_ENABLED env-var gate from the remote console feature, leaving the dashboard's per-box Remote console toggle as the sole opt-in. Console endpoints are now unconditionally mounted on the agent, and documentation/comments/tests are updated to reflect the simpler (and explicitly more permissive in the stolen-API-key threat model) posture.
Changes:
- Agent: drop
ConsoleEnabledfromconfig.Configandapi.ServerConfig; always mount/api/v1/console/*routes; single startup log line. - Dashboard: rewrite stale comments and toggle help text to reflect that the per-box flag is the sole gate (no schema/handler logic change).
- Docs: rewrite
console-setup.mdfor the new single-step enable flow; honestly update the threat model's stolen-API-key section and drop resolved residual risks.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| gearbox-agent/internal/framework/config/config.go | Removes ConsoleEnabled field and env-var read. |
| gearbox-agent/internal/api/server.go | Removes config gate; unconditionally constructs handler and mounts the three routes. |
| gearbox-agent/internal/api/server_test.go | Replaces "disabled returns 404" test with one pinning that all three routes are mounted and auth-gated. |
| gearbox-agent/internal/api/console/capabilities.go | Updates Enabled field doc to drop env-var reference. |
| gearbox-agent/cmd/gearbox-agent/main.go | Replaces enabled/disabled branch with a single "endpoints mounted" log line; drops ConsoleEnabled from ServerConfig. |
| gearbox/internal/framework/templates/pages/haproxy_settings.templ | Updates toggle help text to remove env-var requirement. |
| gearbox/internal/framework/handler/api_console.go | Updates per-box opt-in comment to drop belt-and-suspenders framing. |
| gearbox/internal/framework/database/servers.go | Updates ConsoleEnabled field doc to drop env-var reference. |
| docs/console-setup.md | Replaces per-mode env-var enable instructions with a single dashboard toggle section; adds [!IMPORTANT] callout about API-key direct access; updates troubleshooting. |
| docs/security-review/console-threat-model.md | Rewrites stolen-API-key section to reflect new posture; adds residual-risk entry; drops two resolved residuals. |
sarg3nt
added a commit
that referenced
this pull request
May 17, 2026
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
added a commit
that referenced
this pull request
May 17, 2026
* feat(rotation): Phase 1 multi-key keyring plumbing (#72) 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> * chore(rotation): address Copilot review on PR #128 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> * docs(rotation): clarify constant-time precondition on MatchToken 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> * fix(rotation): adapt phase-1 to main after rebase 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> --------- 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
Drops the
HAPROXY_AGENT_CONSOLE_ENABLEDenv-var gate from the agent. The dashboard's per-boxRemote consoletoggle is the sole switch — one click to enable a box, one click to revoke.Why
The two-layer "agent env var AND dashboard per-box flag" design from #127 was friction without proportionate security benefit. The agent's API key already grants full administrative control of the box (logs, systemd, restarts, package management); the marginal exposure of always-mounting the console endpoints is small in the homelab single-operator threat model this is built for.
Operator UX before: edit
/etc/default/gearbox-agent, restart the agent, then flip the dashboard toggle. After: just flip the dashboard toggle.What changes
Agent (
gearbox-agent):internal/framework/config/config.go— dropConsoleEnabledfield + env-var readinternal/api/server.go— dropConsoleEnabledfromServerConfig; always construct the console handler and mount its three routescmd/gearbox-agent/main.go— single startup log line: "Console: endpoints mounted at /api/v1/console/* (per-box opt-in is dashboard-side)"internal/api/server_test.go— replacesTestNewServer_ConsoleDisabled_RoutesReturn404(premise gone) withTestNewServer_ConsoleRoutesAlwaysMounted, which pins that all three routes exist and that each is behind its appropriate auth (API key on token + capabilities; single-use token on WS, verified by 401 on token-less call)internal/api/console/capabilities.go— stale env-var reference in field doc removedDashboard (
gearbox):internal/framework/templates/pages/haproxy_settings.templ— toggle help text drops the env-var clause; keeps thebox_console:connectmention with "non-admin users also need" framing (admins get the permission for free viaIsAdminshortcut)internal/framework/database/servers.go— rewriteConsoleEnabledfield doc; drop the "belt-and-suspenders" framinginternal/framework/handler/api_console.go— comment on the per-box check updated for the same reasonDocs:
docs/console-setup.md— replaces per-mode "set env var, restart" instructions with a single Enable for a box section pointing at the dashboard toggle. Adds an[!IMPORTANT]callout that the API key alone suffices to use the console (matches the existing trust model — surfaced explicitly so operators aren't surprised). Removes the env var from Mode A / B.1 / B.2 examples. Troubleshooting table: "/api/v1/console/* returns 404" now means the agent build predates this feature, not a flipped flag; new row for "capabilities returns 404 from the dashboard" → "flip the per-box toggle."docs/security-review/console-threat-model.md— rewrites the Stolen agent API key section to be honest about the new posture: API key alone is enough to open a session directly against the agent; the dashboard's per-box toggle only gates the dashboard path. Adds a residual-risk entry that calls this out explicitly. Drops two stale residuals (InsecureSkipVerifyon WS dialer + fixed idle timeout — both resolved in feat: remote console (#89) — phases 1a→3 + per-box toggle + WS TLS pinning #127).Test plan
cd gearbox-agent && go test -race -count=1 ./internal/api/...— greencd gearbox-agent && go build ./...— greencd gearbox && go test -race -count=1 ./internal/framework/handler/... ./internal/framework/database/... ./internal/framework/agent/...— greencd gearbox && go build ./...— greennpx markdownlint-cli docs/console-setup.md docs/security-review/console-threat-model.md --config .markdownlint.json— cleanlight-hugger: deploy this build, confirmjournalctl -u gearbox-agent | grep -i consoleshows the new "endpoints mounted" line; toggle on per-box from the dashboard, click>_on the Bx tile, get a shellPart of #89.
🤖 Generated with Claude Code