mirror of
https://github.com/block/buzz.git
synced 2026-11-12 20:31:12 +00:00
hardening(auth): TTL ceiling, key-case invariant, structured error tracing
Red-team pass against the auth lane surfaced one real bug and two
robustness gaps. All three caught by tests, the bug verified by
temporarily reverting the fix and watching the test fail with the
real Redis error.
1. Real bug: a caller passing ttl_secs > i64::MAX (e.g. u64::MAX from
a config bug) caused Redis to return "ResponseError: value is not an
integer or out of range" from `SET NX EX <ttl>`. RedisNip98ReplayGuard
then returned Err, the trait contract forces callers to fail closed,
and every NIP-98-gated request from that point would have errored
with no visible link back to the bad config. Fix: introduce
MAX_REPLAY_TTL_SECS (1 hour — 30× the natural physical maximum, well
inside i64::MAX) and clamp ttl_secs into [DEFAULT, MAX] before the
SET. New ignored-Redis test `above_ceiling_ttl_is_clamped` exercises
the path with u64::MAX and asserts the claim+replay sequence
succeeds, which it only does with the clamp.
2. Robustness: pin "all rate-limit and replay key components are
lowercase ASCII" as a unit-level invariant. If pubkey::to_hex,
Uuid::Display, or LimitType::key_suffix ever started emitting
uppercase, the same logical (community, pubkey/event_id) would map to
two distinct Redis keys — silently doubling the rate-limit quota or
breaking the seen-set's identity. Two new tests
(`rate_limit_key_components_are_lowercase`,
`key_components_are_lowercase`) catch the regression in CI rather
than production.
3. Robustness: structured tracing on every Redis failure path with
`community = %ctx.community()` as a structured field, so ops can
group log alerts by tenant without needing the community id to be
embedded in the AuthError string. The user-facing AuthError::Internal
payload stays the existing convention (consistent with rate_limit.rs
neighbors); the per-tenant context lives in tracing fields, not in
the error string.
Also: add `ttl_floor_below_ceiling` and `max_ttl_fits_in_redis_signed_ex`
unit tests so the two TTL constants can't drift past each other or
above Redis's signed-EX limit in a future edit.
Out of scope for this lane (flagged to other lane owners):
- AuthError::Internal generally embeds raw downstream error strings
(existing pattern across rate_limit.rs and nip98_replay.rs). Could
leak community/tenant identifiers if those strings ever surface to
clients. Audit lane (Quinn) owns the error-message safety rule per
Eva's [6] lane split.
- check_ip_connection MUST be called before host resolution / on
every connection (including failed-host-resolution attempts).
Otherwise an attacker who picks a non-matching host header bypasses
the IP cap. Wiring lives in the relay-wiring lane (Eva).
Validation:
- cargo test -p buzz-auth --lib: 44 passed (4 new red-team tests).
- cargo test -p buzz-pubsub --lib: 3 passed, 10 Redis-required
ignored.
- cargo test -p buzz-pubsub --lib nip98_replay -- --ignored against
local Redis: 4 passed (1 new ceiling-clamp test).
- Bug verified: with the clamp temporarily reverted, the
above_ceiling_ttl_is_clamped test fails with the real Redis error
"value is not an integer or out of range" — proving the test
catches the regression, not just the fix.
(cherry picked from commit f54d728e25)
Co-authored-by: Sami <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: tlongwell-block <109685178+tlongwell-block@users.noreply.github.com>
This commit is contained in:
@@ -6,7 +6,9 @@
|
||||
|
||||
use buzz_auth::{
|
||||
error::AuthError,
|
||||
nip98_replay::{nip98_replay_key, Nip98ReplayGuard, DEFAULT_REPLAY_TTL_SECS},
|
||||
nip98_replay::{
|
||||
nip98_replay_key, Nip98ReplayGuard, DEFAULT_REPLAY_TTL_SECS, MAX_REPLAY_TTL_SECS,
|
||||
},
|
||||
};
|
||||
use buzz_core::TenantContext;
|
||||
use nostr::EventId;
|
||||
@@ -37,14 +39,25 @@ impl Nip98ReplayGuard for RedisNip98ReplayGuard {
|
||||
event_id: &EventId,
|
||||
ttl_secs: u64,
|
||||
) -> Result<bool, AuthError> {
|
||||
// §5 gate floor — never accept a sub-floor TTL silently.
|
||||
let ttl = ttl_secs.max(DEFAULT_REPLAY_TTL_SECS);
|
||||
// §5 gate floor + safety ceiling. Sub-floor values are lifted to the
|
||||
// floor (contract permits clamping); above-ceiling values are pushed
|
||||
// down to MAX_REPLAY_TTL_SECS (contract REQUIRES clamping) so a buggy
|
||||
// caller cannot send a Redis-incompatible `EX` arg or pin a slot for
|
||||
// implausibly long.
|
||||
let ttl = ttl_secs
|
||||
.max(DEFAULT_REPLAY_TTL_SECS)
|
||||
.min(MAX_REPLAY_TTL_SECS);
|
||||
|
||||
let mut conn = self
|
||||
.pool
|
||||
.get()
|
||||
.await
|
||||
.map_err(|e| AuthError::Internal(format!("Redis pool: {e}")))?;
|
||||
let mut conn = self.pool.get().await.map_err(|e| {
|
||||
// Structured field for ops; the user-facing AuthError stays a
|
||||
// bounded category string.
|
||||
tracing::warn!(
|
||||
community = %ctx.community(),
|
||||
error = %e,
|
||||
"nip98 replay: redis pool acquire failed — caller MUST fail closed"
|
||||
);
|
||||
AuthError::Internal(format!("Redis pool: {e}"))
|
||||
})?;
|
||||
|
||||
let key = nip98_replay_key(ctx, event_id);
|
||||
|
||||
@@ -59,14 +72,28 @@ impl Nip98ReplayGuard for RedisNip98ReplayGuard {
|
||||
.arg(ttl)
|
||||
.query_async(&mut *conn)
|
||||
.await
|
||||
.map_err(|e| AuthError::Internal(format!("Redis SET NX EX: {e}")))?;
|
||||
.map_err(|e| {
|
||||
tracing::warn!(
|
||||
community = %ctx.community(),
|
||||
error = %e,
|
||||
"nip98 replay: redis SET NX EX failed — caller MUST fail closed"
|
||||
);
|
||||
AuthError::Internal(format!("Redis SET NX EX: {e}"))
|
||||
})?;
|
||||
|
||||
match result.as_deref() {
|
||||
Some("OK") => Ok(true),
|
||||
None => Ok(false),
|
||||
Some(other) => Err(AuthError::Internal(format!(
|
||||
"unexpected SET NX EX reply: {other}"
|
||||
))),
|
||||
Some(other) => {
|
||||
tracing::error!(
|
||||
community = %ctx.community(),
|
||||
reply = %other,
|
||||
"nip98 replay: redis SET NX EX returned an unexpected reply — investigate"
|
||||
);
|
||||
Err(AuthError::Internal(format!(
|
||||
"unexpected SET NX EX reply: {other}"
|
||||
)))
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -148,4 +175,28 @@ mod tests {
|
||||
assert!(guard.try_mark(&ctx, &eid, 30).await.expect("mark"));
|
||||
assert!(!guard.try_mark(&ctx, &eid, 30).await.expect("replay"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Redis"]
|
||||
async fn above_ceiling_ttl_is_clamped() {
|
||||
let guard = RedisNip98ReplayGuard::new(redis_pool());
|
||||
let ctx = fresh_ctx();
|
||||
let eid = fresh_event_id();
|
||||
|
||||
// Caller asks for u64::MAX (well past Redis's i64::MAX `EX` limit).
|
||||
// Impl MUST clamp down to MAX_REPLAY_TTL_SECS so the SET succeeds; if
|
||||
// we instead forwarded u64::MAX, Redis would reject the EX arg and
|
||||
// try_mark would return Err — and per the trait contract, callers
|
||||
// fail closed on Err. That's correctness-preserving but UX-hostile:
|
||||
// every nip98 request errors. The clamp turns a foot-gun into a
|
||||
// contained warning.
|
||||
assert!(guard
|
||||
.try_mark(&ctx, &eid, u64::MAX)
|
||||
.await
|
||||
.expect("mark with extreme ttl must succeed via clamp"));
|
||||
assert!(!guard
|
||||
.try_mark(&ctx, &eid, u64::MAX)
|
||||
.await
|
||||
.expect("replay with extreme ttl must succeed via clamp"));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user