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

[Mira Limbeck]

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