[pve-devel] [PATCH storage 1/3] pbs: encryption support, split "raw client command" API

[pve-devel] [PATCH storage 1/3] pbs: encryption support, split "raw client command" API

[Wolfgang Bumiller]

(And deprecate it...)

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 PVE/Storage/PBSPlugin.pm | 126 +++++++++++++++++++++++++++++++++------
 1 file changed, 109 insertions(+), 17 deletions(-)

diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index 092d800..31af450 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -4,12 +4,12 @@ package PVE::Storage::PBSPlugin;
 
 use strict;
 use warnings;
-use POSIX qw(strftime);
-use IO::File;
+use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
 use HTTP::Request;
-use LWP::UserAgent;
+use IO::File;
 use JSON;
-use Data::Dumper; # fixme: remove
+use LWP::UserAgent;
+use POSIX qw(strftime ENOENT);
 
 use PVE::Tools qw(run_command file_read_firstline trim dir_glob_regex dir_glob_foreach);
 use PVE::Storage::Plugin;
@@ -37,6 +37,10 @@ sub properties {
 	},
 	# openssl s_client -connect <host>:8007 2>&1 |openssl x509 -fingerprint -sha256
 	fingerprint => get_standard_option('fingerprint-sha256'),
+	encryption_key => {
+	    description => "Encryption key.",
+	    type => 'string',
+	},
     };
 }
 
@@ -49,6 +53,7 @@ sub options {
 	content => { optional => 1},
 	username => { optional => 1 },
 	password => { optional => 1},
+	encryption_key => { optional => 1 },
 	maxfiles => { optional => 1 },
 	fingerprint => { optional => 1 },
     };
@@ -87,6 +92,52 @@ sub pbs_get_password {
     return PVE::Tools::file_read_firstline($pwfile);
 }
 
+sub pbs_encryption_key_file_name {
+    my ($scfg, $storeid) = @_;
+
+    return "/etc/pve/priv/storage/${storeid}.enc";
+}
+
+sub pbs_set_encryption_key {
+    my ($scfg, $storeid, $key) = @_;
+
+    my $pwfile = pbs_encryption_key_file_name($scfg, $storeid);
+    mkdir "/etc/pve/priv/storage";
+
+    PVE::Tools::file_set_contents($pwfile, "$key\n");
+}
+
+sub pbs_delete_encryption_key {
+    my ($scfg, $storeid) = @_;
+
+    my $pwfile = pbs_encryption_key_file_name($scfg, $storeid);
+
+    unlink $pwfile;
+}
+
+sub pbs_get_encryption_key {
+    my ($scfg, $storeid) = @_;
+
+    my $pwfile = pbs_encryption_key_file_name($scfg, $storeid);
+
+    return PVE::Tools::file_get_contents($pwfile);
+}
+
+# Returns a file handle if there is an encryption key, or `undef` if there is not. Dies on error.
+sub pbs_open_encryption_key {
+    my ($scfg, $storeid) = @_;
+
+    my $encryption_key_file = pbs_encryption_key_file_name($scfg, $storeid);
+
+    my $keyfd;
+    if (!open($keyfd, '<', $encryption_key_file)) {
+	return undef if $! == ENOENT;
+	die "failed to open encryption key: $encryption_key_file: $!\n";
+    }
+
+    return $keyfd;
+}
+
 sub print_volid {
     my ($storeid, $btype, $bid, $btime) = @_;
 
@@ -96,8 +147,8 @@ sub print_volid {
     return "${storeid}:${volname}";
 }
 
-sub run_raw_client_cmd {
-    my ($scfg, $storeid, $client_cmd, $param, %opts) = @_;
+my sub do_raw_client_cmd {
+    my ($scfg, $storeid, $client_cmd, $param, $can_encrypt, %opts) = @_;
 
     my $client_exe = '/usr/bin/proxmox-backup-client';
     die "executable not found '$client_exe'! Proxmox backup client not installed?\n"
@@ -115,6 +166,20 @@ sub run_raw_client_cmd {
 
     push @$cmd, $client_exe, $client_cmd;
 
+    # This must live in the top scope to not get closed before the `run_command`
+    my $keyfd;
+    if ($can_encrypt) {
+	if (defined($keyfd = pbs_open_encryption_key($scfg, $storeid))) {
+	    my $flags = fcntl($keyfd, F_GETFD, 0)
+		// die "failed to get file descriptor flags: $!\n";
+	    fcntl($keyfd, F_SETFD, $flags & ~FD_CLOEXEC)
+		or die "failed to remove FD_CLOEXEC from encryption key file descriptor\n";
+	    push @$cmd, '--crypt-mode=encrypt', '--keyfd='.fileno($keyfd);
+	} else {
+	    push @$cmd, '--crypt-mode=none';
+	}
+    }
+
     push @$cmd, @$param if defined($param);
 
     push @$cmd, "--repository", "$username\@$server:$datastore";
@@ -134,6 +199,18 @@ sub run_raw_client_cmd {
     run_command($cmd, %opts);
 }
 
+# FIXME: External perl code should NOT have access to this.
+#
+# There should be separate functions to
+# - make backups
+# - restore backups
+# - restore files
+# with a sane API
+sub run_raw_client_cmd{
+    my ($scfg, $storeid, $client_cmd, $param, %opts) = @_;
+    return do_raw_client_cmd($scfg, $storeid, $client_cmd, $param, 1, %opts);
+}
+
 sub run_client_cmd {
     my ($scfg, $storeid, $client_cmd, $param, $no_output) = @_;
 
@@ -145,8 +222,8 @@ sub run_client_cmd {
 
     $param = [@$param, '--output-format=json'] if !$no_output;
 
-    run_raw_client_cmd($scfg, $storeid, $client_cmd, $param,
-		       outfunc => $outfunc, errmsg => 'proxmox-backup-client failed');
+    do_raw_client_cmd($scfg, $storeid, $client_cmd, $param, 0,
+		      outfunc => $outfunc, errmsg => 'proxmox-backup-client failed');
 
     return undef if $no_output;
 
@@ -174,8 +251,8 @@ sub extract_vzdump_config {
 	die "unable to extract configuration for backup format '$format'\n";
     }
 
-    run_raw_client_cmd($scfg, $storeid, 'restore', [ $name, $config_name, '-' ],
-		       outfunc => $outfunc, errmsg => 'proxmox-backup-client failed');
+    do_raw_client_cmd($scfg, $storeid, 'restore', [ $name, $config_name, '-' ], 0,
+		      outfunc => $outfunc, errmsg => 'proxmox-backup-client failed');
 
     return $config;
 }
@@ -183,22 +260,36 @@ sub extract_vzdump_config {
 sub on_add_hook {
     my ($class, $storeid, $scfg, %param) = @_;
 
-    if (defined($param{password})) {
-	pbs_set_password($scfg, $storeid, $param{password});
+    if (defined(my $password = $param{password})) {
+	pbs_set_password($scfg, $storeid, $password);
     } else {
 	pbs_delete_password($scfg, $storeid);
     }
+
+    if (defined(my $encryption_key = delete($scfg->{encryption_key}))) {
+	pbs_set_encryption_key($scfg, $storeid, $encryption_key);
+    } else {
+	pbs_delete_encryption_key($scfg, $storeid);
+    }
 }
 
 sub on_update_hook {
     my ($class, $storeid, $scfg, %param) = @_;
 
-    return if !exists($param{password});
+    if (exists($param{password})) {
+	if (defined($param{password})) {
+	    pbs_set_password($scfg, $storeid, $param{password});
+	} else {
+	    pbs_delete_password($scfg, $storeid);
+	}
+    }
 
-    if (defined($param{password})) {
-	pbs_set_password($scfg, $storeid, $param{password});
-    } else {
-	pbs_delete_password($scfg, $storeid);
+    if (exists($scfg->{encryption_key})) {
+	if (defined(my $encryption_key = delete($scfg->{encryption_key}))) {
+	    pbs_set_encryption_key($scfg, $storeid, $encryption_key);
+	} else {
+	    pbs_delete_encryption_key($scfg, $storeid);
+	}
     }
 }
 
@@ -206,6 +297,7 @@ sub on_delete_hook {
     my ($class, $storeid, $scfg) = @_;
 
     pbs_delete_password($scfg, $storeid);
+    pbs_delete_encryption_key($scfg, $storeid);
 }
 
 sub parse_volname {
-- 
2.20.1

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

[Wolfgang Bumiller]

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

[pve-devel] [PATCH storage 3/3] pvesm: encryption key parameter should load files

[Wolfgang Bumiller]

also `pvesm set` and `pvesm add` should behave the same with
respect to how configuration options are treated

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 PVE/CLI/pvesm.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
index 4f934d6..f223c92 100755
--- a/PVE/CLI/pvesm.pm
+++ b/PVE/CLI/pvesm.pm
@@ -36,9 +36,11 @@ sub param_mapping {
 	    return PVE::PTY::read_password("Enter Password: ");
 	},
     });
+
     my $mapping = {
 	'cifsscan' => [ $password_map ],
-	'create' => [ $password_map ],
+	'create' => [ $password_map, 'encryption_key' ],
+	'update' => [ $password_map, 'encryption_key' ],
     };
     return $mapping->{$name};
 }
-- 
2.20.1

[pve-devel] applied-series: Re: [PATCH storage 1/3] pbs: encryption support, split "raw client command" API

[Thomas Lamprecht]

On 09.07.20 10:25, Wolfgang Bumiller wrote:
> (And deprecate it...)
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
>  PVE/Storage/PBSPlugin.pm | 126 +++++++++++++++++++++++++++++++++------
>  1 file changed, 109 insertions(+), 17 deletions(-)
> 
>

applied series, thanks! Renamed the "encryption_key" parameter to "encryption-key"
and added support for a "autogen" magic value.