[pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config

[pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config

[Fabian Ebner]

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2

 PVE/QemuServer.pm                      | 10 ++--
 test/Makefile                          |  5 +-
 test/restore-config-expected/139.conf  | 16 ++++++
 test/restore-config-expected/142.conf  | 15 ++++++
 test/restore-config-expected/1422.conf | 14 ++++++
 test/restore-config-expected/179.conf  | 17 +++++++
 test/restore-config-input/139.conf     | 18 +++++++
 test/restore-config-input/142.conf     | 16 ++++++
 test/restore-config-input/1422.conf    | 18 +++++++
 test/restore-config-input/179.conf     | 21 ++++++++
 test/run_qemu_restore_config_tests.pl  | 67 ++++++++++++++++++++++++++
 11 files changed, 211 insertions(+), 6 deletions(-)
 create mode 100644 test/restore-config-expected/139.conf
 create mode 100644 test/restore-config-expected/142.conf
 create mode 100644 test/restore-config-expected/1422.conf
 create mode 100644 test/restore-config-expected/179.conf
 create mode 100644 test/restore-config-input/139.conf
 create mode 100644 test/restore-config-input/142.conf
 create mode 100644 test/restore-config-input/1422.conf
 create mode 100644 test/restore-config-input/179.conf
 create mode 100755 test/run_qemu_restore_config_tests.pl

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1c0b5c2..a6e6ae0 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5899,7 +5899,7 @@ my $restore_allocate_devices = sub {
     return $map;
 };
 
-my $restore_update_config_line = sub {
+sub restore_update_config_line {
     my ($cookie, $vmid, $map, $line, $unique) = @_;
 
     return '' if $line =~ m/^\#qmdump\#/;
@@ -5966,7 +5966,7 @@ my $restore_update_config_line = sub {
     }
 
     return $res;
-};
+}
 
 my $restore_deactivate_volumes = sub {
     my ($storecfg, $devinfo) = @_;
@@ -6283,7 +6283,7 @@ sub restore_proxmox_backup_archive {
 
 	my $cookie = { netcount => 0 };
 	while (defined(my $line = <$fh>)) {
-	    $new_conf_raw .= $restore_update_config_line->(
+	    $new_conf_raw .= restore_update_config_line(
 		$cookie,
 		$vmid,
 		$map,
@@ -6456,7 +6456,7 @@ sub restore_vma_archive {
 
 	my $cookie = { netcount => 0 };
 	while (defined(my $line = <$fh>)) {
-	    $new_conf_raw .= $restore_update_config_line->(
+	    $new_conf_raw .= restore_update_config_line(
 		$cookie,
 		$vmid,
 		$map,
@@ -6611,7 +6611,7 @@ sub restore_tar_archive {
 
 	my $cookie = { netcount => 0 };
 	while (defined (my $line = <$srcfd>)) {
-	    $new_conf_raw .= $restore_update_config_line->(
+	    $new_conf_raw .= restore_update_config_line(
 		$cookie,
 		$vmid,
 		$map,
diff --git a/test/Makefile b/test/Makefile
index 5c26382..7a8487c 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -3,7 +3,7 @@ MIGRATION_TEST_TARGETS := $(addprefix test_migration_,$(MIGRATION_TEST_NAMES))
 
 all: test
 
-test: test_snapshot test_ovf test_cfg_to_cmd test_pci_addr_conflicts test_qemu_img_convert test_migration
+test: test_snapshot test_ovf test_cfg_to_cmd test_pci_addr_conflicts test_qemu_img_convert test_migration test_restore_config
 
 test_snapshot: run_snapshot_tests.pl
 	./run_snapshot_tests.pl
@@ -26,6 +26,9 @@ test_migration: run_qemu_migrate_tests.pl MigrationTest/*.pm $(MIGRATION_TEST_TA
 $(MIGRATION_TEST_TARGETS):
 	./run_qemu_migrate_tests.pl $(@:test_migration_%=%)
 
+test_restore_config: run_qemu_restore_config_tests.pl
+	./run_qemu_restore_config_tests.pl
+
 .PHONY: clean
 clean:
 	rm -rf MigrationTest/run
diff --git a/test/restore-config-expected/139.conf b/test/restore-config-expected/139.conf
new file mode 100644
index 0000000..94425f7
--- /dev/null
+++ b/test/restore-config-expected/139.conf
@@ -0,0 +1,16 @@
+# regular VM with an EFI disk
+bios: ovmf
+boot: order=scsi0;ide2;net0
+cores: 1
+efidisk0: target:139/vm-139-disk-0.qcow2,size=128K
+ide2: local:iso/debian-10.6.0-amd64-netinst.iso,media=cdrom
+memory: 2048
+name: eficloneclone
+net0: virtio=7A:6C:A5:8B:11:93,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:139/vm-139-disk-1.raw,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=21a7e7bc-3cd2-4232-a009-a41f4ee992ae
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-expected/142.conf b/test/restore-config-expected/142.conf
new file mode 100644
index 0000000..ac2d2ad
--- /dev/null
+++ b/test/restore-config-expected/142.conf
@@ -0,0 +1,15 @@
+# plain VM
+bootdisk: scsi0
+cores: 1
+ide2: none,media=cdrom
+memory: 512
+name: apache
+net0: virtio=92:38:11:FD:ED:87,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:142/vm-142-disk-0.qcow2,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=ddf91b3f-a597-42be-9a7e-fb6421dcd5cd
+sockets: 1
+vmgenid: 0
+tags: foo bar
diff --git a/test/restore-config-expected/1422.conf b/test/restore-config-expected/1422.conf
new file mode 100644
index 0000000..2d77a44
--- /dev/null
+++ b/test/restore-config-expected/1422.conf
@@ -0,0 +1,14 @@
+# some properties should be filtered
+bootdisk: scsi0
+cores: 1
+ide2: none,media=cdrom
+memory: 512
+name: apache
+net0: virtio=92:38:11:FD:ED:87,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:1422/vm-1422-disk-0.qcow2,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=ddf91b3f-a597-42be-9a7e-fb6421dcd5cd
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-expected/179.conf b/test/restore-config-expected/179.conf
new file mode 100644
index 0000000..4444efb
--- /dev/null
+++ b/test/restore-config-expected/179.conf
@@ -0,0 +1,17 @@
+# many disks
+boot: order=scsi0;ide2;net0
+cores: 1
+ide2: none,media=cdrom
+memory: 2048
+net0: virtio=26:15:5B:73:3F:7C,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:179/vm-179-disk-0.qcow2,cache=none,discard=on,size=32G,ssd=1
+scsi1: target:179/vm-179-disk-1.qcow2,cache=writethrough,size=32G
+scsi2: target:179/vm-179-disk-2.qcow2,mbps_rd=7,mbps_wr=7,replicate=0,size=32G
+scsi3: target:179/vm-179-disk-3.vmdk,size=32G
+#scsi4: myfs:179/vm-179-disk-1.qcow2,backup=0,size=32G
+scsihw: virtio-scsi-pci
+smbios1: uuid=1819ead7-a55d-4544-8d38-29ca94869a9c
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-input/139.conf b/test/restore-config-input/139.conf
new file mode 100644
index 0000000..5acb4d4
--- /dev/null
+++ b/test/restore-config-input/139.conf
@@ -0,0 +1,18 @@
+# regular VM with an EFI disk
+bios: ovmf
+boot: order=scsi0;ide2;net0
+cores: 1
+efidisk0: mydir:139/vm-139-disk-0.qcow2,size=128K
+ide2: local:iso/debian-10.6.0-amd64-netinst.iso,media=cdrom
+memory: 2048
+name: eficloneclone
+net0: virtio=7A:6C:A5:8B:11:93,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: rbdkvm:vm-139-disk-1,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=21a7e7bc-3cd2-4232-a009-a41f4ee992ae
+sockets: 1
+vmgenid: 0
+#qmdump#map:efidisk0:drive-efidisk0:mydir:qcow2:
+#qmdump#map:scsi0:drive-scsi0:rbdkvm::
diff --git a/test/restore-config-input/142.conf b/test/restore-config-input/142.conf
new file mode 100644
index 0000000..f3633aa
--- /dev/null
+++ b/test/restore-config-input/142.conf
@@ -0,0 +1,16 @@
+# plain VM
+bootdisk: scsi0
+cores: 1
+ide2: none,media=cdrom
+memory: 512
+name: apache
+net0: virtio=92:38:11:FD:ED:87,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: mydir:142/vm-142-disk-0.qcow2,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=ddf91b3f-a597-42be-9a7e-fb6421dcd5cd
+sockets: 1
+vmgenid: 0
+tags: foo bar
+#qmdump#map:scsi0:drive-scsi0:mydir:qcow2:
diff --git a/test/restore-config-input/1422.conf b/test/restore-config-input/1422.conf
new file mode 100644
index 0000000..d315502
--- /dev/null
+++ b/test/restore-config-input/1422.conf
@@ -0,0 +1,18 @@
+# some properties should be filtered
+bootdisk: scsi0
+cores: 1
+ide2: none,media=cdrom
+memory: 512
+name: apache
+net0: virtio=92:38:11:FD:ED:87,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: mydir:1422/vm-1422-disk-0.qcow2,size=4G
+unused7: mydir:1422/vm-1422-disk-8.qcow2
+parent: snap
+lock: backup
+scsihw: virtio-scsi-pci
+smbios1: uuid=ddf91b3f-a597-42be-9a7e-fb6421dcd5cd
+sockets: 1
+vmgenid: 0
+#qmdump#map:scsi0:drive-scsi0:mydir:qcow2:
diff --git a/test/restore-config-input/179.conf b/test/restore-config-input/179.conf
new file mode 100644
index 0000000..e1ee01a
--- /dev/null
+++ b/test/restore-config-input/179.conf
@@ -0,0 +1,21 @@
+# many disks
+boot: order=scsi0;ide2;net0
+cores: 1
+ide2: none,media=cdrom
+memory: 2048
+net0: virtio=26:15:5B:73:3F:7C,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: myfs:179/vm-179-disk-4.qcow2,cache=none,discard=on,size=32G,ssd=1
+scsi1: myfs:179/vm-179-disk-0.qcow2,cache=writethrough,size=32G
+scsi2: myfs:179/vm-179-disk-2.qcow2,mbps_rd=7,mbps_wr=7,replicate=0,size=32G
+scsi3: myfs:179/vm-179-disk-3.vmdk,size=32G
+scsi4: myfs:179/vm-179-disk-1.qcow2,backup=0,size=32G
+scsihw: virtio-scsi-pci
+smbios1: uuid=1819ead7-a55d-4544-8d38-29ca94869a9c
+sockets: 1
+vmgenid: 0
+#qmdump#map:scsi0:drive-scsi0:myfs:qcow2:
+#qmdump#map:scsi1:drive-scsi1:myfs:qcow2:
+#qmdump#map:scsi2:drive-scsi2:myfs:qcow2:
+#qmdump#map:scsi3:drive-scsi3:myfs:vmdk:
diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
new file mode 100755
index 0000000..d829216
--- /dev/null
+++ b/test/run_qemu_restore_config_tests.pl
@@ -0,0 +1,67 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::More;
+
+use File::Basename;
+
+use PVE::QemuServer;
+use PVE::Tools qw(dir_glob_foreach file_get_contents);
+
+my $INPUT_DIR = './restore-config-input';
+my $EXPECTED_DIR = './restore-config-expected';
+
+# NOTE update when you add/remove tests
+plan tests => 4;
+
+dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
+    my ($file) = @_;
+
+    my $vmid = basename($file, ('.conf'));
+
+    my $fh = IO::File->new("${INPUT_DIR}/${file}", "r") or
+	die "unable to read '$file' - $!\n";
+
+    my $map = {};
+    my $disknum = 0;
+
+    # NOTE For now, the map is hardcoded to a file-based 'target' storage.
+    # In the future, the test could be extended to include parse_backup_hints
+    # and restore_allocate_devices. Even better if the config-related logic from
+    # the restore_XYZ_archive functions could become a separate function.
+    while (defined(my $line = <$fh>)) {
+	if ($line =~ m/^\#qmdump\#map:(\S+):(\S+):(\S*):(\S*):$/) {
+	    my ($drive, undef, $storeid, $fmt) = ($1, $2, $3, $4);
+
+	    $fmt ||= 'raw';
+
+	    $map->{$drive} = "target:${vmid}/vm-${vmid}-disk-${disknum}.${fmt}";
+	    $disknum++;
+	}
+    }
+
+    $fh->seek(0, 0) or die "seek failed - $!\n";
+
+    my $got = '';
+    my $cookie = { netcount => 0 };
+
+    while (defined(my $line = <$fh>)) {
+	$got .= PVE::QemuServer::restore_update_config_line(
+	    $cookie,
+	    $vmid,
+	    $map,
+	    $line,
+	    0,
+	);
+    }
+
+    my $expected = file_get_contents("${EXPECTED_DIR}/${file}");
+
+    is_deeply($got, $expected, $file);
+});
+
+done_testing();
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2

 PVE/QemuServer.pm                     | 5 +----
 test/run_qemu_restore_config_tests.pl | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a6e6ae0..5fb052c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5900,7 +5900,7 @@ my $restore_allocate_devices = sub {
 };
 
 sub restore_update_config_line {
-    my ($cookie, $vmid, $map, $line, $unique) = @_;
+    my ($cookie, $map, $line, $unique) = @_;
 
     return '' if $line =~ m/^\#qmdump\#/;
     return '' if $line =~ m/^\#vzdump\#/;
@@ -6285,7 +6285,6 @@ sub restore_proxmox_backup_archive {
 	while (defined(my $line = <$fh>)) {
 	    $new_conf_raw .= restore_update_config_line(
 		$cookie,
-		$vmid,
 		$map,
 		$line,
 		$options->{unique},
@@ -6458,7 +6457,6 @@ sub restore_vma_archive {
 	while (defined(my $line = <$fh>)) {
 	    $new_conf_raw .= restore_update_config_line(
 		$cookie,
-		$vmid,
 		$map,
 		$line,
 		$opts->{unique},
@@ -6613,7 +6611,6 @@ sub restore_tar_archive {
 	while (defined (my $line = <$srcfd>)) {
 	    $new_conf_raw .= restore_update_config_line(
 		$cookie,
-		$vmid,
 		$map,
 		$line,
 		$opts->{unique},
diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
index d829216..e5d9f2a 100755
--- a/test/run_qemu_restore_config_tests.pl
+++ b/test/run_qemu_restore_config_tests.pl
@@ -52,7 +52,6 @@ dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
     while (defined(my $line = <$fh>)) {
 	$got .= PVE::QemuServer::restore_update_config_line(
 	    $cookie,
-	    $vmid,
 	    $map,
 	    $line,
 	    0,
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users

[Fabian Ebner]

by dropping privileged options for unprivileged users. For backwards
compatibility for in-place restores, keep the option as long as the value didn't
change.

Note that this softly "breaks" restoring a backup with such a privileged option
under a new VM ID in the sense that the options won't be present in the new VM
configuration. Restoring itself still works. Restoring containers already
behaves similarly.

In a trusted environment, there cannot be any backups that were tampered with,
but it's still worth adding such checks for resilience and future-proofing.

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

Changes from v1:
    * don't capitalize warnings as much
    * add tests
    * add Reported-by tag

 PVE/QemuServer.pm                           | 71 +++++++++++++++++++++
 test/restore-config-expected/1791.conf.root | 17 +++++
 test/restore-config-expected/1791.conf.user | 12 ++++
 test/restore-config-expected/314.conf       | 22 +++++++
 test/restore-config-expected/3141.conf.root | 24 +++++++
 test/restore-config-expected/3141.conf.user | 16 +++++
 test/restore-config-input/1791.conf         | 17 +++++
 test/restore-config-input/314.conf          | 24 +++++++
 test/restore-config-input/3141.conf         | 28 ++++++++
 test/restore-config-old/142.conf            | 14 ++++
 test/restore-config-old/314.conf            | 21 ++++++
 test/run_qemu_restore_config_tests.pl       | 32 +++++++++-
 12 files changed, 295 insertions(+), 3 deletions(-)
 create mode 100644 test/restore-config-expected/1791.conf.root
 create mode 100644 test/restore-config-expected/1791.conf.user
 create mode 100644 test/restore-config-expected/314.conf
 create mode 100644 test/restore-config-expected/3141.conf.root
 create mode 100644 test/restore-config-expected/3141.conf.user
 create mode 100644 test/restore-config-input/1791.conf
 create mode 100644 test/restore-config-input/314.conf
 create mode 100644 test/restore-config-input/3141.conf
 create mode 100644 test/restore-config-old/142.conf
 create mode 100644 test/restore-config-old/314.conf

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 5fb052c..69f7be2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5899,6 +5899,67 @@ my $restore_allocate_devices = sub {
     return $map;
 };
 
+# Make sure user is allowed to have options in config.
+sub sanitize_restored_config {
+    my ($new_config_raw, $oldconf, $authuser) = @_;
+
+    return $new_config_raw if $authuser eq 'root@pam';
+
+    my $res = '';
+    $oldconf //= {};
+
+    # serialN, usbN, etc. handled below
+    my $rootonlyoptions = {
+	args => 1,
+	lock => 1,
+	parent => 1,
+	hookscript => 1,
+    };
+
+    # anything other than 'none' and volume IDs are disallowed here
+    my $is_bad_drive = sub {
+	my ($key, $value) = @_;
+	my $drive = parse_drive($key, $value) // {};
+	my $volid = $drive->{file} // '';
+	return 0 if $volid eq 'none';
+	return 1 if $volid eq 'cdrom'; # disallow physical CD/DVD drive
+	$volid = PVE::Storage::parse_volume_id($volid, 1);
+	return 1 if !defined($volid);
+    };
+
+    my @lines = split(/\n/, $new_config_raw);
+    foreach my $line (@lines) {
+	if ($line =~ m/^#/) {
+	    $res .= "$line\n";
+	} elsif ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) {
+	    my $key = $1;
+	    my $value = $2;
+	    my $oldvalue = $oldconf->{$key};
+
+	    if (defined($oldvalue) && $oldvalue eq $value) {
+		$res .= "$line\n";
+		next;
+	    }
+
+	    if ($rootonlyoptions->{$key} ||
+		(is_valid_drivename($key) && $is_bad_drive->($key, $value)) ||
+		($key =~ m/^serial\d+$/ && $value ne 'socket') ||
+		($key =~ m/^usb\d+$/ && $value !~ m/spice/) ||
+		$key =~ m/^parallel\d+$/ ||
+		$key =~ m/^hostpci\d+$/) {
+		warn "WARN: skipping configuration line '$line' due to " .
+		    "privilege restrictions - restore as root to include it.\n";
+	    } else {
+		$res .= "$line\n";
+	    }
+	} else {
+	    warn "WARN: invalid configuration line '$line'.\n";
+	}
+    }
+
+    return $res;
+}
+
 sub restore_update_config_line {
     my ($cookie, $map, $line, $unique) = @_;
 
@@ -6304,6 +6365,8 @@ sub restore_proxmox_backup_archive {
 	die $err;
     }
 
+    $new_conf_raw = sanitize_restored_config($new_conf_raw, $oldconf, $user);
+
     PVE::Tools::file_set_contents($conffile, $new_conf_raw);
 
     PVE::Cluster::cfs_update(); # make sure we read new file
@@ -6518,6 +6581,8 @@ sub restore_vma_archive {
 	die $err;
     }
 
+    $new_conf_raw = sanitize_restored_config($new_conf_raw, $oldconf, $user);
+
     PVE::Tools::file_set_contents($conffile, $new_conf_raw);
 
     PVE::Cluster::cfs_update(); # make sure we read new file
@@ -6537,6 +6602,10 @@ sub restore_tar_archive {
 
     my $storecfg = PVE::Storage::config();
 
+    # Note: $oldconf is undef if VM does not exists
+    my $cfs_path = PVE::QemuConfig->cfs_config_path($vmid);
+    my $oldconf = PVE::Cluster::cfs_read_file($cfs_path);
+
     # avoid zombie disks when restoring over an existing VM -> cleanup first
     # pass keep_empty_config=1 to keep the config (thus VMID) reserved for us
     # skiplock=1 because qmrestore has set the 'create' lock itself already
@@ -6626,6 +6695,8 @@ sub restore_tar_archive {
 
     rmtree $tmpdir;
 
+    $new_conf_raw = sanitize_restored_config($new_conf_raw, $oldconf, $user);
+
     PVE::Tools::file_set_contents($conffile, $new_conf_raw);
 
     PVE::Cluster::cfs_update(); # make sure we read new file
diff --git a/test/restore-config-expected/1791.conf.root b/test/restore-config-expected/1791.conf.root
new file mode 100644
index 0000000..2c0e9c4
--- /dev/null
+++ b/test/restore-config-expected/1791.conf.root
@@ -0,0 +1,17 @@
+# absolute disk paths, no map
+boot: order=scsi0;ide2;net0
+cores: 1
+ide2: none,media=cdrom
+memory: 2048
+net0: virtio=26:15:5B:73:3F:7C,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi3: /dev/sda1
+efidisk0: /dev/sda2
+sata2: /dev/sdb3
+ide0: /dev/sdc
+virtio3: /dev/sdc2
+scsihw: virtio-scsi-pci
+smbios1: uuid=1819ead7-a55d-4544-8d38-29ca94869a9c
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-expected/1791.conf.user b/test/restore-config-expected/1791.conf.user
new file mode 100644
index 0000000..2797397
--- /dev/null
+++ b/test/restore-config-expected/1791.conf.user
@@ -0,0 +1,12 @@
+# absolute disk paths, no map
+boot: order=scsi0;ide2;net0
+cores: 1
+ide2: none,media=cdrom
+memory: 2048
+net0: virtio=26:15:5B:73:3F:7C,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsihw: virtio-scsi-pci
+smbios1: uuid=1819ead7-a55d-4544-8d38-29ca94869a9c
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-expected/314.conf b/test/restore-config-expected/314.conf
new file mode 100644
index 0000000..129fc4d
--- /dev/null
+++ b/test/restore-config-expected/314.conf
@@ -0,0 +1,22 @@
+# many privileged options
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:314/vm-314-disk-0.qcow2,size=4G
+scsi1: target:314/vm-314-disk-1.raw
+scsihw: virtio-scsi-pci
+serial0: socket
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+serial1: /dev/ttyS0
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+sockets: 1
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+vmgenid: 0
diff --git a/test/restore-config-expected/3141.conf.root b/test/restore-config-expected/3141.conf.root
new file mode 100644
index 0000000..4445b08
--- /dev/null
+++ b/test/restore-config-expected/3141.conf.root
@@ -0,0 +1,24 @@
+# privileged options no in-place restore
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:3141/vm-3141-disk-0.qcow2,size=4G
+scsi1: target:3141/vm-3141-disk-1.raw
+scsihw: virtio-scsi-pci
+serial0: socket
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+serial1: /dev/ttyS0
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+hookscript: /tmp/script.sh
+sockets: 1
+parallel0: /dev/parport3
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+vmgenid: 0
diff --git a/test/restore-config-expected/3141.conf.user b/test/restore-config-expected/3141.conf.user
new file mode 100644
index 0000000..8b5dc97
--- /dev/null
+++ b/test/restore-config-expected/3141.conf.user
@@ -0,0 +1,16 @@
+# privileged options no in-place restore
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:3141/vm-3141-disk-0.qcow2,size=4G
+scsi1: target:3141/vm-3141-disk-1.raw
+scsihw: virtio-scsi-pci
+serial0: socket
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+sockets: 1
+usb0: spice,usb3=1
+vmgenid: 0
diff --git a/test/restore-config-input/1791.conf b/test/restore-config-input/1791.conf
new file mode 100644
index 0000000..2c0e9c4
--- /dev/null
+++ b/test/restore-config-input/1791.conf
@@ -0,0 +1,17 @@
+# absolute disk paths, no map
+boot: order=scsi0;ide2;net0
+cores: 1
+ide2: none,media=cdrom
+memory: 2048
+net0: virtio=26:15:5B:73:3F:7C,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi3: /dev/sda1
+efidisk0: /dev/sda2
+sata2: /dev/sdb3
+ide0: /dev/sdc
+virtio3: /dev/sdc2
+scsihw: virtio-scsi-pci
+smbios1: uuid=1819ead7-a55d-4544-8d38-29ca94869a9c
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-input/314.conf b/test/restore-config-input/314.conf
new file mode 100644
index 0000000..7307e50
--- /dev/null
+++ b/test/restore-config-input/314.conf
@@ -0,0 +1,24 @@
+# many privileged options
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: mydir:314/vm-314-disk-0.qcow2,size=4G
+scsi1: /dev/sdl9
+scsihw: virtio-scsi-pci
+serial0: socket
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+serial1: /dev/ttyS0
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+sockets: 1
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+vmgenid: 0
+#qmdump#map:scsi0:drive-scsi0:mydir:qcow2:
+#qmdump#map:scsi1:drive-scsi1::raw:
diff --git a/test/restore-config-input/3141.conf b/test/restore-config-input/3141.conf
new file mode 100644
index 0000000..d61a502
--- /dev/null
+++ b/test/restore-config-input/3141.conf
@@ -0,0 +1,28 @@
+# privileged options no in-place restore
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: mydir:3141/vm-3141-disk-0.qcow2,size=4G
+scsi1: /dev/sdl9
+scsihw: virtio-scsi-pci
+serial0: socket
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+serial1: /dev/ttyS0
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+hookscript: /tmp/script.sh
+sockets: 1
+parallel0: /dev/parport3
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+lock: backup
+parent: snap
+vmgenid: 0
+#qmdump#map:scsi0:drive-scsi0:mydir:qcow2:
+#qmdump#map:scsi1:drive-scsi1::raw:
diff --git a/test/restore-config-old/142.conf b/test/restore-config-old/142.conf
new file mode 100644
index 0000000..b39b8a4
--- /dev/null
+++ b/test/restore-config-old/142.conf
@@ -0,0 +1,14 @@
+# plain VM
+bootdisk: scsi0
+cores: 2
+ide2: none,media=cdrom
+memory: 2048
+name: apachenew
+net0: virtio=92:38:11:FD:ED:87,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: mydir:142/vm-142-disk-1.qcow2,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=ddf91b3f-a597-42be-9a7e-fb6421dcd5cd
+sockets: 1
+vmgenid: a0779fce-f7bc-4d9a-af06-e2fce88659ba
diff --git a/test/restore-config-old/314.conf b/test/restore-config-old/314.conf
new file mode 100644
index 0000000..42ad8aa
--- /dev/null
+++ b/test/restore-config-old/314.conf
@@ -0,0 +1,21 @@
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:314/vm-314-disk-0.qcow2,size=4G
+scsi1: /dev/sdl9
+scsihw: virtio-scsi-pci
+serial0: socket
+serial1: /dev/ttyS0
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+sockets: 1
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+vmgenid: 3166c634-c2cf-492f-aa55-c8f757adc218
diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
index e5d9f2a..789711a 100755
--- a/test/run_qemu_restore_config_tests.pl
+++ b/test/run_qemu_restore_config_tests.pl
@@ -13,10 +13,13 @@ use PVE::QemuServer;
 use PVE::Tools qw(dir_glob_foreach file_get_contents);
 
 my $INPUT_DIR = './restore-config-input';
+my $OLD_DIR = './restore-config-old';
 my $EXPECTED_DIR = './restore-config-expected';
 
 # NOTE update when you add/remove tests
-plan tests => 4;
+my $NUMBER_OF_FILES = 7;
+
+plan tests => 2 * $NUMBER_OF_FILES;
 
 dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
     my ($file) = @_;
@@ -58,9 +61,32 @@ dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
 	);
     }
 
-    my $expected = file_get_contents("${EXPECTED_DIR}/${file}");
+    my $old_conf;
+    if (-f "${OLD_DIR}/${file}") {
+	my $old_raw = file_get_contents("${OLD_DIR}/${file}");
+	$old_conf = PVE::QemuServer::parse_vm_config("/qemu-server/${vmid}.conf", $old_raw);
+    }
+
+    my $got_root = PVE::QemuServer::sanitize_restored_config($got, $old_conf, 'root@pam');
+    my $got_user = PVE::QemuServer::sanitize_restored_config($got, $old_conf, 'user@pve');
+
+    my $expected_root;
+    my $expected_user;
+
+    my $expected_file = "${EXPECTED_DIR}/${file}";
+
+    if (-f "${expected_file}.root" && -f "${expected_file}.user") {
+	$expected_root = file_get_contents("${expected_file}.root");
+	$expected_user = file_get_contents("${expected_file}.user");
+    } elsif (-f $expected_file) {
+	$expected_root = file_get_contents($expected_file);
+	$expected_user = $expected_root;
+    } else {
+	die "either provide ID.conf or both ID.conf.root and ID.conf.user\n";
+    }
 
-    is_deeply($got, $expected, $file);
+    is_deeply($got_root, $expected_root, $file);
+    is_deeply($got_user, $expected_user, $file);
 });
 
 done_testing();
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 4/4] restore: sanitize config: use new warning system

[Fabian Ebner]

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Depends on pve-common having [0] or any other implementation of warn().

[0]: https://lists.proxmox.com/pipermail/pve-devel/2021-March/047303.html

 PVE/QemuServer.pm                     |  8 +++++---
 test/run_qemu_restore_config_tests.pl | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 69f7be2..11561be 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5905,6 +5905,8 @@ sub sanitize_restored_config {
 
     return $new_config_raw if $authuser eq 'root@pam';
 
+    my $rpcenv = PVE::RPCEnvironment::get();
+
     my $res = '';
     $oldconf //= {};
 
@@ -5947,13 +5949,13 @@ sub sanitize_restored_config {
 		($key =~ m/^usb\d+$/ && $value !~ m/spice/) ||
 		$key =~ m/^parallel\d+$/ ||
 		$key =~ m/^hostpci\d+$/) {
-		warn "WARN: skipping configuration line '$line' due to " .
-		    "privilege restrictions - restore as root to include it.\n";
+		$rpcenv->warn("skipping configuration line '$line' due to " .
+		    "privilege restrictions - restore as root to include it.");
 	    } else {
 		$res .= "$line\n";
 	    }
 	} else {
-	    warn "WARN: invalid configuration line '$line'.\n";
+	    $rpcenv->warn("invalid configuration line '$line'.");
 	}
     }
 
diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
index 789711a..7e3a1cb 100755
--- a/test/run_qemu_restore_config_tests.pl
+++ b/test/run_qemu_restore_config_tests.pl
@@ -6,6 +6,7 @@ use warnings;
 use lib qw(..);
 
 use Test::More;
+use Test::MockModule;
 
 use File::Basename;
 
@@ -21,6 +22,19 @@ my $NUMBER_OF_FILES = 7;
 
 plan tests => 2 * $NUMBER_OF_FILES;
 
+my $rpc_environment_module = Test::MockModule->new('PVE::RPCEnvironment');
+$rpc_environment_module->mock(
+    get => sub {
+	my $self = {};
+	bless $self, 'PVE::RPCEnvironment';
+	return $self;
+    },
+    warn => sub {
+	my ($self, $message) = @_;
+	warn "$message\n";
+    },
+);
+
 dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
     my ($file) = @_;
 
-- 
2.20.1

[pve-devel] applied: [PATCH v2 qemu-server 1/4] test: add tests for restoring config

[Thomas Lamprecht]

On 18.03.21 10:44, Fabian Ebner wrote:
> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2
> 
>  PVE/QemuServer.pm                      | 10 ++--
>  test/Makefile                          |  5 +-
>  test/restore-config-expected/139.conf  | 16 ++++++
>  test/restore-config-expected/142.conf  | 15 ++++++
>  test/restore-config-expected/1422.conf | 14 ++++++
>  test/restore-config-expected/179.conf  | 17 +++++++
>  test/restore-config-input/139.conf     | 18 +++++++
>  test/restore-config-input/142.conf     | 16 ++++++
>  test/restore-config-input/1422.conf    | 18 +++++++
>  test/restore-config-input/179.conf     | 21 ++++++++
>  test/run_qemu_restore_config_tests.pl  | 67 ++++++++++++++++++++++++++
>  11 files changed, 211 insertions(+), 6 deletions(-)
>  create mode 100644 test/restore-config-expected/139.conf
>  create mode 100644 test/restore-config-expected/142.conf
>  create mode 100644 test/restore-config-expected/1422.conf
>  create mode 100644 test/restore-config-expected/179.conf
>  create mode 100644 test/restore-config-input/139.conf
>  create mode 100644 test/restore-config-input/142.conf
>  create mode 100644 test/restore-config-input/1422.conf
>  create mode 100644 test/restore-config-input/179.conf
>  create mode 100755 test/run_qemu_restore_config_tests.pl
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter

[Thomas Lamprecht]

On 18.03.21 10:44, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2
> 
>  PVE/QemuServer.pm                     | 5 +----
>  test/run_qemu_restore_config_tests.pl | 1 -
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users

[Thomas Lamprecht]

On 18.03.21 10:44, Fabian Ebner wrote:
> by dropping privileged options for unprivileged users. For backwards
> compatibility for in-place restores, keep the option as long as the value didn't
> change.
> 
> Note that this softly "breaks" restoring a backup with such a privileged option
> under a new VM ID in the sense that the options won't be present in the 
new VM
> configuration. Restoring itself still works. Restoring containers already
> behaves similarly.
> 
> In a trusted environment, there cannot be any backups that were tampered with,
> but it's still worth adding such checks for resilience and future-proofing.
> 
> Reported-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes from v1:
>     * don't capitalize warnings as much
>     * add tests
>     * add Reported-by tag
> 

waiting out this one for when we can apply it for 7.0, ideally we can define some
better node HW permissions (e.g., for PCI) then and improve this by allowing more
things to be restored as non-root while being safe.