From 2c066cc871338e0e180c43e189668a18f787e59c Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 23 Sep 2026 17:00:44 +0200 Subject: [PATCH] hive-c0re: pin the two security boundaries that had no test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_forbidden` and the webhook-secret load/regenerate path were the last two entries on the shortlist in hyperhive/hyperhive#3950; the other three landed in hyperhive/hyperhive#4650. Both are classification logic whose failure mode is silence, which is why they are worth a test rather than a coverage line. `is_forbidden` gates the arms that tell an operator a Forgejo admin PATCH was refused for want of a scope, and which credential to delete and re-mint to fix it. One of those PATCHes is `ensure_repo_creation_disabled` — the lockdown that stops an agent creating a repo it owns and self-merging in it. Two tests: a 403 is recognised in both shapes the typed client produces (the spec-listed `Forbidden` kind and the bare `UnexpectedStatusCode`), and nothing else is — not a 401, whose remedy is the automatic re-mint one function down, and not a transport error that never reached the forge at all. `load_or_generate` grows the path-taking half `load_or_generate_at`, the same seam `swarm-controller`'s `webhook::load_or_generate_at` already has and for the same stated reason. Three tests over it: a valid stored secret is returned verbatim and never rotated (the newline this module writes itself makes the trim load-bearing, not defensive); a malformed one is replaced by a secret that reaches *disk*, not just the caller, and is then stable; and each near miss — empty, whitespace, 63 chars, 65 chars, right length with a non-hex char — is refused. That last one is the security case: `Hmac::new_from_slice` accepts a key of any length, empty included, so a relaxed check fails nowhere and just keys every signature off a guessable value. Every test was confirmed able to fail: six mutations of the code under test, each watched red, then reverted. The two halves of the validity check and the two arms of `is_forbidden` were broken separately, so neither test passes on one arm alone. --- hive-c0re/src/forge/users.rs | 56 ++++++++++++++++++- hive-c0re/src/webhook_secret.rs | 99 +++++++++++++++++++++++++++++++-- 2 files changed, 149 insertions(+), 6 deletions(-) diff --git a/hive-c0re/src/forge/users.rs b/hive-c0re/src/forge/users.rs index 83e7b8ed..2bc6be80 100644 --- a/hive-c0re/src/forge/users.rs +++ b/hive-c0re/src/forge/users.rs @@ -596,7 +596,7 @@ pub fn core_token() -> Option { #[cfg(test)] mod tests { - use super::{CoreTokenCheck, classify_core_token_error}; + use super::{CoreTokenCheck, classify_core_token_error, is_forbidden}; use forgejo_api::{ApiError, ApiErrorKind, ForgejoError}; use reqwest::StatusCode; @@ -656,4 +656,58 @@ mod tests { CoreTokenCheck::Indeterminate ); } + + /// The typed client only produces `ApiErrorKind::Forbidden` when the + /// endpoint's spec lists 403; every other endpoint surfaces the same + /// response as a bare `UnexpectedStatusCode`. Both shapes must be + /// recognised, because the arms guarded by this predicate are what tell + /// the operator that `ensure_repo_creation_disabled` — the lockdown + /// stopping an agent from creating and self-merging in its own repo — + /// did not apply, and which credential to re-mint to make it apply. + /// Drop the second arm as "redundant" and that lockdown fails with a + /// generic warning that names no remedy. + #[test] + fn a_403_is_recognised_in_both_shapes_the_client_can_produce() { + assert!(is_forbidden(&api_err(ApiErrorKind::Forbidden))); + assert!(is_forbidden(&ForgejoError::UnexpectedStatusCode( + StatusCode::FORBIDDEN + ))); + } + + /// The other direction, and the more expensive one to get wrong: this + /// predicate gates advice to **delete the core token and restart + /// hive-c0re**. A widened match would hand that advice out for a 502 + /// from a restarting forge or a client-side error that never reached + /// the network — destroying a working credential in response to a blip. + /// 401 is called out separately because it is the near miss: it *is* a + /// credential problem, but its remedy is [`classify_core_token_error`]'s + /// automatic re-mint, so classifying it as forbidden buries the cause + /// under a scope warning that does not apply. + #[test] + fn a_non_403_failure_is_not_reported_as_a_missing_admin_scope() { + for kind in [ + ApiErrorKind::Unauthorized, + ApiErrorKind::NotFound { errors: None }, + ApiErrorKind::ValidationFailed, + ApiErrorKind::Generic, + ] { + let e = api_err(kind); + assert!(!is_forbidden(&e), "{e} must not read as forbidden"); + } + for s in [ + StatusCode::UNAUTHORIZED, + StatusCode::NOT_FOUND, + StatusCode::BAD_GATEWAY, + StatusCode::INTERNAL_SERVER_ERROR, + ] { + assert!( + !is_forbidden(&ForgejoError::UnexpectedStatusCode(s)), + "status {s} must not read as forbidden" + ); + } + // A failure with no HTTP status at all — the request never got an + // answer, so nothing has been said about the token's scopes. + assert!(!is_forbidden(&ForgejoError::KeyNotAscii)); + assert!(!is_forbidden(&ForgejoError::HostRequired)); + } } diff --git a/hive-c0re/src/webhook_secret.rs b/hive-c0re/src/webhook_secret.rs index 8674abd1..e3207d2e 100644 --- a/hive-c0re/src/webhook_secret.rs +++ b/hive-c0re/src/webhook_secret.rs @@ -17,8 +17,14 @@ use anyhow::{Context as _, Result}; /// /// Returns a hex-encoded 32-byte secret string (64 hex chars). pub fn load_or_generate() -> Result { - let path = crate::paths::webhook_secret_file(); - if let Ok(raw) = std::fs::read_to_string(&path) { + load_or_generate_at(&crate::paths::webhook_secret_file()) +} + +/// The path-taking half of [`load_or_generate`], split out so a test can +/// point it at a scratch file — same seam, for the same reason, as +/// `swarm-controller`'s `webhook::load_or_generate_at`. +fn load_or_generate_at(path: &std::path::Path) -> Result { + if let Ok(raw) = std::fs::read_to_string(path) { let trimmed = raw.trim().to_owned(); if trimmed.len() == 64 && trimmed.chars().all(|c| c.is_ascii_hexdigit()) { return Ok(trimmed); @@ -30,9 +36,9 @@ pub fn load_or_generate() -> Result { ); } let secret = generate_hex_secret()?; - std::fs::create_dir_all(path.parent().unwrap_or(&path)) + std::fs::create_dir_all(path.parent().unwrap_or(path)) .with_context(|| format!("create dir for {}", path.display()))?; - std::fs::write(&path, format!("{secret}\n")) + std::fs::write(path, format!("{secret}\n")) .with_context(|| format!("write webhook secret to {}", path.display()))?; tracing::info!(path = %path.display(), "webhook secret generated and persisted"); Ok(secret) @@ -100,7 +106,7 @@ fn hex_decode(s: &str) -> Option> { #[cfg(test)] mod tests { - use super::{hex_encode, verify_signature}; + use super::{hex_encode, load_or_generate_at, verify_signature}; /// Compute the `sha256=` header Forgejo would send for `secret` + /// `body`, so the "matches" test below isn't asserting against a @@ -134,4 +140,87 @@ mod tests { assert!(verify_signature("different-secret", b"payload", &header).is_err()); assert!(verify_signature("s3cr3t", b"tampered-payload", &header).is_err()); } + + /// A stored secret that already parses must be handed back exactly as + /// written, on every start. This is the property Forgejo's copy depends + /// on: the secret is registered *with Forgejo* once, so a load that + /// rotates a perfectly good value silently invalidates every webhook + /// delivery afterwards — and it surfaces as an outage, not as anything + /// security-shaped. The trailing newline is the one this module writes + /// itself (`format!("{secret}\n")`), so the trim is load-bearing rather + /// than defensive: without it the file this code just wrote reads back + /// as 65 chars and fails its own validity check on the next boot. + #[test] + fn a_valid_stored_secret_is_returned_verbatim_and_never_rotated() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("webhook-secret"); + let seeded = "a".repeat(64); + std::fs::write(&path, format!("{seeded}\n")).expect("seed"); + + assert_eq!( + load_or_generate_at(&path).expect("load"), + seeded, + "a valid secret must be read back, not regenerated" + ); + assert_eq!( + load_or_generate_at(&path).expect("second load"), + seeded, + "and still on the next start" + ); + } + + /// The regeneration branch has two halves and only one of them is in the + /// return value: the replacement must also be *persisted*, or every + /// restart mints a fresh secret and the registration in Forgejo is never + /// the one being verified against. + #[test] + fn a_malformed_secret_file_is_replaced_by_a_valid_persisted_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("webhook-secret"); + std::fs::write(&path, "not-a-hex-secret\n").expect("seed"); + + let secret = load_or_generate_at(&path).expect("regenerates"); + assert_eq!(secret.len(), 64, "hex-encoded 32 bytes"); + assert!(secret.chars().all(|c| c.is_ascii_hexdigit())); + assert_eq!( + std::fs::read_to_string(&path).expect("read back").trim(), + secret, + "the regenerated secret must reach disk, not just the caller" + ); + assert_eq!( + load_or_generate_at(&path).expect("second load"), + secret, + "and must then be stable across a restart" + ); + } + + /// The length + charset check is what stops a truncated or half-written + /// file from being adopted as the HMAC key. `Hmac::new_from_slice` + /// accepts a key of *any* length, empty included — so relaxing this + /// check does not fail anywhere, it just silently keys every signature + /// off a value an attacker can guess. Each near miss is listed + /// separately so a check that stops distinguishing one of them shows up + /// as that case rather than as a single opaque failure. + #[test] + fn a_near_miss_secret_file_is_not_adopted_as_the_key() { + for (label, seeded) in [ + ("empty file", String::new()), + ("whitespace only", " \n".to_owned()), + ("one hex digit short", "b".repeat(63)), + ("one hex digit long", "b".repeat(65)), + ("right length, non-hex char", format!("z{}", "b".repeat(63))), + ] { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("webhook-secret"); + std::fs::write(&path, &seeded).expect("seed"); + + let secret = load_or_generate_at(&path).expect("regenerates"); + assert_ne!(secret, seeded.trim(), "{label} must not become the key"); + assert_eq!(secret.len(), 64, "{label}: replacement is 64 hex chars"); + assert!( + secret.chars().all(|c| c.is_ascii_hexdigit()), + "{label}: replacement is hex" + ); + } + } }