Re: [pve-devel] [PATCH v1 access-control 2/5] tfa: allow superusers to edit root@pam tfa

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

On February 8, 2022 2:10 pm, Oguz Bektas wrote:
> users with the SU privilege are able to override the existing check for
> 'root@pam' when calling tfa-related endpoints of the API.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  src/PVE/API2/TFA.pm | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/TFA.pm b/src/PVE/API2/TFA.pm
> index bee4dee..70555cc 100644
> --- a/src/PVE/API2/TFA.pm
> +++ b/src/PVE/API2/TFA.pm
> @@ -102,13 +102,16 @@ my $TFA_UPDATE_INFO_SCHEMA = {
>  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);
> +    raise_perm_exc() if $userid eq 'root@pam' && !$is_superuser; # root@pam can be only edited by himself or a SuperUser

can be shorted (root@pam is by definition a SuperUser), but IMHO, the 
whole comment can be dropped - the variables have meaningful names 
anyway?

some rationale why this was changed, but the very similar code in 
PVE:API2::AccessControl->change_password is not adapted is also missing. 

also, a similar mechanism exists in the other direction as well: 
root@pam can create tickets for other users - should SuperUser also be 
allowed to do that? if not, the reason for that exception should be 
added to the relevant part as comment (or if it should, then the 
relevant changes should be part of the series ;))

one more point - check_api2_permissions has the general 'no permission 
specified in schema => only root@pam allowed' rule. obviously with the 
semantics of 'superuser does not entail all privileges' that can't be 
switched to superuser - but it means all the existing API calls without 
specific permissions need to be checked and either adapted (e.g., with 
an appropriate ACL with regular privilege + SuperUser), or marked as 
root only with a reason. one way to do this would be to analyze the API 
dump, or even forbid endpoints without a permission in the schema (e.g. 
in the api verification code we run at build time), and switch the 
remaining root-only endpoints to just have an explicit permission in the 
schema that encodes that. obviously in any case the check in 
check_api2_permissions needs to remain to catch anything that slipped 
through.

>  
>      # 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
> 
> 
>