diff --git a/server/core/src/lib.rs b/server/core/src/lib.rs index 70b69e8e1..63b5a938f 100644 --- a/server/core/src/lib.rs +++ b/server/core/src/lib.rs @@ -47,6 +47,7 @@ use kanidmd_lib::idm::ldap::LdapServer; use kanidmd_lib::prelude::*; use kanidmd_lib::schema::Schema; use kanidmd_lib::status::StatusActor; +use kanidmd_lib::value::CredentialType; #[cfg(not(target_family = "windows"))] use libc::umask; @@ -876,6 +877,7 @@ pub async fn create_server_core( return Err(()); } }; + // Add admin to idm_admins to allow tests more flexibility wrt to permissions. // This way our default access controls can be stricter to prevent lateral // movement. @@ -891,7 +893,25 @@ pub async fn create_server_core( ); return Err(()); } - } + }; + + match idms_prox_write.qs_write.internal_modify_uuid( + UUID_IDM_ALL_PERSONS, + &ModifyList::new_purge_and_set( + Attribute::CredentialTypeMinimum, + CredentialType::Any.into(), + ), + ) { + Ok(_) => {} + Err(e) => { + error!( + "Unable to configure INTEGRATION TEST default credential policy -> {:?}", + e + ); + return Err(()); + } + }; + match idms_prox_write.commit() { Ok(_) => {} Err(e) => { diff --git a/server/lib/src/constants/mod.rs b/server/lib/src/constants/mod.rs index a2f9ca338..e140b8a71 100644 --- a/server/lib/src/constants/mod.rs +++ b/server/lib/src/constants/mod.rs @@ -45,12 +45,13 @@ pub type DomainVersion = u32; pub const DOMAIN_LEVEL_1: DomainVersion = 1; pub const DOMAIN_LEVEL_2: DomainVersion = 2; +pub const DOMAIN_LEVEL_3: DomainVersion = 3; // The minimum supported domain functional level pub const DOMAIN_MIN_LEVEL: DomainVersion = DOMAIN_LEVEL_2; // The target supported domain functional level -pub const DOMAIN_TGT_LEVEL: DomainVersion = DOMAIN_LEVEL_2; +pub const DOMAIN_TGT_LEVEL: DomainVersion = DOMAIN_LEVEL_3; // The maximum supported domain functional level -pub const DOMAIN_MAX_LEVEL: DomainVersion = DOMAIN_LEVEL_2; +pub const DOMAIN_MAX_LEVEL: DomainVersion = DOMAIN_LEVEL_3; // On test builds, define to 60 seconds #[cfg(test)] diff --git a/server/lib/src/filter.rs b/server/lib/src/filter.rs index b04a1e217..ac32dcc1e 100644 --- a/server/lib/src/filter.rs +++ b/server/lib/src/filter.rs @@ -660,7 +660,7 @@ impl Filter { // This has to have two versions to account for ro/rw traits, because RS can't // monomorphise on the trait to call clone_value. An option is to make a fn that // takes "clone_value(t, a, v) instead, but that may have a similar issue. - #[instrument(name = "filter::from_ro", level = "debug", skip_all)] + #[instrument(name = "filter::from_ro", level = "trace", skip_all)] pub fn from_ro( ev: &Identity, f: &ProtoFilter, @@ -675,7 +675,7 @@ impl Filter { }) } - #[instrument(name = "filter::from_rw", level = "debug", skip_all)] + #[instrument(name = "filter::from_rw", level = "trace", skip_all)] pub fn from_rw( ev: &Identity, f: &ProtoFilter, @@ -690,7 +690,7 @@ impl Filter { }) } - #[instrument(name = "filter::from_ldap_ro", level = "debug", skip_all)] + #[instrument(name = "filter::from_ldap_ro", level = "trace", skip_all)] pub fn from_ldap_ro( ev: &Identity, f: &LdapFilter, diff --git a/server/lib/src/idm/credupdatesession.rs b/server/lib/src/idm/credupdatesession.rs index 2254c0b92..8242a0717 100644 --- a/server/lib/src/idm/credupdatesession.rs +++ b/server/lib/src/idm/credupdatesession.rs @@ -2448,6 +2448,13 @@ mod tests { ) -> (CredentialUpdateSessionToken, CredentialUpdateSessionStatus) { let mut idms_prox_write = idms.proxy_write(ct).await; + // Remove the default all persons policy, it interfers with our test. + let modlist = ModifyList::new_purge(Attribute::CredentialTypeMinimum); + idms_prox_write + .qs_write + .internal_modify_uuid(UUID_IDM_ALL_PERSONS, &modlist) + .expect("Unable to change default session exp"); + let e2 = entry_init!( (Attribute::Class, EntryClass::Object.to_value()), (Attribute::Class, EntryClass::Account.to_value()), diff --git a/server/lib/src/idm/reauth.rs b/server/lib/src/idm/reauth.rs index 4a7a3efdf..c730abb35 100644 --- a/server/lib/src/idm/reauth.rs +++ b/server/lib/src/idm/reauth.rs @@ -279,12 +279,10 @@ mod tests { let pw = crate::utils::password_from_random(); - let c_status = cutxn + let _c_status = cutxn .credential_primary_set_password(&cust, ct, &pw) .expect("Failed to update the primary cred password"); - assert!(c_status.can_commit()); - let c_status = cutxn .credential_primary_init_totp(&cust, ct) .expect("Failed to update the primary cred password"); diff --git a/server/lib/src/plugins/domain.rs b/server/lib/src/plugins/domain.rs index 1fff3c146..5dd480c99 100644 --- a/server/lib/src/plugins/domain.rs +++ b/server/lib/src/plugins/domain.rs @@ -106,8 +106,10 @@ impl Domain { if !e.attribute_pres(Attribute::Version) { let n = Value::Uint32(DOMAIN_MIN_LEVEL); e.set_ava(Attribute::Version, once(n)); - trace!("plugin_domain: Applying domain version transform"); - } + warn!("plugin_domain: Applying domain version transform"); + } else { + warn!("plugin_domain: NOT Applying domain version transform"); + }; // create the domain_display_name if it's missing if !e.attribute_pres(Attribute::DomainDisplayName) { diff --git a/server/lib/src/repl/consumer.rs b/server/lib/src/repl/consumer.rs index 1f4d8d0de..e1b04aabe 100644 --- a/server/lib/src/repl/consumer.rs +++ b/server/lib/src/repl/consumer.rs @@ -351,8 +351,7 @@ impl<'a> QueryServerWriteTransaction<'a> { e })?; - // This is re-loaded in case the domain name changed on the remote. Also needed for changing - // the domain version. + // This is re-loaded in case the domain name changed on the remote self.reload_domain_info().map_err(|e| { error!("Failed to reload domain info"); e @@ -371,6 +370,17 @@ impl<'a> QueryServerWriteTransaction<'a> { e })?; + // Reload the domain version, doing any needed migrations. + // + // While it seems odd that we do the migrations after we recieve the entries, + // this is because the supplier will already be sending us everything that + // was just migrated. As a result, we only need to apply the migrations to entries + // that were not on the supplier, and therefore need updates here. + self.reload_domain_info_version().map_err(|e| { + error!("Failed to reload domain info version"); + e + })?; + // Finally, confirm that the ranges that we have added match the ranges from our // context. Note that we get this in a writeable form! let ruv = self.be_txn.get_ruv_write(); diff --git a/server/lib/src/server/access/mod.rs b/server/lib/src/server/access/mod.rs index f94a38476..0d3127b82 100644 --- a/server/lib/src/server/access/mod.rs +++ b/server/lib/src/server/access/mod.rs @@ -164,7 +164,7 @@ pub trait AccessControlsTransaction<'a> { &self, ) -> &mut ARCacheReadTxn<'a, (IdentityId, Filter), Filter, ()>; - #[instrument(level = "debug", name = "access::search_related_acp", skip_all)] + #[instrument(level = "trace", name = "access::search_related_acp", skip_all)] fn search_related_acp<'b>(&'b self, ident: &Identity) -> Vec> { let search_state = self.get_search(); let acp_resolve_filter_cache = self.get_acp_resolve_filter_cache(); @@ -369,7 +369,7 @@ pub trait AccessControlsTransaction<'a> { Ok(allowed_entries) } - #[instrument(level = "debug", name = "access::modify_related_acp", skip_all)] + #[instrument(level = "trace", name = "access::modify_related_acp", skip_all)] fn modify_related_acp<'b>(&'b self, ident: &Identity) -> Vec> { // Some useful references we'll use for the remainder of the operation let modify_state = self.get_modify(); @@ -706,7 +706,7 @@ pub trait AccessControlsTransaction<'a> { Ok(r) } - #[instrument(level = "debug", name = "access::delete_related_acp", skip_all)] + #[instrument(level = "trace", name = "access::delete_related_acp", skip_all)] fn delete_related_acp<'b>(&'b self, ident: &Identity) -> Vec> { // Some useful references we'll use for the remainder of the operation let delete_state = self.get_delete(); diff --git a/server/lib/src/server/migrations.rs b/server/lib/src/server/migrations.rs index ba356c370..b5d61f053 100644 --- a/server/lib/src/server/migrations.rs +++ b/server/lib/src/server/migrations.rs @@ -1,3 +1,4 @@ +use crate::value::CredentialType; use kanidm_proto::v1::SchemaError; use std::time::Duration; @@ -121,16 +122,42 @@ impl QueryServer { } } - // This is the start of domain info related migrations which we will need in future - // to handle replication. Due to the access control rework, and the addition of "managed by" - // syntax, we need to ensure both node "fence" replication from each other. We do this - // by changing domain infos to be incompatible during this phase. - let domain_info_version = match write_txn.internal_search_uuid(UUID_DOMAIN_INFO) { + // Reload if anything in migrations requires it. + write_txn.reload()?; + + // This is what tells us if the domain entry existed before or not. + let db_domain_version = match write_txn.internal_search_uuid(UUID_DOMAIN_INFO) { Ok(e) => Ok(e.get_ava_single_uint32(Attribute::Version).unwrap_or(0)), Err(OperationError::NoMatchingEntries) => Ok(0), Err(r) => Err(r), }?; - admin_debug!(?domain_info_version); + + info!(?db_domain_version, "Before setting internal domain info"); + + // Migrations complete. Init idm will now set the system config version and minimum domain + // level if none was present + write_txn.initialise_domain_info()?; + + // No domain info was present, so neither was the rest of the IDM. + if db_domain_version == 0 { + write_txn.initialise_idm()?; + } + + // Now force everything to reload, init idm can touch a lot. + write_txn.force_all_reload(); + write_txn.reload()?; + + // Domain info is now ready and reloaded, we can proceed. + write_txn.set_phase(ServerPhase::DomainInfoReady); + + // This is the start of domain info related migrations which we will need in future + // to handle replication. Due to the access control rework, and the addition of "managed by" + // syntax, we need to ensure both node "fence" replication from each other. We do this + // by changing domain infos to be incompatible during this phase. + + // The reloads will have populated this structure now. + let domain_info_version = write_txn.get_domain_version(); + info!(?db_domain_version, "After setting internal domain info"); if domain_info_version < DOMAIN_TGT_LEVEL { write_txn @@ -146,13 +173,9 @@ impl QueryServer { })?; } - // Reload if anything in migrations requires it. + // Reload if anything in migrations requires it - this triggers the domain migrations. write_txn.reload()?; - // Migrations complete. Init idm will now set the version as needed. - write_txn.initialise_idm()?; - // Now force everything to reload. - write_txn.force_all_reload(); // We are ready to run write_txn.set_phase(ServerPhase::Running); @@ -643,6 +666,36 @@ impl<'a> QueryServerWriteTransaction<'a> { Ok(()) } + #[instrument(level = "info", skip_all)] + /// This migration will + /// * Trigger a "once off" mfa account policy rule on all persons. + pub fn migrate_domain_2_to_3(&mut self) -> Result<(), OperationError> { + let idm_all_persons = match self.internal_search_uuid(UUID_IDM_ALL_PERSONS) { + Ok(entry) => entry, + Err(OperationError::NoMatchingEntries) => return Ok(()), + Err(e) => return Err(e), + }; + + let credential_policy = + idm_all_persons.get_ava_single_credential_type(Attribute::CredentialTypeMinimum); + + if credential_policy.is_some() { + debug!("Credential policy already present, not applying change."); + return Ok(()); + } + + self.internal_modify_uuid( + UUID_IDM_ALL_PERSONS, + &ModifyList::new_purge_and_set( + Attribute::CredentialTypeMinimum, + CredentialType::Mfa.into(), + ), + ) + .map(|()| { + info!("Upgraded default account policy to enforce MFA"); + }) + } + #[instrument(level = "info", skip_all)] pub fn initialise_schema_core(&mut self) -> Result<(), OperationError> { admin_debug!("initialise_schema_core -> start ..."); @@ -790,7 +843,7 @@ impl<'a> QueryServerWriteTransaction<'a> { #[instrument(level = "info", skip_all)] /// This function is idempotent, runs all the startup functionality and checks - pub fn initialise_idm(&mut self) -> Result<(), OperationError> { + pub fn initialise_domain_info(&mut self) -> Result<(), OperationError> { // First, check the system_info object. This stores some server information // and details. It's a pretty const thing. Also check anonymous, important to many // concepts. @@ -802,8 +855,12 @@ impl<'a> QueryServerWriteTransaction<'a> { admin_error!("initialise_idm p1 -> result {:?}", res); } debug_assert!(res.is_ok()); - res?; + res + } + #[instrument(level = "info", skip_all)] + /// This function is idempotent, runs all the startup functionality and checks + pub fn initialise_idm(&mut self) -> Result<(), OperationError> { // The domain info now exists, we should be able to do these migrations as they will // cause SPN regenerations to occur diff --git a/server/lib/src/server/mod.rs b/server/lib/src/server/mod.rs index ae24da268..fbd3aa0b9 100644 --- a/server/lib/src/server/mod.rs +++ b/server/lib/src/server/mod.rs @@ -56,6 +56,7 @@ pub type ResolveFilterCacheReadTxn<'a> = enum ServerPhase { Bootstrap, SchemaReady, + DomainInfoReady, Running, } @@ -165,6 +166,8 @@ pub trait QueryServerTransaction<'a> { fn denied_names(&self) -> &HashSet; + fn get_domain_version(&self) -> DomainVersion; + fn get_domain_uuid(&self) -> Uuid; fn get_domain_name(&self) -> &str; @@ -989,6 +992,10 @@ impl<'a> QueryServerTransaction<'a> for QueryServerReadTransaction<'a> { &self.system_config.denied_names } + fn get_domain_version(&self) -> DomainVersion { + self.d_info.d_vers + } + fn get_domain_uuid(&self) -> Uuid { self.d_info.d_uuid } @@ -1110,6 +1117,10 @@ impl<'a> QueryServerTransaction<'a> for QueryServerWriteTransaction<'a> { &self.system_config.denied_names } + fn get_domain_version(&self) -> DomainVersion { + self.d_info.d_vers + } + fn get_domain_uuid(&self) -> Uuid { self.d_info.d_uuid } @@ -1580,6 +1591,40 @@ impl<'a> QueryServerWriteTransaction<'a> { Ok(()) } + /// Pulls the domain name from the database and updates the DomainInfo data in memory + #[instrument(level = "debug", skip_all)] + pub(crate) fn reload_domain_info_version(&mut self) -> Result<(), OperationError> { + let domain_info_version = self + .internal_search_uuid(UUID_DOMAIN_INFO) + .and_then(|e| { + e.get_ava_single_uint32(Attribute::Version) + .ok_or(OperationError::InvalidEntryState) + }) + .map_err(|err| { + error!(?err, "Error getting domain version"); + err + })?; + + // We have to set the domain version here so that features which check for it + // will now see it's been increased. + let mut_d_info = self.d_info.get_mut(); + let previous_version = mut_d_info.d_vers; + mut_d_info.d_vers = domain_info_version; + + if previous_version == domain_info_version || *self.phase < ServerPhase::DomainInfoReady { + return Ok(()); + } + + debug!(domain_previous_version = ?previous_version, domain_target_version = ?domain_info_version); + + if previous_version <= DOMAIN_LEVEL_2 && domain_info_version >= DOMAIN_LEVEL_3 { + // 2 -> 3 Migration + self.migrate_domain_2_to_3()?; + } + + Ok(()) + } + /// Pulls the domain name from the database and updates the DomainInfo data in memory #[instrument(level = "debug", skip_all)] pub(crate) fn reload_domain_info(&mut self) -> Result<(), OperationError> { @@ -1689,6 +1734,11 @@ impl<'a> QueryServerWriteTransaction<'a> { } pub(crate) fn reload(&mut self) -> Result<(), OperationError> { + // First, check if the domain version has changed. + if self.changed_domain { + self.reload_domain_info_version()?; + } + // This could be faster if we cache the set of classes changed // in an operation so we can check if we need to do the reload or not // diff --git a/server/testkit/tests/identity_verification_tests.rs b/server/testkit/tests/identity_verification_tests.rs index 577461fe7..b7bb8eb2a 100644 --- a/server/testkit/tests/identity_verification_tests.rs +++ b/server/testkit/tests/identity_verification_tests.rs @@ -1,9 +1,6 @@ use core::result::Result::Err; use kanidm_client::KanidmClient; -use kanidm_proto::{ - internal::{IdentifyUserRequest, IdentifyUserResponse}, - v1::Entry, -}; +use kanidm_proto::internal::{IdentifyUserRequest, IdentifyUserResponse}; use kanidmd_lib::prelude::Attribute; use kanidmd_testkit::ADMIN_TEST_PASSWORD; @@ -266,24 +263,15 @@ async fn setup_server(rsclient: &KanidmClient) { let res = rsclient .auth_simple_password("admin", ADMIN_TEST_PASSWORD) .await; + assert!(res.is_ok()); } async fn create_user(rsclient: &KanidmClient, user: &str) -> String { - let e: Entry = serde_json::from_str(&format!( - r#"{{ - "attrs": {{ - "class": ["account", "person", "object"], - "name": ["{}"], - "displayname": ["dx{}"] - }} - }}"#, - user, user - )) - .unwrap(); - let res = rsclient.create(vec![e.clone()]).await; - - assert!(res.is_ok()); + rsclient + .idm_person_account_create(user, &format!("dx{}", user)) + .await + .expect("Unable to create person"); rsclient .idm_person_account_primary_credential_set_password(user, UNIVERSAL_PW)