OAuth client secret verification for confidential clients

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
 

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.",
     ))
 }
 

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)

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<Output = trc::Result<Option<ErrorType>>> + Send;
+
+    fn verify_client_secret(
+        &self,
+        client_id: &str,
+        client_secret: Option<&str>,
+    ) -> impl Future<Output = trc::Result<Option<ErrorType>>> + 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::<String>();
 
+        // Generate client secret
+        let client_secret = rng()
+            .sample_iter(Alphanumeric)
+            .take(48)
+            .map(char::from)
+            .collect::<String>();
+        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<Option<ErrorType>> {
+        // 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::<OAuthClient>(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 {

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, &params);
 
         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<Cow<'x, str>>, Option<Cow<'x, str>>) {
+    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())

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?

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

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::<TokenResponse>(&metadata.token_endpoint, &params).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::<TokenResponse>(&metadata.token_endpoint, &params).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, &params).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,
+        &params,
+    )
+    .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::<TokenResponse>(&metadata.token_endpoint, &params).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, &params).await);
+
     // ------------------------
     // Device code flow
     // ------------------------
@@ -734,6 +906,74 @@ async fn post_json<D: DeserializeOwned>(
     .unwrap()
 }
 
+async fn post_json_basic<D: DeserializeOwned>(
+    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<T: DeserializeOwned>(
+    url: &str,
+    username: &str,
+    password: &str,
+    params: &AHashMap<String, String>,
+) -> 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::<LoginResponse>(
+        "/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))