LDAP: escape critical characters in DN

crates/defguard_core/src/enterprise/ldap/client.rs

@@ -221,7 +221,7 @@ impl LDAPConnection {
         // dn: user map
         let dn_map = all_ldap_users
             .iter()
-            .map(|u| (self.config.user_dn_from_user(u).to_lowercase(), u))
+            .map(|u| (self.config.user_dn_for_user(u).to_lowercase(), u))
             .collect::<HashMap<_, _>>();
 
         for entry in &mut membership_entries {

crates/defguard_core/src/enterprise/ldap/mod.rs

@@ -10,7 +10,7 @@ use defguard_common::db::{
 };
 #[cfg(not(test))]
 use ldap3::Ldap;
-use ldap3::{Mod, ldap_escape};
+use ldap3::{Mod, dn_escape, ldap_escape};
 use model::UserObjectClass;
 use rand::Rng;
 use sqlx::PgPool;
@@ -194,7 +194,7 @@ impl LDAPConfig {
     /// Returns the RDN attribute used for constructing user distinguished names.
     /// If the `ldap_user_rdn_attr` is not set, it defaults to `ldap_username_attr`.
     #[must_use]
-    pub(crate) fn get_rdn_attr(&self) -> &str {
+    pub fn get_rdn_attr(&self) -> &str {
         let attr = self
             .ldap_user_rdn_attr
             .as_deref()
@@ -211,20 +211,24 @@ impl LDAPConfig {
     /// Constructs user distinguished name.
     ///
     /// This function is used to construct the user's DN based on the RDN value and user path.
-    /// Prefer using `user_dn_from_user` method to ensure that the RDN value and user path are
+    /// Prefer using `user_dn_for_user` method to ensure that the RDN value and user path are
     /// correctly derived from the user object.
     ///
     /// Use it only if you need to construct a user DN manually.
     #[must_use]
     pub(crate) fn user_dn(&self, user_rdn_value: &str, user_path: &str) -> String {
-        format!("{}={user_rdn_value},{user_path}", self.get_rdn_attr())
+        format!(
+            "{}={},{user_path}",
+            self.get_rdn_attr(),
+            dn_escape(user_rdn_value)
+        )
     }
 
     /// Constructs the user's distinguished name based on the user object.
     /// This should be the preferred way of getting the user DN, as it
     /// ensures that the RDN value and user path is correctly derived from the user object.
     #[must_use]
-    pub(crate) fn user_dn_from_user<I>(&self, user: &User<I>) -> String {
+    pub(crate) fn user_dn_for_user<I>(&self, user: &User<I>) -> String {
         let path = if let Some(path) = &user.ldap_user_path {
             path.as_str()
         } else {
@@ -236,13 +240,15 @@ impl LDAPConfig {
     /// Constructs group distinguished name.
     ///
     /// Uses the `ldap_group_search_base` to construct the DN.
-    /// Note: This may turn out to be a problem if some groups are
-    /// are nested and have different DN paths.
+    /// Note: This may turn out to be a problem if some groups are nested and have different DN
+    /// paths.
     #[must_use]
     pub(crate) fn group_dn(&self, groupname: &str) -> String {
         format!(
-            "{}={groupname},{}",
-            self.ldap_groupname_attr, self.ldap_group_search_base,
+            "{}={},{}",
+            self.ldap_groupname_attr,
+            dn_escape(groupname),
+            self.ldap_group_search_base,
         )
     }
 
@@ -418,7 +424,7 @@ impl LDAPConnection {
             return Ok(true);
         }
 
-        let dn = self.config.user_dn_from_user(user);
+        let dn = self.config.user_dn_for_user(user);
 
         if !self.user_exists(user).await? {
             debug!("User {user} does not exist, not syncing user");
@@ -492,7 +498,7 @@ impl LDAPConnection {
     /// usernames which Defguard doesn't handle well.
     async fn user_exists<I>(&mut self, user: &User<I>) -> Result<bool, LdapError> {
         let username = &user.username;
-        let dn = self.config.user_dn_from_user(user);
+        let dn = self.config.user_dn_for_user(user);
         let username_exists = self.user_exists_by_username(username).await?;
         let dn_exists = self.user_exists_by_dn(&dn).await?;
         Ok(username_exists || dn_exists)
@@ -569,7 +575,7 @@ impl LDAPConnection {
     /// Retrieves user from LDAP by DN (Distinguished Name).
     /// Returns an error if the user doesn't exist at the specified DN.
     pub async fn get_user_by_dn<I>(&mut self, user: &User<I>) -> Result<User, LdapError> {
-        let dn = self.config.user_dn_from_user(user);
+        let dn = self.config.user_dn_for_user(user);
         debug!("Trying to retrieve LDAP user with the following DN: {dn}");
         match self.get(&dn).await? {
             Some(entry) => {
@@ -591,7 +597,7 @@ impl LDAPConnection {
         pool: &PgPool,
     ) -> Result<(), LdapError> {
         debug!("Adding LDAP user {user}");
-        let user_dn = self.config.user_dn_from_user(user);
+        let user_dn = self.config.user_dn_for_user(user);
         let password_is_random = password.is_none();
         let password = if let Some(password) = password {
             debug!("Using provided password for user {user}");
@@ -691,7 +697,7 @@ impl LDAPConnection {
     /// First removes the user from all group memberships (if any), then deletes the user entry.
     pub async fn delete_user<I>(&mut self, user: &User<I>) -> Result<(), LdapError> {
         debug!("Deleting user {user}");
-        let dn = self.config.user_dn_from_user(user);
+        let dn = self.config.user_dn_for_user(user);
         debug!("Removing group memberships first...");
         let user_groups = self.get_user_groups(&dn).await?;
         debug!("Removing user from groups: {user_groups:?}");
@@ -736,7 +742,7 @@ impl LDAPConnection {
         password: &str,
     ) -> Result<(), LdapError> {
         debug!("Setting password for user {user}");
-        let user_dn = self.config.user_dn_from_user(user);
+        let user_dn = self.config.user_dn_for_user(user);
 
         if self.config.ldap_uses_ad {
             let unicode_pwd = hash::unicode_pwd(password);
@@ -808,7 +814,7 @@ impl LDAPConnection {
         // Extend the group attr with multiple members.
         let member_dns = members
             .iter()
-            .map(|member| self.config.user_dn_from_user(member))
+            .map(|member| self.config.user_dn_for_user(member))
             .collect::<Vec<_>>();
         let member_group_attr = self.config.ldap_group_member_attr.clone();
         let member_refs = member_dns
@@ -875,7 +881,7 @@ impl LDAPConnection {
         groupname: &str,
     ) -> Result<(), LdapError> {
         debug!("Adding user {user} to group {groupname} in LDAP, checking if that group exists...");
-        let user_dn = self.config.user_dn_from_user(user);
+        let user_dn = self.config.user_dn_for_user(user);
         if self.is_member_of(&user_dn, groupname).await? {
             debug!("User {user} is already a member of group {groupname}, skipping");
             return Ok(());
@@ -911,7 +917,7 @@ impl LDAPConnection {
         groupname: &str,
     ) -> Result<(), LdapError> {
         debug!("Removing user {user} from group {groupname} in LDAP");
-        let user_dn = self.config.user_dn_from_user(user);
+        let user_dn = self.config.user_dn_for_user(user);
         if !self.is_member_of(&user_dn, groupname).await? {
             debug!("User {user} is not a member of group {groupname}, skipping");
             return Ok(());

crates/defguard_core/src/enterprise/ldap/sync.rs

@@ -161,17 +161,17 @@ pub(super) fn compute_user_sync_changes(
     let mut ldap_identifiers = HashSet::with_capacity(all_ldap_users.len());
     let defguard_identifiers = all_defguard_users
         .iter()
-        .map(|u| ldap_config.user_dn_from_user(u))
+        .map(|u| ldap_config.user_dn_for_user(u))
         .collect::<HashSet<_>>();
 
     trace!("Defguard identifiers: {defguard_identifiers:?}");
     trace!("LDAP identifiers: {ldap_identifiers:?}");
 
     for user in all_ldap_users.drain(..) {
-        ldap_identifiers.insert(ldap_config.user_dn_from_user(&user));
+        ldap_identifiers.insert(ldap_config.user_dn_for_user(&user));
 
         debug!("Checking if user {} is in Defguard", user.username);
-        if !defguard_identifiers.contains(&ldap_config.user_dn_from_user(&user)) {
+        if !defguard_identifiers.contains(&ldap_config.user_dn_for_user(&user)) {
             debug!("User {} not found in Defguard", user.username);
             match authority {
                 Authority::LDAP => add_defguard.push(user),
@@ -182,7 +182,7 @@ pub(super) fn compute_user_sync_changes(
 
     for user in all_defguard_users.drain(..) {
         debug!("Checking if user {} is in LDAP", user.username);
-        if !ldap_identifiers.contains(&ldap_config.user_dn_from_user(&user)) {
+        if !ldap_identifiers.contains(&ldap_config.user_dn_for_user(&user)) {
             debug!("User {} not found in LDAP", user.username);
             match authority {
                 Authority::LDAP => {
@@ -260,19 +260,19 @@ pub(super) fn compute_group_sync_changes<'a>(
             let missing_from_defguard = ldap_members
                 .iter()
                 .filter(|u| {
-                    !members.iter().any(|m| {
-                        ldap_config.user_dn_from_user(m) == ldap_config.user_dn_from_user(u)
-                    })
+                    !members
+                        .iter()
+                        .any(|m| ldap_config.user_dn_for_user(m) == ldap_config.user_dn_for_user(u))
                 })
                 .copied()
                 .collect::<HashSet<_>>();
 
             let missing_from_ldap = members
                 .iter()
                 .filter(|m| {
-                    !ldap_members.iter().any(|u| {
-                        ldap_config.user_dn_from_user(m) == ldap_config.user_dn_from_user(u)
-                    })
+                    !ldap_members
+                        .iter()
+                        .any(|u| ldap_config.user_dn_for_user(m) == ldap_config.user_dn_for_user(u))
                 })
                 .cloned()
                 .collect::<HashSet<_>>();
@@ -440,7 +440,7 @@ pub(super) fn extract_intersecting_users(
         if let Some(ldap_user) = ldap_users
             .iter()
             .position(|u| {
-                ldap_config.user_dn_from_user(u) == ldap_config.user_dn_from_user(defguard_user)
+                ldap_config.user_dn_for_user(u) == ldap_config.user_dn_for_user(defguard_user)
             })
             .map(|i| ldap_users.remove(i))
         {
@@ -451,7 +451,7 @@ pub(super) fn extract_intersecting_users(
     for user in intersecting_users_ldap {
         if let Some(defguard_user) = defguard_users
             .iter()
-            .position(|u| ldap_config.user_dn_from_user(u) == ldap_config.user_dn_from_user(&user))
+            .position(|u| ldap_config.user_dn_for_user(u) == ldap_config.user_dn_for_user(&user))
             .map(|i| defguard_users.remove(i))
         {
             intersecting_users.push((user, defguard_user));
@@ -526,7 +526,7 @@ impl super::LDAPConnection {
             Authority::LDAP
         };
 
-        let user_dn = self.config.user_dn_from_user(user);
+        let user_dn = self.config.user_dn_for_user(user);
         let ldap_user = self.get_user_by_dn(user).await?;
         let defguard_groups = user.member_of_names(pool).await?;
         let ldap_groups = self.get_user_groups(&user_dn).await?;
@@ -863,8 +863,8 @@ impl super::LDAPConnection {
             if let Some(defguard_user) =
                 User::find_by_username(&mut *transaction, &user.username).await?
             {
-                let defguard_user_dn = self.config.user_dn_from_user(&defguard_user);
-                let ldap_user_dn = self.config.user_dn_from_user(&user);
+                let defguard_user_dn = self.config.user_dn_for_user(&defguard_user);
+                let ldap_user_dn = self.config.user_dn_for_user(&user);
                 if defguard_user_dn == ldap_user_dn {
                     debug!(
                         "User {} (DN: {}) already exists in Defguard, skipping...",

crates/defguard_core/src/enterprise/ldap/test_client.rs

@@ -192,13 +192,13 @@ impl TestClient {
     }
 
     pub(super) fn add_test_user(&mut self, user: &User, config: &LDAPConfig) {
-        let dn = config.user_dn_from_user(user);
+        let dn = config.user_dn_for_user(user);
         self.objects
             .insert(dn, Object::User(Box::new(user.clone())));
     }
 
     pub(super) fn remove_test_user(&mut self, user: &User, config: &LDAPConfig) {
-        let dn = config.user_dn_from_user(user);
+        let dn = config.user_dn_for_user(user);
         self.objects.remove(&dn);
     }
 
@@ -210,7 +210,7 @@ impl TestClient {
 
     pub(super) fn add_test_membership(&mut self, group: &Group, user: &User, config: &LDAPConfig) {
         let group_dn = config.group_dn(&group.name);
-        let user_dn = config.user_dn_from_user(user);
+        let user_dn = config.user_dn_for_user(user);
         self.memberships
             .entry(group_dn)
             .or_default()
@@ -224,7 +224,7 @@ impl TestClient {
         config: &LDAPConfig,
     ) {
         let group_dn = config.group_dn(&group.name);
-        let user_dn = config.user_dn_from_user(user);
+        let user_dn = config.user_dn_for_user(user);
         if let Some(members) = self.memberships.get_mut(&group_dn) {
             members.remove(&user_dn);
             if members.is_empty() {
@@ -437,7 +437,7 @@ impl LDAPConnection {
         let mut result = HashMap::new();
         let user_dns = all_ldap_users
             .iter()
-            .map(|user| self.config.user_dn_from_user(user))
+            .map(|user| self.config.user_dn_for_user(user))
             .collect::<HashSet<_>>();
         for (group_dn, member_dns) in memberships {
             let members = member_dns
@@ -446,7 +446,7 @@ impl LDAPConnection {
                     if user_dns.contains(member_dn) {
                         all_ldap_users
                             .iter()
-                            .find(|user| self.config.user_dn_from_user(user) == *member_dn)
+                            .find(|user| self.config.user_dn_for_user(user) == *member_dn)
                     } else {
                         None
                     }
@@ -599,7 +599,7 @@ pub(super) fn group_to_test_attrs<I>(
 
     if let Some(members) = members {
         for user in members {
-            let user_dn = config.user_dn_from_user(user);
+            let user_dn = config.user_dn_for_user(user);
             attrs.push((config.ldap_group_member_attr.clone(), hashset![user_dn]));
         }
     }