all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH access-control 1/2] tfa: when modifying others, verify the current user's password
@ 2021-12-06 13:36 Wolfgang Bumiller
  2021-12-06 13:36 ` [pve-devel] [PATCH access-control 2/2] tfa list: account for admin permissions Wolfgang Bumiller
  2021-12-06 14:06 ` [pve-devel] [PATCH access-control 1/2] tfa: when modifying others, verify the current user's password Fabian Grünbichler
  0 siblings, 2 replies; 3+ messages in thread
From: Wolfgang Bumiller @ 2021-12-06 13:36 UTC (permalink / raw)
  To: pve-devel

this was wrong as it asked for the password of the
to-be-edited user instead, which makes no sense

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 src/PVE/API2/TFA.pm | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
index be696e1..343374e 100644
--- a/src/PVE/API2/TFA.pm
+++ b/src/PVE/API2/TFA.pm
@@ -101,7 +101,7 @@ my $TFA_UPDATE_INFO_SCHEMA = {
 my sub root_permission_check : prototype($$$$) {
     my ($rpcenv, $authuser, $userid, $password) = @_;
 
-    ($userid, my $ruid, my $realm) = PVE::AccessControl::verify_username($userid);
+    ($userid, undef, my $realm) = PVE::AccessControl::verify_username($userid);
     $rpcenv->check_user_exist($userid);
 
     raise_perm_exc() if $userid eq 'root@pam' && $authuser ne 'root@pam';
@@ -111,11 +111,13 @@ my sub root_permission_check : prototype($$$$) {
 	raise_param_exc({ 'password' => 'password is required to modify TFA data' })
 	    if !defined($password);
 
+	($authuser, my $ruid, my $auth_realm) = PVE::AccessControl::verify_username($authuser);
+
 	my $domain_cfg = cfs_read_file('domains.cfg');
-	my $cfg = $domain_cfg->{ids}->{$realm};
-	die "auth domain '$realm' does not exist\n" if !$cfg;
+	my $cfg = $domain_cfg->{ids}->{$auth_realm};
+	die "auth domain '$auth_realm' does not exist\n" if !$cfg;
 	my $plugin = PVE::Auth::Plugin->lookup($cfg->{type});
-	$plugin->authenticate_user($cfg, $realm, $ruid, $password);
+	$plugin->authenticate_user($cfg, $auth_realm, $authuser, $password);
     }
 
     return wantarray ? ($userid, $realm) : $userid;
-- 
2.30.2





^ permalink raw reply	[flat|nested] 3+ messages in thread

* [pve-devel] [PATCH access-control 2/2] tfa list: account for admin permissions
  2021-12-06 13:36 [pve-devel] [PATCH access-control 1/2] tfa: when modifying others, verify the current user's password Wolfgang Bumiller
@ 2021-12-06 13:36 ` Wolfgang Bumiller
  2021-12-06 14:06 ` [pve-devel] [PATCH access-control 1/2] tfa: when modifying others, verify the current user's password Fabian Grünbichler
  1 sibling, 0 replies; 3+ messages in thread
From: Wolfgang Bumiller @ 2021-12-06 13:36 UTC (permalink / raw)
  To: pve-devel

instead of restricting listing tfa entries of others to
root@pam, perform the same checks the user-list does and
which also reflect the permissions of the api calls actually
operating on those users, so, `User.Modify` on the user (but
also `Sys.Audit`, since it's only a read-operation, just
like the user index API call)

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 src/PVE/API2/TFA.pm | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
index 343374e..123aeba 100644
--- a/src/PVE/API2/TFA.pm
+++ b/src/PVE/API2/TFA.pm
@@ -373,10 +373,24 @@ __PACKAGE__->register_method ({
 
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $authuser = $rpcenv->get_user();
-	my $top_level_allowed = ($authuser eq 'root@pam');
 
 	my $tfa_cfg = cfs_read_file('priv/tfa.cfg');
-	return $tfa_cfg->api_list_tfa($authuser, $top_level_allowed);
+	my $entries = $tfa_cfg->api_list_tfa($authuser, 1);
+
+	my $privs = [ 'User.Modify', 'Sys.Audit' ];
+	if ($rpcenv->check_any($authuser, "/access/groups", $privs, 1)) {
+	    # can modify all
+	    return $entries;
+	}
+
+	my $groups = $rpcenv->filter_groups($authuser, $privs, 1);
+	my $allowed_users = $rpcenv->group_member_join([keys %$groups]);
+	return [
+	    grep {
+		my $userid = $_->{userid};
+		$userid eq $authuser || $allowed_users->{$userid}
+	    } $entries->@*
+	];
     }});
 
 __PACKAGE__->register_method ({
-- 
2.30.2





^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH access-control 1/2] tfa: when modifying others, verify the current user's password
  2021-12-06 13:36 [pve-devel] [PATCH access-control 1/2] tfa: when modifying others, verify the current user's password Wolfgang Bumiller
  2021-12-06 13:36 ` [pve-devel] [PATCH access-control 2/2] tfa list: account for admin permissions Wolfgang Bumiller
@ 2021-12-06 14:06 ` Fabian Grünbichler
  1 sibling, 0 replies; 3+ messages in thread
From: Fabian Grünbichler @ 2021-12-06 14:06 UTC (permalink / raw)
  To: Proxmox VE development discussion; +Cc: w.bumiller

doesn't work for me

On December 6, 2021 2:36 pm, Wolfgang Bumiller wrote:
> this was wrong as it asked for the password of the
> to-be-edited user instead, which makes no sense
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
>  src/PVE/API2/TFA.pm | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
> index be696e1..343374e 100644
> --- a/src/PVE/API2/TFA.pm
> +++ b/src/PVE/API2/TFA.pm
> @@ -101,7 +101,7 @@ my $TFA_UPDATE_INFO_SCHEMA = {
>  my sub root_permission_check : prototype($$$$) {
>      my ($rpcenv, $authuser, $userid, $password) = @_;
>  
> -    ($userid, my $ruid, my $realm) = PVE::AccessControl::verify_username($userid);
> +    ($userid, undef, my $realm) = PVE::AccessControl::verify_username($userid);
>      $rpcenv->check_user_exist($userid);
>  
>      raise_perm_exc() if $userid eq 'root@pam' && $authuser ne 'root@pam';
> @@ -111,11 +111,13 @@ my sub root_permission_check : prototype($$$$) {
>  	raise_param_exc({ 'password' => 'password is required to modify TFA data' })
>  	    if !defined($password);
>  
> +	($authuser, my $ruid, my $auth_realm) = PVE::AccessControl::verify_username($authuser);
> +
>  	my $domain_cfg = cfs_read_file('domains.cfg');
> -	my $cfg = $domain_cfg->{ids}->{$realm};
> -	die "auth domain '$realm' does not exist\n" if !$cfg;
> +	my $cfg = $domain_cfg->{ids}->{$auth_realm};
> +	die "auth domain '$auth_realm' does not exist\n" if !$cfg;
>  	my $plugin = PVE::Auth::Plugin->lookup($cfg->{type});
> -	$plugin->authenticate_user($cfg, $realm, $ruid, $password);
> +	$plugin->authenticate_user($cfg, $auth_realm, $authuser, $password);

this should likely still be $ruid? (which is actually just the username 
without realm, which is what authenticate_user expects. so maybe 
$username might also be a more readable variable name - ruid reads to me 
like euid and friends)

>      }
>  
>      return wantarray ? ($userid, $realm) : $userid;
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-12-06 14:07 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-06 13:36 [pve-devel] [PATCH access-control 1/2] tfa: when modifying others, verify the current user's password Wolfgang Bumiller
2021-12-06 13:36 ` [pve-devel] [PATCH access-control 2/2] tfa list: account for admin permissions Wolfgang Bumiller
2021-12-06 14:06 ` [pve-devel] [PATCH access-control 1/2] tfa: when modifying others, verify the current user's password Fabian Grünbichler

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal