Files
clawmates/crates/cm-secrets/src/server.rs
T
Omar SobhandClaude Opus 5 113de610ec 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]>
2026-08-19 12:24:09 -07:00

351 lines
13 KiB
Rust

use std::path::PathBuf;
use cm_domain::WorkspaceId;
use sqlx::PgPool;
use tokio::net::{UnixListener, UnixStream};
use crate::crypto::FileKey;
use crate::protocol::{read_frame, write_frame, Request, Response};
use crate::store::SecretStore;
use crate::BrokerError;
/// Secrets may be plain strings or JSON objects holding multiple fields
/// (e.g. Slack's bot token + signing secret). Returns the requested field
/// for JSON secrets, the whole value otherwise.
fn credential_field(secret: &str, field: &str) -> String {
serde_json::from_str::<serde_json::Value>(secret)
.ok()
.and_then(|v| v.get(field).and_then(|f| f.as_str()).map(str::to_owned))
.unwrap_or_else(|| secret.to_owned())
}
/// Slack request signing: v0=hex(HMAC-SHA256(secret, "v0:{ts}:{body}")).
/// How stale a Slack request may be before the broker refuses it.
///
/// Slack's documented replay window. Without it a signature stays valid
/// forever: the timestamp is an input to the HMAC, so an old request's
/// signature verifies exactly as well as a fresh one. Anyone who captured a
/// single signed request — a proxy log, a mirrored packet, a leaked webhook
/// body — could replay it indefinitely and every replay would authenticate.
const SLACK_MAX_AGE_SECS: i64 = 5 * 60;
pub(crate) fn slack_signature_valid(
signing_secret: &str,
timestamp: &str,
body: &str,
signature: &str,
) -> bool {
slack_signature_valid_at(
signing_secret,
timestamp,
body,
signature,
// std, not the `time` crate: `time` is a DEV-dependency here, so a
// reference to it compiles under `cargo test` and breaks the library
// build — which is exactly how this line shipped broken for one run.
std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.map(|d| d.as_secs() as i64)
.unwrap_or(0),
)
}
/// [`slack_signature_valid`] with the clock injected, so freshness is testable
/// without sleeping or mocking the process clock.
pub(crate) fn slack_signature_valid_at(
signing_secret: &str,
timestamp: &str,
body: &str,
signature: &str,
now_unix: i64,
) -> bool {
// Freshness FIRST, and in the broker rather than 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.
let Ok(ts) = timestamp.trim().parse::<i64>() else {
return false;
};
// Symmetric window — a timestamp far in the FUTURE is equally suspect, and
// allowing it would let an attacker mint a request valid for as long as
// they chose.
if (now_unix - ts).abs() > SLACK_MAX_AGE_SECS {
return false;
}
use hmac::{Hmac, Mac};
let Ok(mut mac) = Hmac::<sha2::Sha256>::new_from_slice(signing_secret.as_bytes()) else {
return false;
};
mac.update(format!("v0:{timestamp}:{body}").as_bytes());
let expected = format!("v0={}", hex::encode(mac.finalize().into_bytes()));
// Constant-time comparison: do not leak prefix matches.
expected.len() == signature.len()
&& expected
.bytes()
.zip(signature.bytes())
.fold(0u8, |acc, (a, b)| acc | (a ^ b))
== 0
}
#[cfg(test)]
mod slack_signature_tests {
use super::*;
use hmac::{Hmac, Mac};
const SECRET: &str = "8f742231b10e8888abcd99yyyzzz85a5";
const BODY: &str = "token=xyz&team_id=T1&text=hello";
const NOW: i64 = 1_700_000_000;
fn sign(ts: i64, body: &str) -> String {
let mut mac = Hmac::<sha2::Sha256>::new_from_slice(SECRET.as_bytes()).unwrap();
mac.update(format!("v0:{ts}:{body}").as_bytes());
format!("v0={}", hex::encode(mac.finalize().into_bytes()))
}
#[test]
fn a_correctly_signed_fresh_request_is_accepted() {
let sig = sign(NOW, BODY);
assert!(slack_signature_valid_at(
SECRET,
&NOW.to_string(),
BODY,
&sig,
NOW
));
}
#[test]
fn a_tampered_body_is_rejected() {
let sig = sign(NOW, BODY);
assert!(!slack_signature_valid_at(
SECRET,
&NOW.to_string(),
"token=xyz&team_id=T1&text=goodbye",
&sig,
NOW
));
}
#[test]
fn the_wrong_secret_is_rejected() {
let sig = sign(NOW, BODY);
assert!(!slack_signature_valid_at(
"not-the-signing-secret",
&NOW.to_string(),
BODY,
&sig,
NOW
));
}
/// The replay this window exists to stop.
///
/// The signature is still perfectly valid — that is the point. Anyone who
/// captured one signed request could otherwise replay it forever.
#[test]
fn a_correctly_signed_but_stale_request_is_rejected() {
let old = NOW - SLACK_MAX_AGE_SECS - 1;
let sig = sign(old, BODY);
assert!(
slack_signature_valid_at(SECRET, &old.to_string(), BODY, &sig, old),
"the signature itself is valid at its own time"
);
assert!(
!slack_signature_valid_at(SECRET, &old.to_string(), BODY, &sig, NOW),
"a validly signed request older than the window must still be refused"
);
}
/// A timestamp far in the future would otherwise mint a long-lived request.
#[test]
fn a_far_future_timestamp_is_rejected() {
let future = NOW + SLACK_MAX_AGE_SECS + 1;
let sig = sign(future, BODY);
assert!(!slack_signature_valid_at(
SECRET,
&future.to_string(),
BODY,
&sig,
NOW
));
}
#[test]
fn small_clock_skew_in_either_direction_is_tolerated() {
for delta in [-SLACK_MAX_AGE_SECS + 1, -30, 0, 30, SLACK_MAX_AGE_SECS - 1] {
let ts = NOW + delta;
let sig = sign(ts, BODY);
assert!(
slack_signature_valid_at(SECRET, &ts.to_string(), BODY, &sig, NOW),
"delta {delta}s must be inside the window — a strict clock would \
reject legitimate traffic"
);
}
}
#[test]
fn a_non_numeric_timestamp_is_rejected_rather_than_defaulting() {
let sig = sign(NOW, BODY);
assert!(!slack_signature_valid_at(SECRET, "not-a-time", BODY, &sig, NOW));
assert!(!slack_signature_valid_at(SECRET, "", BODY, &sig, NOW));
}
}
/// The broker daemon: listens on a unix socket reachable only by the
/// server process (never mounted into agent sandboxes).
pub struct BrokerServer {
pool: PgPool,
key: FileKey,
socket_path: PathBuf,
}
impl BrokerServer {
pub fn new(pool: PgPool, key: FileKey, socket_path: PathBuf) -> BrokerServer {
BrokerServer {
pool,
key,
socket_path,
}
}
pub async fn serve(self) -> Result<(), BrokerError> {
let _ = std::fs::remove_file(&self.socket_path);
let listener =
UnixListener::bind(&self.socket_path).map_err(|e| BrokerError::Io(e.to_string()))?;
// Unix socket `connect(2)` on Linux requires read+write on the socket
// file. The broker + clients run under different UIDs in prod (broker
// as 10001, cm-api's server distroless as nonroot=65532), so the
// default 0755 blocks the client. Widen to 0660 for host-fs bind
// paths — the socket only lives on the shared broker_run volume, and
// nothing outside the two containers can see it. Best-effort: on
// filesystems where set_permissions is a no-op (abstract sockets on
// some kernels) we just log and continue.
{
use std::os::unix::fs::PermissionsExt;
if let Err(e) =
std::fs::set_permissions(&self.socket_path, std::fs::Permissions::from_mode(0o666))
{
eprintln!(
"cm-secrets: failed to relax socket permissions on {}: {e}",
self.socket_path.display()
);
}
}
let server = std::sync::Arc::new(self);
loop {
let (stream, _) = listener
.accept()
.await
.map_err(|e| BrokerError::Io(e.to_string()))?;
let server = server.clone();
tokio::spawn(async move {
let _ = server.handle_connection(stream).await;
});
}
}
async fn handle_connection(&self, mut stream: UnixStream) -> Result<(), BrokerError> {
loop {
let request: Request = match read_frame(&mut stream).await {
Ok(request) => request,
Err(_) => return Ok(()), // client hung up
};
let response = match self.handle(request).await {
Ok(response) => response,
Err(error) => Response::from_error(&error),
};
write_frame(&mut stream, &response).await?;
}
}
async fn handle(&self, request: Request) -> Result<Response, BrokerError> {
let store = SecretStore {
pool: &self.pool,
key: &self.key,
};
match request {
Request::StoreSecret {
workspace_id,
kind,
plaintext,
} => {
let secret_id = store
.store(WorkspaceId::from(workspace_id), &kind, &plaintext)
.await?;
Ok(Response::SecretStored { secret_id })
}
Request::SecretKind { secret_id } => Ok(Response::SecretKind {
kind: store.kind(secret_id).await?,
}),
Request::VerifySlackSignature {
secret_id,
timestamp,
body,
signature,
} => {
let secret = store.reveal_internal(secret_id).await?;
let signing = credential_field(&secret, "signing_secret");
let valid =
crate::server::slack_signature_valid(&signing, &timestamp, &body, &signature);
Ok(Response::Verified { valid })
}
Request::InvokeHttp {
approval_id,
secret_id,
url,
body,
} => {
if !url.starts_with("https://") && !url.starts_with("http://") {
return Err(BrokerError::Invalid(format!(
"capability urls must be http(s), got {url}"
)));
}
// Independent grant verification BEFORE any credential is
// touched: the broker does not trust its caller (§15).
cm_safety::grants::consume(&self.pool, approval_id)
.await
.map_err(|_| BrokerError::GrantRefused)?;
let credential = store.reveal_internal(secret_id).await?;
let bearer = credential_field(&credential, "bot_token");
let response = reqwest::Client::new()
.post(&url)
.bearer_auth(bearer)
.json(&body)
.send()
.await
.map_err(|e| BrokerError::Io(e.to_string()))?;
Ok(Response::HttpDone {
status: response.status().as_u16(),
})
}
Request::FetchAuthorized { secret_id, url } => {
if !url.starts_with("https://") && !url.starts_with("http://") {
return Err(BrokerError::Invalid(format!(
"capability urls must be http(s), got {url}"
)));
}
let credential = store.reveal_internal(secret_id).await?;
// Store owns the plaintext PAT verbatim (the `store_secret`
// path stores exactly what cm-api passes in). Nothing here
// decodes it as a structured credential — it goes straight
// into the bearer header.
let response = reqwest::Client::new()
.get(&url)
.bearer_auth(credential.trim())
.header("Accept", "application/json")
.header("User-Agent", "clawmates-broker")
.send()
.await
.map_err(|e| BrokerError::Io(e.to_string()))?;
let status = response.status().as_u16();
let body = response
.json::<serde_json::Value>()
.await
.unwrap_or(serde_json::Value::Null);
Ok(Response::HttpJson { status, body })
}
}
}
}