* [pve-devel] [PATCH common v3 1/4] ldap: handle errors explicitly everywhere instead of simply `die`ing
2023-08-10 12:37 [pve-devel] [PATCH common/access-control/manager v3 0/4] ldap: check bind connection on realm add/update Christoph Heiss
@ 2023-08-10 12:37 ` Christoph Heiss
2023-08-10 12:37 ` [pve-devel] [PATCH common v3 2/4] section config: allow base properties for {create, update}Schema() Christoph Heiss
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Christoph Heiss @ 2023-08-10 12:37 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH access-control v3 3/4] ldap: add opt-in `check-connection` param to perform a bind check
2023-08-10 12:37 [pve-devel] [PATCH common/access-control/manager v3 0/4] ldap: check bind connection on realm add/update Christoph Heiss
2023-08-10 12:37 ` [pve-devel] [PATCH common v3 1/4] ldap: handle errors explicitly everywhere instead of simply `die`ing Christoph Heiss
2023-08-10 12:37 ` [pve-devel] [PATCH common v3 2/4] section config: allow base properties for {create, update}Schema() Christoph Heiss
@ 2023-08-10 12:37 ` Christoph Heiss
2023-08-10 12:37 ` [pve-devel] [PATCH manager v3 4/4] ui: ldap: add 'Check connection' checkbox as advanced option Christoph Heiss
2023-08-11 11:39 ` [pve-devel] applied-series: [PATCH common/access-control/manager v3 0/4] ldap: check bind connection on realm add/update Wolfgang Bumiller
4 siblings, 0 replies; 6+ messages in thread
From: Christoph Heiss @ 2023-08-10 12:37 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH manager v3 4/4] ui: ldap: add 'Check connection' checkbox as advanced option
2023-08-10 12:37 [pve-devel] [PATCH common/access-control/manager v3 0/4] ldap: check bind connection on realm add/update Christoph Heiss
` (2 preceding siblings ...)
2023-08-10 12:37 ` [pve-devel] [PATCH access-control v3 3/4] ldap: add opt-in `check-connection` param to perform a bind check Christoph Heiss
@ 2023-08-10 12:37 ` Christoph Heiss
2023-08-11 11:39 ` [pve-devel] applied-series: [PATCH common/access-control/manager v3 0/4] ldap: check bind connection on realm add/update Wolfgang Bumiller
4 siblings, 0 replies; 6+ messages in thread
From: Christoph Heiss @ 2023-08-10 12:37 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread