public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling
@ 2023-06-26  9:39 Stefan Sterz
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 1/4] ldap: remove support for unauthenticated binds Stefan Sterz
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Stefan Sterz @ 2023-06-26  9:39 UTC (permalink / raw)
  To: pbs-devel

this patch series does two things:

1. it improves the creation and updating of ldap configurations by
   checking it against an ldap directory instead of a regex. thus,
   increasing the likelihood of the configuration being correct
2. remove the ability configure and use unauthenticated binds.
   unauthenticated binds are generally discouraged and we don't
   support them in proxmox ve either, so remove them in the backup
   server too.

removing unauthenticated binds is a breaking change as some users may
already rely on this functionality.

Stefan Sterz (1):
  window: ldap auth edit forbid specifying a bind_dn without a password

 src/window/AuthEditLDAP.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.39.2





^ permalink raw reply	[flat|nested] 18+ messages in thread

* [pbs-devel] [PATCH proxmox 1/4] ldap: remove support for unauthenticated binds
  2023-06-26  9:39 [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Stefan Sterz
@ 2023-06-26  9:39 ` Stefan Sterz
  2023-06-26 13:00   ` [pbs-devel] applied: " Wolfgang Bumiller
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 2/4] ldap: add check_connection function Stefan Sterz
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Stefan Sterz @ 2023-06-26  9:39 UTC (permalink / raw)
  To: pbs-devel

by using the default empty string if no password was provided,
unauthenticated binds were possible. to bring pbs in-line with pve,
switch to throwing an error in this case instead. however, this will
break any pre-existing setup that relied on this behavior.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 proxmox-ldap/src/lib.rs | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/proxmox-ldap/src/lib.rs b/proxmox-ldap/src/lib.rs
index 7c735b8..cdc4c9d 100644
--- a/proxmox-ldap/src/lib.rs
+++ b/proxmox-ldap/src/lib.rs
@@ -6,7 +6,7 @@ use std::{
     time::Duration,
 };
 
-use anyhow::{bail, Error};
+use anyhow::{bail, format_err, Error};
 use ldap3::adapters::{Adapter, EntriesOnly, PagedResults};
 use ldap3::{Ldap, LdapConnAsync, LdapConnSettings, LdapResult, Scope, SearchEntry};
 use native_tls::{Certificate, TlsConnector, TlsConnectorBuilder};
