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 64768909E8 for ; Wed, 15 Mar 2023 15:06:27 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 256D4AA87 for ; Wed, 15 Mar 2023 15:05:57 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 15 Mar 2023 15:05:55 +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 0AFFB4180D for ; Wed, 15 Mar 2023 15:05:55 +0100 (CET) Date: Wed, 15 Mar 2023 15:05:53 +0100 From: Wolfgang Bumiller To: Leo Nunner Cc: pve-devel@lists.proxmox.com Message-ID: <20230315140553.iqyxwjhqsv2vo4b2@casey.proxmox.com> References: <20230301091229.53505-1-l.nunner@proxmox.com> <20230301091229.53505-2-l.nunner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230301091229.53505-2-l.nunner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.182 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 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. [qemu.pm, qemuserver.pm] Subject: Re: [pve-devel] [PATCH qemu-server 1/2] fix #4068: implement support for fw_cfg 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, 15 Mar 2023 14:06:27 -0000 On Wed, Mar 01, 2023 at 10:12:26AM +0100, Leo Nunner wrote: > Implements support for passing values to the fw_cfg argument for QEMU. > If the value looks like a file, the backend checks for the correct > permissions/path and if everything is right, includes it as a file > instead of as a string. > > Setting the argument requires the VM.Config.Options permission on the > guest. Including files requires Datastore.Audit and Datastore.Allocate > on the specific storage. > > Signed-off-by: Leo Nunner > --- > RFC: I feel like a more implicit option for passing files would be > nicer, but I can't really think of a nice way… > > PVE/API2/Qemu.pm | 14 ++++++++++++++ > PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+) > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index 587bb22..c03394f 100644 > --- a/PVE/API2/Qemu.pm > +++ b/PVE/API2/Qemu.pm > @@ -639,6 +639,8 @@ my $check_vm_modify_config_perm = sub { > # the user needs Disk and PowerMgmt privileges to change the vmstate > # also needs privileges on the storage, that will be checked later > $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk', 'VM.PowerMgmt' ]); > + } elsif ($opt eq 'fw_cfg') { > + $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']); > } else { > # catches hostpci\d+, args, lock, etc. > # new options will be checked here > @@ -1770,6 +1772,18 @@ my $update_vm_api = sub { > } elsif ($opt eq 'tags') { > assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser); > $conf->{pending}->{$opt} = PVE::GuestHelpers::get_unique_tags($param->{$opt}); > + } elsif ($opt eq 'fw_cfg') { > + foreach my $fw_cfg (PVE::Tools::split_list($param->{$opt})) { > + my ($opt, $val) = split("=", $fw_cfg); > + > + if (my $storage = PVE::Storage::parse_volume_id($val, 1)) { > + $rpcenv->check($authuser, "/storage/$storage", ['Datastore.Audit', 'Datastore.Allocate']); > + > + my ($path, undef, $type) = PVE::Storage::path($storecfg, $val); > + die "File $val is not in snippets directory\n" if $type ne "snippets"; > + } > + } > + $conf->{pending}->{$opt} = $param->{$opt}; > } else { > $conf->{pending}->{$opt} = $param->{$opt}; > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > index 40be44d..c5dea1f 100644 > --- a/PVE/QemuServer.pm > +++ b/PVE/QemuServer.pm > @@ -723,6 +723,12 @@ EODESCR > description => "List of host cores used to execute guest processes, for example: 0,5,8-11", > optional => 1, > }, > + fw_cfg => { > + type => 'string', > + optional => 1, > + description => 'Pass values to the guest via the fw_cfg parameter.', > + format => 'pve-fw-cfg-list', > + }, > }; > > my $cicustom_fmt = { > @@ -1076,6 +1082,21 @@ sub verify_volume_id_or_absolute_path { > return $volid; > } > > +PVE::JSONSchema::register_format('pve-fw-cfg', \&verify_fw_cfg); > +sub verify_fw_cfg { > + my ($value, $noerr) = @_; > + > + my $FW_CFG_REGEX = qr/[a-z0-9\.\/:\-]+/; IMO this is way too restrictive. If we pass the `name=` prefix in the `-fw_cfg` parameter we can even allow `=`. In fact, AFAICT only `,` is problematic and would need to be doubled. Also if we really just have a single option with a list... both `,` and `;` will be problematic on our end, so it's fine to exclude those 2 for now. (Until we add quoting support to split_list ;-) ) That said, for the use cases I have in mind for us, the current code would be sufficient, but at least uppercase letters, underscores and spaces would be nice... > + > + if ($value =~ m/^(opt\/$FW_CFG_REGEX)=($FW_CFG_REGEX)$/) { > + return $value; > + } > + > + return if $noerr; > + > + die "unable to parse fw_cfg option\n"; > +} > + > my $usb_fmt = { > host => { > default_key => 1, > @@ -4181,6 +4202,20 @@ sub config_to_command { > push @$cmd, '-snapshot'; > } > > + if ($conf->{fw_cfg}) { > + foreach my $conf (PVE::Tools::split_list($conf->{fw_cfg})) { > + my ($opt, $val) = split("=", $conf); > + > + push @$cmd, "-fw_cfg"; > + if (PVE::Storage::parse_volume_id($val, 1)) { > + my $path = PVE::Storage::path($storecfg, $val); > + push @$cmd, "$opt,file=$path"; > + } else { > + push @$cmd, "$opt,string=$val"; ^ This would definitely be safer with the `name=` portion included.