From 51776ef87c85b9e1e397570a3bdbfdc343180899 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 09:54:05 +0200 Subject: [PATCH] fix: refuse a private PEM as a guest verification key; fix stale log jsonwebtoken 8.3's from_rsa_pem accepts private encodings (PKCS#1, PKCS#8), so a pasted private key would be stored and then served back through the settings response. Refuse any private PEM in decoding_key_from_pem, the single choke point for both the save endpoint and per-request verification. Also drop the last "canonical" references the is_scope_literal_path switch left in the JWT arm: the refusal log no longer misdiagnoses a reserved character as a malformed path, and the relocated guest_session_scopes doc carries the sentinel/label distinction for both its callers. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3 --- backend/windmill-api-auth/src/auth.rs | 25 +++++++++++++++++------- backend/windmill-api-auth/src/scopes.rs | 3 +++ backend/windmill-common/src/guest_jwt.rs | 20 +++++++++++++++++++ 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 6ca9a5200f..6f88ed9a4e 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -219,10 +219,13 @@ impl AuthCache { _ if token.starts_with(windmill_common::guest_jwt::BEARER_PREFIX) => { // A workspace-less route never accepts a guest JWT: the identity is // pinned to the workspace its claim names, like a DB guest session. - let Some(w_id) = w_id.as_deref() else { return None }; + let Some(w_id) = w_id.as_deref() else { + return None; + }; let jwt = token.trim_start_matches(windmill_common::guest_jwt::BEARER_PREFIX); let claims = - match windmill_common::guest_jwt::verify_for_workspace(&self.db, w_id, jwt).await + match windmill_common::guest_jwt::verify_for_workspace(&self.db, w_id, jwt) + .await { Ok(c) => c, Err(e) => { @@ -235,8 +238,12 @@ impl AuthCache { // and the no-account rule per request through the sentinel below // (guest_session_stands), so turning any of them off stops a cached JWT // session on its next call. - match windmill_common::workspaces::guest_app_admits(&self.db, w_id, &claims.app_path) - .await + match windmill_common::workspaces::guest_app_admits( + &self.db, + w_id, + &claims.app_path, + ) + .await { Ok(true) => {} Ok(false) => return None, @@ -268,11 +275,11 @@ impl AuthCache { } // guest_session_scopes already carries the sentinel, and it is the whole // grant; a JWT has no label, so the sentinel is what governs it. It also - // re-checks the path is canonical (verify already did); refuse if not. + // re-checks the path holds no scope metacharacter (verify already did). let scopes = match crate::scopes::guest_session_scopes(&claims.app_path) { Ok(s) => Some(s), Err(e) => { - tracing::error!("guest JWT app_path not canonical for {w_id}: {e:#}"); + tracing::error!("guest JWT app_path cannot be scoped for {w_id}: {e:#}"); return None; } }; @@ -301,7 +308,11 @@ impl AuthCache { }; AUTH_CACHE.insert( key, - ExpiringAuthCache { authed: authed.clone(), expiry: cache_expiry, job_id: None }, + ExpiringAuthCache { + authed: authed.clone(), + expiry: cache_expiry, + job_id: None, + }, ); Some(OptJobAuthed { authed, job_id: None }) } diff --git a/backend/windmill-api-auth/src/scopes.rs b/backend/windmill-api-auth/src/scopes.rs index 26e7237511..5bd7b4d275 100644 --- a/backend/windmill-api-auth/src/scopes.rs +++ b/backend/windmill-api-auth/src/scopes.rs @@ -797,6 +797,9 @@ pub fn with_guest_sentinel(mut scopes: Vec) -> Vec { /// allowlist by the sentinel (`guest_route_denied`), plus the two path-scoped app /// grants. A guest has no `usr` row, so this list is the whole of what it can do. The /// single source both the mint (a signed-in guest) and the JWT auth arm build from. +/// +/// The sentinel here only narrows. A signed-in guest is made one by the server-minted +/// label; a JWT guest has no label, so for it the sentinel is what governs. pub fn guest_session_scopes(app_path: &str) -> windmill_common::error::Result> { // The path is spliced into a scope, whose grammar reserves `:`, `,`, `*` and a leading // `/`; app paths may otherwise carry spaces and `@`, so guard only those reserved chars. diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index 6b2e5c4a42..c82f5ac06e 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -81,6 +81,14 @@ pub async fn key_source(db: &DB, w_id: &str) -> Result /// from here by construction; the JWKS path refuses it explicitly. pub fn decoding_key_from_pem(pem: &str) -> Result<(DecodingKey, &'static [Algorithm])> { let pem = pem.trim(); + // A verification key must be public. jsonwebtoken 8.3's `from_rsa_pem` also accepts private + // encodings (PKCS#1, PKCS#8), so a pasted private key would be stored and then served back + // through the settings response. Refuse any private PEM before it is parsed or persisted. + if pem.contains("PRIVATE KEY") { + return Err(Error::BadRequest( + "expected a PEM public key (-----BEGIN PUBLIC KEY-----), not a private key".to_string(), + )); + } if let Ok(key) = DecodingKey::from_rsa_pem(pem.as_bytes()) { return Ok((key, &RSA_ALGORITHMS)); } @@ -525,6 +533,18 @@ mod tests { .is_err()); } + #[test] + fn a_private_pem_is_refused() { + // A verification key must be public; a private key must never be stored, as it is + // served back through the settings response. PRIV1 is a real EC private key the PEM + // parser would otherwise accept; the RSA label covers jsonwebtoken's PKCS#1/#8 case. + assert!(decoding_key_from_pem(PRIV1).is_err()); + assert!(decoding_key_from_pem( + "-----BEGIN RSA PRIVATE KEY-----\nMIIBOgIBAAJBAKj\n-----END RSA PRIVATE KEY-----\n" + ) + .is_err()); + } + #[tokio::test] async fn a_concurrent_cold_burst_makes_one_jwks_fetch() { use std::sync::atomic::{AtomicUsize, Ordering};