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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.