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

[pve-devel] [PATCH common/access-control 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 was done 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
[1] https://lists.proxmox.com/pipermail/pbs-devel/2023-June/006237.html

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               |  9 +++++++
 test/ldap_dn_format_test.pl | 54 +++++++++++++++++++++++++++++++++++++
 5 files changed, 83 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/Auth/LDAP.pm | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

--
2.41.0

[pve-devel] [PATCH common 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>
---
 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 2/5] test: add test cases for new 'ldap-dn' schema format

[Christoph Heiss]

Mostly from [0], slightly adapted due 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>
---
 debian/control              |  1 +
 test/Makefile               |  9 +++++++
 test/ldap_dn_format_test.pl | 54 +++++++++++++++++++++++++++++++++++++
 3 files changed, 64 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..5e62f12 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -7,6 +7,15 @@ TESTS = lock_file.test			\
 	section_config_test.test	\
 	api_parameter_test.test		\

+TESTS := \
+	lock_file.test \
+	calendar_event_test.test \
+	convert_size_test.test \
+	procfs_tests.test \
+	format_test.test \
+	ldap_dn_format_test.test \
+	section_config_test.test
+
 all:

 .PHONY: check install clean distclean
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 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>
---
 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 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>
---
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 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>
---
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/Auth/LDAP.pm | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index e0ead1b..b73be9d 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 = defined($param->{password}) ? $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 0/5] improve LDAP DN and bind creds checking on creation/change

[Friedrich Weber]

Tested against slapd 2.4.47+dfsg-3+deb10u6. I quite like the connection
check when creating/updating the realm, and also, it seems sensible to
delegate DN validation to Net::LDAP.

I noticed one bug: Weirdly, updating the realm via CLI or manually via
API now errors out for me (the connection details are correct):

$ cat /etc/pve/domains.cfg
pam: pam
	comment Linux PAM standard authentication

pve: pve
	comment Proxmox VE authentication server
	default 0

ldap: ldap
	comment foo
	base_dn dc=example,dc=com
	server1 [...]
	user_attr uid
	bind_dn cn=admin,dc=example,dc=com
	default 0
	secure 0

$ pveum realm modify ldap -comment foo
update auth server failed: Expected 'PeerHost' at
/usr/share/perl5/Net/LDAP.pm line 173.

$ http --verify no PUT
'https://[...]:8006/api2/json/access/domains/ldap' comment=foo [...]
HTTP/1.1 500 update auth server failed: Expected 'PeerHost' at
/usr/share/perl5/Net/LDAP.pm line 173.

On 19/07/2023 17:51, 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 was done 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
> [1] https://lists.proxmox.com/pipermail/pbs-devel/2023-June/006237.html
> 
> 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               |  9 +++++++
>  test/ldap_dn_format_test.pl | 54 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 83 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/Auth/LDAP.pm | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 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 0/5] improve LDAP DN and bind creds checking on creation/change

[Christoph Heiss]

Thanks for taking a look and testing this!

On Thu, Jul 20, 2023 at 02:42:10PM +0200, Friedrich Weber wrote:
>
> Tested against slapd 2.4.47+dfsg-3+deb10u6. I quite like the connection
> check when creating/updating the realm, and also, it seems sensible to
> delegate DN validation to Net::LDAP.
>
> I noticed one bug: Weirdly, updating the realm via CLI or manually via
> API now errors out for me (the connection details are correct):
I only tested it via the UI, definitely a good catch.

>
> $ cat /etc/pve/domains.cfg
> pam: pam
> 	comment Linux PAM standard authentication
>
> pve: pve
> 	comment Proxmox VE authentication server
> 	default 0
>
> ldap: ldap
> 	comment foo
> 	base_dn dc=example,dc=com
> 	server1 [...]
> 	user_attr uid
> 	bind_dn cn=admin,dc=example,dc=com
> 	default 0
> 	secure 0
>
> $ pveum realm modify ldap -comment foo
> update auth server failed: Expected 'PeerHost' at
> /usr/share/perl5/Net/LDAP.pm line 173.
Weird. That error doesn't really match up with anything on my machine in
that file - what version of the `libnet-ldap-perl` package do
you have installed exactly?

Because I cannot seem to reproduce that error on my machine, both
`pveum` and `pvesh` work just fine for me.

>
> $ http --verify no PUT
> 'https://[...]:8006/api2/json/access/domains/ldap' comment=foo [...]
> HTTP/1.1 500 update auth server failed: Expected 'PeerHost' at
> /usr/share/perl5/Net/LDAP.pm line 173.
>
> On 19/07/2023 17:51, Christoph Heiss wrote:
> [..]