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

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

a few more places that come to my mind that might warrant further
thinking or discussion:
- restoring a backup
- cloning a VM

On May 26, 2023 9:33 am, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..ebef93f 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -46,6 +46,12 @@ use PVE::SSHInfo;
>  use PVE::Replication;
>  use PVE::StorageTunnel;
>  
> +my $have_sdn;
> +eval {
> +    require PVE::Network::SDN;
> +    $have_sdn = 1;
> +};
> +
>  BEGIN {
>      if (!$ENV{PVE_GENERATING_DOCS}) {
>  	require PVE::HA::Env::PVE2;
> @@ -601,6 +607,33 @@ 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 $zone = 'local';
> +
> +	if ($have_sdn) {
> +	    my $vnet_cfg = PVE::Network::SDN::Vnets::config();
> +	    if (defined(my $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $bridge, 1))) {
> +		$zone = $vnet->{zone};
> +	    }
> +	}
> +	return 1 if $rpcenv->check_any($authuser, "/sdn/zones/$zone", ['SDN.Audit', 'SDN.Allocate'], 1);

why is this $noerr? should be a hard fail AFAICT! a, re-reading the
cover letter, this is intentional. might be worth a callout here in a
comment (and once this is finalized, in the docs ;))

> +	return 1 if $tag && $rpcenv->check_any($authuser, "/sdn/vnets/$bridge.$tag", ['SDN.Audit', 'SDN.Allocate'], 1);

same here, I think I'd prefer /sdn/vnets/$bridge/$tag if we go down that
route! then this would also be improved, since having access to the tag
also checks access to the bridge, and this could be turned into a hard
fail, which it should be!

what about trunking?

> +
> +	$rpcenv->check_any($authuser, "/sdn/vnets/$bridge", ['SDN.Audit', 'SDN.Allocate']);

for the whole block of checks here:

what are the semantics of Audit and Allocate here? do we need another
level between "sees bridge/vnet" and "can administer/reconfigure
bridge/vnet" that says "can use bridge/vnet"?

> +    }
> +    return 1;
> +};
> +
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> @@ -878,7 +911,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 +1611,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);
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>