diff --git a/crates/cli/src/sync.rs b/crates/cli/src/sync.rs index 737f9cc01..f96fe9b69 100644 --- a/crates/cli/src/sync.rs +++ b/crates/cli/src/sync.rs @@ -251,7 +251,7 @@ pub async fn config_sync( } if provider.jwks_uri.is_none() { - error!("Provider has discovery disabled but no JWKS URI set"); + warn!("Provider has discovery disabled but no JWKS URI set"); } } diff --git a/crates/handlers/src/upstream_oauth2/callback.rs b/crates/handlers/src/upstream_oauth2/callback.rs index 846eb8c6b..ed5f0ac95 100644 --- a/crates/handlers/src/upstream_oauth2/callback.rs +++ b/crates/handlers/src/upstream_oauth2/callback.rs @@ -14,10 +14,9 @@ use axum_extra::response::Html; use hyper::StatusCode; use mas_axum_utils::{cookies::CookieJar, sentry::SentryEventID}; use mas_data_model::{UpstreamOAuthProvider, UpstreamOAuthProviderResponseMode}; +use mas_jose::claims::TokenHash; use mas_keystore::{Encrypter, Keystore}; -use mas_oidc_client::requests::{ - authorization_code::AuthorizationValidationData, jose::JwtVerificationData, -}; +use mas_oidc_client::requests::jose::JwtVerificationData; use mas_router::UrlBuilder; use mas_storage::{ upstream_oauth2::{ @@ -27,7 +26,7 @@ use mas_storage::{ BoxClock, BoxRepository, BoxRng, Clock, }; use mas_templates::{FormPostContext, Templates}; -use oauth2_types::errors::ClientErrorCode; +use oauth2_types::{errors::ClientErrorCode, requests::AccessTokenRequest}; use serde::{Deserialize, Serialize}; use serde_json::json; use thiserror::Error; @@ -88,9 +87,6 @@ pub(crate) enum RouteError { #[error("State parameter mismatch")] StateMismatch, - #[error("Missing ID token")] - MissingIDToken, - #[error("Could not extract subject from ID token")] ExtractSubject(#[source] minijinja::Error), @@ -125,7 +121,8 @@ impl_from_error_for_route!(mas_templates::TemplateError); impl_from_error_for_route!(mas_storage::RepositoryError); impl_from_error_for_route!(mas_oidc_client::error::DiscoveryError); impl_from_error_for_route!(mas_oidc_client::error::JwksError); -impl_from_error_for_route!(mas_oidc_client::error::TokenAuthorizationCodeError); +impl_from_error_for_route!(mas_oidc_client::error::TokenRequestError); +impl_from_error_for_route!(mas_oidc_client::error::IdTokenError); impl_from_error_for_route!(mas_oidc_client::error::UserInfoError); impl_from_error_for_route!(super::ProviderCredentialsError); impl_from_error_for_route!(super::cookie::UpstreamSessionNotFound); @@ -253,11 +250,6 @@ pub(crate) async fn handler( let mut lazy_metadata = LazyProviderInfos::new(&metadata_cache, &provider, &client); - // Fetch the JWKS - let jwks = - mas_oidc_client::requests::jose::fetch_jwks(&client, lazy_metadata.jwks_uri().await?) - .await?; - // Figure out the client credentials let client_credentials = client_credentials_for_provider( &provider, @@ -268,41 +260,72 @@ pub(crate) async fn handler( let redirect_uri = url_builder.upstream_oauth_callback(provider.id); - // TODO: all that should be borrowed - let validation_data = AuthorizationValidationData { - state: session.state_str.clone(), - nonce: session.nonce.clone(), - code_challenge_verifier: session.code_challenge_verifier.clone(), - redirect_uri, - }; + let token_response = mas_oidc_client::requests::token::request_access_token( + &client, + client_credentials, + lazy_metadata.token_endpoint().await?, + AccessTokenRequest::AuthorizationCode(oauth2_types::requests::AuthorizationCodeGrant { + code: code.clone(), + redirect_uri: Some(redirect_uri), + code_verifier: session.code_challenge_verifier.clone(), + }), + clock.now(), + &mut rng, + ) + .await?; - let verification_data = JwtVerificationData { - issuer: &provider.issuer, - jwks: &jwks, - // TODO: make that configurable - signing_algorithm: &mas_iana::jose::JsonWebSignatureAlg::Rs256, - client_id: &provider.client_id, - }; + let mut context = AttributeMappingContext::new(); + if let Some(id_token) = token_response.id_token.as_ref() { + // Fetch the JWKS + let jwks = + mas_oidc_client::requests::jose::fetch_jwks(&client, lazy_metadata.jwks_uri().await?) + .await?; - let (response, id_token_map) = - mas_oidc_client::requests::authorization_code::access_token_with_authorization_code( - &client, - client_credentials, - lazy_metadata.token_endpoint().await?, - code, - validation_data, - Some(verification_data), + let verification_data = JwtVerificationData { + issuer: &provider.issuer, + jwks: &jwks, + // TODO: make that configurable + signing_algorithm: &mas_iana::jose::JsonWebSignatureAlg::Rs256, + client_id: &provider.client_id, + }; + + // Decode and verify the ID token + let id_token = mas_oidc_client::requests::jose::verify_id_token( + id_token, + verification_data, + None, clock.now(), - &mut rng, - ) - .await?; + )?; - let (_header, id_token) = id_token_map - .clone() - .ok_or(RouteError::MissingIDToken)? - .into_parts(); + let (_headers, mut claims) = id_token.into_parts(); + + // Access token hash must match. + mas_jose::claims::AT_HASH + .extract_optional_with_options( + &mut claims, + TokenHash::new( + verification_data.signing_algorithm, + &token_response.access_token, + ), + ) + .map_err(mas_oidc_client::error::IdTokenError::from)?; + + // Code hash must match. + mas_jose::claims::C_HASH + .extract_optional_with_options( + &mut claims, + TokenHash::new(verification_data.signing_algorithm, &code), + ) + .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)?; + + context = context.with_id_token_claims(claims); + } - let mut context = AttributeMappingContext::new().with_id_token_claims(id_token); if let Some(extra_callback_parameters) = extra_callback_parameters.clone() { context = context.with_extra_callback_parameters(extra_callback_parameters); } @@ -312,9 +335,8 @@ pub(crate) async fn handler( mas_oidc_client::requests::userinfo::fetch_userinfo( &client, lazy_metadata.userinfo_endpoint().await?, - response.access_token.as_str(), - Some(verification_data), - &id_token_map.ok_or(RouteError::MissingIDToken)?, + token_response.access_token.as_str(), + None, ) .await? )) @@ -364,7 +386,7 @@ pub(crate) async fn handler( &clock, session, &link, - response.id_token, + token_response.id_token, extra_callback_parameters, userinfo, ) diff --git a/crates/oidc-client/src/requests/token.rs b/crates/oidc-client/src/requests/token.rs index adb2bd8d2..38e988495 100644 --- a/crates/oidc-client/src/requests/token.rs +++ b/crates/oidc-client/src/requests/token.rs @@ -7,7 +7,9 @@ //! Requests for the Token endpoint. use chrono::{DateTime, Utc}; +use http::header::ACCEPT; use mas_http::RequestBuilderExt; +use mime::APPLICATION_JSON; use oauth2_types::requests::{AccessTokenRequest, AccessTokenResponse}; use rand::Rng; use url::Url; @@ -48,7 +50,9 @@ pub async fn request_access_token( ) -> Result { tracing::debug!(?request, "Requesting access token..."); - let token_request = http_client.post(token_endpoint.as_str()); + let token_request = http_client + .post(token_endpoint.as_str()) + .header(ACCEPT, APPLICATION_JSON.as_ref()); let token_response = client_credentials .authenticated_form(token_request, &request, now, rng)? diff --git a/crates/oidc-client/src/requests/userinfo.rs b/crates/oidc-client/src/requests/userinfo.rs index 594516b30..fbcd27039 100644 --- a/crates/oidc-client/src/requests/userinfo.rs +++ b/crates/oidc-client/src/requests/userinfo.rs @@ -13,7 +13,6 @@ use std::collections::HashMap; use headers::{ContentType, HeaderMapExt, HeaderValue}; use http::header::ACCEPT; use mas_http::RequestBuilderExt; -use mas_jose::claims; use mime::Mime; use serde_json::Value; use url::Url; @@ -22,7 +21,6 @@ use super::jose::JwtVerificationData; use crate::{ error::{IdTokenError, ResponseExt, UserInfoError}, requests::jose::verify_signed_jwt, - types::IdToken, }; /// Obtain information about an authenticated end-user. @@ -59,7 +57,6 @@ pub async fn fetch_userinfo( userinfo_endpoint: &Url, access_token: &str, jwt_verification_data: Option>, - auth_id_token: &IdToken<'_>, ) -> Result, UserInfoError> { tracing::debug!("Obtaining user info…"); @@ -94,7 +91,7 @@ pub async fn fetch_userinfo( }); } - let mut claims = if let Some(verification_data) = jwt_verification_data { + let claims = if let Some(verification_data) = jwt_verification_data { let response_body = userinfo_response.text().await?; verify_signed_jwt(&response_body, verification_data) .map_err(IdTokenError::from)? @@ -104,18 +101,5 @@ pub async fn fetch_userinfo( userinfo_response.json().await? }; - let mut auth_claims = auth_id_token.payload().clone(); - - // Subject identifier must always be the same. - let sub = claims::SUB - .extract_required(&mut claims) - .map_err(IdTokenError::from)?; - let auth_sub = claims::SUB - .extract_required(&mut auth_claims) - .map_err(IdTokenError::from)?; - if sub != auth_sub { - return Err(IdTokenError::WrongSubjectIdentifier.into()); - } - Ok(claims) } diff --git a/crates/oidc-client/tests/it/requests/userinfo.rs b/crates/oidc-client/tests/it/requests/userinfo.rs index 7aea757e5..4e97eb590 100644 --- a/crates/oidc-client/tests/it/requests/userinfo.rs +++ b/crates/oidc-client/tests/it/requests/userinfo.rs @@ -4,24 +4,19 @@ // SPDX-License-Identifier: AGPL-3.0-only // Please see LICENSE in the repository root for full details. -use assert_matches::assert_matches; -use mas_oidc_client::{ - error::{IdTokenError, UserInfoError}, - requests::userinfo::fetch_userinfo, -}; +use mas_oidc_client::requests::userinfo::fetch_userinfo; use serde_json::json; use wiremock::{ matchers::{header, method, path}, Mock, ResponseTemplate, }; -use crate::{id_token, init_test, ACCESS_TOKEN, SUBJECT_IDENTIFIER}; +use crate::{init_test, ACCESS_TOKEN, SUBJECT_IDENTIFIER}; #[tokio::test] async fn pass_fetch_userinfo() { let (http_client, mock_server, issuer) = init_test().await; let userinfo_endpoint = issuer.join("userinfo").unwrap(); - let (auth_id_token, _) = id_token(issuer.as_str()); Mock::given(method("GET")) .and(path("/userinfo")) @@ -36,50 +31,9 @@ async fn pass_fetch_userinfo() { .mount(&mock_server) .await; - let claims = fetch_userinfo( - &http_client, - &userinfo_endpoint, - ACCESS_TOKEN, - None, - &auth_id_token, - ) - .await - .unwrap(); + let claims = fetch_userinfo(&http_client, &userinfo_endpoint, ACCESS_TOKEN, None) + .await + .unwrap(); assert_eq!(claims.get("email").unwrap(), "janedoe@example.com"); } - -#[tokio::test] -async fn fail_wrong_subject_identifier() { - let (http_client, mock_server, issuer) = init_test().await; - let userinfo_endpoint = issuer.join("userinfo").unwrap(); - let (auth_id_token, _) = id_token(issuer.as_str()); - - Mock::given(method("GET")) - .and(path("/userinfo")) - .and(header( - "authorization", - format!("Bearer {ACCESS_TOKEN}").as_str(), - )) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "sub": "wrong_subject_identifier", - "email": "janedoe@example.com", - }))) - .mount(&mock_server) - .await; - - let error = fetch_userinfo( - &http_client, - &userinfo_endpoint, - ACCESS_TOKEN, - None, - &auth_id_token, - ) - .await - .unwrap_err(); - - assert_matches!( - error, - UserInfoError::IdToken(IdTokenError::WrongSubjectIdentifier) - ); -}