From 4bfe79ebe075eae86f355a9b41a47264435f3ab8 Mon Sep 17 00:00:00 2001 From: sebadob Date: Fri, 7 Jun 2024 11:09:46 +0200 Subject: [PATCH 1/6] refactor `User.saccount_type()` --- rauthy-models/src/entity/users.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/rauthy-models/src/entity/users.rs b/rauthy-models/src/entity/users.rs index 53097b1b..8b385801 100644 --- a/rauthy-models/src/entity/users.rs +++ b/rauthy-models/src/entity/users.rs @@ -913,12 +913,14 @@ impl User { impl User { #[inline] pub fn account_type(&self) -> AccountType { - if self.federation_uid.is_some() && self.password.is_some() { - AccountType::FederatedPassword - } else if self.federation_uid.is_some() && self.has_webauthn_enabled() { - AccountType::FederatedPasskey - } else if self.federation_uid.is_some() { - AccountType::Federated + if self.federation_uid.is_some() { + if self.password.is_some() { + AccountType::FederatedPassword + } else if self.has_webauthn_enabled() { + AccountType::FederatedPasskey + } else { + AccountType::Federated + } } else if self.password.is_some() { AccountType::Password } else if self.has_webauthn_enabled() { @@ -1363,7 +1365,7 @@ impl User { if self.password.is_none() { return Err(ErrorResponse::new( ErrorResponseType::PasswordExpired, - String::from("No password set"), + "No password set", )); } @@ -1373,7 +1375,7 @@ impl User { if !self.enabled { return Err(ErrorResponse::new( ErrorResponseType::PasswordExpired, - String::from("The password has expired"), + "The password has expired", )); } @@ -1390,12 +1392,12 @@ impl User { Err(ErrorResponse::new( ErrorResponseType::PasswordRefresh, - String::from("The password has expired. A reset E-Mail has been sent out."), + "The password has expired. A reset E-Mail has been sent out.", )) } else { Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - String::from("Invalid user credentials"), + "Invalid user credentials", )) }; } @@ -1406,7 +1408,7 @@ impl User { } else { Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - String::from("Invalid user credentials"), + "Invalid user credentials", )) } } From 1dc8d4ff0705caa9f47a4627945c76d64cde2bde Mon Sep 17 00:00:00 2001 From: sebadob Date: Fri, 7 Jun 2024 11:10:31 +0200 Subject: [PATCH 2/6] more error message `&'static str` in `Client` --- rauthy-models/src/entity/clients.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rauthy-models/src/entity/clients.rs b/rauthy-models/src/entity/clients.rs index 2d573f0d..90698585 100644 --- a/rauthy-models/src/entity/clients.rs +++ b/rauthy-models/src/entity/clients.rs @@ -763,7 +763,7 @@ impl Client { trace!("'code_challenge_method' is missing"); return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("'code_challenge_method' is missing"), + "'code_challenge_method' is missing", )); } @@ -781,7 +781,7 @@ impl Client { trace!("'code_challenge' not enabled for this client"); Err(ErrorResponse::new( ErrorResponseType::BadRequest, - "'code_challenge' not enabled for this client".to_string(), + "'code_challenge' not enabled for this client", )) } else { Ok(()) From 7c637d3e87ed910526b874a548b66d3715f00724 Mon Sep 17 00:00:00 2001 From: sebadob Date: Fri, 7 Jun 2024 12:17:12 +0200 Subject: [PATCH 3/6] `impl From for ErrorResponse` --- rauthy-common/src/error_response.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/rauthy-common/src/error_response.rs b/rauthy-common/src/error_response.rs index 1730473b..914e37aa 100644 --- a/rauthy-common/src/error_response.rs +++ b/rauthy-common/src/error_response.rs @@ -449,3 +449,13 @@ impl From for ErrorResponse { ) } } + +impl From for ErrorResponse { + fn from(value: std::fmt::Error) -> Self { + trace!("{:?}", value); + ErrorResponse::new( + ErrorResponseType::Internal, + format!("fmt error: {:?}", value), + ) + } +} From 0ff5918667b0535504543aefd9ce5afff7b8cc5e Mon Sep 17 00:00:00 2001 From: sebadob Date: Fri, 7 Jun 2024 12:23:23 +0200 Subject: [PATCH 4/6] simplify and clean up post authorize logic to make it better maintainable --- rauthy-handlers/src/auth_providers.rs | 4 +- rauthy-handlers/src/lib.rs | 19 +-- rauthy-handlers/src/oidc.rs | 74 +++++++-- rauthy-models/src/entity/auth_providers.rs | 19 +-- rauthy-models/src/lib.rs | 2 - rauthy-service/src/auth.rs | 169 +++++++++------------ 6 files changed, 148 insertions(+), 139 deletions(-) diff --git a/rauthy-handlers/src/auth_providers.rs b/rauthy-handlers/src/auth_providers.rs index 4643ab9b..0a2be701 100644 --- a/rauthy-handlers/src/auth_providers.rs +++ b/rauthy-handlers/src/auth_providers.rs @@ -193,9 +193,7 @@ pub async fn post_provider_callback( let (auth_step, cookie) = AuthProviderCallback::login_finish(&data, &req, &payload, session.clone()).await?; - let (mut resp, _) = map_auth_step(auth_step, &req) - .await - .map_err(|(err, _)| err)?; + let mut resp = map_auth_step(auth_step, &req).await?; resp.add_cookie(&cookie).map_err(|err| { ErrorResponse::new( ErrorResponseType::Internal, diff --git a/rauthy-handlers/src/lib.rs b/rauthy-handlers/src/lib.rs index 05fc8bfd..d1fd3a9a 100644 --- a/rauthy-handlers/src/lib.rs +++ b/rauthy-handlers/src/lib.rs @@ -45,7 +45,7 @@ pub async fn map_auth_step( req: &HttpRequest, // the bool for Ok() is true is the password has been hashed // the bool for Err() means if we need to add a login delay (and none otherwise for better UX) -) -> Result<(HttpResponse, bool), (ErrorResponse, bool)> { +) -> Result { // we will only get here after a successful login -> always return logged-in header let fed_cm_header = FedCMLoginStatus::LoggedIn.as_header_pair(); @@ -59,7 +59,7 @@ pub async fn map_auth_step( if let Some((name, value)) = res.header_origin { resp.headers_mut().insert(name, value); } - Ok((resp, res.has_password_been_hashed)) + Ok(resp) } AuthStep::AwaitWebauthn(res) => { @@ -82,23 +82,20 @@ pub async fn map_auth_step( WebauthnCookie::parse_validate(&ApiCookie::from_req(req, COOKIE_MFA)) { if mfa_cookie.email != res.email { - add_req_mfa_cookie(&mut resp, res.email.clone()).map_err(|err| (err, true))?; + add_req_mfa_cookie(&mut resp, res.email.clone())?; } } else { - add_req_mfa_cookie(&mut resp, res.email.clone()).map_err(|err| (err, true))?; + add_req_mfa_cookie(&mut resp, res.email.clone())?; } - Ok((resp, res.has_password_been_hashed)) + Ok(resp) } AuthStep::ProviderLink => { // TODO generate a new event type in this case? - Ok(( - HttpResponse::NoContent() - .insert_header(fed_cm_header) - .finish(), - false, - )) + Ok(HttpResponse::NoContent() + .insert_header(fed_cm_header) + .finish()) } } } diff --git a/rauthy-handlers/src/oidc.rs b/rauthy-handlers/src/oidc.rs index 25dbd4c9..8198a9a1 100644 --- a/rauthy-handlers/src/oidc.rs +++ b/rauthy-handlers/src/oidc.rs @@ -256,13 +256,55 @@ pub async fn post_authorize( let start = SystemTime::now().duration_since(UNIX_EPOCH).unwrap(); let session = principal.get_session()?; - let res = match auth::authorize(&data, &req, payload.into_inner(), session.clone()).await { + + let mut has_password_been_hashed = false; + let mut add_login_delay = true; + let mut user_needs_mfa = false; + + let res = match auth::authorize( + &data, + &req, + payload.into_inner(), + session.clone(), + &mut has_password_been_hashed, + &mut add_login_delay, + &mut user_needs_mfa, + ) + .await + { Ok(auth_step) => map_auth_step(auth_step, &req).await, - Err(err) => Err(err), + Err(err) => { + debug!("{:?}", err); + // We always must return the exact same error type, no matter what the actual error is, + // to prevent information enumeration. The only exception is when the user needs to add + // a passkey to the account while having given the correct credentials. In that case, + // we return the original error to be able to display the info message in the UI. + if user_needs_mfa { + // in this case, we can return directly without any login delay + return Err(err); + } + + let err = Err(ErrorResponse::new( + ErrorResponseType::Unauthorized, + "Invalid user credentials", + )); + if !add_login_delay { + return err; + } + err + } }; let ip = real_ip_from_req(&req); - auth::handle_login_delay(&data, ip, start, &data.caches.ha_cache_config, res).await + auth::handle_login_delay( + &data, + ip, + start, + &data.caches.ha_cache_config, + res, + has_password_been_hashed, + ) + .await } /// Immediate login refresh with valid session @@ -303,10 +345,7 @@ pub async fn post_authorize_refresh( let auth_step = auth::authorize_refresh(&data, session, client, header_origin, req_data.into_inner()) .await?; - map_auth_step(auth_step, &req) - .await - .map(|res| res.0) - .map_err(|err| err.0) + map_auth_step(auth_step, &req).await } #[get("/oidc/callback")] @@ -852,7 +891,7 @@ pub async fn post_token( } let start = SystemTime::now().duration_since(UNIX_EPOCH).unwrap(); - let add_login_delay = payload.grant_type == "password"; + let has_password_been_hashed = payload.grant_type == "password"; let res = match auth::get_token_set(payload.into_inner(), &data, req).await { Ok((token_set, headers)) => { @@ -861,12 +900,25 @@ pub async fn post_token( builder.insert_header(h); } let resp = builder.json(token_set); - Ok((resp, add_login_delay)) + Ok(resp) + } + Err(err) => { + if !has_password_been_hashed { + return Err(err); + } + Err(err) } - Err(err) => Err((err, add_login_delay)), }; - auth::handle_login_delay(&data, ip, start, &data.caches.ha_cache_config, res).await + auth::handle_login_delay( + &data, + ip, + start, + &data.caches.ha_cache_config, + res, + has_password_been_hashed, + ) + .await } /// The tokenInfo endpoint for the OIDC standard. diff --git a/rauthy-models/src/entity/auth_providers.rs b/rauthy-models/src/entity/auth_providers.rs index 87d2402d..b6d44024 100644 --- a/rauthy-models/src/entity/auth_providers.rs +++ b/rauthy-models/src/entity/auth_providers.rs @@ -701,7 +701,7 @@ impl AuthProviderCallback { let callback_id = ApiCookie::from_req(req, COOKIE_UPSTREAM_CALLBACK).ok_or_else(|| { ErrorResponse::new( ErrorResponseType::Forbidden, - "Missing encrypted callback cookie".to_string(), + "Missing encrypted callback cookie", ) })?; @@ -712,7 +712,7 @@ impl AuthProviderCallback { error!("`state` does not match"); return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - "`state` does not match".to_string(), + "`state` does not match", )); } debug!("callback state is valid"); @@ -725,7 +725,7 @@ impl AuthProviderCallback { error!("invalid CSRF token"); return Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - "invalid CSRF token".to_string(), + "invalid CSRF token", )); } debug!("callback csrf token is valid"); @@ -739,7 +739,7 @@ impl AuthProviderCallback { error!("invalid PKCE verifier"); return Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - "invalid PKCE verifier".to_string(), + "invalid PKCE verifier", )); } debug!("callback pkce verifier is valid"); @@ -835,10 +835,7 @@ impl AuthProviderCallback { } else { let err = "Neither `access_token` nor `id_token` existed"; error!("{}", err); - return Err(ErrorResponse::new( - ErrorResponseType::BadRequest, - err.to_string(), - )); + return Err(ErrorResponse::new(ErrorResponseType::BadRequest, err)); } } Err(err) => { @@ -871,7 +868,7 @@ impl AuthProviderCallback { if provider_mfa_login == ProviderMfaLogin::No && !user.has_webauthn_enabled() { return Err(ErrorResponse::new( ErrorResponseType::MfaRequired, - "MFA is required for this client".to_string(), + "MFA is required for this client", )); } session.set_mfa(data, true).await?; @@ -905,12 +902,11 @@ impl AuthProviderCallback { // location header let mut loc = format!("{}?code={}", slf.req_redirect_uri, code.id); if let Some(state) = slf.req_state { - write!(loc, "&state={}", state).expect("`write!` to succeed"); + write!(loc, "&state={}", state)?; }; let auth_step = if user.has_webauthn_enabled() { let step = AuthStepAwaitWebauthn { - has_password_been_hashed: false, code: get_rand(48), header_csrf: Session::get_csrf_header(&session.csrf_token), header_origin, @@ -935,7 +931,6 @@ impl AuthProviderCallback { AuthStep::AwaitWebauthn(step) } else { AuthStep::LoggedIn(AuthStepLoggedIn { - has_password_been_hashed: false, user_id: user.id, email: user.email, header_loc: (header::LOCATION, HeaderValue::from_str(&loc).unwrap()), diff --git a/rauthy-models/src/lib.rs b/rauthy-models/src/lib.rs index 31d076cd..300aa381 100644 --- a/rauthy-models/src/lib.rs +++ b/rauthy-models/src/lib.rs @@ -33,7 +33,6 @@ pub enum AuthStep { } pub struct AuthStepLoggedIn { - pub has_password_been_hashed: bool, pub user_id: String, pub email: String, pub header_loc: (HeaderName, HeaderValue), @@ -42,7 +41,6 @@ pub struct AuthStepLoggedIn { } pub struct AuthStepAwaitWebauthn { - pub has_password_been_hashed: bool, pub code: String, pub header_csrf: (HeaderName, HeaderValue), pub header_origin: Option<(HeaderName, HeaderValue)>, diff --git a/rauthy-service/src/auth.rs b/rauthy-service/src/auth.rs index e849dd16..a1734973 100644 --- a/rauthy-service/src/auth.rs +++ b/rauthy-service/src/auth.rs @@ -54,6 +54,7 @@ use ring::digest; use std::borrow::Cow; use std::cmp::PartialEq; use std::collections::{HashMap, HashSet}; +use std::fmt::Write; use std::ops::{Add, Sub}; use std::str::FromStr; use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -68,22 +69,21 @@ pub async fn authorize( req: &HttpRequest, req_data: LoginRequest, mut session: Session, - // the second argument with the error will be 'true' if a login delay should be added -) -> Result { - // This Error must be the same if user does not exist AND passwords do not match to prevent - // username enumeration + has_password_been_hashed: &mut bool, + add_login_delay: &mut bool, + user_needs_mfa: &mut bool, +) -> Result { let mut user = User::find_by_email(data, req_data.email) .await - .map_err(|e| { - error!("{:?}", e); - // be careful, that this Err and the one in User::validate_password are exactly the same - ( - ErrorResponse::new( - ErrorResponseType::Unauthorized, - String::from("Invalid user credentials"), - ), - false, - ) + .map_err(|err| { + // The UI does not show the password input form when there is no user yet. + // To prevent username enumeration, we should not add a login delay if a user does not + // even exist, when the UI is in that phase where the user does not provide any + // password. + if req_data.password.is_none() { + *add_login_delay = false; + } + err })?; let mfa_cookie = @@ -91,8 +91,8 @@ pub async fn authorize( if c.email == user.email && user.has_webauthn_enabled() { Some(c) } else { - // If a possibly existing mfa cookie does not match the given email, or user has webauthn - // disabled in the meantime -> ignore the cookie + // If a possibly existing mfa cookie does not match the given email, or the user + // has webauthn disabled in the meantime, ignore the cookie None } } else { @@ -101,76 +101,62 @@ pub async fn authorize( let account_type = user.account_type(); - // this allows a user without the mfa cookie to login anyway if it is an only passkey account - // in this case, UV is always enforced, not matter what -> safe to login without cookie + // only allow an empty password, if the user has a passkey only account or a valid MFA cookie let user_must_provide_password = req_data.password.is_none() && account_type != AccountType::Passkey && mfa_cookie.is_none(); if user_must_provide_password { + // if we get here, the UI did the first step from the login form + // -> username only without password + // we should not add a delay in that case, because the user did nothing wrong, we just need + // to get the password, because it is no passkey only account + *add_login_delay = false; + trace!("No user password has been provided"); - return Err(( - ErrorResponse::new( - ErrorResponseType::Unauthorized, - String::from("Invalid user credentials"), - ), - false, + return Err(ErrorResponse::new( + ErrorResponseType::Unauthorized, + "User needs to provide a password", )); } if account_type == AccountType::New { - return Err(( - ErrorResponse::new( - ErrorResponseType::Unauthorized, - String::from("Invalid user credentials"), - ), - // this basically means, if the user did the first login in the UI with just username, - // do not add any login delay afterwards for a better UX - !user_must_provide_password, + // the user has created an account but no password has been set so far + return Err(ErrorResponse::new( + ErrorResponseType::Unauthorized, + "The account has not been set up yet", )); } - user.check_enabled() - .map_err(|err| (err, !user_must_provide_password))?; - user.check_expired() - .map_err(|err| (err, !user_must_provide_password))?; - - let has_password_been_hashed = if let Some(pwd) = req_data.password { - match user.validate_password(data, pwd).await { - Ok(_) => { - // update user info - // in case of webauthn login, the info will be updates in the auth finish step - user.last_login = Some(OffsetDateTime::now_utc().unix_timestamp()); - user.last_failed_login = None; - user.failed_login_attempts = None; - user.save(data, None, None) - .await - .map_err(|err| (err, true))?; - } - Err(err) => { - trace!("Provided user password is invalid"); - return Err((err, true)); - } - } - true - } else { - false - }; + user.check_enabled()?; + user.check_expired()?; + + // TODO should we move the password hashing as far back as possible? -> most expensive operation + // maybe it makes sense to do additional DB requests instead of hashing a password? + // what about brute force attempts in that case? + // -> identify the best ordering and if it maybe makes sense to check the client first + if let Some(pwd) = req_data.password { + *has_password_been_hashed = true; + user.validate_password(data, pwd).await?; + + // update user info + // in case of webauthn login, the info will be updated in the auth finish step + user.last_login = Some(Utc::now().timestamp()); + user.last_failed_login = None; + user.failed_login_attempts = None; + user.save(data, None, None).await?; + } // client validations - let client = Client::find_maybe_ephemeral(data, req_data.client_id) - .await - .map_err(|err| (err, !user_must_provide_password))?; - client - .validate_mfa(&user) - .map_err(|err| (err, has_password_been_hashed))?; - client - .validate_redirect_uri(&req_data.redirect_uri) - .map_err(|err| (err, !user_must_provide_password))?; - client - .validate_code_challenge(&req_data.code_challenge, &req_data.code_challenge_method) - .map_err(|err| (err, !user_must_provide_password))?; - let header_origin = client - .validate_origin(req, &data.listen_scheme, &data.public_url) - .map_err(|err| (err, !user_must_provide_password))?; + let client = Client::find_maybe_ephemeral(data, req_data.client_id).await?; + client.validate_mfa(&user).map_err(|err| { + // in this case, we do not want to add a login delay + // the user password was correct, we only need a passkey being added to the account + *user_needs_mfa = true; + *add_login_delay = false; + err + })?; + client.validate_redirect_uri(&req_data.redirect_uri)?; + client.validate_code_challenge(&req_data.code_challenge, &req_data.code_challenge_method)?; + let header_origin = client.validate_origin(req, &data.listen_scheme, &data.public_url)?; // build authorization code let code_lifetime = if user.has_webauthn_enabled() { @@ -178,9 +164,7 @@ pub async fn authorize( } else { client.auth_code_lifetime }; - let scopes = client - .sanitize_login_scopes(&req_data.scopes) - .map_err(|err| (err, !user_must_provide_password))?; + let scopes = client.sanitize_login_scopes(&req_data.scopes)?; let code = AuthCode::new( user.id.clone(), client.id, @@ -191,27 +175,21 @@ pub async fn authorize( scopes, code_lifetime, ); - code.save(data) - .await - .map_err(|err| (err, !user_must_provide_password))?; + code.save(data).await?; // build location header let mut loc = format!("{}?code={}", req_data.redirect_uri, code.id); if let Some(state) = req_data.state { - loc = format!("{}&state={}", loc, state); + write!(loc, "&state={}", state)?; }; // TODO double check that we do not have any problems with the direct webauthn login here // TODO should we allow to skip this step if set so in the config? // check if we need to validate the 2nd factor if user.has_webauthn_enabled() { - session - .set_mfa(data, true) - .await - .map_err(|err| (err, !user_must_provide_password))?; + session.set_mfa(data, true).await?; let step = AuthStepAwaitWebauthn { - has_password_been_hashed, code: get_rand(48), header_csrf: Session::get_csrf_header(&session.csrf_token), header_origin, @@ -231,13 +209,11 @@ pub async fn authorize( .map(|h| h.1.to_str().unwrap().to_string()), } .save(data) - .await - .map_err(|err| (err, !user_must_provide_password))?; + .await?; Ok(AuthStep::AwaitWebauthn(step)) } else { Ok(AuthStep::LoggedIn(AuthStepLoggedIn { - has_password_been_hashed, user_id: user.id, email: user.email, header_loc: (header::LOCATION, HeaderValue::from_str(&loc).unwrap()), @@ -296,7 +272,6 @@ pub async fn authorize_refresh( // check if we need to validate the 2nd factor if user.has_webauthn_enabled() && *SESSION_RENEW_MFA { let step = AuthStepAwaitWebauthn { - has_password_been_hashed: false, code: get_rand(48), header_csrf: Session::get_csrf_header(&session.csrf_token), header_origin, @@ -320,7 +295,6 @@ pub async fn authorize_refresh( Ok(AuthStep::AwaitWebauthn(step)) } else { Ok(AuthStep::LoggedIn(AuthStepLoggedIn { - has_password_been_hashed: false, user_id: user.id, email: user.email, header_loc: ( @@ -1497,9 +1471,8 @@ pub async fn handle_login_delay( peer_ip: Option, start: Duration, cache_config: &redhac::CacheConfig, - // the bool for Ok() is true is the password has been hashed - // the bool for Err() means if we need to add a login delay (and none otherwise for better UX) - res: Result<(HttpResponse, bool), (ErrorResponse, bool)>, + res: Result, + has_password_been_hashed: bool, ) -> Result { let success_time = cache_get!( i64, @@ -1515,7 +1488,7 @@ pub async fn handle_login_delay( let delta = end - start; match res { - Ok((resp, has_password_been_hashed)) => { + Ok(resp) => { // cleanup possibly blacklisted IP if let Some(ip) = peer_ip { data.tx_ip_blacklist @@ -1543,11 +1516,7 @@ pub async fn handle_login_delay( Ok(resp) } - Err((err, add_login_delay)) => { - if !add_login_delay { - return Err(err); - } - + Err(err) => { let mut failed_logins = 1; // check possibly blacklisted IP @@ -1946,7 +1915,7 @@ pub async fn validate_auth_req_param( if code_challenge.is_none() { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("'code_challenge' is missing"), + "'code_challenge' is missing", )); } else { // 'plain' is the default method to be assumed by the OAuth specification when it is From 609363dbed09049751e2b4649e7dcc9b7ff6360d Mon Sep 17 00:00:00 2001 From: sebadob Date: Fri, 7 Jun 2024 12:24:54 +0200 Subject: [PATCH 5/6] clippy + additionally less string allocation in error cases --- rauthy-models/src/entity/users.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/rauthy-models/src/entity/users.rs b/rauthy-models/src/entity/users.rs index 8b385801..b542ce26 100644 --- a/rauthy-models/src/entity/users.rs +++ b/rauthy-models/src/entity/users.rs @@ -21,6 +21,7 @@ use crate::response::UserResponseSimple; use crate::templates::UserEmailChangeConfirmHtml; use actix_web::{web, HttpRequest}; use argon2::PasswordHash; +use chrono::Utc; use rauthy_common::constants::{ CACHE_NAME_12HR, CACHE_NAME_USERS, IDX_USERS, RAUTHY_ADMIN_ROLE, USER_COUNT_IDX, WEBAUTHN_NO_PASSWORD_EXPIRY, @@ -1071,24 +1072,26 @@ impl User { Ok(()) } + #[inline] pub fn check_enabled(&self) -> Result<(), ErrorResponse> { if !self.enabled { trace!("The user is not enabled"); return Err(ErrorResponse::new( ErrorResponseType::Disabled, - String::from("User is not enabled"), + "User is not enabled", )); } Ok(()) } + #[inline] pub fn check_expired(&self) -> Result<(), ErrorResponse> { if let Some(ts) = self.user_expires { - if OffsetDateTime::now_utc().unix_timestamp() > ts { + if Utc::now().timestamp() > ts { trace!("User has expired"); return Err(ErrorResponse::new( ErrorResponseType::Disabled, - String::from("User has expired"), + "User has expired", )); } } From e79f3627be84a921f1b201d8c67332bb77e25433 Mon Sep 17 00:00:00 2001 From: sebadob Date: Fri, 7 Jun 2024 12:41:11 +0200 Subject: [PATCH 6/6] additional less `ErrorResponse.message` String allocations --- rauthy-handlers/src/auth_providers.rs | 4 +- rauthy-handlers/src/oidc.rs | 2 +- rauthy-models/src/entity/app_version.rs | 3 +- rauthy-models/src/entity/principal.rs | 12 ++-- rauthy-models/src/entity/sessions.rs | 2 +- rauthy-service/src/auth.rs | 92 ++++++++++--------------- 6 files changed, 48 insertions(+), 67 deletions(-) diff --git a/rauthy-handlers/src/auth_providers.rs b/rauthy-handlers/src/auth_providers.rs index 0a2be701..6e5152ce 100644 --- a/rauthy-handlers/src/auth_providers.rs +++ b/rauthy-handlers/src/auth_providers.rs @@ -409,7 +409,7 @@ pub async fn put_provider_img( None => { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - "content_type is missing".to_string(), + "content_type is missing", )); } } @@ -464,7 +464,7 @@ pub async fn post_provider_link( if user.auth_provider_id.is_some() { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - "user is already federated".to_string(), + "user is already federated", )); } diff --git a/rauthy-handlers/src/oidc.rs b/rauthy-handlers/src/oidc.rs index 8198a9a1..97059755 100644 --- a/rauthy-handlers/src/oidc.rs +++ b/rauthy-handlers/src/oidc.rs @@ -884,7 +884,7 @@ pub async fn post_token( let ip = real_ip_from_req(&req); if payload.grant_type == GRANT_TYPE_DEVICE_CODE { - // TODO the `urn:ietf:params:oauth:grant-type:device_code` needs + // the `urn:ietf:params:oauth:grant-type:device_code` needs // a fully customized handling here with customized error response // to meet the oauth rfc return Ok(auth::grant_type_device_code(&data, ip, payload.into_inner()).await); diff --git a/rauthy-models/src/entity/app_version.rs b/rauthy-models/src/entity/app_version.rs index 9c748a66..deb504c9 100644 --- a/rauthy-models/src/entity/app_version.rs +++ b/rauthy-models/src/entity/app_version.rs @@ -145,8 +145,7 @@ impl LatestAppVersion { } else { Err(ErrorResponse::new( ErrorResponseType::Internal, - "Could not find 'tag_name' in Rauthy App Version lookup response" - .to_string(), + "Could not find 'tag_name' in Rauthy App Version lookup response", )) }?; diff --git a/rauthy-models/src/entity/principal.rs b/rauthy-models/src/entity/principal.rs index 79e9e874..6afd1787 100644 --- a/rauthy-models/src/entity/principal.rs +++ b/rauthy-models/src/entity/principal.rs @@ -114,7 +114,7 @@ impl Principal { if *ADMIN_FORCE_MFA && !self.has_mfa_active() { return Err(ErrorResponse::new( ErrorResponseType::MfaRequired, - "Rauthy admin access only allowed with MFA active".to_string(), + "Rauthy admin access only allowed with MFA active", )); } @@ -184,14 +184,14 @@ impl Principal { trace!("Validating the session failed - was not in auth state"); Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - "Unauthorized Session".to_string(), + "Unauthorized Session", )) } } else { trace!("Validating the session failed - no session found"); Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - "No valid session".to_string(), + "No valid session", )) } } @@ -205,14 +205,14 @@ impl Principal { trace!("Validating the session failed - was not in init or auth state"); Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - "Unauthorized Session".to_string(), + "Unauthorized Session", )) } } else { trace!("Validating the session failed - no session found"); Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - "No valid session".to_string(), + "No valid session", )) } } @@ -230,7 +230,7 @@ impl Principal { trace!("Validating the session failed - was not in init state"); Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - "Session in Init state mandatory".to_string(), + "Session in Init state mandatory", )) } } diff --git a/rauthy-models/src/entity/sessions.rs b/rauthy-models/src/entity/sessions.rs index 032e3914..1b5b205b 100644 --- a/rauthy-models/src/entity/sessions.rs +++ b/rauthy-models/src/entity/sessions.rs @@ -701,7 +701,7 @@ impl Session { if OffsetDateTime::now_utc().unix_timestamp() > ts { return Err(ErrorResponse::new( ErrorResponseType::Forbidden, - "User has expired".to_string(), + "User has expired", )); } else if ts < self.exp { self.exp = ts; diff --git a/rauthy-service/src/auth.rs b/rauthy-service/src/auth.rs index a1734973..8dd481bf 100644 --- a/rauthy-service/src/auth.rs +++ b/rauthy-service/src/auth.rs @@ -234,7 +234,7 @@ pub async fn authorize_refresh( let user_id = session.user_id.as_ref().ok_or_else(|| { ErrorResponse::new( ErrorResponseType::Internal, - String::from("No linked user_id for already validated session"), + "No linked user_id for already validated session", ) })?; let user = User::find(data, user_id.clone()).await?; @@ -628,16 +628,13 @@ pub async fn build_refresh_token( #[inline(always)] pub fn get_bearer_token_from_header(headers: &HeaderMap) -> Result { - let bearer = headers.get("Authorization").ok_or_else(|| { - ErrorResponse::new( - ErrorResponseType::Unauthorized, - String::from("Bearer Token missing"), - ) - }); + let bearer = headers + .get("Authorization") + .ok_or_else(|| ErrorResponse::new(ErrorResponseType::Unauthorized, "Bearer Token missing")); if bearer.is_err() { return Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - String::from("Authorization header missing"), + "Authorization header missing", )); } @@ -646,7 +643,7 @@ pub fn get_bearer_token_from_header(headers: &HeaderMap) -> Result Result grant_type_refresh(data, req, req_data).await, _ => Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("Invalid 'grant_type'"), + "Invalid 'grant_type'", )), } } @@ -888,7 +885,7 @@ async fn grant_type_code( warn!("'code' is missing"); return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("'code' is missing"), + "'code' is missing", )); } @@ -906,10 +903,7 @@ async fn grant_type_code( if client.confidential { let secret = client_secret.ok_or_else(|| { warn!("'client_secret' is missing"); - ErrorResponse::new( - ErrorResponseType::BadRequest, - String::from("'client_secret' is missing"), - ) + ErrorResponse::new(ErrorResponseType::BadRequest, "'client_secret' is missing") })?; client.validate_secret(&secret, &req)?; } @@ -942,7 +936,7 @@ async fn grant_type_code( ); ErrorResponse::new( ErrorResponseType::Unauthorized, - "'auth_code' could not be found inside the cache".to_string(), + "'auth_code' could not be found inside the cache", ) })?; // validate the auth code @@ -955,7 +949,7 @@ async fn grant_type_code( warn!("The Authorization Code has expired"); return Err(ErrorResponse::new( ErrorResponseType::SessionExpired, - String::from("The Authorization Code has expired"), + "The Authorization Code has expired", )); } if code.challenge.is_some() { @@ -963,7 +957,7 @@ async fn grant_type_code( warn!("'code_verifier' is missing"); return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("'code_verifier' is missing"), + "'code_verifier' is missing", )); } @@ -972,7 +966,7 @@ async fn grant_type_code( warn!("'code_verifier' does not match the challenge"); return Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - String::from("'code_verifier' does not match the challenge"), + "'code_verifier' does not match the challenge", )); } } else { @@ -983,7 +977,7 @@ async fn grant_type_code( warn!("'code_verifier' does not match the challenge"); return Err(ErrorResponse::new( ErrorResponseType::Unauthorized, - String::from("'code_verifier' does not match the challenge"), + "'code_verifier' does not match the challenge", )); } } @@ -1048,7 +1042,7 @@ async fn grant_type_credentials( if req_data.client_secret.is_none() { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("'client_secret' is missing"), + "'client_secret' is missing", )); } @@ -1057,20 +1051,17 @@ async fn grant_type_credentials( if !client.confidential { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("'client_credentials' flow is allowed for confidential clients only"), + "'client_credentials' flow is allowed for confidential clients only", )); } if !client.enabled { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("client is disabled"), + "client is disabled", )); } let secret = client_secret.ok_or_else(|| { - ErrorResponse::new( - ErrorResponseType::BadRequest, - String::from("'client_secret' is missing"), - ) + ErrorResponse::new(ErrorResponseType::BadRequest, "'client_secret' is missing") })?; client.validate_secret(&secret, &req)?; client.validate_flow("client_credentials")?; @@ -1295,13 +1286,13 @@ async fn grant_type_password( if req_data.username.is_none() { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("Missing 'username'"), + "Missing 'username'", )); } if req_data.password.is_none() { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("Missing 'password"), + "Missing 'password", )); } @@ -1313,10 +1304,7 @@ async fn grant_type_password( let header_origin = client.validate_origin(&req, &data.listen_scheme, &data.public_url)?; if client.confidential { let secret = client_secret.ok_or_else(|| { - ErrorResponse::new( - ErrorResponseType::BadRequest, - String::from("Missing 'client_secret'"), - ) + ErrorResponse::new(ErrorResponseType::BadRequest, "Missing 'client_secret'") })?; client.validate_secret(&secret, &req)?; } @@ -1349,10 +1337,7 @@ async fn grant_type_password( get_client_ip(&req), email ); - ErrorResponse::new( - ErrorResponseType::Unauthorized, - String::from("Invalid user credentials"), - ) + ErrorResponse::new(ErrorResponseType::Unauthorized, "Invalid user credentials") })?; user.check_enabled()?; user.check_expired()?; @@ -1420,7 +1405,7 @@ async fn grant_type_refresh( if req_data.refresh_token.is_none() { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("'refresh_token' is missing"), + "'refresh_token' is missing", )); } let (client_id, client_secret) = req_data.try_get_client_id_secret(&req)?; @@ -1430,10 +1415,7 @@ async fn grant_type_refresh( if client.confidential { let secret = client_secret.ok_or_else(|| { - ErrorResponse::new( - ErrorResponseType::BadRequest, - String::from("'client_secret' is missing"), - ) + ErrorResponse::new(ErrorResponseType::BadRequest, "'client_secret' is missing") })?; client.validate_secret(&secret, &req)?; } @@ -1697,7 +1679,7 @@ pub async fn logout( if JwtTokenType::Id != claims.custom.typ { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("The provided token is not an ID token"), + "The provided token is not an ID token", )); } @@ -1709,7 +1691,7 @@ pub async fn logout( if client.post_logout_redirect_uris.is_none() { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("Given 'post_logout_redirect_uri' is not allowed"), + "Given 'post_logout_redirect_uri' is not allowed", )); } @@ -1727,7 +1709,7 @@ pub async fn logout( if valid_redirect.count() == 0 { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("Given 'post_logout_redirect_uri' is not allowed"), + "Given 'post_logout_redirect_uri' is not allowed", )); } // redirect uri is valid at this point @@ -1960,7 +1942,7 @@ pub async fn validate_refresh_token( if claims.custom.typ != JwtTokenType::Refresh { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("Provided Token is not a valid refresh token"), + "Provided Token is not a valid refresh token", )); } @@ -1976,7 +1958,7 @@ pub async fn validate_refresh_token( if client.id != claims.custom.azp { return Err(ErrorResponse::new( ErrorResponseType::BadRequest, - String::from("Invalid 'azp'"), + "Invalid 'azp'", )); } let header_origin = client.validate_origin(req, &data.listen_scheme, &data.public_url)?; @@ -1989,7 +1971,7 @@ pub async fn validate_refresh_token( if fingerprint != cnf.jkt { return Err(ErrorResponse::new( ErrorResponseType::Forbidden, - "The refresh token is bound to a missing DPoP proof".to_string(), + "The refresh token is bound to a missing DPoP proof", )); } debug!("DPoP-Bound refresh token accepted"); @@ -1997,7 +1979,7 @@ pub async fn validate_refresh_token( } else { return Err(ErrorResponse::new( ErrorResponseType::Forbidden, - "The refresh token is bound to a missing DPoP proof".to_string(), + "The refresh token is bound to a missing DPoP proof", )); } } else { @@ -2018,13 +2000,13 @@ pub async fn validate_refresh_token( if &rt.device_id != device_id { return Err(ErrorResponse::new( ErrorResponseType::Forbidden, - "'device_id' does not match".to_string(), + "'device_id' does not match", )); } if rt.user_id != user.id { return Err(ErrorResponse::new( ErrorResponseType::Forbidden, - "'user_id' does not match".to_string(), + "'user_id' does not match", )); }