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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On December 24, 2024 9:24 pm, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  src/PVE/API2/OpenId.pm   | 68 ++++++++++++++++++++++++++++++++++++++++
>  src/PVE/AccessControl.pm | 13 +++++---
>  src/PVE/Auth/OpenId.pm   | 30 ++++++++++++++++++
>  3 files changed, 107 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> index 77410e6..5cfe5a1 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,73 @@ __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_list = map { 

style nit: we mostly use array references in our code, i.e.

my $oidc_group_list = [ map ... ];

> +				$_ =~ s/[^$PVE::Auth::OpenId::groupname_regex_chars]/$replace_character/gr 
> +			    } $groups_list->@*;
> +
> +			    # list groups that exist in pve
> +			    my @existing_groups_list = keys %{$usercfg->{groups}};

same here

> +
> +			    my @groups_intersect;

and here

> +			    if ( $config->{'groups-autocreate'} ) {
> +				# populate all groups in claim
> +				@groups_intersect = @oidc_groups_list;

and here ;)

> +			    }
> +			    else {

style nit: the '}' and 'else {' should be on a single line

> +				# only populate groups that are in the oidc list and exist in pve
> +				@groups_intersect = @{PVE::Tools::array_intersect(
> +				    \@oidc_groups_list,
> +				    \@existing_groups_list,
> +				)};

then this simply becomes

$groups_intersect = PVE::Tools::array_intersect($oidc_groups_list, $existing_groups_list);

but since the existing groups are already a hash, you could also do the
intersection + conversion to a hash in a single go here (see below for
why a hash is nicer anyway):

			    if ($config->{'groups-autocreate'}) {
				# populate all groups in claim
				$groups_intersect = { map { $_ => 1 } $oidc_groups_list->@* };
			    } else {
			        # populate only existing groups
			        for my $group ($oidc_groups_list->@*) {
			            $groups_intersect->{$group} = 1 if $usercfg->{groups}->{$group};
			        }
			    }


> +			    }
> +
> +			    # if groups should be overwritten, find and delete the ones to remove
> +			    if ( $config->{'groups-overwrite'} ) {
> +				# get the groups that need to be removed from the user
> +				my %groups_remove_user;
> +				$groups_remove_user{ $_ } = undef 
> +				    for keys %{$usercfg->{users}->{$username}->{groups}};
> +				delete $groups_remove_user{ $_ } for @groups_intersect;
> +				
> +				# ensure user is not a member of these groups
> +				PVE::AccessControl::delete_user_group_single(
> +				    $username, 
> +				    $usercfg, 
> +				    $_,
> +				) for keys %groups_remove_user;
> +			    }
> +
> +			    # get the groups that need to be added to the user
> +			    my %groups_add_user;
> +			    $groups_add_user{ $_ } = undef for @groups_intersect;
> +			    delete $groups_add_user{ $_ } 
> +			        for keys %{$usercfg->{users}->{$username}->{groups}};
> +			    
> +			    # ensure user is a member of these groups
> +			    PVE::AccessControl::add_user_group(
> +				$username,
> +			    	$usercfg,
> +				$_
> +			    ) for keys %groups_add_user;
> +


we don't do post-for either ;)

you can just do

# converts group intersection to hash (but see above ;))
my $groups_intersect_hash = { map { $_ => 1 } $groups_intersect->@ };

if ($config->{'groups-overwrite'}) {
    my $user_groups = $usercfg->{users}->{$username}->{groups};
    for my $group (keys $user_groups->%*) {
        delete $user_groups->{$group} if !groups_intersect_hash->{$group};
    }
}

and similarly for the addition, although with addition, you could even
do it unconditionally since it is both cheap and idempotent (it just
sets two booleans in the $usercfg hash).

or you could make it a bit easier, and just delete all groups and then
add all the groups in the intersecting set, which would be similar to
how we do it when directly overwriting a user's group via the API/pveum:

https://git.proxmox.com/?p=pve-access-control.git;a=blob;f=src/PVE/API2/User.pm;h=535e58e0b3dbd4c8d8c882b7845173489803d8d0;hb=HEAD#l417

the changes will only persisted on-disk once you hit the cfs_write in the
next line, so this shouldn't be an issue.

some higher-level questions:

