[pve-devel] [PATCH common/access-control/manager v2 0/3] ldap: check bind connection on realm add/update

[pve-devel] [PATCH common/access-control/manager v2 0/3] ldap: check bind connection on realm add/update

[Christoph Heiss]

First of, remove the dreaded LDAP DN regex.

Further, upon saving a LDAP realm in the UI, it tries to connect & bind
using the provided credentials, providing the user with immediate
feedback whether they are valid or not.

The same approach is already implemented in PBS [0], and I'll plan to
implement the same for PMG too, if & when the PVE side is done.

Testing
-------
Changes were tested against slapd 2.5.13+dfsg-5 (for LDAP) and Samba
4.18.5 (for AD), using both the web UI and `pveum` to create and update
realms with different combinations of valid and invalid parameters,
mixed with using new `check-connection` parameter.

Prior art
---------
v1: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058551.html

Notable changes v1 -> v2:
  * Added patch #1 from previous series [1], missed that in v1
  * Do not store the 'check-connection' parameter in the realm config
  * Add "Check connection" checkbox to AD edit too

This series supersedes [1], which previously solved this using a new
schema format by validating DNs using Net::LDAP::Util::canonical_dn().
But this has the problem that it does not support AD-specific DN syntax.

After a off-list discussion with Lukas (summary [2] [3]), it was decided to
rather implement it much like PBS does it - simply drop the explicit
validation of DN parameters, instead just trying to connect & bind to
the target server - although I'm always open for other/better
suggestions to tackle this.

[0] https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=5210f3b5
[1] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html
[2] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html
[3] https://lists.proxmox.com/pipermail/pve-devel/2023-August/058582.html

--
2.41.0

[pve-devel] [PATCH common v2 1/3] ldap: handle errors explicitly everywhere instead of simply `die`ing

[Christoph Heiss]

Most codepaths already have explicit error handling (by the means of
checking the return value), which is essential dead code due to setting
`onerror`.

As LDAP errors might get presented to users due to upcoming changes, the
error location should not be present in these error messages, thus
switch to explicit handling.

Only two calls were missing such explicit handling of errors, so these
are amended as appropriate. Further, some `die`s were missing newlines
at the end of the message, which - again - would cause the error
location to be included.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * New patch; from previous series [0], as it is also needed here

[0] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html

 src/PVE/LDAP.pm | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/PVE/LDAP.pm b/src/PVE/LDAP.pm
index 342c352..16a0a8e 100644
--- a/src/PVE/LDAP.pm
+++ b/src/PVE/LDAP.pm
@@ -22,7 +22,6 @@ sub ldap_connect {
 	scheme => $scheme,
 	port => $port,
 	timeout => 10,
-	onerror => 'die',
     );

     my $hosts = [];
@@ -41,7 +40,8 @@ sub ldap_connect {
     my $ldap = Net::LDAP->new($hosts, %ldap_opts) || die "$@\n";

     if ($start_tls) {
-	$ldap->start_tls(%$opts);
+	my $res = $ldap->start_tls(%$opts);
+	die $res->error . "\n" if $res->code;
     }

     return $ldap;
@@ -73,6 +73,7 @@ sub get_user_dn {
 	filter  => "$attr=$name",
 	attrs   => ['dn']
     );
+    die $result->error . "\n" if $result->code;
     return undef if !$result->entries;
     my @entries = $result->entries;
     return $entries[0]->dn;
@@ -93,7 +94,7 @@ sub auth_user_dn {

     if ($code) {
 	return undef if $noerr;
-	die $err;
+	die "$err\n";
     }

     return 1;
@@ -184,7 +185,7 @@ sub query_users {
 	$err = "LDAP user query unsuccessful" if !$err;
     }

-    die $err if $err;
+    die "$err\n" if $err;

     return $users;
 }
@@ -265,7 +266,7 @@ sub query_groups {
 	$err = "LDAP group query unsuccessful" if !$err;
     }

-    die $err if $err;
+    die "$err\n" if $err;

     return $groups;
 }
--
2.41.0

[pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check

[Christoph Heiss]

Removes the dreaded DN regex, instead introducing a optional
connect/bind check on creation/update, aligning it with the way PBS does
it.

Additionally, it has the benefit that instead of letting a sync fail on
the first try due to e.g. bad bind credentials, it gives the user some
direct feedback when trying to add/update a LDAP realm, if enabled.

Should be rather a corner case, but it's the easiest way for us to
accomodate and the most versatile for users needing this.

This is part of the result of a previous discussion [0], and the same
approach is already implemented for PBS [1].

[0] https://lists.proxmox.com/pipermail/pve-devel/2023-May/056839.html
[1] https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=5210f3b5

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * Do not store the 'check-connection' parameter in the realm config
  * Small fix; pass full config to check_connection() on update

The decision to put this behind an optional (off-by-default) parameter
was made during an off-list discussion with Lukas. The summary of that
can be found as a follow-up on the previous series [2] [3].

Note that the `base_dn`/`group_dn` now goes completely unvalided - but
again, that is how it works in PBS. We /could/ use canonical_dn() for
this (see previous series [4]), depending on whether we want this kind
of "divergence" between PVE & PBS.

[2] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html
[3] https://lists.proxmox.com/pipermail/pve-devel/2023-August/058582.html
[4] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html

 src/PVE/API2/Domains.pm | 10 ++++++++++
 src/PVE/Auth/LDAP.pm    | 25 ++++++++++++++++---------
 src/PVE/Auth/Plugin.pm  |  8 ++++++++
 3 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 9332aa7..ed2b6c4 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -133,6 +133,7 @@ __PACKAGE__->register_method ({

 		my $realm = extract_param($param, 'realm');
 		my $type = $param->{type};
+		my $check_connection = extract_param($param, 'check-connection');

 		die "domain '$realm' already exists\n"
 		    if $ids->{$realm};
@@ -165,6 +166,10 @@ __PACKAGE__->register_method ({
 		}
 		$plugin->on_add_hook($realm, $config, password => $password);

+		# Only for LDAP/AD, implied through the existence of the 'check-connection' param
+		$plugin->check_connection($realm, $config, password => $password)
+		    if $check_connection;
+
 		cfs_write_file($domainconfigfile, $cfg);
 	    }, "add auth server failed");

@@ -198,6 +203,7 @@ __PACKAGE__->register_method ({
 		PVE::SectionConfig::assert_if_modified($cfg, $digest);

 		my $realm = extract_param($param, 'realm');
+		my $check_connection = extract_param($param, 'check-connection');

 		die "domain '$realm' does not exist\n"
 		    if !$ids->{$realm};
@@ -237,6 +243,10 @@ __PACKAGE__->register_method ({
 		    $plugin->on_update_hook($realm, $config);
 		}

+		# Only for LDAP/AD, implied through the existence of the 'check-connection' param
+		$plugin->check_connection($realm, $ids->{$realm}, password => $password)
+		    if $check_connection;
+
 		cfs_write_file($domainconfigfile, $cfg);
 	    }, "update auth server failed");

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index fc82a17..d5f4909 100755
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -10,9 +10,6 @@ use PVE::Tools;

 use base qw(PVE::Auth::Plugin);

-my  $dn_part_regex = qr!("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^ ,+"/<>;=#])!;
-our $dn_regex = qr!\w+=${dn_part_regex}(,\s*\w+=${dn_part_regex})*!;
-
 sub type {
     return 'ldap';
 }
@@ -22,7 +19,6 @@ sub properties {
 	base_dn => {
 	    description => "LDAP base domain name",
 	    type => 'string',
-	    pattern => $dn_regex,
 	    optional => 1,
 	    maxLength => 256,
 	},
@@ -36,7 +32,6 @@ sub properties {
 	bind_dn => {
 	    description => "LDAP bind domain name",
 	    type => 'string',
-	    pattern => $dn_regex,
 	    optional => 1,
 	    maxLength => 256,
 	},
@@ -94,7 +89,6 @@ sub properties {
 	    description => "LDAP base domain name for group sync. If not set, the"
 		." base_dn will be used.",
 	    type => 'string',
-	    pattern => $dn_regex,
 	    optional => 1,
 	    maxLength => 256,
 	},
@@ -137,7 +131,13 @@ sub properties {
 	    type => 'boolean',
 	    optional => 1,
 	    default => 1,
-	}
+	},
+	'check-connection' => {
+	    description => 'Check bind connection to LDAP server.',
+	    type => 'boolean',
+	    optional => 1,
+	    default => 0,
+	},
     };
 }

@@ -169,6 +169,7 @@ sub options {
 	'sync-defaults-options' => { optional => 1 },
 	mode => { optional => 1 },
 	'case-sensitive' => { optional => 1 },
+	'check-connection' => { optional => 1 },
     };
 }

@@ -184,7 +185,7 @@ sub get_scheme_and_port {
 }

 sub connect_and_bind {
-    my ($class, $config, $realm) = @_;
+    my ($class, $config, $realm, $param) = @_;

     my $servers = [$config->{server1}];
     push @$servers, $config->{server2} if $config->{server2};
@@ -215,7 +216,7 @@ sub connect_and_bind {

     if ($config->{bind_dn}) {
 	my $bind_dn = $config->{bind_dn};
-	my $bind_pass = ldap_get_credentials($realm);
+	my $bind_pass = $param->{password} || ldap_get_credentials($realm);
 	die "missing password for realm $realm\n" if !defined($bind_pass);
 	PVE::LDAP::ldap_bind($ldap, $bind_dn, $bind_pass);
     } elsif ($config->{cert} && $config->{certkey}) {
@@ -454,4 +455,10 @@ sub on_delete_hook {
     ldap_delete_credentials($realm);
 }

+sub check_connection {
+    my ($class, $realm, $config, %param) = @_;
+
+    $class->connect_and_bind($config, $realm, \%param);
+}
+
 1;
diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
index 2f64a13..429bc88 100755
--- a/src/PVE/Auth/Plugin.pm
+++ b/src/PVE/Auth/Plugin.pm
@@ -317,4 +317,12 @@ sub on_delete_hook {
     # do nothing by default
 }

+# called during addition and updates of realms (before the new domain config gets written)
+# die to abort addition/update in case the connection/bind fails
+# NOTE: runs in a storage config *locked* context
+sub check_connection {
+    my ($class, $realm, $config, %param) = @_;
+    # do nothing by default
+}
+
 1;
--
2.41.0

[pve-devel] [PATCH manager v2 3/3] ui: ldap: add 'Check connection' checkbox as advanced option

[Christoph Heiss]

The checkbox is enabled by default, setting the new `check-connection`
parameter. See also [0] for the rationale.

[0] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058559.html

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * Add "Check connection" checkbox to AD edit too

As this uses the newly introduced `check-connection` API parameter, a
bump on libpve-access-control is needed.

 www/manager6/dc/AuthEditAD.js   | 15 +++++++++++++++
 www/manager6/dc/AuthEditLDAP.js | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/www/manager6/dc/AuthEditAD.js b/www/manager6/dc/AuthEditAD.js
index a1999cb74..3cbb47c9f 100644
--- a/www/manager6/dc/AuthEditAD.js
+++ b/www/manager6/dc/AuthEditAD.js
@@ -79,6 +79,21 @@ Ext.define('PVE.panel.ADInputPanel', {
 	    },
 	];

+	me.advancedItems = [
+	    {
+		xtype: 'proxmoxcheckbox',
+		fieldLabel: gettext('Check connection'),
+		name: 'check-connection',
+		uncheckedValue: 0,
+		checked: true,
+		autoEl: {
+		    tag: 'div',
+		    'data-qtip':
+			gettext('Verify connection parameters and bind credentials on save'),
+		},
+	    },
+	];
+
 	me.callParent();
     },
     onGetValues: function(values) {
diff --git a/www/manager6/dc/AuthEditLDAP.js b/www/manager6/dc/AuthEditLDAP.js
index 2ce16e58c..9986db8a9 100644
--- a/www/manager6/dc/AuthEditLDAP.js
+++ b/www/manager6/dc/AuthEditLDAP.js
@@ -79,6 +79,21 @@ Ext.define('PVE.panel.LDAPInputPanel', {
 	    },
 	];

+	me.advancedItems = [
+	    {
+		xtype: 'proxmoxcheckbox',
+		fieldLabel: gettext('Check connection'),
+		name: 'check-connection',
+		uncheckedValue: 0,
+		checked: true,
+		autoEl: {
+		    tag: 'div',
+		    'data-qtip':
+			gettext('Verify connection parameters and bind credentials on save'),
+		},
+	    },
+	];
+
 	me.callParent();
     },
     onGetValues: function(values) {
--
2.41.0

Re: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check

[Wolfgang Bumiller]

On Tue, Aug 01, 2023 at 02:37:18PM +0200, Christoph Heiss wrote:
> Removes the dreaded DN regex, instead introducing a optional
> connect/bind check on creation/update, aligning it with the way PBS does
> it.
> 
> Additionally, it has the benefit that instead of letting a sync fail on
> the first try due to e.g. bad bind credentials, it gives the user some
> direct feedback when trying to add/update a LDAP realm, if enabled.
> 
> Should be rather a corner case, but it's the easiest way for us to
> accomodate and the most versatile for users needing this.
> 
> This is part of the result of a previous discussion [0], and the same
> approach is already implemented for PBS [1].
> 
> [0] https://lists.proxmox.com/pipermail/pve-devel/2023-May/056839.html
> [1] https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=5210f3b5
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Changes v1 -> v2:
>   * Do not store the 'check-connection' parameter in the realm config
>   * Small fix; pass full config to check_connection() on update
> 
> The decision to put this behind an optional (off-by-default) parameter
> was made during an off-list discussion with Lukas. The summary of that
> can be found as a follow-up on the previous series [2] [3].
> 
> Note that the `base_dn`/`group_dn` now goes completely unvalided - but
> again, that is how it works in PBS. We /could/ use canonical_dn() for
> this (see previous series [4]), depending on whether we want this kind
> of "divergence" between PVE & PBS.
> 
> [2] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html
> [3] https://lists.proxmox.com/pipermail/pve-devel/2023-August/058582.html
> [4] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html
> 
>  src/PVE/API2/Domains.pm | 10 ++++++++++
>  src/PVE/Auth/LDAP.pm    | 25 ++++++++++++++++---------
>  src/PVE/Auth/Plugin.pm  |  8 ++++++++
>  3 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
> index 9332aa7..ed2b6c4 100644
> --- a/src/PVE/API2/Domains.pm
> +++ b/src/PVE/API2/Domains.pm
> @@ -133,6 +133,7 @@ __PACKAGE__->register_method ({
> 
>  		my $realm = extract_param($param, 'realm');
>  		my $type = $param->{type};
> +		my $check_connection = extract_param($param, 'check-connection');
> 
>  		die "domain '$realm' already exists\n"
>  		    if $ids->{$realm};
> @@ -165,6 +166,10 @@ __PACKAGE__->register_method ({
>  		}
>  		$plugin->on_add_hook($realm, $config, password => $password);
> 
> +		# Only for LDAP/AD, implied through the existence of the 'check-connection' param
> +		$plugin->check_connection($realm, $config, password => $password)
> +		    if $check_connection;
> +
>  		cfs_write_file($domainconfigfile, $cfg);
>  	    }, "add auth server failed");
> 
> @@ -198,6 +203,7 @@ __PACKAGE__->register_method ({
>  		PVE::SectionConfig::assert_if_modified($cfg, $digest);
> 
>  		my $realm = extract_param($param, 'realm');
> +		my $check_connection = extract_param($param, 'check-connection');
> 
>  		die "domain '$realm' does not exist\n"
>  		    if !$ids->{$realm};
> @@ -237,6 +243,10 @@ __PACKAGE__->register_method ({
>  		    $plugin->on_update_hook($realm, $config);
>  		}
> 
> +		# Only for LDAP/AD, implied through the existence of the 'check-connection' param
> +		$plugin->check_connection($realm, $ids->{$realm}, password => $password)
> +		    if $check_connection;
> +
>  		cfs_write_file($domainconfigfile, $cfg);
>  	    }, "update auth server failed");
> 
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index fc82a17..d5f4909 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -10,9 +10,6 @@ use PVE::Tools;
> 
>  use base qw(PVE::Auth::Plugin);
> 
> -my  $dn_part_regex = qr!("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^ ,+"/<>;=#])!;
> -our $dn_regex = qr!\w+=${dn_part_regex}(,\s*\w+=${dn_part_regex})*!;
> -
>  sub type {
>      return 'ldap';
>  }
> @@ -22,7 +19,6 @@ sub properties {
>  	base_dn => {
>  	    description => "LDAP base domain name",
>  	    type => 'string',
> -	    pattern => $dn_regex,
>  	    optional => 1,
>  	    maxLength => 256,
>  	},
> @@ -36,7 +32,6 @@ sub properties {
>  	bind_dn => {
>  	    description => "LDAP bind domain name",
>  	    type => 'string',
> -	    pattern => $dn_regex,
>  	    optional => 1,
>  	    maxLength => 256,
>  	},
> @@ -94,7 +89,6 @@ sub properties {
>  	    description => "LDAP base domain name for group sync. If not set, the"
>  		." base_dn will be used.",
>  	    type => 'string',
> -	    pattern => $dn_regex,
>  	    optional => 1,
>  	    maxLength => 256,
>  	},
> @@ -137,7 +131,13 @@ sub properties {
>  	    type => 'boolean',
>  	    optional => 1,
>  	    default => 1,
> -	}
> +	},
> +	'check-connection' => {
> +	    description => 'Check bind connection to LDAP server.',
> +	    type => 'boolean',
> +	    optional => 1,
> +	    default => 0,
> +	},

While there's special handling for how we store the password, this
schema here should still actually describe the stored config.
Since this is a parameter specifically for the add/update API methods we
should declare it in those functions as parameter.

Some of our methods to get schemas have an optional hash parameter to
include an extra set of base properties in its returned contents (see
`get_standard_option` as an example), but `createSchema` and
`updateSchema` do not.

We could either add this, or, since this is currently only required
once, just move the `{create,update}Schema` calls over the
`register_method()` calls and modify them right there before use...
Since this series already touches pve-common, I have a *slight*
preference to extending the `create/updateSchema` subs in
`PVE::SectionConfig`, although AFAICT the common patch does not strictly
require a dependency bump inside pve-access-control as it mostly about
how errors are presented to end-users (?), so either way is fine with
me. If we update the SectionConfig we'll definitely need a versioned
dependency bump.

>      };
>  }
> 
> @@ -169,6 +169,7 @@ sub options {
>  	'sync-defaults-options' => { optional => 1 },
>  	mode => { optional => 1 },
>  	'case-sensitive' => { optional => 1 },
> +	'check-connection' => { optional => 1 },
>      };
>  }
> 
> @@ -184,7 +185,7 @@ sub get_scheme_and_port {
>  }
> 
>  sub connect_and_bind {
> -    my ($class, $config, $realm) = @_;
> +    my ($class, $config, $realm, $param) = @_;
> 
>      my $servers = [$config->{server1}];
>      push @$servers, $config->{server2} if $config->{server2};
> @@ -215,7 +216,7 @@ sub connect_and_bind {
> 
>      if ($config->{bind_dn}) {
>  	my $bind_dn = $config->{bind_dn};
> -	my $bind_pass = ldap_get_credentials($realm);
> +	my $bind_pass = $param->{password} || ldap_get_credentials($realm);
>  	die "missing password for realm $realm\n" if !defined($bind_pass);
>  	PVE::LDAP::ldap_bind($ldap, $bind_dn, $bind_pass);
>      } elsif ($config->{cert} && $config->{certkey}) {
> @@ -454,4 +455,10 @@ sub on_delete_hook {
>      ldap_delete_credentials($realm);
>  }
> 
> +sub check_connection {
> +    my ($class, $realm, $config, %param) = @_;
> +
> +    $class->connect_and_bind($config, $realm, \%param);
> +}
> +
>  1;
> diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm
> index 2f64a13..429bc88 100755
> --- a/src/PVE/Auth/Plugin.pm
> +++ b/src/PVE/Auth/Plugin.pm
> @@ -317,4 +317,12 @@ sub on_delete_hook {
>      # do nothing by default
>  }
> 
> +# called during addition and updates of realms (before the new domain config gets written)
> +# die to abort addition/update in case the connection/bind fails
> +# NOTE: runs in a storage config *locked* context
> +sub check_connection {
> +    my ($class, $realm, $config, %param) = @_;
> +    # do nothing by default
> +}
> +
>  1;
> --
> 2.41.0

Re: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check

[Christoph Heiss]

On Thu, Aug 10, 2023 at 09:55:51AM +0200, Wolfgang Bumiller wrote:
> On Tue, Aug 01, 2023 at 02:37:18PM +0200, Christoph Heiss wrote:
[..]
> > @@ -137,7 +131,13 @@ sub properties {
> >  	    type => 'boolean',
> >  	    optional => 1,
> >  	    default => 1,
> > -	}
> > +	},
> > +	'check-connection' => {
> > +	    description => 'Check bind connection to LDAP server.',
> > +	    type => 'boolean',
> > +	    optional => 1,
> > +	    default => 0,
> > +	},
>
> While there's special handling for how we store the password, this
> schema here should still actually describe the stored config.
> Since this is a parameter specifically for the add/update API methods we
> should declare it in those functions as parameter.
>
> Some of our methods to get schemas have an optional hash parameter to
> include an extra set of base properties in its returned contents (see
> `get_standard_option` as an example), but `createSchema` and
> `updateSchema` do not.
Right, I was unsure anyway if this was the right way anyway to add this,
at least I did not see any other way - that explains why :^)

>
> We could either add this, or, since this is currently only required
> once, just move the `{create,update}Schema` calls over the
> `register_method()` calls and modify them right there before use...
> Since this series already touches pve-common, I have a *slight*
> preference to extending the `create/updateSchema` subs in
> `PVE::SectionConfig`,
Seems like the right thing - I'd also rather do it properly once than to
introduce a hack that sticks around ..

> although AFAICT the common patch does not strictly
> require a dependency bump inside pve-access-control as it mostly about
> how errors are presented to end-users (?), so either way is fine with
Exactly, the changes in pve-common are purely cosmectic.

> me. If we update the SectionConfig we'll definitely need a versioned
> dependency bump.
If it's OK for you I will go this route, extending
{create,update}Schema() as needed for this, in the same way
get_standard_option() works.

Re: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check

[Wolfgang Bumiller]

On Thu, Aug 10, 2023 at 10:35:14AM +0200, Christoph Heiss wrote:
> 
> On Thu, Aug 10, 2023 at 09:55:51AM +0200, Wolfgang Bumiller wrote:
> > On Tue, Aug 01, 2023 at 02:37:18PM +0200, Christoph Heiss wrote:
> [..]
> > > @@ -137,7 +131,13 @@ sub properties {
> > >  	    type => 'boolean',
> > >  	    optional => 1,
> > >  	    default => 1,
> > > -	}
> > > +	},
> > > +	'check-connection' => {
> > > +	    description => 'Check bind connection to LDAP server.',
> > > +	    type => 'boolean',
> > > +	    optional => 1,
> > > +	    default => 0,
> > > +	},
> >
> > While there's special handling for how we store the password, this
> > schema here should still actually describe the stored config.
> > Since this is a parameter specifically for the add/update API methods we
> > should declare it in those functions as parameter.
> >
> > Some of our methods to get schemas have an optional hash parameter to
> > include an extra set of base properties in its returned contents (see
> > `get_standard_option` as an example), but `createSchema` and
> > `updateSchema` do not.
> Right, I was unsure anyway if this was the right way anyway to add this,
> at least I did not see any other way - that explains why :^)
> 
> >
> > We could either add this, or, since this is currently only required
> > once, just move the `{create,update}Schema` calls over the
> > `register_method()` calls and modify them right there before use...
> > Since this series already touches pve-common, I have a *slight*
> > preference to extending the `create/updateSchema` subs in
> > `PVE::SectionConfig`,
> Seems like the right thing - I'd also rather do it properly once than to
> introduce a hack that sticks around ..
> 
> > although AFAICT the common patch does not strictly
> > require a dependency bump inside pve-access-control as it mostly about
> > how errors are presented to end-users (?), so either way is fine with
> Exactly, the changes in pve-common are purely cosmectic.
> 
> > me. If we update the SectionConfig we'll definitely need a versioned
> > dependency bump.
> If it's OK for you I will go this route, extending
> {create,update}Schema() as needed for this, in the same way
> get_standard_option() works.

Sure, just be aware that `get_standard_option()` only needs 1 level,
whereas for the section config ones it works one level above, as in
it returns a full schema like:

  {
    type => 'object',
    properties => { ... }
  }

It's unlikely that we'll need to modify anything at the top level, so I
think it would be fine for the extra parameter to only affect the
contained properties hash, so no need to do any multi-level merging.