On 3/17/25 13:18, Fabian Grünbichler wrote:
> On February 13, 2025 12:03 pm, Fabian Grünbichler wrote:
>>
>>> Mira Limbeck <m.limb...@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 <tho...@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-2f7b93856...@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.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
