From befbdb46400a80662c7d64004f4b696b49514ed8 Mon Sep 17 00:00:00 2001 From: Justin Willhite <5132924+thejdubb02@users.noreply.github.com> Date: Tue, 29 Sep 2026 20:43:23 +0000 Subject: [PATCH 1/2] Don't answer invalid_grant when the SSO provider is unreachable Clients log out on a 400 invalid_grant from /identity/connect/token. A refresh that failed because the provider could not be reached (discovery failure, connection error, unparseable or 5xx response, temporarily_unavailable) now returns 503, so the client keeps the session and retries. Real rejections from the provider still return invalid_grant. Fixes #7775 --- src/api/identity.rs | 2 + src/error.rs | 5 +++ src/sso_client.rs | 104 ++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 9 deletions(-) diff --git a/src/api/identity.rs b/src/api/identity.rs index 6808ddde..69e3b9b5 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -166,6 +166,8 @@ async fn refresh_login(data: ConnectData, conn: &DbConn, ip: &ClientIp) -> JsonR // --- // let members = Membership::find_confirmed_by_user(&user.uuid, conn).await; match auth::refresh_tokens(ip, &refresh_token, data.client_id, conn).await { + // Clients log out on 400 invalid_grant, so a provider outage is returned as-is (503). + Err(err) if err.get_code() == 503 => Err(err), Err(err) => { err_json!( json!({"error": "invalid_grant"}), diff --git a/src/error.rs b/src/error.rs index d90c38e3..95ea1a7d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -159,6 +159,11 @@ impl Error { self } + #[must_use] + pub const fn get_code(&self) -> u16 { + self.code + } + #[must_use] pub fn with_event(mut self, event: ErrorEvent) -> Self { self.event = Some(event); diff --git a/src/sso_client.rs b/src/sso_client.rs index bc766586..d5134393 100644 --- a/src/sso_client.rs +++ b/src/sso_client.rs @@ -4,8 +4,8 @@ use openidconnect::{ AccessToken, AsyncHttpClient, AuthDisplay, AuthPrompt, AuthType, AuthenticationFlow, AuthorizationCode, AuthorizationRequest, ClientId, ClientSecret, CsrfToken, EmptyAdditionalClaims, EmptyExtraTokenFields, EndpointNotSet, EndpointSet, HttpClientError, HttpRequest, HttpResponse, IdTokenClaims, IdTokenFields, Nonce, - OAuth2TokenResponse, PkceCodeChallenge, PkceCodeVerifier, RefreshToken, ResponseType, Scope, StandardErrorResponse, - StandardTokenResponse, + OAuth2TokenResponse, PkceCodeChallenge, PkceCodeVerifier, RefreshToken, RequestTokenError, ResponseType, Scope, + StandardErrorResponse, StandardTokenResponse, UserInfoError, core::{ CoreAuthDisplay, CoreAuthPrompt, CoreClient, CoreClientAuthMethod, CoreErrorResponseType, CoreGenderClaim, CoreIdTokenVerifier, CoreJsonWebKey, CoreJweContentEncryptionAlgorithm, CoreJwsSigningAlgorithm, @@ -21,6 +21,7 @@ use crate::{ CONFIG, api::{ApiResult, EmptyResult}, db::models::SsoAuth, + error::Error, http_client::get_reqwest_client_builder, sso::{OIDCCode, OIDCCodeChallenge, OIDCCodeVerifier, OIDCState}, }; @@ -32,7 +33,7 @@ static CLIENT_CACHE: LazyLock> = LazyLock::new .time_to_live(Duration::from_secs(CONFIG.sso_client_cache_expiration())) .build() }); -static REFRESH_CACHE: LazyLock>> = +static REFRESH_CACHE: LazyLock> = LazyLock::new(|| moka::future::Cache::builder().max_capacity(1000).time_to_live(Duration::from_secs(30)).build()); /// OpenID Connect Core client. @@ -57,6 +58,8 @@ pub type CustomClient = openidconnect::Client< >; pub type RefreshTokenResponse = (Option, String, Option); +// The error carries the HTTP status to answer with, see `refresh_error_status`. +type RefreshResult = Result; #[derive(Clone)] pub struct Client { @@ -117,7 +120,7 @@ impl Client { }; let provider_metadata = match CoreProviderMetadata::discover_async(issuer_url, &http_client).await { - Err(err) => err!(format!("Failed to discover OpenID provider: {err}")), + Err(err) => err_code!(format!("Failed to discover OpenID provider: {err}"), 503), Ok(metadata) => metadata, }; @@ -276,7 +279,10 @@ impl Client { pub async fn user_info(&self, access_token: AccessToken) -> ApiResult { match self.core_client.user_info(access_token, None).request_async(&self.http_client).await { - Err(err) => err!(format!("Request to user_info endpoint failed: {err}")), + Err(err) => { + let code = user_info_error_status(&err); + err_code!(format!("Request to user_info endpoint failed: {err}"), code) + } Ok(user_info) => Ok(user_info), } } @@ -285,7 +291,8 @@ impl Client { let client = Client::cached().await?; match client.user_info(AccessToken::new(access_token)).await { Err(err) => { - err_silent!(format!("Failed to retrieve user info, token has probably been invalidated: {err}")) + Err(Error::new_msg(format!("Failed to retrieve user info, token has probably been invalidated: {err}")) + .with_code(err.get_code())) } Ok(_) => Ok(()), } @@ -312,16 +319,17 @@ impl Client { REFRESH_CACHE .get_with(refresh_token.clone(), async move { client.exchange_refresh_token_impl(refresh_token).await }) .await - .map_err(Into::into) + .map_err(|(code, msg)| Error::new_msg(msg).with_code(code)) } - async fn exchange_refresh_token_impl(&self, refresh_token: String) -> Result { + async fn exchange_refresh_token_impl(&self, refresh_token: String) -> RefreshResult { let rt = RefreshToken::new(refresh_token); match self.core_client.exchange_refresh_token(&rt).request_async(&self.http_client).await { Err(err) => { error!("Request to exchange_refresh_token endpoint failed: {err}"); - Err(format!("Request to exchange_refresh_token endpoint failed: {err}")) + let code = refresh_error_status(&err); + Err((code, format!("Request to exchange_refresh_token endpoint failed: {err}"))) } Ok(token_response) => Ok(( token_response.refresh_token().map(|token| token.secret().clone()), @@ -346,3 +354,81 @@ impl<'a, AD: AuthDisplay, P: AuthPrompt, RT: ResponseType> AuthorizationRequestE self } } + +fn refresh_error_status(err: &RequestTokenError>) -> u16 +where + RE: std::error::Error + 'static, +{ + match err { + RequestTokenError::ServerResponse(resp) => match resp.error() { + CoreErrorResponseType::Extension(code) if code == "temporarily_unavailable" || code == "server_error" => { + 503 + } + _ => 400, + }, + RequestTokenError::Request(_) | RequestTokenError::Parse(..) | RequestTokenError::Other(_) => 503, + } +} + +fn user_info_error_status(err: &UserInfoError) -> u16 +where + RE: std::error::Error + 'static, +{ + match err { + UserInfoError::Request(_) => 503, + UserInfoError::Response(status, ..) if status.is_server_error() => 503, + _ => 400, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + type TokenErr = RequestTokenError, StandardErrorResponse>; + + #[test] + fn test_refresh_error_status_invalid_grant() { + let err = TokenErr::ServerResponse(StandardErrorResponse::new(CoreErrorResponseType::InvalidGrant, None, None)); + assert_eq!(refresh_error_status(&err), 400); + } + + #[test] + fn test_refresh_error_status_temporarily_unavailable() { + let err = TokenErr::ServerResponse(StandardErrorResponse::new( + CoreErrorResponseType::Extension("temporarily_unavailable".to_owned()), + None, + None, + )); + assert_eq!(refresh_error_status(&err), 503); + } + + #[test] + fn test_refresh_error_status_other() { + let err = TokenErr::Other("unexpected response from the token endpoint".to_owned()); + assert_eq!(refresh_error_status(&err), 503); + } + + #[test] + fn test_refresh_error_status_request() { + let err = TokenErr::Request(HttpClientError::Io(std::io::Error::new( + std::io::ErrorKind::ConnectionRefused, + "connection refused", + ))); + assert_eq!(refresh_error_status(&err), 503); + } + + type UserInfoErr = UserInfoError>; + + #[test] + fn test_user_info_error_status_unauthorized() { + let err = UserInfoErr::Response(http::StatusCode::UNAUTHORIZED, Vec::new(), "invalid_token".to_owned()); + assert_eq!(user_info_error_status(&err), 400); + } + + #[test] + fn test_user_info_error_status_unavailable() { + let err = UserInfoErr::Response(http::StatusCode::SERVICE_UNAVAILABLE, Vec::new(), "down".to_owned()); + assert_eq!(user_info_error_status(&err), 503); + } +} From d1a9822f7d69baf1af345dd04ac4ea98902a4c6d Mon Sep 17 00:00:00 2001 From: Justin Willhite <5132924+thejdubb02@users.noreply.github.com> Date: Tue, 27 Oct 2026 23:50:27 +0000 Subject: [PATCH 2/2] Pass provider 5xx through and test against a mock provider Add Error::is_server_error, return the provider's own 5xx status from user_info, and replace the injected-error tests with tests that drive the real client against a local mock provider. --- src/api/identity.rs | 4 +- src/error.rs | 5 +- src/sso_client.rs | 139 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 117 insertions(+), 31 deletions(-) diff --git a/src/api/identity.rs b/src/api/identity.rs index 69e3b9b5..bab1da2e 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -166,8 +166,8 @@ async fn refresh_login(data: ConnectData, conn: &DbConn, ip: &ClientIp) -> JsonR // --- // let members = Membership::find_confirmed_by_user(&user.uuid, conn).await; match auth::refresh_tokens(ip, &refresh_token, data.client_id, conn).await { - // Clients log out on 400 invalid_grant, so a provider outage is returned as-is (503). - Err(err) if err.get_code() == 503 => Err(err), + // Clients log out on 400 invalid_grant, so a provider outage is returned as-is (5xx). + Err(err) if err.is_server_error() => Err(err), Err(err) => { err_json!( json!({"error": "invalid_grant"}), diff --git a/src/error.rs b/src/error.rs index 95ea1a7d..589fecdc 100644 --- a/src/error.rs +++ b/src/error.rs @@ -159,11 +159,14 @@ impl Error { self } - #[must_use] pub const fn get_code(&self) -> u16 { self.code } + pub const fn is_server_error(&self) -> bool { + matches!(self.code, 500..=599) + } + #[must_use] pub fn with_event(mut self, event: ErrorEvent) -> Self { self.event = Some(event); diff --git a/src/sso_client.rs b/src/sso_client.rs index d5134393..159a30cc 100644 --- a/src/sso_client.rs +++ b/src/sso_client.rs @@ -376,59 +376,142 @@ where { match err { UserInfoError::Request(_) => 503, - UserInfoError::Response(status, ..) if status.is_server_error() => 503, + UserInfoError::Response(status, ..) if status.is_server_error() => status.as_u16(), _ => 400, } } #[cfg(test)] mod tests { + use std::{ + io::{Read, Write}, + net::{TcpListener, TcpStream}, + }; + + use openidconnect::{AuthUrl, IssuerUrl, JsonWebKeySet, TokenUrl, UserInfoUrl}; + use super::*; - type TokenErr = RequestTokenError, StandardErrorResponse>; + // Answers every request with `response`, after reading the whole request. + fn mock_provider(response: &'static str) -> String { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let base = format!("http://{}", listener.local_addr().unwrap()); + std::thread::spawn(move || { + for mut stream in listener.incoming().flatten() { + read_request(&mut stream); + stream.write_all(response.as_bytes()).ok(); + } + }); + base + } + + fn read_request(stream: &mut TcpStream) { + stream.set_read_timeout(Some(Duration::from_secs(5))).unwrap(); + let mut data = Vec::new(); + let mut buf = [0u8; 1024]; + loop { + let n = stream.read(&mut buf).unwrap_or(0); + if n == 0 { + return; + } + data.extend_from_slice(&buf[..n]); + if let Some(end) = data.windows(4).position(|w| w == b"\r\n\r\n") { + let headers = String::from_utf8_lossy(&data[..end]).to_ascii_lowercase(); + let length = headers + .lines() + .find_map(|line| line.strip_prefix("content-length:")) + .and_then(|value| value.trim().parse().ok()) + .unwrap_or(0); + if data.len() >= end + 4 + length { + return; + } + } + } + } + + fn unreachable_provider() -> String { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + format!("http://{}", listener.local_addr().unwrap()) + } + + fn test_client(base: &str) -> Client { + // main() installs this at startup. Err means another test already installed it. + rustls::crypto::ring::default_provider().install_default().ok(); + let core_client = CoreClient::new( + ClientId::new("vaultwarden".to_owned()), + IssuerUrl::new(base.to_owned()).unwrap(), + JsonWebKeySet::new(Vec::new()), + ) + .set_client_secret(ClientSecret::new("secret".to_owned())) + .set_auth_uri(AuthUrl::new(format!("{base}/auth")).unwrap()) + .set_token_uri(TokenUrl::new(format!("{base}/token")).unwrap()) + .set_user_info_url(UserInfoUrl::new(format!("{base}/userinfo")).unwrap()); + + Client { + http_client: OidcHttpClient { + client: reqwest::Client::new(), + }, + core_client, + } + } + + fn run(future: F) -> F::Output { + tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap().block_on(future) + } + + fn refresh_status(base: &str) -> u16 { + let result = run(test_client(base).exchange_refresh_token_impl("refresh".to_owned())); + result.map_or_else(|(code, _)| code, |_| 200) + } + + fn user_info_status(base: &str) -> u16 { + let result = run(test_client(base).user_info(AccessToken::new("access".to_owned()))); + result.map_or_else(|err| err.get_code(), |_| 200) + } #[test] - fn test_refresh_error_status_invalid_grant() { - let err = TokenErr::ServerResponse(StandardErrorResponse::new(CoreErrorResponseType::InvalidGrant, None, None)); - assert_eq!(refresh_error_status(&err), 400); + fn test_refresh_invalid_grant() { + let base = mock_provider( + "HTTP/1.1 400 Bad Request\r\nContent-Type: application/json\r\nContent-Length: 25\r\nConnection: close\r\n\r\n{\"error\":\"invalid_grant\"}", + ); + assert_eq!(refresh_status(&base), 400); } #[test] - fn test_refresh_error_status_temporarily_unavailable() { - let err = TokenErr::ServerResponse(StandardErrorResponse::new( - CoreErrorResponseType::Extension("temporarily_unavailable".to_owned()), - None, - None, - )); - assert_eq!(refresh_error_status(&err), 503); + fn test_refresh_temporarily_unavailable() { + let base = mock_provider( + "HTTP/1.1 400 Bad Request\r\nContent-Type: application/json\r\nContent-Length: 35\r\nConnection: close\r\n\r\n{\"error\":\"temporarily_unavailable\"}", + ); + assert_eq!(refresh_status(&base), 503); } #[test] - fn test_refresh_error_status_other() { - let err = TokenErr::Other("unexpected response from the token endpoint".to_owned()); - assert_eq!(refresh_error_status(&err), 503); + fn test_refresh_provider_error_page() { + let base = mock_provider( + "HTTP/1.1 502 Bad Gateway\r\nContent-Type: text/html\r\nContent-Length: 11\r\nConnection: close\r\n\r\nBad Gateway", + ); + assert_eq!(refresh_status(&base), 503); } #[test] - fn test_refresh_error_status_request() { - let err = TokenErr::Request(HttpClientError::Io(std::io::Error::new( - std::io::ErrorKind::ConnectionRefused, - "connection refused", - ))); - assert_eq!(refresh_error_status(&err), 503); + fn test_refresh_provider_unreachable() { + assert_eq!(refresh_status(&unreachable_provider()), 503); } - type UserInfoErr = UserInfoError>; + #[test] + fn test_user_info_invalid_token() { + let base = mock_provider("HTTP/1.1 401 Unauthorized\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"); + assert_eq!(user_info_status(&base), 400); + } #[test] - fn test_user_info_error_status_unauthorized() { - let err = UserInfoErr::Response(http::StatusCode::UNAUTHORIZED, Vec::new(), "invalid_token".to_owned()); - assert_eq!(user_info_error_status(&err), 400); + fn test_user_info_provider_unavailable() { + let base = mock_provider("HTTP/1.1 502 Bad Gateway\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"); + assert_eq!(user_info_status(&base), 502); } #[test] - fn test_user_info_error_status_unavailable() { - let err = UserInfoErr::Response(http::StatusCode::SERVICE_UNAVAILABLE, Vec::new(), "down".to_owned()); - assert_eq!(user_info_error_status(&err), 503); + fn test_user_info_provider_unreachable() { + assert_eq!(user_info_status(&unreachable_provider()), 503); } }