feat(server): server-held Bugzilla API key via --api-key-file - #39
Merged
Conversation
Over http the Bugzilla key so far came only from a per-request header, so every client holds the real key — and a client holding the key can bypass the guard by talking to Bugzilla directly. Fleet deployments need the opposite custody: the key belongs to the server (container secret, systemd LoadCredential), and clients present nothing. --api-key-file <path> reads the key at startup: content trimmed, an empty or unreadable file is a startup error naming the path only (I12), and a group/other-accessible file draws a warning recommending 0600. Custody is resolved exactly once in Cli::resolve_key_custody, called from BugWarden::new so every construction path fails at startup rather than at first request: - stdio: key from --api-key or now --api-key-file; the without-key bail moved from main.rs into resolution. - http + --api-key-file: server-held mode — every request is served with the server's key and the per-request header is never consulted (a request carrying one is served, its value never read). - http + --api-key alone: warn-and-ignore stays per-request — no silent upgrade, BUGZILLA_API_KEY is a generic name other Bugzilla tooling sets and flipping it would change deployed custody. - both flags together: mutually exclusive, startup error. There is no fallback between the two custodies in either direction. An empty BUGZILLA_API_KEY_FILE= counts as absent, like its --api-key twin, and custody resolves before the audit sink opens so a key misconfiguration cannot leave a fresh audit file behind. Because the server now whoamis with its own key on behalf of every client, a created_by_me policy under server-held custody collapses onto the service account's identity — startup warns when the two meet, and DESIGN.md/README document the collapse. Cli's Debug redacts api_key (I12). The new http_transport_wiremock.rs suite drives the server over real streamable HTTP (rmcp client + reqwest) and proves through api_key query-param matchers which key served each request: no-credential clients are served in server-held mode, a client-sent header is never read (expect(0) on its value upstream), per-request mode still authenticates with the header, a missing header stays a protocol error costing zero upstream requests, the key file is read once (mid-flight rotation changes nothing), and the uniform denial text holds over http (I2). Startup log lines (mode and source, never key material) are pinned by a tracing capture helper.
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.
Closes #27.
Over http the Bugzilla key so far came only from a per-request header, so every client holds the real key — and a client holding the key can talk to Bugzilla directly, past the guard. This adds the opposite custody for fleet deployments: the key belongs to the server, clients present nothing.
What changed
--api-key-file <PATH>/BUGZILLA_API_KEY_FILEreads the key at startup: content trimmed, empty or unreadable file is a startup error naming the path only (I12), group/other-accessible file draws a chmod-600 warning.Cli::resolve_key_custody()called fromBugWarden::new, so every misconfiguration fails at startup, never at first request — and before the audit sink opens, so a bad key config cannot leave a fresh audit file behind. The startup log states the mode and source, never key material.--api-keyor now--api-key-file;--api-key-file: server-held mode — every request is served with the server's key; the per-request header is never consulted (a request carrying one is served, its value never read);--api-keyalone: warn-and-ignore stays per-request — no silent upgrade, sinceBUGZILLA_API_KEYis a generic name other Bugzilla tooling sets and flipping it would change deployed custody;http_transport_wiremock.rs): an rmcp/reqwest streamable-HTTP client drives the server end to end, andapi_keyquery-param matchers prove which key served each request.LoadCredentialdeployment sketch); DESIGN.md gains the Key custody section, the resolution table, and the extended testing bar.Adversarial review record (pre-PR gate)
Three hostile lenses (security-bypass, correctness/fail-closed, docs-vs-code) plus mutation verification ran against the implementation commit; 10 findings, all addressed, none rebutted:
created_by_meidentity collapse: under server-held custody the server whoamis with its own key for every client, so an identity-relative policy silently describes the service account instead of the caller. Now: startup warning when server-held http custody meets a policy that consults identity, documented in DESIGN.md (Key custody + Identity resolution) and README, pinned by a test.BUGZILLA_API_KEY_FILEno longer breaks pre-existing test suites (builders null it); set-but-emptyBUGZILLA_API_KEY_FILE=counts as absent instead of a raw clap usage error;Cli'sDebugnow redactsapi_key(I12); the mutual-exclusion test pins each flag+env pair unambiguously; the key-rotation test rewrites the file for real (no stale-cursor NUL padding) so itsexpect(0)argument is true; custody resolves before the audit sink opens; the startup log lines are pinned by a tracing capture helper (doubles as an I12 pin — mode and path, never the key); DESIGN.md'sBugWardenstruct listing gained the missingauditfield.Mutation verification: 9/9 killed, no survivors. Each mutation applied singly in a detached worktree, killed by the named test, reverted:
server_held_never_reads_the_client_headerserver_held_serves_clients_with_no_credentialboth_key_sources_are_a_startup_error_naming_both_flagsempty_or_whitespace_key_file_is_an_error_naming_the_pathkey_file_content_is_trimmed+ http suiteserver_held_key_is_resolved_once_at_startup--api-keysilently upgrades to server-heldhttp_with_startup_key_alone_stays_per_requeststdio_without_any_key_source_fails_at_resolutionserver_held_never_reads_the_client_headerFull AGENTS.md gate re-run independently on the squashed commit: fmt, clippy
-D warnings,cargo test --workspace --all-targets --locked(305 tests),cargo deny check, typos — all green. Dev-deps only in the lockfile delta (rmcp http-client features + reqwest 0.13, rustls-only).As the issue notes, this blocks #32: once clients present no credential, per-caller endpoint authentication becomes necessary rather than optional.