From f585dc64106056fc1f0e6dd51aeb85e57bfb3081 Mon Sep 17 00:00:00 2001 From: "dobby-yivi-agent[bot]" <275734547+dobby-yivi-agent[bot]@users.noreply.github.com> Date: Thu, 2 Jul 2026 08:02:47 +0000 Subject: [PATCH] fix: require validated API key on GET /usage (GHSA-5rhx-xgvv-h78h) The `/usage` route used the `ApiKey` guard, whose `FromRequest` always succeeds (yielding the anonymous default tier when no key is present). The handler's default-tier branch then looked up usage by the caller-supplied `email` query parameter with no ownership check, so any unauthenticated caller could probe usage for an arbitrary address. Introduce a `ValidatedApiKey` request guard that fails the request (401 on NoCredentials/Rejected, 503 on PkgUnreachable) so the "authenticated" intent is enforced by the type system. Switch `/usage` to that guard and key the lookup on the validated tenant; the `email` query param is now optional and only echoed back. Also make the `cryptify_token` comparisons constant-time via `subtle::ConstantTimeEq` (new `cryptify_tokens_match` helper), mirroring `recovery_tokens_match`, in `check_cryptify_token`, `upload_chunk`, and `classify_chunk_request`. Adds a regression test asserting unauthenticated `/usage` returns 401, and updates the OpenAPI description. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 5 ++ api-description.yaml | 37 +++++++++--- src/main.rs | 136 ++++++++++++++++++++++++++++++++++++------- 3 files changed, 149 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8850b7..5fa630c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- require a validated API key on `GET /usage` and reject unauthenticated callers with 401 (GHSA-5rhx-xgvv-h78h) +- compare `cryptify_token` values in constant time, matching the recovery-token path + ## [0.1.27](https://github.com/encryption4all/cryptify/compare/v0.1.26...v0.1.27) - 2026-05-16 ### Added diff --git a/api-description.yaml b/api-description.yaml index e17185c..ab99133 100644 --- a/api-description.yaml +++ b/api-description.yaml @@ -355,23 +355,37 @@ paths: get: tags: - "Usage" - summary: "Get rolling upload usage for a sender email" + summary: "Get rolling upload usage for the authenticated tenant" description: - "Returns the bytes uploaded by the given email address in the last 14 days, - together with the applicable limits. Frontends use this to warn the user - before they hit the limit." + "Returns the bytes uploaded by the authenticated API-key tenant in the + last 14 days, together with the applicable limits. **Requires a valid + `Authorization: Bearer PG-…` API key**: usage is accounted per validated + tenant and is never looked up by a caller-supplied email, so an + unauthenticated caller cannot query an arbitrary address. Requests + without a valid key are rejected with 401." operationId: "getUsage" parameters: + - in: "header" + name: "Authorization" + description: + "`Bearer PG-…` API key validated against pg-pkg. Required; a missing, + malformed, or rejected key returns 401." + schema: + type: "string" + required: true - in: "query" name: "email" - description: "The sender email address to query." - required: true + description: + "Optional. Echoed back in the response `email` field for the + frontend's convenience. It does NOT influence the usage lookup, which + is keyed to the validated tenant." + required: false schema: type: "string" format: "email" responses: "200": - description: "Usage information for the given email." + description: "Usage information for the authenticated tenant." content: application/json: schema: @@ -393,14 +407,14 @@ paths: limit_bytes: type: "integer" format: "int64" - description: "Rolling-window upload limit in bytes for the requesting sender (5 GB for non-API-key uploads, 100 GB for API-key uploads)." + description: "Rolling-window upload limit in bytes for the authenticated API-key tenant (100 GB)." window_days: type: "integer" description: "Length of the rolling window in days." per_upload_limit_bytes: type: "integer" format: "int64" - description: "Maximum size of a single upload in bytes for the requesting sender (5 GB for non-API-key uploads, 100 GB for API-key uploads)." + description: "Maximum size of a single upload in bytes for the authenticated API-key tenant (100 GB)." resets_at: type: "string" format: "date-time" @@ -409,6 +423,11 @@ paths: "RFC-3339 timestamp at which the oldest recorded upload falls out of the rolling window, partially freeing quota. Null if the sender has no recorded uploads." + "401": + description: + "No valid `Authorization: Bearer PG-…` API key was presented. Usage + can only be queried by the authenticated tenant it is accounted + for." /filedownload/{uuid}: get: diff --git a/src/main.rs b/src/main.rs index 30fe919..e1944e8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -300,6 +300,47 @@ impl<'r> FromRequest<'r> for ApiKey { } } +/// Request guard for routes that require a *validated* pg-pkg API key. +/// +/// Unlike [`ApiKey`], whose `FromRequest` always succeeds (degrading callers +/// without a valid key to the anonymous "default tier"), this guard **fails** +/// the request when no valid credentials are presented: +/// - `NoCredentials` / `Rejected` → `401 Unauthorized` +/// - `PkgUnreachable` → `503 Service Unavailable` (we cannot confirm the key) +/// +/// A route carrying this guard is therefore authenticated by construction — +/// the "authenticated" intent is enforced by the type system rather than by a +/// guard that always succeeds and leaves the check to the handler. +struct ValidatedApiKey { + tenant: String, +} + +#[rocket::async_trait] +impl<'r> FromRequest<'r> for ValidatedApiKey { + type Error = (); + async fn from_request(req: &'r rocket::Request<'_>) -> rocket::request::Outcome { + let header = req.headers().get_one("Authorization"); + let Some(client) = req.rocket().state::() else { + log::error!("PkgClient missing from Rocket state — rejecting authenticated request"); + return rocket::request::Outcome::Error((rocket::http::Status::ServiceUnavailable, ())); + }; + match client.validate(header).await { + ValidationOutcome::Validated(tenant) => { + rocket::request::Outcome::Success(ValidatedApiKey { tenant }) + } + ValidationOutcome::NoCredentials | ValidationOutcome::Rejected => { + rocket::request::Outcome::Error((rocket::http::Status::Unauthorized, ())) + } + ValidationOutcome::PkgUnreachable => { + log::warn!( + "pg-pkg unreachable during API-key validation on an authenticated route; returning 503" + ); + rocket::request::Outcome::Error((rocket::http::Status::ServiceUnavailable, ())) + } + } + } +} + #[post("/fileupload/init", data = "")] async fn upload_init( config: &State, @@ -495,8 +536,22 @@ fn compute_hash(cryptify_token: &[u8], data: &[u8]) -> String { /// message can't drift silently between call sites. const TOKEN_MISMATCH_MSG: &str = "Cryptify Token header does not match"; +/// Constant-time compare of a presented `CryptifyToken` against the expected +/// value. Both are hex-encoded SHA-256 strings; `subtle::ConstantTimeEq` keeps +/// the timing independent of where the bytes first differ, mirroring +/// `recovery_tokens_match`. A network timing oracle against these 256-bit +/// high-entropy tokens is impractical, but we compare them in constant time +/// for consistency with the recovery-token path. +fn cryptify_tokens_match(presented: &str, expected: &str) -> bool { + use subtle::ConstantTimeEq; + if presented.len() != expected.len() { + return false; + } + presented.as_bytes().ct_eq(expected.as_bytes()).into() +} + fn check_cryptify_token(header: &str, expected: &str) -> Result<(), Error> { - if header != expected { + if !cryptify_tokens_match(header, expected) { return Err(Error::BadRequest(Some(TOKEN_MISMATCH_MSG.to_owned()))); } Ok(()) @@ -548,9 +603,11 @@ async fn upload_chunk( // `classify_chunk_request` — we only commit to reading the body when // the request looks like either a normal next chunk or a candidate // replay of the last committed chunk. - let is_normal_next = state.uploaded == start && headers.cryptify_token == state.cryptify_token; + let is_normal_next = state.uploaded == start + && cryptify_tokens_match(&headers.cryptify_token, &state.cryptify_token); let is_replay_candidate = state.last_chunk.as_ref().is_some_and(|last| { - last.prev_uploaded == start && headers.cryptify_token == last.prev_token + last.prev_uploaded == start + && cryptify_tokens_match(&headers.cryptify_token, &last.prev_token) }); if !is_normal_next && !is_replay_candidate { if state.uploaded != start { @@ -672,12 +729,12 @@ fn classify_chunk_request( start: u64, body: &[u8], ) -> ChunkClassification { - if state.uploaded == start && request_token == state.cryptify_token { + if state.uploaded == start && cryptify_tokens_match(request_token, &state.cryptify_token) { return ChunkClassification::NormalNext; } if let Some(last) = state.last_chunk.as_ref() { - if request_token == last.prev_token && start == last.prev_uploaded { + if cryptify_tokens_match(request_token, &last.prev_token) && start == last.prev_uploaded { // Recompute the rolling hash over the incoming body. Identity // is implicit in the rolling-token construction itself: if the // hash matches `response_token`, the body is byte-identical to @@ -981,30 +1038,31 @@ struct UsageResponse { } #[get("/usage?")] -fn usage(store: &State, api_key: ApiKey, email: String) -> Json { - let (rolling_limit, per_upload_limit) = if api_key.tenant.is_some() { - (API_KEY_ROLLING_LIMIT, API_KEY_PER_UPLOAD_LIMIT) - } else { - (ROLLING_LIMIT, PER_UPLOAD_LIMIT) - }; +fn usage( + store: &State, + api_key: ValidatedApiKey, + email: Option, +) -> Json { let now = chrono::offset::Utc::now().timestamp(); - // For API-key callers the rolling window is accounted per tenant, not - // per sender email — query the same key the finalize path records under. - let lookup_key = match api_key.tenant.as_deref() { - Some(tenant) => format!("api-key:{}", tenant), - None => email.clone(), - }; + // Usage is accounted per validated tenant, keyed by the tenant proven via + // the `Authorization` header — never by a caller-supplied email. The + // `ValidatedApiKey` guard has already rejected any unauthenticated caller + // with 401, so there is no way to query usage for an arbitrary address and + // hence no user-enumeration / activity-monitoring oracle. The `email` + // query parameter is retained only so the response can echo it back for + // frontends; it does not influence the lookup. + let lookup_key = format!("api-key:{}", api_key.tenant); let usage = store.get_usage(&lookup_key, now); let resets_at = usage .oldest_expires_at .and_then(|ts| chrono::DateTime::from_timestamp(ts, 0)) .map(|dt| dt.to_rfc3339_opts(chrono::SecondsFormat::Secs, true)); Json(UsageResponse { - email, + email: email.unwrap_or_default(), used_bytes: usage.used_bytes, - limit_bytes: rolling_limit, + limit_bytes: API_KEY_ROLLING_LIMIT, window_days: (ROLLING_WINDOW_SECS / 86_400) as u64, - per_upload_limit_bytes: per_upload_limit, + per_upload_limit_bytes: API_KEY_PER_UPLOAD_LIMIT, resets_at, }) } @@ -1486,6 +1544,44 @@ mod tests { ); } + #[test] + fn cryptify_tokens_match_accepts_equal_and_rejects_different() { + assert!(cryptify_tokens_match("abc123", "abc123")); + assert!(!cryptify_tokens_match("abc123", "abc124")); + // Differing lengths must not match (and must not panic). + assert!(!cryptify_tokens_match("abc", "abc123")); + assert!(!cryptify_tokens_match("", "abc")); + } + + // Mounts only the `/usage` route with the state its guard depends on + // (`Store` + `PkgClient`). The `PkgClient` url is never contacted for the + // unauthenticated case: `PkgClient::validate(None)` short-circuits to + // `NoCredentials` before any network call. + async fn usage_client() -> Client { + let rocket = rocket::build() + .mount("/", routes![usage]) + .manage(Store::new(Arc::new(Metrics::new()))) + .manage(PkgClient::new("http://localhost:1".to_string())); + Client::tracked(rocket).await.expect("valid rocket") + } + + // Regression test for GHSA-5rhx-xgvv-h78h: an unauthenticated caller (no + // `Authorization: Bearer PG-…` header) must be rejected with 401 rather + // than served usage for an arbitrary, caller-supplied email address. + #[rocket::async_test] + async fn usage_rejects_unauthenticated_request() { + let client = usage_client().await; + let res = client + .get("/usage?email=alice@example.com") + .dispatch() + .await; + assert_eq!( + res.status(), + Status::Unauthorized, + "unauthenticated /usage must be 401, not a usage oracle for an arbitrary email" + ); + } + // Builds a minimal rocket instance that mounts only `upload_init` and the // state it depends on, with a fresh per-test `data_dir` under // `std::env::temp_dir()`. Used to verify upload_init's rejection path