From d0b0b163fdd734f022cfee2d706b60c3e3d21762 Mon Sep 17 00:00:00 2001
From: Firstyear <william@blackhats.net.au>
Date: Sat, 15 Feb 2025 16:01:44 +1000
Subject: [PATCH 1/2] Book fixes (#3433)
---
book/src/integrations/oauth2.md | 2 +-
book/src/supported_features.md | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/book/src/integrations/oauth2.md b/book/src/integrations/oauth2.md
index 54d09bcf8..6aaac2f26 100644
--- a/book/src/integrations/oauth2.md
+++ b/book/src/integrations/oauth2.md
@@ -215,7 +215,7 @@ Token signing public key
### Create the Kanidm Configuration
-By default, members of the `system_admins` or `idm_hp_oauth2_manage_priv` groups are able to create
+By default, members of the `idm_admins` or `idm_oauth2_admins` groups are able to create
or manage OAuth2 client integrations.
You can create a new client by specifying its client name, application display name and the landing
diff --git a/book/src/supported_features.md b/book/src/supported_features.md
index 20895a41a..d2ef9a53f 100644
--- a/book/src/supported_features.md
+++ b/book/src/supported_features.md
@@ -22,6 +22,7 @@ This is a list of supported features and standards within Kanidm.
- [RFC4519 LDAP Schema](https://www.rfc-editor.org/rfc/rfc4519)
- FreeIPA User Schema
- [RFC7644 SCIM Bulk Data Import](https://www.rfc-editor.org/rfc/rfc7644)
+ - NOTE: SCIM is only supported for synchronisation from another IDP at this time.
# Database
From ed88b72080458dfe7b6dfddbfd46d20d8e556e45 Mon Sep 17 00:00:00 2001
From: Firstyear <william@blackhats.net.au>
Date: Sun, 16 Feb 2025 08:45:25 +1000
Subject: [PATCH 2/2] 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<EntrySealedCommitted>],
+ pre_cand: &[Arc<EntrySealedCommitted>],
cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
_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<EntrySealedCommitted>],
+ pre_cand: &[Arc<EntrySealedCommitted>],
cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
_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<Result<(), ConsistencyError>> {
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<Entry<EntryInvalid, EntryCommitted>>,
+ pre_cand: &[Arc<EntrySealedCommitted>],
+ 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()