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 3D2BEBA34 for ; Thu, 10 Aug 2023 09:55:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1E9441DBDE for ; Thu, 10 Aug 2023 09:55:54 +0200 (CEST) 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 ; Thu, 10 Aug 2023 09:55:53 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 075494481E for ; Thu, 10 Aug 2023 09:55:53 +0200 (CEST) Date: Thu, 10 Aug 2023 09:55:51 +0200 From: Wolfgang Bumiller To: Christoph Heiss Cc: pve-devel@lists.proxmox.com Message-ID: References: <20230801123734.446797-1-c.heiss@proxmox.com> <20230801123734.446797-3-c.heiss@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230801123734.446797-3-c.heiss@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.043 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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. [proxmox.com, ldap.pm, domains.pm, plugin.pm] Subject: Re: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check 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: Thu, 10 Aug 2023 07:55:54 -0000 On Tue, Aug 01, 2023 at 02:37:18PM +0200, Christoph Heiss wrote: > Removes the dreaded DN regex, instead introducing a optional > connect/bind check on creation/update, aligning it with the way PBS does > it. > > Additionally, it has the benefit that instead of letting a sync fail on > the first try due to e.g. bad bind credentials, it gives the user some > direct feedback when trying to add/update a LDAP realm, if enabled. > > Should be rather a corner case, but it's the easiest way for us to > accomodate and the most versatile for users needing this. > > This is part of the result of a previous discussion [0], and the same > approach is already implemented for PBS [1]. > > [0] https://lists.proxmox.com/pipermail/pve-devel/2023-May/056839.html > [1] https://git.proxmox.com/?p=proxmox-backup.git;a=commitdiff;h=5210f3b5 > > Signed-off-by: Christoph Heiss > --- > Changes v1 -> v2: > * Do not store the 'check-connection' parameter in the realm config > * Small fix; pass full config to check_connection() on update > > The decision to put this behind an optional (off-by-default) parameter > was made during an off-list discussion with Lukas. The summary of that > can be found as a follow-up on the previous series [2] [3]. > > Note that the `base_dn`/`group_dn` now goes completely unvalided - but > again, that is how it works in PBS. We /could/ use canonical_dn() for > this (see previous series [4]), depending on whether we want this kind > of "divergence" between PVE & PBS. > > [2] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058540.html > [3] https://lists.proxmox.com/pipermail/pve-devel/2023-August/058582.html > [4] https://lists.proxmox.com/pipermail/pve-devel/2023-July/058392.html > > src/PVE/API2/Domains.pm | 10 ++++++++++ > src/PVE/Auth/LDAP.pm | 25 ++++++++++++++++--------- > src/PVE/Auth/Plugin.pm | 8 ++++++++ > 3 files changed, 34 insertions(+), 9 deletions(-) > > diff --git a/src/PVE/API2/Domains.pm b/src/PVE/API2/Domains.pm > index 9332aa7..ed2b6c4 100644 > --- a/src/PVE/API2/Domains.pm > +++ b/src/PVE/API2/Domains.pm > @@ -133,6 +133,7 @@ __PACKAGE__->register_method ({ > > my $realm = extract_param($param, 'realm'); > my $type = $param->{type}; > + my $check_connection = extract_param($param, 'check-connection'); > > die "domain '$realm' already exists\n" > if $ids->{$realm}; > @@ -165,6 +166,10 @@ __PACKAGE__->register_method ({ > } > $plugin->on_add_hook($realm, $config, password => $password); > > + # Only for LDAP/AD, implied through the existence of the 'check-connection' param > + $plugin->check_connection($realm, $config, password => $password) > + if $check_connection; > + > cfs_write_file($domainconfigfile, $cfg); > }, "add auth server failed"); > > @@ -198,6 +203,7 @@ __PACKAGE__->register_method ({ > PVE::SectionConfig::assert_if_modified($cfg, $digest); > > my $realm = extract_param($param, 'realm'); > + my $check_connection = extract_param($param, 'check-connection'); > > die "domain '$realm' does not exist\n" > if !$ids->{$realm}; > @@ -237,6 +243,10 @@ __PACKAGE__->register_method ({ > $plugin->on_update_hook($realm, $config); > } > > + # Only for LDAP/AD, implied through the existence of the 'check-connection' param > + $plugin->check_connection($realm, $ids->{$realm}, password => $password) > + if $check_connection; > + > cfs_write_file($domainconfigfile, $cfg); > }, "update auth server failed"); > > diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm > index fc82a17..d5f4909 100755 > --- a/src/PVE/Auth/LDAP.pm > +++ b/src/PVE/Auth/LDAP.pm > @@ -10,9 +10,6 @@ use PVE::Tools; > > use base qw(PVE::Auth::Plugin); > > -my $dn_part_regex = qr!("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^ ,+"/<>;=#])!; > -our $dn_regex = qr!\w+=${dn_part_regex}(,\s*\w+=${dn_part_regex})*!; > - > sub type { > return 'ldap'; > } > @@ -22,7 +19,6 @@ sub properties { > base_dn => { > description => "LDAP base domain name", > type => 'string', > - pattern => $dn_regex, > optional => 1, > maxLength => 256, > }, > @@ -36,7 +32,6 @@ sub properties { > bind_dn => { > description => "LDAP bind domain name", > type => 'string', > - pattern => $dn_regex, > optional => 1, > maxLength => 256, > }, > @@ -94,7 +89,6 @@ sub properties { > description => "LDAP base domain name for group sync. If not set, the" > ." base_dn will be used.", > type => 'string', > - pattern => $dn_regex, > optional => 1, > maxLength => 256, > }, > @@ -137,7 +131,13 @@ sub properties { > type => 'boolean', > optional => 1, > default => 1, > - } > + }, > + 'check-connection' => { > + description => 'Check bind connection to LDAP server.', > + type => 'boolean', > + optional => 1, > + default => 0, > + }, While there's special handling for how we store the password, this schema here should still actually describe the stored config. Since this is a parameter specifically for the add/update API methods we should declare it in those functions as parameter. Some of our methods to get schemas have an optional hash parameter to include an extra set of base properties in its returned contents (see `get_standard_option` as an example), but `createSchema` and `updateSchema` do not. We could either add this, or, since this is currently only required once, just move the `{create,update}Schema` calls over the `register_method()` calls and modify them right there before use... Since this series already touches pve-common, I have a *slight* preference to extending the `create/updateSchema` subs in `PVE::SectionConfig`, although AFAICT the common patch does not strictly require a dependency bump inside pve-access-control as it mostly about how errors are presented to end-users (?), so either way is fine with me. If we update the SectionConfig we'll definitely need a versioned dependency bump. > }; > } > > @@ -169,6 +169,7 @@ sub options { > 'sync-defaults-options' => { optional => 1 }, > mode => { optional => 1 }, > 'case-sensitive' => { optional => 1 }, > + 'check-connection' => { optional => 1 }, > }; > } > > @@ -184,7 +185,7 @@ sub get_scheme_and_port { > } > > sub connect_and_bind { > - my ($class, $config, $realm) = @_; > + my ($class, $config, $realm, $param) = @_; > > my $servers = [$config->{server1}]; > push @$servers, $config->{server2} if $config->{server2}; > @@ -215,7 +216,7 @@ sub connect_and_bind { > > if ($config->{bind_dn}) { > my $bind_dn = $config->{bind_dn}; > - my $bind_pass = ldap_get_credentials($realm); > + my $bind_pass = $param->{password} || ldap_get_credentials($realm); > die "missing password for realm $realm\n" if !defined($bind_pass); > PVE::LDAP::ldap_bind($ldap, $bind_dn, $bind_pass); > } elsif ($config->{cert} && $config->{certkey}) { > @@ -454,4 +455,10 @@ sub on_delete_hook { > ldap_delete_credentials($realm); > } > > +sub check_connection { > + my ($class, $realm, $config, %param) = @_; > + > + $class->connect_and_bind($config, $realm, \%param); > +} > + > 1; > diff --git a/src/PVE/Auth/Plugin.pm b/src/PVE/Auth/Plugin.pm > index 2f64a13..429bc88 100755 > --- a/src/PVE/Auth/Plugin.pm > +++ b/src/PVE/Auth/Plugin.pm > @@ -317,4 +317,12 @@ sub on_delete_hook { > # do nothing by default > } > > +# called during addition and updates of realms (before the new domain config gets written) > +# die to abort addition/update in case the connection/bind fails > +# NOTE: runs in a storage config *locked* context > +sub check_connection { > + my ($class, $realm, $config, %param) = @_; > + # do nothing by default > +} > + > 1; > -- > 2.41.0