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 4C67B8DAAD for ; Wed, 9 Nov 2022 13:51:15 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2C49B1C358 for ; Wed, 9 Nov 2022 13:51:15 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 9 Nov 2022 13:51:13 +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 1827C42652 for ; Wed, 9 Nov 2022 13:51:13 +0100 (CET) Message-ID: <1d2d1b03-7ff7-792b-d021-59c7c7c2a97c@proxmox.com> Date: Wed, 9 Nov 2022 13:51:11 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Content-Language: en-US To: Proxmox VE development discussion , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20220920125041.3636561-1-d.csapak@proxmox.com> <20220920125041.3636561-18-d.csapak@proxmox.com> <1667995675.8a5847bwnj.astroid@yuna.none> From: Dominik Csapak In-Reply-To: <1667995675.8a5847bwnj.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.066 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% 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) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, qemu.pm] Subject: Re: [pve-devel] [PATCH qemu-server v3 08/13] PVE/API2/Qemu: add permission checks for mapped pci devices 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: Wed, 09 Nov 2022 12:51:15 -0000 On 11/9/22 13:14, Fabian Grünbichler wrote: > On September 20, 2022 2:50 pm, Dominik Csapak wrote: >> Signed-off-by: Dominik Csapak >> --- >> PVE/API2/Qemu.pm | 54 ++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 52 insertions(+), 2 deletions(-) >> >> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm >> index 7afd7a4..d6d393f 100644 >> --- a/PVE/API2/Qemu.pm >> +++ b/PVE/API2/Qemu.pm >> @@ -26,6 +26,7 @@ use PVE::QemuServer::Drive; >> use PVE::QemuServer::ImportDisk; >> use PVE::QemuServer::Monitor qw(mon_cmd); >> use PVE::QemuServer::Machine; >> +use PVE::QemuServer::PCI; >> use PVE::QemuServer::USB qw(parse_usb_device); >> use PVE::QemuMigrate; >> use PVE::RPCEnvironment; >> @@ -603,6 +604,26 @@ my $check_vm_create_usb_perm = sub { >> return 1; >> }; >> >> +my $check_vm_create_hostpci_perm = sub { >> + my ($rpcenv, $authuser, $vmid, $pool, $param) = @_; >> + >> + return 1 if $authuser eq 'root@pam'; >> + >> + foreach my $opt (keys %{$param}) { >> + next if $opt !~ m/^hostpci\d+$/; >> + >> + my $device = PVE::JSONSchema::parse_property_string('pve-qm-hostpci', $param->{$opt}); >> + if ($device->{host} !~ m/:/) { > > while thinking about the priv patch I decided to check out the ACL handling as > well - sorry for not asking earlier about this aspect! > > would it make sense to have a prefix for "ID of mapped hardware" instead of > claiming "everything that doesn't contain ':'" as namespace? i mean we could, but a few downsides come to mind: * if we prefix the ids in the hardware-map itself, the user always sees that prefix, which is useless to them, or we have to remove it everywhere when displaying, which is imho unnecessary work * if we only prefix it in the vm config, we have to add/remove it in the gui/cli (or api?) which also means users that edit the config don't have the direct correlation with the hardware map basically, more work for us all around for usb devices we actually check the vendor/id/path regex first, then for spice (which is the only exception to the namespace clash) and all other values we interpret as mapped devices (imho the small downside of not being able to name a mapping 'spice' is worth the work we save by not introducing a prefix/namespace) we could do the same for pci ids of course, i just used the shortcut of having ':' ofc if you insist on having some separated namespace by prefix (or similar), i'll implement that, i'm not very attached to the current implementation ;) > > the same also applies to the USB ACL checks and the other checks in this patch.. > >> + $rpcenv->check_full($authuser, "/hardware/$device->{host}", ['Hardware.Use']); > > this and similar sites would then also be more explicit: > > my $hw_id = ..; # extract actual ID > $rpvenv->check_full($authuser, "/hardware/$hw_id", ['Hardware.Use']); > >> + $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']); >> + } else { >> + die "only root can set '$opt' config for non-mapped devices\n"; >> + } >> + } >> + >> + return 1; >> +}; >> + >> >> [...] > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > >