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

[Dominik Csapak <d.csapak@proxmox.com>]

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
>>>
>>>
>>
>>