[pve-devel] [PATCH v4 qemu-server 08/18] api: allow superusers to use 'skiplock' option

[Fabian Grünbichler]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> also mark the intentionally root-only migration related options
> in param descriptions and leave a reminder comment.

how are these changes related? please split them up into two patches (or 
merge the comment part into the other path that adds similar comments)

> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 71 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 2e75ab6..198e736 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -1337,8 +1337,8 @@ my $update_vm_api  = sub {
>      my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
>  
>      my $skiplock = extract_param($param, 'skiplock');
> -    raise_param_exc({ skiplock => "Only root may use this option." })
> -	if $skiplock && $authuser ne 'root at pam';
> +    raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	if $skiplock && !$is_superuser;
>  
>      my $delete_str = extract_param($param, 'delete');
>  
> @@ -1864,9 +1864,11 @@ __PACKAGE__->register_method({
>  	my $authuser = $rpcenv->get_user();
>  	my $vmid = $param->{vmid};
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = $param->{skiplock};
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root at pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $early_checks = sub {
>  	    # test if VM exists
> @@ -2474,25 +2476,27 @@ __PACKAGE__->register_method({
>  	    migration_type => {
>  		type => 'string',
>  		enum => ['secure', 'insecure'],
> -		description => "Migration traffic is encrypted using an SSH " .
> +		description => "Migration-internal parameter. Migration traffic is encrypted using an SSH " .
>  		  "tunnel by default. On secure, completely private networks " .
>  		  "this can be disabled to increase performance.",
>  		optional => 1,
>  	    },
>  	    migration_network => {
>  		type => 'string', format => 'CIDR',
> -		description => "CIDR of the (sub) network that is used for migration.",
> +		description => "Migration-internal parameter. CIDR of the (sub)network " .
> +		    "that is used for migration.",
>  		optional => 1,
>  	    },
>  	    machine => get_standard_option('pve-qemu-machine'),
>  	    'force-cpu' => {
> -		description => "Override QEMU's -cpu argument with the given string.",
> +		description => "Migration-internal parameter. Override QEMU's" .
> +		    "-cpu argument with the given string.",
>  		type => 'string',
>  		optional => 1,
>  	    },
>  	    targetstorage => get_standard_option('pve-targetstorage'),
>  	    timeout => {
> -		description => "Wait maximal timeout seconds.",
> +		description => "Migration-internal parameter. Wait maximal timeout seconds.",
>  		type => 'integer',
>  		minimum => 0,
>  		default => 'max(30, vm memory in GiB)',
> @@ -2514,6 +2518,14 @@ __PACKAGE__->register_method({
>  	my $timeout = extract_param($param, 'timeout');
>  	my $machine = extract_param($param, 'machine');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
> +	my $skiplock = extract_param($param, 'skiplock');
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
> +
> +	# since they are only used for migration-internal flows,
> +	# these parameters are still intentionally limited to root at pam
>  	my $get_root_param = sub {
>  	    my $value = extract_param($param, $_[0]);
>  	    raise_param_exc({ "$_[0]" => "Only root may use this option." })
> @@ -2522,7 +2534,6 @@ __PACKAGE__->register_method({
>  	};
>  
>  	my $stateuri = $get_root_param->('stateuri');
> -	my $skiplock = $get_root_param->('skiplock');
>  	my $migratedfrom = $get_root_param->('migratedfrom');
>  	my $migration_type = $get_root_param->('migration_type');
>  	my $migration_network = $get_root_param->('migration_network');
> @@ -2662,9 +2673,11 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root at pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $keepActive = extract_param($param, 'keepActive');
>  	raise_param_exc({ keepActive => "Only root may use this option." })
> @@ -2739,9 +2752,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root at pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
>  
> @@ -2806,9 +2821,11 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root at pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $keepActive = extract_param($param, 'keepActive');
>  	raise_param_exc({ keepActive => "Only root may use this option." })
> @@ -2965,9 +2982,11 @@ __PACKAGE__->register_method({
>  
>  	my $statestorage = extract_param($param, 'statestorage');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root at pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
>  
> @@ -3037,9 +3056,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root at pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $nocheck = extract_param($param, 'nocheck');
>  	raise_param_exc({ nocheck => "Only root may use this option." })
> @@ -3109,9 +3130,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root at pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	PVE::QemuServer::vm_sendkey($vmid, $skiplock, $param->{key});
>  
> @@ -4372,9 +4395,11 @@ __PACKAGE__->register_method({
>  
>  	my $sizestr = extract_param($param, 'size');
>  
> +	my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -        raise_param_exc({ skiplock => "Only root may use this option." })
> -            if $skiplock && $authuser ne 'root at pam';
> +        raise_param_exc({ skiplock => "Only superusers may use this option." })
> +            if $skiplock && !$is_superuser;
>  
>          my $storecfg = PVE::Storage::config();
>  
> -- 
> 2.30.2
> 
>