diff --git a/crates/data-model/src/lib.rs b/crates/data-model/src/lib.rs index a7539293..0475878e 100644 --- a/crates/data-model/src/lib.rs +++ b/crates/data-model/src/lib.rs @@ -36,7 +36,7 @@ pub use self::{ }, oauth2::{ AuthorizationCode, AuthorizationGrant, AuthorizationGrantStage, Client, - InvalidRedirectUriError, JwksOrJwksUri, Pkce, Session, + InvalidRedirectUriError, JwksOrJwksUri, Pkce, PkceVerificationError, Session, }, tokens::{AccessToken, RefreshToken, TokenFormatError, TokenType}, traits::{StorageBackend, StorageBackendMarker}, diff --git a/crates/data-model/src/oauth2/authorization_grant.rs b/crates/data-model/src/oauth2/authorization_grant.rs index efa0108e..5ae77b4e 100644 --- a/crates/data-model/src/oauth2/authorization_grant.rs +++ b/crates/data-model/src/oauth2/authorization_grant.rs @@ -24,6 +24,21 @@ use url::Url; use super::{client::Client, session::Session}; use crate::{traits::StorageBackend, StorageBackendMarker}; +#[derive(Debug, Error)] +pub enum PkceVerificationError { + #[error("code_verifier should be at least 43 characters long")] + TooShort, + + #[error("code_verifier should be at most 128 characters long")] + TooLong, + + #[error("code_verifier contains invalid characters")] + InvalidCharacters, + + #[error("challenge verification failed")] + VerificationFailed, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub struct Pkce { pub challenge_method: PkceCodeChallengeMethod, @@ -39,9 +54,27 @@ impl Pkce { } } - #[must_use] - pub fn verify(&self, verifier: &str) -> bool { - self.challenge_method.verify(&self.challenge, verifier) + pub fn verify(&self, verifier: &str) -> Result<(), PkceVerificationError> { + if verifier.len() < 43 { + return Err(PkceVerificationError::TooShort); + } + + if verifier.len() > 43 { + return Err(PkceVerificationError::TooLong); + } + + if verifier + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '.' || c == '_' || c == '~') + { + return Err(PkceVerificationError::InvalidCharacters); + } + + if !self.challenge_method.verify(&self.challenge, verifier) { + return Err(PkceVerificationError::VerificationFailed); + } + + Ok(()) } } diff --git a/crates/data-model/src/oauth2/mod.rs b/crates/data-model/src/oauth2/mod.rs index ef512260..fdf11254 100644 --- a/crates/data-model/src/oauth2/mod.rs +++ b/crates/data-model/src/oauth2/mod.rs @@ -17,7 +17,9 @@ pub(self) mod client; pub(self) mod session; pub use self::{ - authorization_grant::{AuthorizationCode, AuthorizationGrant, AuthorizationGrantStage, Pkce}, + authorization_grant::{ + AuthorizationCode, AuthorizationGrant, AuthorizationGrantStage, Pkce, PkceVerificationError, + }, client::{Client, InvalidRedirectUriError, JwksOrJwksUri}, session::Session, }; diff --git a/crates/handlers/src/oauth2/token.rs b/crates/handlers/src/oauth2/token.rs index ce5dcd74..2d6d2fc4 100644 --- a/crates/handlers/src/oauth2/token.rs +++ b/crates/handlers/src/oauth2/token.rs @@ -22,7 +22,7 @@ use headers::{CacheControl, HeaderMap, HeaderMapExt, Pragma}; use hyper::StatusCode; use mas_axum_utils::client_authorization::{ClientAuthorization, CredentialsVerificationError}; use mas_config::Encrypter; -use mas_data_model::{AuthorizationGrantStage, Client, TokenType}; +use mas_data_model::{AuthorizationGrantStage, Client, PkceVerificationError, TokenType}; use mas_iana::jose::JsonWebSignatureAlg; use mas_jose::{ claims::{self, ClaimError}, @@ -86,6 +86,9 @@ pub(crate) enum RouteError { #[error("bad request")] BadRequest, + #[error("pkce verification failed")] + PkceVerification(#[from] PkceVerificationError), + #[error("client not found")] ClientNotFound, @@ -129,6 +132,10 @@ impl IntoResponse for RouteError { (StatusCode::INTERNAL_SERVER_ERROR, Json(SERVER_ERROR)) } Self::BadRequest => (StatusCode::BAD_REQUEST, Json(INVALID_REQUEST)), + Self::PkceVerification(err) => ( + StatusCode::BAD_REQUEST, + Json(INVALID_GRANT.with_description(format!("PKCE verification failed: {err}"))), + ), Self::ClientNotFound | Self::ClientCredentialsVerification(_) => { (StatusCode::UNAUTHORIZED, Json(INVALID_CLIENT)) } @@ -274,9 +281,7 @@ async fn authorization_code_grant( (Some(_), None) | (None, Some(_)) => return Err(RouteError::BadRequest), // If we have both, we need to check the code validity (Some(pkce), Some(verifier)) => { - if !pkce.verify(verifier) { - return Err(RouteError::BadRequest); - } + pkce.verify(verifier)?; } }; diff --git a/crates/oauth2-types/src/errors.rs b/crates/oauth2-types/src/errors.rs index 54f0355f..2920e894 100644 --- a/crates/oauth2-types/src/errors.rs +++ b/crates/oauth2-types/src/errors.rs @@ -12,10 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -#[derive(serde::Serialize)] +use std::borrow::Cow; + +#[derive(serde::Serialize, Clone)] pub struct ClientError { pub error: &'static str, - pub error_description: &'static str, + pub error_description: Cow<'static, str>, } impl ClientError { @@ -23,7 +25,15 @@ impl ClientError { pub const fn new(error: &'static str, error_description: &'static str) -> Self { Self { error, - error_description, + error_description: Cow::Borrowed(error_description), + } + } + + #[must_use] + pub const fn with_description(&self, description: String) -> Self { + Self { + error: self.error, + error_description: Cow::Owned(description), } } }