[pve-devel] [PATCH common/access-control v2 0/5] improve LDAP DN and bind creds checking on creation/change

[pve-devel] [PATCH common/access-control v2 0/5] improve LDAP DN and bind creds checking on creation/change

[Christoph Heiss]

tl;dr implements the result of the discussion in [0].

First, this removes the dreaded LDAP DN regex, replacing it instead with
a proper schema format, which does validation using
Net::LDAP::Util::canonical_dn().

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 [1].

Changes were tested against slapd 2.5.13+dfsg-5.

[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

v1: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058274.html

Notable changes v1 -> v2:
  * Fixed updating a LDAP realm via raw API/pveum/pvesh (see #5)

pve-common:

Christoph Heiss (3):
  schema: add `ldap-dn` format for validating LDAP distinguished names
  test: add test cases for new 'ldap-dn' schema format
  ldap: handle errors explicitly everywhere instead of simply `die`ing

 debian/control              |  2 ++
 src/PVE/JSONSchema.pm       | 12 +++++++++
 src/PVE/LDAP.pm             | 11 ++++----
 test/Makefile               |  1 +
 test/ldap_dn_format_test.pl | 54 +++++++++++++++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 5 deletions(-)

pve-access-control:

Christoph Heiss (2):
  ldap: validate LDAP DNs using the `ldap-dn` schema format
  ldap: check bind credentials with LDAP directory directly on change

 src/PVE/API2/Domains.pm |  7 +++++--
 src/PVE/Auth/LDAP.pm    | 28 ++++++++++++++++------------
 2 files changed, 21 insertions(+), 14 deletions(-)

--
2.41.0

[pve-devel] [PATCH common v2 1/5] schema: add `ldap-dn` format for validating LDAP distinguished names

[Christoph Heiss]

The Net::LDAP library conveniently provides a canonical_dn() function,
which can be used to (roughly) check if a DN is valid or not. This will
be used in future changes to replace the current dreaded regex to
validate DNs.

pve-common previously already (silently) depended on the Net::LDAP
library (see PVE::LDAP), but `libnet-ldap-perl` was missing in the
control file - fix it while at it.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * No changes

 debian/control        |  1 +
 src/PVE/JSONSchema.pm | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/debian/control b/debian/control
index ac4cd66..53cbb57 100644
--- a/debian/control
+++ b/debian/control
@@ -34,6 +34,7 @@ Depends: libanyevent-perl,
          libmime-base32-perl,
          libnet-dbus-perl,
          libnet-ip-perl,
+         libnet-ldap-perl,
          libnetaddr-ip-perl,
          libproxmox-acme-perl,
          libproxmox-rs-perl,
diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 7589bba..8238281 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -12,6 +12,7 @@ use PVE::Exception qw(raise);
 use HTTP::Status qw(:constants);
 use JSON;
 use Net::IP qw(:PROC);
+use Net::LDAP::Util;
 use Data::Dumper;

 use base 'Exporter';
@@ -414,6 +415,17 @@ sub verify_ldap_simple_attr {
     return undef;
 }

+PVE::JSONSchema::register_format('ldap-dn', \&verify_ldap_dn);
+sub verify_ldap_dn {
+    my ($attr, $noerr) = @_;
+
+    # canonical_dn() considers emtpy strings as valid DNs, so reject them explicitly.
+    return $attr if $attr ne '' && defined(Net::LDAP::Util::canonical_dn($attr));
+
+    die "value '$attr' does not look like a valid LDAP distinguished name\n" if !$noerr;
+    return undef;
+}
+
 my $ipv4_mask_hash = {
     '0.0.0.0' => 0,
     '128.0.0.0' => 1,
--
2.41.0

[pve-devel] [PATCH common v2 2/5] test: add test cases for new 'ldap-dn' schema format

[Christoph Heiss]

Mostly from [0], slightly adapted to marginally different rules due to
using Net::LDAP::Util::canonical_dn() under the hood.

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

Co-authored-by: Stefan Sterz <s.sterz@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * Removed (accidental) duplicate `TESTS` assignment in test/Makefile

 debian/control              |  1 +
 test/Makefile               |  1 +
 test/ldap_dn_format_test.pl | 54 +++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)
 create mode 100755 test/ldap_dn_format_test.pl

diff --git a/debian/control b/debian/control
index 53cbb57..f59ce0d 100644
--- a/debian/control
+++ b/debian/control
@@ -11,6 +11,7 @@ Build-Depends: debhelper-compat (= 13),
                libjson-perl,
                liblinux-inotify2-perl,
                libnet-ip-perl,
+               libnet-ldap-perl,
                libnetaddr-ip-perl,
                libproxmox-rs-perl,
                libstring-shellquote-perl,
diff --git a/test/Makefile b/test/Makefile
index 82f40ab..e77ed73 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -4,6 +4,7 @@ TESTS = lock_file.test			\
 	convert_size_test.test		\
 	procfs_tests.test		\
 	format_test.test		\
+	ldap_dn_format_test.test	\
 	section_config_test.test	\
 	api_parameter_test.test		\

diff --git a/test/ldap_dn_format_test.pl b/test/ldap_dn_format_test.pl
new file mode 100755
index 0000000..c41d324
--- /dev/null
+++ b/test/ldap_dn_format_test.pl
@@ -0,0 +1,54 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib '../src';
+use PVE::JSONSchema;
+
+use Test::More;
+
+my @pass = (
+    "ou=a",			# single AttributeTypeValue
+    "ou=orga,dc=com,cn=name",	# multiple RelativeDistinguishedNames
+    "STREET=a,cn=a,C=c",	# single character AttributeValues
+    "UID=tt,cn=\"#+,;<>\\ \"",	# forbidden characters are allowed when quoted
+    "c=\\\"\\#\\+\\;\\<\\=\\>",	# specific characters allowed when escaped
+    "a=\\\\",			# escaped backslashes are allowed
+    "ST=a,cn=\"Test, User\"",	# allow un-escaped commas in quoted AttributeValues
+    "o2u=bc,cn=Test\\, User",	# allow escaped commas
+    "T2=a #b",			# spaces (' ') and '#' are allowed in the middle of AttributeValues
+    "word4word=ab#",		# allow '#' at the end of an AttributeValue
+    "ou=orga+sub=ab",		# allow '+' as separators for multi-valued RelativeDistinguishedName
+    "dc=\\f0\\Ac\\93",		# allow escaping hex values in unquoted AttributeValues
+    "ou=",			# empty AttributeValue is allowed
+    "ou= or",			# spaces at the front of an AttributeValue are allowed
+    "ou=orgs ",			# spaces at the end of an AttributeValue are also allowed
+    "ou= foo ",			# combination of the two cases above
+
+    # regression tests
+    "ou=adf-bd,dc=abcd+efOuId=BL:BL:sldkf:704004,dc=or,dc=com",
+    "gvGid=DE:8A:wordCaps,ou=Service,dc=alsdkj+abOuId=UK:A8:137100,dc=edu,dc=de",
+);
+
+my @fail = (
+    "",				# no empty distinguished name
+    "ou=a,",			# no empty AttributeTypeAndValue
+    "ou=a+",			# no multi-valued RelativeDistinguishedName with empty second part
+    "ou",			# missing separator and AttributeValue
+    "ou=+",			# forbidden character '+' in AttributeValue
+    "ou=#value",		# no '#' at the beginning an AttributeValue
+    "ou=\"+,;<>\\\0",		# no un-escaped forbidden characters in unquoted AttributeValues
+    "ou=name\0",		# no null value in AttributeValue
+    "ou=zy\\xw\\v"		# no unescaped backslashes that are not escaping specific characters
+);
+
+for my $dn (@pass) {
+    is(PVE::JSONSchema::verify_ldap_dn($dn, 1), $dn, 'valid LDAP DN');
+}
+
+for my $dn (@fail) {
+    is(PVE::JSONSchema::verify_ldap_dn($dn, 1), undef, 'invalid LDAP DN');
+}
+
+done_testing();
--
2.41.0

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

 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 4/5] ldap: validate LDAP DNs using the `ldap-dn` schema format

[Christoph Heiss]

Remove the dreaded DN regex, instead validating DNs using the newly
defined `ldap-dn` schema format (which uses
Net::LDAP::Util::canonical_dn() under the hood).

Result of a previous discussion [0].

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

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * No changes

Needs a bump of libpve-common-perl due to the new 'ldap-dn' schema
format.

 src/PVE/Auth/LDAP.pm | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index fc82a17..e0ead1b 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,7 @@ sub properties {
 	base_dn => {
 	    description => "LDAP base domain name",
 	    type => 'string',
-	    pattern => $dn_regex,
+	    format => 'ldap-dn',
 	    optional => 1,
 	    maxLength => 256,
 	},
@@ -36,7 +33,7 @@ sub properties {
 	bind_dn => {
 	    description => "LDAP bind domain name",
 	    type => 'string',
-	    pattern => $dn_regex,
+	    format => 'ldap-dn',
 	    optional => 1,
 	    maxLength => 256,
 	},
@@ -94,7 +91,7 @@ sub properties {
 	    description => "LDAP base domain name for group sync. If not set, the"
 		." base_dn will be used.",
 	    type => 'string',
-	    pattern => $dn_regex,
+	    format => 'ldap-dn',
 	    optional => 1,
 	    maxLength => 256,
 	},
--
2.41.0

[pve-devel] [PATCH access-control v2 5/5] ldap: check bind credentials with LDAP directory directly on change

[Christoph Heiss]

Instead of letting a sync fail on the first try due to e.g. bad bind
credentials, give the user some direct feedback when trying to save a
LDAP realm in the UI.

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>
---
Changes v1 -> v2:
  * Fixed updating a LDAP realm via pveum/pvesh/etc (thanks Friedrich!)
    Summary: $plugin->on_update_hook() was passed only the updated
    parameters, meaning if only e.g. the `comment` field was updated,
    other options like `server1` were missing and thus the connect/bind
    would fail - it worked via the GUI, as that always passes all set
    options.
  * Replaced ternary operator with simplier short-circuit ||

One thing to note might be that this "regresses" the UI in that you
cannot create or change realm settings, if the target LDAP server cannot
be reached.
After a short off-list discussion with Sterzy, a 'force' API
parameter (and accompanying checkbox in the UI) could be added to bypass
this "restriction", although open to discussion how useful that even
would be.

 src/PVE/API2/Domains.pm |  7 +++++--
 src/PVE/Auth/LDAP.pm    | 19 +++++++++++++------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm
index 9332aa7..68e3c7d 100644
--- a/src/PVE/API2/Domains.pm
+++ b/src/PVE/API2/Domains.pm
@@ -231,10 +231,13 @@ __PACKAGE__->register_method ({
 		}

 		my $opts = $plugin->options();
+
+		# Some update hooks, like e.g. LDAP, which tries to connect and bind to the target
+		# server, need the rest of config too => use the already-merged config
 		if ($delete_pw || defined($password)) {
-		    $plugin->on_update_hook($realm, $config, password => $password);
+		    $plugin->on_update_hook($realm, $ids->{$realm}, password => $password);
 		} else {
-		    $plugin->on_update_hook($realm, $config);
+		    $plugin->on_update_hook($realm, $ids->{$realm});
 		}

 		cfs_write_file($domainconfigfile, $cfg);
diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index e0ead1b..ec4cd31 100755
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -181,7 +181,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};
@@ -212,7 +212,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}) {
@@ -427,20 +427,27 @@ sub on_add_hook {
     my ($class, $realm, $config, %param) = @_;

     if (defined($param{password})) {
+	$class->connect_and_bind($config, $realm, \%param);
 	ldap_set_credentials($param{password}, $realm);
     } else {
-	ldap_delete_credentials($realm);
+	# Still validate the bind credentials, even if no user/password is given
+	$class->connect_and_bind($config, $realm);
     }
 }

 sub on_update_hook {
     my ($class, $realm, $config, %param) = @_;

-    return if !exists($param{password});
-
-    if (defined($param{password})) {
+    if (!exists($param{password})) {
+	# Password wasn't changed, login still needs to be validated in case bind_dn changed
+	$class->connect_and_bind($config, $realm);
+    } elsif (defined($param{password})) {
+	# Password was updated, validate the login and only then store it
+	$class->connect_and_bind($config, $realm, \%param);
 	ldap_set_credentials($param{password}, $realm);
     } else {
+	# Otherwise, the password was removed, so delete it
+	$class->connect_and_bind($config, $realm, \%param);
 	ldap_delete_credentials($realm);
     }
 }
--
2.41.0

Re: [pve-devel] [PATCH common/access-control v2 0/5] improve LDAP DN and bind creds checking on creation/change

[Friedrich Weber]

Tested against slapd 2.4.47+dfsg-3+deb10u6 again, also with a base DN
with escaped UTF-8 -- connection check and authentication worked fine.

Also tested that updating the realm via API/pveum works now, thanks for
fixing this!

Tested-by: Friedrich Weber <f.weber@proxmox.com>

On 24/07/2023 11:03, Christoph Heiss wrote:
> tl;dr implements the result of the discussion in [0].
> 
> First, this removes the dreaded LDAP DN regex, replacing it instead with
> a proper schema format, which does validation using
> Net::LDAP::Util::canonical_dn().
> 
> 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 [1].
> 
> Changes were tested against slapd 2.5.13+dfsg-5.
> 
> [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
> 
> v1: https://lists.proxmox.com/pipermail/pve-devel/2023-July/058274.html
> 
> Notable changes v1 -> v2:
>   * Fixed updating a LDAP realm via raw API/pveum/pvesh (see #5)
> 
> pve-common:
> 
> Christoph Heiss (3):
>   schema: add `ldap-dn` format for validating LDAP distinguished names
>   test: add test cases for new 'ldap-dn' schema format
>   ldap: handle errors explicitly everywhere instead of simply `die`ing
> 
>  debian/control              |  2 ++
>  src/PVE/JSONSchema.pm       | 12 +++++++++
>  src/PVE/LDAP.pm             | 11 ++++----
>  test/Makefile               |  1 +
>  test/ldap_dn_format_test.pl | 54 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 75 insertions(+), 5 deletions(-)
> 
> pve-access-control:
> 
> Christoph Heiss (2):
>   ldap: validate LDAP DNs using the `ldap-dn` schema format
>   ldap: check bind credentials with LDAP directory directly on change
> 
>  src/PVE/API2/Domains.pm |  7 +++++--
>  src/PVE/Auth/LDAP.pm    | 28 ++++++++++++++++------------
>  2 files changed, 21 insertions(+), 14 deletions(-)
> 
> --
> 2.41.0
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH common/access-control v2 0/5] improve LDAP DN and bind creds checking on creation/change

[Lukas Wagner]

On 7/24/23 11:03, Christoph Heiss wrote:
> tl;dr implements the result of the discussion in [0].
> 
> First, this removes the dreaded LDAP DN regex, replacing it instead with
> a proper schema format, which does validation using
> Net::LDAP::Util::canonical_dn().
> 
Already discussed off-list, but for the sake of completeness:

I'd say we can just do the same thing as in PBS, were we only verify the settings by
connecting to the server, but nothing else.
If we drop the check through `canonical_dn()`, then we actually improve
the AD realm implementation, which is also based on the LDAP code.

AD not only supports the regular DN syntax, but also:
   Domain\Administrator
   Administrator@Domain

However, these two formats are not accepted by `canonical_dn`. If we just drop the
check, then these alternative forms will work automatically (I've actually tested
this against a real AD server)


-- 
- Lukas