Re: [pmg-devel] applied: [PATCH api] oidc realm: add more flexiple access-role assignment

[Stoiko Ivanov <s.ivanov@proxmox.com>]

Thanks! from a quick look - it seems sensible, and the naming is in order.

one typo and 2 minor nits inline:
On Thu, 27 Feb 2025 10:51:52 +0100
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> deprecate the fixed role assignment property and replace it with one
> that allows to select the source of the role, for now allow static,
> which is equivalent with what we had, and also to query the role from
> a role claim, so that the OIDC provider can return the correct role
> inside the returned user information.
> 
> Tested the dynamic role assignment with keycload by setting up an
> 'User Attribute' Mapper for a client and added added the respective
> user attribute to some test user.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
> 
> It's not much code and most is schema and parsing, the implementation
> itself is pretty straigt forward, basically the only questionable thing
> is naming of properties.
> 
>  src/PMG/API2/OIDC.pm | 26 ++++++++++++++++++++-
>  src/PMG/Auth/OIDC.pm | 54 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PMG/API2/OIDC.pm b/src/PMG/API2/OIDC.pm
> index 92ff88d..20acc41 100644
> --- a/src/PMG/API2/OIDC.pm
> +++ b/src/PMG/API2/OIDC.pm
> @@ -13,6 +13,7 @@ use PVE::JSONSchema qw(get_standard_option);
>  use PVE::RESTHandler;
>  
>  use PMG::AccessControl;
> +use PMG::Auth::OIDC;
>  use PMG::Auth::Plugin;
>  use PMG::RESTEnvironment;
>  
> @@ -204,7 +205,30 @@ __PACKAGE__->register_method ({
>  		    if (defined(my $family_name = $info->{'family_name'})) {
>  			$entry->{lastname} = $family_name;
>  		    }
> -		    $entry->{role} = $config->{'autocreate-role'} // 'audit';
> +
> +		    # NOTE: 'autocreate-role' is deprecated and has less priority than the more
> +		    # flexible 'autocreate-role-assignment'
> +		    $entry->{role} = $config->{'autocreate-role'} // 'audit'; # default
> +		    if (my $role_assignment_raw = $config->{'autocreate-role-assignment'}) {
> +			my $role_assignment =
> +			    PMG::Auth::OIDC::parse_autocreate_role_assignment($role_assignment_raw);
> +
> +			if ($role_assignment->{source} eq 'fixed') {
> +			    $entry->{role} = $role_assignment->{'fixed-role'};
> +			} elsif ($role_assignment->{source} eq 'from-claim') {
> +			    my $role_attr = $role_assignment->{'role-claim'};
> +			    if (my $role = $info->{$role_attr}) {
> +				$role = lc($role); # normalize to lower-case
> +				die "required '$role_attr' role-claim attribute not found, cannot autocreate user\n"
> +				    if $role !~ /^(?:admin|qmanager|audit|helpdesk)$/;
> +				$entry->{role} = $role;
> +			    } else {
> +				die "required '$role_attr' role-claim attribute not found, cannot autocreate user\n";
> +			    }
> +			} else {
> +			    die "unkown role assignment source '$role_assignment->{source}' - implement me";
> +			}
> +		    }
>  		    $entry->{userid} = $username;
>  		    $entry->{username} = $unique_name;
>  		    $entry->{realm} = $realm;
> diff --git a/src/PMG/Auth/OIDC.pm b/src/PMG/Auth/OIDC.pm
> index 4129d47..9082c75 100755
> --- a/src/PMG/Auth/OIDC.pm
> +++ b/src/PMG/Auth/OIDC.pm
> @@ -4,6 +4,8 @@ use strict;
>  use warnings;
>  
>  use PVE::Tools;
> +use PVE::JSONSchema qw(parse_property_string);
> +
>  use PMG::Auth::Plugin;
>  
>  use base qw(PMG::Auth::Plugin);
> @@ -12,6 +14,44 @@ sub type {
>      return 'oidc';
>  }
>  
> +my $autocreate_role_assignment_format = {
> +    source => {
> +	type => 'string',
> +	enum => ['fixed', 'from-claim'],
> +	default => 'fixed',
> +	description => "How the access role for a newly auto-created user should be selected.",
> +    },
> +    'fixed-role' => {
> +	type => 'string',
> +	enum => ['admin', 'qmanager', 'audit', 'helpdesk'],
> +	default => 'audit',
> +	optional => 1,
> +	description => "The fixed role that should be assigned to auto-created users.",
> +    },
> +    'role-claim' => {
> +	description => "OIDC claim used to assign the unique username.",
> +	type => 'string',
> +	default => 'role',
> +	optional => 1,
> +	pattern => qr/^[a-zA-Z0-9._:-]+$/,
> +    },
> +};
> +
> +
> +sub parse_autocreate_role_assignment {
> +    my ($raw) = @_;
> +    return undef if !$raw or !length($raw);
> +
> +    my $role_assignment = parse_property_string($autocreate_role_assignment_format, $raw);
> +    $role_assignment->{'fixed-role'} = 'audit'
> +	if $role_assignment->{'source'} eq 'fixed' && !defined($role_assignment->{'fixed-role'});
> +
> +    $role_assignment->{'role-claim'} = 'role'
> +	if $role_assignment->{'source'} eq 'from-clain' && !defined($role_assignment->{'role-claim'});
`s/from-clain/from-claim/`
> +
> +    return $role_assignment;
> +}
> +
>  sub properties {
>      return {
>  	'issuer-url' => {
> @@ -39,13 +79,20 @@ sub properties {
>  	    type => 'boolean',
>  	    default => 0,
>  	},
> -	'autocreate-role' => {
> -	    description => "Automatically create users with a specific role.",
> +	'autocreate-role' => { # NOTE: depreacated since the beginning, just here for compat
nit: `s/depreacated/deprecated/` (had to google myself, as I always get
that wrong)

> +	    description => "Automatically create users with a specific role."
> +		." NOTE: Depreacated, favor 'autocreate-role-assignment'",
nit: deprecated
>  	    type => 'string',
>  	    enum => ['admin', 'qmanager', 'audit', 'helpdesk'],
>  	    default => 'audit',
>  	    optional => 1,
>  	},
> +	'autocreate-role-assignment' => {
> +	    description => "Defines which role should be assigned to auto-created users.",
> +	    type => 'string', format => $autocreate_role_assignment_format,
> +	    default => 'source=fixed,fixed-role=auditor',
> +	    optional => 1,
> +	},
>  	'username-claim' => {
>  	    description => "OpenID Connect claim used to generate the unique username.",
>  	    type => 'string',
> @@ -84,7 +131,8 @@ sub options {
>  	'client-id' => {},
>  	'client-key' => { optional => 1 },
>  	autocreate => { optional => 1 },
> -	'autocreate-role' => { optional => 1 },
> +	'autocreate-role' => { optional => 1 }, # NOTE: depreacated in favor of 'autocreate-role-assignment'
nit: deprecated

> +	'autocreate-role-assignment' => { optional => 1 },
>  	'username-claim' => { optional => 1, fixed => 1 },
>  	prompt => { optional => 1 },
>  	scopes => { optional => 1 },



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