Re: [pve-devel] [PATCH v4 access-control 05/18] api: allow superusers to edit tfa and password settings

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

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> - prevent non-SU to change SU passwords
> - warning messages on raise_perm_exc()
> - log who did the password change
> - has_superuser_anywhere helper
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/API2/AccessControl.pm | 24 ++++++++++++++----------
>  src/PVE/API2/TFA.pm           | 16 +++++++++++++++-
>  src/PVE/RPCEnvironment.pm     | 15 +++++++++++++++
>  3 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
> index 5d78c6f..2a584ab 100644
> --- a/src/PVE/API2/AccessControl.pm
> +++ b/src/PVE/API2/AccessControl.pm
> @@ -378,23 +378,27 @@ __PACKAGE__->register_method ({
>  
>  	$rpcenv->check_user_exist($userid);
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/access", ['SuperUser'], 1);
> +
>  	if ($authuser eq 'root@pam') {
>  	    # OK - root can change anything
> -	} else {
> -	    if ($authuser eq $userid) {
> -		$rpcenv->check_user_enabled($userid);
> -		# OK - each user can change its own password
> -	    } else {
> -		# only root may change root password
> -		raise_perm_exc() if $userid eq 'root@pam';
> -		# do not allow to change system user passwords
> -		raise_perm_exc() if $realm eq 'pam';
> +	} elsif ($authuser eq $userid) {
> +	    $rpcenv->check_user_enabled($userid);
> +	    # OK - each user can change its own password
> +	} else { # changing someone else's password
> +	    raise_perm_exc("only root\@pam may change their password!\n") if $userid eq 'root@pam';
> +	    raise_perm_exc("changing system user passwords is not allowed!\n") if $realm eq 'pam';
> +
> +	    if (!$is_superuser) {
> +		# check if the target user has SU privileges
> +		raise_perm_exc("only superusers can change another superuser's password!\n")
> +		    if $rpcenv->has_superuser_anywhere($userid);
>  	    }
>  	}
>  
>  	PVE::AccessControl::domain_set_password($realm, $ruid, $param->{password});
>  
> -	PVE::Cluster::log_msg('info', 'root@pam', "changed password for user '$userid'");
> +	PVE::Cluster::log_msg('info', "$authuser", "changed password for user '$userid'");
>  
>  	return undef;
>      }});
> diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
> index bee4dee..c1cdd5e 100644
> --- a/src/PVE/API2/TFA.pm
> +++ b/src/PVE/API2/TFA.pm
> @@ -96,22 +96,36 @@ my $TFA_UPDATE_INFO_SCHEMA = {
>  };
>  
>  # Only root may modify root, regular users need to specify their password.
> +# Only users with SU on /access may modify other users with SU anywhere
>  #
>  # Returns the userid returned from `verify_username`.
>  # Or ($userid, $realm) in list context.
>  my sub root_permission_check : prototype($$$$) {
>      my ($rpcenv, $authuser, $userid, $password) = @_;
>  
> +    # authuser = the user making the change
> +    # userid = the user to be changed
> +
> +    if ($userid eq 'root@pam') {
> +	raise_perm_exc("only root\@pam may edit themselves!\n")
> +	    if $authuser ne 'root@pam';
> +    }

if ($foo && $bar) {
  baz();
}

or

baz() if $foo && $bar;

also, the escaping can be avoided if you just quote with '' instead of 
"" in the message.

> +
>      ($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';
> +    my $is_superuser = $rpcenv->check($authuser, "/access", ['SuperUser'], 1);

only used once, inside the next if, please move it there (I told you 
this already?)

>  
>      # Regular users need to confirm their password to change TFA settings.
>      if ($authuser ne 'root@pam') {
>  	raise_param_exc({ 'password' => 'password is required to modify TFA data' })
>  	    if !defined($password);
>  
> +	if (!$is_superuser && $authuser ne $userid) {
> +	    raise_perm_exc("only superusers can change another superuser's TFA settings!\n")
> +		if $rpcenv->has_superuser_anywhere($userid);

could actually be ordered by order of expensiveness here, but it's not 
too important..

if ($authuser ne $userid) {
  my $is_superuser = ...;
  raise_perm_exc..
    if !is_superuser && $rpcenv->has_superuser_anywhere(..);
}

or

raise_perm_exc
  if user_mismatch_check
    && isnt_superuser_check
    && has_superuser_check;

that way each check is only actually done if needed, after the previous 
check made us go down further. the current way is just hard to parse 
with no benefit at all.

> +	}
> +
>  	($authuser, my $auth_username, my $auth_realm) =
>  	    PVE::AccessControl::verify_username($authuser);

the whole code here is rather similar to the check in change_password, 
have you thought about unifying them?

>  
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 4c55b25..5bcb4ba 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -479,6 +479,21 @@ sub check_api2_permissions {
>      raise_perm_exc();
>  }
>  
> +sub has_superuser_anywhere {
> +    my ($self, $username) = @_;
> +
> +    return 1 if $username eq 'root@pam';
> +
> +    # get all ACL paths from config and check for SU privilege
> +    my $acls = $self->{user_cfg}->{acl};
> +    my @acl_paths = keys(%$acls);
> +    @acl_paths = ['/'] if !@acl_paths;
> +    foreach my $path (@acl_paths) {
> +	return 1 if $self->check($username, $path, ['SuperUser'], 1);
> +    }
> +    return 0;
> +}

this is basically a reduced $rpcenv->get_effective_permissions with 
early abort. some possible options:
- extend it (add a '$needle' parameter, early return 1 if any path 
  matches that check)
- just use it (and drop the early abort)
- factor out the "get all ACL paths" part for re-using

else we risk missing this helper here if we ever add another layer of 
indirection for ACL path resolution, like we currently have for pools..

> +
>  sub log_cluster_msg {
>      my ($self, $pri, $user, $msg) = @_;
>  
> -- 
> 2.30.2
> 
>