From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Fabian Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users
Date: Tue, 9 Mar 2021 11:15:01 +0100 [thread overview]
Message-ID: <4ec317bd-130f-4345-25e8-3cec2c9aef32@proxmox.com> (raw)
In-Reply-To: <358dabb5-1b45-a1e4-1795-6fb310b78df3@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.
next prev parent reply other threads:[~2021-03-09 10:15 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-03-09 7:05 ` [pve-devel] applied: [PATCH qemu-server 1/2] restore: write new config to variable first Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4ec317bd-130f-4345-25e8-3cec2c9aef32@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox