From 45d49c64b33212d5a548311600d69e5eaf36b8ce Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 09:02:36 +0000 Subject: [PATCH] Device login: never wait for a poll's row lock, and rate limit the token endpoint A poll of a pending device code took a pool connection and then waited for the request's row lock (SELECT ... FOR UPDATE), so concurrent polls of one code, which anyone can send without an account, held up the whole pool and every database-backed request behind it. - lock_for_poll locks with FOR UPDATE NOWAIT. A poll that finds the row held (lock_not_available) answers slow_down at once, without recording a poll or waiting with a connection. - POST /auth/device/token is rate limited per instance before the database is asked: per client network, auth.device_polls_per_minute (600; 429 slow_down with Retry-After), and per device code, ten polls per ten seconds (slow_down). A KeyedWindowLimiter shares WindowLimiter's windows; a map full of live windows lets an untracked code through, so filling it cannot refuse everyone else's polls. The test sends 50 concurrent polls of one pending code to a server on a two-connection pool, also while another transaction holds the row: each is answered within a second, and an unrelated request is served. Fixes #819 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012e6DJZVgk6pyRx2Dk7AHqN --- ...01d608f6220dca563034db86142c75332425.json} | 4 +- crates/core/src/auth/login_throttle.rs | 102 ++++++++-- crates/core/src/auth/mod.rs | 4 +- crates/db/src/device_auth_dao.rs | 71 ++++--- crates/server/src/auth/device.rs | 160 ++++++++++++++-- crates/server/src/auth/mod.rs | 3 +- crates/server/src/config.rs | 14 ++ crates/server/src/state.rs | 7 + crates/server/tests/common/mod.rs | 17 ++ crates/server/tests/device_auth.rs | 178 +++++++++++++++++- docs/changes/2.2.0/649.feature.md | 5 +- docs/deployment.md | 23 ++- forge.toml.example | 5 + 13 files changed, 533 insertions(+), 60 deletions(-) rename .sqlx/{query-d308dc16d28a0e5a6629c141569ac745dc25768f6de2bc8c0b424993e3397dc8.json => query-561cc809dfe2e381d20b3cd0949601d608f6220dca563034db86142c75332425.json} (95%) diff --git a/.sqlx/query-d308dc16d28a0e5a6629c141569ac745dc25768f6de2bc8c0b424993e3397dc8.json b/.sqlx/query-561cc809dfe2e381d20b3cd0949601d608f6220dca563034db86142c75332425.json similarity index 95% rename from .sqlx/query-d308dc16d28a0e5a6629c141569ac745dc25768f6de2bc8c0b424993e3397dc8.json rename to .sqlx/query-561cc809dfe2e381d20b3cd0949601d608f6220dca563034db86142c75332425.json index f6c995d9..50ec6327 100644 --- a/.sqlx/query-d308dc16d28a0e5a6629c141569ac745dc25768f6de2bc8c0b424993e3397dc8.json +++ b/.sqlx/query-561cc809dfe2e381d20b3cd0949601d608f6220dca563034db86142c75332425.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT id, client_id, scope, status, user_id, interval_seconds,\n expires_at <= now() AS \"expired!\",\n COALESCE(\n last_polled_at > now() - make_interval(secs => interval_seconds),\n false\n ) AS \"too_fast!\"\n FROM device_authorization\n WHERE device_code_hash = $1\n FOR UPDATE\n ", + "query": "\n SELECT id, client_id, scope, status, user_id, interval_seconds,\n expires_at <= now() AS \"expired!\",\n COALESCE(\n last_polled_at > now() - make_interval(secs => interval_seconds),\n false\n ) AS \"too_fast!\"\n FROM device_authorization\n WHERE device_code_hash = $1\n FOR UPDATE NOWAIT\n ", "describe": { "columns": [ { @@ -98,5 +98,5 @@ null ] }, - "hash": "d308dc16d28a0e5a6629c141569ac745dc25768f6de2bc8c0b424993e3397dc8" + "hash": "561cc809dfe2e381d20b3cd0949601d608f6220dca563034db86142c75332425" } diff --git a/crates/core/src/auth/login_throttle.rs b/crates/core/src/auth/login_throttle.rs index 14c2a49e..f87df780 100644 --- a/crates/core/src/auth/login_throttle.rs +++ b/crates/core/src/auth/login_throttle.rs @@ -65,6 +65,7 @@ use std::borrow::Cow; use std::collections::HashMap; +use std::hash::Hash; use std::net::IpAddr; use std::sync::{Arc, Mutex, MutexGuard, RwLock}; use std::time::{Duration, Instant}; @@ -789,14 +790,58 @@ impl Default for LoginThrottle { /// full of live windows refuses networks it does not track yet. #[derive(Debug)] pub struct WindowLimiter { - max: u32, - window: Duration, - state: Mutex>, + windows: Windows, } impl WindowLimiter { /// Allow `max` calls per `window` per network; `0` disables the limit. pub fn new(max: u32, window: Duration) -> Self { + Self { + windows: Windows::new(max, window), + } + } + + /// Count a call from `ip`; `false` when its network is over the limit. + pub fn allow(&self, ip: IpAddr) -> bool { + self.windows.allow(client_network(ip)).unwrap_or(false) + } +} + +/// A fixed-window rate limit per key of the caller's choosing (e.g. a secret +/// the call names), next to a [`WindowLimiter`] that bounds the caller's +/// network. Bounded memory like it, but a map still full of live windows +/// lets a key it does not track yet through uncounted: whoever fills the map +/// would otherwise have every other key refused, and the network's limit +/// still applies. +#[derive(Debug)] +pub struct KeyedWindowLimiter { + windows: Windows, +} + +impl KeyedWindowLimiter { + /// Allow `max` calls per `window` per key; `0` disables the limit. + pub fn new(max: u32, window: Duration) -> Self { + Self { + windows: Windows::new(max, window), + } + } + + /// Count a call under `key`; `false` when the key is over the limit. + pub fn allow(&self, key: K) -> bool { + self.windows.allow(key).unwrap_or(true) + } +} + +/// The fixed windows of a [`WindowLimiter`] or [`KeyedWindowLimiter`]. +#[derive(Debug)] +struct Windows { + max: u32, + window: Duration, + state: Mutex>, +} + +impl Windows { + fn new(max: u32, window: Duration) -> Self { Self { max, window, @@ -804,31 +849,32 @@ impl WindowLimiter { } } - /// Count a call from `ip`; `false` when its network is over the limit. - pub fn allow(&self, ip: IpAddr) -> bool { + /// Count a call under `key`: whether it is within the limit, `None` when + /// the key is not tracked and the map is full of live windows (nothing + /// counted). + fn allow(&self, key: K) -> Option { if self.max == 0 { - return true; + return Some(true); } let now = Instant::now(); - let net = client_network(ip); let mut state = self.state.lock().expect("window limiter mutex poisoned"); - if !state.contains_key(&net) && state.len() >= MAX_TRACKED { + if !state.contains_key(&key) && state.len() >= MAX_TRACKED { let window = self.window; state.retain(|_, (start, _)| now.duration_since(*start) < window); if state.len() >= MAX_TRACKED { - return false; + return None; } } - let (start, count) = state.entry(net).or_insert((now, 0)); + let (start, count) = state.entry(key).or_insert((now, 0)); if now.duration_since(*start) >= self.window { *start = now; *count = 0; } if *count >= self.max { - return false; + return Some(false); } *count += 1; - true + Some(true) } } @@ -1092,6 +1138,38 @@ mod tests { assert!(WindowLimiter::new(0, Duration::ZERO).allow(ip("192.0.2.1"))); } + #[test] + fn keyed_window_limiter_counts_per_key_and_window() { + let l = KeyedWindowLimiter::new(2, Duration::from_millis(50)); + assert!(l.allow("a")); + assert!(l.allow("a")); + assert!(!l.allow("a"), "over the limit"); + assert!(l.allow("b"), "another key has its own budget"); + std::thread::sleep(Duration::from_millis(60)); + assert!(l.allow("a"), "a new window"); + assert!(KeyedWindowLimiter::new(0, Duration::ZERO).allow("a")); + } + + #[test] + fn a_full_map_refuses_new_networks_but_lets_new_keys_through() { + let networks = WindowLimiter::new(1, Duration::from_secs(60)); + let keys = KeyedWindowLimiter::new(1, Duration::from_secs(60)); + for n in 0..MAX_TRACKED as u32 { + assert!(networks.allow(IpAddr::from(n.to_be_bytes()))); + assert!(keys.allow(n)); + } + let new = MAX_TRACKED as u32; + assert!( + !networks.allow(IpAddr::from(new.to_be_bytes())), + "a network the full map has no room for is refused" + ); + assert!( + keys.allow(new) && keys.allow(new), + "a key the full map has no room for is let through, uncounted" + ); + assert!(!keys.allow(0), "a tracked key keeps its limit"); + } + #[tokio::test] async fn success_clears_the_pair() { let t = LoginThrottle::new(2, Duration::from_secs(60)); diff --git a/crates/core/src/auth/mod.rs b/crates/core/src/auth/mod.rs index 48d76fa5..71a75a0e 100644 --- a/crates/core/src/auth/mod.rs +++ b/crates/core/src/auth/mod.rs @@ -49,8 +49,8 @@ pub use cred_cache::{ }; pub use local_password::{LimiterSaturated, LocalPasswordProvider, PasswordHashLimiter}; pub use login_throttle::{ - client_network, username_for_log, AttemptRefused, KnownNetworkStore, LoginAttempt, - LoginThrottle, PgKnownNetworks, WindowLimiter, MAX_USERNAME_BYTES, + client_network, username_for_log, AttemptRefused, KeyedWindowLimiter, KnownNetworkStore, + LoginAttempt, LoginThrottle, PgKnownNetworks, WindowLimiter, MAX_USERNAME_BYTES, }; pub use principal::{AuthMethod, Principal, TrustedPublisherMatch, UserPrincipal}; pub use provider::{ diff --git a/crates/db/src/device_auth_dao.rs b/crates/db/src/device_auth_dao.rs index b0f6b90a..6169b7b2 100644 --- a/crates/db/src/device_auth_dao.rs +++ b/crates/db/src/device_auth_dao.rs @@ -125,13 +125,30 @@ pub struct PolledRequest { pub too_fast: bool, } +/// What a poll found when it locked its request ([`lock_for_poll`]). +#[derive(Debug, Clone)] +pub enum PollLock { + /// The request, locked for the poll's transaction. + Locked(PolledRequest), + /// Another transaction holds the request's row: another poll of the same + /// device code, mostly. Nothing was read, and the transaction is aborted. + Busy, + /// No request has the device code. + Unknown, +} + /// Find and lock the request whose device code hashes to `device_code_hash` -/// in the transaction on `conn` (`SELECT … FOR UPDATE`, so two polls of one -/// code are answered one after the other). The clock is the database's. +/// in the transaction on `conn` (`SELECT … FOR UPDATE`, so the polls of one +/// code are answered one at a time). The clock is the database's. +/// +/// `NOWAIT`: a row another transaction holds is [`PollLock::Busy`] at once, +/// never waited for, so no poll keeps a connection of the pool while it waits +/// for another's (#819). The error aborts the transaction on `conn`: the +/// caller drops it. pub async fn lock_for_poll( conn: &mut PgConnection, device_code_hash: &str, -) -> sqlx::Result> { +) -> sqlx::Result { let row = sqlx::query!( r#" SELECT id, client_id, scope, status, user_id, interval_seconds, @@ -142,32 +159,42 @@ pub async fn lock_for_poll( ) AS "too_fast!" FROM device_authorization WHERE device_code_hash = $1 - FOR UPDATE + FOR UPDATE NOWAIT "#, device_code_hash, ) .fetch_optional(&mut *conn) - .await?; - Ok(row.map(|row| { - let state = match (row.status.as_str(), row.user_id) { - _ if row.expired => PollState::Expired, - ("pending", _) => PollState::Pending, - ("approved", Some(user_id)) => PollState::Approved { user_id }, - ("denied", _) => PollState::Denied, - // `collected`, or an approval without its user (deleted). - _ => PollState::Expired, - }; - PolledRequest { - id: row.id, - client_id: row.client_id, - scope: row.scope, - state, - interval_seconds: row.interval_seconds, - too_fast: row.too_fast, - } + .await; + let row = match row { + Ok(Some(row)) => row, + Ok(None) => return Ok(PollLock::Unknown), + Err(e) if is_lock_not_available(&e) => return Ok(PollLock::Busy), + Err(e) => return Err(e), + }; + let state = match (row.status.as_str(), row.user_id) { + _ if row.expired => PollState::Expired, + ("pending", _) => PollState::Pending, + ("approved", Some(user_id)) => PollState::Approved { user_id }, + ("denied", _) => PollState::Denied, + // `collected`, or an approval without its user (deleted). + _ => PollState::Expired, + }; + Ok(PollLock::Locked(PolledRequest { + id: row.id, + client_id: row.client_id, + scope: row.scope, + state, + interval_seconds: row.interval_seconds, + too_fast: row.too_fast, })) } +/// Whether `e` is PostgreSQL's `lock_not_available` (`55P03`): a `NOWAIT` +/// lock found its row held. +fn is_lock_not_available(e: &sqlx::Error) -> bool { + matches!(e, sqlx::Error::Database(db) if db.code().as_deref() == Some("55P03")) +} + /// Record a poll of the pending request `id` now, with the interval the /// client must keep from now on. pub async fn record_poll( diff --git a/crates/server/src/auth/device.rs b/crates/server/src/auth/device.rs index bb571022..cb8a69bc 100644 --- a/crates/server/src/auth/device.rs +++ b/crates/server/src/auth/device.rs @@ -13,7 +13,10 @@ //! `authorization_pending` until the user decides, `slow_down` (and five //! more seconds of interval) when polled sooner than the interval, //! `access_denied`, `expired_token`, or, once, the token. Errors are RFC -//! 6749 §5.2 JSON. +//! 6749 §5.2 JSON. Anyone may poll, without an account, so polls are +//! limited per client network and per device code before the database is +//! asked ([`DevicePollLimits`]), and a poll never waits for another poll's +//! lock on the request ([`device_auth_dao::lock_for_poll`], #819). //! * `GET /.well-known/oauth-authorization-server` (RFC 8414) names both //! endpoints, so a client finds everything from the server URL alone. //! * `POST /auth/device/lookup` and `/auth/device/decide` serve the `/device` @@ -45,6 +48,7 @@ use std::collections::{BTreeSet, HashMap}; use std::fmt; use std::net::{IpAddr, Ipv6Addr, SocketAddr}; +use std::time::Duration; use axum::body::Bytes; use axum::extract::rejection::BytesRejection; @@ -55,10 +59,12 @@ use axum::routing::{get, post}; use axum::{Extension, Router}; use chrono::{DateTime, Utc}; use forge_core::auth::token::{generate_token_secret, hash_token}; -use forge_core::auth::{AttemptRefused, LoginAttempt, LoginThrottle}; +use forge_core::auth::{ + AttemptRefused, KeyedWindowLimiter, LoginAttempt, LoginThrottle, WindowLimiter, +}; use forge_core::authz::{Level, TokenScope}; use forge_db::device_auth_dao::{ - self, DeviceAuthorization, NewDeviceAuthorization, PollState, PolledRequest, + self, DeviceAuthorization, NewDeviceAuthorization, PollLock, PollState, PolledRequest, }; use forge_db::{repository_dao, token_dao, user_dao}; use serde::{Deserialize, Serialize}; @@ -109,6 +115,13 @@ const SLOW_DOWN_STEP_SECONDS: i32 = 5; /// The interval `slow_down` raises a request's to at most. const MAX_INTERVAL_SECONDS: i32 = 3600; +/// Polls of one device code an instance answers per [`CODE_POLL_WINDOW`]: +/// one a second, the shortest interval there is, in bursts of up to ten. +const POLLS_PER_CODE: u32 = 10; + +/// The window [`POLLS_PER_CODE`] is counted in. +const CODE_POLL_WINDOW: Duration = Duration::from_secs(10); + /// The largest request body of these endpoints (a form or a JSON object of /// a few short fields). const MAX_BODY_BYTES: usize = 8 * 1024; @@ -522,6 +535,7 @@ struct TokenResponse { /// `POST /auth/device/token` — a client polls its device login. async fn device_token( State(state): State, + connect: Option>>, headers: HeaderMap, body: Result, ) -> Response { @@ -550,22 +564,88 @@ async fn device_token( let Some(device_code) = params.get("device_code") else { return bad_request("invalid_request", "device_code is required"); }; - poll(&state, client_id, device_code).await + let client_ip = login_client_ip(&state, connect.map(|Extension(ci)| ci.0), &headers); + let device_code_hash = hash_token(device_code); + if let Err(resp) = state.device_polls.check(client_ip, &device_code_hash) { + return resp; + } + poll(&state, client_id, &device_code_hash).await +} + +/// The rate limits of the token endpoint, counted by each instance (#819): +/// anyone may poll, without an account, and a device code is free to have. +/// Polls are limited per client network (`auth.device_polls_per_minute`, +/// IPv6 by /64) and per device code ([`POLLS_PER_CODE`] per +/// [`CODE_POLL_WINDOW`]), before the database is asked. +#[derive(Debug)] +pub struct DevicePollLimits { + networks: WindowLimiter, + codes: KeyedWindowLimiter, } -/// Answer a poll of `device_code` by `client_id`. The request's row is -/// locked for the answer, so two polls of one code are answered one after -/// the other, and the token is minted in the transaction that marks the -/// request collected: it is answered once, and a failure leaves the -/// approval to collect on the next poll. -async fn poll(state: &AppState, client_id: &str, device_code: &str) -> Response { +impl DevicePollLimits { + /// `per_network_per_minute` polls per client network and minute (`0`: + /// no limit per network), and [`POLLS_PER_CODE`] per device code. + pub fn new(per_network_per_minute: u32) -> Self { + DevicePollLimits { + networks: WindowLimiter::new(per_network_per_minute, Duration::from_secs(60)), + codes: KeyedWindowLimiter::new(POLLS_PER_CODE, CODE_POLL_WINDOW), + } + } + + /// Count a poll from `client_ip` of the device code that hashes to + /// `device_code_hash`. The refusal when the network (`429`) or the code + /// (`400`) is over its limit; both are `slow_down`. + fn check(&self, client_ip: IpAddr, device_code_hash: &str) -> Result<(), Response> { + if !self.networks.allow(client_ip) { + let mut resp = oauth_error( + StatusCode::TOO_MANY_REQUESTS, + "slow_down", + "too many polls from this network; try again later", + ); + resp.headers_mut() + .insert(header::RETRY_AFTER, header::HeaderValue::from_static("60")); + return Err(resp); + } + if !self.codes.allow(code_key(device_code_hash)) { + return Err(bad_request( + "slow_down", + "the device_code is polled too often; wait five seconds longer from now on", + )); + } + Ok(()) + } +} + +/// The key a device code's polls are counted under: the first 64 bits of +/// its SHA-256 (hex), so an entry has a fixed size whatever is sent. Codes +/// that share it share a budget, which nobody can aim at a code they do not +/// know. +fn code_key(device_code_hash: &str) -> u64 { + device_code_hash + .get(..16) + .and_then(|hex| u64::from_str_radix(hex, 16).ok()) + .unwrap_or_default() +} + +/// Answer a poll by `client_id` of the device code that hashes to +/// `device_code_hash`. The request's row is locked for the answer, so the +/// polls of one code are answered one at a time, and the token is minted in +/// the transaction that marks the request collected: it is answered once, +/// and a failure leaves the approval to collect on the next poll. A poll +/// that finds the row locked by another is answered `slow_down` at once: it +/// never waits for the lock while it holds a connection of the pool +/// (#819). +async fn poll(state: &AppState, client_id: &str, device_code_hash: &str) -> Response { let mut tx = match forge_db::begin(&state.pool).await { Ok(tx) => tx, Err(e) => return oauth_unavailable(&e), }; - let request = match device_auth_dao::lock_for_poll(&mut tx, &hash_token(device_code)).await { - Ok(Some(request)) => request, - Ok(None) => return bad_request("invalid_grant", "unknown device_code"), + let request = match device_auth_dao::lock_for_poll(&mut tx, device_code_hash).await { + Ok(PollLock::Locked(request)) => request, + // The transaction is aborted; dropping it rolls it back. + Ok(PollLock::Busy) => return polled_at_once(), + Ok(PollLock::Unknown) => return bad_request("invalid_grant", "unknown device_code"), Err(e) => return oauth_unavailable(&e), }; if request.client_id != client_id { @@ -610,6 +690,14 @@ async fn poll(state: &AppState, client_id: &str, device_code: &str) -> Response } } +/// `slow_down`: another poll of the same device code holds its request. +fn polled_at_once() -> Response { + bad_request( + "slow_down", + "another poll of the device_code is being answered; wait five seconds longer from now on", + ) +} + /// Mint the token of an approved request, in its poll's transaction `tx`. async fn collect( state: &AppState, @@ -1099,6 +1187,52 @@ mod tests { assert!(form("application/x-www-form-urlencoded", "a=1&a=2").is_err()); } + #[test] + fn a_device_code_is_counted_under_its_hash() { + let hash = hash_token("a device code"); + assert_eq!( + code_key(&hash), + u64::from_str_radix(&hash[..16], 16).unwrap() + ); + assert_ne!( + code_key(&hash), + code_key(&hash_token("another device code")) + ); + assert_eq!(code_key("short"), 0); + } + + #[test] + fn polls_are_limited_per_network_and_per_code() { + let limits = DevicePollLimits::new(15); + let ip: IpAddr = "203.0.113.5".parse().unwrap(); + let (one, two) = (hash_token("one"), hash_token("two")); + for _ in 0..POLLS_PER_CODE { + assert!(limits.check(ip, &one).is_ok()); + } + let refused = limits.check(ip, &one).unwrap_err(); + assert_eq!( + refused.status(), + StatusCode::BAD_REQUEST, + "the code's limit" + ); + for _ in 0..4 { + assert!(limits.check(ip, &two).is_ok(), "another code's own budget"); + } + let refused = limits.check(ip, &two).unwrap_err(); + assert_eq!( + refused.status(), + StatusCode::TOO_MANY_REQUESTS, + "the network's limit" + ); + assert!(refused.headers().contains_key(header::RETRY_AFTER)); + let other: IpAddr = "198.51.100.5".parse().unwrap(); + assert!(limits.check(other, &two).is_ok(), "another network"); + assert!( + limits.check(other, &one).is_err(), + "the code's limit, from any network" + ); + } + #[test] fn tokens_are_named_after_the_host() { assert_eq!(host_of("https://forge.example.com"), "forge.example.com"); diff --git a/crates/server/src/auth/mod.rs b/crates/server/src/auth/mod.rs index 8ec36a54..4328d2d1 100644 --- a/crates/server/src/auth/mod.rs +++ b/crates/server/src/auth/mod.rs @@ -77,7 +77,8 @@ mod trusted_header; mod wif; pub use device::{ - AUTHORIZATION_SERVER_METADATA_PATH, DEVICE_CODE_GRANT, DEVICE_CODE_PATH, DEVICE_TOKEN_PATH, + DevicePollLimits, AUTHORIZATION_SERVER_METADATA_PATH, DEVICE_CODE_GRANT, DEVICE_CODE_PATH, + DEVICE_TOKEN_PATH, }; pub use issuer::{ issuer_router, spawn_issuer_refresh, AssertionIssuer, DISCOVERY_PATH, ISSUER_KEYS_SETTING, diff --git a/crates/server/src/config.rs b/crates/server/src/config.rs index 37970d48..e00a0251 100644 --- a/crates/server/src/config.rs +++ b/crates/server/src/config.rs @@ -605,6 +605,14 @@ pub struct AuthConfig { /// row. `0` disables the limit. #[serde(default = "default_oidc_login_starts_per_minute")] pub oidc_login_starts_per_minute: u32, + /// Polls of the device login's token endpoint (`POST /auth/device/token`) + /// allowed per client network (IPv6: per /64) and minute, per instance. + /// Anyone may poll, without an account; a poll past the limit is answered + /// `429` (`slow_down`) without asking the database. Each device code is + /// also held to ten polls per ten seconds. `0` disables the limit per + /// network. + #[serde(default = "default_device_polls_per_minute")] + pub device_polls_per_minute: u32, /// Maximum concurrent upstream credential forwards **per /// credential-forwarding realm, per instance** (verdict-cache misses /// only; cache hits never forward). A miss beyond the bound fails closed @@ -697,6 +705,12 @@ fn default_oidc_login_starts_per_minute() -> u32 { 30 } +/// Ten device logins polling at once from one network at the shortest +/// interval (1 s), or fifty at the default one (5 s). +fn default_device_polls_per_minute() -> u32 { + 600 +} + fn default_realm_cache_ttl() -> u64 { 10 } diff --git a/crates/server/src/state.rs b/crates/server/src/state.rs index 50efd715..70e77e03 100644 --- a/crates/server/src/state.rs +++ b/crates/server/src/state.rs @@ -90,6 +90,9 @@ pub struct AppState { /// Rate limit on starting OIDC logins, per client network /// (`auth.oidc_login_starts_per_minute`). pub oidc_login_starts: Arc, + /// Rate limits on polling a device login (`POST /auth/device/token`), per + /// client network (`auth.device_polls_per_minute`) and per device code. + pub device_polls: Arc, /// Secret encryption at rest: seals and opens every secret Forge keeps. /// Built fallibly by [`crate::crypto::build_envelope`] **before** /// [`AppState::new`] (a missing or broken root key must abort startup). @@ -367,6 +370,9 @@ impl AppState { config.auth.oidc_login_starts_per_minute, std::time::Duration::from_secs(60), )); + let device_polls = Arc::new(crate::auth::DevicePollLimits::new( + config.auth.device_polls_per_minute, + )); let (pool_for_mailer, config_for_mailer) = (pool.clone(), config.clone()); let instance = Arc::new(Instance::current()); @@ -395,6 +401,7 @@ impl AppState { invitation_throttle, device_code_throttle, oidc_login_starts, + device_polls, upstream, registry_keys: crate::images::token::TokenKeys::new(vec![{ use rand::Rng; diff --git a/crates/server/tests/common/mod.rs b/crates/server/tests/common/mod.rs index fe0209a2..ef4f33f5 100644 --- a/crates/server/tests/common/mod.rs +++ b/crates/server/tests/common/mod.rs @@ -126,6 +126,23 @@ pub async fn pool() -> PgPool { pool } +/// A pool on the shared database with at most `max_connections` connections, +/// which gives up on a connection after `acquire_timeout`: for a server +/// whose pool a test means to run short (`database.max_connections`). +pub async fn small_pool(max_connections: u32, acquire_timeout: Duration) -> PgPool { + pool().await.close().await; + let url = match std::env::var("FORGE_TEST_DATABASE_URL") { + Ok(url) => url, + Err(_) => format!("postgres://forge:forge@{}/forge", pg_endpoint().await), + }; + PgPoolOptions::new() + .max_connections(max_connections) + .acquire_timeout(acquire_timeout) + .connect(&url) + .await + .expect("connect") +} + /// Every test database starts with its local realm accepting passwords from /// clients (tests sign in over Basic), and the auth settings at their /// defaults (anonymous access off). A test that needs others changes them on diff --git a/crates/server/tests/device_auth.rs b/crates/server/tests/device_auth.rs index 03b634fc..1f168d12 100644 --- a/crates/server/tests/device_auth.rs +++ b/crates/server/tests/device_auth.rs @@ -6,13 +6,16 @@ //! Covers a pending request, `slow_down`, expiry, denial and approval, a //! token collected once (and what it is), the scope, unknown clients and //! malformed requests (always RFC 6749 JSON), the rate limit on guessing -//! codes per network and per session, and the settings that turn the -//! device login off and set its interval. Requires Docker (Postgres + +//! codes per network and per session, concurrent polls on a small pool and +//! the rate limit on polling (#819), and the settings that turn the device +//! login off and set its interval. Requires Docker (Postgres + //! versitygw via the shared harness), or `FORGE_TEST_DATABASE_URL` / //! `FORGE_TEST_S3_ENDPOINT`. mod common; +use std::time::{Duration, Instant}; + use common::{fresh_pool, start_with, unique, BrowserSession, ServerOpts, TestServer}; use reqwest::StatusCode; use serde_json::{json, Value}; @@ -26,11 +29,18 @@ async fn start() -> TestServer { } async fn start_on(pool: sqlx::PgPool) -> TestServer { + start_configured(pool, "").await +} + +/// [`start_on`], with more `[auth]` lines. +async fn start_configured(pool: sqlx::PgPool, auth_extra: &str) -> TestServer { start_with( pool, ServerOpts { bootstrap_admin: Some((format!("admin{}@example.com", unique()), "adminpw".into())), - auth_extra: Some("trusted_proxy_cidrs = [\"127.0.0.1/32\", \"::1/128\"]".into()), + auth_extra: Some(format!( + "trusted_proxy_cidrs = [\"127.0.0.1/32\", \"::1/128\"]\n{auth_extra}" + )), ..Default::default() }, ) @@ -102,6 +112,28 @@ async fn poll(srv: &TestServer, client_id: &str, device_code: &str) -> (StatusCo .await } +/// Poll the token endpoint once from `ip` (`X-Forwarded-For`). +async fn poll_from( + srv: &TestServer, + client_id: &str, + device_code: &str, + ip: &str, +) -> (StatusCode, Value) { + let resp = srv + .client() + .post(format!("{}/auth/device/token", srv.base_url)) + .header("x-forwarded-for", ip) + .form(&[ + ("grant_type", GRANT), + ("client_id", client_id), + ("device_code", device_code), + ]) + .send() + .await + .unwrap(); + answer(resp).await +} + /// Expect an RFC 6749 §5.2 error. fn assert_error(answer: (StatusCode, Value), error: &str) { let (status, body) = answer; @@ -708,3 +740,143 @@ async fn the_settings_turn_the_device_login_off_and_set_its_interval() { "authorization_pending", ); } + +/// Poll `device_code` `n` times at once, and start an unrelated device login +/// (a request that needs the database) meanwhile. Every poll must be answered +/// within a second, `authorization_pending` or `slow_down`, and the other +/// request served; returns how many polls were told to slow down. +async fn poll_at_once(srv: &TestServer, device_code: &str, n: usize) -> usize { + let polls: Vec<_> = (0..n) + .map(|_| { + let client = srv.client(); + let url = format!("{}/auth/device/token", srv.base_url); + let device_code = device_code.to_string(); + tokio::spawn(async move { + let sent = Instant::now(); + let resp = client + .post(url) + .timeout(Duration::from_secs(5)) + .form(&[ + ("grant_type", GRANT), + ("client_id", "forge-cli"), + ("device_code", device_code.as_str()), + ]) + .send() + .await + .expect("the poll is answered"); + let answer = answer(resp).await; + (sent.elapsed(), answer) + }) + }) + .collect(); + let sent = Instant::now(); + let (status, other) = post_form(srv, "/auth/device/code", &[("client_id", "forge-cli")]).await; + assert_eq!(status, 200, "an unrelated request is served: {other}"); + assert!( + sent.elapsed() < Duration::from_secs(1), + "an unrelated request is served at once, not after {:?}", + sent.elapsed() + ); + let mut slowed = 0; + for poll in polls { + let (took, (status, body)) = poll.await.unwrap(); + assert_eq!(status, 400, "{body}"); + assert!( + took < Duration::from_secs(1), + "a poll is answered at once, not after {took:?}: {body}" + ); + match body["error"].as_str() { + Some("slow_down") => slowed += 1, + Some("authorization_pending") => {} + _ => panic!("neither pending nor slow_down: {body}"), + } + } + slowed +} + +/// Polls of a pending code never wait for its row while they hold a pool +/// connection, and are limited per code (#819): on a pool of two +/// connections, 50 concurrent polls are each answered at once, and an +/// unrelated request is served meanwhile. Also while another transaction +/// holds the row, which a poll used to wait for. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn concurrent_polls_of_a_pending_code_never_wait_for_its_row() { + let srv = start_on(common::small_pool(2, Duration::from_secs(3)).await).await; + + let device_code = start_login(&srv, "forge-cli", None).await["device_code"] + .as_str() + .unwrap() + .to_string(); + let slowed = poll_at_once(&srv, &device_code, 50).await; + assert!( + slowed >= 40, + "ten polls per code and ten seconds are answered from the database, \ + the others told to slow down: {slowed} of 50" + ); + + // Another transaction holds the request's row (another poll, still + // working on its answer): every poll is told to slow down at once. + let started = start_login(&srv, "forge-cli", None).await; + let device_code = started["device_code"].as_str().unwrap().to_string(); + let user_code = started["user_code"].as_str().unwrap().to_string(); + let other = common::pool().await; + let mut holder = other.begin().await.unwrap(); + sqlx::query("SELECT id FROM device_authorization WHERE user_code = $1 FOR UPDATE") + .bind(user_code.replace('-', "")) + .execute(&mut *holder) + .await + .unwrap(); + assert_eq!(poll_at_once(&srv, &device_code, 50).await, 50); + holder.rollback().await.unwrap(); + assert_eq!( + stored_interval(&srv, &user_code).await, + 5, + "a poll that found the row held recorded nothing" + ); +} + +/// Polls are limited per client network (`auth.device_polls_per_minute`, +/// #819): past it, `429` `slow_down` with `Retry-After`, without asking the +/// database; another network still polls. +#[tokio::test] +async fn polls_are_limited_per_network() { + let srv = start_configured(common::pool().await, "device_polls_per_minute = 3").await; + let device_code = start_login(&srv, "forge-cli", None).await["device_code"] + .as_str() + .unwrap() + .to_string(); + assert_error( + poll_from(&srv, "forge-cli", &device_code, "203.0.113.20").await, + "authorization_pending", + ); + for _ in 0..2 { + assert_error( + poll_from(&srv, "forge-cli", &device_code, "203.0.113.20").await, + "slow_down", + ); + } + let resp = srv + .client() + .post(format!("{}/auth/device/token", srv.base_url)) + .header("x-forwarded-for", "203.0.113.20") + .form(&[ + ("grant_type", GRANT), + ("client_id", "forge-cli"), + ("device_code", device_code.as_str()), + ]) + .send() + .await + .unwrap(); + assert_eq!(resp.headers()["retry-after"], "60"); + let (status, body) = answer(resp).await; + assert_eq!(status, 429, "{body}"); + assert_eq!(body["error"], "slow_down", "{body}"); + // Not even an unknown code reaches the database from that network. + let (status, body) = poll_from(&srv, "forge-cli", "unknown", "203.0.113.20").await; + assert_eq!(status, 429, "{body}"); + // Another network polls on. + assert_error( + poll_from(&srv, "forge-cli", &device_code, "198.51.100.20").await, + "slow_down", + ); +} diff --git a/docs/changes/2.2.0/649.feature.md b/docs/changes/2.2.0/649.feature.md index b814898b..f270091d 100644 --- a/docs/changes/2.2.0/649.feature.md +++ b/docs/changes/2.2.0/649.feature.md @@ -10,6 +10,7 @@ endpoints. Only the helpers' client ids are accepted, a `scope` (`read`, `write`, `delete`, `repository:`) narrows the token, and no refresh token is issued: the token lasts `cli_login.token_lifetime_days`, like npm's browser login's, and is listed and revocable on the user's tokens page. -Wrong codes are limited per network and per session. **Admin → Settings → -Device login** turns it off and sets the polling interval. See +Wrong codes are limited per network and per session, and polls per network +(`auth.device_polls_per_minute`, 600 a minute) and per code. **Admin → +Settings → Device login** turns it off and sets the polling interval. See [docs/deployment.md](docs/deployment.md#device-login). diff --git a/docs/deployment.md b/docs/deployment.md index 0b93978e..60edf92c 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -698,7 +698,7 @@ authorization server metadata ([RFC 8414](https://www.rfc-editor.org/rfc/rfc8414 | --- | --- | --- | | `GET` | `/.well-known/oauth-authorization-server` | `issuer` (`server.public_base_url`), `device_authorization_endpoint`, `token_endpoint`, `grant_types_supported` (`urn:ietf:params:oauth:grant-type:device_code`). | | `POST` | `/auth/device/code` | Start a login (RFC 8628 §3.1): `client_id` and an optional `scope`, form-encoded. Answers `device_code`, `user_code`, `verification_uri` (`/device`), `verification_uri_complete` (`/device?code=BCDF-GHJK`), `expires_in` and `interval`. | -| `POST` | `/auth/device/token` | Poll it (§3.4): `grant_type=urn:ietf:params:oauth:grant-type:device_code`, `client_id` and `device_code`. Answers `authorization_pending` until the user decides, `slow_down` when polled sooner than the interval (which then grows by five seconds), `access_denied`, `expired_token`, or the token: `access_token`, `token_type` `Bearer`, `expires_in`, `scope` when one was asked for, and `username`, whose token it is. | +| `POST` | `/auth/device/token` | Poll it (§3.4): `grant_type=urn:ietf:params:oauth:grant-type:device_code`, `client_id` and `device_code`. Answers `authorization_pending` until the user decides, `slow_down` when polled sooner than the interval (which then grows by five seconds) or too often ([polling](#device-login), below), `access_denied`, `expired_token`, or the token: `access_token`, `token_type` `Bearer`, `expires_in`, `scope` when one was asked for, and `username`, whose token it is. | ```bash curl -s https://forge.example.com/auth/device/code \ @@ -711,8 +711,9 @@ curl -s https://forge.example.com/auth/device/token \ Every refusal of these two endpoints is an RFC 6749 §5.2 JSON object (`{"error": "…", "error_description": "…"}`, `400`), never an HTML page, whatever credentials the request carries; `503` with `Retry-After` when -Forge's database does not answer, `429` when a network starts too many -logins (`auth.oidc_login_starts_per_minute`, as for OIDC sign-ins). +Forge's database does not answer, `429` with `Retry-After` when a network +starts too many logins (`auth.oidc_login_starts_per_minute`, as for OIDC +sign-ins) or polls too often (`auth.device_polls_per_minute`). - **Clients.** Forge has no client registration: it accepts its helpers' fixed client ids, `forge-cli`, `docker-credential-forge` and @@ -739,6 +740,22 @@ logins (`auth.oidc_login_starts_per_minute`, as for OIDC sign-ins). waits for its approval for `cli_login.web_login_ttl_seconds` (10 minutes); its token is answered to one poll, and a second poll gets `expired_token`. The auth reaper deletes the requests past their lifetime. +- **Polling.** Anyone may poll, without an account, so each instance counts + the polls before it asks the database: per client network (IPv6 by /64), + `auth.device_polls_per_minute` a minute (600; `0`: no limit), past which a + poll is answered `429` `slow_down` with `Retry-After: 60`; and per device + code, ten every ten seconds (one a second, the shortest interval), past + which it is answered `slow_down`. Polls of one code are answered one at a + time: a poll that comes while another poll of the code is being answered + is told `slow_down` at once, rather than waiting for it with a database + connection. Raise `auth.device_polls_per_minute` when many users sign in + at once from behind one address, or set `auth.trusted_proxy_cidrs` when + every client seems to come from Forge's reverse proxy. + + ```toml + [auth] + device_polls_per_minute = 600 + ``` - **The `/device` page.** A signed-in user types the code, or arrives with it filled in from `verification_uri_complete`; a visitor is sent through the sign-in and back, with the code. The page shows which client asks, for diff --git a/forge.toml.example b/forge.toml.example index f2e66cb3..ff111e30 100644 --- a/forge.toml.example +++ b/forge.toml.example @@ -169,6 +169,11 @@ realm_cache_ttl_seconds = 10 # (default: 10) # Concurrent upstream credential forwards (verdict-cache misses) allowed per # credential-forwarding realm, per instance; excess logins fail closed (401). credential_forward_max_concurrent = 32 # (default: 32) +# Device login polls (POST /auth/device/token) allowed per client network +# (IPv6: per /64) and minute, per instance; more are answered 429 slow_down. +# Raise it when many users sign in at once from behind one address. 0 = no +# limit. +device_polls_per_minute = 600 # (default: 600) # Optional: bootstrap admin, created once on the first start with a password. # Omit the section, or leave the password empty/unset, for no bootstrap admin