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

[Dominik Csapak]

On 4/10/25 10:17, Shannon Sterz wrote:
> 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.

i don't think returning an extra boolean to configure is too much work
but err'ing on the safe side to require all permissions defined by default
does not sound too bad to me to start (we can always loosen it)

> 
> 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.

as i wrote my change is only an example and probably should not be used
as is without thinking more about it

also I wanted to put more emphasis on the token/user issue below, than
on the check_privs call

> 
>>>> +        )
>>>> +        .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 => {
>>>> ---
>>>
>