do we want to have a bit more logging here, although it would only end
up in syslog/journal.. maybe at least for groups which are created?

do we want to mangle the group names to include the OIDC-realm name,
like we do for LDAP/AD syncing? that way it is more clear that those
groups originated from OIDC.. downside is that you can't use a group
shared between OIDC and other realms..

> +			    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/AccessControl.pm b/src/PVE/AccessControl.pm
> index 47f2d38..d643a00 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -990,12 +990,17 @@ sub delete_user_group {
>      my ($username, $usercfg) = @_;
>  
>      foreach my $group (keys %{$usercfg->{groups}}) {
> -
> -	delete ($usercfg->{groups}->{$group}->{users}->{$username})
> -	    if $usercfg->{groups}->{$group}->{users}->{$username};
> +	delete_user_group_single($username, $usercfg, $group);

to be honest I am not sure why that post-if is there in the first place..

when parsing the config, the group is initialized with an empty 'users'
hash as members before filling using the member list, so there is no
risk of auto-vivifying something here..

if you want to delete a user's group membership, you can just

delete $usercfg->{groups}->{$group}->{users}->{$username};

and we don't need this refactoring

>      }
>  }
>  
> +sub delete_user_group_single {
> +    my ($username, $usercfg, $group) = @_;
> +
> +    delete ($usercfg->{groups}->{$group}->{users}->{$username})
> +	if $usercfg->{groups}->{$group}->{users}->{$username};
> +}
> +
>  sub delete_user_acl {
>      my ($username, $usercfg) = @_;
>  
> @@ -1293,7 +1298,7 @@ PVE::JSONSchema::register_format('pve-groupid', \&verify_groupname);
>  sub verify_groupname {
>      my ($groupname, $noerr) = @_;
>  
> -    if ($groupname !~ m/^[A-Za-z0-9\.\-_]+$/) {
> +    if ($groupname !~ m/^[$PVE::Auth::OpenId::groupname_regex_chars]+$/) {

see below ;) if the RE moves there, we could actually move the whole
helper and format registration (maybe as a separate commit/patch?)

>  
>  	die "group name '$groupname' contains invalid characters\n" if !$noerr;
>  
> diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
> index c8e4db9..d7b5574 100755
> --- a/src/PVE/Auth/OpenId.pm
> +++ b/src/PVE/Auth/OpenId.pm
> @@ -9,6 +9,8 @@ use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file
>  
>  use base qw(PVE::Auth::Plugin);
>  
> +our $groupname_regex_chars = qr/A-Za-z0-9\.\-_/;

this should probably be in PVE::Auth::Plugin, where the user and realm
REs are as well.

> +
>  sub type {
>      return 'openid';
>  }
> @@ -42,6 +44,30 @@ sub properties {
>  	    type => 'string',
>  	    optional => 1,
>  	},
> +	"groups-claim" => {
> +	    description => "OpenID claim used to retrieve groups with.",
> +	    type => 'string',

this should probably be restricted (our usual safeid might be too
strict (I know some OIDC providers like to user things like @ and : in
attributes, would have to check what the spec says here?), but some
sensible set of characters at least would be nice ;))

> +	    optional => 1,
> +	},
> +	"groups-autocreate" => {
> +	    description => "Automatically create groups if they do not exist.",
> +	    optional => 1,
> +	    type => 'boolean',
> +	    default => 0,
> +	},
> +	"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 => qr/^[$groupname_regex_chars]$/,
> +	    default => '_',
> +	    optional => 1,
> +	},
>  	prompt => {
>  	    description => "Specifies whether the Authorization Server prompts the End-User for"
>  	        ." reauthentication and consent.",
> @@ -73,6 +99,10 @@ sub options {
>  	"client-key" => { optional => 1 },
>  	autocreate => { optional => 1 },
>  	"username-claim" => { optional => 1, fixed => 1 },
> +	"groups-claim" => { optional => 1 },
> +	"groups-autocreate" => { optional => 1 },
> +	"groups-overwrite" => { optional => 1 },
> +	"groups-replace-character" => { optional => 1},
>  	prompt => { optional => 1 },
>  	scopes => { optional => 1 },
>  	"acr-values" => { optional => 1 },
> -- 
> 2.39.5
> 
> 
> _______________________________________________
> 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