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

[pve-devel] [PATCH common/access-control/manager v3 0/4] 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), Samba 4.18.5
and Windows Server 2022 (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
---------
v2: https://lists.proxmox.com/pipermail/pve-devel/2023-August/058586.html
v1: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058551.html

Notable changes v2 -> v3:
  * Move 'check-connection' param out of schema to extra param
    (thanks Wolfgang!)

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.

[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

pve-common:

Christoph Heiss (2):
  ldap: handle errors explicitly everywhere instead of simply `die`ing
  section config: allow base properties for {create,update}Schema()

 src/PVE/LDAP.pm          | 11 ++++++-----
 src/PVE/SectionConfig.pm |  8 ++++----
 2 files changed, 10 insertions(+), 9 deletions(-)

pve-access-control:

Christoph Heiss (1):
  ldap: add opt-in `check-connection` param to perform a bind check

 src/PVE/API2/Domains.pm | 36 +++++++++++++++++++++++++++++++++---
 src/PVE/Auth/LDAP.pm    | 18 +++++++++---------
 src/PVE/Auth/Plugin.pm  |  8 ++++++++
 3 files changed, 50 insertions(+), 12 deletions(-)

pve-manager:

Christoph Heiss (1):
  ui: ldap: add 'Check connection' checkbox as advanced option

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

--
2.41.0

[pve-devel] [PATCH common v3 1/4] 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 v2 -> v3:
  * No changes

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 common v3 2/4] section config: allow base properties for {create, update}Schema()

[Christoph Heiss]

This works the same way as e.g. get_standard_option does it.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
  * New patch; as suggested by Wolfgang

 src/PVE/SectionConfig.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index 97492db..2b75e6a 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -52,13 +52,13 @@ sub plugindata {
 }

 sub createSchema {
-    my ($class, $skip_type) = @_;
+    my ($class, $skip_type, $base) = @_;

     my $pdata = $class->private();
     my $propertyList = $pdata->{propertyList};
     my $plugins = $pdata->{plugins};

-    my $props = {};
+    my $props = $base || {};

     my $copy_property = sub {
 	my ($src) = @_;
@@ -107,13 +107,13 @@ sub createSchema {
 }

 sub updateSchema {
-    my ($class, $single_class) = @_;
+    my ($class, $single_class, $base) = @_;

     my $pdata = $class->private();
     my $propertyList = $pdata->{propertyList};
     my $plugins = $pdata->{plugins};

-    my $props = {};
+    my $props = $base || {};

     my $filter_type = $single_class ? $class->type() : undef;

--
2.41.0

[pve-devel] [PATCH access-control v3 3/4] 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://lists.proxmox.com/pipermail/pbs-devel/2023-June/006237.html

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
N.B.: Needs a version bump of libpve-common-perl for the base properties
parameter on {create,update}Schema().

Changes v2 -> v3:
  * Move 'check-connection' out of schema definition to extra param
    Pointed out by Wolfgang, it should not be part of the (stored)
    config schema if never stored. [2]

Changes v1 -> v2:
  * Do not store the 'check-connection' parameter in the realm config

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 [3] [4].

Note that the `base_dn`/`group_dn` now goes completely unvalided - but
again, that is how it works in PBS.

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

 src/PVE/API2/Domains.pm | 36 +++++++++++++++++++++++++++++++++---
 src/PVE/Auth/LDAP.pm    | 18 +++++++++---------
 src/PVE/Auth/Plugin.pm  |  8 ++++++++
 3 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 9332aa7..e7b7d39 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -117,7 +117,14 @@ __PACKAGE__->register_method ({
 	check => ['perm', '/access/realm', ['Realm.Allocate']],
     },
     description => "Add an authentication server.",
-    parameters => PVE::Auth::Plugin->createSchema(),
+    parameters => PVE::Auth::Plugin->createSchema(0, {
+	'check-connection' => {
+	    description => 'Check bind connection to the server.',
+	    type => 'boolean',
+	    optional => 1,
+	    default => 0,
+	},
+    }),
     returns => { type => 'null' },
     code => sub {
 	my ($param) = @_;
@@ -133,6 +140,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};
@@ -143,6 +151,9 @@ __PACKAGE__->register_method ({
 		die "unable to create builtin type '$type'\n"
 		    if ($type eq 'pam' || $type eq 'pve');

+		die "'check-connection' parameter can only be set for realms of type 'ldap' or 'ad'\n"
+		    if defined($check_connection) && !($type eq 'ldap' || $type eq 'ad');
+
 		if ($type eq 'ad' || $type eq 'ldap') {
 		    $map_sync_default_options->($param, 1);
 		}
@@ -165,6 +176,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");

@@ -180,7 +195,14 @@ __PACKAGE__->register_method ({
     },
     description => "Update authentication server settings.",
     protected => 1,
-    parameters => PVE::Auth::Plugin->updateSchema(),
+    parameters => PVE::Auth::Plugin->updateSchema(0, {
+	'check-connection' => {
+	    description => 'Check bind connection to the server.',
+	    type => 'boolean',
+	    optional => 1,
+	    default => 0,
+	},
+    }),
     returns => { type => 'null' },
     code => sub {
 	my ($param) = @_;
@@ -198,10 +220,15 @@ __PACKAGE__->register_method ({
 		PVE::SectionConfig::assert_if_modified($cfg, $digest);

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

 		die "domain '$realm' does not exist\n"
 		    if !$ids->{$realm};

+		die "'check-connection' parameter can only be set for realms of type 'ldap' or 'ad'\n"
+		    if defined($check_connection) && !($type eq 'ldap' || $type eq 'ad');
+
 		my $delete_str = extract_param($param, 'delete');
 		die "no options specified\n"
 		    if !$delete_str && !scalar(keys %$param) && !defined($password);
@@ -212,7 +239,6 @@ __PACKAGE__->register_method ({
 		    $delete_pw = 1 if $opt eq 'password';
 		}

-		my $type = $ids->{$realm}->{type};
 		if ($type eq 'ad' || $type eq 'ldap') {
 		    $map_sync_default_options->($param, 1);
 		}
@@ -237,6 +263,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..b958f2b 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,7 @@ sub properties {
 	    type => 'boolean',
 	    optional => 1,
 	    default => 1,
-	}
+	},
     };
 }

@@ -184,7 +178,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 +209,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 +448,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 v3 4/4] 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>
---
N.B.: As this uses the newly introduced `check-connection` API
parameter, a bump on libpve-access-control is needed.

Changes v2 -> v3:
  * No changes

Changes v1 -> v2:
  * Add "Check connection" checkbox to AD edit too

 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

[pve-devel] applied-series: [PATCH common/access-control/manager v3 0/4] ldap: check bind connection on realm add/update

[Wolfgang Bumiller]

applied series, thanks