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

[Fabian Grünbichler]

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

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.