Re: [pve-devel] [PATCH v2 manager 04/12] api: backup: allow SUs to use 'tmpdir', 'dumpdir' and 'script' options

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

On March 11, 2022 12:24 pm, Oguz Bektas wrote:
> previously limited to root@pam; we can allow SUs to use these options if
> they have the privilege on the whole API path.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  PVE/API2/Backup.pm | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index 9953a704..142eddd1 100644
> --- a/PVE/API2/Backup.pm
> +++ b/PVE/API2/Backup.pm
> @@ -43,8 +43,13 @@ my $assert_param_permission = sub {
>      my ($param, $user) = @_;
>      return if $user eq 'root@pam'; # always OK
>  
> +    my $rpcenv = PVE::RPCEnvironment::get();
> +    # we need to have SU privs on / path for these options to be used safely

that comment is bogus - having some privileges doesn't make this 
options 'safe', it's the other way round - we require this privilege 
because the options are not safe. the check call itself and the messages 
below are already very clear, I don't think we need a comment here at 
all? but if you think we do, a simple 

# SuperUser always OK

like for root above would suffice..

> +    my $is_superuser = $rpcenv->check($user, "/", ['SuperUser'], 1);
> +    return if $is_superuser;

return if $rpcenv...

> +
>      for my $key (qw(tmpdir dumpdir script)) {
> -	raise_param_exc({ $key => "Only root may set this option."}) if exists $param->{$key};
> +	raise_param_exc({ $key => "Only superusers may set this option."}) if exists $param->{$key};
>      }
>  };
>  
> @@ -142,7 +147,7 @@ __PACKAGE__->register_method({
>      description => "Create new vzdump backup job.",
>      permissions => {
>  	check => ['perm', '/', ['Sys.Modify']],
> -	description => "The 'tmpdir', 'dumpdir' and 'script' parameters are additionally restricted to the 'root\@pam' user.",
> +	description => "The 'tmpdir', 'dumpdir' and 'script' parameters are additionally restricted to superusers.",
>      },
>      parameters => {
>      	additionalProperties => 0,
> @@ -344,7 +349,7 @@ __PACKAGE__->register_method({
>      description => "Update vzdump backup job definition.",
>      permissions => {
>  	check => ['perm', '/', ['Sys.Modify']],
> -	description => "The 'tmpdir', 'dumpdir' and 'script' parameters are additionally restricted to the 'root\@pam' user.",
> +	description => "The 'tmpdir', 'dumpdir' and 'script' parameters are additionally restricted to superusers.",
>      },
>      parameters => {
>      	additionalProperties => 0,
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>