Re: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 <c.heiss@proxmox.com>
> ---
> 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