fix: fail closed on JWKS keys past an absolute stale window

Stale-while-revalidate re-served cached keys and bumped their window on every
request, but never tracked when the keys were last fetched successfully, so a
persistently failing refresh (a revoked kid, or an unreachable issuer) served
the old keys forever, minting fresh 24h guest JWTs indefinitely. Track
fetched_at, preserve it across stale re-serves, and stop serving keys older than
JWKS_MAX_STALE (1h): too-old keys fall through to a blocking refresh that fails
closed. Also log the oversized-bearer refusal, which returned a bare None.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3

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 = (

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<Arc<JwksEntry>> {
     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<Arc<JwksEntry>> {
         .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<String, Jwk> = [("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") };
+    }
 }