[pve-devel] [PATCH v2 00/13] Single-file-restore GUI for PBS snapshots

[pve-devel] [PATCH v2 00/13] Single-file-restore GUI for PBS snapshots

[Stefan Reiter]

Implements the necessary API for allowing single file restore via the PVE web
GUI.

This requires some adaptations of the HTTP server, to allow data streaming
without buffering - otherwise bigger restores would a) use up too much memory b)
get slowed down by the pvedaemon->pveproxy->client return path. Instead, we
transfer the data via a temporary FIFO.

Known issues/further work:
* restore VMs are not shown or stopped, they always run until timeout. Ideas
  would be to stop them when a user closes the restore window (but what if
  another user/tab/... is using the VM too?), or start a worker task for them
  (how to detect new VMs? parse proxmox-file-restore status before and after
  command?). Thoughts appreciated :)
* restarting the daemon or proxy marks the worker as failed and correctly
  stops it, but the browser says the download is OK - just with not enough data.
  Not the best experience, but unsure on how to fix this. Chunked encoding
  maybe, and terminate with an invalid chunk in case of error/abort?


v2:
* attach widget-toolkit patches, forgot them in v1
* drop patch "PBSClient: allow different command execution callback"
* drop applied patches, rebase
* nit cleanups from Fabian's preliminary review (thanks!)
  * includes API permission changes to keep them uniform for pve-storage
* fixups for Dominic's testing (thanks!)
* now supports encrypted CT and VM backups


proxmox-backup: Stefan Reiter (3):
  file-restore: don't force PBS_FINGERPRINT env var
  client-tools: add crypto_parameters_keep_fd
  file-restore: support encrypted VM backups

 src/bin/proxmox-file-restore.rs              | 22 +++++++++++++++++---
 src/bin/proxmox_client_tools/key_source.rs   | 17 +++++++++++++--
 src/bin/proxmox_file_restore/block_driver.rs |  1 +
 src/bin/proxmox_file_restore/qemu_helper.rs  | 12 ++++++-----
 4 files changed, 42 insertions(+), 10 deletions(-)

common: Stefan Reiter (4):
  PBSClient: adapt error message to include full package names
  PBSClient: add file_restore_list command
  PBSClient: add file_restore_extract function
  PBSClient: use crypt params for file 'list' and 'extract'

 src/PVE/PBSClient.pm | 71 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 2 deletions(-)

http-server: Stefan Reiter (2):
  support streaming data form fh to client
  allow stream download from path and over pvedaemon-proxy

 PVE/APIServer/AnyEvent.pm | 145 +++++++++++++++++++++++++++++++++++---
 1 file changed, 137 insertions(+), 8 deletions(-)

storage: Stefan Reiter (1):
  add FileRestore API for PBS

 PVE/API2/Storage/FileRestore.pm | 160 ++++++++++++++++++++++++++++++++
 PVE/API2/Storage/Makefile       |   2 +-
 PVE/API2/Storage/Status.pm      |   6 ++
 3 files changed, 167 insertions(+), 1 deletion(-)
 create mode 100644 PVE/API2/Storage/FileRestore.pm

proxmox-widget-toolkit: Stefan Reiter (3):
  Utils: add errorCallback to monStoreErrors
  FileBrowser: support 'virtual'/'v' file type
  FileBrowser: show errors in messagebox and allow expand 'all'

 src/Utils.js              |  6 ++++--
 src/window/FileBrowser.js | 24 +++++++++++++++++++++---
 2 files changed, 25 insertions(+), 5 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v2 proxmox-backup 01/13] file-restore: don't force PBS_FINGERPRINT env var

[Stefan Reiter]

