starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-06 16:02:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: address round-10 review on the guest JWT entry

Browse Source 
Save the guest JWT key before the Enterprise-only default-app and rate-limit
writes, so a refused write cannot swallow a valid key change on CE. Name the
JWT entry in the Guests card summary. Complete verify()'s doc with the email
and app_path rules. Anchor the refusal suite with a positive control and make
enable_guests assert its status, so a broken fixture cannot pass it vacuously.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3
This commit is contained in:
Ruben Fiszel
2026-09-04 09:33:29 +02:00
co-authored by Claude Opus 4.8
parent ac2fd76709
commit 82fc97dfc5
3 changed files with 46 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/tests/app_guest_jwt_entry.rs
+9 -1
View File
@@ -86,7 +86,7 @@ fn bearer(claims: &Claims, priv_pem: &str, alg: Algorithm) -> String {
}
async fn enable_guests(port: u16, ws: &str, on: bool) -> anyhow::Result<()> {
authed(
let resp = authed(
client().post(format!(
"http://localhost:{port}/api/w/{ws}/workspaces/edit_guest_access"
)),
@@ -95,6 +95,7 @@ async fn enable_guests(port: u16, ws: &str, on: bool) -> anyhow::Result<()> {
.json(&json!({ "guest_access_enabled": on }))
.send()
.await?;
assert_eq!(resp.status(), 200, "{}", resp.text().await?);
Ok(())
}
@@ -243,6 +244,13 @@ async fn guest_jwt_refusals(db: Pool<Postgres>) -> anyhow::Result<()> {
create_app(port, ws, app(APP_PATH, "guest", false)).await?;
create_app(port, ws, app("u/test-user/members_app", "publisher", false)).await?;
// Positive control: a token valid against this exact fixture is admitted. Without it a
// broken setup would 401 every bearer below and the whole suite would pass vacuously.
let control = whoami(port, ws, &bearer(&Claims::valid(), PRIV1, Algorithm::ES256))
.send()
.await?;
assert_eq!(control.status(), 200, "{}", control.text().await?);
// wrong workspace: the claim must name the route's workspace.
let mut c = Claims::valid();
c.workspace_id = "other-ws".to_string();
backend/windmill-common/src/guest_jwt.rs
+28 -13
View File
@@ -67,11 +67,13 @@ pub async fn key_source(db: &DB, w_id: &str) -> Result<Option<GuestJwtKeySource>
.fetch_optional(db)
.await
.map_err(|e| Error::internal_err(format!("reading guest JWT key of {w_id}: {e:#}")))?;
Ok(row.and_then(|r| match (r.guest_jwt_public_key, r.guest_jwt_jwks_url) {
(Some(pem), _) => Some(GuestJwtKeySource::Pem(pem)),
(None, Some(url)) => Some(GuestJwtKeySource::JwksUrl(url)),
(None, None) => None,
}))
Ok(
row.and_then(|r| match (r.guest_jwt_public_key, r.guest_jwt_jwks_url) {
(Some(pem), _) => Some(GuestJwtKeySource::Pem(pem)),
(None, Some(url)) => Some(GuestJwtKeySource::JwksUrl(url)),
(None, None) => None,
}),
)
}
/// Parse a PEM public key and the algorithms it may verify: RSA keys the RS/PS family,
@@ -86,8 +88,7 @@ pub fn decoding_key_from_pem(pem: &str) -> Result<(DecodingKey, &'static [Algori
return Ok((key, &EC_ALGORITHMS));
}
Err(Error::BadRequest(
"not an RSA or EC public key in PEM form (expected -----BEGIN PUBLIC KEY-----)"
.to_string(),
"not an RSA or EC public key in PEM form (expected -----BEGIN PUBLIC KEY-----)".to_string(),
))
}
@@ -113,7 +114,9 @@ pub fn jwk_algorithms(jwk: &Jwk) -> Option<Vec<Algorithm>> {
return None;
}
match (&jwk.algorithm, jwk.common.algorithm) {
(AlgorithmParameters::RSA(_), Some(alg)) if RSA_ALGORITHMS.contains(&alg) => Some(vec![alg]),
(AlgorithmParameters::RSA(_), Some(alg)) if RSA_ALGORITHMS.contains(&alg) => {
Some(vec![alg])
}
(AlgorithmParameters::RSA(_), None) => Some(RSA_ALGORITHMS.to_vec()),
(AlgorithmParameters::EllipticCurve(_), Some(alg)) if EC_ALGORITHMS.contains(&alg) => {
Some(vec![alg])
@@ -129,7 +132,8 @@ pub fn jwk_algorithms(jwk: &Jwk) -> Option<Vec<Algorithm>> {
/// Verify `token` against `key`, honouring only the accepted `algorithms`, and check
/// every claim rule that needs no database: signature, `exp` (mandatory), `nbf` and
/// `iat` when present, the lifetime cap, and that the token names `w_id`.
/// `iat` when present, the lifetime cap, that the token names `w_id`, that `email` is a
/// valid address bounded to 254 bytes, and that `app_path` is a canonical path.
pub fn verify(
token: &str,
key: &DecodingKey,
@@ -289,7 +293,9 @@ fn servable(entry: Arc<JwksEntry>) -> Result<Arc<JwksEntry>> {
fn spawn_jwks_refresh(url: String) {
tokio::spawn(async move {
let lock = JWKS_FETCH_LOCKS
.get_or_insert_with(url.as_str(), || Ok::<_, ()>(Arc::new(tokio::sync::Mutex::new(()))))
.get_or_insert_with(url.as_str(), || {
Ok::<_, ()>(Arc::new(tokio::sync::Mutex::new(())))
})
.unwrap();
let Ok(_guard) = lock.try_lock() else { return };
match fetch_jwks(&url).await {
@@ -510,7 +516,10 @@ mod tests {
#[test]
fn a_non_key_pem_is_rejected() {
assert!(decoding_key_from_pem("-----BEGIN CERTIFICATE-----\nnope\n-----END CERTIFICATE-----").is_err());
assert!(decoding_key_from_pem(
"-----BEGIN CERTIFICATE-----\nnope\n-----END CERTIFICATE-----"
)
.is_err());
}
#[tokio::test]
@@ -546,10 +555,16 @@ mod tests {
let mut handles = Vec::new();
for _ in 0..10 {
let u = url.clone();
handles.push(tokio::spawn(async move { cached_jwks(&u).await.map(|e| e.keys.len()) }));
handles.push(tokio::spawn(async move {
cached_jwks(&u).await.map(|e| e.keys.len())
}));
}
for h in handles {
assert_eq!(h.await.unwrap().unwrap(), 1, "each caller resolves the one key");
assert_eq!(
h.await.unwrap().unwrap(),
1,
"each caller resolves the one key"
);
}
assert_eq!(
hits.load(Ordering::SeqCst),
frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte
+9 -9
View File
@@ -539,23 +539,23 @@
}
async function saveDefaultAppSettings(): Promise<void> {
// Guests first: the only write of this card available on every plan, so a refused
// Enterprise-only write after it cannot swallow it.
// Guest access and the guest JWT key are the writes of this card available on every plan;
// save them first so a refused Enterprise-only write after cannot swallow them.
if (guestAccessEnabled !== initialGuestAccessEnabled) {
await editGuestAccess()
}
if (workspaceDefaultAppPath !== initialWorkspaceDefaultAppPath) {
await editWorkspaceDefaultApp()
}
if (publicAppRateLimitPerMinute !== initialPublicAppRateLimitPerMinute) {
await editPublicAppRateLimit()
}
if (
effectiveGuestJwt.pem !== initialGuestJwtPublicKey ||
effectiveGuestJwt.jwks !== initialGuestJwtJwksUrl
) {
await editGuestJwtKey()
}
if (workspaceDefaultAppPath !== initialWorkspaceDefaultAppPath) {
await editWorkspaceDefaultApp()
}
if (publicAppRateLimitPerMinute !== initialPublicAppRateLimitPerMinute) {
await editPublicAppRateLimit()
}
}
async function editGuestJwtKey(): Promise<void> {
@@ -2228,7 +2228,7 @@ export async function main(
<SettingCard
label="Guests"
description="Let anyone your identity provider authenticates open the apps set to Guests without a Windmill account. They join no workspace, see nothing else, and take no seat. Off by default. Turning it off stops guests immediately, even for apps already set to Guests."
description="Let anyone your identity provider authenticates, or a JWT your own backend signs (configured below), open the apps set to Guests without a Windmill account. They join no workspace, see nothing else, and take no seat. Off by default. Turning it off stops guests immediately, even for apps already set to Guests."
class="mt-6"
>
<Toggle