From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pdm-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 96E9C1FF164
for <inbox@lore.proxmox.com>; Fri, 11 Apr 2025 12:54:21 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id AE8BBF2A5;
Fri, 11 Apr 2025 12:54:16 +0200 (CEST)
Message-ID: <ad8c93a3-11f3-4a55-86f8-ba20e9a50c1c@proxmox.com>
Date: Fri, 11 Apr 2025 12:53:43 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird Beta
To: Shannon Sterz <s.sterz@proxmox.com>,
Proxmox Datacenter Manager development discussion
<pdm-devel@lists.proxmox.com>, Dietmar Maurer <dietmar@proxmox.com>
References: <20250403141806.402974-1-s.sterz@proxmox.com>
<20250403141806.402974-3-s.sterz@proxmox.com>
<2022147362.1680.1744196508347@webmail.proxmox.com>
<3293442a-0aed-4ab6-a6ee-5a0f8ea6b1e6@proxmox.com>
<D924J9S27KUN.3T32GISEJ9JRV@proxmox.com>
<D93QMCLHVQJ0.20JKBLOMR1LW8@proxmox.com>
Content-Language: en-US
From: Dominik Csapak <d.csapak@proxmox.com>
In-Reply-To: <D93QMCLHVQJ0.20JKBLOMR1LW8@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.021 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api
feature
X-BeenThere: pdm-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Datacenter Manager development discussion
<pdm-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pdm-devel>,
<mailto:pdm-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pdm-devel/>
List-Post: <mailto:pdm-devel@lists.proxmox.com>
List-Help: <mailto:pdm-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel>,
<mailto:pdm-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Datacenter Manager development discussion
<pdm-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pdm-devel-bounces@lists.proxmox.com
Sender: "pdm-devel" <pdm-devel-bounces@lists.proxmox.com>
On 4/11/25 12:29, Shannon Sterz wrote:
> On Wed Apr 9, 2025 at 2:58 PM CEST, Shannon Sterz wrote:
>> On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
>>> On 4/9/25 13:01, Dietmar Maurer wrote:
>>> maybe something like this for the update case (untested, please verify before using this!):
>>> (the diff is for pbs, where the code was copied from)
>>>
>>> this also includes a reformatted check for the token,non-token, same user checks
>>> that are IMHO more readable than what we currently have
>>> with the match, i think it's much more obvious that all cases are handled
>>>
>>> ---
>>> let user_info = CachedUserInfo::new()?;
>>>
>>> - let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
>>> - if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>>> + let has_modify_permission = user_info
>>> + .check_privs(
>>> + ¤t_auth_id,
>>> + &["access", "acl"],
>>> + PRIV_PERMISSIONS_MODIFY,
>>> + false,
>>> + )
>>> + .is_ok();
>>> +
>>> + if !has_modify_permission {
>>> if group.is_some() {
>>> bail!("Unprivileged users are not allowed to create group ACL item.");
>>> }
>>>
>>> match &auth_id {
>>> Some(auth_id) => {
>>> - if current_auth_id.is_token() {
>>> - bail!("Unprivileged API tokens can't set ACL items.");
>>> - } else if !auth_id.is_token() {
>>> - bail!("Unprivileged users can only set ACL items for API tokens.");
>>> - } else if auth_id.user() != current_auth_id.user() {
>>> - bail!("Unprivileged users can only set ACL items for their own API tokens.");
>>> + let same_user = auth_id.user() == current_auth_id.user();
>>> + match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
>>> + (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
>>> + (false, false, _) => {
>>> + bail!("Unprivileged users can only set ACL items for API tokens.")
>>> + }
>>> + (false, true, true) => {
>>> + // users are always allowed to modify ACLs for their own tokens
>>> + }
>>> + (false, true, false) => {
>>> + bail!("Unprivileged users can only set ACL items for their own API tokens.")
>>> + }
>>> }
>>> }
>>> None => {
>>> ---
>
> had another think about this, i'd tend towards something like the below.
> the match statement is a nice idea, but it couples together things that
> aren't really related. for example, why pull in the
> `current_auth_id.is_token()` check, but not the `group.is_some()` check?
> having match statements with tuples like this is making the code more
> complex. imo, this is simpler:
>
> ```rs
> let unprivileged_user = CachedUserInfo::new()?
> .check_privs(
> ¤t_auth_id,
> &["access", "acl"],
> access_conf.acl_modify_privileges(),
> access_conf.allow_partial_permission_match(),
> )
> .is_err();
>
> if unprivileged_user {
> if group.is_some() {
> bail!("Unprivileged users are not allowed to create group ACL item.");
> }
>
> let auth_id = auth_id.as_ref().ok_or_else(|| {
> format_err!("Unprivileged user needs to provide auth_id to update ACL item.")
> })?;
>
> if current_auth_id.is_token() {
> bail!("Unprivileged API tokens can't set ACL items.");
> }
>
> if !auth_id.is_token() {
> bail!("Unprivileged users can only set ACL items for API tokens.");
> }
>
> if current_auth_id != *auth_id {
> bail!("Unprivileged users can only set ACL items for their own API tokens.");
> }
> }
> ```
>
> what do you think?
I see what you mean, and yes i think it's more readable, but what I really wanted to convey with my
approach was to clarify which condition is ok
we currently try to filter out all invalid states, and it is not really obvious what
condition makes the code continue at first glance
Maybe we have to approach it completely different, for example check only the valid cases first
and let that pass through, and then fail with the specific errors and have a fallback error
for all other cases. that way we can't come into a situation where we forget/overlook some edge
case.
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel