Re: [pve-devel] [PATCH v2 access-control 11/12] api: allow superusers to edit tfa and password settings

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

On March 11, 2022 12:25 pm, Oguz Bektas wrote:
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> v1->v2:
> * also adapt change_password
> * didn't remove the comments in TFA.pm since it was still confusing without them
> 
>  src/PVE/API2/AccessControl.pm | 6 ++++++
>  src/PVE/API2/TFA.pm           | 7 +++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
> index 5d78c6f..1b471f6 100644
> --- a/src/PVE/API2/AccessControl.pm
> +++ b/src/PVE/API2/AccessControl.pm
> @@ -378,9 +378,15 @@ __PACKAGE__->register_method ({
>  
>  	$rpcenv->check_user_exist($userid);
>  
> +	# check for SU privs on user groups
> +	my $is_superuser = $rpcenv->check($authuser, "/access/groups", ['SuperUser'], 1);

I know this is modelled after userid-group, but it's a lot more 
restrictive? I'd either use full userid-group semantics, or just 
'/access' as path, the current one doesn't make much sense as a middle 
ground.. I think I'd prefer '/access', given the issue further below..

but also, this check here is with $noerr set.. (ctd)

> +
>  	if ($authuser eq 'root@pam') {
>  	    # OK - root can change anything
>  	} else {
> +	    if ($is_superuser && $userid ne 'root@pam') {
> +		# OK - superusers can change any user's password except root@pam
> +	    }

and here you add an empty `if` with a side-effect free condition as only 
place where the result of the check above is used.. so effectively, 
nothing about this API path changes at all? how did you test this? what 
was the expected change, "adapt change_password" doesn't really give me 
a clue ;)

one could argue that if a user is SU anywhere, changing their password 
should require SU as well (on '/access' ?) or be limited to the user 
itself, similar to the semantics of 'root@pam'..

the 'user is SU anywhere' part doesn't exist yet and is a bit tricky 
(handling of groups/..) or expensive (get list of all currently defined 
ACL paths, query actual permissions on all of them).

changing another user's password already requires fairly high privileges:
- realm != pam
- Realm.AllocateUser on realm
- User.Modify on user (i.e., either User.Modify on all groups or a group 
  the user belongs to, like the other user-modification API endpoints)

there obviously is a pit-fall there though with the last part (add a SU 
to some group without knowing about the implication that other users 
with User.Modify on that group can now modify the SU, including changing 
password if the other conditions are met).

>  	    if ($authuser eq $userid) {
>  		$rpcenv->check_user_enabled($userid);
>  		# OK - each user can change its own password
> diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
> index bee4dee..bab78ea 100644
> --- a/src/PVE/API2/TFA.pm
> +++ b/src/PVE/API2/TFA.pm
> @@ -102,13 +102,16 @@ my $TFA_UPDATE_INFO_SCHEMA = {

comment above this sub is now wrong..

>  my sub root_permission_check : prototype($$$$) {
>      my ($rpcenv, $authuser, $userid, $password) = @_;
>  
> +    # authuser = the user making the change
> +    # userid = the user to be changed
>      ($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 = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/access/groups", ['SuperUser'], 1);

same as above applies here as well w.r.t. the checked path / semantics.

> +    raise_perm_exc() if $userid eq 'root@pam' && !$is_superuser; # root@pam can be only edited by superusers

if we add a check for the other direction (special restriction of who 
can change a SU's password) above, the same semantics might make sense 
here (i.e., changing a SU's TFA settings requires SU as well).

>  
>      # Regular users need to confirm their password to change TFA settings.
> -    if ($authuser ne 'root@pam') {
> +    if ($authuser ne 'root@pam') { # we still do password confirmation for superusers like the other users
>  	raise_param_exc({ 'password' => 'password is required to modify TFA data' })
>  	    if !defined($password);
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>