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

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

On June 5, 2023 1:37 am, Alexandre Derumier wrote:
> test first if user have access to the full zone (any bridge/vlan)
> if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl)
> if no tag, test if user have access to full bridge. (if trunks are defined, it need also access to full bridge)
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Qemu.pm | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 587bb22..4de7b32 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,34 @@ 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};
> +	    }
> +	}
> +	# test first if user have access to the full zone (any bridge/vlan)
> +	return 1 if $rpcenv->check_any($authuser, "/sdn/zones/$zone", ['SDN.Audit', 'SDN.Allocate'], 1);
> +	# if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl)
> +	return 1 if $tag && $rpcenv->check_any($authuser, "/sdn/vnets/$bridge/$tag", ['SDN.Audit', 'SDN.Allocate'], 1);

this one IMHO should be

$rpcenv->check($authuser, "/sdn/vnets/$bridge/$tag", ['SDN.Use']) if $tag;

see comment in cover letter and below.

> +	# if no tag, test if user have access to full bridge. (if trunks are defined, it need also access to full bridge)
> +	$rpcenv->check_any($authuser, "/sdn/vnets/$bridge", ['SDN.Audit', 'SDN.Allocate']);

trunks also allow (sub)ranges, in that case we could check for those
tags? or do we really need the full bridge?

these checks here should check for (a not yet existing) 'SDN.Use'
privilege, so that we can differentiate between:

- Audit: see the config, but neither change nor use its entries
- Use: use the entries
- Allocate: modify the entries

similar to Dominik's series for PCI/USB (where there is 'Modify' to
manage the entries, and 'Use' to reference them in guest configs).

I agree with Thomas that it likely makes sense to have all of that after
the parse_net call in a helper in pve-guest-common, like:

sub check_vnet_access {
  my ($rpcenv, $authuser, $vnet, $tag) = @_;

  ...
}

for code reuse with pve-container.

> +    }
> +    return 1;
> +};
> +
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> @@ -878,7 +912,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 +1612,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
> 
> 
>