diff --git a/book/src/integrations/ldap.md b/book/src/integrations/ldap.md index da28e67ed..adb629434 100644 --- a/book/src/integrations/ldap.md +++ b/book/src/integrations/ldap.md @@ -145,7 +145,8 @@ with a dn of `dn=token` and provide the api token in the password. > [!NOTE] > > The `dn=token` keyword is guaranteed to not be used by any other entry, which is why it was chosen -> as the keyword to initiate api token binds. +> as the keyword to initiate api token binds. Additionally it is not required, leaving the field empty +> will fall back to the service-account if a "password" is provided ```bash ldapwhoami -H ldaps://URL -x -D "dn=token" -w "TOKEN" @@ -234,6 +235,7 @@ ldapwhoami ... -x -D '22a65b6c-80c8-4e1a-9b76-3f3afdff8400' ldapwhoami ... -x -D 'spn=test1@idm.example.com,dc=idm,dc=example,dc=com' ldapwhoami ... -x -D 'name=test1,dc=idm,dc=example,dc=com' ``` +in fact, the key of the bind isn't used at all so `googoogaaga=test1` is entirely valid ;) ## Troubleshooting diff --git a/server/lib/src/be/mod.rs b/server/lib/src/be/mod.rs index dfc23a2e5..954f8a9a5 100644 --- a/server/lib/src/be/mod.rs +++ b/server/lib/src/be/mod.rs @@ -571,6 +571,10 @@ pub trait BackendTransaction { filter_error!("Requested a top level or isolated AndNot, returning empty"); (IdList::Indexed(IDLBitRange::new()), FilterPlan::Invalid) } + FilterResolved::Invalid(_) => { + // Indexed since it is always false and we don't want to influence filter testing + (IdList::Indexed(IDLBitRange::new()), FilterPlan::Invalid) + } }) } @@ -2376,6 +2380,46 @@ mod tests { }); } + #[test] + fn test_be_search_with_invalid() { + run_test!(|be: &mut BackendWriteTransaction| { + trace!("Simple Search"); + + let mut e: Entry = Entry::new(); + e.add_ava(Attribute::UserId, Value::from("bagel")); + e.add_ava( + Attribute::Uuid, + Value::from("db237e8a-0079-4b8c-8a56-593b22aa44d1"), + ); + let e = e.into_sealed_new(); + + let single_result = be.create(&CID_ZERO, vec![e]); + assert!(single_result.is_ok()); + + // Test Search with or condition including invalid attribute + let filt = filter_resolved!(f_or(vec![ + f_eq(Attribute::UserId, PartialValue::new_utf8s("bagel")), + f_invalid(Attribute::UserId) + ])); + + let lims = Limits::unlimited(); + + let r = be.search(&lims, &filt); + assert!(r.expect("Search failed!").len() == 1); + + // Test Search with or condition including invalid attribute + let filt = filter_resolved!(f_and(vec![ + f_eq(Attribute::UserId, PartialValue::new_utf8s("bagel")), + f_invalid(Attribute::UserId) + ])); + + let lims = Limits::unlimited(); + + let r = be.search(&lims, &filt); + assert!(r.expect("Search failed!").len() == 0); + }); + } + #[test] fn test_be_simple_modify() { run_test!(|be: &mut BackendWriteTransaction| { diff --git a/server/lib/src/entry.rs b/server/lib/src/entry.rs index 5ad112a07..88efd32b8 100644 --- a/server/lib/src/entry.rs +++ b/server/lib/src/entry.rs @@ -2912,6 +2912,7 @@ impl Entry { false } FilterResolved::AndNot(f, _) => !self.entry_match_no_index_inner(f), + FilterResolved::Invalid(_) => false, } } diff --git a/server/lib/src/filter.rs b/server/lib/src/filter.rs index cfb09de53..b9062e463 100644 --- a/server/lib/src/filter.rs +++ b/server/lib/src/filter.rs @@ -83,6 +83,10 @@ pub fn f_self() -> FC { FC::SelfUuid } +pub fn f_invalid(a: Attribute) -> FC { + FC::Invalid(a) +} + pub fn f_id(uuid: &str) -> FC { let uf = Uuid::parse_str(uuid) .ok() @@ -117,6 +121,7 @@ pub enum FC { Inclusion(Vec), AndNot(Box), SelfUuid, + Invalid(Attribute), // Not(Box), } @@ -135,6 +140,7 @@ enum FilterComp { Inclusion(Vec), AndNot(Box), SelfUuid, + Invalid(Attribute), // Does this mean we can add a true not to the type now? // Not(Box), } @@ -196,12 +202,15 @@ impl fmt::Debug for FilterComp { FilterComp::SelfUuid => { write!(f, "uuid eq self") } + FilterComp::Invalid(attr) => { + write!(f, "invalid ( {:?} )", attr) + } } } } /// This is the fully resolved internal representation. Note the lack of Not and selfUUID -/// because these are resolved into And(Pres(class), AndNot(term)) and Eq(uuid, ...). +/// because these are resolved into And(Pres(class), AndNot(term)) and Eq(uuid, ...) respectively. /// Importantly, we make this accessible to Entry so that it can then match on filters /// internally. /// @@ -221,6 +230,7 @@ pub enum FilterResolved { LessThan(Attribute, PartialValue, Option), Or(Vec, Option), And(Vec, Option), + Invalid(Attribute), // All terms must have 1 or more items, or the inclusion is false! Inclusion(Vec, Option), AndNot(Box, Option), @@ -310,6 +320,9 @@ impl fmt::Debug for FilterResolved { FilterResolved::AndNot(inner, idx) => { write!(f, "not (s{} {:?})", idx.unwrap_or(NonZeroU8::MAX), inner) } + FilterResolved::Invalid(attr) => { + write!(f, "{} inv", attr) + } } } } @@ -777,6 +790,7 @@ impl FilterComp { FC::Inclusion(v) => FilterComp::Inclusion(v.into_iter().map(FilterComp::new).collect()), FC::AndNot(b) => FilterComp::AndNot(Box::new(FilterComp::new(*b))), FC::SelfUuid => FilterComp::SelfUuid, + FC::Invalid(a) => FilterComp::Invalid(a), } } @@ -804,7 +818,8 @@ impl FilterComp { | FilterComp::Stw(attr, _) | FilterComp::Enw(attr, _) | FilterComp::Pres(attr) - | FilterComp::LessThan(attr, _) => { + | FilterComp::LessThan(attr, _) + | FilterComp::Invalid(attr) => { r_set.insert(attr.clone()); } FilterComp::Or(vs) => vs.iter().for_each(|f| f.get_attr_set(r_set)), @@ -952,6 +967,11 @@ impl FilterComp { // Pretty hard to mess this one up ;) Ok(FilterComp::SelfUuid) } + FilterComp::Invalid(attr) => { + // FilterComp may be invalid but Invalid is still a valid value. + // we continue the evaluation so OR queries can still succeed + Ok(FilterComp::Invalid(attr.clone())) + } } } @@ -1097,8 +1117,13 @@ impl FilterComp { } LdapFilter::Equality(a, v) => { let a = ldap_attr_filter_map(a); - let v = qs.clone_partialvalue(&a, v)?; - FilterComp::Eq(a, v) + let pv = qs.clone_partialvalue(&a, v); + + match pv { + Ok(pv) => FilterComp::Eq(a, pv), + Err(_) if a == Attribute::Spn => FilterComp::Invalid(a), + Err(err) => return Err(err), + } } LdapFilter::Present(a) => FilterComp::Pres(ldap_attr_filter_map(a)), LdapFilter::Substring( @@ -1267,6 +1292,7 @@ impl FilterResolved { FilterResolved::Eq(a, v, idx) } FilterComp::SelfUuid => panic!("Not possible to resolve SelfUuid in from_invalid!"), + FilterComp::Invalid(attr) => FilterResolved::Invalid(attr), FilterComp::Cnt(a, v) => { let idx = idxmeta.contains(&(&a, &IndexType::SubString)); let idx = NonZeroU8::new(idx as u8); @@ -1340,6 +1366,7 @@ impl FilterResolved { | FilterComp::Stw(..) | FilterComp::Enw(..) | FilterComp::Pres(_) + | FilterComp::Invalid(_) | FilterComp::LessThan(..) => true, } } @@ -1435,6 +1462,7 @@ impl FilterResolved { FilterResolved::resolve_idx((*f).clone(), ev, idxmeta) .map(|fi| FilterResolved::AndNot(Box::new(fi), None)) } + FilterComp::Invalid(attr) => Some(FilterResolved::Invalid(attr)), } } @@ -1493,6 +1521,7 @@ impl FilterResolved { FilterResolved::resolve_no_idx((*f).clone(), ev) .map(|fi| FilterResolved::AndNot(Box::new(fi), None)) } + FilterComp::Invalid(attr) => Some(FilterResolved::Invalid(attr)), } } @@ -1632,6 +1661,8 @@ impl FilterResolved { | FilterResolved::And(_, sf) | FilterResolved::Inclusion(_, sf) | FilterResolved::AndNot(_, sf) => *sf, + // We hard code 1 because there is no slope for an invlid filter + FilterResolved::Invalid(_) => NonZeroU8::new(1), } } } diff --git a/server/lib/src/idm/ldap.rs b/server/lib/src/idm/ldap.rs index f71562b72..63956762f 100644 --- a/server/lib/src/idm/ldap.rs +++ b/server/lib/src/idm/ldap.rs @@ -205,9 +205,9 @@ impl LdapServer { // Map the Some(a,v) to ...? let ext_filter = match (&sr.scope, req_dn) { - // OneLevel and Child searches are veerrrryyy similar for us because child + // OneLevel and Child searches are **very** similar for us because child // is a "subtree search excluding base". Because we don't have a tree structure at - // all, this is the same as a onelevel (ald children of base excludeing base). + // all, this is the same as a one level (all children of base excluding base). (LdapSearchScope::Children, Some(_r)) | (LdapSearchScope::OneLevel, Some(_r)) => { return Ok(vec![sr.gen_success()]) } @@ -239,7 +239,7 @@ impl LdapServer { let mut all_attrs = false; let mut all_op_attrs = false; - // TODO #67: limit the number of attributes here! + // TODO #3406: limit the number of attributes here! if sr.attrs.is_empty() { // If [], then "all" attrs all_attrs = true; @@ -1158,6 +1158,88 @@ mod tests { assert_eq!(r1.len(), r2.len()); } + #[idm_test] + async fn test_ldap_spn_search(idms: &IdmServer, _idms_delayed: &IdmServerDelayed) { + let ldaps = LdapServer::new(idms).await.expect("failed to start ldap"); + + let usr_uuid = Uuid::new_v4(); + let usr_name = "panko"; + + // Setup person, group and application + { + let e1: Entry = 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(usr_name)), + (Attribute::Uuid, Value::Uuid(usr_uuid)), + (Attribute::DisplayName, Value::new_utf8s(usr_name)) + ); + + let ct = duration_from_epoch_now(); + let mut server_txn = idms.proxy_write(ct).await.unwrap(); + assert!(server_txn + .qs_write + .internal_create(vec![e1]) + .and_then(|_| server_txn.commit()) + .is_ok()); + } + + // Setup the anonymous login + let anon_t = ldaps.do_bind(idms, "", "").await.unwrap().unwrap(); + assert_eq!( + anon_t.effective_session, + LdapSession::UnixBind(UUID_ANONYMOUS) + ); + + // Searching a malformed spn shouldn't cause the query to fail + let sr = SearchRequest { + msgid: 1, + base: format!("dc=example,dc=com"), + scope: LdapSearchScope::Subtree, + filter: LdapFilter::Or(vec![ + LdapFilter::Equality(Attribute::Name.to_string(), usr_name.to_string()), + LdapFilter::Equality(Attribute::Spn.to_string(), usr_name.to_string()), + ]), + attrs: vec!["*".to_string()], + }; + + let result = ldaps + .do_search(idms, &sr, &anon_t, Source::Internal) + .await + .map(|r| { + r.into_iter() + .filter(|r| matches!(r.op, LdapOp::SearchResultEntry(_))) + .collect::>() + }) + .unwrap(); + + assert!(!result.is_empty()); + + let sr = SearchRequest { + msgid: 1, + base: format!("dc=example,dc=com"), + scope: LdapSearchScope::Subtree, + filter: LdapFilter::And(vec![ + LdapFilter::Equality(Attribute::Name.to_string(), usr_name.to_string()), + LdapFilter::Equality(Attribute::Spn.to_string(), usr_name.to_string()), + ]), + attrs: vec!["*".to_string()], + }; + + let empty_result = ldaps + .do_search(idms, &sr, &anon_t, Source::Internal) + .await + .map(|r| { + r.into_iter() + .filter(|r| matches!(r.op, LdapOp::SearchResultEntry(_))) + .collect::>() + }) + .unwrap(); + + assert!(empty_result.is_empty()); + } + #[idm_test] async fn test_ldap_application_bind(idms: &IdmServer, _idms_delayed: &IdmServerDelayed) { let ldaps = LdapServer::new(idms).await.expect("failed to start ldap"); diff --git a/server/lib/src/lib.rs b/server/lib/src/lib.rs index f52c5c11b..562042536 100644 --- a/server/lib/src/lib.rs +++ b/server/lib/src/lib.rs @@ -89,8 +89,8 @@ pub mod prelude { }; pub use crate::event::{CreateEvent, DeleteEvent, ExistsEvent, ModifyEvent, SearchEvent}; pub use crate::filter::{ - f_and, f_andnot, f_eq, f_id, f_inc, f_lt, f_or, f_pres, f_self, f_spn_name, f_sub, Filter, - FilterInvalid, FilterValid, FC, + f_and, f_andnot, f_eq, f_id, f_inc, f_invalid, f_lt, f_or, f_pres, f_self, f_spn_name, + f_sub, Filter, FilterInvalid, FilterValid, FC, }; pub use crate::idm::server::{IdmServer, IdmServerAudit, IdmServerDelayed}; pub use crate::idm::{ClientAuthInfo, ClientCertInfo};