@@ -119,7 +119,11 @@ impl Connection {
         let mut ldap = self.create_connection().await?;
 
         if let Some(bind_dn) = self.config.bind_dn.as_deref() {
-            let password = self.config.bind_password.as_deref().unwrap_or_default();
+            let password = self
+                .config
+                .bind_password
+                .as_deref()
+                .ok_or_else(|| format_err!("Missing bind password for {bind_dn}"))?;
             let _: LdapResult = ldap.simple_bind(bind_dn, password).await?.success()?;
         }
 
@@ -254,7 +258,11 @@ impl Connection {
         let mut ldap = self.create_connection().await?;
 
         if let Some(bind_dn) = self.config.bind_dn.as_deref() {
-            let password = self.config.bind_password.as_deref().unwrap_or_default();
+            let password = self
+                .config
+                .bind_password
+                .as_deref()
+                .ok_or_else(|| format_err!("Missing bind password for {bind_dn}"))?;
             let _: LdapResult = ldap.simple_bind(bind_dn, password).await?.success()?;
 
             let user_dn = self.do_search_user_dn(username, &mut ldap).await;
-- 
2.39.2





^ permalink raw reply	[flat|nested] 18+ messages in thread

* [pbs-devel] [PATCH proxmox 2/4] ldap: add check_connection function
  2023-06-26  9:39 [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Stefan Sterz
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 1/4] ldap: remove support for unauthenticated binds Stefan Sterz
@ 2023-06-26  9:39 ` Stefan Sterz
  2023-06-26 12:23   ` Lukas Wagner
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change Stefan Sterz
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Stefan Sterz @ 2023-06-26  9:39 UTC (permalink / raw)
  To: pbs-devel

this function checks if a given connection could work. it uses the
current config to connect to an ldap directory and perform a search
with the provided base_dn. this enables us to verify a connection
before storing it in a more meaningful way than with a regex.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 proxmox-ldap/src/lib.rs | 50 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/proxmox-ldap/src/lib.rs b/proxmox-ldap/src/lib.rs
index cdc4c9d..c47870d 100644
--- a/proxmox-ldap/src/lib.rs
+++ b/proxmox-ldap/src/lib.rs
@@ -6,7 +6,7 @@ use std::{
     time::Duration,
 };
 
-use anyhow::{bail, format_err, Error};
+use anyhow::{bail, format_err, Context, Error};
 use ldap3::adapters::{Adapter, EntriesOnly, PagedResults};
 use ldap3::{Ldap, LdapConnAsync, LdapConnSettings, LdapResult, Scope, SearchEntry};
 use native_tls::{Certificate, TlsConnector, TlsConnectorBuilder};
@@ -158,6 +158,54 @@ impl Connection {
         Ok(results)
     }
 
+    /// Helper to check if a connection with the current configuration is possible.
+    ///
+    /// This performs a search with the current configuration. If the search succeeds `Ok(()) is
+    /// returned, otherwise an `Error` is returned.
+    pub async fn check_connection(&self) -> Result<(), Error> {
+        let mut ldap = self.create_connection().await?;
+
+        if let Some(bind_dn) = self.config.bind_dn.as_deref() {
+            let password = self
+                .config
+                .bind_password
+                .as_deref()
+                .ok_or_else(|| format_err!("Missing bind password for {bind_dn}"))?;
+
+            let _: LdapResult = ldap
+                .simple_bind(bind_dn, password)
+                .await?
+                .success()
+                .context("LDAP bind failed, bind_dn or password could be incorrect")?;
+
+            let (_, _) = ldap
+                .search(
+                    &self.config.base_dn,
+                    Scope::Subtree,
+                    "(objectClass=*)",
+                    vec!["*"],
+                )
+                .await?
+                .success()
+                .context("Could not search LDAP realm, base_dn could be incorrect")?;
+
+            let _: Result<(), _> = ldap.unbind().await; // ignore errors, search succeeded already
+        } else {
+            let (_, _) = ldap
+                .search(
+                    &self.config.base_dn,
+                    Scope::Subtree,
+                    "(objectClass=*)",
+                    vec!["*"],
+                )
+                .await?
+                .success()
+                .context("Could not search LDAP realm, base_dn could be incorrect")?;
+        }
+
+        Ok(())
+    }
+
     /// Retrive port from LDAP configuration, otherwise use the correct default
     fn port_from_config(&self) -> u16 {
         self.config.port.unwrap_or_else(|| {
-- 
2.39.2





^ permalink raw reply	[flat|nested] 18+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change
  2023-06-26  9:39 [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Stefan Sterz
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 1/4] ldap: remove support for unauthenticated binds Stefan Sterz
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 2/4] ldap: add check_connection function Stefan Sterz
@ 2023-06-26  9:39 ` Stefan Sterz
  2023-06-26 12:36   ` Lukas Wagner
  2023-06-26  9:39 ` [pbs-devel] [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password Stefan Sterz
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Stefan Sterz @ 2023-06-26  9:39 UTC (permalink / raw)
  To: pbs-devel

this commit makes the ldap realm endpoints check whether a new or
updated configuration works correctly. it uses the new
`check_connection` function to make sure that a configuration can be
successfully used to connect to and query an ldap directory.

doing so allows us to significantly relax the ldap domain regex.
instead of relying on a regex to make sure that a given distinguished
name (dn) could be correct, we simply let the ldap directory tell us
whether it accepts it. this should also aid with usability as a dn
that looks correct could still be invalid.

the chosen regex still tries to make sure that what is provided as a
dn resembles one structurally. however, it does not impose strict
limits on what attribute values or types can look like.

this also implicitly removes unauthenticated binds, since the new
`ccheck_connection` function does not support those. it will simply
bail out of the check if a `bind_dn` but no password is configured.
therefore, this is a breaking change.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 pbs-api-types/src/ldap.rs      |  6 +++---
 src/api2/config/access/ldap.rs | 26 +++++++++++++++++++++-----
 src/auth.rs                    | 12 +++++++++++-
 3 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/pbs-api-types/src/ldap.rs b/pbs-api-types/src/ldap.rs
index 762f560a..bc164a06 100644
--- a/pbs-api-types/src/ldap.rs
+++ b/pbs-api-types/src/ldap.rs
@@ -144,15 +144,15 @@ pub enum RemoveVanished {
 
 macro_rules! DOMAIN_PART_REGEX {
     () => {
-        r#"("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^ ,+"/<>;=#])"#
+        r#"[^\s,\+=]+=(?:"[^"]+"|(?:\\[,\+=]|[^,\+=])+)"#
     };
 }
 
 const_regex! {
     pub LDAP_DOMAIN_REGEX = concat!(
-        r#"^\w+="#,
+        r#"^"#,
         DOMAIN_PART_REGEX!(),
-        r#"(,\s*\w+="#,
+        r#"(?:[,\+]\s*"#,
         DOMAIN_PART_REGEX!(),
         ")*$"
     );
diff --git a/src/api2/config/access/ldap.rs b/src/api2/config/access/ldap.rs
index 90cd43c9..911142a0 100644
--- a/src/api2/config/access/ldap.rs
+++ b/src/api2/config/access/ldap.rs
@@ -1,8 +1,10 @@
+use crate::auth::LdapAuthenticator;
 use ::serde::{Deserialize, Serialize};
-use anyhow::Error;
+use anyhow::{format_err, Error};
 use hex::FromHex;
 use serde_json::Value;
 
+use proxmox_ldap::Connection;
 use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
 use proxmox_schema::{api, param_bail};
 
@@ -70,6 +72,11 @@ pub fn create_ldap_realm(config: LdapRealmConfig, password: Option<String>) -> R
         param_bail!("realm", "realm '{}' already exists.", config.realm);
     }
 
+    let ldap_config =
+        LdapAuthenticator::api_type_to_config_with_password(&config, password.clone())?;
+    let conn = Connection::new(ldap_config);
+    proxmox_async::runtime::block_on(conn.check_connection()).map_err(|e| format_err!("{e:#}"))?;
+
     if let Some(password) = password {
         auth_helpers::store_ldap_bind_password(&config.realm, &password, &domain_config_lock)?;
     }
@@ -317,10 +324,6 @@ pub fn update_ldap_realm(
         config.bind_dn = Some(bind_dn);
     }
 
-    if let Some(password) = password {
-        auth_helpers::store_ldap_bind_password(&realm, &password, &domain_config_lock)?;
-    }
-
     if let Some(filter) = update.filter {
         config.filter = Some(filter);
     }
@@ -334,6 +337,19 @@ pub fn update_ldap_realm(
         config.user_classes = Some(user_classes);
     }
 
+    let ldap_config = if let Some(_) = password {
+        LdapAuthenticator::api_type_to_config_with_password(&config, password.clone())?
+    } else {
+        LdapAuthenticator::api_type_to_config(&config)?
+    };
+
+    let conn = Connection::new(ldap_config);
+    proxmox_async::runtime::block_on(conn.check_connection()).map_err(|e| format_err!("{e:#}"))?;
+
+    if let Some(password) = password {
+        auth_helpers::store_ldap_bind_password(&realm, &password, &domain_config_lock)?;
+    }
+
     domains.set_data(&realm, "ldap", &config)?;
 
     domains::save_config(&domains)?;
diff --git a/src/auth.rs b/src/auth.rs
index e8d8d2da..318d1ff2 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -170,6 +170,16 @@ impl Authenticator for LdapAuthenticator {
 
 impl LdapAuthenticator {
     pub fn api_type_to_config(config: &LdapRealmConfig) -> Result<Config, Error> {
+        Self::api_type_to_config_with_password(
+            config,
+            auth_helpers::get_ldap_bind_password(&config.realm)?,
+        )
+    }
+
+    pub fn api_type_to_config_with_password(
+        config: &LdapRealmConfig,
+        password: Option<String>,
+    ) -> Result<Config, Error> {
         let mut servers = vec![config.server1.clone()];
         if let Some(server) = &config.server2 {
             servers.push(server.clone());
@@ -198,7 +208,7 @@ impl LdapAuthenticator {
             user_attr: config.user_attr.clone(),
             base_dn: config.base_dn.clone(),
             bind_dn: config.bind_dn.clone(),
-            bind_password: auth_helpers::get_ldap_bind_password(&config.realm)?,
+            bind_password: password,
             tls_mode,
             verify_certificate: config.verify.unwrap_or_default(),
             additional_trusted_certificates: trusted_cert,
-- 
2.39.2





^ permalink raw reply	[flat|nested] 18+ messages in thread

* [pbs-devel] [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password
  2023-06-26  9:39 [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Stefan Sterz
                   ` (2 preceding siblings ...)
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change Stefan Sterz
@ 2023-06-26  9:39 ` Stefan Sterz
  2023-06-26 13:04   ` [pbs-devel] applied: " Wolfgang Bumiller
  2023-06-26 18:30   ` [pbs-devel] " Thomas Lamprecht
  2023-06-26 12:39 ` [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Lukas Wagner
  2023-06-26 12:46 ` Stefan Hanreich
  5 siblings, 2 replies; 18+ messages in thread
From: Stefan Sterz @ 2023-06-26  9:39 UTC (permalink / raw)
  To: pbs-devel

this commit enforces passwords when using an non-anonymous bind.
hence, it removes the possibility of configuring unauthenticated binds
and brings the gui in-line with the backend.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/window/AuthEditLDAP.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/window/AuthEditLDAP.js b/src/window/AuthEditLDAP.js
index cae7c80..6aafb98 100644
--- a/src/window/AuthEditLDAP.js
+++ b/src/window/AuthEditLDAP.js
@@ -113,9 +113,9 @@ Ext.define('Proxmox.panel.LDAPInputPanel', {
 	    inputType: 'password',
 	    fieldLabel: gettext('Bind Password'),
 	    name: 'password',
-	    allowBlank: true,
 	    cbind: {
 		emptyText: get => !get('isCreate') ? gettext('Unchanged') : '',
+		allowBlank: '{!isCreate}',
 	    },
 	    bind: {
 		disabled: "{anonymous_search}",
-- 
2.39.2





^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox 2/4] ldap: add check_connection function
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 2/4] ldap: add check_connection function Stefan Sterz
@ 2023-06-26 12:23   ` Lukas Wagner
  2023-06-26 12:24     ` Stefan Sterz
  0 siblings, 1 reply; 18+ messages in thread
From: Lukas Wagner @ 2023-06-26 12:23 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Stefan Sterz

Hello,

On 6/26/23 11:39, Stefan Sterz wrote:
> +    /// Helper to check if a connection with the current configuration is possible.
> +    ///
> +    /// This performs a search with the current configuration. If the search succeeds `Ok(()) is
> +    /// returned, otherwise an `Error` is returned.
> +    pub async fn check_connection(&self) -> Result<(), Error> {

The `proxmox-ldap` crate actually has some integration tests, running tests against a
real LDAP server (glauth). For reference, take a look at `tests/glauth.rs` and
`tests/run_integration_tests.sh`. They are ignored by default, since they require the
`glauth` binary in your `PATH`.

It would be cool if you could add some tests for this new function. Can of course happen in a
followup patch! :)


-- 
- Lukas




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox 2/4] ldap: add check_connection function
  2023-06-26 12:23   ` Lukas Wagner
@ 2023-06-26 12:24     ` Stefan Sterz
  2023-06-26 12:57       ` Wolfgang Bumiller
  0 siblings, 1 reply; 18+ messages in thread
From: Stefan Sterz @ 2023-06-26 12:24 UTC (permalink / raw)
  To: Lukas Wagner, Proxmox Backup Server development discussion

On 26.06.23 14:23, Lukas Wagner wrote:
> Hello,
> 
> On 6/26/23 11:39, Stefan Sterz wrote:
>> +    /// Helper to check if a connection with the current
>> configuration is possible.
>> +    ///
>> +    /// This performs a search with the current configuration. If the
>> search succeeds `Ok(()) is
>> +    /// returned, otherwise an `Error` is returned.
>> +    pub async fn check_connection(&self) -> Result<(), Error> {
> 
> The `proxmox-ldap` crate actually has some integration tests, running
> tests against a
> real LDAP server (glauth). For reference, take a look at
> `tests/glauth.rs` and
> `tests/run_integration_tests.sh`. They are ignored by default, since
> they require the
> `glauth` binary in your `PATH`.
> 
> It would be cool if you could add some tests for this new function. Can
> of course happen in a
> followup patch! :)
> 
> 
Ah thanks, yeah i'll look into it, but i wanted to get this out in time
for the next release. the tests probably won't make it in time, hope
that's alright.




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change Stefan Sterz
@ 2023-06-26 12:36   ` Lukas Wagner
  2023-06-26 12:40     ` Stefan Sterz
  0 siblings, 1 reply; 18+ messages in thread
From: Lukas Wagner @ 2023-06-26 12:36 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Stefan Sterz



On 6/26/23 11:39, Stefan Sterz wrote:
>   macro_rules! DOMAIN_PART_REGEX {
>       () => {
> -        r#"("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^ ,+"/<>;=#])"#
> +        r#"[^\s,\+=]+=(?:"[^"]+"|(?:\\[,\+=]|[^,\+=])+)"#
>       };
>   }
>   

I wonder, if we validate any change of the LDAP parameters against the actual server anyway, is there
even any value in validating DNs using a regex?

If the config is manipulated via the API, a malformed DN will be rejected by the server, and in case
the configuration file is edited directly, the regex also does not really help that much.

-- 
- Lukas




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling
  2023-06-26  9:39 [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Stefan Sterz
                   ` (3 preceding siblings ...)
  2023-06-26  9:39 ` [pbs-devel] [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password Stefan Sterz
@ 2023-06-26 12:39 ` Lukas Wagner
  2023-06-26 12:46 ` Stefan Hanreich
  5 siblings, 0 replies; 18+ messages in thread
From: Lukas Wagner @ 2023-06-26 12:39 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Stefan Sterz

Heya,

reviewed these changes and tested them on the respective latest master branches.
Tested against a `glauth` instance running on my local machine.

Seems to work as advertised. Code looks good to me. Some comments were posted
in reply to the individual patches, but nothing that prohibits a merge IMO.

Consider this:

Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

On 6/26/23 11:39, Stefan Sterz wrote:
> this patch series does two things:
> 
> 1. it improves the creation and updating of ldap configurations by
>     checking it against an ldap directory instead of a regex. thus,
>     increasing the likelihood of the configuration being correct
> 2. remove the ability configure and use unauthenticated binds.
>     unauthenticated binds are generally discouraged and we don't
>     support them in proxmox ve either, so remove them in the backup
>     server too.
> 
> removing unauthenticated binds is a breaking change as some users may
> already rely on this functionality.
> 
> Stefan Sterz (1):
>    window: ldap auth edit forbid specifying a bind_dn without a password
> 
>   src/window/AuthEditLDAP.js | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

-- 
- Lukas




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change
  2023-06-26 12:36   ` Lukas Wagner
@ 2023-06-26 12:40     ` Stefan Sterz
  2023-06-26 12:59       ` Wolfgang Bumiller
  0 siblings, 1 reply; 18+ messages in thread
From: Stefan Sterz @ 2023-06-26 12:40 UTC (permalink / raw)
  To: Lukas Wagner, Proxmox Backup Server development discussion

On 26.06.23 14:36, Lukas Wagner wrote:
> 
> 
> On 6/26/23 11:39, Stefan Sterz wrote:
>>   macro_rules! DOMAIN_PART_REGEX {
>>       () => {
>> -        r#"("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^
>> ,+"/<>;=#])"#
>> +        r#"[^\s,\+=]+=(?:"[^"]+"|(?:\\[,\+=]|[^,\+=])+)"#
>>       };
>>   }
>>   
> 
> I wonder, if we validate any change of the LDAP parameters against the
> actual server anyway, is there
> even any value in validating DNs using a regex?
> 

it could be dropped, i just assumed that having it there would help in
cases of obviously wrong dns and would save us the somewhat expensive
round-trip in such cases.

> If the config is manipulated via the API, a malformed DN will be
> rejected by the server, and in case
> the configuration file is edited directly, the regex also does not
> really help that much. 




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling
  2023-06-26  9:39 [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Stefan Sterz
                   ` (4 preceding siblings ...)
  2023-06-26 12:39 ` [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Lukas Wagner
@ 2023-06-26 12:46 ` Stefan Hanreich
  5 siblings, 0 replies; 18+ messages in thread
From: Stefan Hanreich @ 2023-06-26 12:46 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Stefan Sterz

I have also shortly tested this against a slapd instance, worked for 
both base & bind DN on create & edit.

Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>

On 6/26/23 11:39, Stefan Sterz wrote:
> this patch series does two things:
> 
> 1. it improves the creation and updating of ldap configurations by
>     checking it against an ldap directory instead of a regex. thus,
>     increasing the likelihood of the configuration being correct
> 2. remove the ability configure and use unauthenticated binds.
>     unauthenticated binds are generally discouraged and we don't
>     support them in proxmox ve either, so remove them in the backup
>     server too.
> 
> removing unauthenticated binds is a breaking change as some users may
> already rely on this functionality.
> 
> Stefan Sterz (1):
>    window: ldap auth edit forbid specifying a bind_dn without a password
> 
>   src/window/AuthEditLDAP.js | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox 2/4] ldap: add check_connection function
  2023-06-26 12:24     ` Stefan Sterz
@ 2023-06-26 12:57       ` Wolfgang Bumiller
  0 siblings, 0 replies; 18+ messages in thread
From: Wolfgang Bumiller @ 2023-06-26 12:57 UTC (permalink / raw)
  To: Stefan Sterz; +Cc: Lukas Wagner, Proxmox Backup Server development discussion

On Mon, Jun 26, 2023 at 02:24:40PM +0200, Stefan Sterz wrote:
> On 26.06.23 14:23, Lukas Wagner wrote:
> > Hello,
> > 
> > On 6/26/23 11:39, Stefan Sterz wrote:
> >> +    /// Helper to check if a connection with the current
> >> configuration is possible.
> >> +    ///
> >> +    /// This performs a search with the current configuration. If the
> >> search succeeds `Ok(()) is
> >> +    /// returned, otherwise an `Error` is returned.
> >> +    pub async fn check_connection(&self) -> Result<(), Error> {
> > 
> > The `proxmox-ldap` crate actually has some integration tests, running
> > tests against a
> > real LDAP server (glauth). For reference, take a look at
> > `tests/glauth.rs` and
> > `tests/run_integration_tests.sh`. They are ignored by default, since
> > they require the
> > `glauth` binary in your `PATH`.
> > 
> > It would be cool if you could add some tests for this new function. Can
> > of course happen in a
> > followup patch! :)
> > 
> > 
> Ah thanks, yeah i'll look into it, but i wanted to get this out in time
> for the next release. the tests probably won't make it in time, hope
> that's alright.

It's fine as a followup later.




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change
  2023-06-26 12:40     ` Stefan Sterz
@ 2023-06-26 12:59       ` Wolfgang Bumiller
  2023-06-26 13:17         ` Stefan Sterz
  0 siblings, 1 reply; 18+ messages in thread
From: Wolfgang Bumiller @ 2023-06-26 12:59 UTC (permalink / raw)
  To: Stefan Sterz; +Cc: Lukas Wagner, Proxmox Backup Server development discussion

On Mon, Jun 26, 2023 at 02:40:38PM +0200, Stefan Sterz wrote:
> On 26.06.23 14:36, Lukas Wagner wrote:
> > 
> > 
> > On 6/26/23 11:39, Stefan Sterz wrote:
> >>   macro_rules! DOMAIN_PART_REGEX {
> >>       () => {
> >> -        r#"("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^
> >> ,+"/<>;=#])"#
> >> +        r#"[^\s,\+=]+=(?:"[^"]+"|(?:\\[,\+=]|[^,\+=])+)"#
> >>       };
> >>   }
> >>   
> > 
> > I wonder, if we validate any change of the LDAP parameters against the
> > actual server anyway, is there
> > even any value in validating DNs using a regex?
> > 
> 
> it could be dropped, i just assumed that having it there would help in
> cases of obviously wrong dns and would save us the somewhat expensive
> round-trip in such cases.

I guess we can just drop it.




^ permalink raw reply	[flat|nested] 18+ messages in thread

* [pbs-devel] applied: [PATCH proxmox 1/4] ldap: remove support for unauthenticated binds
  2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 1/4] ldap: remove support for unauthenticated binds Stefan Sterz
@ 2023-06-26 13:00   ` Wolfgang Bumiller
  0 siblings, 0 replies; 18+ messages in thread
From: Wolfgang Bumiller @ 2023-06-26 13:00 UTC (permalink / raw)
  To: Stefan Sterz; +Cc: pbs-devel

applied 1/4 and 2/4




^ permalink raw reply	[flat|nested] 18+ messages in thread

* [pbs-devel] applied: [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password
  2023-06-26  9:39 ` [pbs-devel] [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password Stefan Sterz
@ 2023-06-26 13:04   ` Wolfgang Bumiller
  2023-06-26 18:30   ` [pbs-devel] " Thomas Lamprecht
  1 sibling, 0 replies; 18+ messages in thread
From: Wolfgang Bumiller @ 2023-06-26 13:04 UTC (permalink / raw)
  To: Stefan Sterz; +Cc: pbs-devel

applied




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change
  2023-06-26 12:59       ` Wolfgang Bumiller
@ 2023-06-26 13:17         ` Stefan Sterz
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2023-06-26 13:17 UTC (permalink / raw)
  To: Wolfgang Bumiller
  Cc: Lukas Wagner, Proxmox Backup Server development discussion

On 26.06.23 14:59, Wolfgang Bumiller wrote:
> On Mon, Jun 26, 2023 at 02:40:38PM +0200, Stefan Sterz wrote:
>> On 26.06.23 14:36, Lukas Wagner wrote:
>>>
>>>
>>> On 6/26/23 11:39, Stefan Sterz wrote:
>>>>   macro_rules! DOMAIN_PART_REGEX {
>>>>       () => {
>>>> -        r#"("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^
>>>> ,+"/<>;=#])"#
>>>> +        r#"[^\s,\+=]+=(?:"[^"]+"|(?:\\[,\+=]|[^,\+=])+)"#
>>>>       };
>>>>   }
>>>>   
>>>
>>> I wonder, if we validate any change of the LDAP parameters against the
>>> actual server anyway, is there
>>> even any value in validating DNs using a regex?
>>>
>>
>> it could be dropped, i just assumed that having it there would help in
>> cases of obviously wrong dns and would save us the somewhat expensive
>> round-trip in such cases.
> 
> I guess we can just drop it.

alright, i'll send a v2 with that dropped in a minute.




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password
  2023-06-26  9:39 ` [pbs-devel] [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password Stefan Sterz
  2023-06-26 13:04   ` [pbs-devel] applied: " Wolfgang Bumiller
@ 2023-06-26 18:30   ` Thomas Lamprecht
  2023-06-27  7:23     ` Stefan Sterz
  1 sibling, 1 reply; 18+ messages in thread
From: Thomas Lamprecht @ 2023-06-26 18:30 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Stefan Sterz

Am 26/06/2023 um 11:39 schrieb Stefan Sterz:
> this commit enforces passwords when using an non-anonymous bind.
> hence, it removes the possibility of configuring unauthenticated binds
> and brings the gui in-line with the backend.
> 

nit: please don't base the commit subject tags strictly on file hierarchy, for
copying this over to the changelog the following would be IMO a bit nicer:

> ldap realm edit: forbid specifying a bind_dn without a password


More importantly, albeit just to be sure: this doesn't clashes with PVE or PMG as
it's either not used there, and/or would be already compatible anyway (like you
mentioned PVE in the cover letter)?




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pbs-devel] [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password
  2023-06-26 18:30   ` [pbs-devel] " Thomas Lamprecht
@ 2023-06-27  7:23     ` Stefan Sterz
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2023-06-27  7:23 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox Backup Server development discussion

On 26.06.23 20:30, Thomas Lamprecht wrote:
> Am 26/06/2023 um 11:39 schrieb Stefan Sterz:
>> this commit enforces passwords when using an non-anonymous bind.
>> hence, it removes the possibility of configuring unauthenticated binds
>> and brings the gui in-line with the backend.
>>
> 
> nit: please don't base the commit subject tags strictly on file hierarchy, for
> copying this over to the changelog the following would be IMO a bit nicer:
> 
>> ldap realm edit: forbid specifying a bind_dn without a password
> 

sorry, i'll try to keep that in mind.

> More importantly, albeit just to be sure: this doesn't clashes with PVE or PMG as
> it's either not used there, and/or would be already compatible anyway (like you
> mentioned PVE in the cover letter)?

so in pve you can configure this. however, it will fail as soon as the
configuration is actually used [1] (e.g., for a sync). i'm already
working on a patch that also make the gui enforce setting a password in
such cases.

pmg from what i can tell allows unauthenticated binds just like pbs did
previously.

[1]:
https://git.proxmox.com/?p=pve-access-control.git;a=blob;f=src/PVE/Auth/LDAP.pm;h=fc82a17a#l219




^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-06-27  7:23 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-26  9:39 [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Stefan Sterz
2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 1/4] ldap: remove support for unauthenticated binds Stefan Sterz
2023-06-26 13:00   ` [pbs-devel] applied: " Wolfgang Bumiller
2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox 2/4] ldap: add check_connection function Stefan Sterz
2023-06-26 12:23   ` Lukas Wagner
2023-06-26 12:24     ` Stefan Sterz
2023-06-26 12:57       ` Wolfgang Bumiller
2023-06-26  9:39 ` [pbs-devel] [PATCH proxmox-backup 3/4] access: ldap check connection on creation and change Stefan Sterz
2023-06-26 12:36   ` Lukas Wagner
2023-06-26 12:40     ` Stefan Sterz
2023-06-26 12:59       ` Wolfgang Bumiller
2023-06-26 13:17         ` Stefan Sterz
2023-06-26  9:39 ` [pbs-devel] [PATCH widget-toolkit 4/4] window: ldap auth edit forbid specifying a bind_dn without a password Stefan Sterz
2023-06-26 13:04   ` [pbs-devel] applied: " Wolfgang Bumiller
2023-06-26 18:30   ` [pbs-devel] " Thomas Lamprecht
2023-06-27  7:23     ` Stefan Sterz
2023-06-26 12:39 ` [pbs-devel] [PATCH proxmox(-backup), widget-toolkit 0/4] improve ldap configuration handling Lukas Wagner
2023-06-26 12:46 ` Stefan Hanreich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal