diff --git a/backend/tests/app_guest_jwt_entry.rs b/backend/tests/app_guest_jwt_entry.rs index 53b490201c..b3492a0591 100644 --- a/backend/tests/app_guest_jwt_entry.rs +++ b/backend/tests/app_guest_jwt_entry.rs @@ -288,6 +288,12 @@ async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { c.email = "u/x@example.com".to_string(); let slash_in_email = bearer(&c, PRIV1, Algorithm::ES256); + // an email longer than the `guest_activity.email` column: refused before auth, so a + // guest is never admitted without the activity row and audit event the count needs. + let mut c = Claims::valid(); + c.email = format!("{}@example.com", "a".repeat(250)); + let oversized_email = bearer(&c, PRIV1, Algorithm::ES256); + for (label, token) in [ ("wrong workspace", wrong_ws), ("wrong key", wrong_key), @@ -299,6 +305,7 @@ async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { ("over the 24h lifetime cap", over_lifetime_cap), ("group-shaped email", group_shaped_email), ("slash in email", slash_in_email), + ("oversized email", oversized_email), ] { let resp = whoami(port, ws, &token).send().await?; assert_eq!(resp.status(), 401, "{label} must be refused"); diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index 1b6f907200..32a8340185 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -162,14 +162,17 @@ pub fn verify( } // The email becomes the guest's username, and `username_to_permissioned_as` reads a // name with no `@` (or the `group-` prefix) as a `u/` or `g/` - // principal. Require a plain email address so a guest's name can only ever be its - // own principal, never a user's or a group's. + // principal. Require a plain, bounded email: so a guest's name can only ever be its + // own principal, never a user's or a group's, and so it fits the + // `guest_activity.email` column (a longer one fails that insert while the guest is + // admitted uncounted). if !claims.email.contains('@') || claims.email.contains('/') || claims.email.chars().any(char::is_whitespace) + || claims.email.len() > 254 { return Err(Error::NotAuthorized( - "guest JWT refused: email is not a plain email address".to_string(), + "guest JWT refused: email is not a plain, bounded email address".to_string(), )); } Ok(claims) @@ -263,20 +266,58 @@ fn servable(entry: Arc) -> Result> { } } -/// The workspace's JWKS, from cache when fresh. A cold or stale entry is refetched -/// under a per-URL lock, so a burst triggers one fetch, not one per request; a failed -/// refetch serves the last good keys, or a short-lived empty entry that reads as -/// "issuer unreachable" rather than "kid not found", so an unreachable issuer is hit at -/// most once per `JWKS_NEGATIVE_TTL`. Fetches follow a schedule, never a per-request, +/// Refresh a URL's JWKS off the request path, under the single-flight lock. A held +/// lock means a refresh is already running, so this is a no-op. A failed refresh +/// leaves the served stale keys in place rather than dropping them. +fn spawn_jwks_refresh(url: String) { + tokio::spawn(async move { + let lock = JWKS_FETCH_LOCKS + .get_or_insert_with(url.as_str(), || Ok::<_, ()>(Arc::new(tokio::sync::Mutex::new(())))) + .unwrap(); + let Ok(_guard) = lock.try_lock() else { return }; + match fetch_jwks(&url).await { + Ok(keys) => { + JWKS_CACHE.insert( + url, + Arc::new(JwksEntry { + keys: Arc::new(keys), + expires_at: Instant::now() + JWKS_TTL, + }), + ); + } + Err(e) => tracing::warn!("guest JWKS background refresh failed for {url}: {e:#}"), + } + }); +} + +/// The workspace's JWKS. A fresh entry is served directly; a stale-but-good one is +/// served while a refresh runs off the request path, so a slow issuer never stalls a +/// request. Only a cold or negative entry blocks, under a per-URL lock so a burst +/// triggers one fetch; a failed fetch serves the last good keys, or a short-lived empty +/// entry that reads as "issuer unreachable", so an unreachable issuer is hit at most +/// once per `JWKS_NEGATIVE_TTL`. Fetches follow a schedule, never a per-request, /// 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); + } } - // Single-flight: hold the per-URL lock across the fetch. `get_or_insert_with` - // creates the lock atomically, so two cold requests share one. + // Cold or negative entry, nothing good to serve: block on a single-flight refresh. + // `get_or_insert_with` creates the lock atomically, so two cold requests share one. let lock = JWKS_FETCH_LOCKS .get_or_insert_with(url, || Ok::<_, ()>(Arc::new(tokio::sync::Mutex::new(())))) .unwrap();