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.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
