From f5713621c8de9a0927f90d0224ef1f38ca4e4464 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 10:30:46 +0200 Subject: [PATCH] fix: accept any PEM wrapping for the guest key, keep the public-key check The SPKI guard decoded the body with the strict RFC 7468 decoder, which enforces 64-column wrapping, so a legitimate public key wrapped otherwise (or a PKCS#1 `RSA PUBLIC KEY`) was refused where jsonwebtoken would have parsed it. Decode the body leniently like jsonwebtoken, then require the DER to be a public-key structure: an SPKI (RSA or EC) or a PKCS#1 RSA public key. Private material satisfies neither, so the round-13 bypass stays closed. Test adds a one-line (non-64-column) public key as a positive control. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3 --- backend/Cargo.lock | 1 + backend/Cargo.toml | 1 + backend/windmill-common/Cargo.toml | 1 + backend/windmill-common/src/guest_jwt.rs | 39 ++++++++++++++++++------ 4 files changed, 32 insertions(+), 10 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 941c3c29ee..9678fbe993 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -15602,6 +15602,7 @@ dependencies = [ "pep440_rs", "phf 0.11.3", "pin-project-lite", + "pkcs1", "postgres-native-tls 0.5.3", "prometheus", "quick_cache", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index e58cb3f5d0..8c6b6038b7 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -601,6 +601,7 @@ const-str = "0.5" constant_time_eq = "0.3.1" rsa = "^0" spki = { version = "0.7", features = ["pem"] } +pkcs1 = "0.7" aes-gcm = "0.10.3" async_zip = { version = "0.0.17", features = ["tokio", "tokio-fs", "deflate", "chrono"] } once_cell = "1.17.1" diff --git a/backend/windmill-common/Cargo.toml b/backend/windmill-common/Cargo.toml index 76b7b8cbf3..dcf3465a94 100644 --- a/backend/windmill-common/Cargo.toml +++ b/backend/windmill-common/Cargo.toml @@ -110,6 +110,7 @@ systemstat.workspace = true size.workspace = true rsa = { workspace = true, optional = true } spki = { workspace = true } +pkcs1 = { workspace = true } aes-gcm = { workspace = true, optional = true } semver.workspace = true diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index d496cc6a01..360c3dc804 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -80,20 +80,30 @@ pub async fn key_source(db: &DB, w_id: &str) -> Result /// EC keys the ES family. Anything symmetric has no PEM form, so HS* is unreachable /// from here by construction; the JWKS path refuses it explicitly. pub fn decoding_key_from_pem(pem: &str) -> Result<(DecodingKey, &'static [Algorithm])> { - use spki::der::{Decode, Document}; + use base64::{engine::general_purpose::STANDARD, Engine}; + use spki::der::Decode; let pem = pem.trim(); // A verification key must be public. jsonwebtoken 8.3 keys the public/private distinction // off the PEM label alone and never inspects the DER, so private material relabelled - // `PUBLIC KEY` would be stored and then served back through the settings response. Require - // the key to parse as a SubjectPublicKeyInfo, which private-key DER cannot satisfy. - let (_, doc) = Document::from_pem(pem) - .map_err(|e| Error::BadRequest(format!("not a valid PEM key: {e}")))?; - spki::SubjectPublicKeyInfoRef::from_der(doc.as_bytes()).map_err(|_| { - Error::BadRequest( - "expected an RSA or EC public key in SPKI form (-----BEGIN PUBLIC KEY-----)" - .to_string(), + // `PUBLIC KEY` would be stored and then served back through the settings response. Decode + // the body leniently, as jsonwebtoken does (tolerating any wrapping the strict RFC 7468 + // decoder would refuse), then require it to be a public-key structure: an SPKI (RSA or EC) + // or a PKCS#1 RSA public key. Private-key DER satisfies neither. + let der = STANDARD + .decode( + pem.lines() + .filter(|l| !l.trim_start().starts_with("-----")) + .flat_map(|l| l.split_whitespace()) + .collect::(), ) - })?; + .map_err(|e| Error::BadRequest(format!("guest key is not valid PEM: {e}")))?; + let is_public = spki::SubjectPublicKeyInfoRef::from_der(&der).is_ok() + || pkcs1::RsaPublicKey::from_der(&der).is_ok(); + if !is_public { + return Err(Error::BadRequest( + "expected an RSA or EC public key in PEM form (-----BEGIN PUBLIC KEY-----)".to_string(), + )); + } if let Ok(key) = DecodingKey::from_rsa_pem(pem.as_bytes()) { return Ok((key, &RSA_ALGORITHMS)); } @@ -589,6 +599,15 @@ y9rTR828ADcaZ63Ej1oL4GcqmGhODxCLy1YKKcy0FHzChqPMV6g=\n\ // only parsing the DER as a SubjectPublicKeyInfo refuses it. A real public key still // parses, so the guard is not vacuous. assert!(decoding_key_from_pem(RSA_PUBLIC).is_ok()); + // The same key with its body on one line (not 64-column wrapped): jsonwebtoken accepts + // any wrapping, so the guard must too rather than lean on the strict RFC 7468 decoder. + let body: String = RSA_PUBLIC + .lines() + .filter(|l| !l.starts_with("-----")) + .collect(); + let one_line = format!("-----BEGIN PUBLIC KEY-----\n{body}\n-----END PUBLIC KEY-----\n"); + assert!(decoding_key_from_pem(&one_line).is_ok()); + assert!(decoding_key_from_pem(PRIV1).is_err()); // The same PKCS#8 EC private key, relabelled `PUBLIC KEY`. assert!(decoding_key_from_pem(&PRIV1.replace("PRIVATE KEY", "PUBLIC KEY")).is_err());