From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B4A8190673 for ; Wed, 15 Mar 2023 12:17:52 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 98A008DBC for ; Wed, 15 Mar 2023 12:17:52 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 15 Mar 2023 12:17:50 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5A3234041D for ; Wed, 15 Mar 2023 12:17:50 +0100 (CET) Date: Wed, 15 Mar 2023 12:17:48 +0100 From: Christoph Heiss To: Dominik Csapak Cc: Proxmox VE development discussion Message-ID: <20230315111748.irvdaowr73thr3o5@maui.proxmox.com> References: <20230131125043.380402-1-c.heiss@proxmox.com> <20230131125043.380402-2-c.heiss@proxmox.com> <3c2d120e-eb11-aa79-be1f-eba3879cd58a@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3c2d120e-eb11-aa79-be1f-eba3879cd58a@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.078 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [ldap.pm, proxmox.com] Subject: Re: [pve-devel] [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Mar 2023 11:17:52 -0000 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 > > --- > > 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 > > > > > >