From 8778e1bc45f35ea1abca30709ff38fac598248fa Mon Sep 17 00:00:00 2001 From: Maurus Decimus <11444311+mdecimus@users.noreply.github.com> Date: Wed, 17 Jun 2026 19:04:01 +0200 Subject: [PATCH] OAuth client secret verification for confidential clients --- CHANGELOG.md | 1 + crates/common/src/auth/oauth/registration.rs | 14 +- crates/http/src/auth/oauth/auth.rs | 2 +- crates/http/src/auth/oauth/registration.rs | 73 +++++- crates/http/src/auth/oauth/token.rs | 56 ++++- crates/jmap/src/registry/set.rs | 22 +- tests/src/system/mod.rs | 8 +- tests/src/system/oidc.rs | 246 ++++++++++++++++++- 8 files changed, 400 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d129d57..bc0c0e3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ If you are upgrading from v0.16.x, replace the binary (or run `docker pull`). If ## Added - OAuth Profile for Open Public Clients ([draft-ietf-mailmaint-oauth-public](https://datatracker.ietf.org/doc/draft-ietf-mailmaint-oauth-public/)) +- OAuth client secret verification for confidential clients. ## Changed diff --git a/crates/common/src/auth/oauth/registration.rs b/crates/common/src/auth/oauth/registration.rs index 60e02532..6e4b5be3 100644 --- a/crates/common/src/auth/oauth/registration.rs +++ b/crates/common/src/auth/oauth/registration.rs @@ -212,16 +212,16 @@ pub fn validate_redirect_uri(uri: &str) -> Result<(), ClientRegistrationError> { return Err(ClientRegistrationError::invalid_redirect_uri( "Redirect URI must not contain a fragment.", )); - } - if uri.contains("..") { + } else if uri.contains("..") { return Err(ClientRegistrationError::invalid_redirect_uri( "Redirect URI must not contain consecutive dots.", )); - } - if uri.starts_with("http://127.0.0.1/") || uri.starts_with("http://[::1]/") { + } else if uri.starts_with("http://127.0.0.1/") + || uri.starts_with("http://[::1]/") + || uri.starts_with("https://") + { return Ok(()); - } - if let Some((scheme, _)) = uri.split_once(':') + } else if let Some((scheme, _)) = uri.split_once(':') && scheme.contains('.') && scheme .as_bytes() @@ -235,7 +235,7 @@ pub fn validate_redirect_uri(uri: &str) -> Result<(), ClientRegistrationError> { } Err(ClientRegistrationError::invalid_redirect_uri( - "Redirect URI must be a loopback (http://127.0.0.1/, http://[::1]/) or private-use scheme URI.", + "Redirect URI must be an https URL, a loopback (http://127.0.0.1/, http://[::1]/) or a private-use scheme URI.", )) } diff --git a/crates/http/src/auth/oauth/auth.rs b/crates/http/src/auth/oauth/auth.rs index a196ef01..41c54d95 100644 --- a/crates/http/src/auth/oauth/auth.rs +++ b/crates/http/src/auth/oauth/auth.rs @@ -219,7 +219,7 @@ impl OAuthApiHandler for Server { } grant_scope(scope.as_deref(), meta.scope_mask) } - None => grant_scope(scope.as_deref(), u64::MAX), + None => scope, }; // Validate Resource Indicators (RFC 8707) diff --git a/crates/http/src/auth/oauth/registration.rs b/crates/http/src/auth/oauth/registration.rs index b430f464..72e4a10f 100644 --- a/crates/http/src/auth/oauth/registration.rs +++ b/crates/http/src/auth/oauth/registration.rs @@ -19,11 +19,12 @@ use common::{ }, }, }; +use directory::core::secret::{hash_secret, verify_secret_hash}; use http_proto::{request::fetch_body, *}; use hyper::StatusCode; use registry::schema::{ - enums::Permission, - prelude::{ObjectType, Property}, + enums::{PasswordHashAlgorithm, Permission}, + prelude::{ObjectType, Property, UTCDateTime}, structs::OAuthClient, }; use std::future::Future; @@ -48,6 +49,12 @@ pub trait ClientRegistrationHandler: Sync + Send { redirect_uri: Option<&str>, account_id: u32, ) -> impl Future>> + Send; + + fn verify_client_secret( + &self, + client_id: &str, + client_secret: Option<&str>, + ) -> impl Future>> + Send; } impl ClientRegistrationHandler for Server { async fn handle_oauth_registration_request( @@ -143,6 +150,19 @@ impl ClientRegistrationHandler for Server { .map(|ch| char::from(ch.to_ascii_lowercase())) .collect::(); + // Generate client secret + let client_secret = rng() + .sample_iter(Alphanumeric) + .take(48) + .map(char::from) + .collect::(); + let secret_hash = hash_secret( + PasswordHashAlgorithm::Argon2id, + client_secret.clone().into_bytes(), + ) + .await + .caused_by(trc::location!())?; + let result = self .registry() .write(RegistryWrite::insert( @@ -153,6 +173,8 @@ impl ClientRegistrationHandler for Server { member_tenant_id: tenant_id.map(|id| Id::new(id as u64)), redirect_uris: request.redirect_uris.clone().into(), logo: request.logo_uri.clone(), + secret: Some(secret_hash), + created_at: UTCDateTime::now(), ..Default::default() } .into(), @@ -178,6 +200,9 @@ impl ClientRegistrationHandler for Server { StatusCode::CREATED, ClientRegistrationResponse { client_id, + client_secret: Some(client_secret), + client_id_issued_at: Some(now()), + client_secret_expires_at: Some(0), request, ..Default::default() }, @@ -253,6 +278,50 @@ impl ClientRegistrationHandler for Server { ErrorType::InvalidRequest })) } + + async fn verify_client_secret( + &self, + client_id: &str, + client_secret: Option<&str>, + ) -> trc::Result> { + // Stateless and unregistered clients have no secret to verify + if decode_client_id(self.core.oauth.oauth_key.as_bytes(), client_id).is_some() { + return Ok(None); + } + let Some(client_id) = self + .registry() + .primary_key( + ObjectType::OAuthClient.into(), + Property::ClientId, + client_id.as_bytes().to_vec(), + ) + .await? + else { + return Ok(None); + }; + let Some(client) = self + .registry() + .object::(client_id.id()) + .await + .caused_by(trc::location!())? + else { + return Ok(None); + }; + + match client.secret.as_deref() { + Some(hash) if !hash.is_empty() => match client_secret { + Some(secret) + if verify_secret_hash(hash, secret.as_bytes()) + .await + .caused_by(trc::location!())? => + { + Ok(None) + } + _ => Ok(Some(ErrorType::InvalidClient)), + }, + _ => Ok(None), + } + } } fn registration_error(error: ClientRegistrationError) -> HttpResponse { diff --git a/crates/http/src/auth/oauth/token.rs b/crates/http/src/auth/oauth/token.rs index 805d8126..5b499807 100644 --- a/crates/http/src/auth/oauth/token.rs +++ b/crates/http/src/auth/oauth/token.rs @@ -8,7 +8,11 @@ use super::{ ArchivedOAuthStatus, ArchivedPkceCodeChallenge, ErrorType, FormData, MAX_POST_LEN, OAuthCode, OAuthResponse, OAuthStatus, TokenResponse, registration::ClientRegistrationHandler, }; -use base64::{Engine, engine::general_purpose::URL_SAFE_NO_PAD}; +use crate::auth::authenticate::HttpHeaders; +use base64::{ + Engine, + engine::general_purpose::{STANDARD, URL_SAFE_NO_PAD}, +}; use common::{ KV_OAUTH, Server, auth::{ @@ -19,7 +23,7 @@ use common::{ use http_proto::*; use hyper::StatusCode; use sha2::{Digest, Sha256}; -use std::future::Future; +use std::{borrow::Cow, future::Future}; use store::{ dispatch::lookup::KeyValue, write::{AlignedBytes, Archive}, @@ -63,6 +67,7 @@ impl TokenHandler for Server { // Parse form let params = FormData::from_request(req, MAX_POST_LEN, session.session_id).await?; let grant_type = params.get("grant_type").unwrap_or_default(); + let (client_id_cred, client_secret_cred) = client_credentials(req, ¶ms); let mut response = TokenResponse::error(ErrorType::InvalidGrant); @@ -71,7 +76,7 @@ impl TokenHandler for Server { if grant_type.eq_ignore_ascii_case("authorization_code") { response = if let (Some(code), Some(client_id), Some(redirect_uri)) = ( params.get("code"), - params.get("client_id"), + client_id_cred.as_deref(), params.get("redirect_uri"), ) { // Obtain code @@ -102,6 +107,11 @@ impl TokenHandler for Server { .await? { TokenResponse::error(error) + } else if let Some(error) = self + .verify_client_secret(client_id, client_secret_cred.as_deref()) + .await? + { + TokenResponse::error(error) } else { // Mark this token as issued self.in_memory_store() @@ -212,6 +222,17 @@ impl TokenHandler for Server { } } else if grant_type.eq_ignore_ascii_case("refresh_token") { if let Some(refresh_token) = params.get("refresh_token") { + if let Some(client_id) = client_id_cred.as_deref() + && let Some(error) = self + .verify_client_secret(client_id, client_secret_cred.as_deref()) + .await? + { + return Ok(JsonResponse::with_status( + StatusCode::BAD_REQUEST, + TokenResponse::error(error), + ) + .into_http_response()); + } response = match self .validate_access_token(GrantType::RefreshToken.into(), refresh_token) .await @@ -352,6 +373,35 @@ impl TokenHandler for Server { } } +fn client_credentials<'x>( + req: &'x HttpRequest, + params: &'x FormData, +) -> (Option>, Option>) { + let mut client_id = params.get("client_id").map(Cow::Borrowed); + let mut client_secret = params.get("client_secret").map(Cow::Borrowed); + + if (client_id.is_none() || client_secret.is_none()) + && let Some((id, secret)) = req + .authorization_basic() + .and_then(|token| STANDARD.decode(token).ok()) + .and_then(|bytes| String::from_utf8(bytes).ok()) + .and_then(|creds| { + creds + .split_once(':') + .map(|(id, secret)| (id.to_string(), secret.to_string())) + }) + { + if client_id.is_none() { + client_id = Some(Cow::Owned(id)); + } + if client_secret.is_none() { + client_secret = Some(Cow::Owned(secret)); + } + } + + (client_id, client_secret) +} + fn verify_pkce(stored: &ArchivedPkceCodeChallenge, verifier: Option<&str>) -> bool { let is_valid_pkce_challenge = |challenge: &str| { (43..=128).contains(&challenge.len()) diff --git a/crates/jmap/src/registry/set.rs b/crates/jmap/src/registry/set.rs index ab72e643..7bdf46ae 100644 --- a/crates/jmap/src/registry/set.rs +++ b/crates/jmap/src/registry/set.rs @@ -31,6 +31,7 @@ use common::{ Server, auth::AccessToken, cache::invalidate::CacheInvalidationBuilder, expr::if_block::BootstrapExprExt, ipc::CacheInvalidation, }; +use directory::core::secret::{hash_secret, is_password_hash}; use http_proto::HttpSessionData; use jmap_proto::{ error::set::{SetError, SetErrorType}, @@ -463,8 +464,25 @@ impl RegistrySet for Server { ObjectInner::MailingList(_) if is_create => { validate_tenant_quota(&set, TenantStorageQuota::MaxMailingLists).await? } - ObjectInner::OAuthClient(_) if is_create => { - validate_tenant_quota(&set, TenantStorageQuota::MaxOauthClients).await? + ObjectInner::OAuthClient(client) => { + if let Some(secret) = client.secret.as_mut() + && !secret.is_empty() + && !(matches!(secret.as_bytes().first(), Some(&b'$' | &b'{')) + && is_password_hash(secret)) + { + *secret = hash_secret( + set.server.core.network.security.password_hash_algorithm, + std::mem::take(secret).into_bytes(), + ) + .await + .caused_by(trc::location!())?; + } + if is_create { + validate_tenant_quota(&set, TenantStorageQuota::MaxOauthClients) + .await? + } else { + Ok(ObjectResponse::default()) + } } ObjectInner::Directory(_) if is_create => { validate_tenant_quota(&set, TenantStorageQuota::MaxDirectories).await? diff --git a/tests/src/system/mod.rs b/tests/src/system/mod.rs index 260dfad7..037f165f 100644 --- a/tests/src/system/mod.rs +++ b/tests/src/system/mod.rs @@ -58,10 +58,10 @@ pub async fn system_tests() { .await; test.insert_account(admin); - /*directory::test(&test).await; - authentication::test(&test).await;*/ + directory::test(&test).await; + authentication::test(&test).await; oidc::test(&mut test).await; - /*authorization::test(&mut test).await; + authorization::test(&mut test).await; tenant::test(&mut test).await; security::test(&mut test).await; quota::test(&mut test).await; @@ -70,7 +70,7 @@ pub async fn system_tests() { crypto::test(&mut test).await; antispam::test(&mut test).await; archiving::test(&mut test).await; - task::test(&mut test).await;*/ + task::test(&mut test).await; if test.is_reset() { test.temp_dir.delete(); diff --git a/tests/src/system/oidc.rs b/tests/src/system/oidc.rs index e229faf7..e34b739b 100644 --- a/tests/src/system/oidc.rs +++ b/tests/src/system/oidc.rs @@ -17,7 +17,9 @@ use bytes::Bytes; use common::auth::oauth::{ introspect::OAuthIntrospect, oidc::StandardClaims, - registration::{ClientRegistrationRequest, ClientRegistrationResponse}, + registration::{ + ClientRegistrationRequest, ClientRegistrationResponse, TokenEndpointAuthMethod, + }, }; use http::auth::oauth::{ DeviceAuthResponse, ErrorType, TokenResponse, @@ -31,7 +33,7 @@ use jmap_client::{ use registry::schema::{ enums::JwtSignatureAlgorithm, prelude::{ObjectType, Property}, - structs::{OidcProvider, SecretText, SecretTextValue}, + structs::{OAuthClient, OidcProvider, SecretText, SecretTextValue}, }; use serde::{Serialize, de::DeserializeOwned}; use std::time::{Duration, Instant}; @@ -222,7 +224,7 @@ pub async fn test(test: &mut TestServer) { // Dynamic Client Registration: invalid redirect URIs are rejected (RFC 7591 ยง3.2.2) for bad_uri in [ - "https://example.com/cb", + "http://example.com/cb", "http://127.0.0.1/cb#frag", "http://127.0.0.1/../cb", ] { @@ -509,6 +511,176 @@ pub async fn test(test: &mut TestServer) { .await; pop3.assert_read(crate::utils::pop3::ResponseType::Ok).await; + // ------------------------ + // Confidential client with client_secret + // ------------------------ + + // Registering a confidential client requires authentication and returns a + // generated client_secret exactly once. Web (https) redirect URIs are allowed. + let confidential_redirect = "https://confidential.example.org/callback"; + let confidential: ClientRegistrationResponse = post_json_basic( + &metadata.registration_endpoint, + "admin", + "popolna_zapora", + &ClientRegistrationRequest { + redirect_uris: vec![confidential_redirect.to_string()], + scope: Some(PROFILE_SCOPE.to_string()), + token_endpoint_auth_method: Some(TokenEndpointAuthMethod::ClientSecretPost), + ..Default::default() + }, + ) + .await; + let confidential_id = confidential.client_id; + let confidential_secret = confidential + .client_secret + .expect("confidential client must receive a client_secret"); + assert!( + !confidential_id.starts_with("swc1."), + "confidential client id must be registry-backed, got {confidential_id}" + ); + assert!( + confidential_secret.len() >= 40, + "client secret is too short: {confidential_secret}" + ); + + // Registering a confidential client anonymously must be rejected + let (status, _) = post_json_raw( + &metadata.registration_endpoint, + &ClientRegistrationRequest { + redirect_uris: vec![confidential_redirect.to_string()], + token_endpoint_auth_method: Some(TokenEndpointAuthMethod::ClientSecretBasic), + ..Default::default() + }, + ) + .await; + assert_ne!( + status, 201, + "anonymous confidential client registration must be rejected" + ); + + let base_params = || { + AHashMap::from_iter([ + ("client_id".to_string(), confidential_id.to_string()), + ( + "redirect_uri".to_string(), + confidential_redirect.to_string(), + ), + ("grant_type".to_string(), "authorization_code".to_string()), + ]) + }; + + // A confidential client that omits its secret must be rejected + let mut params = base_params(); + params.insert( + "code".to_string(), + obtain_auth_code(&http, &confidential_id, confidential_redirect).await, + ); + assert_eq!( + post::(&metadata.token_endpoint, ¶ms).await, + TokenResponse::Error { + error: ErrorType::InvalidClient + }, + "token request without client_secret must be rejected" + ); + + // A confidential client that presents a wrong secret must be rejected + let mut params = base_params(); + params.insert( + "code".to_string(), + obtain_auth_code(&http, &confidential_id, confidential_redirect).await, + ); + params.insert("client_secret".to_string(), "not-the-secret".to_string()); + assert_eq!( + post::(&metadata.token_endpoint, ¶ms).await, + TokenResponse::Error { + error: ErrorType::InvalidClient + }, + "token request with a wrong client_secret must be rejected" + ); + + // The correct secret in the request body (client_secret_post) grants a usable token + let mut params = base_params(); + params.insert( + "code".to_string(), + obtain_auth_code(&http, &confidential_id, confidential_redirect).await, + ); + params.insert("client_secret".to_string(), confidential_secret.to_string()); + let (token, _, _) = unwrap_token_response(post(&metadata.token_endpoint, ¶ms).await); + let confidential_client = Client::new() + .credentials(Credentials::bearer(&token)) + .accept_invalid_certs(true) + .follow_redirects(["127.0.0.1"]) + .connect("https://127.0.0.1:8899") + .await + .unwrap(); + assert_eq!( + confidential_client.default_account_id(), + user_id.to_string() + ); + + // The correct secret in the Authorization header (client_secret_basic) also works + let mut params = base_params(); + params.remove("client_id"); + params.insert( + "code".to_string(), + obtain_auth_code(&http, &confidential_id, confidential_redirect).await, + ); + let granted: TokenResponse = post_form_basic( + &metadata.token_endpoint, + &confidential_id, + &confidential_secret, + ¶ms, + ) + .await; + unwrap_token_response(granted); + + // A confidential client created through the management API must have its + // secret hashed before storage; authenticating with the plaintext secret + // only succeeds if the stored value is a verifiable hash. + let managed_secret = "managed-client-secret-abcdefghijklmnopqrstuvwxyz"; + let managed_id = "managed-confidential-client"; + admin + .registry_create_object(OAuthClient { + client_id: managed_id.to_string(), + redirect_uris: vec![confidential_redirect.to_string()].into(), + secret: Some(managed_secret.to_string()), + ..Default::default() + }) + .await; + + let managed_params = || { + AHashMap::from_iter([ + ("client_id".to_string(), managed_id.to_string()), + ( + "redirect_uri".to_string(), + confidential_redirect.to_string(), + ), + ("grant_type".to_string(), "authorization_code".to_string()), + ]) + }; + + let mut params = managed_params(); + params.insert( + "code".to_string(), + obtain_auth_code(&http, managed_id, confidential_redirect).await, + ); + params.insert("client_secret".to_string(), "wrong-secret".to_string()); + assert_eq!( + post::(&metadata.token_endpoint, ¶ms).await, + TokenResponse::Error { + error: ErrorType::InvalidClient + }, + "management-api client must reject a wrong secret" + ); + + let mut params = managed_params(); + params.insert( + "code".to_string(), + obtain_auth_code(&http, managed_id, confidential_redirect).await, + ); + params.insert("client_secret".to_string(), managed_secret.to_string()); + unwrap_token_response(post(&metadata.token_endpoint, ¶ms).await); + // ------------------------ // Device code flow // ------------------------ @@ -734,6 +906,74 @@ async fn post_json( .unwrap() } +async fn post_json_basic( + url: &str, + username: &str, + password: &str, + body: &impl Serialize, +) -> D { + let response = reqwest::Client::builder() + .timeout(Duration::from_millis(500)) + .danger_accept_invalid_certs(true) + .build() + .unwrap_or_default() + .post(url) + .basic_auth(username, Some(password)) + .body(serde_json::to_string(body).unwrap().into_bytes()) + .send() + .await + .unwrap() + .bytes() + .await + .unwrap(); + serde_json::from_slice(&response).unwrap() +} + +async fn post_form_basic( + url: &str, + username: &str, + password: &str, + params: &AHashMap, +) -> T { + let response = reqwest::Client::builder() + .timeout(Duration::from_millis(500)) + .danger_accept_invalid_certs(true) + .build() + .unwrap_or_default() + .post(url) + .basic_auth(username, Some(password)) + .form(params) + .send() + .await + .unwrap() + .bytes() + .await + .unwrap(); + serde_json::from_slice(&response).unwrap() +} + +async fn obtain_auth_code(http: &HttpRequest, client_id: &str, redirect_uri: &str) -> String { + http.post::( + "/api/auth", + &LoginRequest::AuthCode { + account_name: "user@example.org".to_string(), + account_secret: "this is a very strong password".to_string(), + mfa_token: None, + client_id: client_id.to_string(), + redirect_uri: redirect_uri.to_string().into(), + nonce: None, + scope: Some(PROFILE_SCOPE.to_string()), + code_challenge: None, + code_challenge_method: None, + state: None, + resource: vec![], + }, + ) + .await + .unwrap() + .unwrap_code() +} + async fn post_json_raw(url: &str, body: &impl Serialize) -> (u16, serde_json::Value) { let response = reqwest::Client::builder() .timeout(Duration::from_millis(500))