[pve-devel] [PATCH pve-access-control v2 0/5] add OpenId realms

[pve-devel] [PATCH pve-access-control v2 0/5] add OpenId realms

[Dietmar Maurer]

Changes in v2:
- also check if user is expired (in check_user_enabled)
- always die with newline
- rename "user-attr" to "username-claim"


Dietmar Maurer (5):
  check_user_enabled: also check if user is expired
  add OpenId configuration
  depend on libpve-rs-perl
  api: implement openid API
  implement OpenID autocreate user feature

 debian/control                |   2 +
 src/PVE/API2/AccessControl.pm |  60 ++-------
 src/PVE/API2/Makefile         |   3 +-
 src/PVE/API2/OpenId.pm        | 238 ++++++++++++++++++++++++++++++++++
 src/PVE/AccessControl.pm      |  18 +--
 src/PVE/Auth/Makefile         |   3 +-
 src/PVE/Auth/OpenId.pm        |  68 ++++++++++
 src/PVE/RPCEnvironment.pm     |  49 +++++++
 8 files changed, 378 insertions(+), 63 deletions(-)
 create mode 100644 src/PVE/API2/OpenId.pm
 create mode 100755 src/PVE/Auth/OpenId.pm

-- 
2.30.2

[pve-devel] [PATCH pve-access-control v2 1/5] check_user_enabled: also check if user is expired

[Dietmar Maurer]

---
 src/PVE/AccessControl.pm | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 2569a35..8628678 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -428,12 +428,10 @@ sub verify_token {
     check_user_enabled($usercfg, $username);
     check_token_exist($usercfg, $username, $token);
 
-    my $ctime = time();
-
     my $user = $usercfg->{users}->{$username};
-    die "account expired\n" if $user->{expire} && ($user->{expire} < $ctime);
-
     my $token_info = $user->{tokens}->{$token};
+
+    my $ctime = time();
     die "token expired\n" if $token_info->{expire} && ($token_info->{expire} < $ctime);
 
     die "invalid token value!\n" if !PVE::Cluster::verify_token($tokenid, $value);
@@ -579,6 +577,11 @@ sub check_user_enabled {
 
     die "user '$username' is disabled\n" if !$noerr;
 
+    my $ctime = time();
+    my $expire = $usercfg->{users}->{$username}->{expire};
+
+    die "account expired\n" if $expire && ($expire < $ctime);
+
     return undef;
 }
 
@@ -629,11 +632,6 @@ sub authenticate_user {
 
     check_user_enabled($usercfg, $username);
 
-    my $ctime = time();
-    my $expire = $usercfg->{users}->{$username}->{expire};
-
-    die "account expired\n" if $expire && ($expire < $ctime);
-
     my $domain_cfg = cfs_read_file('domains.cfg');
 
     my $cfg = $domain_cfg->{ids}->{$realm};
-- 
2.30.2

[pve-devel] [PATCH pve-access-control v2 2/5] add OpenId configuration

[Dietmar Maurer]

---
 src/PVE/AccessControl.pm |  2 ++
 src/PVE/Auth/Makefile    |  3 +-
 src/PVE/Auth/OpenId.pm   | 68 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 1 deletion(-)
 create mode 100755 src/PVE/Auth/OpenId.pm

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 8628678..3d8d01c 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -24,6 +24,7 @@ use PVE::Auth::AD;
 use PVE::Auth::LDAP;
 use PVE::Auth::PVE;
 use PVE::Auth::PAM;
+use PVE::Auth::OpenId;
 
 # load and initialize all plugins
 
@@ -31,6 +32,7 @@ PVE::Auth::AD->register();
 PVE::Auth::LDAP->register();
 PVE::Auth::PVE->register();
 PVE::Auth::PAM->register();
+PVE::Auth::OpenId->register();
 PVE::Auth::Plugin->init();
 
 # $authdir must be writable by root only!
diff --git a/src/PVE/Auth/Makefile b/src/PVE/Auth/Makefile
index 58ae362..be7bde3 100644
--- a/src/PVE/Auth/Makefile
+++ b/src/PVE/Auth/Makefile
@@ -4,7 +4,8 @@ AUTH_SOURCES= 		 	\
 	PVE.pm		 	\
 	PAM.pm		 	\
 	AD.pm		 	\
-	LDAP.pm
+	LDAP.pm			\
+	OpenId.pm
 
 .PHONY: install
 install:
diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
new file mode 100755
index 0000000..515d2f4
--- /dev/null
+++ b/src/PVE/Auth/OpenId.pm
@@ -0,0 +1,68 @@
+package PVE::Auth::OpenId;
+
+use strict;
+use warnings;
+
+use PVE::Tools;
+use PVE::Auth::Plugin;
+use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file);
+
+use base qw(PVE::Auth::Plugin);
+
+sub type {
+    return 'openid';
+}
+
+sub properties {
+    return {
+	"issuer-url" => {
+	    description => "OpenID Issuer Url",
+	    type => 'string',
+	    maxLength => 256,
+	},
+	"client-id" => {
+            description => "OpenID Client ID",
+            type => 'string',
+	    maxLength => 256,
+       },
+       "client-key" => {
+	    description => "OpenID Client Key",
+	    type => 'string',
+	    optional => 1,
+	    maxLength => 256,
+       },
+       autocreate => {
+	   description => "Automatically create users if they do not exist.",
+	   optional => 1,
+	   type => 'boolean',
+	   default => 0,
+       },
+       "username-claim" => {
+	   description => "OpenID claim used to generate the unique username.",
+	   type => 'string',
+	   enum => ['subject', 'username', 'email'],
+	   optional => 1,
+       },
+   };
+}
+
+sub options {
+    return {
+	"issuer-url" => {},
+	 "client-id" => {},
+	 "client-key" => { optional => 1 },
+	 autocreate => { optional => 1 },
+	 "username-claim" => { optional => 1, fixed => 1 },
+	 default => { optional => 1 },
+	 comment => { optional => 1 },
+    };
+}
+
+sub authenticate_user {
+    my ($class, $config, $realm, $username, $password) = @_;
+
+    die "OpenID realm does not allow password verification.\n";
+}
+
+
+1;
-- 
2.30.2

[pve-devel] [PATCH pve-access-control v2 3/5] depend on libpve-rs-perl

[Dietmar Maurer]

---
 debian/control | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/debian/control b/debian/control
index 81a32bd..3ef748b 100644
--- a/debian/control
+++ b/debian/control
@@ -10,6 +10,7 @@ Build-Depends: debhelper (>= 12~),
                lintian,
                perl,
                libpve-cluster-perl,
+	       libpve-rs-perl,
                pve-cluster (>= 6.1-4),
                pve-doc-generator (>= 5.3-3),
 Standards-Version: 4.5.1
@@ -27,6 +28,7 @@ Depends: libauthen-pam-perl,
          libnet-ssleay-perl,
          libpve-common-perl (>= 6.0-18),
          libpve-cluster-perl,
+	 libpve-rs-perl,
          libpve-u2f-server-perl (>= 1.0-2),
          libuuid-perl,
          pve-cluster (>= 6.1-4),
-- 
2.30.2

[pve-devel] [PATCH pve-access-control v2 4/5] api: implement openid API

[Dietmar Maurer]

This moves compute_api_permission() into RPCEnvironment.pm.
---
 src/PVE/API2/AccessControl.pm |  60 ++--------
 src/PVE/API2/Makefile         |   3 +-
 src/PVE/API2/OpenId.pm        | 211 ++++++++++++++++++++++++++++++++++
 src/PVE/RPCEnvironment.pm     |  49 ++++++++
 4 files changed, 270 insertions(+), 53 deletions(-)
 create mode 100644 src/PVE/API2/OpenId.pm

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index a77694b..6dec66c 100644
--- a/src/PVE/API2/AccessControl.pm
+++ b/src/PVE/API2/AccessControl.pm
@@ -19,9 +19,9 @@ use PVE::API2::User;
 use PVE::API2::Group;
 use PVE::API2::Role;
 use PVE::API2::ACL;
+use PVE::API2::OpenId;
 use PVE::Auth::Plugin;
 use PVE::OTP;
-use PVE::Tools;
 
 my $u2f_available = 0;
 eval {
@@ -56,6 +56,11 @@ __PACKAGE__->register_method ({
     path => 'domains',
 });
 
+__PACKAGE__->register_method ({
+    subclass => "PVE::API2::OpenId",
+    path => 'openid',
+});
+
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',
@@ -165,55 +170,6 @@ my $create_ticket = sub {
     };
 };
 
-my $compute_api_permission = sub {
-    my ($rpcenv, $authuser) = @_;
-
-    my $usercfg = $rpcenv->{user_cfg};
-
-    my $res = {};
-    my $priv_re_map = {
-	vms => qr/VM\.|Permissions\.Modify/,
-	access => qr/(User|Group)\.|Permissions\.Modify/,
-	storage => qr/Datastore\.|Permissions\.Modify/,
-	nodes => qr/Sys\.|Permissions\.Modify/,
-	sdn => qr/SDN\.|Permissions\.Modify/,
-	dc => qr/Sys\.Audit|SDN\./,
-    };
-    map { $res->{$_} = {} } keys %$priv_re_map;
-
-    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage', '/sdn'];
-
-    my $checked_paths = {};
-    foreach my $path (@$required_paths, keys %{$usercfg->{acl}}) {
-	next if $checked_paths->{$path};
-	$checked_paths->{$path} = 1;
-
-	my $path_perm = $rpcenv->permissions($authuser, $path);
-
-	my $toplevel = ($path =~ /^\/(\w+)/) ? $1 : 'dc';
-	if ($toplevel eq 'pool') {
-	    foreach my $priv (keys %$path_perm) {
-		if ($priv =~ m/^VM\./) {
-		    $res->{vms}->{$priv} = 1;
-		} elsif ($priv =~ m/^Datastore\./) {
-		    $res->{storage}->{$priv} = 1;
-		} elsif ($priv eq 'Permissions.Modify') {
-		    $res->{storage}->{$priv} = 1;
-		    $res->{vms}->{$priv} = 1;
-		}
-	    }
-	} else {
-	    my $priv_regex = $priv_re_map->{$toplevel} // next;
-	    foreach my $priv (keys %$path_perm) {
-		next if $priv !~ m/^($priv_regex)/;
-		$res->{$toplevel}->{$priv} = 1;
-	    }
-	}
-    }
-
-    return $res;
-};
-
 __PACKAGE__->register_method ({
     name => 'get_ticket',
     path => 'ticket',
@@ -314,7 +270,7 @@ __PACKAGE__->register_method ({
 	    die PVE::Exception->new("authentication failure\n", code => 401);
 	}
 
-	$res->{cap} = &$compute_api_permission($rpcenv, $username)
+	$res->{cap} = $rpcenv->compute_api_permission($username)
 	    if !defined($res->{NeedTFA});
 
 	my $clinfo = PVE::Cluster::get_clinfo();
@@ -659,7 +615,7 @@ __PACKAGE__->register_method({
 
 	return {
 	    ticket => PVE::AccessControl::assemble_ticket($authuser),
-	    cap => &$compute_api_permission($rpcenv, $authuser),
+	    cap => $rpcenv->compute_api_permission($authuser),
 	}
     }});
 
diff --git a/src/PVE/API2/Makefile b/src/PVE/API2/Makefile
index 1bf8c05..4e49037 100644
--- a/src/PVE/API2/Makefile
+++ b/src/PVE/API2/Makefile
@@ -5,7 +5,8 @@ API2_SOURCES= 		 	\
 	ACL.pm		 	\
 	Role.pm		 	\
 	Group.pm	 	\
-	User.pm
+	User.pm			\
+	OpenId.pm
 
 .PHONY: install
 install:
diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
new file mode 100644
index 0000000..d0b29fc
--- /dev/null
+++ b/src/PVE/API2/OpenId.pm
@@ -0,0 +1,211 @@
+package PVE::API2::OpenId;
+
+use strict;
+use warnings;
+
+use PVE::Tools qw(extract_param);
+use PVE::RS::OpenId;
+
+use PVE::Exception qw(raise raise_perm_exc raise_param_exc);
+use PVE::SafeSyslog;
+use PVE::RPCEnvironment;
+use PVE::Cluster qw(cfs_read_file);
+use PVE::AccessControl;
+use PVE::JSONSchema qw(get_standard_option);
+
+use PVE::RESTHandler;
+
+use base qw(PVE::RESTHandler);
+
+my $openid_state_path = "/var/lib/pve-manager";
+
+my $lookup_openid_auth = sub {
+    my ($realm, $redirect_url) = @_;
+
+    my $cfg = cfs_read_file('domains.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} != openid)\n" if $config->{type} ne "openid";
+
+    my $openid_config = {
+	issuer_url => $config->{'issuer-url'},
+	client_id => $config->{'client-id'},
+	client_key => $config->{'client-key'},
+    };
+
+    my $openid = PVE::RS::OpenId->discover($openid_config, $redirect_url);
+    return ($config, $openid);
+};
+
+__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 Authorization Url for the specified realm.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    realm => get_standard_option('realm'),
+	    '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, $openid) = $lookup_openid_auth->($realm, $redirect_url);
+	my $url = $openid->authorize_url($openid_state_path , $realm);
+
+	return $url;
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'login',
+    path => 'login',
+    method => 'POST',
+    protected => 1,
+    description => " Verify OpenID authorization code and create a ticket.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    'state' => {
+		description => "OpenId state.",
+		type => 'string',
+		maxLength => 1024,
+            },
+	    code => {
+		description => "OpenId authorization code.",
+		type => 'string',
+		maxLength => 1024,
+            },
+	    'redirect-url' => {
+		description => "Redirection Url. The client should set this to the used server url (location.origin).",
+		type => 'string',
+		maxLength => 255,
+	    },
+	},
+    },
+    returns => {
+	properties => {
+	    username => { type => 'string' },
+	    ticket => { type => 'string' },
+	    CSRFPreventionToken => { type => 'string' },
+	    cap => { type => 'object' },  # computed api permissions
+	    clustername => { type => 'string', optional => 1 },
+	},
+    },
+    permissions => { user => 'world' },
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+
+	my $res;
+	eval {
+	    my ($realm, $private_auth_state) = PVE::RS::OpenId::verify_public_auth_state(
+		$openid_state_path, $param->{'state'});
+
+	    my $redirect_url = extract_param($param, 'redirect-url');
+
+	    my ($config, $openid) = $lookup_openid_auth->($realm, $redirect_url);
+
+	    my $info = $openid->verify_authorization_code($param->{code}, $private_auth_state);
+	    my $subject = $info->{'sub'};
+
+	    die "missing openid claim 'sub'\n" if !defined($subject);
+
+	    my $unique_name = $subject; # default
+	    if (defined(my $user_attr = $config->{'user-attr'})) {
+		if ($user_attr eq 'subject') {
+		    $unique_name = $subject;
+		} elsif ($user_attr eq 'username') {
+		    my $username = $info->{'preferred_username'};
+		    die "missing claim 'preferred_username'\n" if !defined($username);
+		    $unique_name =  $username;
+		} elsif ($user_attr eq 'email') {
+		    my $email = $info->{'email'};
+		    die "missing claim 'email'\n" if !defined($email);
+		    $unique_name = $email;
+		} else {
+		    die "got unexpected value for user_attr '${user_attr}'\n";
+		}
+	    }
+
+	    my $username = "${unique_name}\@${realm}";
+
+	    # test if user exists and is enabled
+	    $rpcenv->check_user_enabled($username);
+
+	    my $ticket = PVE::AccessControl::assemble_ticket($username);
+	    my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
+	    my $cap = $rpcenv->compute_api_permission($username);
+
+	    $res = {
+		ticket => $ticket,
+		username => $username,
+		CSRFPreventionToken => $csrftoken,
+		cap => $cap,
+	    };
+
+	    my $clinfo = PVE::Cluster::get_clinfo();
+	    if ($clinfo->{cluster}->{name} && $rpcenv->check($username, '/', ['Sys.Audit'], 1)) {
+		$res->{clustername} = $clinfo->{cluster}->{name};
+	    }
+	};
+	if (my $err = $@) {
+	    my $clientip = $rpcenv->get_client_ip() || '';
+	    syslog('err', "openid authentication failure; rhost=$clientip msg=$err");
+	    # do not return any info to prevent user enumeration attacks
+	    die PVE::Exception->new("authentication failure\n", code => 401);
+	}
+
+	PVE::Cluster::log_msg('info', 'root@pam', "successful openid auth for user '$res->{username}'");
+
+	return $res;
+    }});
diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index 2e5a33b..8aae094 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -126,6 +126,55 @@ sub permissions {
     return &$compile_acl_path($self, $user, $path);
 }
 
