letro/letro-authentication-service
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Don't generate and send a nonce for non-OIDC-compliant auth requests

Browse Source
This commit is contained in:
Quentin Gliech
2025-05-07 15:26:14 +02:00
parent 67f7d7cb8d
commit 955bd28590
11 changed files with 41 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
crates/data-model/src/upstream_oauth2/session.rs
View File

@@ -250,7 +250,7 @@ pub struct UpstreamOAuthAuthorizationSession {
pub provider_id: Ulid,
pub state_str: String,
pub code_challenge_verifier: Option<String>,
pub nonce: String,
pub nonce: Option<String>,
pub created_at: DateTime<Utc>,
}

9
crates/handlers/src/admin/v1/upstream_oauth_links/delete.rs
View File

@@ -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();

10
crates/handlers/src/upstream_oauth2/callback.rs
View File

@@ -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);
}

2
crates/handlers/src/upstream_oauth2/link.rs
View File

@@ -998,7 +998,7 @@ mod tests {
&provider,
"state".to_owned(),
None,
"nonce".to_owned(),
None,
)
.await
.unwrap();

22
crates/oidc-client/src/requests/authorization_code.rs
View File

@@ -191,7 +191,7 @@ pub struct AuthorizationValidationData {
pub state: String,
/// A string to mitigate replay attacks.
pub nonce: String,
pub nonce: Option<String>,
/// 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 {

6
crates/oidc-client/tests/it/requests/authorization_code.rs
View File

@@ -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()),
};

2
crates/storage-pg/.sqlx/query-37a124678323380357fa9d1375fd125fb35476ac3008e5adbd04a761d5edcd42.json generated
View File

@@ -80,7 +80,7 @@
true,
false,
true,
false,
true,
true,
true,
true,

8
crates/storage-pg/migrations/20250507131948_upstream_oauth_session_optional_nonce.sql Normal file
View File

@@ -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;

2
crates/storage-pg/src/upstream_oauth2/mod.rs
View File

@@ -108,7 +108,7 @@ mod tests {
&provider,
"some-state".to_owned(),
None,
"some-nonce".to_owned(),
Some("some-nonce".to_owned()),
)
.await
.unwrap();

4
crates/storage-pg/src/upstream_oauth2/session.rs
View File

@@ -38,7 +38,7 @@ struct SessionLookup {
upstream_oauth_link_id: Option<Uuid>,
state: String,
code_challenge_verifier: Option<String>,
nonce: String,
nonce: Option<String>,
id_token: Option<String>,
userinfo: Option<serde_json::Value>,
created_at: DateTime<Utc>,
@@ -191,7 +191,7 @@ impl UpstreamOAuthSessionRepository for PgUpstreamOAuthSessionRepository<'_> {
upstream_oauth_provider: &UpstreamOAuthProvider,
state_str: String,
code_challenge_verifier: Option<String>,
nonce: String,
nonce: Option<String>,
) -> Result<UpstreamOAuthAuthorizationSession, Self::Error> {
let created_at = clock.now();
let id = Ulid::from_datetime_with_source(created_at.into(), rng);

6
crates/storage/src/upstream_oauth2/session.rs
View File

@@ -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<String>,
nonce: String,
nonce: Option<String>,
) -> Result<UpstreamOAuthAuthorizationSession, Self::Error>;
/// 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<String>,
nonce: String,
nonce: Option<String>,
) -> Result<UpstreamOAuthAuthorizationSession, Self::Error>;
async fn complete_with_link(