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 B77941FF165
for <inbox@lore.proxmox.com>; Thu, 10 Apr 2025 10:18:03 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id CF1C5313A5;
Thu, 10 Apr 2025 10:17:59 +0200 (CEST)
Mime-Version: 1.0
Date: Thu, 10 Apr 2025 10:17:26 +0200
Message-Id: <D92T6VJN81W9.3PVQMWLTN9Q6B@proxmox.com>
From: "Shannon Sterz" <s.sterz@proxmox.com>
To: "Dominik Csapak" <d.csapak@proxmox.com>, "Proxmox Datacenter Manager
development discussion" <pdm-devel@lists.proxmox.com>, "Dietmar Maurer"
<dietmar@proxmox.com>
X-Mailer: aerc 0.20.1-0-g2ecb8770224a-dirty
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>
<c4110fb3-3313-422d-99b5-ca7514405a47@proxmox.com>
In-Reply-To: <c4110fb3-3313-422d-99b5-ca7514405a47@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.017 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-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pdm-devel-bounces@lists.proxmox.com
Sender: "pdm-devel" <pdm-devel-bounces@lists.proxmox.com>
On Thu Apr 10, 2025 at 8:28 AM CEST, Dominik Csapak wrote:
> On 4/9/25 14:58, 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:
>>>>
>>>>> +/// Get ACL entries, can be filter by path.
>>>>> +pub fn read_acl(
>>>>> + path: Option<String>,
>>>>> + exact: bool,
>>>>> + rpcenv: &mut dyn RpcEnvironment,
>>>>> +) -> Result<Vec<AclListItem>, Error> {
>>>>> + let auth_id = rpcenv
>>>>> + .get_auth_id()
>>>>> + .ok_or_else(|| format_err!("endpoint called without an auth id"))?
>>>>> + .parse()?;
>>>>> +
>>>>> + let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
>>>>> +
>>>>> + let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
>>>>> + Some(auth_id)
>>>>> + } else {
>>>>> + None
>>>>> + };
>>>>
>>>> As discussed offline, maybe we can use CachedUserInfo::check_privs here?
>>>>
>>>>
>>>
>>> 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,
>>
>> the false here means that partial matches are discounted. i'm not sure
>> this is correct as at least in pbs and pdm, we do use a partial check as
>> that is equivalent to the check i ported over.
>>
>> imo, we'd need to discuss what the proper semantics are here and at
>> least up until now, we decided for partial semantics.
>
> IIUC the PRIV_PERMISSIONS_MODIFY is only a single bit, so partial/not partial makes
> no difference in this diff here.
>
> but yeah sure, if we have multiple privileges that would all allow setting
> ACL individually, we would have to match with `partial = true`
well, except your code stems from the pre-existing code (in pbs
presumably?). in my moved implementation PRIV_PERMISSIONS_MODIFY isn't
used anymore. the value is parameterized by the product via the
AccessControlConfig. which is necessary, otherwise we need to create
some kind of minimal set of common privileges between all products.
keeping that clean and conflict free sounds more tedious to me, though.
we could also expose whether this should allow partial matches or not
there (in the AccessControlConfig) too. for now i'd stick with keeping
the pre-existing behaviour where we can (and we can do this easily here)
in order to avoid possibly confusing bugs. unless there is a good reason
to forbid partial matches at this point already.
but yes, as mentioned yesterday, all current products define the
privileges as a bitmap (via the `constnamedbitmap!` macro). therefore no
privileges *should* use more than one bit and making that change
*should* be safe.
>>> + )
>>> + .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 => {
>>> ---
>>
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel