From 955bd285908913388cced34b37c0a746eafdaa56 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 7 May 2025 15:26:14 +0200 Subject: [PATCH] Don't generate and send a nonce for non-OIDC-compliant auth requests --- .../data-model/src/upstream_oauth2/session.rs | 2 +- .../admin/v1/upstream_oauth_links/delete.rs | 9 +------- .../handlers/src/upstream_oauth2/callback.rs | 10 +++++---- crates/handlers/src/upstream_oauth2/link.rs | 2 +- .../src/requests/authorization_code.rs | 22 ++++++++++++------- .../tests/it/requests/authorization_code.rs | 6 ++--- ...d125fb35476ac3008e5adbd04a761d5edcd42.json | 2 +- ..._upstream_oauth_session_optional_nonce.sql | 8 +++++++ crates/storage-pg/src/upstream_oauth2/mod.rs | 2 +- .../storage-pg/src/upstream_oauth2/session.rs | 4 ++-- crates/storage/src/upstream_oauth2/session.rs | 6 ++--- 11 files changed, 41 insertions(+), 32 deletions(-) create mode 100644 crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql diff --git a/crates/data-model/src/upstream_oauth2/session.rs b/crates/data-model/src/upstream_oauth2/session.rs index ef3623cfc..c5b45234c 100644 --- a/crates/data-model/src/upstream_oauth2/session.rs +++ b/crates/data-model/src/upstream_oauth2/session.rs @@ -250,7 +250,7 @@ pub struct UpstreamOAuthAuthorizationSession { pub provider_id: Ulid, pub state_str: String, pub code_challenge_verifier: Option, - pub nonce: String, + pub nonce: Option, pub created_at: DateTime, } diff --git a/crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs b/crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs index 0c270c64b..403c1bc33 100644 --- a/crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs +++ b/crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs @@ -108,14 +108,7 @@ mod tests { // Pretend it was linked by an authorization session let session = repo .upstream_oauth_session() - .add( - &mut rng, - &state.clock, - &provider, - String::new(), - None, - String::new(), - ) + .add(&mut rng, &state.clock, &provider, String::new(), None, None) .await .unwrap(); diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index f9b85adc8..75e8d63a0 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/crates/handlers/src/upstream_oauth2/callback.rs @@ -356,10 +356,12 @@ pub(crate) async fn handler( ) .map_err(mas_oidc_client::error::IdTokenError::from)?; - // Nonce must match. - mas_jose::claims::NONCE - .extract_required_with_options(&mut claims, session.nonce.as_str()) - .map_err(mas_oidc_client::error::IdTokenError::from)?; + // Nonce must match if present. + if let Some(nonce) = session.nonce.as_deref() { + mas_jose::claims::NONCE + .extract_required_with_options(&mut claims, nonce) + .map_err(mas_oidc_client::error::IdTokenError::from)?; + } context = context.with_id_token_claims(claims); } diff --git a/crates/handlers/src/upstream_oauth2/link.rs b/crates/handlers/src/upstream_oauth2/link.rs index ce2138b2a..d95854faa 100644 --- a/crates/handlers/src/upstream_oauth2/link.rs +++ b/crates/handlers/src/upstream_oauth2/link.rs @@ -998,7 +998,7 @@ mod tests { &provider, "state".to_owned(), None, - "nonce".to_owned(), + None, ) .await .unwrap(); diff --git a/crates/oidc-client/src/requests/authorization_code.rs b/crates/oidc-client/src/requests/authorization_code.rs index 161f71127..850813d8b 100644 --- a/crates/oidc-client/src/requests/authorization_code.rs +++ b/crates/oidc-client/src/requests/authorization_code.rs @@ -191,7 +191,7 @@ pub struct AuthorizationValidationData { pub state: String, /// A string to mitigate replay attacks. - pub nonce: String, + pub nonce: Option, /// The URI where the end-user will be redirected after authorization. pub redirect_uri: Url, @@ -216,7 +216,7 @@ fn build_authorization_request( ) -> Result<(FullAuthorizationRequest, AuthorizationValidationData), AuthorizationError> { let AuthorizationRequestData { client_id, - mut scope, + scope, redirect_uri, code_challenge_methods_supported, display, @@ -229,9 +229,13 @@ fn build_authorization_request( response_mode, } = authorization_data; + let is_openid = scope.contains(&OPENID); + // Generate a random CSRF "state" token and a nonce. let state = Alphanumeric.sample_string(rng, 16); - let nonce = Alphanumeric.sample_string(rng, 16); + + // Generate a random nonce if we're in 'OpenID Connect' mode + let nonce = is_openid.then(|| Alphanumeric.sample_string(rng, 16)); // Use PKCE, whenever possible. let (pkce, code_challenge_verifier) = if code_challenge_methods_supported @@ -263,7 +267,7 @@ fn build_authorization_request( scope, state: Some(state.clone()), response_mode, - nonce: Some(nonce.clone()), + nonce: nonce.clone(), display, prompt, max_age, @@ -440,10 +444,12 @@ pub async fn access_token_with_authorization_code( .extract_optional_with_options(&mut claims, TokenHash::new(signing_alg, &code)) .map_err(IdTokenError::from)?; - // Nonce must match. - claims::NONCE - .extract_required_with_options(&mut claims, validation_data.nonce.as_str()) - .map_err(IdTokenError::from)?; + // Nonce must match if we have one. + if let Some(nonce) = validation_data.nonce.as_deref() { + claims::NONCE + .extract_required_with_options(&mut claims, nonce) + .map_err(IdTokenError::from)?; + } Some(id_token.into_owned()) } else { diff --git a/crates/oidc-client/tests/it/requests/authorization_code.rs b/crates/oidc-client/tests/it/requests/authorization_code.rs index 131db26e9..d0eb2a8c1 100644 --- a/crates/oidc-client/tests/it/requests/authorization_code.rs +++ b/crates/oidc-client/tests/it/requests/authorization_code.rs @@ -186,7 +186,7 @@ async fn pass_access_token_with_authorization_code() { let redirect_uri = Url::parse(REDIRECT_URI).unwrap(); let validation_data = AuthorizationValidationData { state: "some_state".to_owned(), - nonce: NONCE.to_owned(), + nonce: Some(NONCE.to_owned()), redirect_uri, code_challenge_verifier: Some(CODE_VERIFIER.to_owned()), }; @@ -244,7 +244,7 @@ async fn fail_access_token_with_authorization_code_wrong_nonce() { let redirect_uri = Url::parse(REDIRECT_URI).unwrap(); let validation_data = AuthorizationValidationData { state: "some_state".to_owned(), - nonce: "wrong_nonce".to_owned(), + nonce: Some("wrong_nonce".to_owned()), redirect_uri, code_challenge_verifier: Some(CODE_VERIFIER.to_owned()), }; @@ -306,7 +306,7 @@ async fn fail_access_token_with_authorization_code_no_id_token() { let nonce = "some_nonce".to_owned(); let validation_data = AuthorizationValidationData { state: "some_state".to_owned(), - nonce: nonce.clone(), + nonce: Some(nonce.clone()), redirect_uri, code_challenge_verifier: Some(CODE_VERIFIER.to_owned()), }; diff --git a/crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json b/crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json index eac08aed7..0e28ac022 100644 --- a/crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json +++ b/crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json @@ -80,7 +80,7 @@ true, false, true, - false, + true, true, true, true, diff --git a/crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql b/crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql new file mode 100644 index 000000000..1b637c91b --- /dev/null +++ b/crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql @@ -0,0 +1,8 @@ +-- Copyright 2025 New Vector Ltd. +-- +-- SPDX-License-Identifier: AGPL-3.0-only +-- Please see LICENSE in the repository root for full details. + +-- Make the nonce column optional on the upstream oauth sessions +ALTER TABLE "upstream_oauth_authorization_sessions" + ALTER COLUMN "nonce" DROP NOT NULL; diff --git a/crates/storage-pg/src/upstream_oauth2/mod.rs b/crates/storage-pg/src/upstream_oauth2/mod.rs index 2b2662df7..a5cda570b 100644 --- a/crates/storage-pg/src/upstream_oauth2/mod.rs +++ b/crates/storage-pg/src/upstream_oauth2/mod.rs @@ -108,7 +108,7 @@ mod tests { &provider, "some-state".to_owned(), None, - "some-nonce".to_owned(), + Some("some-nonce".to_owned()), ) .await .unwrap(); diff --git a/crates/storage-pg/src/upstream_oauth2/session.rs b/crates/storage-pg/src/upstream_oauth2/session.rs index d9cad86a7..594f3be4c 100644 --- a/crates/storage-pg/src/upstream_oauth2/session.rs +++ b/crates/storage-pg/src/upstream_oauth2/session.rs @@ -38,7 +38,7 @@ struct SessionLookup { upstream_oauth_link_id: Option, state: String, code_challenge_verifier: Option, - nonce: String, + nonce: Option, id_token: Option, userinfo: Option, created_at: DateTime, @@ -191,7 +191,7 @@ impl UpstreamOAuthSessionRepository for PgUpstreamOAuthSessionRepository<'_> { upstream_oauth_provider: &UpstreamOAuthProvider, state_str: String, code_challenge_verifier: Option, - nonce: String, + nonce: Option, ) -> Result { let created_at = clock.now(); let id = Ulid::from_datetime_with_source(created_at.into(), rng); diff --git a/crates/storage/src/upstream_oauth2/session.rs b/crates/storage/src/upstream_oauth2/session.rs index d87ee7d3a..c563fce5e 100644 --- a/crates/storage/src/upstream_oauth2/session.rs +++ b/crates/storage/src/upstream_oauth2/session.rs @@ -48,7 +48,7 @@ pub trait UpstreamOAuthSessionRepository: Send + Sync { /// upstream OAuth provider /// * `code_challenge_verifier`: the code challenge verifier used in this /// session, if PKCE is being used - /// * `nonce`: the `nonce` used in this session + /// * `nonce`: the `nonce` used in this session if in OIDC mode /// /// # Errors /// @@ -60,7 +60,7 @@ pub trait UpstreamOAuthSessionRepository: Send + Sync { upstream_oauth_provider: &UpstreamOAuthProvider, state: String, code_challenge_verifier: Option, - nonce: String, + nonce: Option, ) -> Result; /// Mark a session as completed and associate the given link @@ -122,7 +122,7 @@ repository_impl!(UpstreamOAuthSessionRepository: upstream_oauth_provider: &UpstreamOAuthProvider, state: String, code_challenge_verifier: Option, - nonce: String, + nonce: Option, ) -> Result; async fn complete_with_link(