[pve-devel] [PATCH access-control/docs 0/2] fix #3748: Allow reserved characters in attribute values of LDAP DNs

[pve-devel] [PATCH access-control/docs 0/2] fix #3748: Allow reserved characters in attribute values of LDAP DNs

[Christoph Heiss]

This fixes #3748 [0] by allowing reserved characters in `bind_dn` (and
other properties of the same format) if they are properly quoted and
adds some corresponding documentation regarding that.

This was tested by setting up a slapd server and creating a user with
the CN `Test, User` much like in the bug report, then using this user as
`bind_dn` in the sync options. I also tested some variants of that CN,
including just `TestUser`.)

One thing that still won't work is syncing of LDAP users with colons or
slashes in their CNs. In such cases, the message

  value 'Test, User@ldap' does not look like a valid user name

will pop up.

This is due to spaces and colons being explicitly disallowed in
usernames [1]. This probably means that such names can never be allowed,
which is being documented too as part of patch 2.

But with this series, such users can be at least used to bind for
syncing.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=3748
[1] https://git.proxmox.com/?p=pve-access-control.git;a=blob;f=src/PVE/Auth/Plugin.pm;hb=HEAD#l115

Christoph Heiss (1):
      ldap: Allow quoted values for DN attribute values

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

Christoph Heiss (1):
      pveum: Document reserved characters and quoting of LDAP DNs

 pveum.adoc | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

--
2.34.1

[pve-devel] [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 src/PVE/Auth/LDAP.pm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index 4792586..4d771e7 100755
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -10,6 +10,8 @@ use PVE::Tools;

 use base qw(PVE::Auth::Plugin);

+our $dn_regex = qr!\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+)(,\s*\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+))*!;
+
 sub type {
     return 'ldap';
 }
@@ -19,7 +21,7 @@ sub properties {
 	base_dn => {
 	    description => "LDAP base domain name",
 	    type => 'string',
-	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
+	    pattern => $dn_regex,
 	    optional => 1,
 	    maxLength => 256,
 	},
@@ -33,7 +35,7 @@ sub properties {
 	bind_dn => {
 	    description => "LDAP bind domain name",
 	    type => 'string',
-	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
+	    pattern => $dn_regex,
 	    optional => 1,
 	    maxLength => 256,
 	},
@@ -91,7 +93,7 @@ sub properties {
 	    description => "LDAP base domain name for group sync. If not set, the"
 		." base_dn will be used.",
 	    type => 'string',
-	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
+	    pattern => $dn_regex,
 	    optional => 1,
 	    maxLength => 256,
 	},
--
2.34.1

Re: [pve-devel] [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values

[Dominik Csapak]

hi,

so high level comment:
i'd write most of what you wrote in the cover letter here in the commit message,
makes it much more convenient to find it only via git ;)

also i'm missing a bit the rationale for how the regex was chosen, besides
that it works in some conditions

further comment inline

On 1/31/23 13:50, Christoph Heiss wrote:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>   src/PVE/Auth/LDAP.pm | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index 4792586..4d771e7 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -10,6 +10,8 @@ use PVE::Tools;
> 
>   use base qw(PVE::Auth::Plugin);
> 
> +our $dn_regex = qr!\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+)(,\s*\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+))*!;

are you sure you did not make it more strict than what is allowed?

e.g. if i had 'foo=<,bar=>' that would have previously worked, but now is forbidden AFAICS
while we can make such changes, we should only do so on major releases where it's a breaking
change, preferably with a workaround and/or script where we can rewrite/warn the user
that it's not valid syntax

OTOH, most users probably won't notice since they did not use such 'strange' values

the problem here is that possibly working configs are not valid anymore
(for logins it's problematic, depending on how the admins log in)

> +
>   sub type {
>       return 'ldap';
>   }
> @@ -19,7 +21,7 @@ sub properties {
>   	base_dn => {
>   	    description => "LDAP base domain name",
>   	    type => 'string',
> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> +	    pattern => $dn_regex,
>   	    optional => 1,
>   	    maxLength => 256,
>   	},
> @@ -33,7 +35,7 @@ sub properties {
>   	bind_dn => {
>   	    description => "LDAP bind domain name",
>   	    type => 'string',
> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> +	    pattern => $dn_regex,
>   	    optional => 1,
>   	    maxLength => 256,
>   	},
> @@ -91,7 +93,7 @@ sub properties {
>   	    description => "LDAP base domain name for group sync. If not set, the"
>   		." base_dn will be used.",
>   	    type => 'string',
> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> +	    pattern => $dn_regex,
>   	    optional => 1,
>   	    maxLength => 256,
>   	},
> --
> 2.34.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values

[Christoph Heiss]

Thanks for the review!

On Wed, Mar 15, 2023 at 10:54:38AM +0100, Dominik Csapak wrote:
> hi,
>
> so high level comment:
> i'd write most of what you wrote in the cover letter here in the commit message,
> makes it much more convenient to find it only via git ;)
Good point, I'll do that if/when I spin a v2 and for further patchsets!
I will also include the main points from below, to really make things clear.

>
> also i'm missing a bit the rationale for how the regex was chosen, besides
> that it works in some conditions
Ack, I should have elaborated on that in the commit.

Basically, I took the current regex and what characters are allowed in
attribute values (see patch #2). From that, constructing the character
class for the not-allowed characters (and conversely, the quoted version
of that to allow such special characters) and further the whole regex
was rather simple. The latter was based on the previous one.

So although it looks a bit like a mess, it's a rather simple regex if
you look at it this way.

>
> further comment inline
>
> On 1/31/23 13:50, Christoph Heiss wrote:
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> >   src/PVE/Auth/LDAP.pm | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> > index 4792586..4d771e7 100755
> > --- a/src/PVE/Auth/LDAP.pm
> > +++ b/src/PVE/Auth/LDAP.pm
> > @@ -10,6 +10,8 @@ use PVE::Tools;
> >
> >   use base qw(PVE::Auth::Plugin);
> >
> > +our $dn_regex = qr!\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+)(,\s*\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+))*!;
>
> are you sure you did not make it more strict than what is allowed?
>
> e.g. if i had 'foo=<,bar=>' that would have previously worked, but now is forbidden AFAICS
Thing is, that would have not worked previously anyway. "Worked" in that
sense that any sensible LDAP server would have failed to parse or
outright rejected such DNs anyway, but could be configured using the
API/UI.

Picking up on your example, "<" and ">" are both not allowed (at least
unquoted) in DN attribute values - see the docs patch again. But using
them properly quoted (e.g. foo="<",bar=">") worked before as does it
with the patch.

I just tested this exact example (using an unpatched PVE) against a
(somewhat current, v2.5.13 as available in bullseye-backports) slapd
server for the sake of it - it fails when performing the search with
"invalid DN" - as expected.

> while we can make such changes, we should only do so on major releases where it's a breaking
> change, preferably with a workaround and/or script where we can rewrite/warn the user
> that it's not valid syntax
>
> OTOH, most users probably won't notice since they did not use such 'strange' values
>
> the problem here is that possibly working configs are not valid anymore
> (for logins it's problematic, depending on how the admins log in)
Following up on the above, I'd hope no user has such configuration. And
if so, that user has to be using a completely bonkers LDAP
server/implementation.

In conclusion, I don't see how this could break existing setups. But I
do see your point - breaking someones existing setup is a no-go. In that
case I would just hold onto this patchset for the next major release.

>
> > +
> >   sub type {
> >       return 'ldap';
> >   }
> > @@ -19,7 +21,7 @@ sub properties {
> >   	base_dn => {
> >   	    description => "LDAP base domain name",
> >   	    type => 'string',
> > -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> > +	    pattern => $dn_regex,
> >   	    optional => 1,
> >   	    maxLength => 256,
> >   	},
> > @@ -33,7 +35,7 @@ sub properties {
> >   	bind_dn => {
> >   	    description => "LDAP bind domain name",
> >   	    type => 'string',
> > -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> > +	    pattern => $dn_regex,
> >   	    optional => 1,
> >   	    maxLength => 256,
> >   	},
> > @@ -91,7 +93,7 @@ sub properties {
> >   	    description => "LDAP base domain name for group sync. If not set, the"
> >   		." base_dn will be used.",
> >   	    type => 'string',
> > -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> > +	    pattern => $dn_regex,
> >   	    optional => 1,
> >   	    maxLength => 256,
> >   	},
> > --
> > 2.34.1
> >
> >
> >
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> >
> >
>
>

Re: [pve-devel] [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values

[Dominik Csapak]

On 3/15/23 12:17, Christoph Heiss wrote:
> Thanks for the review!
> 
> On Wed, Mar 15, 2023 at 10:54:38AM +0100, Dominik Csapak wrote:
>> hi,
>>
>> so high level comment:
>> i'd write most of what you wrote in the cover letter here in the commit message,
>> makes it much more convenient to find it only via git ;)
> Good point, I'll do that if/when I spin a v2 and for further patchsets!
> I will also include the main points from below, to really make things clear.
> 
>>
>> also i'm missing a bit the rationale for how the regex was chosen, besides
>> that it works in some conditions
> Ack, I should have elaborated on that in the commit.
> 
> Basically, I took the current regex and what characters are allowed in
> attribute values (see patch #2). From that, constructing the character
> class for the not-allowed characters (and conversely, the quoted version
> of that to allow such special characters) and further the whole regex
> was rather simple. The latter was based on the previous one.
> 
> So although it looks a bit like a mess, it's a rather simple regex if
> you look at it this way.
> 
>>
>> further comment inline
>>
>> On 1/31/23 13:50, Christoph Heiss wrote:
>>> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
>>> ---
>>>    src/PVE/Auth/LDAP.pm | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
>>> index 4792586..4d771e7 100755
>>> --- a/src/PVE/Auth/LDAP.pm
>>> +++ b/src/PVE/Auth/LDAP.pm
>>> @@ -10,6 +10,8 @@ use PVE::Tools;
>>>
>>>    use base qw(PVE::Auth::Plugin);
>>>
>>> +our $dn_regex = qr!\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+)(,\s*\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+))*!;
>>
>> are you sure you did not make it more strict than what is allowed?
>>
>> e.g. if i had 'foo=<,bar=>' that would have previously worked, but now is forbidden AFAICS
> Thing is, that would have not worked previously anyway. "Worked" in that
> sense that any sensible LDAP server would have failed to parse or
> outright rejected such DNs anyway, but could be configured using the
> API/UI.
> 
> Picking up on your example, "<" and ">" are both not allowed (at least
> unquoted) in DN attribute values - see the docs patch again. But using
> them properly quoted (e.g. foo="<",bar=">") worked before as does it
> with the patch.
> 
> I just tested this exact example (using an unpatched PVE) against a
> (somewhat current, v2.5.13 as available in bullseye-backports) slapd
> server for the sake of it - it fails when performing the search with
> "invalid DN" - as expected.
> 
>> while we can make such changes, we should only do so on major releases where it's a breaking
>> change, preferably with a workaround and/or script where we can rewrite/warn the user
>> that it's not valid syntax
>>
>> OTOH, most users probably won't notice since they did not use such 'strange' values
>>
>> the problem here is that possibly working configs are not valid anymore
>> (for logins it's problematic, depending on how the admins log in)
> Following up on the above, I'd hope no user has such configuration. And
> if so, that user has to be using a completely bonkers LDAP
> server/implementation.
> 
> In conclusion, I don't see how this could break existing setups. But I
> do see your point - breaking someones existing setup is a no-go. In that
> case I would just hold onto this patchset for the next major release.


ok i mistook the 'reserved' characters as reserved by us, not ldap.
in such a case, when there is an external format/etc. please
include a reference on where to find these restrictions
(e.g. a link to an rfc)

if my example and all that could have been configured but
would now be invalid are not valid ldap syntax anyway, i think
we can get more strict and "break" someones config
(as you said, shouldn't have worked anyway)
or how do you see that @thomas?

(maybe there are some weirdly configured ldap instances out there?)


> 
>>
>>> +
>>>    sub type {
>>>        return 'ldap';
>>>    }
>>> @@ -19,7 +21,7 @@ sub properties {
>>>    	base_dn => {
>>>    	    description => "LDAP base domain name",
>>>    	    type => 'string',
>>> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
>>> +	    pattern => $dn_regex,
>>>    	    optional => 1,
>>>    	    maxLength => 256,
>>>    	},
>>> @@ -33,7 +35,7 @@ sub properties {
>>>    	bind_dn => {
>>>    	    description => "LDAP bind domain name",
>>>    	    type => 'string',
>>> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
>>> +	    pattern => $dn_regex,
>>>    	    optional => 1,
>>>    	    maxLength => 256,
>>>    	},
>>> @@ -91,7 +93,7 @@ sub properties {
>>>    	    description => "LDAP base domain name for group sync. If not set, the"
>>>    		." base_dn will be used.",
>>>    	    type => 'string',
>>> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
>>> +	    pattern => $dn_regex,
>>>    	    optional => 1,
>>>    	    maxLength => 256,
>>>    	},
>>> --
>>> 2.34.1
>>>
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>
>>

Re: [pve-devel] [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values

[Christoph Heiss]

Comment inline.

On Wed, Mar 15, 2023 at 12:41:39PM +0100, Dominik Csapak wrote:
> On 3/15/23 12:17, Christoph Heiss wrote:
> > Thanks for the review!
> >
> > [..]
> > > >
> > > > diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> > > > index 4792586..4d771e7 100755
> > > > --- a/src/PVE/Auth/LDAP.pm
> > > > +++ b/src/PVE/Auth/LDAP.pm
> > > > @@ -10,6 +10,8 @@ use PVE::Tools;
> > > >
> > > >    use base qw(PVE::Auth::Plugin);
> > > >
> > > > +our $dn_regex = qr!\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+)(,\s*\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+))*!;
> > >
> > > are you sure you did not make it more strict than what is allowed?
> > >
> > > e.g. if i had 'foo=<,bar=>' that would have previously worked, but now is forbidden AFAICS
> > Thing is, that would have not worked previously anyway. "Worked" in that
> > sense that any sensible LDAP server would have failed to parse or
> > outright rejected such DNs anyway, but could be configured using the
> > API/UI.
> >
> > Picking up on your example, "<" and ">" are both not allowed (at least
> > unquoted) in DN attribute values - see the docs patch again. But using
> > them properly quoted (e.g. foo="<",bar=">") worked before as does it
> > with the patch.
> >
> > I just tested this exact example (using an unpatched PVE) against a
> > (somewhat current, v2.5.13 as available in bullseye-backports) slapd
> > server for the sake of it - it fails when performing the search with
> > "invalid DN" - as expected.
> >
> > > while we can make such changes, we should only do so on major releases where it's a breaking
> > > change, preferably with a workaround and/or script where we can rewrite/warn the user
> > > that it's not valid syntax
> > >
> > > OTOH, most users probably won't notice since they did not use such 'strange' values
> > >
> > > the problem here is that possibly working configs are not valid anymore
> > > (for logins it's problematic, depending on how the admins log in)
> > Following up on the above, I'd hope no user has such configuration. And
> > if so, that user has to be using a completely bonkers LDAP
> > server/implementation.
> >
> > In conclusion, I don't see how this could break existing setups. But I
> > do see your point - breaking someones existing setup is a no-go. In that
> > case I would just hold onto this patchset for the next major release.
>
>
> ok i mistook the 'reserved' characters as reserved by us, not ldap.
> in such a case, when there is an external format/etc. please
> include a reference on where to find these restrictions
> (e.g. a link to an rfc)
For context; see RFC 2253 [0], section 4. Interestingly, this document
was obsoleted by RFC 4514 [1] in 2006, which only mentions this in
section 2.4 ("Converting an AttributeValue from ASN.1 to a String") and
and appendix A ("Presentation Issues").

But the first one seems to be the "authoritive" document on this matter,
at least looking at some other docs about LDAP DNs (RedHat, Microsoft, ..).

I will include that too in the commit next time.

[0] https://www.ietf.org/rfc/rfc2253.txt
[1] https://docs.ldap.com/specs/rfc4514.txt

>
> if my example and all that could have been configured but
> would now be invalid are not valid ldap syntax anyway, i think
> we can get more strict and "break" someones config
> (as you said, shouldn't have worked anyway)
> or how do you see that @thomas?
>
> (maybe there are some weirdly configured ldap instances out there?)
>
> [..]
>
>

Re: [pve-devel] [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values

[Thomas Lamprecht]

Am 15/03/2023 um 12:41 schrieb Dominik Csapak:
> if my example and all that could have been configured but
> would now be invalid are not valid ldap syntax anyway, i think
> we can get more strict and "break" someones config
> (as you said, shouldn't have worked anyway)
> or how do you see that @thomas?

yeah fine by me.

Breaking semantic is the one thing we want to avoid, technical breakages is
basically any change in some form and overly strict argument. And while not
all use cases (or implementations not following the spec one bases the decision
on) are known.

The argument is strong enough for me to go ahead with it, that is as long as
there's a pointer in the breaking changes section of release notes that is,
as then admins can find a reference to odd/changed behavior quickly and if
really an issue we can always fix it up then.

ps. trimming out unrelated quotes from mail replies is encouraged ;P

[pve-devel] applied: [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values

[Thomas Lamprecht]

Am 15/03/2023 um 10:54 schrieb Dominik Csapak:
> hi,
> 
> so high level comment:
> i'd write most of what you wrote in the cover letter here in the commit message,
> makes it much more convenient to find it only via git ;)
> 
> also i'm missing a bit the rationale for how the regex was chosen, besides
> that it works in some conditions
> 
> further comment inline
> 
> On 1/31/23 13:50, Christoph Heiss wrote:
>> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
>> ---
>>   src/PVE/Auth/LDAP.pm | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>>

applied, with Dominiks R-b/T-b and more info and references added to the commit
message, thanks!

[pve-devel] [PATCH docs 2/2] pveum: Document reserved characters and quoting of LDAP DNs

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 pveum.adoc | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/pveum.adoc b/pveum.adoc
index 65d874a..1562b6c 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -390,6 +390,39 @@ The main options for syncing are:
 * `Preview` (`dry-run`): No data is written to the config. This is useful if you
   want to see which users and groups would get synced to the `user.cfg`.

+[[pveum_ldap_reserved_characters]]
+Reserved characters
+^^^^^^^^^^^^^^^^^^^
+
+Certain characters are reserved and cannot be easily used in attribute values
+in DNs without being escaped properly.
+
+Following characters need escaping:
+
+* Space (` `)
+
+* Comma (`,`)
+
+* Plus sign (`+`)
+
+* Double quote (`"`)
+
+* Forward slashes (`/`)
+
+* Angle brackets (`<>`)
+
+* Semicolon (`;`)
+
+* Equals sign (`=`)
+
+To use such characters in DNs, surround the attribute value in double quotes.
+For example, to bind with a user with the CN (Common Name) `Example, User`, use
+`CN="Example, User",OU=people,DC=example,DC=com` as value for `bind_dn`.
+
+This applies to the `base_dn`, `bind_dn`, and `group_dn` attributes.
+
+NOTE: Users with colons and forward slashes cannot be synced since these are
+reserved characters in usernames.

 [[pveum_openid]]
 OpenID Connect
--
2.34.1

[pve-devel] applied: [PATCH docs 2/2] pveum: Document reserved characters and quoting of LDAP DNs

[Thomas Lamprecht]

Am 31/01/2023 um 13:50 schrieb Christoph Heiss:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  pveum.adoc | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
>

applied, dropped the extra newlines for the list and the backtick quoting on
the whitespace character to play safe as asciidoc seems to only partially handle
that well, thanks!

Re: [pve-devel] [PATCH access-control/docs 0/2] fix #3748: Allow reserved characters in attribute values of LDAP DNs

[Christoph Heiss]

Ping. It's a rather minor change and it would be nice to get this in
sometime, as this fixes an actual problem some users have :^)

On Tue, Jan 31, 2023 at 01:50:41PM +0100, Christoph Heiss wrote:
> This fixes #3748 [0] by allowing reserved characters in `bind_dn` (and
> other properties of the same format) if they are properly quoted and
> adds some corresponding documentation regarding that.
>
> This was tested by setting up a slapd server and creating a user with
> the CN `Test, User` much like in the bug report, then using this user as
> `bind_dn` in the sync options. I also tested some variants of that CN,
> including just `TestUser`.)
>
> One thing that still won't work is syncing of LDAP users with colons or
> slashes in their CNs. In such cases, the message
>
>   value 'Test, User@ldap' does not look like a valid user name
>
> will pop up.
>
> This is due to spaces and colons being explicitly disallowed in
> usernames [1]. This probably means that such names can never be allowed,
> which is being documented too as part of patch 2.
>
> But with this series, such users can be at least used to bind for
> syncing.
>
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=3748
> [1] https://git.proxmox.com/?p=pve-access-control.git;a=blob;f=src/PVE/Auth/Plugin.pm;hb=HEAD#l115
>
> Christoph Heiss (1):
>       ldap: Allow quoted values for DN attribute values
>
>  src/PVE/Auth/LDAP.pm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> Christoph Heiss (1):
>       pveum: Document reserved characters and quoting of LDAP DNs
>
>  pveum.adoc | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> --
> 2.34.1
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] [PATCH access-control/docs 0/2] fix #3748: Allow reserved characters in attribute values of LDAP DNs

[Dominik Csapak]

since my reservations about being too strict were unfounded
(since these configs wouldn't have worked in the first place)

i'll send a follow up for adding a link to the rfc shortly

consider this:

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>