diff --git a/backend/migrations/030_raise_join_ip_rate.down.sql b/backend/migrations/030_raise_join_ip_rate.down.sql new file mode 100644 index 0000000..15dbdf4 --- /dev/null +++ b/backend/migrations/030_raise_join_ip_rate.down.sql @@ -0,0 +1,3 @@ +-- Revert the join ceiling to 60/min for installs still on the raised default +-- (preserves any explicit admin override at another value). +UPDATE config SET value = '60' WHERE key = 'join_ip_rate_per_min' AND value = '300'; diff --git a/backend/migrations/030_raise_join_ip_rate.up.sql b/backend/migrations/030_raise_join_ip_rate.up.sql new file mode 100644 index 0000000..7747ec5 --- /dev/null +++ b/backend/migrations/030_raise_join_ip_rate.up.sql @@ -0,0 +1,15 @@ +-- Raise the per-IP join ceiling from 60/min to 300/min. +-- +-- Rationale: every guest at the venue arrives through one NAT'd public address, +-- so `join_ip:{ip}` is not a per-guest limit at all — it is a ceiling on the +-- whole party. The QR code goes up once and is scanned in a burst: at 60/min, +-- guest 61 onwards is refused on the join screen, which is the one screen with +-- no auto-retry, and every manual retry spends another slot. +-- +-- The code default was already raised to 300 (auth/handlers.rs), but a default +-- only applies when the key is ABSENT, and migration 017 seeds it. Without this +-- UPDATE the raise is dead code on every existing install. +-- +-- Only bump installs still on the seeded default; an admin who deliberately set +-- a different value keeps it (migration 017 seeded 60; this UPDATE is scoped to '60'). +UPDATE config SET value = '300' WHERE key = 'join_ip_rate_per_min' AND value = '60'; diff --git a/backend/src/auth/handlers.rs b/backend/src/auth/handlers.rs index ebe6e7f..811a550 100644 --- a/backend/src/auth/handlers.rs +++ b/backend/src/auth/handlers.rs @@ -243,43 +243,7 @@ pub async fn join( }; if let Some(existing) = replay { - let pin: String = format!("{:04}", rand::rng().random_range(0..10000u32)); - let pin_hash = hash_password(pin.clone(), 12).await?; - sqlx::query( - "UPDATE \"user\" - SET recovery_pin_hash = $2, failed_pin_attempts = 0, pin_locked_until = NULL - WHERE id = $1", - ) - .bind(existing.id) - .bind(&pin_hash) - .execute(&state.pool) - .await?; - - let token = jwt::create_token( - existing.id, - event.id, - existing.role.clone(), - &state.config.jwt_secret, - state.config.session_expiry_days, - ) - .map_err(|e| AppError::Internal(anyhow::anyhow!(e)))?; - let token_hash = jwt::hash_token(&token); - let expires_at = Utc::now() + chrono::Duration::days(state.config.session_expiry_days); - Session::create(&state.pool, existing.id, &token_hash, expires_at).await?; - - tracing::info!( - user_id = %existing.id, - "join retry matched client_join_id; rotated the PIN and re-issued a session" - ); - return Ok(( - StatusCode::CREATED, - Json(JoinResponse { - jwt: token, - pin, - user_id: existing.id, - is_new: true, - }), - )); + return replay_join(&state, event.id, existing).await; } // Reject if a user with this name (case-insensitive) already exists @@ -312,14 +276,26 @@ pub async fn join( // holds this", and for the join key that somebody is a concurrent retry of this very // request — so re-check the key and replay it rather than reporting a name clash the // guest cannot act on. + // REPLAY it, don't 409. The loser of this race is holding proof that it is the same + // attempt as the winner — the same `client_join_id`, minted by the same phone — and + // the winner's row is a few milliseconds old under the same name, so the ordinary + // name+window replay guard passes. Returning a Conflict instead sent the guest to the + // join page's `code === 'conflict'` branch, which renders the NAME-TAKEN screen with a + // PIN entry form — for a PIN that was never displayed to anybody. That is precisely + // the dead end migration 027 exists to close, re-entered through the racing path, and + // it is easy to hit: the guest's instinctive response to a hung request is to reload + // and tap Join again. if let Some(join_key) = effective_join_key - && User::find_by_client_join_id(&state.pool, event.id, join_key) - .await? - .is_some() + && let Some(winner) = + User::find_by_client_join_id(&state.pool, event.id, join_key).await? + && join_key_replayable( + &winner.display_name, + display_name, + winner.created_at, + Utc::now(), + ) { - return Err(AppError::Conflict( - "Dieser Beitritt wird bereits verarbeitet. Bitte versuche es erneut.".into(), - )); + return replay_join(&state, event.id, &winner).await; } return Err(AppError::Conflict(format!( "Der Name \"{}\" ist bereits vergeben.", @@ -353,6 +329,61 @@ pub async fn join( )) } +/// Answer a join retry with the row its `client_join_id` already minted. +/// +/// Rotates the PIN and issues a fresh session. Rotating is safe precisely here: the previous PIN +/// was never displayed to anyone (that is what "the response was lost" means), so no device holds +/// it and there is nothing to invalidate. The alternative — persisting plaintext PINs so they can +/// be replayed — would put a recoverable credential in the database for every guest, to fix a lost +/// packet. +/// +/// Callers must have established that this caller is ENTITLED to the replay (`join_key_replayable`: +/// same display name, inside the window). Both call sites do; the key is accepted pre-auth, so +/// without that check it would be a bearer credential for someone else's account. +async fn replay_join( + state: &AppState, + event_id: Uuid, + existing: &User, +) -> Result<(StatusCode, Json), AppError> { + let pin: String = format!("{:04}", rand::rng().random_range(0..10000u32)); + let pin_hash = hash_password(pin.clone(), 12).await?; + sqlx::query( + "UPDATE \"user\" + SET recovery_pin_hash = $2, failed_pin_attempts = 0, pin_locked_until = NULL + WHERE id = $1", + ) + .bind(existing.id) + .bind(&pin_hash) + .execute(&state.pool) + .await?; + + let token = jwt::create_token( + existing.id, + event_id, + existing.role.clone(), + &state.config.jwt_secret, + state.config.session_expiry_days, + ) + .map_err(|e| AppError::Internal(anyhow::anyhow!(e)))?; + let token_hash = jwt::hash_token(&token); + let expires_at = Utc::now() + chrono::Duration::days(state.config.session_expiry_days); + Session::create(&state.pool, existing.id, &token_hash, expires_at).await?; + + tracing::info!( + user_id = %existing.id, + "join retry matched client_join_id; rotated the PIN and re-issued a session" + ); + Ok(( + StatusCode::CREATED, + Json(JoinResponse { + jwt: token, + pin, + user_id: existing.id, + is_new: true, + }), + )) +} + /// Default for `recover_name_rate_per_15min` — wrong PINs allowed per (IP, name) per 15 min. /// Mirrors migration 023; kept here so the invariant below can be asserted in a test. const RECOVER_NAME_CEILING_DEFAULT: usize = 4; @@ -388,6 +419,10 @@ pub const RECOVER_NAME_CEILING_MAX: usize = (PIN_LOCK_THRESHOLD as usize) / 3; const RECOVER_IP_FAILURE_CEILING: usize = 30; const RECOVER_IP_FAILURE_WINDOW: Duration = Duration::from_secs(15 * 60); +/// Window for the per-(IP, name) failure budget. Matches the per-account lockout duration, so a +/// guest who trips both waits the same fifteen minutes rather than two stacked penalties. +const RECOVER_NAME_WINDOW: Duration = Duration::from_secs(15 * 60); + /// Charge one failed PIN attempt against the per-IP budget, and report whether it is now spent. /// /// Deliberately charged on the way OUT of a failure rather than checked on the way in, so that the @@ -522,9 +557,23 @@ fn bcrypt_busy() -> AppError { } async fn verify_password(candidate: String, hash: String) -> Result { + verify_password_within(candidate, hash, BCRYPT_ACQUIRE_TIMEOUT).await +} + +/// [`verify_password`] with an explicit ceiling on how long it will queue for a hash permit. +/// +/// The short-timeout variant is what lets an over-budget caller still be *served* rather than +/// refused outright: the CPU bound is the semaphore, so shedding on a brief acquire timeout caps +/// the work just as hard as a rate bucket does — but it sheds whoever happens to arrive while the +/// permits are busy, instead of categorically refusing an IP that a flooder shares with the victim. +async fn verify_password_within( + candidate: String, + hash: String, + acquire_timeout: Duration, +) -> Result { // Bounded wait — see BCRYPT_ACQUIRE_TIMEOUT. `acquire()` itself only fails if the semaphore is // closed, which never happens here; the timeout is the case we care about. - let _permit = tokio::time::timeout(BCRYPT_ACQUIRE_TIMEOUT, BCRYPT_PERMITS.acquire()) + let _permit = tokio::time::timeout(acquire_timeout, BCRYPT_PERMITS.acquire()) .await .map_err(|_| bcrypt_busy())?; Ok( @@ -587,30 +636,63 @@ pub async fn recover( Some(retry_after_secs), )); } - - let name_ceiling = config::get_usize( - &state.config_cache, - "recover_name_rate_per_15min", - RECOVER_NAME_CEILING_DEFAULT, - ) - .await - // CLAMPED, not merely defaulted — see RECOVER_NAME_CEILING_MAX. The config value is - // operator-settable and the invariant it has to respect is not expressible in - // `patch_config`'s numeric range, so it is enforced at the point of use. - .min(RECOVER_NAME_CEILING_MAX); - let name_key = display_name.to_lowercase(); - if let Err(retry_after_secs) = state.rate_limiter.check_with_retry( - format!("recover:{ip}:{name_key}"), - name_ceiling, - Duration::from_secs(15 * 60), - ) { - return Err(AppError::TooManyRequests( - "Zu viele Versuche. Bitte warte kurz und versuche es erneut.".into(), - Some(retry_after_secs), - )); - } } + // The per-(IP, name) tier. Same treatment as the cross-name tier below, and for the same + // reason: this bucket is keyed on an IP that behind the venue's NAT is the ENTIRE PARTY, so a + // gate that refuses the request outright refuses it for everyone who shares that name key. + // + // It used to be a `check_with_retry` sitting here, before the account was looked up — which + // means it charged EVERY request, including successful recoveries, and refused before any PIN + // was verified. Ceiling is clamped to RECOVER_NAME_CEILING_MAX (4). So four POSTs naming + // "Braut Sophie" with PIN 0000 locked Sophie out of her own recovery for fifteen minutes, WITH + // THE CORRECT PIN, from any phone on the venue wifi — and four more every fifteen minutes + // sustained it indefinitely, at a request rate far under every volume ceiling above. The + // benign version needs no attacker: the host mistypes their own 4-digit PIN four times. + // + // Now it counts FAILURES only, and a spent budget changes what a failure answers rather than + // refusing outright. A correct PIN always authenticates. Guessing is bounded exactly as before + // — wrong PINs are what spend the budget — with the per-account 3-strike lockout underneath it. + let name_key = display_name.to_lowercase(); + let name_bucket = format!("recover_name_fail:{ip}:{name_key}"); + let name_ceiling = if rate_limits_on && recover_rate_on { + Some( + config::get_usize( + &state.config_cache, + "recover_name_rate_per_15min", + RECOVER_NAME_CEILING_DEFAULT, + ) + .await + // CLAMPED, not merely defaulted — see RECOVER_NAME_CEILING_MAX. The config value is + // operator-settable and the invariant it has to respect is not expressible in + // `patch_config`'s numeric range, so it is enforced at the point of use. + .min(RECOVER_NAME_CEILING_MAX), + ) + } else { + None + }; + let name_budget_spent: Option = name_ceiling.and_then(|max| { + state + .rate_limiter + .peek(&name_bucket, max, RECOVER_NAME_WINDOW) + .err() + }); + // Charged on the way OUT of a failure, exactly like `charge_recover_failure`. + let charge_name_failure = || { + if let Some(max) = name_ceiling { + let _ = + state + .rate_limiter + .check_with_retry(name_bucket.clone(), max, RECOVER_NAME_WINDOW); + } + }; + let name_refusal = |retry_after_secs: u64| { + AppError::TooManyRequests( + "Zu viele fehlgeschlagene Versuche für diesen Namen. Bitte warte 15 Minuten.".into(), + Some(retry_after_secs), + ) + }; + // The cross-name tier (H2). Read-only here — budget is spent only by an actual wrong PIN // below — so a venue full of guests recovering their own devices never trips it, while a // horizontal sweep across the public name list runs out after RECOVER_IP_FAILURE_CEILING. @@ -667,6 +749,10 @@ pub async fn recover( // Charged here too, or the cheapest sweep (guessing names that don't exist) would be free // — and the whole point of the tier is that guessing costs the guesser something. charge_recover_failure(&state, &ip); + charge_name_failure(); + if let Some(retry_after_secs) = name_budget_spent { + return Err(name_refusal(retry_after_secs)); + } if let Some(retry_after_secs) = ip_budget_spent { return Err(sweep_refusal(retry_after_secs)); } @@ -721,6 +807,7 @@ pub async fn recover( // Wrong PIN — charge both the per-account counter and this IP's cross-name budget. The // account counter alone never fires against a sweep that only spends 4 guesses per name. charge_recover_failure(&state, &ip); + charge_name_failure(); let attempts = User::increment_failed_pin(&state.pool, user.id).await?; tracing::warn!( user_id = %user.id, @@ -741,6 +828,9 @@ pub async fn recover( } } + if let Some(retry_after_secs) = name_budget_spent { + return Err(name_refusal(retry_after_secs)); + } if let Some(retry_after_secs) = ip_budget_spent { return Err(sweep_refusal(retry_after_secs)); } @@ -761,18 +851,32 @@ pub struct AdminLoginResponse { pub display_name: String, } -/// Requests per minute per IP that may reach `verify_password` at all. +/// Requests per minute per IP after which admin login is served in DEGRADED mode. /// /// Not a security control — the failure bucket below is. It bounds how deep a queue can form on /// `BCRYPT_PERMITS`, which is what actually caps the CPU cost. /// -/// Still far above anything a person typing a password produces, but note the honest limitation: -/// unlike the failure bucket, this ceiling CAN refuse a correct password, and on venue NAT every -/// guest shares the operator's IP. It is a smaller number than it first was for exactly that -/// reason — the earlier 120 was chosen when this was the only bound on bcrypt, which made it both -/// too weak to cap CPU and too coarse to be safe for the operator. +/// This used to REFUSE above the ceiling, and that made it a denial of service against the one +/// person who cannot route around it. `/admin/login` is a public linkable page, every guest at the +/// venue shares the operator's IP behind NAT, and the check ran before `verify_password` — so one +/// phone posting twice a minute kept the bucket full and the operator, on that same IP, was +/// refused WITH THE CORRECT PASSWORD. The escape hatch was circular: `admin_login_rate_enabled` +/// is only flippable through `PATCH /admin/config`, which needs the session being refused. That +/// locked out moderation, gallery release and every config key — including the ones that would +/// undo it. +/// +/// So exceeding it no longer refuses; it shortens the hash-permit wait to +/// [`ADMIN_LOGIN_DEGRADED_ACQUIRE`]. The CPU bound is unchanged, because the CPU bound was always +/// the semaphore and never this bucket: a flood now sheds itself on a busy semaphore, while the +/// operator's single well-timed request still gets a permit and a truthful answer. const ADMIN_LOGIN_CPU_CEILING: usize = 30; +/// How long an over-budget admin login will queue for a hash permit before shedding with 503. +/// +/// Short enough that a flood cannot build a queue that starves `/join`, long enough that a +/// request arriving while two permits are mid-hash (~250 ms each) still waits its turn. +const ADMIN_LOGIN_DEGRADED_ACQUIRE: Duration = Duration::from_secs(1); + pub async fn admin_login( State(state): State, ConnectInfo(peer): ConnectInfo, @@ -800,26 +904,35 @@ pub async fn admin_login( let admin_rate_on = config::get_bool(&state.config_cache, "admin_login_rate_enabled", true).await; - // Part 1: a deliberately GENEROUS ceiling whose only job is to bound bcrypt CPU. Cost-12 - // verification is ~250 ms of a core, so an unbounded endpoint is a CPU exhaustion vector - // regardless of whether anyone guesses right. No human typing a password reaches this. - if rate_limits_on + // Part 1: a ceiling whose only job is to bound how deep a queue can form on the hash permits. + // Cost-12 verification is ~250 ms of a core, so an endpoint with no bound at all lets a flood + // build a backlog that starves `/join`. Exceeding it DEGRADES rather than refuses — see + // ADMIN_LOGIN_CPU_CEILING for why refusing here was a DoS against the operator. + let degraded = rate_limits_on && admin_rate_on - && let Err(retry_after_secs) = state.rate_limiter.check_with_retry( - format!("admin_login_cpu:{ip}"), - ADMIN_LOGIN_CPU_CEILING, - Duration::from_secs(60), - ) - { - return Err(AppError::TooManyRequests( - "Zu viele Anmeldeversuche. Bitte warte kurz und versuche es erneut.".into(), - Some(retry_after_secs), - )); + && state + .rate_limiter + .check_with_retry( + format!("admin_login_cpu:{ip}"), + ADMIN_LOGIN_CPU_CEILING, + Duration::from_secs(60), + ) + .is_err(); + if degraded { + tracing::warn!( + ip = %ip, + "admin login over its per-IP volume ceiling; serving with a shortened hash-permit wait" + ); } - let valid = verify_password( + let valid = verify_password_within( body.password.clone(), state.config.admin_password_hash.clone(), + if degraded { + ADMIN_LOGIN_DEGRADED_ACQUIRE + } else { + BCRYPT_ACQUIRE_TIMEOUT + }, ) .await?; diff --git a/backend/src/handlers/test_admin.rs b/backend/src/handlers/test_admin.rs index 0be9770..311707f 100644 --- a/backend/src/handlers/test_admin.rs +++ b/backend/src/handlers/test_admin.rs @@ -61,7 +61,7 @@ pub async fn truncate_all( ('upload_rate_per_hour', '100'), ('feed_rate_per_min', '60'), ('export_rate_per_day', '3'), - ('join_ip_rate_per_min', '60'), + ('join_ip_rate_per_min', '300'), ('recover_ip_rate_per_min', '30'), ('social_rate_per_min', '120'), ('quota_tolerance', '0.75'), diff --git a/backend/src/services/config.rs b/backend/src/services/config.rs index 1894459..f309b8e 100644 --- a/backend/src/services/config.rs +++ b/backend/src/services/config.rs @@ -148,3 +148,104 @@ pub async fn get_bool(cache: &ConfigCache, key: &str, default: bool) -> bool { _ => default, } } + +#[cfg(test)] +mod seed_tests { + /// The value a fresh database actually ends up with for `key`, by replaying the migrations. + /// + /// This exists because a `config::get_*` default is only a fallback for a MISSING key, and the + /// migrations seed nearly every key there is. So the literal in the handler is dead code on any + /// real install, and changing it changes nothing — which is exactly what happened to + /// `join_ip_rate_per_min`: it was raised 60 → 300 in `auth/handlers.rs` to stop one QR-code + /// burst from locking the venue out of `/join`, shipped, and did nothing at all, because + /// migration 017 seeds 60 and the seed wins. Nothing in the test suite could see it: the e2e + /// regression guard fires 12 concurrent joins, which is green at 60 and at 300 alike. + fn effective_seed(key: &str) -> Option { + let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("migrations"); + let mut files: Vec<_> = std::fs::read_dir(&dir) + .expect("migrations directory") + .filter_map(|e| e.ok().map(|e| e.path())) + .filter(|p| p.to_string_lossy().ends_with(".up.sql")) + .collect(); + // Version order: migrations are applied in filename order and later ones override. + files.sort(); + + let mut value: Option = None; + for path in files { + let sql = std::fs::read_to_string(&path).expect("readable migration"); + for line in sql.lines() { + let line = line.trim(); + if line.starts_with("--") { + continue; + } + // Seed form: ('key', 'value') + if let Some(rest) = line.strip_prefix(&format!("('{key}',")) + && let Some(v) = rest.split('\'').nth(1) + { + value = Some(v.to_string()); + } + // Update form: UPDATE config SET value = 'new' WHERE key = 'key' AND value = 'old' + if line.starts_with("UPDATE config SET value") + && line.contains(&format!("key = '{key}'")) + && let Some(new) = line.split('\'').nth(1) + { + let scoped_to = line + .rsplit_once("AND value = '") + .and_then(|(_, tail)| tail.split('\'').next().map(|s| s.to_string())); + // Only applies if the current value still matches the scope it was written for. + if scoped_to.is_none() || scoped_to.as_deref() == value.as_deref() { + value = Some(new.to_string()); + } + } + } + } + value + } + + /// The invariant, not the number: `join_ip:{ip}` is keyed on an address the WHOLE VENUE shares + /// behind NAT, and `/join` is the one screen with no auto-retry. A ceiling near the size of the + /// party is a ceiling on the party. Asserted against the effective seed rather than the code + /// default precisely because the code default is what silently did not apply. + #[test] + fn the_join_ceiling_a_real_install_gets_is_sized_for_a_whole_venue_arriving_at_once() { + let seeded = effective_seed("join_ip_rate_per_min") + .expect("join_ip_rate_per_min must be seeded by a migration"); + let seeded: usize = seeded.parse().expect("numeric"); + assert!( + seeded >= 300, + "a fresh database ends up with join_ip_rate_per_min = {seeded}. Every guest shares one \ + NAT address, so this is the ceiling for the entire party scanning one QR code. Raise \ + it with a value-scoped UPDATE migration (see 030) — changing the default in \ + auth/handlers.rs does nothing, because the seed wins." + ); + } + + /// Pins the other half of the same trap: the seeded value must not exceed the ceiling the + /// handler clamps to, or an operator reading `GET /admin/config` sees a number that is not the + /// one being enforced. + #[test] + fn the_seeded_recover_name_ceiling_is_within_what_the_handler_will_honour() { + let seeded = effective_seed("recover_name_rate_per_15min") + .expect("recover_name_rate_per_15min must be seeded by a migration"); + let seeded: usize = seeded.parse().expect("numeric"); + assert!( + seeded <= crate::auth::handlers::RECOVER_NAME_CEILING_MAX, + "seeded recover_name_rate_per_15min = {seeded} exceeds RECOVER_NAME_CEILING_MAX = {}; \ + the handler clamps at the point of use, so the advertised value would be a lie.", + crate::auth::handlers::RECOVER_NAME_CEILING_MAX + ); + } + + /// The parser itself, against a value migration 015 really does change. Without this a bug in + /// `effective_seed` makes both tests above vacuously green. + #[test] + fn the_seed_parser_follows_a_value_through_a_later_update_migration() { + assert_eq!( + effective_seed("upload_rate_per_hour").as_deref(), + Some("100"), + "005 seeds 10 and 015 raises it to 100; reading 10 here means the UPDATE form is not \ + being applied, and every assertion built on this helper is worthless." + ); + assert_eq!(effective_seed("no_such_key_anywhere"), None); + } +} diff --git a/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts index 39dbde6..860a83f 100644 --- a/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts +++ b/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts @@ -296,5 +296,22 @@ test.describe('Rate limits — /recover name cycling', () => { const codes: number[] = []; for (let i = 0; i < 7; i++) codes.push((await attempt()).status); expect(codes.at(-1), 'guessing one name must still be throttled').toBe(429); + + // AND THE OTHER HALF, which is the whole reason this bucket counts failures instead of + // requests: the victim's own CORRECT PIN must still let them in, from the same IP, while that + // budget is spent. Behind the venue's NAT the "attacker" and the victim are the same address, + // so a bucket that refused before verifying handed any guest a fifteen-minute lockout of any + // named person — the host included, whose only credential is a 4-digit PIN and whose only way + // back after losing a session is this endpoint. Four wrong guesses did it, and four more every + // fifteen minutes sustained it indefinitely. + const rightful = await fetch(`${BASE}/api/v1/recover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ display_name: victim.displayName, pin: victim.pin }), + }); + expect( + rightful.status, + 'a correct PIN must authenticate even when this IP has spent the name budget' + ).toBe(200); }); });