Re: [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp v3 00/10] add support for checking acl permissions in (yew) front-ends

["Shannon Sterz" <s.sterz@proxmox.com>]

On Thu Nov 13, 2025 at 5:18 PM CET, Thomas Lamprecht wrote:
> Am 13.11.25 um 15:26 schrieb Shannon Sterz:
>>> high-level question:
>>>
>>> the actual privilege checks in the backend use the full set of ACLs. the
>>> frontend can only ever see a subset of ACLs, since giving it all ACLs
>>> would leak a lot of sensitive information.
>>>
>>> doesn't that mean that the frontend will make wrong decisions in some
>>> scenarios?
>>>
>>> e.g., the backend currently doesn't return any group ACLs if you do
>>> exact filtering. but group ACLs can influence the ACL resolution
>>
>> as discussed off list, that'd be true if the filter in
>> `extract_acl_node_data` isn't adapted. i'll add a fixme comment to the
>> next version of this series for now.
>>
>> how exactely to takle this will depend on how we implement groups:
>>
>> * does a user know that they are part of a group?
>
> No general objection, a lot of ACL system work that way, but should not
> be required.
>
>> * if disclosing such membership is fine, is it fine to disclose what the
>>   group has access to in all cases? e.g. what if the user is part of a
>>   group, but certain acl entries are then restricted on top via a
>>   NoAccess privilege or similar?
>
> Disclosing access is IMO always fine, that can basically be probed anyway.
> Where that access comes from does not really matter here.
>
>> * will looking up whether the user is part of a group be handled by the
>>   acl tree directly? (this is at least indicated by comments already
>>   present in `AclTreeNode::extract_group_roles()`)
>
> Probably, otherwise it would need to get the relevant acl's "injected".
>
>> most of these are difficult to answer without actually tackling an
>> implementation of the group feature. not entirely sure how i can address
>> this here beside adding that `fixme` comment.
>
> Seems OK to me. If we need more changes to make this safe enough for
> groups we can still change this, be it by adding a new endpoint and
> deprecating the old one, where the old one would then probably ignores
> groups for the rest of its existence.
> One option could then be to have something like the heuristic in PVE
> but more generic (and maybe we're able to make it a bit easier to
> understand), but tbh. that isn't exactly perfect either, and so trying
> this route until we get an actual blocker seems still worthwhile to me.
>
>
>> i suppose i could also try to extract the roles with `AclTree::roles`,
>> which extracts the roles via `AclTreeNode::extract_roles` which is
>> already somewhat opinionated about how groups should work here. not sure
>> what is ideal here.
>
> That I did not looked closely enough into to answer for sure.

thanks for your quick reply, i looked into this some more after i send
this mail and i have a version of this series here now that uses
`AclTreeNode::extract_roles` to extract the roles for user in general.
the upshot of this approach is that we already use the acl tree's
implementation here instead of manually extracting the roles. so once we
add better support for groups in the acl tree, we'll pick them up here
for free.

context: the acl tree has somewhat half-baked support for groups already
and supports extracting them correctly for a given user. the part that
is missing there is the look up of what groups a user belongs too. so
relying on that seems somewhat safe to me as the pre-existing codepaths
have been around for a while now.

it looks somewhat like this:

        if all_for_authid {
            if let Some(auth_id) = auth_id_filter {
               for (role, propagate) in node.extract_roles(auth_id, true) {
                    to_return.push(AclListItem {
                        path: path_str.to_owned(),
                        propagate,
                        // do not disclose what groups exist and by
                        // making them always look like user permissions
                        ugid_type: AclUgidType::User,
                        ugid: auth_id.to_string(),
                        roleid: role.to_string(),
                    })
                }
            }

what do you think? should we go with this approach instead?



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel