From f74bbfe91b2709672569054fdba119e5b712dc3a Mon Sep 17 00:00:00 2001 From: David Mulder Date: Mon, 25 Mar 2024 13:25:27 -0600 Subject: [PATCH 1/4] Use the Kanidm MFA patches Signed-off-by: David Mulder --- .gitmodules | 4 +- src/common/src/idprovider/himmelblau.rs | 181 +++++++++++++++--------- src/daemon/src/daemon.rs | 17 ++- src/kanidm | 2 +- 4 files changed, 134 insertions(+), 70 deletions(-) diff --git a/.gitmodules b/.gitmodules index c49ff159..34b776d8 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,5 +1,5 @@ [submodule "src/kanidm"] path = src/kanidm - url = https://github.com/dmulder/kanidm.git - branch = dmulder/mfa_capabilities + url = https://github.com/kanidm/kanidm.git + branch = master shallow = true diff --git a/src/common/src/idprovider/himmelblau.rs b/src/common/src/idprovider/himmelblau.rs index 5f1c8fa5..715eefb6 100644 --- a/src/common/src/idprovider/himmelblau.rs +++ b/src/common/src/idprovider/himmelblau.rs @@ -23,7 +23,7 @@ use std::sync::Arc; use std::thread::sleep; use std::time::Duration; use std::time::SystemTime; -use tokio::sync::RwLock; +use tokio::sync::{broadcast, RwLock}; use uuid::Uuid; use rand::Rng; @@ -181,6 +181,7 @@ impl IdProvider for HimmelblauMultiProvider { token: Option<&UserToken>, tpm: &mut tpm::BoxedDynTpm, machine_key: &tpm::MachineKey, + shutdown_rx: &broadcast::Receiver<()>, ) -> Result<(AuthRequest, AuthCredHandler), IdpError> { match split_username(account_id) { Some((_sam, domain)) => { @@ -188,7 +189,13 @@ impl IdProvider for HimmelblauMultiProvider { match providers.get(domain) { Some(provider) => { provider - .unix_user_online_auth_init(account_id, token, tpm, machine_key) + .unix_user_online_auth_init( + account_id, + token, + tpm, + machine_key, + shutdown_rx, + ) .await } None => Err(IdpError::NotFound), @@ -209,6 +216,7 @@ impl IdProvider for HimmelblauMultiProvider { keystore: &mut D, tpm: &mut tpm::BoxedDynTpm, machine_key: &tpm::MachineKey, + shutdown_rx: &broadcast::Receiver<()>, ) -> Result<(AuthResult, AuthCacheAction), IdpError> { match split_username(account_id) { Some((_sam, domain)) => { @@ -223,6 +231,7 @@ impl IdProvider for HimmelblauMultiProvider { keystore, tpm, machine_key, + shutdown_rx, ) .await } @@ -334,6 +343,14 @@ struct MFAAuthContinueI(MFAAuthContinue); #[allow(clippy::from_over_into)] impl Into> for MFAAuthContinueI { fn into(self) -> Vec { + let max_poll_attempts = match self.0.max_poll_attempts { + Some(n) => n.to_string(), + None => String::new(), + }; + let polling_interval = match self.0.polling_interval { + Some(n) => n.to_string(), + None => String::new(), + }; vec![ self.0.mfa_method, self.0.msg, @@ -344,17 +361,29 @@ impl Into> for MFAAuthContinueI { self.0.url_end_auth, self.0.url_begin_auth, self.0.url_post, + max_poll_attempts, + polling_interval, ] } } -impl From> for MFAAuthContinueI { - fn from(src: Vec) -> Self { +impl From<&Vec> for MFAAuthContinueI { + fn from(src: &Vec) -> Self { + let max_poll_attempts: Option = if src[9].is_empty() { + None + } else { + src[9].parse().ok() + }; + let polling_interval: Option = if src[10].is_empty() { + None + } else { + src[10].parse().ok() + }; MFAAuthContinueI(MFAAuthContinue { mfa_method: src[0].clone(), msg: src[1].clone(), - max_poll_attempts: None, - polling_interval: None, + max_poll_attempts, + polling_interval, session_id: src[2].clone(), flow_token: src[3].clone(), ctx: src[4].clone(), @@ -445,6 +474,7 @@ impl IdProvider for HimmelblauProvider { _token: Option<&UserToken>, _tpm: &mut tpm::BoxedDynTpm, _machine_key: &tpm::MachineKey, + _shutdown_rx: &broadcast::Receiver<()>, ) -> Result<(AuthRequest, AuthCredHandler), IdpError> { Ok((AuthRequest::Password, AuthCredHandler::Password)) } @@ -457,8 +487,10 @@ impl IdProvider for HimmelblauProvider { keystore: &mut D, tpm: &mut tpm::BoxedDynTpm, machine_key: &tpm::MachineKey, + shutdown_rx: &broadcast::Receiver<()>, ) -> Result<(AuthResult, AuthCacheAction), IdpError> { - match (cred_handler, pam_next_req) { + let mut shutdown_rx_cl = shutdown_rx.resubscribe(); + match (&cred_handler, pam_next_req) { (AuthCredHandler::Password, PamAuthRequest::Password { cred }) => { let mut scopes = vec!["GroupMember.Read.All"]; if !self.is_domain_joined(keystore).await { @@ -499,11 +531,12 @@ impl IdProvider for HimmelblauProvider { }; match resp.mfa_method.as_str() { "PhoneAppNotification" | "PhoneAppOTP" => { + let msg = resp.msg.clone(); + *cred_handler = AuthCredHandler::MFA { + data: MFAAuthContinueI(resp).into(), + }; return Ok(( - AuthResult::Next(AuthRequest::MFACode { - msg: resp.msg.clone(), - data: MFAAuthContinueI(resp).into(), - }), + AuthResult::Next(AuthRequest::MFACode { msg }), /* An MFA auth cannot cache the password. This would * lead to a potential downgrade to SFA attack (where * the attacker auths with a stolen password, then @@ -512,18 +545,18 @@ impl IdProvider for HimmelblauProvider { )); } _ => { + let msg = resp.msg.clone(); + let polling_interval = resp.polling_interval.ok_or({ + error!("Invalid response from the server"); + IdpError::BadRequest + })?; + *cred_handler = AuthCredHandler::MFA { + data: MFAAuthContinueI(resp).into(), + }; return Ok(( AuthResult::Next(AuthRequest::MFAPoll { - msg: resp.msg.clone(), - max_poll_attempts: resp.max_poll_attempts.ok_or({ - error!("Invalid response from the server"); - IdpError::BadRequest - })?, - polling_interval: resp.polling_interval.ok_or({ - error!("Invalid response from the server"); - IdpError::BadRequest - })?, - data: MFAAuthContinueI(resp).into(), + msg, + polling_interval, }), /* An MFA auth cannot cache the password. This would * lead to a potential downgrade to SFA attack (where @@ -613,11 +646,12 @@ impl IdProvider for HimmelblauProvider { }; match resp.mfa_method.as_str() { "PhoneAppNotification" | "PhoneAppOTP" => { + let msg = resp.msg.clone(); + *cred_handler = AuthCredHandler::MFA { + data: MFAAuthContinueI(resp).into(), + }; return Ok(( - AuthResult::Next(AuthRequest::MFACode { - msg: resp.msg.clone(), - data: MFAAuthContinueI(resp).into(), - }), + AuthResult::Next(AuthRequest::MFACode { msg }), /* An MFA auth cannot cache the password. This would * lead to a potential downgrade to SFA attack (where * the attacker auths with a stolen password, then @@ -626,18 +660,18 @@ impl IdProvider for HimmelblauProvider { )); } _ => { + let msg = resp.msg.clone(); + let polling_interval = resp.polling_interval.ok_or({ + error!("Invalid response from the server"); + IdpError::BadRequest + })?; + *cred_handler = AuthCredHandler::MFA { + data: MFAAuthContinueI(resp).into(), + }; return Ok(( AuthResult::Next(AuthRequest::MFAPoll { - msg: resp.msg.clone(), - max_poll_attempts: resp.max_poll_attempts.ok_or({ - error!("Invalid response from the server"); - IdpError::BadRequest - })?, - polling_interval: resp.polling_interval.ok_or({ - error!("Invalid response from the server"); - IdpError::BadRequest - })?, - data: MFAAuthContinueI(resp).into(), + msg, + polling_interval, }), /* An MFA auth cannot cache the password. This would * lead to a potential downgrade to SFA attack (where @@ -737,7 +771,7 @@ impl IdProvider for HimmelblauProvider { Err(e) => Err(e), } } - (_, PamAuthRequest::MFACode { cred, data }) => { + (AuthCredHandler::MFA { data }, PamAuthRequest::MFACode { cred }) => { let mut token = self .client .write() @@ -785,36 +819,51 @@ impl IdProvider for HimmelblauProvider { Err(e) => Err(e), } } - (_, PamAuthRequest::MFAPoll { poll_attempt, data }) => { - let mut token = match self - .client - .write() - .await - .acquire_token_by_mfa_flow( - account_id, - None, - Some(poll_attempt), - MFAAuthContinueI::from(data).0, - ) - .await - { - Ok(token) => token, - Err(e) => match e { - MsalError::MFAPollContinue => { - return Ok(( - AuthResult::Next(AuthRequest::MFAPollWait), - /* An MFA auth cannot cache the password. This would - * lead to a potential downgrade to SFA attack (where - * the attacker auths with a stolen password, then - * disconnects the network to complete the auth). */ - AuthCacheAction::None, - )); - } - e => { - error!("{:?}", e); - return Err(IdpError::NotFound); - } - }, + (AuthCredHandler::MFA { data }, PamAuthRequest::MFAPoll) => { + let flow = MFAAuthContinueI::from(data).0; + let max_poll_attempts = flow.max_poll_attempts.ok_or({ + error!("Invalid response from the server"); + IdpError::BadRequest + })?; + let polling_interval = flow.polling_interval.ok_or({ + error!("Invalid response from the server"); + IdpError::BadRequest + })?; + let mut poll_attempt = 1; + let mut token = loop { + if poll_attempt > max_poll_attempts { + error!("MFA polling timed out"); + return Err(IdpError::BadRequest); + } + if shutdown_rx_cl.try_recv().ok().is_some() { + debug!("Received a signal to shutdown, bailing MFA poll"); + return Err(IdpError::BadRequest); + } + sleep(Duration::from_secs(polling_interval.into())); + match self + .client + .write() + .await + .acquire_token_by_mfa_flow( + account_id, + None, + Some(poll_attempt), + MFAAuthContinueI::from(data).0, + ) + .await + { + Ok(token) => break token, + Err(e) => match e { + MsalError::MFAPollContinue => { + poll_attempt += 1; + continue; + } + e => { + error!("{:?}", e); + return Err(IdpError::NotFound); + } + }, + } }; if !self.is_domain_joined(keystore).await { self.join_domain(tpm, &token, keystore, machine_key) diff --git a/src/daemon/src/daemon.rs b/src/daemon/src/daemon.rs index c97705f1..dd8e318c 100644 --- a/src/daemon/src/daemon.rs +++ b/src/daemon/src/daemon.rs @@ -203,6 +203,10 @@ async fn handle_client( let mut reqs = Framed::new(sock, ClientCodec); let mut pam_auth_session_state = None; + // Setup a broadcast channel so that if we have an unexpected disconnection, we can + // tell consumers to stop work. + let (shutdown_tx, _shutdown_rx) = broadcast::channel(1); + trace!("Waiting for requests ..."); while let Some(Ok(req)) = reqs.next().await { let resp = match req { @@ -296,7 +300,10 @@ async fn handle_client( } None => { match cachelayer - .pam_account_authenticate_init(account_id.as_str()) + .pam_account_authenticate_init( + account_id.as_str(), + shutdown_tx.subscribe(), + ) .await { Ok((auth_session, pam_auth_response)) => { @@ -408,6 +415,14 @@ async fn handle_client( debug!("flushed response!"); } + // Signal any tasks that they need to stop. + if let Err(shutdown_err) = shutdown_tx.send(()) { + warn!( + ?shutdown_err, + "Unable to signal tasks to stop, they will naturally timeout instead." + ) + } + // Disconnect them debug!("Disconnecting client ..."); Ok(()) diff --git a/src/kanidm b/src/kanidm index c550b391..a61038f4 160000 --- a/src/kanidm +++ b/src/kanidm @@ -1 +1 @@ -Subproject commit c550b391c71d40b8ced77dbf9c732aa608cf5800 +Subproject commit a61038f40024373f2a88c7a2cfa04ab984ff71a9 From 785c075e9be938d1f2f5ddfa96a160fccf9c74d0 Mon Sep 17 00:00:00 2001 From: David Mulder Date: Fri, 29 Mar 2024 10:15:30 -0600 Subject: [PATCH 2/4] Revert dependabot update that broke the build Kanidm depends on these older versions of opentelemetry Signed-off-by: David Mulder --- Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0f6733e4..aaa05fa1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -86,14 +86,14 @@ utoipa = "4.0.0" utoipa-swagger-ui = "4.0.0" opentelemetry = { version = "0.20.0" } opentelemetry_api = { version = "0.20.0", features = ["logs", "metrics"] } -opentelemetry-otlp = { version = "0.15.0", default-features = false, features = [ +opentelemetry-otlp = { version = "0.13.0", default-features = false, features = [ "serde", "logs", "metrics", "http-proto", "grpc-tonic", ] } -opentelemetry_sdk = "0.22.1" +opentelemetry_sdk = "0.20.0" opentelemetry-stdout = { version = "0.1.0", features = [ "logs", "metrics", From e56926fb22811571660a4c2df6bd8d5656e0775d Mon Sep 17 00:00:00 2001 From: David Mulder Date: Fri, 29 Mar 2024 10:24:22 -0600 Subject: [PATCH 3/4] fix dependabot --- .github/dependabot.yml | 1 + Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 2a844324..f10ba185 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -15,3 +15,4 @@ updates: # Kanidm currently depends on these outdated versions of opentelemetry - dependency-name: "opentelemetry-otlp" - dependency-name: "opentelemetry_sdk" + - dependency-name: "tracing-opentelemetry" diff --git a/Cargo.toml b/Cargo.toml index aaa05fa1..3652b96a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -100,6 +100,6 @@ opentelemetry-stdout = { version = "0.1.0", features = [ "trace", ] } tonic = "0.10.2" -tracing-opentelemetry = "0.23.0" +tracing-opentelemetry = "0.21.0" compact_jwt = { version = "0.3.5", features = ["hsm-crypto", "msextensions"] } kanidm-hsm-crypto = { version = "^0.2.0", features = ["msextensions"] } From cc1f4066cab31ecaa5134ae915cea6c3393f6919 Mon Sep 17 00:00:00 2001 From: David Mulder Date: Fri, 29 Mar 2024 10:25:21 -0600 Subject: [PATCH 4/4] Revert dependabot hsm-crypto update that broke the build Signed-off-by: David Mulder --- .github/dependabot.yml | 2 ++ Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index f10ba185..7cc591a3 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -16,3 +16,5 @@ updates: - dependency-name: "opentelemetry-otlp" - dependency-name: "opentelemetry_sdk" - dependency-name: "tracing-opentelemetry" + # This requires an update to compact-jwt first, awaiting that update + - dependency-name: "kanidm-hsm-crypto" diff --git a/Cargo.toml b/Cargo.toml index 3652b96a..a01ae486 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -102,4 +102,4 @@ opentelemetry-stdout = { version = "0.1.0", features = [ tonic = "0.10.2" tracing-opentelemetry = "0.21.0" compact_jwt = { version = "0.3.5", features = ["hsm-crypto", "msextensions"] } -kanidm-hsm-crypto = { version = "^0.2.0", features = ["msextensions"] } +kanidm-hsm-crypto = { version = "^0.1.6", features = ["msextensions"] }