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.he...@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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
