[pve-devel] [PATCH access-control/docs] fix #4609: fix ldap regex

[pve-devel] [PATCH access-control/docs] fix #4609: fix ldap regex

[Dominik Csapak]

to better cover existing configurations and the spec

pve-access-control:

Dominik Csapak (1):
  fix #4609: allow valid DN in ldap/ad realm config

 src/PVE/Auth/LDAP.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

pve-docs:

Dominik Csapak (1):
  user management: ldap: specify space and number sign escaping

 pveum.adoc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.30.2

[pve-devel] [PATCH access-control 1/1] fix #4609: allow valid DN in ldap/ad realm config

[Dominik Csapak]

we previously added support for ',' in the dns attribute by allowing a
quoted format. the regex was sadly too restrictive:

in a quoted attribute we'd only allow \w (alphanumeric + _) and the
restricted characters. this patch now changes that to everything
except " (nearer to the original regex which allowed everything aside
from ',')

the unquoted attributes now did not allow spaces, but reading the RFC[0]
again, spaces are only forbidden at the beginning (also #) and end
so fix the regex to accommodate for that

Fixes 1aa2355 ("ldap: Allow quoted values for DN attribute values")

0: https://www.ietf.org/rfc/rfc2253.txt

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/Auth/LDAP.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index 4d771e7..57782ad 100755
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -10,7 +10,8 @@ use PVE::Tools;
 
 use base qw(PVE::Auth::Plugin);
 
-our $dn_regex = qr!\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+)(,\s*\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+))*!;
+my  $dn_part_regex = qr!("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^,+"/<>;=#])!;
+our $dn_regex = qr!\w+=${dn_part_regex}(,\s*\w+=${dn_part_regex})*!;
 
 sub type {
     return 'ldap';
-- 
2.30.2

Re: [pve-devel] [PATCH access-control 1/1] fix #4609: allow valid DN in ldap/ad realm config

[Thomas Lamprecht]

Am 23/03/2023 um 14:14 schrieb Dominik Csapak:
> we previously added support for ',' in the dns attribute by allowing a
> quoted format. the regex was sadly too restrictive:
> 
> in a quoted attribute we'd only allow \w (alphanumeric + _) and the
> restricted characters. this patch now changes that to everything
> except " (nearer to the original regex which allowed everything aside
> from ',')
> 
> the unquoted attributes now did not allow spaces, but reading the RFC[0]
> again, spaces are only forbidden at the beginning (also #) and end
> so fix the regex to accommodate for that
> 
> Fixes 1aa2355 ("ldap: Allow quoted values for DN attribute values")

this should be a git trailer, directly above your S-o-b as then various
git analyzing script can actually make use of it.

> 
> 0: https://www.ietf.org/rfc/rfc2253.txt
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/Auth/LDAP.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index 4d771e7..57782ad 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -10,7 +10,8 @@ use PVE::Tools;
>  
>  use base qw(PVE::Auth::Plugin);
>  
> -our $dn_regex = qr!\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+)(,\s*\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+))*!;
> +my  $dn_part_regex = qr!("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^,+"/<>;=#])!;
> +our $dn_regex = qr!\w+=${dn_part_regex}(,\s*\w+=${dn_part_regex})*!;

Christoph, can you check/review this?

>  
>  sub type {
>      return 'ldap';

Re: [pve-devel] [PATCH access-control 1/1] fix #4609: allow valid DN in ldap/ad realm config

[Friedrich Weber]

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

Tested the following:

* PVE 7.3: setup LDAP realms
  realm #1 with `base_dn ou=Foo- und Bar,dc=example,dc=com`
  realm #2 with `base_dn ou=Users,dc=example,dc=com`
  both work, i.e., sync is possible and users can log in
* Update to 7.4:
  realm #1: users cannot login ("value does not match the regex pattern")
  realm #2: still works
* With this patch:
  realm #1: login works again
  realm #2: still works

On 23/03/2023 14:14, Dominik Csapak wrote:
> we previously added support for ',' in the dns attribute by allowing a
> quoted format. the regex was sadly too restrictive:

[pve-devel] applied: [PATCH access-control 1/1] fix #4609: allow valid DN in ldap/ad realm config

[Thomas Lamprecht]

Am 23/03/2023 um 14:14 schrieb Dominik Csapak:
> we previously added support for ',' in the dns attribute by allowing a
> quoted format. the regex was sadly too restrictive:
> 
> in a quoted attribute we'd only allow \w (alphanumeric + _) and the
> restricted characters. this patch now changes that to everything
> except " (nearer to the original regex which allowed everything aside
> from ',')
> 
> the unquoted attributes now did not allow spaces, but reading the RFC[0]
> again, spaces are only forbidden at the beginning (also #) and end
> so fix the regex to accommodate for that
> 
> Fixes 1aa2355 ("ldap: Allow quoted values for DN attribute values")
> 
> 0: https://www.ietf.org/rfc/rfc2253.txt
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/Auth/LDAP.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied, with Friedrich's T-b, a small fixup that adds space to the not allowed
characters at the end of the single character branch of the regex like talked
off-list and a bit of rewording of the commit message, thanks!

[pve-devel] [PATCH docs 1/1] user management: ldap: specify space and number sign escaping

[Dominik Csapak]

number sign at the beginning (was missing completely)
space only needs escaping at the start and end

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pveum.adoc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pveum.adoc b/pveum.adoc
index 147f317..6a0ad17 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -403,7 +403,8 @@ easily used in attribute values in DNs without being escaped properly.
 
 Following characters need escaping:
 
-* Space ( )
+* Space ( ) at the beginning or end
+* Number sign (`#`) at the beginning
 * Comma (`,`)
 * Plus sign (`+`)
 * Double quote (`"`)
-- 
2.30.2

[pve-devel] applied: [PATCH docs 1/1] user management: ldap: specify space and number sign escaping

[Thomas Lamprecht]

Am 23/03/2023 um 14:14 schrieb Dominik Csapak:
> number sign at the beginning (was missing completely)
> space only needs escaping at the start and end
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pveum.adoc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied, thanks!