From 303a9d09d602c2cebce5013060521f97dfc0bc1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ciarcin=CC=81ski?= Date: Tue, 21 Apr 2026 14:08:12 +0200 Subject: [PATCH] LDAP: escape critical characters in DN --- .../src/enterprise/ldap/client.rs | 2 +- .../defguard_core/src/enterprise/ldap/mod.rs | 42 ++++++++------- .../defguard_core/src/enterprise/ldap/sync.rs | 30 +++++------ .../src/enterprise/ldap/test_client.rs | 14 ++--- .../src/enterprise/ldap/tests.rs | 22 ++++---- .../tests/integration/ldap/mod.rs | 51 ++++++++++++++++++- 6 files changed, 107 insertions(+), 54 deletions(-) diff --git a/crates/defguard_core/src/enterprise/ldap/client.rs b/crates/defguard_core/src/enterprise/ldap/client.rs index 3cbaad5605..d5347cfa74 100644 --- a/crates/defguard_core/src/enterprise/ldap/client.rs +++ b/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::>(); for entry in &mut membership_entries { diff --git a/crates/defguard_core/src/enterprise/ldap/mod.rs b/crates/defguard_core/src/enterprise/ldap/mod.rs index 541a5f7a56..9f24dbe577 100644 --- a/crates/defguard_core/src/enterprise/ldap/mod.rs +++ b/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(&self, user: &User) -> String { + pub(crate) fn user_dn_for_user(&self, user: &User) -> 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(&mut self, user: &User) -> Result { 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(&mut self, user: &User) -> Result { - 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(&mut self, user: &User) -> 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::>(); 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(()); diff --git a/crates/defguard_core/src/enterprise/ldap/sync.rs b/crates/defguard_core/src/enterprise/ldap/sync.rs index cd4676e40f..7e1b7d15ed 100644 --- a/crates/defguard_core/src/enterprise/ldap/sync.rs +++ b/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::>(); 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,9 +260,9 @@ 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::>(); @@ -270,9 +270,9 @@ pub(super) fn compute_group_sync_changes<'a>( 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::>(); @@ -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...", diff --git a/crates/defguard_core/src/enterprise/ldap/test_client.rs b/crates/defguard_core/src/enterprise/ldap/test_client.rs index 79681ed325..1a4f1042c2 100644 --- a/crates/defguard_core/src/enterprise/ldap/test_client.rs +++ b/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::>(); 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( 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])); } } diff --git a/crates/defguard_core/src/enterprise/ldap/tests.rs b/crates/defguard_core/src/enterprise/ldap/tests.rs index 0a6d64237d..9ed3218b11 100644 --- a/crates/defguard_core/src/enterprise/ldap/tests.rs +++ b/crates/defguard_core/src/enterprise/ldap/tests.rs @@ -147,12 +147,12 @@ fn test_user_dn() { } #[test] -fn test_user_dn_from_user() { +fn test_user_dn_for_user() { let config = LDAPConfig::default(); // User without stored LDAP data uses default search base let user = make_test_user("testuser", None, None); - let dn = config.user_dn_from_user(&user); + let dn = config.user_dn_for_user(&user); assert_eq!(dn, "cn=testuser,ou=users,dc=example,dc=com"); // User with stored RDN and path uses the stored path instead of default @@ -161,7 +161,7 @@ fn test_user_dn_from_user() { Some("testuser".to_string()), Some("ou=admins,dc=example,dc=com".to_string()), ); - let dn = config.user_dn_from_user(&user); + let dn = config.user_dn_for_user(&user); assert_eq!(dn, "cn=testuser,ou=admins,dc=example,dc=com"); // RDN value takes precedence over username when available @@ -170,7 +170,7 @@ fn test_user_dn_from_user() { Some("testuser3".to_string()), Some("ou=people,dc=example,dc=com".to_string()), ); - let dn = config.user_dn_from_user(&user); + let dn = config.user_dn_for_user(&user); assert_eq!(dn, "cn=testuser3,ou=people,dc=example,dc=com"); // Custom RDN attribute affects the final DN format @@ -179,7 +179,7 @@ fn test_user_dn_from_user() { ..LDAPConfig::default() }; let user = make_test_user("user4", Some("testuser4".to_string()), None); - let dn = config.user_dn_from_user(&user); + let dn = config.user_dn_for_user(&user); assert_eq!(dn, "uid=testuser4,ou=users,dc=example,dc=com"); } @@ -363,7 +363,7 @@ async fn test_update_users_state(_: PgPoolOptions, options: PgConnectOptions) { assert!(ldap_conn.test_client.events_match( &[ LdapEvent::ObjectAdded { - dn: ldap_conn.config.user_dn_from_user(&active_user_not_in_ldap), + dn: ldap_conn.config.user_dn_for_user(&active_user_not_in_ldap), attrs: user_to_test_attrs( &active_user_not_in_ldap, Some(PASSWORD), @@ -371,7 +371,7 @@ async fn test_update_users_state(_: PgPoolOptions, options: PgConnectOptions) { ), }, LdapEvent::ObjectDeleted { - dn: ldap_conn.config.user_dn_from_user(&inactive_user_in_ldap), + dn: ldap_conn.config.user_dn_for_user(&inactive_user_in_ldap), }, ], false @@ -425,7 +425,7 @@ async fn test_update_users_state(_: PgPoolOptions, options: PgConnectOptions) { dn: ldap_conn.config.group_dn(&group.name), }, LdapEvent::ObjectDeleted { - dn: ldap_conn.config.user_dn_from_user(&active_user_in_ldap), + dn: ldap_conn.config.user_dn_for_user(&active_user_in_ldap), } ], true @@ -468,11 +468,11 @@ async fn test_update_users_state(_: PgPoolOptions, options: PgConnectOptions) { new_dn: ldap_conn.config.group_dn(&group.name), mods: vec![Mod::Delete( ldap_conn.config.ldap_group_member_attr.clone(), - hashset![ldap_conn.config.user_dn_from_user(&active_user_in_ldap)], + hashset![ldap_conn.config.user_dn_for_user(&active_user_in_ldap)], )], }, LdapEvent::ObjectDeleted { - dn: ldap_conn.config.user_dn_from_user(&active_user_in_ldap), + dn: ldap_conn.config.user_dn_for_user(&active_user_in_ldap), }, ], true, @@ -505,7 +505,7 @@ async fn test_update_users_state(_: PgPoolOptions, options: PgConnectOptions) { LdapEvent::ObjectDeleted { dn: ldap_conn .config - .user_dn_from_user(&another_active_user_in_ldap), + .user_dn_for_user(&another_active_user_in_ldap), }, ], true, diff --git a/crates/defguard_core/tests/integration/ldap/mod.rs b/crates/defguard_core/tests/integration/ldap/mod.rs index 06591911bf..61f4eb74c6 100644 --- a/crates/defguard_core/tests/integration/ldap/mod.rs +++ b/crates/defguard_core/tests/integration/ldap/mod.rs @@ -42,11 +42,15 @@ async fn set_ldap_settings(pool: &PgPool) { settings.ldap_member_attr = env::var("LDAP_MEMBER_ATTR").ok(); settings.ldap_use_starttls = env::var("LDAP_STARTTLS").is_ok(); settings.ldap_tls_verify_cert = env::var("LDAP_TLS_VERIFY").is_ok(); + if env::var("LDAP_USES_AD").is_ok() { + settings.ldap_user_rdn_attr = Some(String::from("cn")); + settings.ldap_user_auxiliary_obj_classes = vec![String::from("user")]; + } settings.ldap_enabled = true; set_settings(Some(settings)); } -#[ignore = "Requires LDAP server"] +#[ignore = "requires LDAP server"] #[sqlx::test] async fn test_ldap(_: PgPoolOptions, options: PgConnectOptions) { let pool = setup_pool(options).await; @@ -68,6 +72,7 @@ async fn test_ldap(_: PgPoolOptions, options: PgConnectOptions) { let mut ldap_conn = LDAPConnection::create().await.unwrap(); ldap_conn.config.ldap_sync_groups = vec![String::from("testers")]; + ldap_conn.config.ldap_uses_ad = env::var("LDAP_USES_AD").is_ok(); // Try to remove user first, in case the previous test run failed. let _ = ldap_conn.delete_user(&user).await; @@ -97,7 +102,7 @@ async fn test_ldap(_: PgPoolOptions, options: PgConnectOptions) { // Build user DN. let dn = format!( "{}={},{}", - ldap_conn.config.ldap_username_attr, + ldap_conn.config.get_rdn_attr(), user.ldap_rdn.as_ref().unwrap(), user.ldap_user_path.as_ref().unwrap() ); @@ -110,3 +115,45 @@ async fn test_ldap(_: PgPoolOptions, options: PgConnectOptions) { ldap_conn.delete_group(&group.name).await.unwrap(); ldap_conn.delete_user(&user).await.unwrap(); } + +#[ignore = "requires LDAP server"] +#[sqlx::test] +async fn test_special_characters(_: PgPoolOptions, options: PgConnectOptions) { + let pool = setup_pool(options).await; + set_ldap_settings(&pool).await; + + let mut ldap_conn = LDAPConnection::create().await.unwrap(); + ldap_conn.config.ldap_uses_ad = env::var("LDAP_USES_AD").is_ok(); + + let password = "pass123"; + let mut user = User::new( + "ハリー・ポッター", + Some(password), + "ハリー", + "ポッター", + "hari.potta@hogwards.jp", + None, + ) + .save(&pool) + .await + .unwrap(); + + // Try to remove user first, in case the previous test run failed. + let _ = ldap_conn.delete_user(&user).await; + // Add user to LDAP. + ldap_conn + .add_user(&mut user, Some(password), &pool) + .await + .unwrap(); + + const TEST_GROUP: &str = "Wizards🪄,+\"\\<>=#🧙‍♂️"; + // Add group to LDAP. This is redundant as `add_user_to_group` does the same. + ldap_conn + .add_group_with_members(TEST_GROUP, &[&user]) + .await + .unwrap(); + + // Cleanup + ldap_conn.delete_group(TEST_GROUP).await.unwrap(); + ldap_conn.delete_user(&user).await.unwrap(); +}