feat: identify this build to Bugzilla with a User-Agent - #58
Merged
Conversation
reqwest sends no User-Agent unless one is configured, so every request bugwarden made to Bugzilla arrived anonymous: accept and host and nothing more. Run as fleet infrastructure with a server-held key (#27) and no per-caller identity yet (#32), one Bugzilla account carries the whole fleet's traffic, and the operator on the other end had no name to allowlist or complain about — nothing distinguished it from a person with a browser. The identity is threaded in as a parameter rather than built where the client lives. Built in bugwarden-core, env!("CARGO_PKG_NAME") expands to bugwarden-core and would name the library in the access log of every binary embedding it: the shape of #53 one layer down, and just as plausible-looking. The binary crate supplies server::USER_AGENT, {name}/{version} (+{repository}) from its own manifest, and main builds its client through server::bugzilla_client — the single production construction path, and unlike a constructor call in main, one a test can reach. A test drives the shipped binary itself, because every assertion one call frame in still passes for a main that builds its own client. BugzillaClient::new refuses a blank or non-header-value user_agent at construction, so an embedder cannot get an anonymous client back from a value that silently resolved to nothing. The header is public — it lands in every configured Bugzilla's access log — so it carries name, version and the project repository and nothing else: no key material, no policy path, no host. That is the discipline of I12 on a surface I12 does not itself name; no invariant text changes. Including the version is a deliberate choice recorded in DESIGN.md: unlike #53's, this disclosure is optional, and the party learning it is the Bugzilla this deployment already authenticates to with an API key and sends every query. BREAKING CHANGE: BugzillaClient::new takes a third argument, so the next bugwarden-core release cannot be a patch. Closes #55
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.
What changed
BugzillaClient::newtakes auser_agentand sets it once on the sharedreqwest::Client, so every request carries it — the authenticated REST calls,the POST/PUT bodies, and the unauthenticated
page.cgifetch alike.bugwarden-corenever picks a value of its own: the binary crate suppliesserver::USER_AGENT,{name}/{version} (+{repository})from its own manifest,and
mainconstructs through the newserver::bugzilla_client(&cli).Why
Every request to Bugzilla was anonymous —
acceptandhostand nothing more.With a server-held key (#27) and no per-caller identity yet (#32), one Bugzilla
account carries a whole fleet's traffic, and the operator on the other end had
no name to allowlist or complain about.
Setting the header where the client lives is the trap:
env!("CARGO_PKG_NAME")inbugwarden-coreexpands tobugwarden-coreandwould name the library in the access log of every binary embedding it — the
shape of #53 one layer down, and it would look entirely plausible. Hence a
parameter, and hence
bugzilla_clientliving inserver.rswhere a test canreach it.
Issue #55 left "whether to include a contact URL" open; this resolves it as
yes, read from
CARGO_PKG_REPOSITORY, so the name leads somewhere withoutbecoming a second copy to keep in step.
Invariants touched
No guard behaviour moves: the policy engine, the tool gates and the audit
schema are untouched, and no CLI flag or policy key is added, so
examples/policy.tomlandexamples/audit.tomlneed no change.I12 is cited as a discipline, not extended. Its text — the API key must
never appear in logs, error messages, or tool results — does not name outbound
request metadata, and this PR does not widen it. The header is held to the same
standard: it carries name, version and repository only, and cannot carry
runtime data, being a
concat!of compile-time literals.Version disclosure is accepted deliberately and now recorded in DESIGN.md.
Unlike #53's, this disclosure is not forced —
User-Agentis optional, andnothing was disclosed before — so it is a choice. It is accepted because the
party learning it already authenticates this deployment by API key and sees
every query, and because a version is what lets an operator tell whether a
reported misbehaviour is fixed in what is deployed.
How it was verified
cargo fmt --check, both clippy invocations,cargo test --workspace --all-targets --locked(357 tests),cargo deny checkandtyposare clean.tests/binary_user_agent.rsspawns the shipped executable against a realHTTP server, drives an MCP session over stdio, and reads the header off the
request that arrives. This exists because every assertion one call frame in
still passes for a
mainthat builds its own client — verified by mutation,see below. The same run pins that
--use-auth-headerreaches the constructor.crates/bugwarden-core/tests/user_agent_wiremock.rs: the caller's valuereaches all four request shapes in both auth modes, using an agent naming
neither crate.
crates/bugwarden/src/server.rs: a request throughbugzilla_clientcarriesthe production value, matching the identity the MCP handshake advertises.
crates/bugwarden-core/src/client.rs: blank and non-header-value identitiesfail at construction.
local HTTP server and sent
bugwarden/0.3.0 (+https://github.com/plusky/bugwarden).Adversarial review
Three reviewers (completeness, test-strength with mutation, docs and process).
Findings addressed in this branch:
main.rsbypassing
bugzilla_clientand shippingcurl/7.68.0; the identity droppedwhenever
--use-auth-headeris set;bugzilla_clientignoring that flag andputting the key back in the URL; a per-request header on the PUT path only
(reqwest replaces the client default rather than duplicating it, so this
class of defect is silent); and an
assert_eq!comparing the header to thesame manifest fields the code reads, which agreed with any value they took.
outbound headers), in three places — narrowed to "the discipline of I12 on a
surface I12 does not name".
discovertest; corrected.actually lives", a blank-identity clause reading as if this binary could hit
it, a test-constant comment claiming a proof that lives in another file, and
an assertion that no key material reaches a compile-time constant — which
cannot fail, and which DESIGN.md then recorded as proven. Removed.
Rebutted: validating
base_urlat construction so a transposednew(agent, bool, url)fails at startup rather than at request time. A realhazard, but a separate concern from #55; the one production call site is
covered, since a transposition there fails the new tests. Worth a follow-up
issue if wanted.
Note for the reviewer
The DESIGN.md "Identity tests" bullet also covers #53's tests, which landed in
f944402without a Testing entry. Documented here because the two directionsof one identity read as a single bullet; say the word and it splits into its
own
docs(design):commit on this branch.