* [pve-devel] [PATCH qemu-server 1/2] restore: write new config to variable first
@ 2021-03-08 12:26 Fabian Ebner
2021-03-08 12:26 ` [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users Fabian Ebner
2021-03-09 7:05 ` [pve-devel] applied: [PATCH qemu-server 1/2] restore: write new config to variable first Thomas Lamprecht
0 siblings, 2 replies; 6+ messages in thread
From: Fabian Ebner @ 2021-03-08 12:26 UTC (permalink / raw)
To: pve-devel
and use file_set_contents to really commit it afterwards. Mostly done as a
preparation for the later patch for sanitizing the config on restore, but
shouldn't hurt by itself either.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
PVE/QemuServer.pm | 83 +++++++++++++++++++++++++----------------------
1 file changed, 45 insertions(+), 38 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0ac4fcf..1410ecb 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5876,13 +5876,15 @@ my $restore_allocate_devices = sub {
};
my $restore_update_config_line = sub {
- my ($outfd, $cookie, $vmid, $map, $line, $unique) = @_;
+ my ($cookie, $vmid, $map, $line, $unique) = @_;
- return if $line =~ m/^\#qmdump\#/;
- return if $line =~ m/^\#vzdump\#/;
- return if $line =~ m/^lock:/;
- return if $line =~ m/^unused\d+:/;
- return if $line =~ m/^parent:/;
+ return '' if $line =~ m/^\#qmdump\#/;
+ return '' if $line =~ m/^\#vzdump\#/;
+ return '' if $line =~ m/^lock:/;
+ return '' if $line =~ m/^unused\d+:/;
+ return '' if $line =~ m/^parent:/;
+
+ my $res = '';
my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
if (($line =~ m/^(vlan(\d+)):\s*(\S+)\s*$/)) {
@@ -5898,7 +5900,7 @@ my $restore_update_config_line = sub {
};
my $netstr = print_net($net);
- print $outfd "net$cookie->{netcount}: $netstr\n";
+ $res .= "net$cookie->{netcount}: $netstr\n";
$cookie->{netcount}++;
}
} elsif (($line =~ m/^(net\d+):\s*(\S+)\s*$/) && $unique) {
@@ -5906,20 +5908,20 @@ my $restore_update_config_line = sub {
my $net = parse_net($netstr);
$net->{macaddr} = PVE::Tools::random_ether_addr($dc->{mac_prefix}) if $net->{macaddr};
$netstr = print_net($net);
- print $outfd "$id: $netstr\n";
+ $res .= "$id: $netstr\n";
} elsif ($line =~ m/^((ide|scsi|virtio|sata|efidisk)\d+):\s*(\S+)\s*$/) {
my $virtdev = $1;
my $value = $3;
my $di = parse_drive($virtdev, $value);
if (defined($di->{backup}) && !$di->{backup}) {
- print $outfd "#$line";
+ $res .= "#$line";
} elsif ($map->{$virtdev}) {
delete $di->{format}; # format can change on restore
$di->{file} = $map->{$virtdev};
$value = print_drive($di);
- print $outfd "$virtdev: $value\n";
+ $res .= "$virtdev: $value\n";
} else {
- print $outfd $line;
+ $res .= $line;
}
} elsif (($line =~ m/^vmgenid: (.*)/)) {
my $vmgenid = $1;
@@ -5927,17 +5929,19 @@ my $restore_update_config_line = sub {
# always generate a new vmgenid if there was a valid one setup
$vmgenid = generate_uuid();
}
- print $outfd "vmgenid: $vmgenid\n";
+ $res .= "vmgenid: $vmgenid\n";
} elsif (($line =~ m/^(smbios1: )(.*)/) && $unique) {
my ($uuid, $uuid_str);
UUID::generate($uuid);
UUID::unparse($uuid, $uuid_str);
my $smbios1 = parse_smbios1($2);
$smbios1->{uuid} = $uuid_str;
- print $outfd $1.print_smbios1($smbios1)."\n";
+ $res .= $1.print_smbios1($smbios1)."\n";
} else {
- print $outfd $line;
+ $res .= $line;
}
+
+ return $res;
};
my $restore_deactivate_volumes = sub {
@@ -6141,7 +6145,6 @@ sub restore_proxmox_backup_archive {
mkpath $tmpdir;
my $conffile = PVE::QemuConfig->config_file($vmid);
- my $tmpfn = "$conffile.$$.tmp";
# disable interrupts (always do cleanups)
local $SIG{INT} =
local $SIG{TERM} =
@@ -6151,6 +6154,7 @@ sub restore_proxmox_backup_archive {
# 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);
+ my $new_conf_raw = '';
my $rpcenv = PVE::RPCEnvironment::get();
my $devinfo = {};
@@ -6253,15 +6257,18 @@ sub restore_proxmox_backup_archive {
$fh->seek(0, 0) || die "seek failed - $!\n";
- my $outfd = IO::File->new($tmpfn, "w") || die "unable to write config for VM $vmid\n";
-
my $cookie = { netcount => 0 };
while (defined(my $line = <$fh>)) {
- $restore_update_config_line->($outfd, $cookie, $vmid, $map, $line, $options->{unique});
+ $new_conf_raw .= $restore_update_config_line->(
+ $cookie,
+ $vmid,
+ $map,
+ $line,
+ $options->{unique},
+ );
}
$fh->close();
- $outfd->close();
};
my $err = $@;
@@ -6270,13 +6277,11 @@ sub restore_proxmox_backup_archive {
rmtree $tmpdir;
if ($err) {
- unlink $tmpfn;
$restore_destroy_volumes->($storecfg, $devinfo);
die $err;
}
- rename($tmpfn, $conffile) ||
- die "unable to commit configuration file '$conffile'\n";
+ PVE::Tools::file_set_contents($conffile, $new_conf_raw);
PVE::Cluster::cfs_update(); # make sure we read new file
@@ -6351,11 +6356,11 @@ sub restore_vma_archive {
my $rpcenv = PVE::RPCEnvironment::get();
my $conffile = PVE::QemuConfig->config_file($vmid);
- my $tmpfn = "$conffile.$$.tmp";
# Note: $oldconf is undef if VM does not exist
my $cfs_path = PVE::QemuConfig->cfs_config_path($vmid);
my $oldconf = PVE::Cluster::cfs_read_file($cfs_path);
+ my $new_conf_raw = '';
my %storage_limits;
@@ -6423,15 +6428,18 @@ sub restore_vma_archive {
$fh->seek(0, 0) || die "seek failed - $!\n";
- my $outfd = IO::File->new($tmpfn, "w") || die "unable to write config for VM $vmid\n";
-
my $cookie = { netcount => 0 };
while (defined(my $line = <$fh>)) {
- $restore_update_config_line->($outfd, $cookie, $vmid, $map, $line, $opts->{unique});
+ $new_conf_raw .= $restore_update_config_line->(
+ $cookie,
+ $vmid,
+ $map,
+ $line,
+ $opts->{unique},
+ );
}
$fh->close();
- $outfd->close();
};
eval {
@@ -6482,13 +6490,11 @@ sub restore_vma_archive {
rmtree $tmpdir;
if ($err) {
- unlink $tmpfn;
$restore_destroy_volumes->($cfg, $devinfo);
die $err;
}
- rename($tmpfn, $conffile) ||
- die "unable to commit configuration file '$conffile'\n";
+ PVE::Tools::file_set_contents($conffile, $new_conf_raw);
PVE::Cluster::cfs_update(); # make sure we read new file
@@ -6533,7 +6539,7 @@ sub restore_tar_archive {
local $ENV{VZDUMP_USER} = $user;
my $conffile = PVE::QemuConfig->config_file($vmid);
- my $tmpfn = "$conffile.$$.tmp";
+ my $new_conf_raw = '';
# disable interrupts (always do cleanups)
local $SIG{INT} =
@@ -6577,26 +6583,27 @@ sub restore_tar_archive {
my $srcfd = IO::File->new($confsrc, "r") || die "unable to open file '$confsrc'\n";
- my $outfd = IO::File->new($tmpfn, "w") || die "unable to write config for VM $vmid\n";
-
my $cookie = { netcount => 0 };
while (defined (my $line = <$srcfd>)) {
- $restore_update_config_line->($outfd, $cookie, $vmid, $map, $line, $opts->{unique});
+ $new_conf_raw .= $restore_update_config_line->(
+ $cookie,
+ $vmid,
+ $map,
+ $line,
+ $opts->{unique},
+ );
}
$srcfd->close();
- $outfd->close();
};
if (my $err = $@) {
- unlink $tmpfn;
tar_restore_cleanup($storecfg, "$tmpdir/qmrestore.stat") if !$opts->{info};
die $err;
}
rmtree $tmpdir;
- rename $tmpfn, $conffile ||
- die "unable to commit configuration file '$conffile'\n";
+ PVE::Tools::file_set_contents($conffile, $new_conf_raw);
PVE::Cluster::cfs_update(); # make sure we read new file
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users
2021-03-08 12:26 [pve-devel] [PATCH qemu-server 1/2] restore: write new config to variable first Fabian Ebner
@ 2021-03-08 12:26 ` Fabian Ebner
2021-03-09 7:17 ` Thomas Lamprecht
2021-03-09 7:05 ` [pve-devel] applied: [PATCH qemu-server 1/2] restore: write new config to variable first Thomas Lamprecht
1 sibling, 1 reply; 6+ messages in thread
From: Fabian Ebner @ 2021-03-08 12:26 UTC (permalink / raw)
To: pve-devel
by dropping privileged options for unprivileged users. For better backwards
compatibility for in-place restores, keep the option if 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. Note that restoring containers
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.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
PVE/QemuServer.pm | 71 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1410ecb..1d74ee2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5875,6 +5875,67 @@ my $restore_allocate_devices = sub {
return $map;
};
+# Make sure user is allowed to have options in config.
+my $sanitize_restored_config = sub {
+ 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 "WARNING: SKIPPING CONFIGURATION LINE '$line'. " .
+ "Restore as root to include it.\n";
+ } else {
+ $res .= "$line\n";
+ }
+ } else {
+ warn "WARNING: INVALID CONFIGURATION LINE '$line'.\n";
+ }
+ }
+
+ return $res;
+};
+
my $restore_update_config_line = sub {
my ($cookie, $vmid, $map, $line, $unique) = @_;
@@ -6281,6 +6342,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
@@ -6494,6 +6557,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
@@ -6513,6 +6578,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
@@ -6603,6 +6672,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
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users
2021-03-08 12:26 ` [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users Fabian Ebner
@ 2021-03-09 7:17 ` Thomas Lamprecht
2021-03-09 10:08 ` Fabian Ebner
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2021-03-09 7:17 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
On 08.03.21 13:26, Fabian Ebner wrote:
> by dropping privileged options for unprivileged users. For better backwards
> compatibility for in-place restores, keep the option if 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. Note that restoring containers
> 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.
>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> PVE/QemuServer.pm | 71 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
>
is this covered by some tests already?
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 1410ecb..1d74ee2 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5875,6 +5875,67 @@ my $restore_allocate_devices = sub {
> return $map;
> };
>
> +# Make sure user is allowed to have options in config.
> +my $sanitize_restored_config = sub {
> + 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) {
To work a bit better this would need to normalize values for comparison, as for
example, one can have a boolean option as '1' or 'on', IIRC, and format strings
may have changed order, so the equal check may fail even if they are
semantically equal.
Not sure how often this can occur in practice as we control the outgoing parser,
especially as this only matters for $rootonlyoptions, but did you thought/checked
this already?
> + $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 "WARNING: SKIPPING CONFIGURATION LINE '$line'. " .
> + "Restore as root to include it.\n";
I'd tone down the capslock a bit, eg.:
warn "WARN: skip restoring line '$line' due to privilege restrictions"
." - restore as root to include it\n"
ideally we set the warning count for tasks in the restore worker, like PBS does, so that
this shows up in the gui as "orange" task with some warnings in the task list.
> + } else {
> + $res .= "$line\n";
> + }
> + } else {
> + warn "WARNING: INVALID CONFIGURATION LINE '$line'.\n";
I'd tone down capslock here too and rather see how worker task warnings
could be set.
> + }
> + }
> +
> + return $res;
> +};
> +
> my $restore_update_config_line = sub {
> my ($cookie, $vmid, $map, $line, $unique) = @_;
>
> @@ -6281,6 +6342,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
> @@ -6494,6 +6557,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
> @@ -6513,6 +6578,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
> @@ -6603,6 +6672,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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users
2021-03-09 7:17 ` Thomas Lamprecht
@ 2021-03-09 10:08 ` Fabian Ebner
2021-03-09 10:15 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Fabian Ebner @ 2021-03-09 10:08 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
On 09.03.21 08:17, Thomas Lamprecht wrote:
> On 08.03.21 13:26, Fabian Ebner wrote:
>> by dropping privileged options for unprivileged users. For better backwards
>> compatibility for in-place restores, keep the option if 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. Note that restoring containers
>> 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.
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>> PVE/QemuServer.pm | 71 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 71 insertions(+)
>>
>
> is this covered by some tests already?
>
Haven't seen any. I can try and add some tests that mirror the
config-related behavior of the restore_XYZ_archive functions (directly
testing those doesn't seem feasible to me at the moment, lots of
PBS/VMA+pipes interaction...)
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 1410ecb..1d74ee2 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -5875,6 +5875,67 @@ my $restore_allocate_devices = sub {
>> return $map;
>> };
>>
>> +# Make sure user is allowed to have options in config.
>> +my $sanitize_restored_config = sub {
>> + 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) {
>
> To work a bit better this would need to normalize values for comparison, as for
> example, one can have a boolean option as '1' or 'on', IIRC, and format strings
> may have changed order, so the equal check may fail even if they are
> semantically equal.
>
> Not sure how often this can occur in practice as we control the outgoing parser,
> especially as this only matters for $rootonlyoptions, but did you thought/checked
> this already?
>
I did briefly think about it, but decided it wasn't worth the extra
complexity of parsing and deeply comparing everything. It's not only the
$rootonlyoptions though, e.g.
usb0: host=1-1,usb3=1
and
usb0: usb3=1,host=1-1
would also be affected.
For containers, we don't even look at the current values at all, but
always drop the 'lxc' options for a non-root user restore. But if you
think it's worth it, I can try and add that.
>> + $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 "WARNING: SKIPPING CONFIGURATION LINE '$line'. " .
>> + "Restore as root to include it.\n";
>
> I'd tone down the capslock a bit, eg.:
>
> warn "WARN: skip restoring line '$line' due to privilege restrictions"
> ." - restore as root to include it\n"
>
> ideally we set the warning count for tasks in the restore worker, like PBS does, so that
> this shows up in the gui as "orange" task with some warnings in the task list.
>
I wasn't aware of this feature. I suppose we should do that for the
skipped options for LXC too then.
>> + } else {
>> + $res .= "$line\n";
>> + }
>> + } else {
>> + warn "WARNING: INVALID CONFIGURATION LINE '$line'.\n";
>
> I'd tone down capslock here too and rather see how worker task warnings
> could be set.
>
Ok.
>> + }
>> + }
>> +
>> + return $res;
>> +};
>> +
>> my $restore_update_config_line = sub {
>> my ($cookie, $vmid, $map, $line, $unique) = @_;
>>
>> @@ -6281,6 +6342,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
>> @@ -6494,6 +6557,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
>> @@ -6513,6 +6578,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
>> @@ -6603,6 +6672,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
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users
2021-03-09 10:08 ` Fabian Ebner
@ 2021-03-09 10:15 ` Thomas Lamprecht
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2021-03-09 10:15 UTC (permalink / raw)
To: Fabian Ebner, Proxmox VE development discussion
On 09.03.21 11:08, Fabian Ebner wrote:
> On 09.03.21 08:17, Thomas Lamprecht wrote:
>> On 08.03.21 13:26, Fabian Ebner wrote:
>>
>> is this covered by some tests already?
>>
>
> Haven't seen any. I can try and add some tests that mirror the config-related behavior of the restore_XYZ_archive functions (directly testing those doesn't seem feasible to me at the moment, lots of PBS/VMA+pipes interaction...)
Yeah, I'd mostly just do unit testing on sanitize_restored_config directly,
although if more can be covered without a bigger extra cost that'd be naturally
great, but not a must.
>>> + my $oldvalue = $oldconf->{$key};
>>> +
>>> + if (defined($oldvalue) &&
$oldvalue eq $value) {
>>
>> To work a bit better this would need to normalize values for comparison, as for
>> example, one can have a boolean option as '1' or 'on', IIRC, and format strings
>> may have changed order, so the equal check may fail even if they are
>> semantically equal.
>>
>> Not sure how often this can occur in practice as we control the outgoing parser,
>> especially as this only matters for $rootonlyoptions, but did you thought/checked
>> this already?
>>
>
> I did briefly think about it, but decided it wasn't worth the extra complexity of parsing and deeply comparing everything. It's not only the $rootonlyoptions though, e.g.
> usb0: host=1-1,usb3=1
> and
> usb0: usb3=1,host=1-1
> would also be affected.
>
> For containers, we don't even look at the current values at all, but always drop the 'lxc' options for a non-root user restore. But if you think
it's worth it, I can try and add that.
hmm, if the output of the config is somewhat stable then I', ok with simple
for now..
>>> + $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 "WARNING: SKIPPING CONFIGURATION LINE '$line'. " .
>>> + "Restore as root to include it.\n";
>>
>> I'd tone down the capslock a bit, eg.:
>>
>> warn "WARN: skip restoring line '$line' due to privilege restrictions"
>> ." - restore as root to include it\n"
>>
>> ideally we set the warning count for tasks in the restore worker, like
PBS does, so that
>> this shows up in the gui as "orange" task with some warnings in the task list.
>>
>
> I wasn't aware of this feature. I suppose we should do that for the skipped options for LXC too then.
>
It's relatively new and introduced in PBS only, so there may be some catch up work to do
in pve-common, but GUI support should be universal.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] applied: [PATCH qemu-server 1/2] restore: write new config to variable first
2021-03-08 12:26 [pve-devel] [PATCH qemu-server 1/2] restore: write new config to variable first Fabian Ebner
2021-03-08 12:26 ` [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users Fabian Ebner
@ 2021-03-09 7:05 ` Thomas Lamprecht
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2021-03-09 7:05 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Ebner
On 08.03.21 13:26, Fabian Ebner wrote:
> and use file_set_contents to really commit it afterwards. Mostly done as a
> preparation for the later patch for sanitizing the config on restore, but
> shouldn't hurt by itself either.
>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> PVE/QemuServer.pm | 83 +++++++++++++++++++++++++----------------------
> 1 file changed, 45 insertions(+), 38 deletions(-)
>
>
applied, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-09 10:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08 12:26 [pve-devel] [PATCH qemu-server 1/2] restore: write new config to variable first Fabian Ebner
2021-03-08 12:26 ` [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users Fabian Ebner
2021-03-09 7:17 ` Thomas Lamprecht
2021-03-09 10:08 ` Fabian Ebner
2021-03-09 10:15 ` Thomas Lamprecht
2021-03-09 7:05 ` [pve-devel] applied: [PATCH qemu-server 1/2] restore: write new config to variable first Thomas Lamprecht
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.