From 9dbb5ccb592d22f4513b7a2f5d1d67d645042e45 Mon Sep 17 00:00:00 2001 From: Firstyear Date: Mon, 28 Dec 2020 09:41:16 +1000 Subject: [PATCH] Unixd - NXCache of unknown items (#338) Previously we would only cache "hits" - items that kanidm is aware of and did know about. However, this mean querying a raw uid/gid number that was not known to files or kanidm would result in kanidm doing an online check each request. This adds a NXcache to cache misses, so they can be served as misses, faster, and to reduce load on the main kanidm servers. Fixes #336 --- Cargo.lock | 43 ++++++++---- kanidm_book/src/pam_and_nsswitch.md | 4 +- kanidm_client/src/lib.rs | 2 + kanidm_unix_int/Cargo.toml | 2 + kanidm_unix_int/src/cache.rs | 77 ++++++++++++++++++++- kanidm_unix_int/src/daemon.rs | 2 + kanidm_unix_int/tests/cache_layer_test.rs | 82 ++++++++++++++++++++++- kanidmd/src/lib/credential/mod.rs | 17 +++-- kanidmd/src/lib/idm/account.rs | 1 + kanidmd/src/lib/idm/authsession.rs | 16 ++--- kanidmd/src/lib/idm/mfareg.rs | 27 +++----- kanidmd/src/lib/idm/server.rs | 2 +- 12 files changed, 221 insertions(+), 54 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c36a7cf6f..472baa1dc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1336,10 +1336,11 @@ dependencies = [ [[package]] name = "ghash" -version = "0.3.0" +version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d6e27f0689a6e15944bdce7e45425efb87eaa8ab0c6e87f11d0987a9133e2531" +checksum = "97304e4cd182c3846f7575ced3890c53012ce534ad9114046b0a9e00bb30a375" dependencies = [ + "opaque-debug 0.3.0", "polyval", ] @@ -1397,6 +1398,9 @@ name = "hashbrown" version = "0.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d7afe4a420e3fe79967a00898cc1f4db7c8a49a9333a29f8a4bd76a253d5cd04" +dependencies = [ + "ahash 0.4.7", +] [[package]] name = "heck" @@ -1773,6 +1777,7 @@ dependencies = [ "libc", "libsqlite3-sys", "log", + "lru", "r2d2", "r2d2_sqlite", "reqwest", @@ -1905,6 +1910,15 @@ dependencies = [ "cfg-if 0.1.10", ] +[[package]] +name = "lru" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3aae342b73d57ad0b8b364bd12584819f2c1fe9114285dfcf8b0722607671635" +dependencies = [ + "hashbrown 0.9.1", +] + [[package]] name = "lru-cache" version = "0.1.2" @@ -2406,11 +2420,12 @@ dependencies = [ [[package]] name = "polyval" -version = "0.4.3" +version = "0.4.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4fd92d8e0c06d08525d2e2643cc2b5c80c69ae8eb12c18272d501cd7079ccc0" +checksum = "eebcc4aa140b9abd2bc40d9c3f7ccec842679cd79045ac3a7ac698c1a064b7cd" dependencies = [ "cpuid-bool 0.2.0", + "opaque-debug 0.3.0", "universal-hash", ] @@ -2956,9 +2971,9 @@ dependencies = [ [[package]] name = "signal-hook" -version = "0.1.16" +version = "0.1.17" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "604508c1418b99dfe1925ca9224829bb2a8a9a04dda655cc01fcad46f4ab05ed" +checksum = "7e31d442c16f047a671b5a71e2161d6e68814012b7f5379d269ebd915fac2729" dependencies = [ "libc", "signal-hook-registry", @@ -2966,9 +2981,9 @@ dependencies = [ [[package]] name = "signal-hook-registry" -version = "1.2.2" +version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ce32ea0c6c56d5eacaeb814fbed9960547021d3edd010ded1425f180536b20ab" +checksum = "16f1d0fef1604ba8f7a073c7e701f213e056707210e9020af4528e0101ce11a6" dependencies = [ "libc", ] @@ -3127,9 +3142,9 @@ checksum = "1e81da0851ada1f3e9d4312c704aa4f8806f0f9d69faaf8df2f3464b4a9437c2" [[package]] name = "syn" -version = "1.0.55" +version = "1.0.56" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a571a711dddd09019ccc628e1b17fe87c59b09d513c06c026877aa708334f37a" +checksum = "a9802ddde94170d186eeee5005b798d9c159fa970403f1be19976d0cfb939b72" dependencies = [ "proc-macro2", "quote", @@ -3182,18 +3197,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.22" +version = "1.0.23" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0e9ae34b84616eedaaf1e9dd6026dbe00dcafa92aa0c8077cb69df1fcfe5e53e" +checksum = "76cc616c6abf8c8928e2fdcc0dbfab37175edd8fb49a4641066ad1364fdab146" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.22" +version = "1.0.23" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9ba20f23e85b10754cd195504aebf6a27e2e6cbe28c17778a0c930724628dd56" +checksum = "9be73a2caec27583d0046ef3796c3794f868a5bc813db689eed00c7631275cd1" dependencies = [ "proc-macro2", "quote", diff --git a/kanidm_book/src/pam_and_nsswitch.md b/kanidm_book/src/pam_and_nsswitch.md index a0fdd0f99..7273ab79d 100644 --- a/kanidm_book/src/pam_and_nsswitch.md +++ b/kanidm_book/src/pam_and_nsswitch.md @@ -8,7 +8,9 @@ can be used on the machine for various interactive tasks. Kanidm provide a unix daemon that runs on any client that wants to use pam and nsswitch integration. This is provided as the daemon can cache the accounts -for users who have unreliable networks or leave the site where kanidm is. +for users who have unreliable networks or leave the site where kanidm is. The +cache is also able to cache missing-entry responses to reduce network traffic +and main server load. Additionally, the daemon means that the pam and nsswitch integration libraries can be small, helping to reduce the attack surface of the machine. diff --git a/kanidm_client/src/lib.rs b/kanidm_client/src/lib.rs index dc566d549..60927e5c0 100644 --- a/kanidm_client/src/lib.rs +++ b/kanidm_client/src/lib.rs @@ -281,8 +281,10 @@ impl KanidmClientBuilder { let client = client_builder.build()?; // Now get the origin. + #[allow(clippy::expect_used)] let uri = Url::parse(&address).expect("can not fail"); + #[allow(clippy::expect_used)] let origin = uri .host_str() .map(|h| format!("{}://{}", uri.scheme(), h)) diff --git a/kanidm_unix_int/Cargo.toml b/kanidm_unix_int/Cargo.toml index 2fd34b6da..44c03c815 100644 --- a/kanidm_unix_int/Cargo.toml +++ b/kanidm_unix_int/Cargo.toml @@ -68,6 +68,8 @@ reqwest = { version = "0.10" } users = "0.10" async-std = "1.6" +lru = "0.6" + [features] # default = [ "libsqlite3-sys/bundled" ] diff --git a/kanidm_unix_int/src/cache.rs b/kanidm_unix_int/src/cache.rs index 9b571449e..c462ddae5 100644 --- a/kanidm_unix_int/src/cache.rs +++ b/kanidm_unix_int/src/cache.rs @@ -4,6 +4,7 @@ use crate::unix_proto::{NssGroup, NssUser}; use kanidm_client::asynchronous::KanidmAsyncClient; use kanidm_client::ClientError; use kanidm_proto::v1::{OperationError, UnixGroupToken, UnixUserToken}; +use lru::LruCache; use reqwest::StatusCode; use std::collections::BTreeSet; use std::ops::Add; @@ -11,6 +12,9 @@ use std::string::ToString; use std::time::{Duration, SystemTime}; use tokio::sync::{Mutex, RwLock}; +const NXCACHE_SIZE: usize = 2048; + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Id { Name(String), Gid(u32), @@ -35,6 +39,7 @@ pub struct CacheLayer { home_attr: HomeAttr, uid_attr_map: UidAttr, gid_attr_map: UidAttr, + nxcache: Mutex>, } impl ToString for Id { @@ -85,6 +90,7 @@ impl CacheLayer { home_attr, uid_attr_map, gid_attr_map, + nxcache: Mutex::new(LruCache::new(NXCACHE_SIZE)), }) } @@ -109,11 +115,15 @@ impl CacheLayer { } pub async fn clear_cache(&self) -> Result<(), ()> { + let mut nxcache_txn = self.nxcache.lock().await; + nxcache_txn.clear(); let dbtxn = self.db.write().await; dbtxn.clear_cache().and_then(|_| dbtxn.commit()) } pub async fn invalidate(&self) -> Result<(), ()> { + let mut nxcache_txn = self.nxcache.lock().await; + nxcache_txn.clear(); let dbtxn = self.db.write().await; dbtxn.invalidate().and_then(|_| dbtxn.commit()) } @@ -128,6 +138,17 @@ impl CacheLayer { dbtxn.get_groups() } + async fn set_nxcache(&self, id: &Id) { + let mut nxcache_txn = self.nxcache.lock().await; + let ex_time = SystemTime::now() + Duration::from_secs(self.timeout_seconds); + nxcache_txn.put(id.clone(), ex_time); + } + + pub async fn check_nxcache(&self, id: &Id) -> bool { + let nxcache_txn = self.nxcache.lock().await; + nxcache_txn.contains(id) + } + async fn get_cached_usertoken( &self, account_id: &Id, @@ -154,8 +175,30 @@ impl CacheLayer { Ok((false, Some(ut))) } } - None => Ok((true, None)), - } + None => { + // it wasn't in the DB - lets see if it's in the nxcache. + let mut nxcache_txn = self.nxcache.lock().await; + match nxcache_txn.get(account_id) { + Some(ex_time) => { + let now = SystemTime::now(); + if &now >= ex_time { + // It's in the LRU, but we are past the expiry so + // lets attempt a refresh. + Ok((true, None)) + } else { + // It's in the LRU and still valid, so return that + // no check is needed. + Ok((false, None)) + } + } + None => { + // Not in the LRU. Return that this IS expired + // and we have no data. + Ok((true, None)) + } + } + } + } // end match r } async fn get_cached_grouptoken( @@ -184,7 +227,29 @@ impl CacheLayer { Ok((false, Some(ut))) } } - None => Ok((true, None)), + None => { + // it wasn't in the DB - lets see if it's in the nxcache. + let mut nxcache_txn = self.nxcache.lock().await; + match nxcache_txn.get(grp_id) { + Some(ex_time) => { + let now = SystemTime::now(); + if &now >= ex_time { + // It's in the LRU, but we are past the expiry so + // lets attempt a refresh. + Ok((true, None)) + } else { + // It's in the LRU and still valid, so return that + // no check is needed. + Ok((false, None)) + } + } + None => { + // Not in the LRU. Return that this IS expired + // and we have no data. + Ok((true, None)) + } + } + } } } @@ -312,6 +377,9 @@ impl CacheLayer { if let Some(tok) = token { self.delete_cache_usertoken(&tok.uuid).await?; }; + // Cache the NX here. + self.set_nxcache(account_id).await; + Ok(None) } er => { @@ -385,6 +453,9 @@ impl CacheLayer { if let Some(tok) = token { self.delete_cache_grouptoken(&tok.uuid).await?; }; + // Cache the NX here. + self.set_nxcache(grp_id).await; + Ok(None) } er => { diff --git a/kanidm_unix_int/src/daemon.rs b/kanidm_unix_int/src/daemon.rs index b136b18e2..61dd68718 100644 --- a/kanidm_unix_int/src/daemon.rs +++ b/kanidm_unix_int/src/daemon.rs @@ -408,6 +408,8 @@ async fn main() { // Undo it. let _ = unsafe { umask(before) }; + // TODO: Setup a task that handles pre-fetching here. + let server = async move { let mut incoming = listener.incoming(); while let Some(socket_res) = incoming.next().await { diff --git a/kanidm_unix_int/tests/cache_layer_test.rs b/kanidm_unix_int/tests/cache_layer_test.rs index f4e5203ca..1bbbe85ca 100644 --- a/kanidm_unix_int/tests/cache_layer_test.rs +++ b/kanidm_unix_int/tests/cache_layer_test.rs @@ -7,7 +7,7 @@ use kanidm::audit::LogLevel; use kanidm::config::{Configuration, IntegrationTestConfig}; use kanidm::core::create_server_core; -use kanidm_unix_common::cache::CacheLayer; +use kanidm_unix_common::cache::{CacheLayer, Id}; use kanidm_unix_common::constants::{ DEFAULT_GID_ATTR_MAP, DEFAULT_HOME_ATTR, DEFAULT_HOME_PREFIX, DEFAULT_SHELL, DEFAULT_UID_ATTR_MAP, @@ -655,3 +655,83 @@ fn test_cache_account_expiry() { rt.block_on(fut); }) } + +#[test] +fn test_cache_nxcache() { + run_test(test_fixture, |cachelayer, mut _adminclient| { + let mut rt = Runtime::new().expect("Failed to start tokio"); + let fut = async move { + cachelayer.attempt_online().await; + assert!(cachelayer.test_connection().await); + // Is it in the nxcache? + + assert!( + !cachelayer + .check_nxcache(&Id::Name("root".to_string())) + .await + ); + assert!(!cachelayer.check_nxcache(&Id::Gid(0)).await); + assert!( + !cachelayer + .check_nxcache(&Id::Name("root_group".to_string())) + .await + ); + assert!(!cachelayer.check_nxcache(&Id::Gid(1)).await); + + // Look for the acc id + nss id + let ut = cachelayer + .get_nssaccount_name("root") + .await + .expect("Failed to get from cache"); + assert!(ut.is_none()); + let ut = cachelayer + .get_nssaccount_gid(0) + .await + .expect("Failed to get from cache"); + assert!(ut.is_none()); + + let gt = cachelayer + .get_nssgroup_name("root_group") + .await + .expect("Failed to get from cache"); + assert!(gt.is_none()); + let gt = cachelayer + .get_nssgroup_gid(1) + .await + .expect("Failed to get from cache"); + assert!(gt.is_none()); + + // Should all now be nxed + assert!( + cachelayer + .check_nxcache(&Id::Name("root".to_string())) + .await + ); + assert!(cachelayer.check_nxcache(&Id::Gid(0)).await); + assert!( + cachelayer + .check_nxcache(&Id::Name("root_group".to_string())) + .await + ); + assert!(cachelayer.check_nxcache(&Id::Gid(1)).await); + + // invalidate cache + assert!(cachelayer.invalidate().await.is_ok()); + + // Both should NOT be in nxcache now. + assert!( + !cachelayer + .check_nxcache(&Id::Name("root".to_string())) + .await + ); + assert!(!cachelayer.check_nxcache(&Id::Gid(0)).await); + assert!( + !cachelayer + .check_nxcache(&Id::Name("root_group".to_string())) + .await + ); + assert!(!cachelayer.check_nxcache(&Id::Gid(1)).await); + }; + rt.block_on(fut); + }) +} diff --git a/kanidmd/src/lib/credential/mod.rs b/kanidmd/src/lib/credential/mod.rs index 3e2e21d85..e1ce6a7aa 100644 --- a/kanidmd/src/lib/credential/mod.rs +++ b/kanidmd/src/lib/credential/mod.rs @@ -318,7 +318,7 @@ impl Credential { policy: &CryptoPolicy, cleartext: &str, ) -> Result { - Password::new(policy, cleartext).map(|pw| Self::new_from_password(pw)) + Password::new(policy, cleartext).map(Self::new_from_password) } pub fn new_webauthn_only(label: String, cred: WebauthnCredential) -> Self { @@ -352,7 +352,7 @@ impl Credential { } CredentialType::PasswordMFA(pw, totp, map) => { let mut nmap = map.clone(); - if let Some(_) = nmap.insert(label.clone(), cred) { + if nmap.insert(label.clone(), cred).is_some() { return Err(OperationError::InvalidAttribute(format!( "Webauthn label '{:?}' already exists", label @@ -362,7 +362,7 @@ impl Credential { } CredentialType::Webauthn(map) => { let mut nmap = map.clone(); - if let Some(_) = nmap.insert(label.clone(), cred) { + if nmap.insert(label.clone(), cred).is_some() { return Err(OperationError::InvalidAttribute(format!( "Webauthn label '{:?}' already exists", label @@ -380,6 +380,7 @@ impl Credential { }) } + #[allow(clippy::ptr_arg)] pub fn update_webauthn_counter( &self, cid: &CredentialID, @@ -402,9 +403,11 @@ impl Credential { .map(|label| { let mut webauthn_map = map.clone(); + if let Some(cred) = webauthn_map - .get_mut(label) - .map(|cred| cred.counter = counter); + .get_mut(label) { + cred.counter = counter + }; webauthn_map }), }; @@ -420,7 +423,7 @@ impl Credential { let type_ = match &self.type_ { CredentialType::Password(_pw) | CredentialType::GeneratedPassword(_pw) => { // Should not be possible! - unreachable!(); + return Err(OperationError::InvalidState); } CredentialType::Webauthn(_) => CredentialType::Webauthn(map), CredentialType::PasswordMFA(pw, totp, _) => { @@ -576,7 +579,7 @@ impl Credential { CredentialType::PasswordMFA(_pw, totp, wan) => { if let Some(r_totp) = totp { Some(CredSoftLockPolicy::TOTP(r_totp.step)) - } else if wan.len() > 0 { + } else if !wan.is_empty() { Some(CredSoftLockPolicy::Webauthn) } else { None diff --git a/kanidmd/src/lib/idm/account.rs b/kanidmd/src/lib/idm/account.rs index c82bfecad..a1c18c327 100644 --- a/kanidmd/src/lib/idm/account.rs +++ b/kanidmd/src/lib/idm/account.rs @@ -259,6 +259,7 @@ impl Account { Ok(ModifyList::new_purge_and_set("primary_credential", vcred)) } + #[allow(clippy::ptr_arg)] pub(crate) fn gen_webauthn_counter_mod( &self, cid: &CredentialID, diff --git a/kanidmd/src/lib/idm/authsession.rs b/kanidmd/src/lib/idm/authsession.rs index ed878708d..bcd893d54 100644 --- a/kanidmd/src/lib/idm/authsession.rs +++ b/kanidmd/src/lib/idm/authsession.rs @@ -89,7 +89,7 @@ impl CredHandler { } CredentialType::PasswordMFA(_, None, _) => Err(()), CredentialType::Webauthn(wan) => webauthn - .generate_challenge_authenticate(wan.values().map(|c| c.clone()).collect()) + .generate_challenge_authenticate(wan.values().cloned().collect()) .map(|(chal, wan_state)| { CredHandler::Webauthn(CredWebauthn { chal, @@ -103,7 +103,7 @@ impl CredHandler { "Unable to create webauthn authentication challenge -> {:?}", e ); - () + // maps to unit. }), } } @@ -385,7 +385,7 @@ impl AuthSession { pub fn new( au: &mut AuditScope, account: Account, - _appid: Option, + _appid: &Option, webauthn: &Webauthn, ct: Duration, ) -> (Option, AuthState) { @@ -632,7 +632,7 @@ mod tests { let (session, state) = AuthSession::new( &mut audit, anon_account, - None, + &None, &webauthn, duration_from_epoch_now(), ); @@ -679,7 +679,7 @@ mod tests { let (session, state) = AuthSession::new( &mut audit, anon_account, - Some("NonExistantAppID".to_string()), + &Some("NonExistantAppID".to_string()), &webauthn, duration_from_epoch_now(), ); @@ -703,7 +703,7 @@ mod tests { let (session, state) = AuthSession::new( $audit, $account.clone(), - None, + &None, $webauthn, duration_from_epoch_now(), ); @@ -800,7 +800,7 @@ mod tests { let (session, state) = AuthSession::new( $audit, $account.clone(), - None, + &None, $webauthn, duration_from_epoch_now(), ); @@ -992,7 +992,7 @@ mod tests { let (session, state) = AuthSession::new( $audit, $account.clone(), - None, + &None, $webauthn, duration_from_epoch_now(), ); diff --git a/kanidmd/src/lib/idm/mfareg.rs b/kanidmd/src/lib/idm/mfareg.rs index 86e174b37..693cc4673 100644 --- a/kanidmd/src/lib/idm/mfareg.rs +++ b/kanidmd/src/lib/idm/mfareg.rs @@ -26,6 +26,7 @@ pub(crate) enum MfaRegNext { } impl MfaRegNext { + #[allow(clippy::wrong_self_convention)] pub fn to_proto(self, u: Uuid) -> SetCredentialResponse { match self { MfaRegNext::Success => SetCredentialResponse::Success, @@ -64,13 +65,18 @@ impl MfaRegSession { ) -> Result<(Self, MfaRegNext), OperationError> { // Based on the req, init our session, and the return the next step. // Store the ID of the event that start's the attempt - let state = MfaRegState::TOTPInit(TOTP::generate_secure(label, TOTP_DEFAULT_STEP)); + let token = TOTP::generate_secure(label, TOTP_DEFAULT_STEP); + + let accountname = account.name.as_str(); + let issuer = account.spn.as_str(); + let next = MfaRegNext::TOTPCheck(token.to_proto(accountname, issuer)); + + let state = MfaRegState::TOTPInit(token); let s = MfaRegSession { origin, account, state, }; - let next = s.next(); Ok((s, next)) } @@ -165,20 +171,3 @@ impl MfaRegSession { } } } - -impl MfaRegSession { - pub fn next(&self) -> MfaRegNext { - // Given our current state, what is the next step we need to process or offer? - match &self.state { - MfaRegState::TOTPDone | MfaRegState::WebauthnDone => MfaRegNext::Success, - MfaRegState::TOTPInit(token) => { - let accountname = self.account.name.as_str(); - let issuer = self.account.spn.as_str(); - MfaRegNext::TOTPCheck(token.to_proto(accountname, issuer)) - } - MfaRegState::WebauthnInit(_label, _registration_state) => { - unreachable!(); - } - } - } -} diff --git a/kanidmd/src/lib/idm/server.rs b/kanidmd/src/lib/idm/server.rs index 275b617d3..256c3ac76 100644 --- a/kanidmd/src/lib/idm/server.rs +++ b/kanidmd/src/lib/idm/server.rs @@ -374,7 +374,7 @@ impl<'a> IdmServerWriteTransaction<'a> { }; let (auth_session, state) = if is_valid { - AuthSession::new(au, account, init.appid.clone(), self.webauthn, ct) + AuthSession::new(au, account, &init.appid, self.webauthn, ct) } else { // it's softlocked, don't even bother. lsecurity!(au, "Account is softlocked.");