+sub compute_api_permission {
+    my ($self, $authuser) = @_;
+
+    my $usercfg = $self->{user_cfg};
+
+    my $res = {};
+    my $priv_re_map = {
+	vms => qr/VM\.|Permissions\.Modify/,
+	access => qr/(User|Group)\.|Permissions\.Modify/,
+	storage => qr/Datastore\.|Permissions\.Modify/,
+	nodes => qr/Sys\.|Permissions\.Modify/,
+	sdn => qr/SDN\.|Permissions\.Modify/,
+	dc => qr/Sys\.Audit|SDN\./,
+    };
+    map { $res->{$_} = {} } keys %$priv_re_map;
+
+    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage', '/sdn'];
+
+    my $checked_paths = {};
+    foreach my $path (@$required_paths, keys %{$usercfg->{acl}}) {
+	next if $checked_paths->{$path};
+	$checked_paths->{$path} = 1;
+
+	my $path_perm = $self->permissions($authuser, $path);
+
+	my $toplevel = ($path =~ /^\/(\w+)/) ? $1 : 'dc';
+	if ($toplevel eq 'pool') {
+	    foreach my $priv (keys %$path_perm) {
+		if ($priv =~ m/^VM\./) {
+		    $res->{vms}->{$priv} = 1;
+		} elsif ($priv =~ m/^Datastore\./) {
+		    $res->{storage}->{$priv} = 1;
+		} elsif ($priv eq 'Permissions.Modify') {
+		    $res->{storage}->{$priv} = 1;
+		    $res->{vms}->{$priv} = 1;
+		}
+	    }
+	} else {
+	    my $priv_regex = $priv_re_map->{$toplevel} // next;
+	    foreach my $priv (keys %$path_perm) {
+		next if $priv !~ m/^($priv_regex)/;
+		$res->{$toplevel}->{$priv} = 1;
+	    }
+	}
+    }
+
+    return $res;
+}
+
 sub get_effective_permissions {
     my ($self, $user) = @_;
 
-- 
2.30.2

[pve-devel] [PATCH pve-access-control v2 5/5] implement OpenID autocreate user feature

[Dietmar Maurer]

---
 src/PVE/API2/OpenId.pm | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
index d0b29fc..8384729 100644
--- a/src/PVE/API2/OpenId.pm
+++ b/src/PVE/API2/OpenId.pm
@@ -9,9 +9,10 @@ use PVE::RS::OpenId;
 use PVE::Exception qw(raise raise_perm_exc raise_param_exc);
 use PVE::SafeSyslog;
 use PVE::RPCEnvironment;
-use PVE::Cluster qw(cfs_read_file);
+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::RESTHandler;
 
@@ -161,7 +162,7 @@ __PACKAGE__->register_method ({
 	    die "missing openid claim 'sub'\n" if !defined($subject);
 
 	    my $unique_name = $subject; # default
-	    if (defined(my $user_attr = $config->{'user-attr'})) {
+	    if (defined(my $user_attr = $config->{'username-claim'})) {
 		if ($user_attr eq 'subject') {
 		    $unique_name = $subject;
 		} elsif ($user_attr eq 'username') {
@@ -179,8 +180,34 @@ __PACKAGE__->register_method ({
 
 	    my $username = "${unique_name}\@${realm}";
 
-	    # test if user exists and is enabled
-	    $rpcenv->check_user_enabled($username);
+	    # first, check if $username respects our naming conventions
+	    PVE::Auth::Plugin::verify_username($username);
+
+	    if ($config->{'autocreate'} && !$rpcenv->check_user_exist($username, 1)) {
+		PVE::AccessControl::lock_user_config(sub {
+		    my $usercfg = cfs_read_file("user.cfg");
+
+		    die "user '$username' already exists\n" if $usercfg->{users}->{$username};
+
+		    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;
+		    }
+
+		    $usercfg->{users}->{$username} = $entry;
+
+		    cfs_write_file("user.cfg", $usercfg);
+		}, "autocreate openid user failed");
+	    } else {
+		# test if user exists and is enabled
+		$rpcenv->check_user_enabled($username);
+	    }
 
 	    my $ticket = PVE::AccessControl::assemble_ticket($username);
 	    my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
-- 
2.30.2

Re: [pve-devel] [PATCH pve-access-control v2 1/5] check_user_enabled: also check if user is expired

[Fabian Grünbichler]

the check is cheap, so it does not matter much that it happens in more 
places now (like for every request with a ticket in addition to the old 
every request with a token).

would have been nice to mention whether this is intentional though ;)

On June 30, 2021 8:10 am, Dietmar Maurer wrote:
> ---
>  src/PVE/AccessControl.pm | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 2569a35..8628678 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -428,12 +428,10 @@ sub verify_token {
>      check_user_enabled($usercfg, $username);
>      check_token_exist($usercfg, $username, $token);
>  
> -    my $ctime = time();
> -
>      my $user = $usercfg->{users}->{$username};
> -    die "account expired\n" if $user->{expire} && ($user->{expire} < $ctime);
> -
>      my $token_info = $user->{tokens}->{$token};
> +
> +    my $ctime = time();
>      die "token expired\n" if $token_info->{expire} && ($token_info->{expire} < $ctime);
>  
>      die "invalid token value!\n" if !PVE::Cluster::verify_token($tokenid, $value);
> @@ -579,6 +577,11 @@ sub check_user_enabled {
>  
>      die "user '$username' is disabled\n" if !$noerr;
>  
> +    my $ctime = time();
> +    my $expire = $usercfg->{users}->{$username}->{expire};
> +
> +    die "account expired\n" if $expire && ($expire < $ctime);
> +
>      return undef;
>  }
>  
> @@ -629,11 +632,6 @@ sub authenticate_user {
>  
>      check_user_enabled($usercfg, $username);
>  
> -    my $ctime = time();
> -    my $expire = $usercfg->{users}->{$username}->{expire};
> -
> -    die "account expired\n" if $expire && ($expire < $ctime);
> -
>      my $domain_cfg = cfs_read_file('domains.cfg');
>  
>      my $cfg = $domain_cfg->{ids}->{$realm};
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] applied: [PATCH pve-access-control v2 0/5] add OpenId realms

[Thomas Lamprecht]

On 30.06.21 08:10, Dietmar Maurer wrote:
> Changes in v2:
> - also check if user is expired (in check_user_enabled)
> - always die with newline
> - rename "user-attr" to "username-claim"
> 
> 
> Dietmar Maurer (5):
>   check_user_enabled: also check if user is expired
>   add OpenId configuration
>   depend on libpve-rs-perl
>   api: implement openid API
>   implement OpenID autocreate user feature
> 
>  debian/control                |   2 +
>  src/PVE/API2/AccessControl.pm |  60 ++-------
>  src/PVE/API2/Makefile         |   3 +-
>  src/PVE/API2/OpenId.pm        | 238 ++++++++++++++++++++++++++++++++++
>  src/PVE/AccessControl.pm      |  18 +--
>  src/PVE/Auth/Makefile         |   3 +-
>  src/PVE/Auth/OpenId.pm        |  68 ++++++++++
>  src/PVE/RPCEnvironment.pm     |  49 +++++++
>  8 files changed, 378 insertions(+), 63 deletions(-)
>  create mode 100644 src/PVE/API2/OpenId.pm
>  create mode 100755 src/PVE/Auth/OpenId.pm
> 



applied series, thanks!

But I moved the second hunk:
-	    if (defined(my $user_attr = $config->{'user-attr'})) {
+	    if (defined(my $user_attr = $config->{'username-claim'})) {

of patch 5/5 ("implement OpenID autocreate user feature") into the previous 
patch 4/5 ("api: implement openid API") and adapted the error message in
that if/else chain, as it still talked about "user_attrs" from v1.