diff --git a/.sqlx/query-e20bb4d7529adc50b620a5a96bdccea9f2f51f0f45e1833555263cb776daf0d0.json b/.sqlx/query-f50806305abd7402f21e5ed8c9e9688c6ea78ba2583da4612d1bff0a13813d7b.json similarity index 95% rename from .sqlx/query-e20bb4d7529adc50b620a5a96bdccea9f2f51f0f45e1833555263cb776daf0d0.json rename to .sqlx/query-f50806305abd7402f21e5ed8c9e9688c6ea78ba2583da4612d1bff0a13813d7b.json index e29f0eff37..7e4f06835f 100644 --- a/.sqlx/query-e20bb4d7529adc50b620a5a96bdccea9f2f51f0f45e1833555263cb776daf0d0.json +++ b/.sqlx/query-f50806305abd7402f21e5ed8c9e9688c6ea78ba2583da4612d1bff0a13813d7b.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT id, username, password_hash, last_name, first_name, email, phone, mfa_enabled, totp_enabled, email_mfa_enabled, totp_secret, email_mfa_secret, mfa_method \"mfa_method: _\", recovery_codes, is_active, openid_sub, from_ldap, ldap_pass_randomized, ldap_rdn, ldap_user_path, ldap_remote_enrollment_completed, enrollment_pending FROM \"user\" WHERE email ILIKE $1", + "query": "SELECT id, username, password_hash, last_name, first_name, email, phone, mfa_enabled, totp_enabled, email_mfa_enabled, totp_secret, email_mfa_secret, mfa_method \"mfa_method: _\", recovery_codes, is_active, openid_sub, from_ldap, ldap_pass_randomized, ldap_rdn, ldap_user_path, ldap_remote_enrollment_completed, enrollment_pending FROM \"user\" WHERE LOWER(email) = LOWER($1)", "describe": { "columns": [ { @@ -156,5 +156,5 @@ false ] }, - "hash": "e20bb4d7529adc50b620a5a96bdccea9f2f51f0f45e1833555263cb776daf0d0" + "hash": "f50806305abd7402f21e5ed8c9e9688c6ea78ba2583da4612d1bff0a13813d7b" } diff --git a/crates/defguard_common/src/db/models/user.rs b/crates/defguard_common/src/db/models/user.rs index 7cbbe8a197..e73f9d73c1 100644 --- a/crates/defguard_common/src/db/models/user.rs +++ b/crates/defguard_common/src/db/models/user.rs @@ -888,7 +888,7 @@ impl User { totp_enabled, email_mfa_enabled, totp_secret, email_mfa_secret, \ mfa_method \"mfa_method: _\", recovery_codes, is_active, openid_sub, \ from_ldap, ldap_pass_randomized, ldap_rdn, ldap_user_path, ldap_remote_enrollment_completed, enrollment_pending \ - FROM \"user\" WHERE email ILIKE $1", + FROM \"user\" WHERE LOWER(email) = LOWER($1)", email ) .fetch_optional(executor) @@ -1358,6 +1358,48 @@ mod test { secret::SecretStringWrapper, }; + #[sqlx::test] + async fn test_find_by_email_is_exact_not_a_pattern( + _: PgPoolOptions, + options: PgConnectOptions, + ) { + let pool = setup_pool(options).await; + + User::new( + "hpotter", + Some("pass123"), + "Potter", + "Harry", + "h.potter@hogwart.edu.uk", + None, + ) + .save(&pool) + .await + .unwrap(); + + // Lookups stay case-insensitive. + assert!( + User::find_by_email(&pool, "H.Potter@Hogwart.Edu.UK") + .await + .unwrap() + .is_some() + ); + + // The argument is a value, not a `LIKE` pattern: callers pass externally + // supplied addresses, so wildcards must never match another user. + for pattern in [ + "%", + "%@%", + "h.potter@hogwart.edu.u_", + "_.potter@hogwart.edu.uk", + ] { + assert!( + User::find_by_email(&pool, pattern).await.unwrap().is_none(), + "email lookup treated {pattern:?} as a pattern" + ); + } + } + #[sqlx::test] async fn test_mfa_code(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; diff --git a/crates/defguard_common/src/db/models/wizard.rs b/crates/defguard_common/src/db/models/wizard.rs index 7c7a34bd32..f95eb8e739 100644 --- a/crates/defguard_common/src/db/models/wizard.rs +++ b/crates/defguard_common/src/db/models/wizard.rs @@ -122,15 +122,13 @@ impl Wizard { .fetch_one(executor) .await?; - let active_wizard; - - if has_auto_adopt_flags { - active_wizard = ActiveWizard::AutoAdoption; + let active_wizard = if has_auto_adopt_flags { + ActiveWizard::AutoAdoption } else if is_fresh_instance { - active_wizard = ActiveWizard::Initial; + ActiveWizard::Initial } else { - active_wizard = ActiveWizard::Migration; - } + ActiveWizard::Migration + }; wizard.active_wizard = active_wizard; diff --git a/crates/defguard_core/src/enterprise/handlers/openid_login.rs b/crates/defguard_core/src/enterprise/handlers/openid_login.rs index 4eadbc6326..49c1075775 100644 --- a/crates/defguard_core/src/enterprise/handlers/openid_login.rs +++ b/crates/defguard_core/src/enterprise/handlers/openid_login.rs @@ -322,6 +322,25 @@ pub async fn user_from_claims( user } None => { + // Only an explicit `false` is rejected. Some providers omit `email_verified`. + match token_claims.email_verified() { + Some(false) => { + warn!( + "OpenID login: provider reported email address {} as unverified, \ + refusing to link or create an account", + email.as_str() + ); + return Err(WebError::Authorization( + "Provider did not verify the email address".into(), + )); + } + None => debug!( + "OpenID login: provider sent no email_verified claim for {}, so the address \ + cannot be confirmed as belonging to this identity", + email.as_str() + ), + Some(true) => {} + } if let Some(mut user) = User::find_by_email(pool, email).await? { if !user.is_active { debug!("User {} tried to log in, but is disabled", user.username); diff --git a/crates/defguard_proxy_manager/src/servers/enrollment.rs b/crates/defguard_proxy_manager/src/servers/enrollment.rs index 506d5cd1de..69416dfbca 100644 --- a/crates/defguard_proxy_manager/src/servers/enrollment.rs +++ b/crates/defguard_proxy_manager/src/servers/enrollment.rs @@ -1113,9 +1113,9 @@ impl EnrollmentServer { if user.is_enrolled() { return Err(Status::permission_denied("User is already enrolled")); } - let mfa_method: MFAMethod; + // enable corresponding MFA - match method { + let mfa_method = match method { MfaMethod::Email => { if !user.verify_email_mfa_code(&request.code) { return Err(Status::invalid_argument("Email code invalid".to_owned())); @@ -1123,7 +1123,7 @@ impl EnrollmentServer { user.enable_email_mfa(&self.pool) .await .map_err(|_| Status::internal("Enabling method failed.".to_owned()))?; - mfa_method = MFAMethod::Email; + MFAMethod::Email } MfaMethod::Totp => { if !user.verify_totp_code(&request.code) { @@ -1132,12 +1132,12 @@ impl EnrollmentServer { user.enable_totp(&self.pool) .await .map_err(|_| Status::internal("Enabling method failed.".to_owned()))?; - mfa_method = MFAMethod::OneTimePassword; + MFAMethod::OneTimePassword } _ => { return Err(Status::invalid_argument("Method not supported")); } - } + }; user.enable_mfa(&self.pool) .await .map_err(|_| Status::internal("Enabling MFA on the account failed.".to_owned()))?; diff --git a/crates/defguard_proxy_manager/src/tests/common/mod.rs b/crates/defguard_proxy_manager/src/tests/common/mod.rs index 476b405eed..840ee3dbcf 100644 --- a/crates/defguard_proxy_manager/src/tests/common/mod.rs +++ b/crates/defguard_proxy_manager/src/tests/common/mod.rs @@ -762,8 +762,10 @@ struct OidcProviderState { /// * `POST /token` – exchange authorization code for ID token /// /// ### Code format -/// The authorization code must be `"{sub}:{email}:{nonce}"`. The `/token` -/// handler parses those three components and embeds them in the signed JWT. +/// The authorization code must be `"{sub}:{email}:{nonce}:{email_verified}"`. +/// The `/token` handler parses those components and embeds them in the signed +/// JWT. `email_verified` is `"true"` or `"false"`; omitting it drops the +/// claim from the token. pub(crate) struct MockOidcProvider { /// HTTP base URL of the mock server, e.g. `http://127.0.0.1:45321`. pub(crate) base_url: String, @@ -871,25 +873,35 @@ async fn oidc_jwks(State(state): State) -> Json, Form(params): Form>, ) -> Json { let code = params.get("code").cloned().unwrap_or_default(); - // code format: "{sub}:{email}:{nonce}" - let mut parts = code.splitn(3, ':'); + // code format: "{sub}:{email}:{nonce}:{email_verified}" + let mut parts = code.splitn(4, ':'); let sub = parts.next().unwrap_or("unknown-sub").to_owned(); let email = parts.next().unwrap_or("unknown@example.com").to_owned(); let nonce = parts.next().unwrap_or("").to_owned(); + let email_verified = match parts.next() { + Some("true") => Some(true), + Some("false") => Some(false), + None => None, + Some(other) => { + panic!("unexpected email_verified segment in authorization code: {other:?}") + } + }; let now = SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() .as_secs(); - let claims = serde_json::json!({ + let mut claims = serde_json::json!({ "iss": state.base_url, "sub": sub, "aud": state.client_id, @@ -901,6 +913,9 @@ async fn oidc_token( "family_name": "OidcUser", "name": "Test OidcUser", }); + if let Some(email_verified) = email_verified { + claims["email_verified"] = serde_json::json!(email_verified); + } let mut header = Header::new(Algorithm::RS256); header.kid = None; diff --git a/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/oidc.rs b/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/oidc.rs index 536732d9dc..bc70a280bc 100644 --- a/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/oidc.rs +++ b/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/oidc.rs @@ -1,5 +1,8 @@ #![allow(deprecated)] -use defguard_common::db::models::settings::{Settings, update_current_settings}; +use defguard_common::db::models::{ + User, + settings::{Settings, update_current_settings}, +}; use defguard_core::{ db::models::enrollment::Token, enterprise::{ @@ -21,10 +24,11 @@ use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; use tokio::time::timeout; use super::support::{ - assert_error_response, assert_vpn_session_exists, clear_test_license, complete_proxy_handshake, - create_external_mfa_network, create_oidc_provider, create_user, create_user_with_device, - expect_bidi_mfa_success, make_device_info, make_oidc_code, send_mfa_finish, send_mfa_start, - set_public_proxy_url, set_test_license_business, + EmailVerified, assert_error_response, assert_error_response_details, assert_vpn_session_exists, + clear_test_license, complete_proxy_handshake, create_external_mfa_network, + create_oidc_provider, create_user, create_user_with_device, expect_bidi_mfa_success, + make_device_info, make_oidc_code, make_oidc_code_with_email_verified, send_mfa_finish, + send_mfa_start, set_public_proxy_url, set_test_license_business, }; use crate::tests::common::{HandlerTestContext, MockOidcProvider, RECEIVE_TIMEOUT}; @@ -528,6 +532,189 @@ async fn test_auth_callback_exchanges_code_for_enrollment_token( context.finish().await.expect_server_finished().await; } +/// When the provider marks the email as unverified, the callback must not merge +/// the identity into a pre-existing account: it returns `PermissionDenied` and +/// leaves the target user's `openid_sub` unset. +#[sqlx::test] +async fn test_auth_callback_unverified_email_does_not_merge_into_existing_account( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let mut context = HandlerTestContext::new(options).await; + complete_proxy_handshake(&mut context).await; + set_test_license_business(); + + // Target account: pre-existing and never used OIDC, so `openid_sub` is NULL. + let target = create_user(&context.pool).await; + + let mock = MockOidcProvider::start().await; + let _provider = create_oidc_provider(&context.pool, &mock).await; + set_public_proxy_url(&context.pool, &mock.base_url).await; + + // The attacker's `sub` is unknown, the email matches the target, and the + // provider reports it unverified. + let raw_nonce = "test-nonce-unverified-email"; + let code = make_oidc_code_with_email_verified( + "attacker-sub", + &target.email, + raw_nonce, + EmailVerified::Unverified, + ); + + context.mock_proxy().send_request(CoreRequest { + id: 12, + device_info: None, + payload: Some(core_request::Payload::AuthCallback(AuthCallbackRequest { + code, + nonce: raw_nonce.to_owned(), + })), + }); + + let response = context.mock_proxy_mut().recv_outbound().await; + let (status, message) = assert_error_response_details(&response); + assert_eq!( + status, + tonic::Code::PermissionDenied, + "expected PermissionDenied when the provider reports the email unverified" + ); + assert!( + message.contains("did not verify the email address"), + "expected the unverified-email rejection, got: {message}" + ); + + // The target account must not be bound to the attacker's identity. + let target = User::find_by_email(&context.pool, &target.email) + .await + .expect("db query failed for target user") + .expect("target user should still exist"); + assert!( + target.openid_sub.is_none(), + "unverified email must not set openid_sub on the existing account" + ); + + clear_test_license(); + context.finish().await.expect_server_finished().await; +} + +/// Many providers omit `email_verified` altogether, so an absent claim must stay +/// permissive: the identity still links to the account matching its email. Pins the +/// compatibility decision behind rejecting only an explicit `false`. +#[sqlx::test] +async fn test_auth_callback_absent_email_verified_claim_still_links_account( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let mut context = HandlerTestContext::new(options).await; + complete_proxy_handshake(&mut context).await; + set_test_license_business(); + + let user = create_user(&context.pool).await; + + let mock = MockOidcProvider::start().await; + let _provider = create_oidc_provider(&context.pool, &mock).await; + set_public_proxy_url(&context.pool, &mock.base_url).await; + + let raw_nonce = "test-nonce-absent-email-verified"; + let code = make_oidc_code_with_email_verified( + "absent-claim-sub", + &user.email, + raw_nonce, + EmailVerified::Absent, + ); + + context.mock_proxy().send_request(CoreRequest { + id: 14, + device_info: None, + payload: Some(core_request::Payload::AuthCallback(AuthCallbackRequest { + code, + nonce: raw_nonce.to_owned(), + })), + }); + + let response = context.mock_proxy_mut().recv_outbound().await; + let auth_cb = match &response.payload { + Some(core_response::Payload::AuthCallback(r)) => r, + Some(core_response::Payload::CoreError(e)) => panic!( + "an absent email_verified claim must not block login: status={} msg={}", + e.status_code, e.message + ), + other => panic!( + "expected AuthCallback response, got: {:?}", + other.as_ref().map(std::mem::discriminant) + ), + }; + + let token = Token::find_by_id(&context.pool, &auth_cb.token) + .await + .expect("db query failed for enrollment token"); + assert_eq!( + token.user_id, user.id, + "enrollment token must belong to the matched user" + ); + + clear_test_license(); + context.finish().await.expect_server_finished().await; +} + +/// Emails are unique, so an account created from an unverified address claims that +/// identity for good. With no account to merge into, the callback must still fail +/// and create nothing. +#[sqlx::test] +async fn test_auth_callback_unverified_email_does_not_create_account( + _: PgPoolOptions, + options: PgConnectOptions, +) { + let mut context = HandlerTestContext::new(options).await; + complete_proxy_handshake(&mut context).await; + set_test_license_business(); + + let mock = MockOidcProvider::start().await; + let _provider = create_oidc_provider(&context.pool, &mock).await; + set_public_proxy_url(&context.pool, &mock.base_url).await; + + // No account holds this address, so this exercises the creation path. + let email = "no-such-user@example.com"; + let raw_nonce = "test-nonce-unverified-email-no-account"; + let code = make_oidc_code_with_email_verified( + "attacker-sub", + email, + raw_nonce, + EmailVerified::Unverified, + ); + + context.mock_proxy().send_request(CoreRequest { + id: 13, + device_info: None, + payload: Some(core_request::Payload::AuthCallback(AuthCallbackRequest { + code, + nonce: raw_nonce.to_owned(), + })), + }); + + let response = context.mock_proxy_mut().recv_outbound().await; + let (status, message) = assert_error_response_details(&response); + assert_eq!( + status, + tonic::Code::PermissionDenied, + "expected PermissionDenied when the provider reports the email unverified" + ); + assert!( + message.contains("did not verify the email address"), + "expected the unverified-email rejection, got: {message}" + ); + + assert!( + User::find_by_email(&context.pool, email) + .await + .expect("db query failed for the claimed email") + .is_none(), + "unverified email must not create an account" + ); + + clear_test_license(); + context.finish().await.expect_server_finished().await; +} + #[sqlx::test] async fn test_auth_callback_blocked_by_license_limit_emits_user_import_blocked_event( _: PgPoolOptions, diff --git a/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/support.rs b/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/support.rs index fbfc93d5dd..a2fb2a97df 100644 --- a/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/support.rs +++ b/crates/defguard_proxy_manager/src/tests/proxy_manager/handler/support.rs @@ -94,8 +94,17 @@ pub(crate) fn assert_device_config_response(response: &CoreResponse) -> &DeviceC /// Assert that a `CoreResponse` carries a `CoreError` payload and return the /// tonic status code. pub(crate) fn assert_error_response(response: &CoreResponse) -> Code { + assert_error_response_details(response).0 +} + +/// Assert that a `CoreResponse` carries a `CoreError` payload and return the +/// tonic status code and message. Use when the status code alone cannot tell two +/// rejections apart. +pub(crate) fn assert_error_response_details(response: &CoreResponse) -> (Code, &str) { match &response.payload { - Some(core_response::Payload::CoreError(err)) => Code::from_i32(err.status_code), + Some(core_response::Payload::CoreError(err)) => { + (Code::from_i32(err.status_code), err.message.as_str()) + } other => panic!( "expected CoreError response, got: {:?}", other.as_ref().map(discriminant) @@ -764,10 +773,34 @@ pub(crate) async fn set_public_proxy_url(pool: &PgPool, url: &str) { .expect("failed to update public_proxy_url in settings"); } +/// The `email_verified` claim a mock ID token should carry. +pub(crate) enum EmailVerified { + /// `"email_verified": true`. + Verified, + /// `"email_verified": false`. + Unverified, + /// The claim is omitted, as many providers do. + Absent, +} + /// Build the authorization code expected by `MockOidcProvider`'s `/token` -/// endpoint. Format: `"{sub}:{email}:{nonce}"`. +/// endpoint, with the email marked as verified. pub(crate) fn make_oidc_code(sub: &str, email: &str, nonce: &str) -> String { - format!("{sub}:{email}:{nonce}") + make_oidc_code_with_email_verified(sub, email, nonce, EmailVerified::Verified) +} + +/// Build an authorization code carrying the given `email_verified` claim. +pub(crate) fn make_oidc_code_with_email_verified( + sub: &str, + email: &str, + nonce: &str, + email_verified: EmailVerified, +) -> String { + match email_verified { + EmailVerified::Verified => format!("{sub}:{email}:{nonce}:true"), + EmailVerified::Unverified => format!("{sub}:{email}:{nonce}:false"), + EmailVerified::Absent => format!("{sub}:{email}:{nonce}"), + } } /// Send an `ActivateUser` request through the handler and return the raw diff --git a/flake.lock b/flake.lock index 90c4cf4257..92d59ee0db 100644 --- a/flake.lock +++ b/flake.lock @@ -32,11 +32,11 @@ }, "nixpkgs": { "locked": { - "lastModified": 1785090369, - "narHash": "sha256-m0pDuRJG7EDo9ri+4Ksu83VsI+PlxNC9lNBfydejce4=", + "lastModified": 1787360063, + "narHash": "sha256-dt4WdcvsA8/RCe+VZZwqU0X+XMM3wBbGCWA0/sFWzGo=", "owner": "NixOS", "repo": "nixpkgs", - "rev": "624af665418d3c65d544145b4d34ad696439570e", + "rev": "2c423e03bbafcff28bfadc6781a4a8257f205cb5", "type": "github" }, "original": { @@ -74,11 +74,11 @@ ] }, "locked": { - "lastModified": 1785302874, - "narHash": "sha256-fpKEww3TJoo1ANHO2q918ei+ayOrp0YEQAO1DuBLOB4=", + "lastModified": 1787540965, + "narHash": "sha256-48/4bbmK3W3Av3YPnrFb/tVQXPvBwU6kyM3Pb13VVD8=", "owner": "oxalica", "repo": "rust-overlay", - "rev": "b99d48435bc3e34309d2c7ae6f7d45e77a156c38", + "rev": "ab450d47a3f906d19de1b332915bfc6e5b29c853", "type": "github" }, "original": {