Add improved domain migration framework and default MFA (#2382)

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) => {

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)]

server/lib/src/filter.rs

@@ -660,7 +660,7 @@ impl Filter<FilterInvalid> {
     // 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<FilterInvalid> {
         })
     }
 
-    #[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<FilterInvalid> {
         })
     }
 
-    #[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,

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()),

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");

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) {

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();

server/lib/src/server/access/mod.rs

@@ -164,7 +164,7 @@ pub trait AccessControlsTransaction<'a> {
         &self,
     ) -> &mut ARCacheReadTxn<'a, (IdentityId, Filter<FilterValid>), Filter<FilterValidResolved>, ()>;
 
-    #[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<AccessControlSearchResolved<'b>> {
         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<AccessControlModifyResolved<'b>> {
         // 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<AccessControlDeleteResolved<'b>> {
         // Some useful references we'll use for the remainder of the operation
         let delete_state = self.get_delete();

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
 

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<String>;
 
+    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
         //

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)