* [pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config @ 2021-03-18 9:44 Fabian Ebner 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter Fabian Ebner ` (3 more replies) 0 siblings, 4 replies; 7+ messages in thread From: Fabian Ebner @ 2021-03-18 9:44 UTC (permalink / raw) To: pve-devel; +Cc: Thomas Lamprecht 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter 2021-03-18 9:44 [pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config Fabian Ebner @ 2021-03-18 9:44 ` Fabian Ebner 2021-04-18 16:15 ` [pve-devel] applied: " Thomas Lamprecht 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users Fabian Ebner ` (2 subsequent siblings) 3 siblings, 1 reply; 7+ messages in thread From: Fabian Ebner @ 2021-03-18 9:44 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] applied: [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter Fabian Ebner @ 2021-04-18 16:15 ` Thomas Lamprecht 0 siblings, 0 replies; 7+ messages in thread From: Thomas Lamprecht @ 2021-04-18 16:15 UTC (permalink / raw) To: Proxmox VE development discussion, Fabian Ebner 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users 2021-03-18 9:44 [pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config Fabian Ebner 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter Fabian Ebner @ 2021-03-18 9:44 ` Fabian Ebner 2021-04-18 16:17 ` Thomas Lamprecht 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 4/4] restore: sanitize config: use new warning system Fabian Ebner 2021-04-18 16:15 ` [pve-devel] applied: [PATCH v2 qemu-server 1/4] test: add tests for restoring config Thomas Lamprecht 3 siblings, 1 reply; 7+ messages in thread From: Fabian Ebner @ 2021-03-18 9:44 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users Fabian Ebner @ 2021-04-18 16:17 ` Thomas Lamprecht 0 siblings, 0 replies; 7+ messages in thread From: Thomas Lamprecht @ 2021-04-18 16:17 UTC (permalink / raw) To: Proxmox VE development discussion, Fabian Ebner 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH v2 qemu-server 4/4] restore: sanitize config: use new warning system 2021-03-18 9:44 [pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config Fabian Ebner 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter Fabian Ebner 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users Fabian Ebner @ 2021-03-18 9:44 ` Fabian Ebner 2021-04-18 16:15 ` [pve-devel] applied: [PATCH v2 qemu-server 1/4] test: add tests for restoring config Thomas Lamprecht 3 siblings, 0 replies; 7+ messages in thread From: Fabian Ebner @ 2021-03-18 9:44 UTC (permalink / raw) To: pve-devel; +Cc: Thomas Lamprecht 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] applied: [PATCH v2 qemu-server 1/4] test: add tests for restoring config 2021-03-18 9:44 [pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config Fabian Ebner ` (2 preceding siblings ...) 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 4/4] restore: sanitize config: use new warning system Fabian Ebner @ 2021-04-18 16:15 ` Thomas Lamprecht 3 siblings, 0 replies; 7+ messages in thread From: Thomas Lamprecht @ 2021-04-18 16:15 UTC (permalink / raw) To: Proxmox VE development discussion, Fabian Ebner 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-18 16:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-18 9:44 [pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config Fabian Ebner 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter Fabian Ebner 2021-04-18 16:15 ` [pve-devel] applied: " Thomas Lamprecht 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users Fabian Ebner 2021-04-18 16:17 ` Thomas Lamprecht 2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 4/4] restore: sanitize config: use new warning system Fabian Ebner 2021-04-18 16:15 ` [pve-devel] applied: [PATCH v2 qemu-server 1/4] test: add tests for restoring config Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox