[pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access

[pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access

[Alexandre Derumier]

For proxmox 8, following the pve-manager patch serie
https://lists.proxmox.com/pipermail/pve-devel/2023-May/056970.html

This patch serie add check of permissions for bridge/vnets access
(currently only at vm create/update, I'm note sureif they are other
places where it should be added)

if user have access to a zone, it have access to all vnets + vnet vlans
if user have access to a vnet, it have access to the vnet + vnet vlans
if user have access to a specific vnet+vlan, it have access to the vlan only

Alexandre Derumier (1):
  api2: add check_bridge_access for create/update vm

 PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

-- 
2.30.2

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

[Alexandre Derumier]

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

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

[Fabian Grünbichler]

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
> 
> 
> 

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

[DERUMIER, Alexandre]

Le vendredi 02 juin 2023 à 13:43 +0200, Fabian Grünbichler a écrit :
> a few more places that come to my mind that might warrant further
> thinking or discussion:
> - restoring a backup
doesn't it also use create_vm ?

__PACKAGE__->register_method({
    name => 'create_vm',
    path => '',
    method => 'POST',
    description => "Create or restore a virtual machine.",


> - cloning a VM

for cloning, we can't change the target bridge, so if user have access
to the clone, isn't it enough ?



> 
> 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 ;))
yes, because we need to check later if we have permissoins on
bridge/tag

> 
> > +       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!
> 
I had tried this way, I don't remember, I think I had a problem
somewhere,
but I'll retry it again, as it's cleaner indeed.


> what about trunking?
> 
not implemented indeed, but I think we could compare the trunk vlans
list to the vlan path permissions.

Or maybe we just want that user have full bridge permissions,to defined
the any vlans in the trunk ?


> > +
> > +       $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"?

mmmm, the SDN.Allocate on /sdn/vnets indeed don't make sense.
(it's only on the /sdn/zones, where you allocate the vnets).

it should be SDN.Audit only.




> 
> > +    }
> > +    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://antiphishing.cetsi.fr/proxy/v3?i=Zk92VEFKaGQ4Ums4cnZEUWMTpfHaXFQGRw1_CnOoOH0&r=bHA1dGV3NWJQVUloaWNFUZPu0fK2BfWNnEHaDLDwG0rtDedpluZBIffSL1M5cj3F&f=SlhDbE9uS2laS2JaZFpNWvmsxai1zlJP9llgnl5HIv-4jAji8Dh2BQawzxID5bzr6Uv-3EQd-eluQbsPfcUOTg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=XRKU
> > 
> > 
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=Zk92VEFKaGQ4Ums4cnZEUWMTpfHaXFQGRw1_CnOoOH0&r=bHA1dGV3NWJQVUloaWNFUZPu0fK2BfWNnEHaDLDwG0rtDedpluZBIffSL1M5cj3F&f=SlhDbE9uS2laS2JaZFpNWvmsxai1zlJP9llgnl5HIv-4jAji8Dh2BQawzxID5bzr6Uv-3EQd-eluQbsPfcUOTg&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=XRKU
> 

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

[Fabian Grünbichler]

On June 2, 2023 2:12 pm, DERUMIER, Alexandre wrote:
> Le vendredi 02 juin 2023 à 13:43 +0200, Fabian Grünbichler a écrit :
>> a few more places that come to my mind that might warrant further
>> thinking or discussion:
>> - restoring a backup
> doesn't it also use create_vm ?

yes, but the potentially problematic parameters are coming from the
backup in that case, not $param :) we do check the storage permissions
at least, if we view nics on bridges/vnet as being the same kind of
entity as volumes on storages than it would make sense to also check
vnet permissions there (PVE::QemuServer -> $parse_backup_hints , but
probably that is not the best place for network related checks ;))

> __PACKAGE__->register_method({
>     name => 'create_vm',
>     path => '',
>     method => 'POST',
>     description => "Create or restore a virtual machine.",
> 
> 
>> - cloning a VM
> 
> for cloning, we can't change the target bridge, so if user have access
> to the clone, isn't it enough ?

same as above - if we treat "volume on storage" and "nic in vnet" as
being equivalent, then cloning would also need to check whether I am
allowed to add new nics to a vnet via cloning (like we do for volumes,
even without a storage override set!). $check_storage_access_clone is
the current helper, a similar one could be added for nics.

note: we'd also need a similar patch for pve-container ;) but if you
want, I can handle that one once this series is done or almost done, the
current approach is guest-agnostic anyway so I don't expect any changes
required for container support.

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

[DERUMIER, Alexandre]

> 
> same as above - if we treat "volume on storage" and "nic in vnet" as
> being equivalent, then cloning would also need to check whether I am
> allowed to add new nics to a vnet via cloning (like we do for
> volumes,
> even without a storage override set!). $check_storage_access_clone is
> the current helper, a similar one could be added for nics.
> 
ok got it! thanks.
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=MlZSTzBhZFZ6Nzl4c3EyN7fbSKDePLMxi5u5_onpAoI&r=cm1qVmRYUWk2WXhYZVFHWA0PXtTaYxz7-FIOTkZBm34_dHdSch-gXn7ST9eGhQLN&f=S1Zkd042VWdrZG5qQUxxWk5ps4t67kNuHsBZzdzhpquLKuXqTZLIq2K1DfKr9N61yBafm7AuAITd6bHtRU4zEQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=F1is

Re: [pve-devel] [PATCH qemu-server 0/1] api2: add check_bridge_access

[Fabian Grünbichler]

On May 26, 2023 9:33 am, Alexandre Derumier wrote:
> For proxmox 8, following the pve-manager patch serie
> https://lists.proxmox.com/pipermail/pve-devel/2023-May/056970.html
> 
> This patch serie add check of permissions for bridge/vnets access
> (currently only at vm create/update, I'm note sureif they are other
> places where it should be added)
> 
> if user have access to a zone, it have access to all vnets + vnet vlans
> if user have access to a vnet, it have access to the vnet + vnet vlans
> if user have access to a specific vnet+vlan, it have access to the vlan only

the last part could be solved more elegantly IMHO by making tags
children of vnets (and delegating the propagation the propagation bit of
the ACL), see comments on individual patches.

nit: if you send a single commit, no need for a cover letter - and then
please include this information in the commit message, as series cover
letters are not included once the patch is applied!

> 
> Alexandre Derumier (1):
>   api2: add check_bridge_access for create/update vm
> 
>  PVE/API2/Qemu.pm | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>