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

[Fabian Grünbichler]

> 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..