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 C71589072A for ; Wed, 15 Mar 2023 13:07:45 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9DD54985C for ; Wed, 15 Mar 2023 13:07:15 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 15 Mar 2023 13:07:13 +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 7F63041601 for ; Wed, 15 Mar 2023 13:07:13 +0100 (CET) Date: Wed, 15 Mar 2023 13:07:11 +0100 From: Christoph Heiss To: Dominik Csapak Cc: Thomas Lamprecht , Proxmox VE development discussion Message-ID: <20230315120711.lk3xwydb4pyxdblq@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> <20230315111748.irvdaowr73thr3o5@maui.proxmox.com> <4837827c-33d4-b861-f45e-9e3531b3a99b@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4837827c-33d4-b861-f45e-9e3531b3a99b@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.077 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. [ietf.org, ldap.pm, ldap.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 12:07:45 -0000 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?) > > [..] > >