From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 0189C68891 for ; Tue, 9 Mar 2021 11:15:03 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EC6E18644 for ; Tue, 9 Mar 2021 11:15:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 653588625 for ; Tue, 9 Mar 2021 11:15:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 2CC60457FF for ; Tue, 9 Mar 2021 11:15:02 +0100 (CET) Message-ID: <4ec317bd-130f-4345-25e8-3cec2c9aef32@proxmox.com> Date: Tue, 9 Mar 2021 11:15:01 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Thunderbird/87.0 Content-Language: en-US To: Fabian Ebner , Proxmox VE development discussion References: <20210308122658.31781-1-f.ebner@proxmox.com> <20210308122658.31781-2-f.ebner@proxmox.com> <04a999a8-ce86-ac53-e3d5-00f17c11ee81@proxmox.com> <358dabb5-1b45-a1e4-1795-6fb310b78df3@proxmox.com> From: Thomas Lamprecht In-Reply-To: <358dabb5-1b45-a1e4-1795-6fb310b78df3@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.050 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Mar 2021 10:15:03 -0000 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? >> >=20 > Haven't seen any. I can try and add some tests that mirror the config-r= elated behavior of the restore_XYZ_archive functions (directly testing th= ose doesn't seem feasible to me at the moment, lots of PBS/VMA+pipes inte= raction...) Yeah, I'd mostly just do unit testing on sanitize_restored_config directl= y, although if more can be covered without a bigger extra cost that'd be nat= urally great, but not a must. >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 my $oldvalue =3D $oldconf= ->{$key}; >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (defined($oldvalue) &&=20 $oldvalue eq $value) { >> >> To work a bit better this would need to normalize values for compariso= n, as for >> example, one can have a boolean option as '1' or 'on', IIRC, and forma= t 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 outgoi= ng parser, >> especially as this only matters for $rootonlyoptions, but did you thou= ght/checked >> this already? >> >=20 > I did briefly think about it, but decided it wasn't worth the extra com= plexity of parsing and deeply comparing everything. It's not only the $ro= otonlyoptions though, e.g. > =C2=A0=C2=A0 usb0: host=3D1-1,usb3=3D1 > and > =C2=A0=C2=A0 usb0: usb3=3D1,host=3D1-1 > would also be affected. >=20 > For containers, we don't even look at the current values at all, but al= ways drop the 'lxc' options for a non-root user restore. But if you think=20 it's worth it, I can try and add that. hmm, if the output of the config is somewhat stable then I', ok with simp= le for now.. >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 $res .=3D "$line\n"; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 next; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>> + >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ($rootonlyoptions->{$k= ey} || >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (is_valid_drivename($key)=20 && $is_bad_drive->($key, $value)) || >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ($key =3D~ m/^serial\d+$/=20 && $value ne 'socket') || >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ($key =3D~ m/^usb\d+$/ &&=20 $value !~ m/spice/) || >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 $key =3D~ m/^parallel\d+$= / || >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 $key =3D~ m/^hostpci\d+$/= ) { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 warn "WARNING: SKIPPING C= ONFIGURATION LINE '$line'. " . >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "= 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"= >> =C2=A0=C2=A0=C2=A0=C2=A0 ." - restore as root to include it\n" >> >> ideally we set the warning count for tasks in the restore worker, like=20 PBS does, so that >> this shows up in the gui as "orange" task with some warnings in the ta= sk list. >> >=20 > I wasn't aware of this feature. I suppose we should do that for the ski= pped options for LXC too then. >=20 It's relatively new and introduced in PBS only, so there may be some catc= h up work to do in pve-common, but GUI support should be universal.