diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index d1d0295d4d..843377228b 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -176,10 +176,15 @@ impl AuthCache { return Some(OptJobAuthed { authed: no_auth_admin_authed(), job_id: None }); } // Reject an oversized guest bearer before the cache key is built from it: the key - // copies and hashes the whole token, so the cap should bound that work too. + // copies and hashes the whole token, so the cap should bound that work too. Log it + // like the other guest refusals, since get_opt_job_authed turns None into a bare 401. if token.starts_with(windmill_common::guest_jwt::BEARER_PREFIX) && token.len() > windmill_common::guest_jwt::MAX_GUEST_JWT_LEN { + tracing::error!( + "guest JWT refused: bearer is longer than {} bytes", + windmill_common::guest_jwt::MAX_GUEST_JWT_LEN + ); return None; } let key = ( diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index f84e8394ba..298524c8d1 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -231,6 +231,10 @@ struct JwksEntry { /// good keys if there are any), so an unreachable issuer cannot be turned into one /// outbound fetch per request by unauthenticated traffic. expires_at: Instant, + /// When these keys were last fetched successfully. Stale keys are served only within + /// `JWKS_MAX_STALE` of this, and a stale re-serve preserves it, so a revoked `kid` or an + /// unreachable issuer stops minting new guest JWTs after a bounded window, not forever. + fetched_at: Instant, } lazy_static::lazy_static! { @@ -246,6 +250,10 @@ const JWKS_TTL: Duration = Duration::from_secs(15 * 60); /// How long a failed fetch is remembered before retrying, so an unreachable issuer is /// hit at most once per this interval however much guest-JWT traffic arrives. const JWKS_NEGATIVE_TTL: Duration = Duration::from_secs(30); +/// The absolute age past which cached keys are no longer served, even while revalidating: +/// once an issuer has been unreachable (or has revoked a `kid`) for this long, its old keys +/// stop authenticating and the request fails closed rather than trusting them indefinitely. +const JWKS_MAX_STALE: Duration = Duration::from_secs(60 * 60); /// A JWKS body larger than this is refused rather than buffered: the URL is admin-set /// but the server it names may be attacker-controlled, and a real key set is a few KB. const JWKS_MAX_BYTES: usize = 1 << 20; @@ -390,11 +398,13 @@ fn spawn_jwks_refresh(url: String) { let Ok(_guard) = lock.try_lock() else { return }; match fetch_jwks(&url).await { Ok(keys) => { + let now = Instant::now(); JWKS_CACHE.insert( url, Arc::new(JwksEntry { keys: Arc::new(keys), - expires_at: Instant::now() + JWKS_TTL, + expires_at: now + JWKS_TTL, + fetched_at: now, }), ); } @@ -412,21 +422,28 @@ fn spawn_jwks_refresh(url: String) { /// attacker-chosen `kid`. async fn cached_jwks(url: &str) -> Result> { if let Some(entry) = JWKS_CACHE.get(url) { - if entry.expires_at > Instant::now() { - return servable(entry); - } - // Stale but still holds good keys: serve them now and refresh off the request - // path, so a slow or hanging issuer adds no latency. Bump the entry first so the - // refresh window does not spawn a task per request. A negative (empty) entry - // falls through to the blocking refresh below. - if !entry.keys.is_empty() { - let served = Arc::new(JwksEntry { - keys: entry.keys.clone(), - expires_at: Instant::now() + JWKS_NEGATIVE_TTL, - }); - JWKS_CACHE.insert(url.to_string(), served.clone()); - spawn_jwks_refresh(url.to_string()); - return Ok(served); + // Keys past JWKS_MAX_STALE are never served, even while revalidating: a stale re-serve + // bumps expires_at but keeps fetched_at, so a persistently failing refresh would + // otherwise serve revoked keys forever. Too-old keys fall through to the blocking + // refresh, which fails closed if the issuer is still down. + if entry.fetched_at.elapsed() < JWKS_MAX_STALE { + if entry.expires_at > Instant::now() { + return servable(entry); + } + // Stale but still holds good keys: serve them now and refresh off the request + // path, so a slow or hanging issuer adds no latency. Bump the entry first so the + // refresh window does not spawn a task per request. A negative (empty) entry + // falls through to the blocking refresh below. + if !entry.keys.is_empty() { + let served = Arc::new(JwksEntry { + keys: entry.keys.clone(), + expires_at: Instant::now() + JWKS_NEGATIVE_TTL, + fetched_at: entry.fetched_at, + }); + JWKS_CACHE.insert(url.to_string(), served.clone()); + spawn_jwks_refresh(url.to_string()); + return Ok(served); + } } } // Cold or negative entry, nothing good to serve: block on a single-flight refresh. @@ -435,28 +452,34 @@ async fn cached_jwks(url: &str) -> Result> { .get_or_insert_with(url, || Ok::<_, ()>(Arc::new(tokio::sync::Mutex::new(())))) .unwrap(); let _guard = lock.lock().await; - // Another task may have refreshed while we waited for the lock. + // Another task may have refreshed while we waited for the lock (honour the age limit too, + // so a concurrent stale re-serve of too-old keys is not mistaken for a fresh entry). if let Some(entry) = JWKS_CACHE.get(url) { - if entry.expires_at > Instant::now() { + if entry.fetched_at.elapsed() < JWKS_MAX_STALE && entry.expires_at > Instant::now() { return servable(entry); } } match fetch_jwks(url).await { Ok(keys) => { - let entry = - Arc::new(JwksEntry { keys: Arc::new(keys), expires_at: Instant::now() + JWKS_TTL }); + let now = Instant::now(); + let entry = Arc::new(JwksEntry { + keys: Arc::new(keys), + expires_at: now + JWKS_TTL, + fetched_at: now, + }); JWKS_CACHE.insert(url.to_string(), entry.clone()); Ok(entry) } Err(e) => { - // The blocking path is reached only with no good keys to serve (a stale-good - // entry is served by the fast path above). Cache a short negative entry so - // the next requests do not each refetch, and surface the error. + // Reached with no servable keys: either nothing cached, or keys too old to trust. + // Cache a short negative entry so the next requests do not each refetch, and + // surface the error, so an issuer that revoked a key or went down fails closed. JWKS_CACHE.insert( url.to_string(), Arc::new(JwksEntry { keys: Arc::new(HashMap::new()), expires_at: Instant::now() + JWKS_NEGATIVE_TTL, + fetched_at: Instant::now(), }), ); Err(e) @@ -804,4 +827,51 @@ y9rTR828ADcaZ63Ej1oL4GcqmGhODxCLy1YKKcy0FHzChqPMV6g=\n\ ); unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") }; } + + #[tokio::test] + async fn stale_jwks_keys_stop_being_served_past_the_grace_window() { + unsafe { std::env::set_var("ALLOW_PRIVATE_GUEST_JWKS_URLS", "true") }; + // A dead loopback port, so every refresh fails (connection refused). + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let port = listener.local_addr().unwrap().port(); + drop(listener); + let jwk = jwk(serde_json::json!( + {"kty":"EC","crv":"P-256","kid":"k1","x":PUB1_X,"y":PUB1_Y} + )); + let keys: HashMap = [("k1".to_string(), jwk)].into_iter().collect(); + let stale = Instant::now().checked_sub(Duration::from_secs(1)).unwrap(); + + // Within the grace window, stale keys are still served while a (failing) refresh runs. + let within = format!("http://127.0.0.1:{port}/within"); + JWKS_CACHE.insert( + within.clone(), + Arc::new(JwksEntry { + keys: Arc::new(keys.clone()), + expires_at: stale, + fetched_at: Instant::now(), + }), + ); + assert!( + cached_jwks(&within).await.is_ok(), + "stale keys within the grace window are still served" + ); + + // Past the grace window, the keys are not served: the failing refresh fails closed. + let beyond = format!("http://127.0.0.1:{port}/beyond"); + JWKS_CACHE.insert( + beyond.clone(), + Arc::new(JwksEntry { + keys: Arc::new(keys), + expires_at: stale, + fetched_at: Instant::now() + .checked_sub(JWKS_MAX_STALE + Duration::from_secs(1)) + .unwrap(), + }), + ); + assert!( + cached_jwks(&beyond).await.is_err(), + "keys past the grace window fail closed once the refresh fails" + ); + unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") }; + } }