From 589f666bf7fdd83565d2e574e08de6243227fb9f Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Sat, 11 Sep 2021 00:53:21 +0200 Subject: [PATCH] more error handling in token endpoint Also adds some OP metadatas to help with conformance --- .../src/filters/client.rs | 4 +- .../src/handlers/oauth2/discovery.rs | 23 +++++++++- .../src/handlers/oauth2/token.rs | 46 ++++++++++++++++--- oauth2-types/src/errors.rs | 17 ++++++- oauth2-types/src/oidc.rs | 6 ++- oauth2-types/src/requests.rs | 21 +++++++++ 6 files changed, 103 insertions(+), 14 deletions(-) diff --git a/matrix-authentication-service/src/filters/client.rs b/matrix-authentication-service/src/filters/client.rs index 5d45d29c7..21c0666c9 100644 --- a/matrix-authentication-service/src/filters/client.rs +++ b/matrix-authentication-service/src/filters/client.rs @@ -139,12 +139,12 @@ mod tests { config.clients.push(OAuth2ClientConfig { client_id: "public".to_string(), client_secret: None, - redirect_uris: None, + redirect_uris: Vec::new(), }); config.clients.push(OAuth2ClientConfig { client_id: "confidential".to_string(), client_secret: Some("secret".to_string()), - redirect_uris: None, + redirect_uris: Vec::new(), }); config } diff --git a/matrix-authentication-service/src/handlers/oauth2/discovery.rs b/matrix-authentication-service/src/handlers/oauth2/discovery.rs index bdfc830de..bd777ef29 100644 --- a/matrix-authentication-service/src/handlers/oauth2/discovery.rs +++ b/matrix-authentication-service/src/handlers/oauth2/discovery.rs @@ -14,7 +14,10 @@ use std::collections::HashSet; -use oauth2_types::{oidc::Metadata, requests::ResponseMode}; +use oauth2_types::{ + oidc::Metadata, + requests::{ClientAuthenticationMethod, GrantType, ResponseMode}, +}; use warp::{Filter, Rejection, Reply}; use crate::config::OAuth2Config; @@ -44,6 +47,21 @@ pub(super) fn filter( s }); + let grant_types_supported = Some({ + let mut s = HashSet::new(); + s.insert(GrantType::AuthorizationCode); + s.insert(GrantType::RefreshToken); + s + }); + + let token_endpoint_auth_methods_supported = Some({ + let mut s = HashSet::new(); + s.insert(ClientAuthenticationMethod::ClientSecretBasic); + s.insert(ClientAuthenticationMethod::ClientSecretPost); + s.insert(ClientAuthenticationMethod::None); + s + }); + let metadata = Metadata { authorization_endpoint: base.join("oauth2/authorize").ok(), token_endpoint: base.join("oauth2/token").ok(), @@ -55,7 +73,8 @@ pub(super) fn filter( scopes_supported: None, response_types_supported, response_modes_supported, - grant_types_supported: None, + grant_types_supported, + token_endpoint_auth_methods_supported, code_challenge_methods_supported: None, }; diff --git a/matrix-authentication-service/src/handlers/oauth2/token.rs b/matrix-authentication-service/src/handlers/oauth2/token.rs index 65c2b0a83..8e22fc0ab 100644 --- a/matrix-authentication-service/src/handlers/oauth2/token.rs +++ b/matrix-authentication-service/src/handlers/oauth2/token.rs @@ -16,9 +16,10 @@ use anyhow::Context; use chrono::Duration; use data_encoding::BASE64URL_NOPAD; use headers::{CacheControl, Pragma}; +use hyper::StatusCode; use jwt_compact::{Claims, Header, TimeOptions}; use oauth2_types::{ - errors::{InvalidGrant, OAuth2Error}, + errors::{InvalidGrant, OAuth2Error, OAuth2ErrorCode, UnauthorizedClient}, requests::{ AccessTokenRequest, AccessTokenResponse, AuthorizationCodeGrant, RefreshTokenGrant, }, @@ -29,7 +30,11 @@ use serde_with::skip_serializing_none; use sha2::{Digest, Sha256}; use sqlx::{pool::PoolConnection, Acquire, PgPool, Postgres}; use url::Url; -use warp::{Filter, Rejection, Reply}; +use warp::{ + reject::Reject, + reply::{json, with_status}, + Filter, Rejection, Reply, +}; use crate::{ config::{KeySet, OAuth2ClientConfig, OAuth2Config}, @@ -62,6 +67,23 @@ struct CustomClaims { c_hash: String, } +#[derive(Debug)] +struct Error { + json: serde_json::Value, + status: StatusCode, +} + +impl Reject for Error {} + +fn error(e: E) -> Result +where + E: OAuth2ErrorCode + 'static, +{ + let status = e.status(); + let json = serde_json::to_value(e.into_response()).wrap_error()?; + Err(Error { json, status }.into()) +} + pub fn filter( pool: &PgPool, oauth2_config: &OAuth2Config, @@ -74,6 +96,15 @@ pub fn filter( .and(warp::any().map(move || issuer.clone())) .and(with_connection(pool)) .and_then(token) + .recover(recover) +} + +async fn recover(rejection: Rejection) -> Result { + if let Some(Error { json, status }) = rejection.find::() { + Ok(with_status(warp::reply::json(json), *status)) + } else { + Err(rejection) + } } async fn token( @@ -87,15 +118,15 @@ async fn token( let reply = match req { AccessTokenRequest::AuthorizationCode(grant) => { let reply = authorization_code_grant(&grant, &client, &keys, issuer, &mut conn).await?; - warp::reply::json(&reply) + json(&reply) } AccessTokenRequest::RefreshToken(grant) => { let reply = refresh_token_grant(&grant, &client, &mut conn).await?; - warp::reply::json(&reply) + json(&reply) } _ => { let reply = InvalidGrant.into_response(); - warp::reply::json(&reply) + json(&reply) } }; @@ -125,7 +156,7 @@ async fn authorization_code_grant( let mut txn = conn.begin().await.wrap_error()?; let code = lookup_code(&mut txn, &grant.code).await.wrap_error()?; if client.client_id != code.client_id { - return Err(anyhow::anyhow!("invalid client")).wrap_error(); + return error(UnauthorizedClient); } // TODO: verify PKCE @@ -194,7 +225,8 @@ async fn refresh_token_grant( .wrap_error()?; if client.client_id != refresh_token_lookup.client_id { - return Err(anyhow::anyhow!("invalid client")).wrap_error(); + // As per https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 + return error(InvalidGrant); } let ttl = Duration::minutes(5); diff --git a/oauth2-types/src/errors.rs b/oauth2-types/src/errors.rs index a7d6cf2f4..13d4df7d8 100644 --- a/oauth2-types/src/errors.rs +++ b/oauth2-types/src/errors.rs @@ -49,11 +49,24 @@ pub trait OAuth2Error: std::fmt::Debug + Send + Sync { } } -trait OAuth2ErrorCode: OAuth2Error { +pub trait OAuth2ErrorCode: OAuth2Error + 'static { /// The HTTP status code that must be returned by this error fn status(&self) -> StatusCode; } +impl OAuth2Error for &Box { + fn error(&self) -> &'static str { + self.as_ref().error() + } + fn description(&self) -> Option { + self.as_ref().description() + } + + fn uri(&self) -> Option { + self.as_ref().uri() + } +} + #[derive(Debug)] pub struct ErrorResponse(Box); @@ -112,7 +125,7 @@ impl Serialize for ErrorResponse { macro_rules! oauth2_error_def { ($name:ident) => { - #[derive(Debug)] + #[derive(Debug, Clone)] pub struct $name; }; } diff --git a/oauth2-types/src/oidc.rs b/oauth2-types/src/oidc.rs index 22cc5b5f7..3d5aab343 100644 --- a/oauth2-types/src/oidc.rs +++ b/oauth2-types/src/oidc.rs @@ -20,7 +20,7 @@ use url::Url; use crate::{ pkce::CodeChallengeMethod, - requests::{GrantType, ResponseMode}, + requests::{ClientAuthenticationMethod, GrantType, ResponseMode}, }; // TODO: https://datatracker.ietf.org/doc/html/rfc8414#section-2 @@ -61,6 +61,10 @@ pub struct Metadata { /// this authorization server supports. pub grant_types_supported: Option>, + /// JSON array containing a list of client authentication methods supported + /// by this token endpoint. + pub token_endpoint_auth_methods_supported: Option>, + /// PKCE code challenge methods supported by this authorization server pub code_challenge_methods_supported: Option>, diff --git a/oauth2-types/src/requests.rs b/oauth2-types/src/requests.rs index a08ed3b96..7e318c6a8 100644 --- a/oauth2-types/src/requests.rs +++ b/oauth2-types/src/requests.rs @@ -70,6 +70,27 @@ pub enum ResponseMode { FormPost, } +#[derive( + Debug, + Hash, + PartialEq, + Eq, + PartialOrd, + Ord, + Clone, + Copy, + Display, + FromStr, + Serialize, + Deserialize, +)] +#[serde(rename_all = "snake_case")] +pub enum ClientAuthenticationMethod { + None, + ClientSecretPost, + ClientSecretBasic, +} + #[derive( Debug, Hash,