[pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature

[Shannon Sterz]

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(&current_auth_id, &["access", "acl"]);
>>> -    if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>>> +    let has_modify_permission = user_info
>>> +        .check_privs(
>>> +            &current_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 => {
>>> ---
>>