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
