[pve-devel] [PATCH storage 2/3] refactor sensitive parameter handling

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

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 PVE/API2/Storage/Config.pm | 61 ++++++++++++++++++++------------------
 PVE/Storage/CIFSPlugin.pm  |  6 ++++
 PVE/Storage/PBSPlugin.pm   |  6 ++--
 3 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/PVE/API2/Storage/Config.pm b/PVE/API2/Storage/Config.pm
index 09c5d59..251b4af 100755
--- a/PVE/API2/Storage/Config.pm
+++ b/PVE/API2/Storage/Config.pm
@@ -112,6 +112,29 @@ __PACKAGE__->register_method ({
 	return &$api_storage_config($cfg, $param->{storage});
     }});
 
+my sub extract_sensitive_params :prototype($$) {
+    my ($param, $delete_list) = @_;
+
+    my $sensitive;
+
+    my %delete = map { $_ => 1 } ($delete_list || [])->@*;
+
+    # always extract pw and keys, so they don't get written to the www-data readable scfg
+    for my $opt (qw(password encryption_key)) {
+	# First handle deletions as explicitly setting `undef`, afterwards new values may override
+	# it.
+	if (exists($delete{$opt})) {
+	    $sensitive->{$opt} = undef;
+	}
+
+	if (defined(my $value = extract_param($param, $opt))) {
+	    $sensitive->{$opt} = $value;
+	}
+    }
+
+    return $sensitive;
+}
+
 __PACKAGE__->register_method ({
     name => 'create',
     protected => 1,
@@ -133,15 +156,7 @@ __PACKAGE__->register_method ({
 	# fix me in section config create never need an empty entity.
 	delete $param->{nodes} if !$param->{nodes};
 
-	my $password;
-	# always extract pw, else it gets written to the www-data readable scfg
-	if (my $tmp_pw = extract_param($param, 'password')) {
-	    if (($type eq 'pbs') || ($type eq 'cifs' && $param->{username})) {
-		$password = $tmp_pw;
-	    } else {
-		warn "ignore password parameter\n";
-	    }
-	}
+	my $sensitive = extract_sensitive_params($param, []);
 
 	my $plugin = PVE::Storage::Plugin->lookup($type);
 	my $opts = $plugin->check_config($storeid, $param, 1, 1);
@@ -157,7 +172,7 @@ __PACKAGE__->register_method ({
 
 		$cfg->{ids}->{$storeid} = $opts;
 
-		$plugin->on_add_hook($storeid, $opts, password => $password);
+		$plugin->on_add_hook($storeid, $opts, %$sensitive);
 
 		eval {
 		    # try to activate if enabled on local node,
@@ -197,6 +212,10 @@ __PACKAGE__->register_method ({
 	my $digest = extract_param($param, 'digest');
 	my $delete = extract_param($param, 'delete');
 
+	if ($delete) {
+	    $delete = [ PVE::Tools::split_list($delete) ];
+	}
+
         PVE::Storage::lock_storage_config(sub {
 
 	    my $cfg = PVE::Storage::config();
@@ -206,24 +225,14 @@ __PACKAGE__->register_method ({
 	    my $scfg = PVE::Storage::storage_config($cfg, $storeid);
 	    my $type = $scfg->{type};
 
-	    my $password;
-	    # always extract pw, else it gets written to the www-data readable scfg
-	    if (my $tmp_pw = extract_param($param, 'password')) {
-		if (($type eq 'pbs') || ($type eq 'cifs' && $param->{username})) {
-		    $password = $tmp_pw;
-		} else {
-		    warn "ignore password parameter\n";
-		}
-	    }
+	    my $sensitive = extract_sensitive_params($param, $delete);
 
 	    my $plugin = PVE::Storage::Plugin->lookup($type);
 	    my $opts = $plugin->check_config($storeid, $param, 0, 1);
 
-	    my $delete_password = 0;
-
 	    if ($delete) {
 		my $options = $plugin->private()->{options}->{$type};
-		foreach my $k (PVE::Tools::split_list($delete)) {
+		foreach my $k (@$delete) {
 		    my $d = $options->{$k} || die "no such option '$k'\n";
 		    die "unable to delete required option '$k'\n" if !$d->{optional};
 		    die "unable to delete fixed option '$k'\n" if $d->{fixed};
@@ -231,16 +240,10 @@ __PACKAGE__->register_method ({
 			if defined($opts->{$k});
 
 		    delete $scfg->{$k};
-
-		    $delete_password = 1 if $k eq 'password';
 		}
 	    }
 
-	    if ($delete_password || defined($password)) {
-		$plugin->on_update_hook($storeid, $opts, password => $password);
-	    } else {
-		$plugin->on_update_hook($storeid, $opts);
-	    }
+	    $plugin->on_update_hook($storeid, $opts, %$sensitive);
 
 	    for my $k (keys %$opts) {
 		$scfg->{$k} = $opts->{$k};
diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index 0eb1722..8de234b 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -161,6 +161,9 @@ sub on_add_hook {
 
     if (defined($param{password})) {
 	cifs_set_credentials($param{password}, $storeid);
+	if (!exists($scfg->{username})) {
+	    warn "ignoring password parameter\n";
+	}
     } else {
 	cifs_delete_credentials($storeid);
     }
@@ -173,6 +176,9 @@ sub on_update_hook {
 
     if (defined($param{password})) {
 	cifs_set_credentials($param{password}, $storeid);
+	if (!exists($scfg->{username})) {
+	    warn "ignoring password parameter\n";
+	}
     } else {
 	cifs_delete_credentials($storeid);
     }
diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index 31af450..10e4fbc 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -266,7 +266,7 @@ sub on_add_hook {
 	pbs_delete_password($scfg, $storeid);
     }
 
-    if (defined(my $encryption_key = delete($scfg->{encryption_key}))) {
+    if (defined(my $encryption_key = $param{encryption_key})) {
 	pbs_set_encryption_key($scfg, $storeid, $encryption_key);
     } else {
 	pbs_delete_encryption_key($scfg, $storeid);
@@ -284,8 +284,8 @@ sub on_update_hook {
 	}
     }
 
-    if (exists($scfg->{encryption_key})) {
-	if (defined(my $encryption_key = delete($scfg->{encryption_key}))) {
+    if (exists($param{encryption_key})) {
+	if (defined(my $encryption_key = delete($param{encryption_key}))) {
 	    pbs_set_encryption_key($scfg, $storeid, $encryption_key);
 	} else {
 	    pbs_delete_encryption_key($scfg, $storeid);
-- 
2.20.1