[pve-devel] applied: [PATCH access-control v2] auth ldap/ad: make password a parameter for the api

[Thomas Lamprecht]

On 4/8/20 9:00 AM, Dominik Csapak wrote:
> Instead of simply requiring it to exist in /etc/pve.
> 
> Takes after the password handling of CIFS in pve-storage.
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from v1:
> * use 'realm' dir instead of 'ldap' (with fallback and comment to removal)
> 

applied, but I split it as adding the on_{add,update,delete}_hook infrastructure
has to be it's own commit, it has per se nothing to do with the ldap password
stuff.

Also, while copy+adapting this from pve-storage is really nice IMO, I do not
understand at all why you left out or changed things then unnecessarily - I mean
missing documentation about how the hooks behave, some really strange $check
parameter (arbitrary non-saying name) which is passed by all callers with a
"FIXME remove in 7.0" instead of the single place it should have be addressed.

That's taking code, boiling and whirling it to get Spaghetti outta it, and
that should be kept to kitchens :)

Anyway, this was long overdue, so thanks for providing the inertia and patch
to address it.

>  PVE/API2/Domains.pm | 26 +++++++++++++++
>  PVE/Auth/AD.pm      |  1 +
>  PVE/Auth/LDAP.pm    | 80 ++++++++++++++++++++++++++++++++++++++++++++-
>  PVE/Auth/Plugin.pm  | 15 +++++++++
>  4 files changed, 121 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Domains.pm b/PVE/API2/Domains.pm
> index 8ae1db0..b25a5fe 100644
> --- a/PVE/API2/Domains.pm
> +++ b/PVE/API2/Domains.pm
> @@ -88,6 +88,9 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> +	# always extract, add it with hook
> +	my $password = extract_param($param, 'password');
> +
>  	PVE::Auth::Plugin::lock_domain_config(
>  	    sub {
>  
> @@ -117,6 +120,13 @@ __PACKAGE__->register_method ({
>  
>  		$ids->{$realm} = $config;
>  
> +		my $opts = $plugin->options();
> +		if (defined($password) && !defined($opts->{password})) {
> +		    $password = undef;
> +		    warn "ignoring password parameter";
> +		}
> +		$plugin->on_add_hook($realm, $config, password => $password);
> +
>  		cfs_write_file($domainconfigfile, $cfg);
>  	    }, "add auth server failed");
>  
> @@ -137,6 +147,9 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> +	# always extract, update in hook
> +	my $password = extract_param($param, 'password');
> +
>  	PVE::Auth::Plugin::lock_domain_config(
>  	    sub {
>  
> @@ -154,8 +167,10 @@ __PACKAGE__->register_method ({
>  		my $delete_str = extract_param($param, 'delete');
>  		die "no options specified\n" if !$delete_str && !scalar(keys %$param);
>  
> +		my $delete_pw = 0;
>  		foreach my $opt (PVE::Tools::split_list($delete_str)) {
>  		    delete $ids->{$realm}->{$opt};
> +		    $delete_pw = 1 if $opt eq 'password';
>  		}
>  
>  		my $plugin = PVE::Auth::Plugin->lookup($ids->{$realm}->{type});
> @@ -171,6 +186,14 @@ __PACKAGE__->register_method ({
>  		    $ids->{$realm}->{$p} = $config->{$p};
>  		}
>  
> +		my $opts = $plugin->options();
> +		if ($delete_pw || ($opts->{password} && defined($password))) {
> +		    $plugin->on_update_hook($realm, $config, password => $password);
> +		} else {
> +		    warn "ignoring password parameter" if defined($password);
> +		    $plugin->on_update_hook($realm, $config);
> +		}
> +
>  		cfs_write_file($domainconfigfile, $cfg);
>  	    }, "update auth server failed");
>  
> @@ -238,6 +261,9 @@ __PACKAGE__->register_method ({
>  
>  		die "domain '$realm' does not exist\n" if !$ids->{$realm};
>  
> +		my $plugin = PVE::Auth::Plugin->lookup($ids->{$realm}->{type});
> +		$plugin->on_delete_hook($realm, $ids->{$realm});
> +
>  		delete $ids->{$realm};
>  
>  		cfs_write_file($domainconfigfile, $cfg);
> diff --git a/PVE/Auth/AD.pm b/PVE/Auth/AD.pm
> index 4f40be3..24b0e9f 100755
> --- a/PVE/Auth/AD.pm
> +++ b/PVE/Auth/AD.pm
> @@ -83,6 +83,7 @@ sub options {
>  	certkey => { optional => 1 },
>  	base_dn => { optional => 1 },
>  	bind_dn => { optional => 1 },
> +	password => { optional => 1 },
>  	user_attr => { optional => 1 },
>  	filter => { optional => 1 },
>  	sync_attributes => { optional => 1 },
> diff --git a/PVE/Auth/LDAP.pm b/PVE/Auth/LDAP.pm
> index 905cc47..315705e 100755
> --- a/PVE/Auth/LDAP.pm
> +++ b/PVE/Auth/LDAP.pm
> @@ -37,6 +37,11 @@ sub properties {
>  	    optional => 1,
>  	    maxLength => 256,
>  	},
> +	password => {
> +	    description => "LDAP bind password. Will be stored in '/etc/pve/priv/realm/<REALM>.pw'.",
> +	    type => 'string',
> +	    optional => 1,
> +	},
>  	verify => {
>  	    description => "Verify the server's SSL certificate",
>  	    type => 'boolean',
> @@ -126,6 +131,7 @@ sub options {
>  	server2 => { optional => 1 },
>  	base_dn => {},
>  	bind_dn => { optional => 1 },
> +	password => { optional => 1 },
>  	user_attr => {},
>  	port => { optional => 1 },
>  	secure => { optional => 1 },
> @@ -185,7 +191,7 @@ sub connect_and_bind {
>  
>      if ($config->{bind_dn}) {
>  	$bind_dn = $config->{bind_dn};
> -	$bind_pass = PVE::Tools::file_read_firstline("/etc/pve/priv/ldap/${realm}.pw");
> +	$bind_pass = ldap_get_credentials($realm);
>  	die "missing password for realm $realm\n" if !defined($bind_pass);
>      }
>  
> @@ -343,4 +349,76 @@ sub authenticate_user {
>      return 1;
>  }
>  
> +my $ldap_pw_dir = "/etc/pve/priv/realm";
> +
> +# check paramter should be removed and pw file should be moved with 7.0
> +sub ldap_cred_filename {
> +    my ($realm, $check) = @_;
> +
> +    my $file = "${ldap_pw_dir}/${realm}.pw";
> +    if ($check && ! -e $file) {
> +	$file = "/etc/pve/priv/ldap/${realm}.pw";
> +    }
> +
> +    return $file;
> +}
> +
> +sub ldap_set_credentials {
> +    my ($password, $realm) = @_;
> +
> +    my $file = ldap_cred_filename($realm);
> +    mkdir $ldap_pw_dir;
> +
> +    PVE::Tools::file_set_contents($file, $password);
> +
> +    return $file;
> +}
> +
> +sub ldap_get_credentials {
> +    my ($realm) = @_;
> +
> +    my $file = ldap_cred_filename($realm, 1); # remove check param with 7.0
> +
> +    if (-e $file) {
> +	return PVE::Tools::file_read_firstline($file);
> +    }
> +    return undef;
> +}
> +
> +sub ldap_delete_credentials {
> +    my ($realm) = @_;
> +
> +    my $file = ldap_cred_filename($realm, 1); # remove check param with 7.0
> +
> +    unlink($file) or warn "removing ldap credentials '$file' failed: $!\n";
> +}
> +
> +sub on_add_hook {
> +    my ($class, $realm, $config, %param) = @_;
> +
> +    if (defined($param{password})) {
> +	ldap_set_credentials($param{password}, $realm);
> +    } else {
> +	ldap_delete_credentials($realm);
> +    }
> +}
> +
> +sub on_update_hook {
> +    my ($class, $realm, $config, %param) = @_;
> +
> +    return if !exists($param{password});
> +
> +    if (defined($param{password})) {
> +	ldap_set_credentials($param{password}, $realm);
> +    } else {
> +	ldap_delete_credentials($realm);
> +    }
> +}
> +
> +sub on_delete_hook {
> +    my ($class, $realm, $config) = @_;
> +
> +    ldap_delete_credentials($realm);
> +}
> +
>  1;
> diff --git a/PVE/Auth/Plugin.pm b/PVE/Auth/Plugin.pm
> index 7a08d27..7843599 100755
> --- a/PVE/Auth/Plugin.pm
> +++ b/PVE/Auth/Plugin.pm
> @@ -268,4 +268,19 @@ sub delete_user {
>      # do nothing by default
>  }
>  
> +sub on_add_hook {
> +    my ($class, $realm, $config, %param) = @_;
> +    # do nothing by default
> +}
> +
> +sub on_update_hook {
> +    my ($class, $realm, $config, %param) = @_;
> +    # do nothing by default
> +}
> +
> +sub on_delete_hook {
> +    my ($class, $realm, $config) = @_;
> +    # do nothing by default
> +}
> +
>  1;
>