mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-07 00:01:49 +00:00
fix: bound every dimension of the JWKS cache (URL, key count, key bytes)
The retained-bytes cap left two dimensions unbounded that reviewers named: the cache keys on the admin-supplied URL (unbounded TEXT column), and the per-key fixed cost (many tiny keys serialize small but each Jwk and its map slot cost memory). Add a URL-length cap in fetch_jwks (the choke point save validates through, so an overlong URL is never stored or cached) and a usable-key count cap alongside the retained-bytes cap, both measured after filtering so a mixed- use set is judged by its signing keys. Every dimension is now bounded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
badc996ab9
commit
072d866e78
@@ -247,12 +247,26 @@ const JWKS_MAX_BYTES: usize = 1 << 20;
|
||||
/// 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;
|
||||
|
||||
/// The retained-bytes cap counts key material; this caps the number of keys so the per-key
|
||||
/// fixed cost (each `Jwk` and its map slot) is bounded too, not just their string content.
|
||||
const JWKS_MAX_KEYS: usize = 50;
|
||||
|
||||
/// Both JWKS caches key on the admin-supplied URL string. The column is unbounded `TEXT`, so
|
||||
/// without this a workspace admin rotating long URLs could grow the caches by the URL bytes
|
||||
/// alone. A real JWKS URL is well under this; the check runs before the URL is ever cached.
|
||||
const MAX_JWKS_URL_LEN: usize = 2048;
|
||||
|
||||
/// 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
|
||||
/// validated addresses; redirects are not followed for the same reason. The body is
|
||||
/// read with a cap so a hostile endpoint cannot exhaust memory.
|
||||
pub async fn fetch_jwks(url: &str) -> Result<HashMap<String, Jwk>> {
|
||||
use futures::StreamExt;
|
||||
if url.len() > MAX_JWKS_URL_LEN {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"JWKS URL is longer than {MAX_JWKS_URL_LEN} bytes"
|
||||
)));
|
||||
}
|
||||
let target = crate::ssrf::validate_guest_jwks_url(url)
|
||||
.await
|
||||
.map_err(|e| Error::BadRequest(format!("JWKS URL is not allowed: {e}")))?;
|
||||
@@ -311,8 +325,14 @@ fn parse_jwks_keys(body: &[u8]) -> Result<HashMap<String, Jwk>> {
|
||||
"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.
|
||||
// Bound the usable keys two ways, both measured after filtering so a large mixed-use set
|
||||
// (many encryption keys, few signing) is not refused for its size: their count (the per-key
|
||||
// fixed cost) and their combined material bytes.
|
||||
if keys.len() > JWKS_MAX_KEYS {
|
||||
return Err(Error::BadRequest(format!(
|
||||
"JWKS holds more than {JWKS_MAX_KEYS} usable signing keys"
|
||||
)));
|
||||
}
|
||||
let retained: usize = keys
|
||||
.values()
|
||||
.filter_map(|jwk| serde_json::to_vec(jwk).ok().map(|v| v.len()))
|
||||
@@ -536,17 +556,40 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
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)
|
||||
fn too_many_usable_keys_is_refused() {
|
||||
// All keys are usable (real coordinates), so this trips the count cap, not the
|
||||
// empty-set path. Small material, so it is the count that refuses them, not the bytes.
|
||||
let keys: Vec<_> = (0..=JWKS_MAX_KEYS)
|
||||
.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());
|
||||
let err = parse_jwks_keys(body.as_bytes()).unwrap_err().to_string();
|
||||
assert!(err.contains("more than"), "{err}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn keys_retaining_too_many_bytes_is_refused() {
|
||||
// Under the key count cap, but the material of these usable RSA keys exceeds the byte
|
||||
// cap. `from_jwk` decodes any-length `n`, so this is reachable without the count cap.
|
||||
let big_n = "A".repeat(2000);
|
||||
let keys: Vec<_> = (0..40)
|
||||
.map(|i| serde_json::json!({"kty":"RSA","kid":format!("k{i}"),"n":big_n,"e":"AQAB"}))
|
||||
.collect();
|
||||
let body = serde_json::json!({ "keys": keys }).to_string();
|
||||
let err = parse_jwks_keys(body.as_bytes()).unwrap_err().to_string();
|
||||
assert!(err.contains("retain more than"), "{err}");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn an_overlong_jwks_url_is_refused() {
|
||||
// The cache keys on the URL string, so an unbounded URL is refused before it is cached.
|
||||
let url = format!(
|
||||
"https://issuer.example.com/{}",
|
||||
"a".repeat(MAX_JWKS_URL_LEN)
|
||||
);
|
||||
assert!(fetch_jwks(&url).await.is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user