From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <l.nunner@proxmox.com>
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 98FB1906C4
for <pve-devel@lists.proxmox.com>; Fri, 24 Mar 2023 10:43:43 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id AFAE49451
for <pve-devel@lists.proxmox.com>; Fri, 24 Mar 2023 10:43:12 +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 <pve-devel@lists.proxmox.com>; Fri, 24 Mar 2023 10:43:10 +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 BFFC1466B7
for <pve-devel@lists.proxmox.com>; Fri, 24 Mar 2023 10:43:08 +0100 (CET)
Message-ID: <7584dc51-7ce8-3b4c-d5a5-149cc8c521c4@proxmox.com>
Date: Fri, 24 Mar 2023 10:43:07 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
Thunderbird/102.9.0
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pve-devel@lists.proxmox.com
References: <20230301091229.53505-1-l.nunner@proxmox.com>
<20230301091229.53505-2-l.nunner@proxmox.com>
<20230315140553.iqyxwjhqsv2vo4b2@casey.proxmox.com>
Content-Language: en-US
From: Leo Nunner <l.nunner@proxmox.com>
In-Reply-To: <20230315140553.iqyxwjhqsv2vo4b2@casey.proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -1.102 Adjusted score from AWL reputation of From: address
DMARC_MISSING 0.1 Missing DMARC policy
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
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 24 Mar 2023 09:43:43 -0000
Thanks for the review!
On 2023-03-15 15:05, Wolfgang Bumiller wrote:
> 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 <l.nunner@proxmox.com>
>> ---
>> 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...
We could also just switch to something like `[^,;]+`, or would that be
too extreme?
>> +
>> + 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.
Agreed, I'll change it accordingly.