fix(security): a signed Slack request could be replayed forever
Phase 5. The headline is not the coverage work — it is what looking for coverage found. A CAPTURED SLACK REQUEST AUTHENTICATED INDEFINITELY `slack_signature_valid` verified the HMAC correctly, and nothing anywhere checked how old the timestamp was. The timestamp is an input to the basestring, so an old request's signature verifies exactly as well as a fresh one — meaning anyone holding a single captured signed request (a proxy log, a mirrored packet, a leaked webhook body) could replay it forever, and every replay would authenticate. Slack's documented 5-minute window is now enforced IN THE BROKER, not the caller: the broker does not trust its caller (§15), and a check the caller can forget to make is one that will eventually be forgotten. Symmetric, so a far-future timestamp cannot mint a request valid for as long as the attacker chooses. Seven unit tests over the pure function with the clock injected, and the HTTP-level test now asserts an hour-old but validly signed request is refused. Negative control: removing the window fails the stale and future cases specifically. The existing slack_inbound test used the literal timestamp "12345" — a 1970 date — which passed only because nothing checked freshness. That is the shape of the whole finding: the fixture could not have failed, so it never told us anything. COVERAGE, RE-EXAMINED The review ranked crates by raw test count. That metric was misleading and found the wrong crates: cm-safety's seven tests already cover the decide CAS, grant double-consume, expiry and the approved/rejected split, and the audit_log immutability trigger is tested over in cm-db. Reading the API surface against the tests found the real gaps — verify_slack_signature above, and `credits_for_tokens`, pure pricing arithmetic that every existing billing test went through the database to reach without ever checking directly. Now pinned: the round-up contract, the deliberate one-credit floor, and that an absurd token count cannot wrap into a negative charge (a refund granted by an overflow). Still genuinely thin: cm-brain, where 6 of 9 tests need live clawbrainhub.com. Stubbing it means reproducing an external registry protocol we have no spec for — its own piece of work, not a coverage chore. Recorded rather than faked. GATEWAY PREFLIGHT ZEROCLAW_GATEWAY_URL and ZEROCLAW_TOKEN have no defaults and are read at FIRST USE, so a deployment missing them boots clean, serves every page, and fails the first time someone presses run. Third sibling of runtime_preflight and validator_preflight, same stance: a report, not a gate. The message names the consequence — "container-tier missions cannot run" — rather than only the unset variable. One process note: `cargo test -p cm-secrets` passed while the LIBRARY build was broken, because `time` is a dev-dependency there and my reference to it only resolved under cfg(test). Switched to std. Checking `cargo build --workspace` as well as the test profile is the guard. Full workspace suite green: 106 binaries, zero build errors. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
5c2c63f8e8
commit
113de610ec
@@ -0,0 +1,160 @@
|
||||
//! Is the mission gateway configured, and is anything listening?
|
||||
//!
|
||||
//! The third sibling of [`crate::runtime_preflight`] and
|
||||
//! [`crate::validator_preflight`], for the same class of failure: the
|
||||
//! configuration is absent or wrong, and nothing says so until a mission pays
|
||||
//! for it.
|
||||
//!
|
||||
//! `ZEROCLAW_GATEWAY_URL` and `ZEROCLAW_TOKEN` have no defaults and are read at
|
||||
//! FIRST USE, inside `ZeroClawDriveExecutor::from_env`. So a deployment missing
|
||||
//! them boots clean, serves every page, lists every mission — and fails the
|
||||
//! first time someone presses run, with an error that surfaces on a phase
|
||||
//! rather than at startup. The information exists the whole time; nobody is
|
||||
//! told until it is expensive.
|
||||
//!
|
||||
//! A report, not a gate, matching its siblings. A server with no gateway should
|
||||
//! still boot: the frontend, the catalogue and every read path work without it,
|
||||
//! and refusing to start would turn a degraded deployment into a dead one.
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
const PROBE_TIMEOUT: Duration = Duration::from_secs(5);
|
||||
|
||||
/// What the preflight found.
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
pub enum Verdict {
|
||||
/// No gateway configured. Missions on the container tier cannot run.
|
||||
NotConfigured { missing: Vec<String> },
|
||||
/// Configured but nothing answered at that address.
|
||||
Unreachable { url: String, error: String },
|
||||
/// Configured and something answered.
|
||||
Reachable { url: String },
|
||||
}
|
||||
|
||||
impl Verdict {
|
||||
/// The line to print at boot.
|
||||
///
|
||||
/// Each names the CONSEQUENCE, not just the state. "ZEROCLAW_TOKEN not set"
|
||||
/// tells an operator what is missing; it does not tell them that every
|
||||
/// container-tier mission they launch will fail on its first phase.
|
||||
pub fn message(&self) -> String {
|
||||
match self {
|
||||
Verdict::NotConfigured { missing } => format!(
|
||||
"gateway_preflight: NOT CONFIGURED ({}) — container-tier missions \
|
||||
cannot run. They will launch, provision a runtime, and fail on \
|
||||
the first turn; the server is otherwise healthy",
|
||||
missing.join(", ")
|
||||
),
|
||||
Verdict::Unreachable { url, error } => format!(
|
||||
"gateway_preflight: {url} is configured but did not answer ({error}) \
|
||||
— container-tier missions will fail on their first turn. The \
|
||||
config is right and the machine is not"
|
||||
),
|
||||
Verdict::Reachable { url } => {
|
||||
format!("gateway_preflight: {url} answered")
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Which required variables are absent.
|
||||
///
|
||||
/// Split from the network probe so the rule is testable without a gateway:
|
||||
/// this is the half that is pure, and it is the half that is wrong most often.
|
||||
pub fn missing_config(url: Option<&str>, token: Option<&str>, pairing: Option<&str>) -> Vec<String> {
|
||||
let mut missing = Vec::new();
|
||||
if url.map(str::trim).unwrap_or("").is_empty() {
|
||||
missing.push("ZEROCLAW_GATEWAY_URL".to_string());
|
||||
}
|
||||
// Either credential works: a durable token, or a one-time pairing code the
|
||||
// executor exchanges on first use.
|
||||
let has_token = !token.map(str::trim).unwrap_or("").is_empty();
|
||||
let has_pairing = !pairing.map(str::trim).unwrap_or("").is_empty();
|
||||
if !has_token && !has_pairing {
|
||||
missing.push("ZEROCLAW_TOKEN or ZEROCLAW_PAIRING_CODE".to_string());
|
||||
}
|
||||
missing
|
||||
}
|
||||
|
||||
fn env_opt(key: &str) -> Option<String> {
|
||||
std::env::var(key).ok().filter(|v| !v.trim().is_empty())
|
||||
}
|
||||
|
||||
/// Probe the configured gateway.
|
||||
pub async fn check() -> Verdict {
|
||||
let url = env_opt("ZEROCLAW_GATEWAY_URL");
|
||||
let missing = missing_config(
|
||||
url.as_deref(),
|
||||
env_opt("ZEROCLAW_TOKEN").as_deref(),
|
||||
env_opt("ZEROCLAW_PAIRING_CODE").as_deref(),
|
||||
);
|
||||
if !missing.is_empty() {
|
||||
return Verdict::NotConfigured { missing };
|
||||
}
|
||||
let url = url.expect("checked above");
|
||||
|
||||
// Any HTTP answer proves something is listening and routable, which is the
|
||||
// question this preflight exists to answer. Authenticating here would need
|
||||
// a pairing exchange that BURNS a one-time code — a preflight that costs
|
||||
// the deployment its credential is worse than no preflight.
|
||||
let client = match reqwest::Client::builder().timeout(PROBE_TIMEOUT).build() {
|
||||
Ok(c) => c,
|
||||
Err(e) => {
|
||||
return Verdict::Unreachable {
|
||||
url,
|
||||
error: e.to_string(),
|
||||
}
|
||||
}
|
||||
};
|
||||
match client.get(&url).send().await {
|
||||
Ok(_) => Verdict::Reachable { url },
|
||||
Err(e) => Verdict::Unreachable {
|
||||
url,
|
||||
error: e.to_string(),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/// Run the probe and print the verdict. Never panics, never blocks boot.
|
||||
pub fn report_at_boot() {
|
||||
tokio::spawn(async {
|
||||
eprintln!("{}", check().await.message());
|
||||
});
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn a_fully_configured_deployment_is_missing_nothing() {
|
||||
assert!(missing_config(Some("http://gw:42617"), Some("tok"), None).is_empty());
|
||||
// A pairing code alone is enough — the executor exchanges it on first use.
|
||||
assert!(missing_config(Some("http://gw:42617"), None, Some("123456")).is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_empty_string_counts_as_absent() {
|
||||
// The failure this whole module exists for: `unwrap_or_default` and an
|
||||
// empty env var turn "unconfigured" into "configured with nothing",
|
||||
// which fails later as a 401 rather than now as a missing setting.
|
||||
let missing = missing_config(Some(" "), Some(""), Some(" "));
|
||||
assert_eq!(missing.len(), 2, "both must be reported: {missing:?}");
|
||||
assert!(missing[0].contains("GATEWAY_URL"));
|
||||
assert!(missing[1].contains("ZEROCLAW_TOKEN"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_message_names_the_consequence_not_just_the_state() {
|
||||
let v = Verdict::NotConfigured {
|
||||
missing: vec!["ZEROCLAW_GATEWAY_URL".into()],
|
||||
};
|
||||
let m = v.message();
|
||||
assert!(m.contains("ZEROCLAW_GATEWAY_URL"));
|
||||
assert!(
|
||||
m.contains("cannot run"),
|
||||
"an operator needs to know what stops working, not only what is \
|
||||
unset: {m}"
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user