> 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: 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. 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. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
