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

[Thomas Skinner]

On Tue, Mar 18, 2025 at 4:34 AM Mira Limbeck <m.limbeck at proxmox.com> wrote:
>
> On 3/17/25 13:18, Fabian Grünbichler wrote:
> > On February 13, 2025 12:03 pm, Fabian Grünbichler wrote:
> >>
> >>> Mira Limbeck <m.limbeck at proxmox.com> hat am 12.02.2025 15:51 CET geschrieben:
> >>>
> >>>
> >>> On 2/11/25 06:40, Thomas Skinner wrote:
> >>>> Signed-off-by: Thomas Skinner <thomas at atskinner.net>
> >>>> ---
> >>>>  src/PVE/API2/OpenId.pm   | 79 ++++++++++++++++++++++++++++++++++++++++
> >>>>  src/PVE/AccessControl.pm |  2 +-
> >>>>  src/PVE/Auth/OpenId.pm   | 33 +++++++++++++++++
> >>>>  src/PVE/Auth/Plugin.pm   |  1 +
> >>>>  4 files changed, 114 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> >>>> index 77410e6..818175e 100644
> >>>> --- a/src/PVE/API2/OpenId.pm
> >>>> +++ b/src/PVE/API2/OpenId.pm
> >>>> @@ -13,6 +13,7 @@ use PVE::Cluster qw(cfs_read_file cfs_write_file);
> >>>>  use PVE::AccessControl;
> >>>>  use PVE::JSONSchema qw(get_standard_option);
> >>>>  use PVE::Auth::Plugin;
> >>>> +use PVE::Auth::OpenId;
> >>>>
> >>>>  use PVE::RESTHandler;
> >>>>
> >>>> @@ -220,6 +221,84 @@ __PACKAGE__->register_method ({
> >>>>            $rpcenv->check_user_enabled($username);
> >>>>        }
> >>>>
> >>>> +      if (defined(my $groups_claim = $config->{'groups-claim'})) {
> >>>> +          if (defined(my $groups_list = $info->{$groups_claim})) {
> >>>> +              if (ref($groups_list) eq 'ARRAY') {
> >>>> +                  PVE::AccessControl::lock_user_config(sub {
> >>>> +                      my $usercfg = cfs_read_file("user.cfg");
> >>>> +
> >>>> +                      # replace any invalid characters with
> >>>> +                      my $replace_character = $config->{'groups-replace-character'} // '_';
> >>>> +                      my $oidc_groups = { map {
> >>>> +                          $_ =~ s/[^$PVE::Auth::Plugin::groupname_regex_chars]/$replace_character/gr => 1
> >>>> +                      } $groups_list->@* };
> >>> maybe we could log any of those replacements here? doing this silently
> >>> may lead to confusion when groups don't match
> >>
> >> a similar issue is filed for LDAP/AD sync as well - and I now wonder based on the discussion there - do we really want to make this configurable? how do we want to handle conflicts? while it's a bit less critical for two or more OIDC groups to be mapped to the same PVE-side group (compared to the same happening with users ;)), if it's possible to avoid it that would still be great..
> >>
> >> groups currently allow .-_ as special characters, so we could designate one of them as escape character and then have a unique mapping for each character that isn't allowed on the PVE side (including that escape character ;))
> >>
> >> e.g., an OIDC group called "foo bar" could be encoded as "foo_32_bar" (where 32 is hex for ASCII-" "). correspondingly, a group called "foo_bar" would need to be encoded as "foo_5F_bar". (the second '_' could of course be left off if desired).
> >>
> >> unfortunately, adding an entirely new escape character is not really possible unless we want to wait for 9.0, as that would then break parsing of user.cfg in a mixed cluster which can have really dangerous side-effects..
> >>
> >> or we could live with such a potentially lossy mapping, but then I am not sure whether a single, hard-coded, documented value wouldn't be better? the main issue with that is if you allow (unprivileged) creation and joining of groups on the OIDC side, as then if there already is a group called "System Administrators" that got mapped to "System_Adminstrators" on the PVE side, a user could create and join "System!Administrators" on the OIDC side and get mapped to the existing, probably privileged "System_Administrators" group..
> >
> > this part now got split out into its own discussion:
> >
> > https://lore.proxmox.com/pve-devel/b8fba9f6-6c83-4846-923f-2f7b93856bcf@proxmox.com/T/#u

I've been lightly following this, but it's clear there's not quite a
determination yet. Groups with spaces in the names would be highly
useful for Active Directory environments.

> >
> > what do you think about the following to not keep this blocked longer:
> >
> > - rebase this series
> > - drop the name mangling/.. part for now, and only allow groups that
> >   work with the PVE constraints for the time being
> >
> > we can implement it when we've decided how to handle the name
> > mangling/collision/.. issue, and ensure we get a consistent
> > implementation for both LDAP/AD and OIDC.
> >
>
> Sounds good to me. Group support is a huge improvement even with this
> limitation for now.
>

Good with me, too. I'll use the same logic as the current LDAP group
sync to only allow the groups that are already within constraints to
get through. I'll submit it by this weekend. Thanks!