diff --git a/README.md b/README.md index ae02b0d..e19f365 100644 --- a/README.md +++ b/README.md @@ -390,6 +390,20 @@ instances that reject the `api_key` query parameter and require this affects only server-to-Bugzilla authentication, not the client-facing header. +Choose the placement the deployment in front of you honours: the query +parameter is what stock Bugzilla accepts, and what BMO still accepts +but documents as deprecated — BMO's supported forms are its own +`X-Bugzilla-API-Key` header and OAuth2 bearer tokens. The Bearer token +is the Red Hat form, and no other header form is sent. If the network +path between bugwarden and Bugzilla strips the chosen form — a proxy +dropping headers, a gateway rewriting query strings — every request +arrives anonymous: reads quietly narrow to what an anonymous caller may see, +and writes fail with "You must log in before using this part of +Bugzilla" (code 410). The key itself is fine in that state; switch the +placement to the form that survives the path. When the guard policy +consults identity, the startup preflight names this cause before the +server starts serving. + #### Server-held key mode (fleet deployments) With `--api-key-file` the Bugzilla API key belongs to the *server*: every diff --git a/crates/bugwarden/src/server.rs b/crates/bugwarden/src/server.rs index 9c9539b..49fd16a 100644 --- a/crates/bugwarden/src/server.rs +++ b/crates/bugwarden/src/server.rs @@ -468,6 +468,21 @@ fn upstream_hint(tool: &str, code: i64, reach: UpstreamReach) -> Option<&'static .map(|row| row.text) } +/// Bugzilla's `login_required` code: the refusal an anonymous request +/// meets. Keyed on the envelope code, not the HTTP status — Bugzilla +/// answers this one as HTTP 401. +const LOGIN_REQUIRED_CODE: i64 = 410; + +/// Whether `err` is Bugzilla's `login_required` refusal: the request +/// reached Bugzilla anonymously, whatever the operator configured — +/// the API key did not arrive in a form the deployment honours, or +/// did not arrive at all. +fn login_required(err: &anyhow::Error) -> bool { + err.downcast_ref::() + .and_then(|bz| bz.code()) + == Some(LOGIN_REQUIRED_CODE) +} + /// A write tool's failure text for an upstream refusal: its fixed first /// line — the audit gate's refusal, so an unhinted failure and the gate /// refusal are byte-identical — plus the hint `code` selects, if any. @@ -3522,6 +3537,17 @@ impl BugWarden { /// closed, distinct from a transport failure so the operator does not /// mistake a wrong login for a broken key); a transport/parse error /// bails naming the endpoint, the same as the `whoami` arm. + /// + /// A refusal carrying Bugzilla code 410 (`login_required`) is + /// diagnosed separately in both server-custody arms: the request + /// reached Bugzilla anonymously, so the bail names the API key + /// placement — the `api_key` query parameter by default, an + /// `Authorization: Bearer` header under `--use-auth-header` — and + /// the transit that can strip the chosen form, rather than the + /// endpoint-existence or wrong-login explanations a 410 is neither + /// of. A `valid_login` `false` can mean the same thing, so that + /// bail names the possibility too. + /// /// [`BugWarden::new`] already refuses to construct a declared-identity /// server under per-request custody (A5), so that combination cannot /// reach this method — the match below still names it explicitly @@ -3539,16 +3565,29 @@ impl BugWarden { tracing::info!(login, "identity endpoint verified"); Ok(()) } - Err(err) => Err(err).context( - "GET /rest/whoami failed during startup preflight; the guard policy \ - consults created_by_me, which needs this endpoint to resolve the \ - caller's identity. Stock Bugzilla Core v1 does not define /rest/whoami \ - (it is a fork/BMO extension) — if this deployment does not have it, \ - starting anyway would deny every access classification the policy's \ - identity rules reach (I4). Confirm the endpoint exists, set \ - global.identity_source = \"declared\" instead, or remove the \ - created_by_me criteria from the policy", - ), + Err(err) => { + let context = if login_required(&err) { + "GET /rest/whoami reached Bugzilla anonymously during startup \ + preflight: Bugzilla refused it with login_required (code 410), \ + so the configured API key did not authenticate the request. \ + The key travels as the api_key query parameter by default, or \ + as an Authorization: Bearer header with --use-auth-header; a \ + deployment honouring only the other form, or a proxy stripping \ + the chosen one in transit, produces exactly this refusal while \ + the key itself stays valid. Match the placement to the \ + deployment and the network path, then restart" + } else { + "GET /rest/whoami failed during startup preflight; the guard policy \ + consults created_by_me, which needs this endpoint to resolve the \ + caller's identity. Stock Bugzilla Core v1 does not define /rest/whoami \ + (it is a fork/BMO extension) — if this deployment does not have it, \ + starting anyway would deny every access classification the policy's \ + identity rules reach (I4). Confirm the endpoint exists, set \ + global.identity_source = \"declared\" instead, or remove the \ + created_by_me criteria from the policy" + }; + Err(err).context(context) + } }, KeyCustody::PerRequest => { tracing::warn!( @@ -3595,13 +3634,38 @@ impl BugWarden { "GET /rest/valid_login says the configured API key does not \ authenticate as \"{login}\" (global.identity_login); Bugzilla \ compares logins case-sensitively, so check for a case mismatch as \ - well as a wrong account. Fail closed: starting anyway would deny \ + well as a wrong account. A request that reaches Bugzilla \ + anonymously answers false here too: the key travels as the api_key \ + query parameter by default, or as an Authorization: Bearer header \ + with --use-auth-header, and a deployment honouring only the other \ + form, or a proxy stripping the chosen one in transit, makes a \ + valid key read exactly like a wrong account — check the \ + placement as well. Fail closed: starting anyway would deny \ every access classification the policy's identity rules reach (I4)", ), - Err(err) => Err(err).context(format!( - "GET /rest/valid_login failed during startup preflight while \ - verifying the declared login \"{login}\" (global.identity_login)", - )), + Err(err) => { + let context = if login_required(&err) { + format!( + "GET /rest/valid_login reached Bugzilla anonymously during \ + startup preflight while verifying the declared login \ + \"{login}\" (global.identity_login): Bugzilla refused it \ + with login_required (code 410), so the configured API key \ + did not authenticate the request. The key travels as the \ + api_key query parameter by default, or as an Authorization: \ + Bearer header with --use-auth-header; a deployment \ + honouring only the other form, or a proxy stripping the \ + chosen one in transit, produces exactly this refusal while \ + the key itself stays valid. Match the placement to the \ + deployment and the network path, then restart" + ) + } else { + format!( + "GET /rest/valid_login failed during startup preflight while \ + verifying the declared login \"{login}\" (global.identity_login)" + ) + }; + Err(err).context(context) + } } } } diff --git a/crates/bugwarden/tests/preflight_wiremock.rs b/crates/bugwarden/tests/preflight_wiremock.rs index faed529..bbcd2f5 100644 --- a/crates/bugwarden/tests/preflight_wiremock.rs +++ b/crates/bugwarden/tests/preflight_wiremock.rs @@ -15,7 +15,12 @@ //! identity criterion (the laziness contract must hold at startup too); //! - `preflight` bailing under `KeyCustody::PerRequest` instead of //! warning-and-continuing (there is no server-held key to probe with); -//! - an API key leaking into a preflight error's text (I12). +//! - an API key leaking into a preflight error's text (I12); +//! - a `login_required` (code 410) refusal diagnosed with the generic +//! endpoint-existence or endpoint-failure context instead of the +//! anonymous-landing one naming the API key placement; +//! - the `valid_login` `false` bail dropping its anonymous-landing +//! sentence (a stripped placement reads exactly like a wrong login). use std::sync::Arc; @@ -271,6 +276,10 @@ async fn preflight_declared_login_transport_error_names_the_endpoint() { msg.contains("GET /rest/valid_login"), "preflight error must name the failing endpoint: {msg}" ); + assert!( + !msg.contains("anonymously"), + "a transport failure must not be diagnosed as an anonymous landing: {msg}" + ); } #[tokio::test] @@ -310,3 +319,107 @@ async fn preflight_transport_error_does_not_leak_the_api_key_i12() { "API key leaked into a preflight error: {msg}" ); } + +#[tokio::test] +async fn preflight_whoami_login_required_names_the_anonymous_landing() { + // The key never arrived in a form Bugzilla honours: whoami answers + // the login_required refusal (HTTP 401, envelope code 410). The bail + // must name the anonymous landing and the placement — this deployment + // HAS the endpoint, so the endpoint-existence context a 404 earns + // would send the operator debugging the wrong thing. + let mock = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/rest/whoami")) + .respond_with(ResponseTemplate::new(401).set_body_json(serde_json::json!({ + "error": true, + "code": 410, + "message": "You must log in before using this part of Bugzilla.", + }))) + .mount(&mock) + .await; + let server = server_custody_server(IDENTITY_POLICY, &mock); + + let err = server + .preflight() + .await + .expect_err("a login_required whoami refusal must fail preflight"); + let msg = format!("{err:#}"); + assert!( + msg.contains("anonymously"), + "preflight error must name the anonymous landing: {msg}" + ); + assert!( + msg.contains("--use-auth-header"), + "preflight error must name the placement that decides it: {msg}" + ); + assert!( + !msg.contains("does not define /rest/whoami"), + "a 410 is not an endpoint-existence failure: {msg}" + ); +} + +#[tokio::test] +async fn preflight_valid_login_login_required_names_the_anonymous_landing() { + // The same refusal on the declared arm: valid_login answers + // login_required instead of a result. The bail names the endpoint + // and the anonymous landing, not a generic endpoint failure. + let mock = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/rest/valid_login")) + .respond_with(ResponseTemplate::new(401).set_body_json(serde_json::json!({ + "error": true, + "code": 410, + "message": "You must log in before using this part of Bugzilla.", + }))) + .mount(&mock) + .await; + let server = server_custody_server(DECLARED_IDENTITY_POLICY, &mock); + + let err = server + .preflight() + .await + .expect_err("a login_required valid_login refusal must fail preflight"); + let msg = format!("{err:#}"); + assert!( + msg.contains("GET /rest/valid_login"), + "preflight error must name the failing endpoint: {msg}" + ); + assert!( + msg.contains("anonymously"), + "preflight error must name the anonymous landing: {msg}" + ); + assert!( + msg.contains("--use-auth-header"), + "preflight error must name the placement that decides it: {msg}" + ); +} + +#[tokio::test] +async fn preflight_declared_login_false_names_the_anonymous_possibility() { + // valid_login answering false is also what an anonymous request + // gets, so alongside the login the bail must name the placement + // that decides whether the key arrived at all — otherwise a + // stripped header or query string is indistinguishable from a + // wrong account and sends the operator after the wrong fix. + let mock = MockServer::start().await; + mount_valid_login(&mock, "svc@example.com", false, 1).await; + let server = server_custody_server(DECLARED_IDENTITY_POLICY, &mock); + + let err = server + .preflight() + .await + .expect_err("a login the key does not own must fail preflight"); + let msg = format!("{err:#}"); + assert!( + msg.contains("svc@example.com"), + "preflight error must name the declared login: {msg}" + ); + assert!( + msg.contains("anonymously"), + "preflight error must name the anonymous-landing possibility: {msg}" + ); + assert!( + msg.contains("--use-auth-header"), + "preflight error must name the placement that decides it: {msg}" + ); +} diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 642de8b..77ebf45 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -812,6 +812,26 @@ Endpoint mapping: Auth per request: `use_auth_header` ? header `Authorization: Bearer {key}` : query param `api_key={key}`. Always `Accept: application/json`. + +The two placements are not interchangeable dialects of one form: stock +Bugzilla honours the query parameter (under both the `api_key` and +`Bugzilla_api_key` names). BMO still accepts it, but documents it as +deprecated — a stopgap for old callers; the forms it supports are its +own `X-Bugzilla-API-Key` header and OAuth2 bearer tokens, so an API +key carried as a Bearer token lands anonymous there too. Red Hat +Bugzilla requires the Bearer header. Stock Bugzilla 5.0/5.2 has no +header form at all; BMO's own header form (`X-Bugzilla-API-Key`) is +not sent. Whichever form the path between the server and Bugzilla +strips — a proxy dropping headers, a gateway rewriting query strings — +every request lands anonymous, and Bugzilla's only signal is +`login_required` (envelope code 410, answered as HTTP 401): reads +silently narrow to the anonymous view and writes refuse. The client +cannot detect the strip, so the diagnosis lives where an operator +first meets a failure: the identity preflight names the anonymous +landing and the placement whenever a probe meets code 410, and the +`valid_login`-false bail names it as a possibility (see "Identity +preflight"). + Errors: a non-2xx status, a body `{"error": true}` under any status, or a 2xx body that is not JSON => a `BugzillaError` inside anyhow. Its Display is `bugzilla error (HTTP {status}): {message}` — the text the untyped error @@ -1323,6 +1343,17 @@ The failure path attaches the sanitized client error (I12: `whoami` / `valid_login` already strip the URL) as the `anyhow` error's source/context — no key material reaches the bailed-out message either. +A probe refusal carrying envelope code 410 (`login_required`) is worded +separately in both server-custody arms: the request reached Bugzilla +anonymously, so the bail names the API key placement — the `api_key` +query parameter by default, an `Authorization: Bearer` header under +`--use-auth-header` — and the transit that can strip the chosen form, +instead of the endpoint-existence (`whoami`) or wrong-login +(`declared`) explanations a 410 is neither of. The `declared` arm's +`false` bail names the same possibility: an anonymous `valid_login` +answers `false`, so a placement the path strips reads exactly like a +wrong account. + Tool descriptions: concise and action-oriented; state the defaults and constraints the model must know. @@ -4297,7 +4328,11 @@ wired, `server.rs` and `main.rs` are the reference. the `declared` arm — a correct declared login passes preflight via one `valid_login` request and zero whoami requests, a login the key does NOT authenticate as fails preflight naming that login, and a transport-level - `valid_login` failure fails preflight naming the endpoint. + `valid_login` failure fails preflight naming the endpoint; a + `login_required` (code 410) refusal on either probe fails preflight + naming the anonymous landing and the API key placement (for `whoami`, + without the endpoint-existence wording), and the `valid_login`-false + bail names the anonymous-landing possibility alongside the login. - Integration tests (crates/bugwarden/tests/http_transport_wiremock.rs, wiremock + rmcp client over REAL streamable HTTP — the only harness in which the per-request key header physically exists; the wiremock upstream