Feat: Allowing spn query with non-spn structured data in LDAP (#3400)

* Added Botch for fixing spn query

* Got Invalid filter working. spn can now be searched on

* Addressed review comments

* Resolved Invalid filter correctly for no index

* Cleaned comments and added tests (still 1 failing)

* Added comments and fixed unit test

* Formatting

* Made Clippy Happy

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'
 ```
+<sub>in fact, the key of the bind isn't used at all so `googoogaaga=test1` is entirely valid</sub> ;)
 
 ## Troubleshooting
 

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<EntryInit, EntryNew> = 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| {

server/lib/src/entry.rs

@@ -2912,6 +2912,7 @@ impl<VALID, STATE> Entry<VALID, STATE> {
                 false
             }
             FilterResolved::AndNot(f, _) => !self.entry_match_no_index_inner(f),
+            FilterResolved::Invalid(_) => false,
         }
     }
 

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<FC>),
     AndNot(Box<FC>),
     SelfUuid,
+    Invalid(Attribute),
     // Not(Box<FC>),
 }
 
@@ -135,6 +140,7 @@ enum FilterComp {
     Inclusion(Vec<FilterComp>),
     AndNot(Box<FilterComp>),
     SelfUuid,
+    Invalid(Attribute),
     // Does this mean we can add a true not to the type now?
     // Not(Box<FilterComp>),
 }
@@ -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<NonZeroU8>),
     Or(Vec<FilterResolved>, Option<NonZeroU8>),
     And(Vec<FilterResolved>, Option<NonZeroU8>),
+    Invalid(Attribute),
     // All terms must have 1 or more items, or the inclusion is false!
     Inclusion(Vec<FilterResolved>, Option<NonZeroU8>),
     AndNot(Box<FilterResolved>, Option<NonZeroU8>),
@@ -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)?;
+                let pv = qs.clone_partialvalue(&a, v);
-                FilterComp::Eq(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),
         }
     }
 }

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<EntryInit, EntryNew> = 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::<Vec<_>>()
+            })
+            .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::<Vec<_>>()
+            })
+            .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");

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,
+        f_and, f_andnot, f_eq, f_id, f_inc, f_invalid, f_lt, f_or, f_pres, f_self, f_spn_name,
-        FilterInvalid, FilterValid, FC,
+        f_sub, Filter, FilterInvalid, FilterValid, FC,
     };
     pub use crate::idm::server::{IdmServer, IdmServerAudit, IdmServerDelayed};
     pub use crate::idm::{ClientAuthInfo, ClientCertInfo};