From ed88b72080458dfe7b6dfddbfd46d20d8e556e45 Mon Sep 17 00:00:00 2001 From: Firstyear Date: Sun, 16 Feb 2025 08:45:25 +1000 Subject: [PATCH] Exempt idm_admin and admin from denied names. (#3429) idm_admin and admin should be exempted from the denied names process, as these values will already be denied due to attribute uniqueness. Additionally improved the denied names check to only validate the name during a change, not during a modifification. This way entries that become denied can get themself out of the pickle. --- server/lib/src/constants/schema.rs | 10 ++ server/lib/src/plugins/valuedeny.rs | 166 +++++++++++++++++++++++----- server/lib/src/server/migrations.rs | 5 +- 3 files changed, 154 insertions(+), 27 deletions(-) diff --git a/server/lib/src/constants/schema.rs b/server/lib/src/constants/schema.rs index 552f76d99..d41680288 100644 --- a/server/lib/src/constants/schema.rs +++ b/server/lib/src/constants/schema.rs @@ -208,6 +208,16 @@ pub static ref SCHEMA_ATTR_DENIED_NAME: SchemaAttribute = SchemaAttribute { ..Default::default() }; +pub static ref SCHEMA_ATTR_DENIED_NAME_DL10: SchemaAttribute = SchemaAttribute { + uuid: UUID_SCHEMA_ATTR_DENIED_NAME, + name: Attribute::DeniedName, + description: "Iname values that are not allowed to be used in 'name'.".to_string(), + + syntax: SyntaxType::Utf8StringIname, + multivalue: true, + ..Default::default() +}; + pub static ref SCHEMA_ATTR_DOMAIN_TOKEN_KEY: SchemaAttribute = SchemaAttribute { uuid: UUID_SCHEMA_ATTR_DOMAIN_TOKEN_KEY, name: Attribute::DomainTokenKey, diff --git a/server/lib/src/plugins/valuedeny.rs b/server/lib/src/plugins/valuedeny.rs index e0a360ee0..6c96bce12 100644 --- a/server/lib/src/plugins/valuedeny.rs +++ b/server/lib/src/plugins/valuedeny.rs @@ -10,7 +10,7 @@ impl Plugin for ValueDeny { "plugin_value_deny" } - #[instrument(level = "debug", name = "base_pre_create_transform", skip_all)] + #[instrument(level = "debug", name = "denied_names_pre_create_transform", skip_all)] #[allow(clippy::cognitive_complexity)] fn pre_create_transform( qs: &mut QueryServerWriteTransaction, @@ -19,9 +19,25 @@ impl Plugin for ValueDeny { ) -> Result<(), OperationError> { let denied_names = qs.denied_names(); + if denied_names.is_empty() { + // Nothing to check. + return Ok(()); + } + let mut pass = true; for entry in cand { + // If the entry doesn't have a uuid, it's invalid anyway and will fail schema. + if let Some(e_uuid) = entry.get_uuid() { + // SAFETY - Thanks to JpWarren blowing his nipper clean off, we need to + // assert that the break glass and system accounts are NOT subject to + // this process. + if e_uuid < DYNAMIC_RANGE_MINIMUM_UUID { + // These entries are exempt + continue; + } + } + if let Some(name) = entry.get_ava_single_iname(Attribute::Name) { if denied_names.contains(name) { pass = false; @@ -37,27 +53,24 @@ impl Plugin for ValueDeny { } } - #[instrument(level = "debug", name = "base_pre_modify", skip_all)] fn pre_modify( qs: &mut QueryServerWriteTransaction, - _pre_cand: &[Arc], + pre_cand: &[Arc], cand: &mut Vec>, _me: &ModifyEvent, ) -> Result<(), OperationError> { - Self::modify(qs, cand) + Self::modify(qs, pre_cand, cand) } - #[instrument(level = "debug", name = "base_pre_modify", skip_all)] fn pre_batch_modify( qs: &mut QueryServerWriteTransaction, - _pre_cand: &[Arc], + pre_cand: &[Arc], cand: &mut Vec>, _me: &BatchModifyEvent, ) -> Result<(), OperationError> { - Self::modify(qs, cand) + Self::modify(qs, pre_cand, cand) } - #[instrument(level = "debug", name = "base::verify", skip_all)] fn verify(qs: &mut QueryServerReadTransaction) -> Vec> { let denied_names = qs.denied_names().clone(); @@ -68,7 +81,15 @@ impl Plugin for ValueDeny { match qs.internal_search(filt) { Ok(entries) => { for entry in entries { - results.push(Err(ConsistencyError::DeniedName(entry.get_uuid()))); + let e_uuid = entry.get_uuid(); + // SAFETY - Thanks to JpWarren blowing his nipper clean off, we need to + // assert that the break glass accounts are NOT subject to this process. + if e_uuid < DYNAMIC_RANGE_MINIMUM_UUID { + // These entries are exempt + continue; + } + + results.push(Err(ConsistencyError::DeniedName(e_uuid))); } } Err(err) => { @@ -83,17 +104,37 @@ impl Plugin for ValueDeny { } impl ValueDeny { + #[instrument(level = "debug", name = "denied_names_modify", skip_all)] fn modify( qs: &mut QueryServerWriteTransaction, - cand: &mut Vec>, + pre_cand: &[Arc], + cand: &mut [EntryInvalidCommitted], ) -> Result<(), OperationError> { let denied_names = qs.denied_names(); + if denied_names.is_empty() { + // Nothing to check. + return Ok(()); + } + let mut pass = true; - for entry in cand { - if let Some(name) = entry.get_ava_single_iname(Attribute::Name) { - if denied_names.contains(name) { + for (pre_entry, post_entry) in pre_cand.iter().zip(cand.iter()) { + // If the entry doesn't have a uuid, it's invalid anyway and will fail schema. + let e_uuid = pre_entry.get_uuid(); + // SAFETY - Thanks to JpWarren blowing his nipper clean off, we need to + // assert that the break glass accounts are NOT subject to this process. + if e_uuid < DYNAMIC_RANGE_MINIMUM_UUID { + // These entries are exempt + continue; + } + + let pre_name = pre_entry.get_ava_single_iname(Attribute::Name); + let post_name = post_entry.get_ava_single_iname(Attribute::Name); + + if let Some(name) = post_name { + // Only if the name is changing, and is denied. + if pre_name != post_name && denied_names.contains(name) { pass = false; error!(?name, "name denied by system configuration"); } @@ -117,10 +158,10 @@ mod tests { let me_inv_m = ModifyEvent::new_internal_invalid( filter!(f_eq(Attribute::Uuid, PVUUID_SYSTEM_CONFIG.clone())), - ModifyList::new_list(vec![Modify::Present( - Attribute::DeniedName, - Value::new_iname("tobias"), - )]), + ModifyList::new_list(vec![ + Modify::Present(Attribute::DeniedName, Value::new_iname("tobias")), + Modify::Present(Attribute::DeniedName, Value::new_iname("ellie")), + ]), ); assert!(server_txn.modify(&me_inv_m).is_ok()); @@ -148,30 +189,103 @@ mod tests { #[qs_test] async fn test_valuedeny_modify(server: &QueryServer) { - setup_name_deny(server).await; - + // Create an entry that has a name which will become denied to test how it + // interacts. let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap(); - let t_uuid = Uuid::new_v4(); + let e_uuid = Uuid::new_v4(); assert!(server_txn .internal_create(vec![entry_init!( (Attribute::Class, EntryClass::Object.to_value()), (Attribute::Class, EntryClass::Account.to_value()), (Attribute::Class, EntryClass::Person.to_value()), - (Attribute::Name, Value::new_iname("newname")), - (Attribute::Uuid, Value::Uuid(t_uuid)), - (Attribute::Description, Value::new_utf8s("Tobias")), - (Attribute::DisplayName, Value::new_utf8s("Tobias")) + (Attribute::Name, Value::new_iname("ellie")), + (Attribute::Uuid, Value::Uuid(e_uuid)), + (Attribute::Description, Value::new_utf8s("Ellie Meow")), + (Attribute::DisplayName, Value::new_utf8s("Ellie Meow")) ),]) .is_ok()); - // Now mod it + assert!(server_txn.commit().is_ok()); + setup_name_deny(server).await; + + let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap(); + + // Attempt to mod ellie. + + // Can mod a different attribute assert!(server_txn .internal_modify_uuid( - t_uuid, + e_uuid, + &ModifyList::new_purge_and_set(Attribute::DisplayName, Value::new_utf8s("tobias")) + ) + .is_ok()); + + // Can't mod to another invalid name. + assert!(server_txn + .internal_modify_uuid( + e_uuid, &ModifyList::new_purge_and_set(Attribute::Name, Value::new_iname("tobias")) ) .is_err()); + + // Can mod to a valid name. + assert!(server_txn + .internal_modify_uuid( + e_uuid, + &ModifyList::new_purge_and_set( + Attribute::Name, + Value::new_iname("miss_meowington") + ) + ) + .is_ok()); + + // Now mod from the valid name to an invalid one. + assert!(server_txn + .internal_modify_uuid( + e_uuid, + &ModifyList::new_purge_and_set(Attribute::Name, Value::new_iname("tobias")) + ) + .is_err()); + + assert!(server_txn.commit().is_ok()); + } + + #[qs_test] + async fn test_valuedeny_jpwarren_special(server: &QueryServer) { + // Assert that our break glass accounts are exempt from this processing. + let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap(); + + let me_inv_m = ModifyEvent::new_internal_invalid( + filter!(f_eq(Attribute::Uuid, PVUUID_SYSTEM_CONFIG.clone())), + ModifyList::new_list(vec![ + Modify::Present(Attribute::DeniedName, Value::new_iname("admin")), + Modify::Present(Attribute::DeniedName, Value::new_iname("idm_admin")), + ]), + ); + assert!(server_txn.modify(&me_inv_m).is_ok()); + assert!(server_txn.commit().is_ok()); + + let mut server_txn = server.write(duration_from_epoch_now()).await.unwrap(); + + assert!(server_txn + .internal_modify_uuid( + UUID_IDM_ADMIN, + &ModifyList::new_purge_and_set( + Attribute::DisplayName, + Value::new_utf8s("Idm Admin") + ) + ) + .is_ok()); + + assert!(server_txn + .internal_modify_uuid( + UUID_ADMIN, + &ModifyList::new_purge_and_set(Attribute::DisplayName, Value::new_utf8s("Admin")) + ) + .is_ok()); + + assert!(server_txn.commit().is_ok()); } #[qs_test] diff --git a/server/lib/src/server/migrations.rs b/server/lib/src/server/migrations.rs index 1d2232654..80a1ab3b5 100644 --- a/server/lib/src/server/migrations.rs +++ b/server/lib/src/server/migrations.rs @@ -427,7 +427,10 @@ impl QueryServerWriteTransaction<'_> { // =========== Apply changes ============== // Now update schema - let idm_schema_changes = [SCHEMA_CLASS_DOMAIN_INFO_DL10.clone().into()]; + let idm_schema_changes = [ + SCHEMA_ATTR_DENIED_NAME_DL10.clone().into(), + SCHEMA_CLASS_DOMAIN_INFO_DL10.clone().into(), + ]; idm_schema_changes .into_iter()