all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: jlcusinato <jlcusinato@neverhack.com>
To: pve-devel@lists.proxmox.com
Cc: felix@ranovus.com, crandler@crandland.de, dreadfulbunny@proton.me
Subject: [PATCH qemu-server] fix #7864: api: create vm: honour pool privileges when setting tags
Date: Tue, 27 Oct 2026 11:32:34 +0100	[thread overview]
Message-ID: <20261027103626.767361-3-jlcusinato@neverhack.com> (raw)
In-Reply-To: <20261027103626.767361-1-jlcusinato@neverhack.com>

From: Jean-Leon CUSINATO <jlcusinato@neverhack.com>

Commit a929b3e ("vm_create: check tag permissions if setting tags")
started checking tag permissions on the create path, but
assert_tag_permissions() resolved VM.Config.Options against a bare
/vms/$vmid.

At that point the VM is not a pool member yet - add_vm_to_pool()
only runs after $realcmd has written the config - so a user or API
token whose privileges are granted solely on /pool/<pool> fails with

  unable to create VM 1021 - 403 Permission check failed
  (/vms/1021, VM.Config.Options)

even though it is allowed to create the VM in that pool, and even
though every other create-time option honours the pool via
$check_vm_modify_config_perm(). Tags are the only exception, because
they are skipped there and checked separately.

Pass the pool through, matching the surrounding checks. Setting the
same tags in a second config update already worked, so this only
removes the need for the caller to hold VM.Config.Options on all of
/vms.

The old-tags argument passed {} rather than an empty string.
PVE::Tools::split_list() stringifies that reference, so with
'user-allow' set to anything but 'free' a pseudo tag like
'HASH(0x...)' enters the per-tag comparison, which no allow list can
ever hold since the address is fresh on every call:

  403 Permission check failed (/, Sys.Modify for modifying tag
  'HASH(0x5c0c4195ab70)')

Pass an empty string instead, as the delete branch of the
config-update path already does; split_list('') returns an empty
list.

Fixes: a929b3e ("vm_create: check tag permissions if setting tags")
Reported-by: crandler <crandler@crandland.de>
Signed-off-by: Felix Winterhalter <felix@ranovus.com>
Signed-off-by: Jean-Léon CUSINATO <jlcusinato@neverhack.com>
---

Compared to the Bugzilla attachment (2057): the old-tags argument
is '' rather than {} per crandler's finding in comment #5 of the
bug (also CCed) - PVE::Tools::split_list() stringifies a hash
reference, so a 'HASH(0x...)' pseudo tag would enter the per-tag
comparison whenever 'user-allow' is not 'free'. Verified against
current pve-common: split_list('') returns () while split_list({})
yields HASH(0x...). Bug reference #7864 already in the subject;
commit message reflowed to the 72-char line-width guideline;
rebased onto current master.

Resend note (submitted by Jean-Léon CUSINATO
<jlcusinato@neverhack.com>):

These patches originate from bug 7864 [0]: code by Felix Winterhalter
<felix@ranovus.com> (the versions attached there, informally reviewed
by a Proxmox maintainer in comment #4, who asked for a posting on this
list) plus crandler's follow-up finding (comment #5) and dreadfulbunny's
data point on the container create path (comment #7). Neither original
author has been active on the bug since September 2026, and the list
never received a posting despite that request, so I am submitting them
here with their attribution intact in the commit messages
(Signed-off-by / Reported-by). All contributors are CCed. My individual
CLA has been submitted to and recorded by Proxmox
(office@proxmox.com, October 2026), who asked for the patches to be
posted here. Not runtime-tested (no test node available); verification
is code-level only. Feedback, especially testing, welcome.

Series merge order: 1) pve-guest-common, then 2) qemu-server and
3) pve-container (the latter two only rely on the new optional $pool
parameter introduced in 1).

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=7864

 src/PVE/API2/Qemu.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index f4dbd2a..5789f99 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -1568,7 +1568,7 @@ __PACKAGE__->register_method({
 
                     if (defined $conf->{tags}) {
                         assert_tag_permissions(
-                            $vmid, {}, $conf->{tags}, $rpcenv, $authuser,
+                            $vmid, '', $conf->{tags}, $rpcenv, $authuser, $pool,
                         );
                         $conf->{tags} = PVE::GuestHelpers::get_unique_tags($conf->{tags});
                     }
-- 
2.56.0




  parent reply	other threads:[~2026-10-27 10:42 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-27 10:32 [PATCH 0/3] fix #7864: allow pool-scoped privileges to create VMs with tags jlcusinato
2026-10-27 10:32 ` [PATCH pve-guest-common] fix #7864: guest helpers: allow pool-scoped privileges for tag permission checks jlcusinato
2026-10-27 10:32 ` jlcusinato [this message]
2026-10-27 10:32 ` [PATCH pve-container] fix #7864: lxc: honour pool privileges when creating containers with tags jlcusinato

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=20261027103626.767361-3-jlcusinato@neverhack.com \
    --to=jlcusinato@neverhack.com \
    --cc=crandler@crandland.de \
    --cc=dreadfulbunny@proton.me \
    --cc=felix@ranovus.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal