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

[Thomas Skinner <thomas@atskinner.net>]

On Wed, Nov 13, 2024 at 6:46 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
> a few nits, mostly style related below

Will get these fixed up and submit in a v2 patch.

> On September 1, 2024 6:55 pm, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
> >  src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> > index 77410e6..22a2188 100644
> > --- a/src/PVE/API2/OpenId.pm
> > +++ b/src/PVE/API2/OpenId.pm
> > @@ -220,6 +220,38 @@ __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 (UNIVERSAL::isa($groups_list, 'ARRAY')) {
>
> we normally use `ref($groups_list) eq 'ARRAY'`
>
> > +                                     PVE::AccessControl::lock_user_config(sub {
> > +                                             my $usercfg = cfs_read_file("user.cfg");
> > +
> > +                                             # if groups should be overwritten, delete them first
> > +                                             if ( $config->{'groups-overwrite'}) {
> > +                                                     PVE::AccessControl::delete_user_group($username, $usercfg);
> > +                                             }
> > +
> > +                                             # replace any invalid characters with
> > +                                             my $replace_character = $config->{'groups-replace-character'};
> > +                                             my @oidc_groups_list = map { $_ =~ s/[^A-Za-z0-9\.\-_]/$replace_character/gr } @{ $groups_list };
>
> we normally use array references, and (for new code) dereferencing via
> ->
>
> this RE (continued below)
>
> > +
> > +                                             # only populate groups that are in the oidc list and exist in pve
> > +                                             my @existing_groups_list = keys %{$usercfg->{groups}};
> > +                                             my @groups_intersect = grep { my $g = $_; grep $_ eq $g, @oidc_groups_list } @existing_groups_list;
>
> we do have PVE::Tools:array_intersect which does this for N array
> references..
>
> > +
> > +                                             # ensure user is a member of these groups
> > +                                             map { PVE::AccessControl::add_user_group($username, $usercfg, $_) } @groups_intersect;
>
> this could be a `for` loop, since the map result is not used at all..
>
> > +
> > +                                             cfs_write_file("user.cfg", $usercfg);
> > +                                     }, "openid group mapping failed");
> > +                             } else {
> > +                                     syslog('err', "groups list is not an array; groups will not be updated");
> > +                             }
> > +                     } else {
> > +                             syslog('err', "groups claim '$groups_claim' is not found in claims");
> > +                     }
> > +             }
> > +
> >           my $ticket = PVE::AccessControl::assemble_ticket($username);
> >           my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
> >           my $cap = $rpcenv->compute_api_permission($username);
> > diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
> > index c8e4db9..0e3fdc4 100755
> > --- a/src/PVE/Auth/OpenId.pm
> > +++ b/src/PVE/Auth/OpenId.pm
> > @@ -42,6 +42,24 @@ sub properties {
> >           type => 'string',
> >           optional => 1,
> >       },
> > +     "groups-claim" => {
> > +         description => "OpenID claim used to retrieve groups with.",
> > +         type => 'string',
>
> forgot this part: this should probably have a format to limit valid
> values..

I would tend to agree, but there doesn't seem to be a specification
that I can find that requires certain characters as part of the claim
name. However, if Proxmox wants to have one, I would suggest the same
RE used for the invalid characters for groups replacement to start off
with and document appropriately what characters are allowed.

> > +         optional => 1,
> > +     },
> > +     "groups-overwrite" => {
> > +             description => "All groups will be overwritten for the user on login.",
> > +         type => 'boolean',
> > +             default => 0,
> > +         optional => 1,
> > +     },
> > +     "groups-replace-character" => {
> > +         description => "Character used to replace any invalid characters in groups from provider.",
> > +         type => 'string',
> > +             pattern => '^[A-Za-z0-9\.\-_]$',
>
> and this RE are inverses of eachother - should we define them in one
> place in case we ever need to update it?

Yes, that would probably be ideal. I'll work it into the v2. It uses
the same RE as the groupid validator, so maybe it can reference that
as well.

> > +             default => '_',
> > +         optional => 1,
> > +     },
> >       prompt => {
> >           description => "Specifies whether the Authorization Server prompts the End-User for"
> >               ." reauthentication and consent.",
> > @@ -73,6 +91,9 @@ sub options {
> >       "client-key" => { optional => 1 },
> >       autocreate => { optional => 1 },
> >       "username-claim" => { optional => 1, fixed => 1 },
> > +     "groups-claim" => { optional => 1 },
> > +     "groups-overwrite" => { optional => 1 },
> > +     "groups-replace-character" => { optional => 1},
> >       prompt => { optional => 1 },
> >       scopes => { optional => 1 },
> >       "acr-values" => { optional => 1 },
> > --
> > 2.39.2
> >
> >
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> >
> >
> >
>

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel