Re: [pve-devel] [PATCH access-control v2 1/1] fix #4411: openid: add logic for openid groups support

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On February 8, 2025 7:40 am, Thomas Skinner wrote:
>> do we want to mangle the group names to include the OIDC-realm name,
>> like we do for LDAP/AD syncing? that way it is more clear that those
>> groups originated from OIDC.. downside is that you can't use a group
>> shared between OIDC and other realms..
> 
> More on this: it looks like in LDAP/AD sync, the group is suffixed
> with `-$realm`, which does meet the requirements of the existing regex
> character requirements for groups. We could do the same thing with the
> OIDC groups. I will add that suffix in as the option to be consistent.
> 
> -----
> 
> On a similar but separate note, I think that it may be more clear of
> where groups come from to have them optionally suffixed with something
> like `@$realm` like is already done for users. The verify function for
> groups could be adjusted to validate on a regex that makes the suffix
> optional and validates it as `group_regex@realm_regex`. The downside
> of this change is that it would break any existing groups or the
> module would need to be adjusted to continue to support or migrate the
> groups with the old suffix. An upside of this change is eliminating
> inadvertent group collisions. For example:

groups are not really scoped to a realm though, we only did this
"soft-namespace" for the sync feature to avoid accidents (similar to the
one you describe below).

> Consider we have 2 realms: AD/LDAP "ad-admins" and OIDC "oidc". Realm
> "ad-admins" suffixes groups automatically and realm "oidc" is
> configured to not suffix. Realm "ad-admins" contains a group
> "privileged" and a realm "oidc" claim sends a group
> "privileged-ad-admins".
> 
> With the existing configuration, the group "privileged" from realm
> "ad-admins" becomes "privileged-ad-admins" in PVE. The user with the
> group "privileged-ad-admins" in the OIDC claim is then granted access
> to this group on login. This could lead to potential erroneous group
> membership because the group names could overlap.

yes, this is why I think OIDC groups should also always add the suffix,
just like LDAP/AD. that way, any group name collision must stem from a
manually created group containing the suffix (which can still happen,
but is a lot less likely than without).

> 
> If the suffix was changed to include a character that would not exist
> in the group name (e.g. "@"), then it would be impossible to have this
> overlap. Additionally, this suffixing option could be extended to any
> other realm type that could introduce groups into PVE. I can see
> scenarios where having the option to enable/disable group suffixes for
> all realms would be useful. If the admin controls all the
> authentication services (or at least can ensure no inadvertent
> collisions would occur), the suffix is not needed. Non-suffixed group
> names could also simplify ACL delegation. If the admin cannot
> guarantee inadvertent collisions in group names, then suffixes that do
> not include a valid group character would be necessary to prevent
> collisions.
> 
> If PVE is interested in moving towards this, I'd be happy to start
> authoring a patch to support it. If nothing else, I can file a bug and
> we can continue discussion there.

I think this would be a very hard transition given that user.cfg
contents are at the very core of PVE - and it would break valid use
cases like pre-configuring groups that are part of a AD/LDAP-sync or
OIDC-autocreation scope. but if other people think this is worth
exploring, I am of course not objecting to further discussing it :)


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