From badc996ab9872360a249d5e0433ef3c91be277e6 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 15:39:38 +0200 Subject: [PATCH] fix: bound the JWKS cache by retained bytes on the usable keys The key-count cap did not bound retained memory (from_jwk decodes n/e/x/y with no length limit, so 50 keys could still carry ~1 MiB), and it counted raw entries, refusing a valid mixed-use set with many encryption keys wholesale. Replace it with a cap on the serialized size of the usable, retained keys (JWKS_MAX_RETAINED_BYTES = 64 KiB), measured after filtering. Cache ceiling is now bounded (200 entries x 64 KiB); a real set retains a few KB. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3 --- backend/windmill-common/src/guest_jwt.rs | 38 +++++++++++++++--------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index f76490cc2f..1a56f22f5d 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -241,10 +241,11 @@ const JWKS_NEGATIVE_TTL: Duration = Duration::from_secs(30); /// 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; -/// A real JWKS carries a handful of keys. A set larger than this is refused so one cache -/// entry cannot retain a densely packed body (the byte cap alone allows thousands of minimal -/// JWKs), which a workspace admin rotating URLs could otherwise use to grow the shared process. -const JWKS_MAX_KEYS: usize = 50; +/// A cache entry retains only the usable signing keys, but nothing else bounds their combined +/// size: the response cap is 1 MiB and `from_jwk` decodes `n`/`e`/`x`/`y` without a length +/// limit, so one entry could retain ~1 MiB (a few hundred MB across the 200-entry LRU). Cap the +/// retained material instead; a real set is a few KB, so this is invisible to a legitimate one. +const JWKS_MAX_RETAINED_BYTES: usize = 64 * 1024; /// Fetch a JWKS, keeping only the keys usable here. The URL was set by a workspace /// admin, so it is validated against private ranges and the connect is pinned to the @@ -298,11 +299,6 @@ fn parse_jwks_keys(body: &[u8]) -> Result> { .get("keys") .and_then(|k| k.as_array()) .ok_or_else(|| Error::BadRequest("JWKS has no `keys` array".to_string()))?; - if entries.len() > JWKS_MAX_KEYS { - return Err(Error::BadRequest(format!( - "JWKS holds more than {JWKS_MAX_KEYS} keys" - ))); - } let keys: HashMap = entries .iter() .filter_map(|entry| serde_json::from_value::(entry.clone()).ok()) @@ -315,6 +311,17 @@ fn parse_jwks_keys(body: &[u8]) -> Result> { "JWKS holds no RSA or EC signing key with a kid".to_string(), )); } + // Bound the retained material by bytes, measured after filtering so a large mixed-use set + // (many encryption keys, few signing) is not refused for its size. + let retained: usize = keys + .values() + .filter_map(|jwk| serde_json::to_vec(jwk).ok().map(|v| v.len())) + .sum(); + if retained > JWKS_MAX_RETAINED_BYTES { + return Err(Error::BadRequest(format!( + "JWKS signing keys retain more than {JWKS_MAX_RETAINED_BYTES} bytes" + ))); + } Ok(keys) } @@ -529,11 +536,14 @@ mod tests { } #[test] - fn an_oversized_jwks_is_refused() { - // The byte cap alone allows thousands of minimal keys in one entry; the count cap - // bounds what a single cache entry can retain. - let keys: Vec<_> = (0..=JWKS_MAX_KEYS) - .map(|i| serde_json::json!({"kty":"RSA","kid":format!("k{i}"),"n":"aa","e":"AQAB"})) + fn a_jwks_retaining_too_many_bytes_is_refused() { + // Every key has real coordinates, so all are usable and this exercises the + // retained-bytes cap rather than the empty-set path: their combined material exceeds + // JWKS_MAX_RETAINED_BYTES, which the byte-per-response and entry-count caps do not bound. + let keys: Vec<_> = (0..600) + .map(|i| { + serde_json::json!({"kty":"EC","crv":"P-256","kid":format!("k{i}"),"x":PUB1_X,"y":PUB1_Y}) + }) .collect(); let body = serde_json::json!({ "keys": keys }).to_string(); assert!(parse_jwks_keys(body.as_bytes()).is_err());