* [pve-devel] [PATCH common] fix #5034 ldap attribute regex @ 2023-11-15 12:23 Markus Frank 2023-11-15 13:28 ` Stefan Sterz 2023-11-15 13:30 ` Thomas Lamprecht 0 siblings, 2 replies; 8+ messages in thread From: Markus Frank @ 2023-11-15 12:23 UTC (permalink / raw) To: pve-devel Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/" to allow hyphen in ldap attribute names for pve & pmg. Signed-off-by: Markus Frank <m.frank@proxmox.com> --- There does not seem to be a regex for LDAP attributes in pbs. Should a regex be added for this? src/PVE/JSONSchema.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm index 49e0d7a..ef58b62 100644 --- a/src/PVE/JSONSchema.pm +++ b/src/PVE/JSONSchema.pm @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr', \&verify_ldap_simple_attr); sub verify_ldap_simple_attr { my ($attr, $noerr) = @_; - if ($attr =~ m/^[a-zA-Z0-9]+$/) { + if ($attr =~ m/^[a-zA-Z0-9\-]+$/) { return $attr; } -- 2.39.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH common] fix #5034 ldap attribute regex 2023-11-15 12:23 [pve-devel] [PATCH common] fix #5034 ldap attribute regex Markus Frank @ 2023-11-15 13:28 ` Stefan Sterz 2023-11-15 14:49 ` Thomas Lamprecht 2023-11-15 15:02 ` Stefan Sterz 2023-11-15 13:30 ` Thomas Lamprecht 1 sibling, 2 replies; 8+ messages in thread From: Stefan Sterz @ 2023-11-15 13:28 UTC (permalink / raw) To: Proxmox VE development discussion, Markus Frank On 15.11.23 13:23, Markus Frank wrote: > Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/" > to allow hyphen in ldap attribute names for pve & pmg. > > Signed-off-by: Markus Frank <m.frank@proxmox.com> > --- > There does not seem to be a regex for LDAP attributes in pbs. > Should a regex be added for this? > we recently moved away from using regex for validating a LDAP configuration, for two reasons: 1. turns out finding a regex that validates all possible valid LDAP DNs is pretty hard and fixing this often comes along with breaking older setups 2. even a valid *looking* DN may not actual work against a real LDAP server. so instead we now actually try to query an LDAP server with the provided config and see if that returns anything. thus, users are more likely to get what they want, and we don't have to validate for every possible case. i guess there could be an argument why a regex here makes sense. however, i'd imagine it's a little odd for users that we are stricter here than we are with DNs. > src/PVE/JSONSchema.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm > index 49e0d7a..ef58b62 100644 > --- a/src/PVE/JSONSchema.pm > +++ b/src/PVE/JSONSchema.pm > @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr', \&verify_ldap_simple_attr); > sub verify_ldap_simple_attr { > my ($attr, $noerr) = @_; > > - if ($attr =~ m/^[a-zA-Z0-9]+$/) { > + if ($attr =~ m/^[a-zA-Z0-9\-]+$/) { if i'm not mistaken, this regex should try to filter an `AttributeValue` [1]. in case we do stick with this regex approach here, you may want to relax this even further, as per the standard: > If that UTF-8-encoded Unicode string does not have any of the > following characters that need escaping, then that string can be used > as the string representation > of the value. > > - a space (' ' U+0020) or number sign ('#' U+0023) occurring at > the beginning of the string; > > - a space (' ' U+0020) character occurring at the end of the > string; > > - one of the characters '"', '+', ',', ';', '<', '>', or '\' > (U+0022, U+002B, U+002C, U+003B, U+003C, U+003E, or U+005C, > respectively); > > - the null (U+0000) character. > > Other characters may be escaped. > return $attr; > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH common] fix #5034 ldap attribute regex 2023-11-15 13:28 ` Stefan Sterz @ 2023-11-15 14:49 ` Thomas Lamprecht 2023-11-15 15:12 ` Stefan Sterz 2023-11-15 15:02 ` Stefan Sterz 1 sibling, 1 reply; 8+ messages in thread From: Thomas Lamprecht @ 2023-11-15 14:49 UTC (permalink / raw) To: Proxmox VE development discussion, Stefan Sterz, Markus Frank Am 15/11/2023 um 14:28 schrieb Stefan Sterz: > On 15.11.23 13:23, Markus Frank wrote: >> Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/" >> to allow hyphen in ldap attribute names for pve & pmg. >> >> Signed-off-by: Markus Frank <m.frank@proxmox.com> >> --- >> There does not seem to be a regex for LDAP attributes in pbs. >> Should a regex be added for this? >> > > we recently moved away from using regex for validating a LDAP > configuration, for two reasons: > > 1. turns out finding a regex that validates all possible valid LDAP DNs > is pretty hard and fixing this often comes along with breaking older > setups > 2. even a valid *looking* DN may not actual work against a real LDAP > server. > > so instead we now actually try to query an LDAP server with the provided > config and see if that returns anything. thus, users are more likely to > get what they want, and we don't have to validate for every possible case. > > i guess there could be an argument why a regex here makes sense. > however, i'd imagine it's a little odd for users that we are stricter > here than we are with DNs. thanks for your input (didn't see your reply before sending mine) > >> src/PVE/JSONSchema.pm | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm >> index 49e0d7a..ef58b62 100644 >> --- a/src/PVE/JSONSchema.pm >> +++ b/src/PVE/JSONSchema.pm >> @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr', \&verify_ldap_simple_attr); >> sub verify_ldap_simple_attr { >> my ($attr, $noerr) = @_; >> >> - if ($attr =~ m/^[a-zA-Z0-9]+$/) { >> + if ($attr =~ m/^[a-zA-Z0-9\-]+$/) { > > if i'm not mistaken, this regex should try to filter an `AttributeValue` > [1]. in case we do stick with this regex approach here, you may want to > relax this even further, as per the standard: > >> If that UTF-8-encoded Unicode string does not have any of the >> following characters that need escaping, then that string can be used >> as the string representation >> of the value. >> >> - a space (' ' U+0020) or number sign ('#' U+0023) occurring at >> the beginning of the string; >> >> - a space (' ' U+0020) character occurring at the end of the >> string; >> >> - one of the characters '"', '+', ',', ';', '<', '>', or '\' >> (U+0022, U+002B, U+002C, U+003B, U+003C, U+003E, or U+005C, >> respectively); >> >> - the null (U+0000) character. >> Ack, so I was wrong, the format might still make sense albeit checking for above cases would then indeed better, something along the lines of: if ($attr !~ /(?:^(?:\s|#))|["+,;<>\0\\]|(?:\s$)/) { return $attr; } If we leave that regex out completely we should ensure that we don't get any tainting issues. The format could move to PVE::Auth::LDAP too, FWIW, but that's a different story. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH common] fix #5034 ldap attribute regex 2023-11-15 14:49 ` Thomas Lamprecht @ 2023-11-15 15:12 ` Stefan Sterz 2023-11-15 15:48 ` Thomas Lamprecht 0 siblings, 1 reply; 8+ messages in thread From: Stefan Sterz @ 2023-11-15 15:12 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion, Markus Frank On 15.11.23 15:49, Thomas Lamprecht wrote: > Am 15/11/2023 um 14:28 schrieb Stefan Sterz: >> On 15.11.23 13:23, Markus Frank wrote: -- >8 snip 8< -- >> >>> src/PVE/JSONSchema.pm | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm >>> index 49e0d7a..ef58b62 100644 >>> --- a/src/PVE/JSONSchema.pm >>> +++ b/src/PVE/JSONSchema.pm >>> @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr', \&verify_ldap_simple_attr); >>> sub verify_ldap_simple_attr { >>> my ($attr, $noerr) = @_; >>> >>> - if ($attr =~ m/^[a-zA-Z0-9]+$/) { >>> + if ($attr =~ m/^[a-zA-Z0-9\-]+$/) { >> >> if i'm not mistaken, this regex should try to filter an `AttributeValue` >> [1]. in case we do stick with this regex approach here, you may want to >> relax this even further, as per the standard: >> >>> If that UTF-8-encoded Unicode string does not have any of the >>> following characters that need escaping, then that string can be used >>> as the string representation >>> of the value. >>> >>> - a space (' ' U+0020) or number sign ('#' U+0023) occurring at >>> the beginning of the string; >>> >>> - a space (' ' U+0020) character occurring at the end of the >>> string; >>> >>> - one of the characters '"', '+', ',', ';', '<', '>', or '\' >>> (U+0022, U+002B, U+002C, U+003B, U+003C, U+003E, or U+005C, >>> respectively); >>> >>> - the null (U+0000) character. >>> > > Ack, so I was wrong, the format might still make sense albeit checking > for above cases would then indeed better, something along the lines of: > > if ($attr !~ /(?:^(?:\s|#))|["+,;<>\0\\]|(?:\s$)/) { > return $attr; > } > > If we leave that regex out completely we should ensure that we don't get > any tainting issues. > > The format could move to PVE::Auth::LDAP too, FWIW, but that's a different > story. just to through this out there, my last attempt at validating this [1] looked something like this: ``` my $escaped = qr!\\(?:[ "#+,;<=>\\]|[0-9a-fA-F]{2})!; my $start = qr!(?:${escaped}|[^"+,;<>\\\0 #])!; my $middle = qr!(?:${escaped}|[^"+,;<>\\\0])!; my $end = qr!(?:${escaped}|[^"+,;<>\\\0 ])!; my $attr_val = qr!("[^"]+"|${start}(?:${middle}*${end})?)!; ``` since things can also be escaped or in quotes, which makes them valid again. could probably be improved here, though. [1]: https://lists.proxmox.com/pipermail/pve-devel/2023-May/056840.html ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH common] fix #5034 ldap attribute regex 2023-11-15 15:12 ` Stefan Sterz @ 2023-11-15 15:48 ` Thomas Lamprecht 0 siblings, 0 replies; 8+ messages in thread From: Thomas Lamprecht @ 2023-11-15 15:48 UTC (permalink / raw) To: Proxmox VE development discussion, Stefan Sterz, Markus Frank Am 15/11/2023 um 16:12 schrieb Stefan Sterz: > just to through this out there, my last attempt at validating this [1] > looked something like this: > > ``` > my $escaped = qr!\\(?:[ "#+,;<=>\\]|[0-9a-fA-F]{2})!; > my $start = qr!(?:${escaped}|[^"+,;<>\\\0 #])!; > my $middle = qr!(?:${escaped}|[^"+,;<>\\\0])!; > my $end = qr!(?:${escaped}|[^"+,;<>\\\0 ])!; > my $attr_val = qr!("[^"]+"|${start}(?:${middle}*${end})?)!; > ``` > > since things can also be escaped or in quotes, which makes them valid > again. could probably be improved here, though. > > [1]: https://lists.proxmox.com/pipermail/pve-devel/2023-May/056840.html Thanks for the pointer, memories are comming back now.. It seems like `$attr =~ /^(.*)$/ && return $1` (if even needed to avoid tainting) seems the more practical solution here... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH common] fix #5034 ldap attribute regex 2023-11-15 13:28 ` Stefan Sterz 2023-11-15 14:49 ` Thomas Lamprecht @ 2023-11-15 15:02 ` Stefan Sterz 1 sibling, 0 replies; 8+ messages in thread From: Stefan Sterz @ 2023-11-15 15:02 UTC (permalink / raw) To: Proxmox VE development discussion, Markus Frank On 15.11.23 14:28, Stefan Sterz wrote: >> src/PVE/JSONSchema.pm | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm >> index 49e0d7a..ef58b62 100644 >> --- a/src/PVE/JSONSchema.pm >> +++ b/src/PVE/JSONSchema.pm >> @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr', \&verify_ldap_simple_attr); >> sub verify_ldap_simple_attr { >> my ($attr, $noerr) = @_; >> >> - if ($attr =~ m/^[a-zA-Z0-9]+$/) { >> + if ($attr =~ m/^[a-zA-Z0-9\-]+$/) { > > if i'm not mistaken, this regex should try to filter an `AttributeValue` > [1]. in case we do stick with this regex approach here, you may want to > relax this even further, as per the standard: > sorry just noticed i forgot to add: [1]: https://datatracker.ietf.org/doc/html/rfc4514#section-2.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH common] fix #5034 ldap attribute regex 2023-11-15 12:23 [pve-devel] [PATCH common] fix #5034 ldap attribute regex Markus Frank 2023-11-15 13:28 ` Stefan Sterz @ 2023-11-15 13:30 ` Thomas Lamprecht 2023-11-21 12:55 ` Christoph Heiss 1 sibling, 1 reply; 8+ messages in thread From: Thomas Lamprecht @ 2023-11-15 13:30 UTC (permalink / raw) To: Proxmox VE development discussion, Markus Frank Am 15/11/2023 um 13:23 schrieb Markus Frank: > Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/" > to allow hyphen in ldap attribute names for pve & pmg. > > Signed-off-by: Markus Frank <m.frank@proxmox.com> > --- > There does not seem to be a regex for LDAP attributes in pbs. > Should a regex be added for this? > > src/PVE/JSONSchema.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm > index 49e0d7a..ef58b62 100644 > --- a/src/PVE/JSONSchema.pm > +++ b/src/PVE/JSONSchema.pm > @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr', \&verify_ldap_simple_attr); > sub verify_ldap_simple_attr { > my ($attr, $noerr) = @_; > > - if ($attr =~ m/^[a-zA-Z0-9]+$/) { > + if ($attr =~ m/^[a-zA-Z0-9\-]+$/) { Pre-existing, but shouldn't the regex actually be? $attr =~ m/^[a-zA-Z][a-zA-Z0-9\-]*$/ I.e., start with a letter and then be any of letter, digit or hyphen (minus). CCing Christoph, you did a bit more LDAP stuff recently - opinions? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH common] fix #5034 ldap attribute regex 2023-11-15 13:30 ` Thomas Lamprecht @ 2023-11-21 12:55 ` Christoph Heiss 0 siblings, 0 replies; 8+ messages in thread From: Christoph Heiss @ 2023-11-21 12:55 UTC (permalink / raw) To: Thomas Lamprecht; +Cc: Proxmox VE development discussion, Markus Frank On Wed, Nov 15, 2023 at 02:30:06PM +0100, Thomas Lamprecht wrote: > > Am 15/11/2023 um 13:23 schrieb Markus Frank: > > Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/" > > to allow hyphen in ldap attribute names for pve & pmg. > > [..] > > --- a/src/PVE/JSONSchema.pm > > +++ b/src/PVE/JSONSchema.pm > > @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr', \&verify_ldap_simple_attr); > > sub verify_ldap_simple_attr { > > my ($attr, $noerr) = @_; > > > > - if ($attr =~ m/^[a-zA-Z0-9]+$/) { > > + if ($attr =~ m/^[a-zA-Z0-9\-]+$/) { > > Pre-existing, but shouldn't the regex actually be? > > $attr =~ m/^[a-zA-Z][a-zA-Z0-9\-]*$/ > > I.e., start with a letter and then be any of letter, digit or hyphen (minus). > > CCing Christoph, you did a bit more LDAP stuff recently - opinions? Sorry for the late reply, just saw this now. I'd definitely agree with Stefan here, that moving away from regex's for validating LDAP DNs/attributes/etc is the right way, instead of continuously having to fix them up. Even if we try to follow the RFCs as closely as possible, that does unfortunaly still not really guarantee that it is indeed valid and will be _accepted by the server_. Just doing a basic sanity check (e.g. not empty, no spaces) and then querying the actual LDAP server whether that accepts it or not would be IMHO preferable. Plus, it's less work on our side in the long run. PBS does it no differently too, so I'd go the same way here too. Being overly strict does not help anyone, especially with LDAP, which does a lot of weird things. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-11-21 12:56 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-15 12:23 [pve-devel] [PATCH common] fix #5034 ldap attribute regex Markus Frank 2023-11-15 13:28 ` Stefan Sterz 2023-11-15 14:49 ` Thomas Lamprecht 2023-11-15 15:12 ` Stefan Sterz 2023-11-15 15:48 ` Thomas Lamprecht 2023-11-15 15:02 ` Stefan Sterz 2023-11-15 13:30 ` Thomas Lamprecht 2023-11-21 12:55 ` Christoph Heiss
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox