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 36CB08DB05 for ; Wed, 9 Nov 2022 14:07:08 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 118C11C5CE for ; Wed, 9 Nov 2022 14:06:38 +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, 9 Nov 2022 14:06:36 +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 59E21431C3 for ; Wed, 9 Nov 2022 14:06:36 +0100 (CET) Date: Wed, 09 Nov 2022 14:06:28 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Dominik Csapak , Proxmox VE development discussion References: <20220920125041.3636561-1-d.csapak@proxmox.com> <20220920125041.3636561-7-d.csapak@proxmox.com> <1667994471.zlbn23bpo0.astroid@yuna.none> <18114244-e7e5-d418-a27d-c2ff9d91bd36@proxmox.com> In-Reply-To: <18114244-e7e5-d418-a27d-c2ff9d91bd36@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1667998564.0rf5qj5m6h.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.091 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 POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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. [rpcenvironment.pm, accesscontrol.pm] Subject: Re: [pve-devel] [PATCH access-control v3 1/1] PVE/AccessControl: add Hardware.* privileges and /hardware/ paths 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 13:07:08 -0000 On November 9, 2022 1:39 pm, Dominik Csapak wrote: > On 11/9/22 13:05, Fabian Gr=C3=BCnbichler wrote: >> On September 20, 2022 2:50 pm, Dominik Csapak wrote: >>> so that we can assign privileges on hardware level >>> >>> this will generate a new role (PVEHardwareAdmin) >>> >>> Signed-off-by: Dominik Csapak >>> --- >>> src/PVE/AccessControl.pm | 13 +++++++++++++ >>> src/PVE/RPCEnvironment.pm | 3 ++- >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm >>> index c32dcc3..9cbc376 100644 >>> --- a/src/PVE/AccessControl.pm >>> +++ b/src/PVE/AccessControl.pm >>> @@ -1080,6 +1080,17 @@ my $privgroups =3D { >>> 'Pool.Audit', >>> ], >>> }, >>> + Hardware =3D> { >>> + root =3D> [ >>> + 'Hardware.Configure', # create/edit mappings >>> + ], >>> + admin =3D> [ >>> + 'Hardware.Use', >>> + ], >>> + audit =3D> [ >>> + 'Hardware.Audit', >>> + ], >>> + }, >>=20 >> I guess the rationale here was that currently hardware is root only, so = having >>=20 >> admin =3D> Configure, >> user =3D> Use, >> audit =3D> Audit, >>=20 >> would mean the existing PVEAdmin roles would gain something that was pre= viously >> root only? >>=20 >> note that the current patch still means that for the "Administrator" rol= e >> anyway, since that gets *all* defined privileges.. (which might also be >> something worthy of calling out somewhere?) >=20 > yes the idea was that existing roles don't get that privilege, but > i did not really find a way to add the privilige and not give it to the=20 > administrator role could only be done by manual filtering in create_roles (similar to how the SuperUser series does it). > and yes putting it in the admin bucket would mean that pveadmin gets it t= oo >=20 >>=20 >> it still might make sense to put Hardware.Use into the user category for >> consistency's sake? also not sure whether it would be worth it to re-thi= nk >> "Configure" (a bit more explicit) vs. "Modify" (consistent with existing >> schema).. >=20 > Modify is fine by me, i didn't choose it because we don't actually=20 > 'modify' the hardware, but yes we modify the hardware configuration well, it does rather refer to the config entry/hardware map, not the hardwa= re itself :) > putting the use in the user bracket has the side effect that > there would be a 'PVEHardwareAdmin' and 'PVEHardwareUser' role > with the same privileges, which i did find weird to have >=20 > this way we only get the PVEHardwareAdmin that can use/see the > devices >=20 > if there is a way to only have 'user' role without the admin one > please do tell ;) no automatic way ;) one way out would be to: - give Hardware.Configure (/Modify) to the Admin role - give Hardware.Use to the User role - still require root in addition to Hardware.Configure (until point or majo= r release time, where the extra check is dropped and documented in the releas= e notes) my guess is that most people that currently hand out Administrator (as oppo= sed to "just" PVEAdmin) would actually want those users to also be able to conf= igure the hardware map anyway.. and no users would get the new role "PVEHardwareAdmin" anyway by default, so handing that out is an explicit gr= ant of the associated privileges anyway. also there is the similar can of worms like with SuperUser - any user that = can change permissions can give themselves or someone else Hardware.Configure (= which is possibly root-level access if misused?). if we add more "special" privileges/roles like that, we might need to have some extension to the acl= and group membership code paths to handle that better.. but I haven't really th= ought that through yet (at least more extensive docs would be a good idea I think= ).