It is valid to not set it, in case the server has a valid certificate.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 src/bin/proxmox_file_restore/qemu_helper.rs | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/bin/proxmox_file_restore/qemu_helper.rs b/src/bin/proxmox_file_restore/qemu_helper.rs
index 22563263..7fd2f1f8 100644
--- a/src/bin/proxmox_file_restore/qemu_helper.rs
+++ b/src/bin/proxmox_file_restore/qemu_helper.rs
@@ -127,9 +127,6 @@ pub async fn start_vm(
     if let Err(_) = std::env::var("PBS_PASSWORD") {
         bail!("environment variable PBS_PASSWORD has to be set for QEMU VM restore");
     }
-    if let Err(_) = std::env::var("PBS_FINGERPRINT") {
-        bail!("environment variable PBS_FINGERPRINT has to be set for QEMU VM restore");
-    }
 
     let pid;
     let (pid_fd, pid_path) = make_tmp_file("/tmp/file-restore-qemu.pid.tmp", CreateOptions::new())?;
-- 
2.20.1

[pve-devel] [PATCH v2 proxmox-backup 02/13] client-tools: add crypto_parameters_keep_fd

[Stefan Reiter]

same functionality as crypto_parameters, except it keeps the file
descriptor passed as "keyfd" open (and seeks to the beginning after
reading), if one is given.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 src/bin/proxmox_client_tools/key_source.rs | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/bin/proxmox_client_tools/key_source.rs b/src/bin/proxmox_client_tools/key_source.rs
index 0ad06bb0..fee00723 100644
--- a/src/bin/proxmox_client_tools/key_source.rs
+++ b/src/bin/proxmox_client_tools/key_source.rs
@@ -86,6 +86,14 @@ pub struct CryptoParams {
 }
 
 pub fn crypto_parameters(param: &Value) -> Result<CryptoParams, Error> {
+    do_crypto_parameters(param, false)
+}
+
+pub fn crypto_parameters_keep_fd(param: &Value) -> Result<CryptoParams, Error> {
+    do_crypto_parameters(param, true)
+}
+
+fn do_crypto_parameters(param: &Value, keep_keyfd_open: bool) -> Result<CryptoParams, Error> {
     let keyfile = match param.get("keyfile") {
         Some(Value::String(keyfile)) => Some(keyfile),
         Some(_) => bail!("bad --keyfile parameter type"),
@@ -135,11 +143,16 @@ pub fn crypto_parameters(param: &Value) -> Result<CryptoParams, Error> {
             file_get_contents(keyfile)?,
         )),
         (None, Some(fd)) => {
-            let input = unsafe { std::fs::File::from_raw_fd(fd) };
+            let mut input = unsafe { std::fs::File::from_raw_fd(fd) };
             let mut data = Vec::new();
-            let _len: usize = { input }.read_to_end(&mut data).map_err(|err| {
+            let _len: usize = input.read_to_end(&mut data).map_err(|err| {
                 format_err!("error reading encryption key from fd {}: {}", fd, err)
             })?;
+            if keep_keyfd_open {
+                // don't close fd if requested, and try to reset seek position
+                std::mem::forget(input);
+                unsafe { libc::lseek(fd, 0, libc::SEEK_SET); }
+            }
             Some(KeyWithSource::from_fd(data))
         }
     };
-- 
2.20.1

[pve-devel] [PATCH v2 proxmox-backup 03/13] file-restore: support encrypted VM backups

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 src/bin/proxmox-file-restore.rs              | 22 +++++++++++++++++---
 src/bin/proxmox_file_restore/block_driver.rs |  1 +
 src/bin/proxmox_file_restore/qemu_helper.rs  |  9 ++++++--
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/bin/proxmox-file-restore.rs b/src/bin/proxmox-file-restore.rs
index 2726eeb7..3d750152 100644
--- a/src/bin/proxmox-file-restore.rs
+++ b/src/bin/proxmox-file-restore.rs
@@ -30,7 +30,7 @@ pub mod proxmox_client_tools;
 use proxmox_client_tools::{
     complete_group_or_snapshot, complete_repository, connect, extract_repository_from_value,
     key_source::{
-        crypto_parameters, format_key_source, get_encryption_key_password, KEYFD_SCHEMA,
+        crypto_parameters_keep_fd, format_key_source, get_encryption_key_password, KEYFD_SCHEMA,
         KEYFILE_SCHEMA,
     },
     REPO_URL_SCHEMA,
@@ -76,6 +76,18 @@ fn parse_path(path: String, base64: bool) -> Result<ExtractPath, Error> {
     }
 }
 
+fn keyfile_path(param: &Value) -> Option<String> {
+    if let Some(Value::String(keyfile)) = param.get("keyfile") {
+        return Some(keyfile.to_owned());
+    }
+
+    if let Some(Value::Number(keyfd)) = param.get("keyfd") {
+        return Some(format!("/dev/fd/{}", keyfd));
+    }
+
+    None
+}
+
 #[api(
    input: {
        properties: {
@@ -138,7 +150,8 @@ async fn list(
     let snapshot: BackupDir = snapshot.parse()?;
     let path = parse_path(path, base64)?;
 
-    let crypto = crypto_parameters(&param)?;
+    let keyfile = keyfile_path(&param);
+    let crypto = crypto_parameters_keep_fd(&param)?;
     let crypt_config = match crypto.enc_key {
         None => None,
         Some(ref key) => {
@@ -210,6 +223,7 @@ async fn list(
                 manifest,
                 repo,
                 snapshot,
+                keyfile,
             };
             let driver: Option<BlockDriverType> = match param.get("driver") {
                 Some(drv) => Some(serde_json::from_value(drv.clone())?),
@@ -309,7 +323,8 @@ async fn extract(
         None => Some(std::env::current_dir()?),
     };
 
-    let crypto = crypto_parameters(&param)?;
+    let keyfile = keyfile_path(&param);
+    let crypto = crypto_parameters_keep_fd(&param)?;
     let crypt_config = match crypto.enc_key {
         None => None,
         Some(ref key) => {
@@ -360,6 +375,7 @@ async fn extract(
                 manifest,
                 repo,
                 snapshot,
+                keyfile,
             };
             let driver: Option<BlockDriverType> = match param.get("driver") {
                 Some(drv) => Some(serde_json::from_value(drv.clone())?),
diff --git a/src/bin/proxmox_file_restore/block_driver.rs b/src/bin/proxmox_file_restore/block_driver.rs
index 924503a7..ba9794e3 100644
--- a/src/bin/proxmox_file_restore/block_driver.rs
+++ b/src/bin/proxmox_file_restore/block_driver.rs
@@ -21,6 +21,7 @@ pub struct SnapRestoreDetails {
     pub repo: BackupRepository,
     pub snapshot: BackupDir,
     pub manifest: BackupManifest,
+    pub keyfile: Option<String>,
 }
 
 /// Return value of a BlockRestoreDriver.status() call, 'id' must be valid for .stop(id)
diff --git a/src/bin/proxmox_file_restore/qemu_helper.rs b/src/bin/proxmox_file_restore/qemu_helper.rs
index 7fd2f1f8..0f3a7feb 100644
--- a/src/bin/proxmox_file_restore/qemu_helper.rs
+++ b/src/bin/proxmox_file_restore/qemu_helper.rs
@@ -190,9 +190,14 @@ pub async fn start_vm(
             continue;
         }
         drives.push("-drive".to_owned());
+        let keyfile = if let Some(ref keyfile) = details.keyfile {
+            format!(",,keyfile={}", keyfile)
+        } else {
+            "".to_owned()
+        };
         drives.push(format!(
-            "file=pbs:repository={},,snapshot={},,archive={},read-only=on,if=none,id=drive{}",
-            details.repo, details.snapshot, file, id
+            "file=pbs:repository={},,snapshot={},,archive={}{},read-only=on,if=none,id=drive{}",
+            details.repo, details.snapshot, file, keyfile, id
         ));
         drives.push("-device".to_owned());
         // drive serial is used by VM to map .fidx files to /dev paths
-- 
2.20.1

[pve-devel] [PATCH v2 common 04/13] PBSClient: adapt error message to include full package names

[Stefan Reiter]

More helpful for a user to know what they're missing.

Suggested-by: Dominic Jäger <d.jaeger@proxmox.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 src/PVE/PBSClient.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 857cff0..16f7765 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -141,7 +141,7 @@ my sub do_raw_client_cmd {
 
     my $client_exe = (delete $opts{binary}) || 'proxmox-backup-client';
     $client_exe = "/usr/bin/$client_exe";
-    die "executable not found '$client_exe'! Proxmox backup client or file restore not installed?\n"
+    die "executable not found '$client_exe'! proxmox-backup-client or proxmox-backup-file-restore not installed?\n"
 	if ! -x $client_exe;
 
     my $scfg = $self->{scfg};
-- 
2.20.1

[pve-devel] [PATCH v2 common 05/13] PBSClient: add file_restore_list command

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

v2:
* one line per param

 src/PVE/PBSClient.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 16f7765..8e4deca 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -335,4 +335,15 @@ sub status {
     return ($total, $free, $used, $active);
 };
 
+sub file_restore_list {
+    my ($self, $snapshot, $filepath, $base64) = @_;
+    return run_client_cmd(
+	$self,
+	"list",
+	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ],
+	0,
+	"proxmox-file-restore",
+    );
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH v2 common 06/13] PBSClient: add file_restore_extract function

[Stefan Reiter]

*_prepare creates a fifo for streaming data back to clients directly,
filefile_restore_extract blocks and should be called from a background
worker - while it is running outcoming data can be read from the FIFO.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

v2:
* don't use "startcmd", just pass "opts" directly
* better error handling

 src/PVE/PBSClient.pm | 56 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 8e4deca..e96f20d 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -5,9 +5,10 @@ use strict;
 use warnings;
 
 use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
+use File::Temp qw(tempdir);
 use IO::File;
 use JSON;
-use POSIX qw(strftime ENOENT);
+use POSIX qw(mkfifo strftime ENOENT);
 
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools qw(run_command file_set_contents file_get_contents file_read_firstline $IPV6RE);
@@ -346,4 +347,57 @@ sub file_restore_list {
     );
 }
 
+# call sync from API, returns a fifo path for streaming data to clients,
+# pass it to file_restore_extract to start transfering data
+sub file_restore_extract_prepare {
+    my ($self) = @_;
+
+    my $tmpdir = tempdir();
+    mkfifo("$tmpdir/fifo", 0600)
+	or die "creating file download fifo '$tmpdir/fifo' failed: $!\n";
+
+    # allow reading data for proxy user
+    my $wwwid = getpwnam('www-data') ||
+	die "getpwnam failed";
+    chown $wwwid, -1, "$tmpdir"
+	or die "changing permission on fifo dir '$tmpdir' failed: $!\n";
+    chown $wwwid, -1, "$tmpdir/fifo"
+	or die "changing permission on fifo '$tmpdir/fifo' failed: $!\n";
+
+    return "$tmpdir/fifo";
+}
+
+# this blocks while data is transfered, call this from a background worker
+sub file_restore_extract {
+    my ($self, $output_file, $snapshot, $filepath, $base64) = @_;
+
+    my $ret = eval {
+	local $SIG{ALRM} = sub { die "got timeout\n" };
+	alarm(30);
+	sysopen(my $fh, "$output_file", O_WRONLY)
+	    or die "open target '$output_file' for writing failed: $!\n";
+	alarm(0);
+
+	my $fn = fileno($fh);
+	my $errfunc = sub { print $_[0], "\n"; };
+
+	return run_raw_client_cmd(
+	    $self,
+            "extract",
+	    [ $snapshot, $filepath, "-", "--base64", $base64 ? 1 : 0 ],
+	    binary => "proxmox-file-restore",
+	    errfunc => $errfunc,
+	    output => ">&$fn",
+	);
+    };
+    my $err = $@;
+
+    unlink($output_file);
+    $output_file =~ s/fifo$//;
+    rmdir($output_file) if -d $output_file;
+
+    die "file restore task failed: $err" if $err;
+    return $ret;
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH v2 common 07/13] PBSClient: use crypt params for file 'list' and 'extract'

[Stefan Reiter]

Necessary for accessing encrypted backups.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 src/PVE/PBSClient.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index e96f20d..537fd83 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -133,6 +133,8 @@ my $USE_CRYPT_PARAMS = {
     backup => 1,
     restore => 1,
     'upload-log' => 1,
+    list => 1,
+    extract => 1,
 };
 
 my sub do_raw_client_cmd {
-- 
2.20.1

[pve-devel] [PATCH v2 http-server 08/13] support streaming data form fh to client

[Stefan Reiter]

Use an explicit AnyEvent::Handle similar to websocket proxying.

Needs some special care to make sure we apply backpressure correctly to
avoid caching too much data. Note that because of AnyEvent restrictions,
specifying a "fh" to point to a file or a packet-based socket may result
in unwanted behaviour[0].

[0]: https://metacpan.org/pod/AnyEvent::Handle#DESCRIPTION

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

v2:
* move to extra sub "response_stream"
* adapt comments

 PVE/APIServer/AnyEvent.pm | 105 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 4 deletions(-)

diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
index 60a2a1c..f3db0e3 100644
--- a/PVE/APIServer/AnyEvent.pm
+++ b/PVE/APIServer/AnyEvent.pm
@@ -188,8 +188,93 @@ sub finish_response {
     }
 }
 
+sub response_stream {
+    my ($self, $reqstate, $stream_fh) = @_;
+
+    # disable timeout, we don't know how big the data is
+    $reqstate->{hdl}->timeout(0);
+
+    my $buf_size = 4*1024*1024;
+
+    my $on_read;
+    $on_read = sub {
+	my ($hdl) = @_;
+	my $reqhdl = $reqstate->{hdl};
+	return if !$reqhdl;
+
+	my $wbuf_len = length($reqhdl->{wbuf});
+	my $rbuf_len = length($hdl->{rbuf});
+	# TODO: Take into account $reqhdl->{wbuf_max} ? Right now
+	# that's unbounded, so just assume $buf_size
+	my $to_read = $buf_size - $wbuf_len;
+	$to_read = $rbuf_len if $rbuf_len < $to_read;
+	if ($to_read > 0) {
+	    my $data = substr($hdl->{rbuf}, 0, $to_read, '');
+	    $reqhdl->push_write($data);
+	    $rbuf_len -= $to_read;
+	} elsif ($hdl->{_eof}) {
+	    # workaround: AnyEvent gives us a fake EPIPE if we don't consume
+	    # any data when called at EOF, so unregister ourselves - data is
+	    # flushed by on_eof anyway
+	    # see: https://sources.debian.org/src/libanyevent-perl/7.170-2/lib/AnyEvent/Handle.pm/#L1329
+	    $hdl->on_read();
+	    return;
+	}
+
+	# apply backpressure so we don't accept any more data into
+	# buffer if the client isn't downloading fast enough
+	# note: read_size can double upon read, and we also need to
+	# account for one more read after start_read, so *4
+	if ($rbuf_len + $hdl->{read_size}*4 > $buf_size) {
+	    # stop reading until write buffer is empty
+	    $hdl->on_read();
+	    my $prev_on_drain = $reqhdl->{on_drain};
+	    $reqhdl->on_drain(sub {
+		my ($wrhdl) = @_;
+		# on_drain called because write buffer is empty, continue reading
+		$hdl->on_read($on_read);
+		if ($prev_on_drain) {
+		    $wrhdl->on_drain($prev_on_drain);
+		    $prev_on_drain->($wrhdl);
+		}
+	    });
+	}
+    };
+
+    $reqstate->{proxyhdl} = AnyEvent::Handle->new(
+	fh => $stream_fh,
+	rbuf_max => $buf_size,
+	timeout => 0,
+	on_read => $on_read,
+	on_eof => sub {
+	    my ($hdl) = @_;
+	    eval {
+		if (my $reqhdl = $reqstate->{hdl}) {
+		    $self->log_aborted_request($reqstate);
+		    # write out any remaining data
+		    $reqhdl->push_write($hdl->{rbuf}) if length($hdl->{rbuf}) > 0;
+		    $hdl->{rbuf} = "";
+		    $reqhdl->push_shutdown();
+		    $self->finish_response($reqstate);
+		}
+	    };
+	    if (my $err = $@) { syslog('err', "$err"); }
+	    $on_read = undef;
+	},
+	on_error => sub {
+	    my ($hdl, $fatal, $message) = @_;
+	    eval {
+		$self->log_aborted_request($reqstate, $message);
+		$self->client_do_disconnect($reqstate);
+	    };
+	    if (my $err = $@) { syslog('err', "$err"); }
+	    $on_read = undef;
+	},
+    );
+}
+
 sub response {
-    my ($self, $reqstate, $resp, $mtime, $nocomp, $delay) = @_;
+    my ($self, $reqstate, $resp, $mtime, $nocomp, $delay, $stream_fh) = @_;
 
     #print "$$: send response: " . Dumper($resp);
 
@@ -231,7 +316,7 @@ sub response {
     $resp->header('Server' => "pve-api-daemon/3.0");
 
     my $content_length;
-    if ($content) {
+    if ($content && !$stream_fh) {
 
 	$content_length = length($content);
 
@@ -258,11 +343,16 @@ sub response {
     #print "SEND(without content) $res\n" if $self->{debug};
 
     $res .= "\015\012";
-    $res .= $content if $content;
+    $res .= $content if $content && !$stream_fh;
 
     $self->log_request($reqstate, $reqstate->{request});
 
-    if ($delay && $delay > 0) {
+    if ($stream_fh) {
+	# write headers and preamble...
+	$reqstate->{hdl}->push_write($res);
+	# ...then stream data via an AnyEvent::Handle
+	$self->response_stream($reqstate, $stream_fh);
+    } elsif ($delay && $delay > 0) {
 	my $w; $w = AnyEvent->timer(after => $delay, cb => sub {
 	    undef $w; # delete reference
 	    $reqstate->{hdl}->push_write($res);
@@ -322,6 +412,13 @@ sub send_file_start {
 	    if (ref($download) eq 'HASH') {
 		$fh = $download->{fh};
 		$mime = $download->{'content-type'};
+
+		if ($download->{stream}) {
+		    my $header = HTTP::Headers->new(Content_Type => $mime);
+		    my $resp = HTTP::Response->new(200, "OK", $header);
+		    $self->response($reqstate, $resp, undef, 1, 0, $fh);
+		    return;
+		}
 	    } else {
 		my $filename = $download;
 		$fh = IO::File->new($filename, '<') ||
-- 
2.20.1

[pve-devel] [PATCH v2 http-server 09/13] allow stream download from path and over pvedaemon-proxy

[Stefan Reiter]

Allow specifying a filepath for stream=1 instead of either a path or fh
with stream=1.

With this in place, we can also just return the path to the proxy in
case we want to stream a response back, and let it read from the file
itself. This way, the pvedaemon is cut out of the transfer pipe.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

v2:
* unchanged

 PVE/APIServer/AnyEvent.pm | 40 +++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
index f3db0e3..0654bd4 100644
--- a/PVE/APIServer/AnyEvent.pm
+++ b/PVE/APIServer/AnyEvent.pm
@@ -410,9 +410,33 @@ sub send_file_start {
 	    my $mime;
 
 	    if (ref($download) eq 'HASH') {
-		$fh = $download->{fh};
 		$mime = $download->{'content-type'};
 
+		if ($download->{path} && $download->{stream} &&
+		    $reqstate->{request}->header('PVEDisableProxy'))
+		{
+		    # avoid double stream from a file, let the proxy handle it
+		    die "internal error: file proxy streaming only available for pvedaemon\n"
+			if !$self->{trusted_env};
+		    my $header = HTTP::Headers->new(
+			pvestreamfile => $download->{path},
+			Content_Type => $mime,
+		    );
+		    # we need some data so Content-Length gets set correctly and
+		    # the proxy doesn't wait for more data - place a canary
+		    my $resp = HTTP::Response->new(200, "OK", $header, "error canary");
+		    $self->response($reqstate, $resp);
+		    return;
+		}
+
+		if (!($fh = $download->{fh})) {
+		    my $path = $download->{path};
+		    die "internal error: {download} returned but neither fh not path given\n"
+			if !$path;
+		    sysopen($fh, "$path", O_NONBLOCK | O_RDONLY)
+			or die "open stream path '$path' for reading failed: $!\n";
+		}
+
 		if ($download->{stream}) {
 		    my $header = HTTP::Headers->new(Content_Type => $mime);
 		    my $resp = HTTP::Response->new(200, "OK", $header);
@@ -750,6 +774,7 @@ sub proxy_request {
 		eval {
 		    my $code = delete $hdr->{Status};
 		    my $msg = delete $hdr->{Reason};
+		    my $stream = delete $hdr->{pvestreamfile};
 		    delete $hdr->{URL};
 		    delete $hdr->{HTTPVersion};
 		    my $header = HTTP::Headers->new(%$hdr);
@@ -757,9 +782,16 @@ sub proxy_request {
 			$location =~ s|^http://localhost:85||;
 			$header->header(Location => $location);
 		    }
-		    my $resp = HTTP::Response->new($code, $msg, $header, $body);
-		    # Note: disable compression, because body is already compressed
-		    $self->response($reqstate, $resp, undef, 1);
+		    if ($stream) {
+			sysopen(my $fh, "$stream", O_NONBLOCK | O_RDONLY)
+			    or die "open stream path '$stream' for forwarding failed: $!\n";
+			my $resp = HTTP::Response->new($code, $msg, $header, undef);
+			$self->response($reqstate, $resp, undef, 1, 0, $fh);
+		    } else {
+			my $resp = HTTP::Response->new($code, $msg, $header, $body);
+			# Note: disable compression, because body is already compressed
+			$self->response($reqstate, $resp, undef, 1);
+		    }
 		};
 		warn $@ if $@;
 	    });
-- 
2.20.1

[pve-devel] [PATCH v2 storage 10/13] add FileRestore API for PBS

[Stefan Reiter]

Includes list and restore calls.

Requires VM.Backup and Datastore.Audit permissions, for the accessed
VM/CT and containing datastore respectively.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Requires updated pve-common, pve-http-server.

v2:
* use same permissions as regular backup access, check with check_volume_access

Note: I didn't add the "snapshot -> volume ID" change for now, but AFAICT this
should be a drop-in replacement (maybe aside from the parameter name). The
$real_volume_id sub in Content.pm should handle that just fine? Well, it would
need the "backup/" prefix to become a volid, but again, should be fine.
Feel free to adapt this though if you like the other way better, no hard
feelings tbh...

 PVE/API2/Storage/FileRestore.pm | 160 ++++++++++++++++++++++++++++++++
 PVE/API2/Storage/Makefile       |   2 +-
 PVE/API2/Storage/Status.pm      |   6 ++
 3 files changed, 167 insertions(+), 1 deletion(-)
 create mode 100644 PVE/API2/Storage/FileRestore.pm

diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
new file mode 100644
index 0000000..68f81eb
--- /dev/null
+++ b/PVE/API2/Storage/FileRestore.pm
@@ -0,0 +1,160 @@
+package PVE::API2::Storage::FileRestore;
+
+use strict;
+use warnings;
+
+use MIME::Base64;
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::PBSClient;
+use PVE::Storage;
+use PVE::Tools qw(extract_param);
+
+use PVE::RESTHandler;
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method ({
+    name => 'list',
+    path => 'list',
+    method => 'GET',
+    proxyto => 'node',
+    permissions => {
+	description => "You need read access for the volume.",
+	user => 'all',
+    },
+    description => "List files and directories for single file restore under the given path.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    storage => get_standard_option('pve-storage-id'),
+	    snapshot => {
+		description => "Backup snapshot identifier.",
+		type => 'string',
+	    },
+	    filepath => {
+		description => 'base64-path to the directory or file being listed, or "/".',
+		type => 'string',
+	    },
+	},
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => "object",
+	    properties => {
+		filepath => {
+		    description => "base64 path of the current entry",
+		    type => 'string',
+		},
+		type => {
+		    description => "Entry type.",
+		    type => 'string',
+		},
+		text => {
+		    description => "Entry display text.",
+		    type => 'string',
+		},
+		leaf => {
+		    description => "If this entry is a leaf in the directory graph.",
+		    type => 'any', # JSON::PP::Boolean gets passed through
+		},
+		size => {
+		    description => "Entry file size.",
+		    type => 'integer',
+		    optional => 1,
+		},
+		mtime => {
+		    description => "Entry last-modified time (unix timestamp).",
+		    type => 'integer',
+		    optional => 1,
+		},
+	    },
+	},
+    },
+    protected => 1,
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
+
+	my $path = extract_param($param, 'filepath') || "/";
+	my $base64 = $path ne "/";
+	my $snap = extract_param($param, 'snapshot');
+	my $storeid = extract_param($param, 'storage');
+	my $cfg = PVE::Storage::config();
+	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
+
+	my $volid = "$storeid:backup/$snap";
+	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $volid);
+
+	my $client = PVE::PBSClient->new($scfg, $storeid);
+	my $ret = $client->file_restore_list($snap, $path, $base64);
+
+	return $ret;
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'download',
+    path => 'download',
+    method => 'GET',
+    proxyto => 'node',
+    permissions => {
+	description => "You need read access for the volume.",
+	user => 'all',
+    },
+    description => "Extract a file or directory (as zip archive) from a PBS backup.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    storage => get_standard_option('pve-storage-id'),
+	    snapshot => {
+		description => "Backup snapshot identifier.",
+		type => 'string',
+	    },
+	    filepath => {
+		description => 'base64-path to the directory or file to download.',
+		type => 'string',
+	    },
+	},
+    },
+    returns => {
+	type => 'any', # download
+    },
+    protected => 1,
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
+
+	my $path = extract_param($param, 'filepath');
+	my $snap = extract_param($param, 'snapshot');
+	my $storeid = extract_param($param, 'storage');
+	my $cfg = PVE::Storage::config();
+	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
+
+	my $volid = "$storeid:backup/$snap";
+	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $volid);
+
+	my $client = PVE::PBSClient->new($scfg, $storeid);
+	my $fifo = $client->file_restore_extract_prepare();
+
+	$rpcenv->fork_worker('pbs-download', undef, $user, sub {
+	    my $name = decode_base64($path);
+	    print "Starting download of file: $name\n";
+	    $client->file_restore_extract($fifo, $snap, $path, 1);
+	});
+
+	my $ret = {
+	    download => {
+		path => $fifo,
+		stream => 1,
+		'content-type' => 'application/octet-stream',
+	    },
+	};
+	return $ret;
+    }});
+
+1;
diff --git a/PVE/API2/Storage/Makefile b/PVE/API2/Storage/Makefile
index 690b437..1705080 100644
--- a/PVE/API2/Storage/Makefile
+++ b/PVE/API2/Storage/Makefile
@@ -1,5 +1,5 @@
 
-SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm
+SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm FileRestore.pm
 
 .PHONY: install
 install:
diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index d12643f..897b4a7 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -12,6 +12,7 @@ use PVE::RRD;
 use PVE::Storage;
 use PVE::API2::Storage::Content;
 use PVE::API2::Storage::PruneBackups;
+use PVE::API2::Storage::FileRestore;
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
 use PVE::JSONSchema qw(get_standard_option);
@@ -32,6 +33,11 @@ __PACKAGE__->register_method ({
     path => '{storage}/content',
 });
 
+__PACKAGE__->register_method ({
+   subclass => "PVE::API2::Storage::FileRestore",
+   path => '{storage}/file-restore',
+});
+
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',
-- 
2.20.1

[pve-devel] [PATCH v2 proxmox-widget-toolkit 11/13] Utils: add errorCallback to monStoreErrors

[Stefan Reiter]

Call a function to decide if we want to mask the component. If the
callback returns true, we assume it has already handled the error (i.e.
shown a messagebox or similar) and skip masking.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 src/Utils.js | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/Utils.js b/src/Utils.js
index 3fd8f91..ee30027 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -317,7 +317,7 @@ utilities: {
 	return msg.join('<br>');
     },
 
-    monStoreErrors: function(component, store, clearMaskBeforeLoad) {
+    monStoreErrors: function(component, store, clearMaskBeforeLoad, errorCallback) {
 	if (clearMaskBeforeLoad) {
 	    component.mon(store, 'beforeload', function(s, operation, eOpts) {
 		Proxmox.Utils.setErrorMask(component, false);
@@ -342,7 +342,9 @@ utilities: {
 
 	    let error = request._operation.getError();
 	    let msg = Proxmox.Utils.getResponseErrorMessage(error);
-	    Proxmox.Utils.setErrorMask(component, msg);
+	    if (!errorCallback || !errorCallback(error, msg)) {
+		Proxmox.Utils.setErrorMask(component, msg);
+	    }
 	});
     },
 
-- 
2.20.1

[pve-devel] [PATCH v2 proxmox-widget-toolkit 12/13] FileBrowser: support 'virtual'/'v' file type

[Stefan Reiter]

Denotes objects like disks ".img.fidx" files, which shouldn't be
downloadable, but should still approximate a directory entry.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 src/window/FileBrowser.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/window/FileBrowser.js b/src/window/FileBrowser.js
index e90b717..d138d1b 100644
--- a/src/window/FileBrowser.js
+++ b/src/window/FileBrowser.js
@@ -36,6 +36,9 @@ Ext.define('proxmox-file-tree', {
 		    case 's': // socket
 			icon = 'plug';
 			break;
+		    case 'v': // virtual
+			icon = 'cube';
+			break;
 		    default:
 			icon = 'file-o';
 			break;
@@ -217,6 +220,7 @@ Ext.define("Proxmox.window.FileBrowser", {
 			    case 'l': return gettext('Softlink');
 			    case 'p': return gettext('Pipe/Fifo');
 			    case 's': return gettext('Socket');
+			    case 'v': return gettext('Virtual');
 			    default: return Proxmox.Utils.unknownText;
 			}
 		    },
-- 
2.20.1

[pve-devel] [PATCH v2 proxmox-widget-toolkit 13/13] FileBrowser: show errors in messagebox and allow expand 'all'

[Stefan Reiter]

If an error is received upon expanding a node, chances are the rest of
the tree is still valid (i.e. opening a partition fails because it
doesn't contain a supported filesystem). Only show an error box for the
user, but don't mask the component in that case. Additionally, disable
the download button.

Also support an archive set to 'all' to expand all children, useful for
initializing a file-restore VM on initial load.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 src/window/FileBrowser.js | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/window/FileBrowser.js b/src/window/FileBrowser.js
index d138d1b..99a7a85 100644
--- a/src/window/FileBrowser.js
+++ b/src/window/FileBrowser.js
@@ -123,6 +123,16 @@ Ext.define("Proxmox.window.FileBrowser", {
 	    me.lookup('downloadBtn').setDisabled(!canDownload);
 	},
 
+	errorHandler: function(error, msg) {
+	    let me = this;
+	    me.lookup('downloadBtn').setDisabled(true);
+	    if (me.initialLoadDone) {
+		Ext.Msg.alert(gettext('Error'), msg);
+		return true;
+	    }
+	    return false;
+	},
+
 	init: function(view) {
 	    let me = this;
 	    let tree = me.lookup('tree');
@@ -134,13 +144,16 @@ Ext.define("Proxmox.window.FileBrowser", {
 	    let store = tree.getStore();
 	    let proxy = store.getProxy();
 
-	    Proxmox.Utils.monStoreErrors(tree, store, true);
+	    let errorCallback = (error, msg) => me.errorHandler(error, msg);
+	    Proxmox.Utils.monStoreErrors(tree, store, true, errorCallback);
 	    proxy.setUrl(view.listURL);
 	    proxy.setExtraParams(view.extraParams);
-	    store.load(() => {
+	    store.load((rec, op, success) => {
 		let root = store.getRoot();
 		root.expand(); // always expand invisible root node
-		if (view.archive) {
+		if (view.archive === 'all') {
+		    root.expandChildren(false);
+		} else if (view.archive) {
 		    let child = root.findChild('text', view.archive);
 		    if (child) {
 			child.expand();
@@ -152,6 +165,7 @@ Ext.define("Proxmox.window.FileBrowser", {
 		} else if (root.childNodes.length === 1) {
 		    root.firstChild.expand();
 		}
+		me.initialLoadDone = success;
 	    });
 	},
 
-- 
2.20.1

[pve-devel] [PATCH v2 manager 1/2] backupview: add file restore button

[Stefan Reiter]

Adds it for both BackupViews, on VM view and storage view.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Forgot to resend this one for v2, it didn't change though AFAIR.

 www/manager6/grid/BackupView.js    | 23 +++++++++++++++++++++++
 www/manager6/storage/BackupView.js | 19 +++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index b50f52ed..7beeca0e 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -228,6 +228,28 @@ Ext.define('PVE.grid.BackupView', {
 	    },
 	});
 
+	let file_restore_btn = Ext.create('Proxmox.button.Button', {
+	    text: gettext('File Restore'),
+	    disabled: true,
+	    selModel: sm,
+	    enableFn: function(rec) {
+		return !!rec && isPBS;
+	    },
+	    handler: function(b, e, rec) {
+		var storage = storagesel.getValue();
+		Ext.create('Proxmox.window.FileBrowser', {
+		    title: gettext('File Restore') + " - " + rec.data.text,
+		    listURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/list`,
+		    downloadURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/download`,
+		    extraParams: {
+			snapshot: rec.data.text,
+		    },
+		    archive: PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format) ?
+			'all' : undefined,
+		}).show();
+	    },
+	});
+
 	Ext.apply(me, {
 	    selModel: sm,
 	    tbar: {
@@ -238,6 +260,7 @@ Ext.define('PVE.grid.BackupView', {
 		    delete_btn,
 		    '-',
 		    config_btn,
+		    file_restore_btn,
 		    '-',
 		    {
 			xtype: 'proxmoxButton',
diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 3dd500c2..5be18409 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -133,6 +133,25 @@ Ext.define('PVE.storage.BackupView', {
 		    renderer: PVE.Utils.render_backup_verification,
 		},
 	    };
+
+	    me.tbar.push({
+		xtype: 'proxmoxButton',
+		text: gettext('File Restore'),
+		disabled: true,
+		selModel: sm,
+		handler: function(b, e, rec) {
+		    Ext.create('Proxmox.window.FileBrowser', {
+			title: gettext('File Restore') + " - " + rec.data.text,
+			listURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/list`,
+			downloadURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/download`,
+			extraParams: {
+			    snapshot: rec.data.text,
+			},
+			archive: PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format) ?
+			    'all' : undefined,
+		    }).show();
+		},
+	    });
 	}
 
 	me.callParent();
-- 
2.20.1

[pve-devel] [PATCH v2 manager 2/2] gui: add task name for 'pbs-download'

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

new in v2

 www/manager6/Utils.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 7fcda854..4b5dc189 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1791,6 +1791,7 @@ Ext.define('PVE.Utils', {
 	    lvmthincreate: [gettext('LVM-Thin Storage'), gettext('Create')],
 	    migrateall: ['', gettext('Migrate all VMs and Containers')],
 	    'move_volume': ['CT', gettext('Move Volume')],
+	    'pbs-download': ['VM/CT', gettext('File Restore Download')],
 	    pull_file: ['CT', gettext('Pull file')],
 	    push_file: ['CT', gettext('Push file')],
 	    qmclone: ['VM', gettext('Clone')],
-- 
2.20.1

[pve-devel] applied: [pbs-devel] [PATCH v2 proxmox-backup 01/13] file-restore: don't force PBS_FINGERPRINT env var

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> It is valid to not set it, in case the server has a valid certificate.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/bin/proxmox_file_restore/qemu_helper.rs | 3 ---
>  1 file changed, 3 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [pbs-devel] [PATCH v2 proxmox-backup 02/13] client-tools: add crypto_parameters_keep_fd

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> same functionality as crypto_parameters, except it keeps the file
> descriptor passed as "keyfd" open (and seeks to the beginning after
> reading), if one is given.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/bin/proxmox_client_tools/key_source.rs | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [pbs-devel] [PATCH v2 proxmox-backup 03/13] file-restore: support encrypted VM backups

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/bin/proxmox-file-restore.rs              | 22 +++++++++++++++++---
>  src/bin/proxmox_file_restore/block_driver.rs |  1 +
>  src/bin/proxmox_file_restore/qemu_helper.rs  |  9 ++++++--
>  3 files changed, 27 insertions(+), 5 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 proxmox-widget-toolkit 11/13] Utils: add errorCallback to monStoreErrors

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> Call a function to decide if we want to mask the component. If the
> callback returns true, we assume it has already handled the error (i.e.
> shown a messagebox or similar) and skip masking.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/Utils.js | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 proxmox-widget-toolkit 12/13] FileBrowser: support 'virtual'/'v' file type

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> Denotes objects like disks ".img.fidx" files, which shouldn't be
> downloadable, but should still approximate a directory entry.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/window/FileBrowser.js | 4 ++++
>  1 file changed, 4 insertions(+)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 proxmox-widget-toolkit 13/13] FileBrowser: show errors in messagebox and allow expand 'all'

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> If an error is received upon expanding a node, chances are the rest of
> the tree is still valid (i.e. opening a partition fails because it
> doesn't contain a supported filesystem). Only show an error box for the
> user, but don't mask the component in that case. Additionally, disable
> the download button.
> 
> Also support an archive set to 'all' to expand all children, useful for
> initializing a file-restore VM on initial load.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/window/FileBrowser.js | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH v2 common 07/13] PBSClient: use crypt params for file 'list' and 'extract'

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> Necessary for accessing encrypted backups.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/PVE/PBSClient.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> index e96f20d..537fd83 100644
> --- a/src/PVE/PBSClient.pm
> +++ b/src/PVE/PBSClient.pm
> @@ -133,6 +133,8 @@ my $USE_CRYPT_PARAMS = {
>      backup => 1,
>      restore => 1,
>      'upload-log' => 1,
> +    list => 1,

can break the proxmox-backup-client list command... that's why I wanted to have
a rather clean split between those two..

> +    extract => 1,
>  };
>  
>  my sub do_raw_client_cmd {
> 

[pve-devel] [PATCH manager] file-restore: pass in full volume ID

[Fabian Grünbichler]

not just the snapshot name.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    only apply if corresponding backend/API patch in pve-storage is applied!

 www/manager6/grid/BackupView.js    | 2 +-
 www/manager6/storage/BackupView.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index 7beeca0e..1d17e964 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -242,7 +242,7 @@ Ext.define('PVE.grid.BackupView', {
 		    listURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/list`,
 		    downloadURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/download`,
 		    extraParams: {
-			snapshot: rec.data.text,
+			volume: rec.data.volid,
 		    },
 		    archive: PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format) ?
 			'all' : undefined,
diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 5be18409..d31a75c2 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -145,7 +145,7 @@ Ext.define('PVE.storage.BackupView', {
 			listURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/list`,
 			downloadURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/download`,
 			extraParams: {
-			    snapshot: rec.data.text,
+			    volume: rec.data.volid,
 			},
 			archive: PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format) ?
 			    'all' : undefined,
-- 
2.20.1

[pve-devel] [PATCH storage 1/2] file-restore: return perl-y booleans

[Fabian Grünbichler]

like we do in most of our API.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 PVE/API2/Storage/FileRestore.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
index 68f81eb..f6dc74a 100644
--- a/PVE/API2/Storage/FileRestore.pm
+++ b/PVE/API2/Storage/FileRestore.pm
@@ -56,7 +56,7 @@ __PACKAGE__->register_method ({
 		},
 		leaf => {
 		    description => "If this entry is a leaf in the directory graph.",
-		    type => 'any', # JSON::PP::Boolean gets passed through
+		    type => 'boolean',
 		},
 		size => {
 		    description => "Entry file size.",
@@ -91,6 +91,13 @@ __PACKAGE__->register_method ({
 	my $client = PVE::PBSClient->new($scfg, $storeid);
 	my $ret = $client->file_restore_list($snap, $path, $base64);
 
+
+	# 'leaf' is a proper JSON boolean, map to perl-y bool
+	# TODO: make PBSClient decode all bools always as 1/0?
+	foreach my $item (@$ret) {
+	    $item->{leaf} = $item->{leaf} ? 1 : 0;
+	}
+
 	return $ret;
     }});
 
-- 
2.20.1

[pve-devel] [PATCH storage 2/2] file-restore: pass in volume ID or name

[Fabian Grünbichler]

instead of just the snapshot for consistency with other API endpoints,
and possible future extension to VMA backups (where 'snapshot' would be
a rather strange terminology).

add some additional checks (pbs storage type, backup volume type),
completion and magic (allow passing in either a full volume ID with
correct storage, or just the volume name, or just the snapshot for
easier API/CLI usage/convenience).

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    requires corresponding patch on the GUI side as well since the parameter was renamed

    the snapshot name convenience part could be dropped by replacing

+    } elsif ($volume =~ m/^backup\//) {
+	$volid = "$storeid:$volume";
+    } else {
+	$volid = "$storeid:backup/$volume";
+    }

with

+    } else {
+	$volid = "$storeid:$volume";
+    }

but since it basically applies as-is to VMA backups and passing in just
the filename, it might be nice to have.. 

 PVE/API2/Storage/FileRestore.pm | 63 +++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 11 deletions(-)

diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
index f6dc74a..a4bad44 100644
--- a/PVE/API2/Storage/FileRestore.pm
+++ b/PVE/API2/Storage/FileRestore.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 
 use MIME::Base64;
+use PVE::Exception qw(raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::PBSClient;
 use PVE::Storage;
@@ -12,6 +13,26 @@ use PVE::Tools qw(extract_param);
 use PVE::RESTHandler;
 use base qw(PVE::RESTHandler);
 
+my $parse_volname_or_id = sub {
+    my ($storeid, $volume) = @_;
+
+    my $volid;
+    my ($sid, $volname) = PVE::Storage::parse_volume_id($volume, 1);
+
+    if (defined($sid)) {
+	raise_param_exc({ volume => "storage ID mismatch ($sid != $storeid)." })
+	    if $sid ne $storeid;
+
+	$volid = $volume;
+    } elsif ($volume =~ m/^backup\//) {
+	$volid = "$storeid:$volume";
+    } else {
+	$volid = "$storeid:backup/$volume";
+    }
+
+    return $volid;
+};
+
 __PACKAGE__->register_method ({
     name => 'list',
     path => 'list',
@@ -26,10 +47,13 @@ __PACKAGE__->register_method ({
 	additionalProperties => 0,
 	properties => {
 	    node => get_standard_option('pve-node'),
-	    storage => get_standard_option('pve-storage-id'),
-	    snapshot => {
-		description => "Backup snapshot identifier.",
+	    storage => get_standard_option('pve-storage-id', {
+		completion => \&PVE::Storage::complete_storage_enabled,
+	    }),
+	    volume => {
+		description => "Backup volume ID or name. Currently only PBS snapshots are supported.",
 		type => 'string',
+		completion => \&PVE::Storage::complete_volume,
 	    },
 	    filepath => {
 		description => 'base64-path to the directory or file being listed, or "/".',
@@ -80,18 +104,25 @@ __PACKAGE__->register_method ({
 
 	my $path = extract_param($param, 'filepath') || "/";
 	my $base64 = $path ne "/";
-	my $snap = extract_param($param, 'snapshot');
+
 	my $storeid = extract_param($param, 'storage');
+
+	my $volid = $parse_volname_or_id->($storeid, $param->{volume});
 	my $cfg = PVE::Storage::config();
 	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
 
-	my $volid = "$storeid:backup/$snap";
 	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $volid);
 
+	raise_param_exc({'storage' => "Only PBS storages supported for file-restore."})
+	    if $scfg->{type} ne 'pbs';
+
+	my ($vtype, $snap) = PVE::Storage::parse_volname($cfg, $volid);
+	raise_param_exc({'volume' => 'Not a backup archive.'})
+	    if $vtype ne 'backup';
+
 	my $client = PVE::PBSClient->new($scfg, $storeid);
 	my $ret = $client->file_restore_list($snap, $path, $base64);
 
-
 	# 'leaf' is a proper JSON boolean, map to perl-y bool
 	# TODO: make PBSClient decode all bools always as 1/0?
 	foreach my $item (@$ret) {
@@ -115,10 +146,13 @@ __PACKAGE__->register_method ({
 	additionalProperties => 0,
 	properties => {
 	    node => get_standard_option('pve-node'),
-	    storage => get_standard_option('pve-storage-id'),
-	    snapshot => {
-		description => "Backup snapshot identifier.",
+	    storage => get_standard_option('pve-storage-id', {
+		completion => \&PVE::Storage::complete_storage_enabled,
+	    }),
+	    volume => {
+		description => "Backup volume ID or name. Currently only PBS snapshots are supported.",
 		type => 'string',
+		completion => \&PVE::Storage::complete_volume,
 	    },
 	    filepath => {
 		description => 'base64-path to the directory or file to download.',
@@ -137,14 +171,21 @@ __PACKAGE__->register_method ({
 	my $user = $rpcenv->get_user();
 
 	my $path = extract_param($param, 'filepath');
-	my $snap = extract_param($param, 'snapshot');
 	my $storeid = extract_param($param, 'storage');
+	my $volid = $parse_volname_or_id->($storeid, $param->{volume});
+
 	my $cfg = PVE::Storage::config();
 	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
 
-	my $volid = "$storeid:backup/$snap";
 	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $volid);
 
+	raise_param_exc({'storage' => "Only PBS storages supported for file-restore."})
+	    if $scfg->{type} ne 'pbs';
+
+	my ($vtype, $snap) = PVE::Storage::parse_volname($cfg, $volid);
+	raise_param_exc({'volume' => 'Not a backup archive.'})
+	    if $vtype ne 'backup';
+
 	my $client = PVE::PBSClient->new($scfg, $storeid);
 	my $fifo = $client->file_restore_extract_prepare();
 
-- 
2.20.1

[pve-devel] applied: [PATCH v2 http-server 08/13] support streaming data form fh to client

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> Use an explicit AnyEvent::Handle similar to websocket proxying.
> 
> Needs some special care to make sure we apply backpressure correctly to
> avoid caching too much data. Note that because of AnyEvent restrictions,
> specifying a "fh" to point to a file or a packet-based socket may result
> in unwanted behaviour[0].
> 
> [0]: https://metacpan.org/pod/AnyEvent::Handle#DESCRIPTION
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> v2:
> * move to extra sub "response_stream"
> * adapt comments
> 
>  PVE/APIServer/AnyEvent.pm | 105 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 4 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 http-server 09/13] allow stream download from path and over pvedaemon-proxy

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> Allow specifying a filepath for stream=1 instead of either a path or fh
> with stream=1.
> 
> With this in place, we can also just return the path to the proxy in
> case we want to stream a response back, and let it read from the file
> itself. This way, the pvedaemon is cut out of the transfer pipe.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> v2:
> * unchanged
> 
>  PVE/APIServer/AnyEvent.pm | 40 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 common 04/13] PBSClient: adapt error message to include full package names

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> More helpful for a user to know what they're missing.
> 
> Suggested-by: Dominic Jäger <d.jaeger@proxmox.com>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/PVE/PBSClient.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks! but replaced to use the binary name (without /usr/bin) instead,
we know what command we want after all...

[pve-devel] applied: [PATCH v2 common 05/13] PBSClient: add file_restore_list command

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> v2:
> * one line per param
> 
>  src/PVE/PBSClient.pm | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 common 06/13] PBSClient: add file_restore_extract function

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> *_prepare creates a fifo for streaming data back to clients directly,
> filefile_restore_extract blocks and should be called from a background
> worker - while it is running outcoming data can be read from the FIFO.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> v2:
> * don't use "startcmd", just pass "opts" directly
> * better error handling
> 
>  src/PVE/PBSClient.pm | 56 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 common 07/13] PBSClient: use crypt params for file 'list' and 'extract'

[Thomas Lamprecht]

On 22.04.21 17:34, Stefan Reiter wrote:
> Necessary for accessing encrypted backups.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> new in v2
> 
>  src/PVE/PBSClient.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
>

applied, thanks! But I separated the USE_CRYPT_PARAMS list by command binary name,
to avoid undesired side-effects on shared command names.