public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Thomas Lamprecht <t.lamprecht@proxmox.com>
Subject: Re: [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config
Date: Wed, 16 Nov 2022 10:31:50 +0100	[thread overview]
Message-ID: <1668590513.dxe50wm4po.astroid@yuna.none> (raw)
In-Reply-To: <7cfca64a-2ab1-405a-fde1-1f4f14e043b8@proxmox.com>

On November 16, 2022 10:10 am, Thomas Lamprecht wrote:
> Am 16/11/2022 um 10:04 schrieb Dominik Csapak:
>> On 11/16/22 09:54, Thomas Lamprecht wrote:
>>> Am 16/11/2022 um 09:47 schrieb Dominik Csapak:
>>>>> I am not sure the second sentence is necessary, or rather, wouldn't it be better
>>>>> to make the two lists mutually exclusive? e.g., by removing privileged tags from
>>>>> the other list?
>>>>
>>>> i don't really want to auto remove stuff from one option when set on another.
>>>> maybe it'd make more sense if we don't allow setting and admin tag when
>>>> it's already set in the 'user-allow-list' and vice versa? then
>>>> there cannot be a situation where a tag is in both lists at the same time?
>>>>
>>>
>>>
>>> Limits use cases, as we'll only ever allow priv'd tags to be used for things
>>> like backup job guest-source selection, and there may be scenarios where an
>>> admin wants to allow the user to set a specific privileged tags in the VMs
>>> they control.
>>>
>>> To make that work we'd:
>>> - explicitly allow such listed tags for "normal" VM users even if they're in the
>>>    privileged-tags (that's why I used the term "registered" in previous comments,
>>>    might be better suited if we deem that privileged is then confusing)
>>>
>>> - highlight the fact if a tag is in both
>>>
>> 
>> ok, then i have to change the permission checking code (currently i forbid
>> 'normal' users the tag if it was in the 'privileged-tags' section, regardless
>>  if it was in the 'user-allow-list' or not)
> 
> maybe wait on Fabian's opinion on that, I don't want to push this to strongly
> but can imagine that it might be sensible and useful, and hard to change later.

If we say vzdump should only use privileged tags for backup inclusion logic (to
avoid unprivileged users adding that tag to their VM and causing it to be backed
up), but then make some of those tags effectively non-privileged (which allows
exactly that), why do we have the restriction in vzdump in the first place?

that sounds like a complicated way (with lots of side-effects, because
privileged tags might be used in other places in the future as well) to make the
"vzdump should only use privileged tags" part configurable.. maybe there should
simply be a list of "vzdump tags" in addition to the privileged ones? those
would then be unprivileged, but the scope of "these allow vzdump job inclusion"
is clear and limited. or we just keep "vzdump only looks at privileged tags" for
now to keep it simple - extending that one way or another in the future is
always possible if it is restricted now, the other way is harder ;)




  reply	other threads:[~2022-11-16  9:32 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-15 13:02 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/wt/manager v10 0/5] add tags to ui Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 1/5] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Dominik Csapak
2022-11-16  9:50   ` Wolfgang Bumiller
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 2/5] Cluster: add get_guest_config_properties Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 3/5] datacenter.cfg: add option for tag-style Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 4/5] datacenter.cfg: add tag rights control to the datacenter config Dominik Csapak
2022-11-15 15:17   ` Fabian Grünbichler
2022-11-16  7:48     ` Thomas Lamprecht
2022-11-16  8:47     ` Dominik Csapak
2022-11-16  8:51       ` Fabian Grünbichler
2022-11-16  8:54       ` Thomas Lamprecht
2022-11-16  9:04         ` Dominik Csapak
2022-11-16  9:10           ` Thomas Lamprecht
2022-11-16  9:31             ` Fabian Grünbichler [this message]
2022-11-16  9:38               ` Dominik Csapak
2022-11-16  9:40               ` Thomas Lamprecht
2022-11-16  9:51                 ` Fabian Grünbichler
2022-11-16 13:56                   ` Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH cluster v10 5/5] datacenter.cfg: add 'ordering' to 'tag-style' config Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH guest-common v10 1/1] GuestHelpers: add 'assert_tag_permissions' Dominik Csapak
2022-11-15 15:34   ` Fabian Grünbichler
2022-11-15 13:02 ` [pve-devel] [PATCH qemu-server v10 1/1] api: update: check for tags permissions with 'assert_tag_permissions' Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH container v10 1/1] check_ct_modify_config_perm: " Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH widget-toolkit v10 1/2] add tag related helpers Dominik Csapak
2022-11-16 13:48   ` [pve-devel] applied: " Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH widget-toolkit v10 2/2] Toolkit: add override for Ext.dd.DragDropManager Dominik Csapak
2022-11-16 13:49   ` [pve-devel] applied: " Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 01/13] api: /cluster/resources: add tags to returned properties Dominik Csapak
2022-11-16  8:02   ` Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 02/13] api: add /ui-options api call Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 03/13] ui: call '/ui-options' and save the result in PVE.UIOptions Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 04/13] ui: parse and save tag infos from /ui-options Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 05/13] ui: add form/TagColorGrid Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 06/13] ui: add PVE.form.ListField Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 07/13] ui: dc/OptionView: add editors for tag settings Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 08/13] ui: add form/Tag Dominik Csapak
2022-11-16 14:57   ` Thomas Lamprecht
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 09/13] ui: add form/TagEdit.js Dominik Csapak
2022-11-16 15:00   ` Thomas Lamprecht
2022-11-16 15:02     ` Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 10/13] ui: {lxc, qemu}/Config: show Tags and make them editable Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 11/13] ui: tree/ResourceTree: show Tags in tree Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 12/13] ui: add tags to ResourceGrid and GlobalSearchField Dominik Csapak
2022-11-15 13:02 ` [pve-devel] [PATCH manager v10 13/13] ui: implement tag ordering from datacenter.cfg Dominik Csapak

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=1668590513.dxe50wm4po.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=d.csapak@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=t.lamprecht@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal