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

[Thomas Skinner <thomas@atskinner.net>]

> 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