Re: [pmg-devel] [PATCH pmg-api v5 7/10] api: oidc login similar to PVE

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

On Tue, Feb 18, 2025 at 05:19:02PM +0100, Markus Frank wrote:
> Allow OpenID Connect login using the Rust OIDC module.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/Makefile                  |   1 +
>  src/PMG/API2/AccessControl.pm |   7 +
>  src/PMG/API2/OIDC.pm          | 243 ++++++++++++++++++++++++++++++++++
>  src/PMG/HTTPServer.pm         |   2 +
>  4 files changed, 253 insertions(+)
>  create mode 100644 src/PMG/API2/OIDC.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index e939bbd..7033a66 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -152,6 +152,7 @@ LIBSOURCES =				\
>  	PMG/API2/Quarantine.pm		\
>  	PMG/API2/AccessControl.pm	\
>  	PMG/API2/Realm.pm		\
> +	PMG/API2/OIDC.pm		\
>  	PMG/API2/TFA.pm			\
>  	PMG/API2/TFAConfig.pm		\
>  	PMG/API2/ObjectGroupHelpers.pm	\
> diff --git a/src/PMG/API2/AccessControl.pm b/src/PMG/API2/AccessControl.pm
> index f4cbc81..1881fac 100644
> --- a/src/PMG/API2/AccessControl.pm
> +++ b/src/PMG/API2/AccessControl.pm
> @@ -13,6 +13,7 @@ use PMG::Utils;
>  use PMG::UserConfig;
>  use PMG::AccessControl;
>  use PMG::API2::Realm;
> +use PMG::API2::OIDC;
>  use PMG::API2::Users;
>  use PMG::API2::TFA;
>  use PMG::TFAConfig;
> @@ -36,6 +37,11 @@ __PACKAGE__->register_method ({
>      path => 'domains',
>  });
>  
> +__PACKAGE__->register_method ({
> +    subclass => "PMG::API2::OIDC",
> +    path => 'oidc',
> +});
> +
>  __PACKAGE__->register_method ({
>      name => 'index',
>      path => '',
> @@ -64,6 +70,7 @@ __PACKAGE__->register_method ({
>  	my $res = [
>  	    { subdir => 'ticket' },
>  	    { subdir => 'domains' },
> +	    { subdir => 'oidc' },
>  	    { subdir => 'password' },
>  	    { subdir => 'users' },
>  	];
> diff --git a/src/PMG/API2/OIDC.pm b/src/PMG/API2/OIDC.pm
> new file mode 100644
> index 0000000..fdab44d
> --- /dev/null
> +++ b/src/PMG/API2/OIDC.pm
> @@ -0,0 +1,243 @@
> +package PMG::API2::OIDC;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Tools qw(extract_param lock_file);
> +use Proxmox::RS::OIDC;
> +
> +use PVE::Exception qw(raise raise_perm_exc raise_param_exc);
> +use PVE::SafeSyslog;
> +use PVE::INotify;
> +use PVE::JSONSchema qw(get_standard_option);
> +
> +use PMG::AccessControl;
> +use PMG::RESTEnvironment;
> +use PVE::RESTHandler;
> +
> +use base qw(PVE::RESTHandler);
> +
> +my $oidc_state_path = "/var/lib/pmg";
> +
> +my $lookup_oidc_auth = sub {
> +    my ($realm, $redirect_url) = @_;
> +
> +    my $cfg = PVE::INotify::read_file('realms.cfg');
> +    my $ids = $cfg->{ids};
> +
> +    die "authentication domain '$realm' does not exist\n" if !$ids->{$realm};
> +
> +    my $config = $ids->{$realm};
> +    die "wrong realm type ($config->{type} != oidc)\n" if $config->{type} ne "oidc";
> +
> +    my $oidc_config = {
> +	issuer_url => $config->{'issuer-url'},
> +	client_id => $config->{'client-id'},
> +	client_key => $config->{'client-key'},
> +    };
> +    $oidc_config->{prompt} = $config->{'prompt'} if defined($config->{'prompt'});
> +
> +    my $scopes = $config->{'scopes'} // 'email profile';
> +    $oidc_config->{scopes} = [ PVE::Tools::split_list($scopes) ];
> +
> +    if (defined(my $acr = $config->{'acr-values'})) {
> +	$oidc_config->{acr_values} = [ PVE::Tools::split_list($acr) ];
> +    }
> +
> +    my $oidc = Proxmox::RS::OIDC->discover($oidc_config, $redirect_url);
> +    return ($config, $oidc);
> +};
> +
> +__PACKAGE__->register_method ({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => "Directory index.",
> +    permissions => {
> +	user => 'all',
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => "object",
> +	    properties => {
> +		subdir => { type => 'string' },
> +	    },
> +	},
> +	links => [ { rel => 'child', href => "{subdir}" } ],
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	return [
> +	    { subdir => 'auth-url' },
> +	    { subdir => 'login' },
> +	];
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'auth_url',
> +    path => 'auth-url',
> +    method => 'POST',
> +    protected => 1,
> +    description => "Get the OpenId Connect Authorization Url for the specified realm.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    realm => {
> +		description => "Authentication domain ID",
> +		type => 'string',
> +		pattern => qr/[A-Za-z][A-Za-z0-9\.\-_]+/,
> +		maxLength => 32,
> +	    },
> +	    'redirect-url' => {
> +		description => "Redirection Url. The client should set this to the used server url (location.origin).",
> +		type => 'string',
> +		maxLength => 255,
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => "string",
> +	description => "Redirection URL.",
> +    },
> +    permissions => { user => 'world' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $realm = extract_param($param, 'realm');
> +	my $redirect_url = extract_param($param, 'redirect-url');
> +
> +	my ($config, $oidc) = $lookup_oidc_auth->($realm, $redirect_url);
> +	my $url = $oidc->authorize_url($oidc_state_path , $realm);
> +
> +	return $url;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'login',
> +    path => 'login',
> +    method => 'POST',
> +    protected => 1,
> +    description => " Verify OpenID Connect authorization code and create a ticket.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    'state' => {
> +		description => "OpenId Connect state.",
> +		type => 'string',
> +		maxLength => 1024,
> +            },
> +	    code => {
> +		description => "OpenId Connect authorization code.",
> +		type => 'string',
> +		maxLength => 4096,
> +            },
> +	    'redirect-url' => {
> +		description => "Redirection Url. The client should set this to the used server url (location.origin).",
> +		type => 'string',
> +		maxLength => 255,
> +	    },
> +	},
> +    },
> +    returns => {
> +	properties => {
> +	    role => { type => 'string', optional => 1},
> +	    username => { type => 'string' },
> +	    ticket => { type => 'string' },
> +	    CSRFPreventionToken => { type => 'string' },
> +	},
> +    },
> +    permissions => { user => 'world' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PMG::RESTEnvironment->get();
> +
> +	my $res;
> +	eval {
> +	    my ($realm, $private_auth_state) = Proxmox::RS::OIDC::verify_public_auth_state(
> +		$oidc_state_path, $param->{'state'});
> +
> +	    my $redirect_url = extract_param($param, 'redirect-url');
> +
> +	    my ($config, $oidc) = $lookup_oidc_auth->($realm, $redirect_url);
> +
> +	    my $info = $oidc->verify_authorization_code($param->{code}, $private_auth_state);
> +	    my $subject = $info->{'sub'};
> +
> +	    my $unique_name;
> +
> +	    my $user_attr = $config->{'username-claim'} // 'sub';
> +	    if (defined($info->{$user_attr})) {
> +		$unique_name = $info->{$user_attr};
> +	    } elsif ($user_attr eq 'subject') { # stay compat with old versions
> +		$unique_name = $subject;
> +	    } elsif ($user_attr eq 'username') { # stay compat with old versions
> +		my $username = $info->{'preferred_username'};
> +		die "missing claim 'preferred_username'\n" if !defined($username);
> +		$unique_name =  $username;
> +	    } else {
> +		# neither the attr nor fallback are defined in info..
> +		die "missing configured claim '$user_attr' in returned info object\n";
> +	    }
> +
> +	    my $username = "${unique_name}\@${realm}";
> +	    # first, check if $username respects our naming conventions
> +	    PMG::Utils::verify_username($username);
> +	    if ($config->{'autocreate'} && !$rpcenv->check_user_exist($username, 1)) {
> +		my $code = sub {
> +		    my $usercfg = PMG::UserConfig->new();
> +
> +		    my $entry = { enable => 1 };
> +		    if (defined(my $email = $info->{'email'})) {
> +			$entry->{email} = $email;
> +		    }
> +		    if (defined(my $given_name = $info->{'given_name'})) {
> +			$entry->{firstname} = $given_name;
> +		    }
> +		    if (defined(my $family_name = $info->{'family_name'})) {
> +			$entry->{lastname} = $family_name;
> +		    }
> +		    $entry->{role} = $config->{'autocreate-role'} // 'audit';
> +		    $entry->{userid} = $username;
> +		    $entry->{username} = $unique_name;
> +		    $entry->{realm} = $realm;
> +
> +		    die "User '$username' already exists\n"
> +			if $usercfg->{$username};
> +
> +		    $usercfg->{$username} = $entry;
> +
> +		    $usercfg->write();
> +		};
> +		PMG::UserConfig::lock_config($code, "autocreate openid connect user failed");
this part failed in a testsetup of Mira - with a user who did not have an
e-mail set:
$usercfg->write() above calls verify_entry in PMG::UserConfig, which checks
if the entries match the schema.

I could not reproduce this with Keycloak easily (as it refuses to store
something as e-mail which does not match its validation, but maybe
Authentik, which Mira uses? sends an empty string if no e-mail is set.

from a quick look I think PVE does not verify the schema (or accepts '' as
a valid entry)

depending on if it's authentik sending '' as non-defined e-mail we maybe
should change the
if (defined(my $email = $info->{'email'})
to if (my $email = $info->{'email'}


> +	    }
> +	    my $role = $rpcenv->check_user_enabled($username);
> +
> +	    my $ticket = PMG::Ticket::assemble_ticket($username);
> +	    my $csrftoken = PMG::Ticket::assemble_csrf_prevention_token($username);
> +
> +	    $res = {
> +		ticket => $ticket,
> +		username => $username,
> +		CSRFPreventionToken => $csrftoken,
> +		role => $role,
> +	    };
> +
> +	};
> +	if (my $err = $@) {
> +	    my $clientip = $rpcenv->get_client_ip() || '';
> +	    syslog('err', "openid connect authentication failure; rhost=$clientip msg=$err");
> +	    # do not return any info to prevent user enumeration attacks
> +	    die PVE::Exception->new("authentication failure $err\n", code => 401);
> +	}
> +
> +	syslog('info', 'root@pam', "successful openid connect auth for user '$res->{username}'");
this causes
```
Redundant argument in sprintf at /usr/lib/x86_64-linux-gnu/perl/5.36/Sys/Syslog.pm line 454.
```
to be printed to the syslog (the 'root@pam' argument should simply be
dropped) - the remaining log calls look ok from a quick glance.


> +	return $res;
> +    }});
> diff --git a/src/PMG/HTTPServer.pm b/src/PMG/HTTPServer.pm
> index 49724fe..27a313d 100644
> --- a/src/PMG/HTTPServer.pm
> +++ b/src/PMG/HTTPServer.pm
> @@ -58,6 +58,8 @@ sub auth_handler {
>  
>      # explicitly allow some calls without auth
>      if (($rel_uri eq '/access/domains' && $method eq 'GET') ||
> +	($rel_uri eq '/access/oidc/login' &&  $method eq 'POST') ||
> +	($rel_uri eq '/access/oidc/auth-url' &&  $method eq 'POST') ||
>  	($rel_uri eq '/quarantine/sendlink' && ($method eq 'GET' || $method eq 'POST')) ||
>  	($rel_uri eq '/access/ticket' && ($method eq 'GET' || $method eq 'POST'))) {
>  	$require_auth = 0;
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
> 


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