Re: [pve-devel] [PATCH v4 qemu-server 1/1] api2: add check_bridge_access for create/update/clone/restore vm

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

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

but this might require a follow-up, see below.

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Qemu.pm | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..9e3a359 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -23,7 +23,7 @@ use PVE::Storage;
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::RESTHandler;
>  use PVE::ReplicationConfig;
> -use PVE::GuestHelpers qw(assert_tag_permissions);
> +use PVE::GuestHelpers qw(assert_tag_permissions check_vnet_access);
>  use PVE::QemuConfig;
>  use PVE::QemuServer;
>  use PVE::QemuServer::Cloudinit;
> @@ -601,6 +601,22 @@ my $check_vm_create_usb_perm = sub {
>      return 1;
>  };
>  
> +my $check_bridge_access = sub {
> +    my ($rpcenv, $authuser, $param) = @_;
> +
> +    return 1 if $authuser eq 'root@pam';
> +
> +    foreach my $opt (keys %{$param}) {
> +	next if $opt !~ m/^net\d+$/;
> +	my $net = PVE::QemuServer::parse_net($param->{$opt});
> +	my $bridge = $net->{bridge};
> +	my $tag = $net->{tag};
> +	my $trunks = $net->{trunks};
> +	check_vnet_access($rpcenv, $authuser, $bridge, $tag, $trunks);
> +    }
> +    return 1;
> +};
> +
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> @@ -728,7 +744,8 @@ __PACKAGE__->register_method({
>      permissions => {
>  	description => "You need 'VM.Allocate' permissions on /vms/{vmid} or on the VM pool /pool/{pool}. " .
>  	    "For restore (option 'archive'), it is enough if the user has 'VM.Backup' permission and the VM already exists. " .
> -	    "If you create disks you need 'Datastore.AllocateSpace' on any used storage.",
> +	    "If you create disks you need 'Datastore.AllocateSpace' on any used storage." .
> +	    "If you use a bridge/vlan, you need 'SDN.Use' on any used bridge/vlan.",
>          user => 'all', # check inside
>      },
>      protected => 1,
> @@ -865,6 +882,10 @@ __PACKAGE__->register_method({
>  		    'backup',
>  		);
>  
> +		my $vzdump_conf = PVE::Storage::extract_vzdump_config($storecfg, $archive);
> +		my $backup_conf = PVE::QemuServer::parse_vm_config("restore/qemu-server/$vmid.conf", $vzdump_conf, 1);
> +		&$check_bridge_access($rpcenv, $authuser, $backup_conf);
> +

this part here should maybe be moved somewhere where we already have the
extracted config, if possible?

>  		$archive = $parse_restore_archive->($storecfg, $archive);
>  	    }
>  	}
> @@ -878,7 +899,7 @@ __PACKAGE__->register_method({
>  
>  	    &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param);
>  	    &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
> -
> +	    &$check_bridge_access($rpcenv, $authuser, $param);
>  	    &$check_cpu_model_access($rpcenv, $authuser, $param);
>  
>  	    $check_drive_param->($param, $storecfg);
> @@ -1578,6 +1599,8 @@ my $update_vm_api  = sub {
>  
>      &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param);
>  
> +    &$check_bridge_access($rpcenv, $authuser, $param);
> +
>      my $updatefn =  sub {
>  
>  	my $conf = PVE::QemuConfig->load_config($vmid);
> @@ -3355,7 +3378,7 @@ __PACKAGE__->register_method({
>      permissions => {
>  	description => "You need 'VM.Clone' permissions on /vms/{vmid}, and 'VM.Allocate' permissions " .
>  	    "on /vms/{newid} (or on the VM pool /pool/{pool}). You also need " .
> -	    "'Datastore.AllocateSpace' on any used storage.",
> +	    "'Datastore.AllocateSpace' on any used storage and 'SDN.Use' on any used bridge/vnet",
>  	check =>
>  	[ 'and',
>  	  ['perm', '/vms/{vmid}', [ 'VM.Clone' ]],
> @@ -3489,6 +3512,8 @@ __PACKAGE__->register_method({
>  
>  	    my $sharedvm = &$check_storage_access_clone($rpcenv, $authuser, $storecfg, $oldconf, $storage);
>  
> +	    &$check_bridge_access($rpcenv, $authuser, $oldconf);
> +
>  	    die "can't clone VM to node '$target' (VM uses local storage)\n"
>  		if $target && !$sharedvm;
>  
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>