[pve-devel] [PATCH-SERIE pve-access-control/pve-manager/pve-guest-common/qemu-server/pve-network] check permissions on local bridge

[pve-devel] [PATCH-SERIE pve-access-control/pve-manager/pve-guest-common/qemu-server/pve-network] check permissions on local bridge

[Alexandre Derumier]

add vnet/localbridge permissions management

Hi,
as we has discuted some weeks ago,
this patche serie introduce management of acl for vnets && local bridges

The permission path is:

/sdn/zones/<zone>/<vnet>

where the local vmbr are in a virtual "localnetwork" zone

/sdn/zones/localnetwork/<vnet>

Vlans permissions  are also handled with
/sdn/zones/<zone>/<vnet>/<tag>

if user have permissions on the vnet/tag, he have access to only the specific vlan.
if user have permissions on the vnet with propagate, he have access to all vlans of the vnet
if user have permissions on the vnet without propagate, he have access to bridge only without any vlan


I have reworked the sdn zone panel from the tree, to manage permissions
on displayed vnets. (patch 3 && 4 pve-manager)

some screenshots:

https://mutulin1.odiso.net/sdnzone-perm.png
https://mutulin1.odiso.net/localzone-perm.png



changelog v2:
 - use /vnets/vlan instead /vnets.vlan
 - rework the bridge filtering when user have access only to a specific vlan
 - api2 network: always check bridge access if no filter is defined

changelog v3:
 - use /sdn/zones/<zone>/vnets/vlan instead /sdn/vnets/vnets.vlan
 - add SDN.Use permission
 - pve-manager: split ui code (could be applied later)
 - remove check on zone (it's now propagate with new path)
 - move check_vnet_access to pve-guest-common for lxc reuse
 - pve-network: fix vnet/tag perm check

changelog v4:
 - qemu-server: check permissions on backup restore
 - guest-common: check trunks permissions

todo:
 - implement lxc check permissions



pve-access-control:

Alexandre Derumier (3):
  access control: add /sdn/zones/<zone>/<vnet>/<vlan> path
  rpcenvironnment: add check_sdn_bridge
  add new SDN.use privilege in PVESDNUser role

 src/PVE/AccessControl.pm  |  6 +++++-
 src/PVE/RPCEnvironment.pm | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

pve-manager:

Alexandre Derumier (4):
  api2: network: check permissions for local bridges
  api2: cluster: ressources: add "localnetwork" zone
  ui: add vnet permissions panel
  ui: add permissions management for "localnetwork" zone

 PVE/API2/Cluster.pm                  |  14 ++
 PVE/API2/Network.pm                  |  25 ++-
 www/manager6/Makefile                |   2 +
 www/manager6/sdn/Browser.js          |  17 +-
 www/manager6/sdn/VnetACLView.js      | 289 +++++++++++++++++++++++++++
 www/manager6/sdn/ZoneContentPanel.js |  41 ++++
 www/manager6/sdn/ZoneContentView.js  |  52 ++++-
 7 files changed, 411 insertions(+), 29 deletions(-)
 create mode 100644 www/manager6/sdn/VnetACLView.js
 create mode 100644 www/manager6/sdn/ZoneContentPanel.js

pve-guest-common:

Alexandre Derumier (1):
  helpers : add check_vnet_access

 src/PVE/GuestHelpers.pm | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

qemu-server:

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

 PVE/API2/Qemu.pm | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)


pve-network:

Alexandre Derumier (1):
  get_local_vnets: fix permission path && perm

 PVE/Network/SDN.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v2 pve-access-control 1/3] access control: add /sdn/zones/<zone>/<vnet>/<vlan> path

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/AccessControl.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 89b7d90..6a3d203 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1283,7 +1283,8 @@ sub check_path {
 	|/pool/[[:alnum:]\.\-\_]+
 	|/sdn
 	|/sdn/zones/[[:alnum:]\.\-\_]+
-	|/sdn/vnets/[[:alnum:]\.\-\_]+
+	|/sdn/zones/[[:alnum:]\.\-\_]+/[[:alnum:]\.\-\_]+
+	|/sdn/zones/[[:alnum:]\.\-\_]+/[[:alnum:]\.\-\_]+/[1-9][0-9]{0,3}
 	|/storage
 	|/storage/[[:alnum:]\.\-\_]+
 	|/vms
-- 
2.30.2

[pve-devel] applied: [PATCH v2 pve-access-control 1/3] access control: add /sdn/zones/<zone>/<vnet>/<vlan> path

[Fabian Grünbichler]

applied this one, with a small fixup..

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  src/PVE/AccessControl.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 89b7d90..6a3d203 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1283,7 +1283,8 @@ sub check_path {
>  	|/pool/[[:alnum:]\.\-\_]+
>  	|/sdn
>  	|/sdn/zones/[[:alnum:]\.\-\_]+
> -	|/sdn/vnets/[[:alnum:]\.\-\_]+
> +	|/sdn/zones/[[:alnum:]\.\-\_]+/[[:alnum:]\.\-\_]+
> +	|/sdn/zones/[[:alnum:]\.\-\_]+/[[:alnum:]\.\-\_]+/[1-9][0-9]{0,3}
>  	|/storage
>  	|/storage/[[:alnum:]\.\-\_]+
>  	|/vms
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

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

[Alexandre Derumier]

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);
+
 		$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

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

[Fabian Grünbichler]

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

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

[DERUMIER, Alexandre]

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


Well, I have looked at this, but I don't see where in the code the
config storages are checked and where the config is extracted.


If the param->{storage} is not defined, the check is done somewhere in
the task with this kind of nice error log in the task ;)

"
error before or during data restore, some or all disks were not
completely restored. VM 249 state is NOT cleaned up.
TASK ERROR: command 'set -o pipefail && zstd -q -d -c
/mnt/pve/cephfs/dump/vzdump-qemu-210-2023_06_06-21_00_03.vma.zst | vma
extract -v -r /var/tmp/vzdumptmp3542000.fifo -
/var/tmp/vzdumptmp3542000' failed: 403 Permission check failed
(/storage/local-zfs, Datastore.AllocateSpace)

"

I was more thinking to add the check before launching the task, seem
better no ?

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

[Thomas Lamprecht]

On 07/06/2023 14:03, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Qemu.pm | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
>

applied, with Fabians R-b, thanks.

Made a follow-up moving the checker method to QemuServer and replacing getting
the config fromthe archive twice by checking after the config from the backup
and the override pa<rameters passed on restore got merged into the actual target
config, so this wasn't only a inefficiency thing IIUC, but actually wrong, i.e.,
if one passed a override for a netX property the one from the backup got checked,
not the effective one.

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

[DERUMIER, Alexandre]

Le jeudi 08 juin 2023 à 18:02 +0200, Thomas Lamprecht a écrit :
> On 07/06/2023 14:03, Alexandre Derumier wrote:
> > Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> > ---
> >  PVE/API2/Qemu.pm | 33 +++++++++++++++++++++++++++++----
> >  1 file changed, 29 insertions(+), 4 deletions(-)
> > 
> > 
> 
> applied, with Fabians R-b, thanks.
> 
> Made a follow-up moving the checker method to QemuServer and
> replacing getting
> the config fromthe archive twice by checking after the config from
> the backup
> and the override pa<rameters passed on restore got merged into the
> actual target
> config, so this wasn't only a inefficiency thing IIUC, but actually
> wrong, i.e.,
> if one passed a override for a netX property the one from the backup
> got checked,
> not the effective one.
> 
Thanks Thomas.

Just wonder, could it be done before disk restore ?  (That's what I was
trying to do)


it seem to be inefficiency too to check it after disk restore (if for
example, user restore a big backup, taking hours)

I have done a test from the gui
"
...
progress 98% (read 21045379072 bytes, duration 14 sec)
progress 99% (read 21260140544 bytes, duration 14 sec)
progress 100% (read 21474836480 bytes, duration 14 sec)
total bytes read 21474836480, sparse bytes 18656022528 (86.9%)
space reduction due to 4K zero blocks 4.54%
no lock found trying to remove 'create'  lock
error before or during data restore, some or all disks were not
completely restored. VM 249 state is NOT cleaned up.
TASK ERROR: 403 Permission check failed
(/sdn/zones/localnetwork/vmbr0/96, SDN.Use)

"

The vm config file is created, mostly empty:
/etc/pve/qemu-server/<vmid>.conf
memory:128

and the restored disk are not removed too

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

[DERUMIER, Alexandre]

Le vendredi 09 juin 2023 à 07:00 +0000, DERUMIER, Alexandre a écrit :
> Le jeudi 08 juin 2023 à 18:02 +0200, Thomas Lamprecht a écrit :
> > On 07/06/2023 14:03, Alexandre Derumier wrote:
> > > Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> > > ---
> > >  PVE/API2/Qemu.pm | 33 +++++++++++++++++++++++++++++----
> > >  1 file changed, 29 insertions(+), 4 deletions(-)
> > > 
> > > 
> > 
> > applied, with Fabians R-b, thanks.
> > 
> > Made a follow-up moving the checker method to QemuServer and
> > replacing getting
> > the config fromthe archive twice by checking after the config from
> > the backup
> > and the override pa<rameters passed on restore got merged into the
> > actual target
> > config, so this wasn't only a inefficiency thing IIUC, but actually
> > wrong, i.e.,
> > if one passed a override for a netX property the one from the
> > backup
> > got checked,
> > not the effective one.
> > 
> Thanks Thomas.
> 
> Just wonder, could it be done before disk restore ?  (That's what I
> was
> trying to do)
> 
> 
> it seem to be inefficiency too to check it after disk restore (if for
> example, user restore a big backup, taking hours)
> 
> I have done a test from the gui
> "
> ...
> progress 98% (read 21045379072 bytes, duration 14 sec)
> progress 99% (read 21260140544 bytes, duration 14 sec)
> progress 100% (read 21474836480 bytes, duration 14 sec)
> total bytes read 21474836480, sparse bytes 18656022528 (86.9%)
> space reduction due to 4K zero blocks 4.54%
> no lock found trying to remove 'create'  lock
> error before or during data restore, some or all disks were not
> completely restored. VM 249 state is NOT cleaned up.
> TASK ERROR: 403 Permission check failed
> (/sdn/zones/localnetwork/vmbr0/96, SDN.Use)
> 
> "
> 
> The vm config file is created, mostly empty:
> /etc/pve/qemu-server/<vmid>.conf
> memory:128
> 
> and the restored disk are not removed too
> 
> 

Or Maybe, we should simply warn && remove the netX from the restore
config ?
(I'm thinking about old backup with older non existing bridge anymore
or coming from another cluster, where user couldn't have any
permissions)

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

[Thomas Lamprecht]

On 09/06/2023 09:14, DERUMIER, Alexandre wrote:
> Or Maybe, we should simply warn && remove the netX from the restore config ? (I'm thinking about old backup with older non existing bridge anymore or coming from another cluster, where user couldn't have any permissions)

I think the whole UX of this new check can definitively be improved for,
at least for restore and clone.

Once we expose nicely overriding also network for the original config,
we can highlight vnets/briges that either do not exist, or the user
has no access to, already as invalid in the web UI.
Then the user can override it with one that exists and that they have
access too in the UI.

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

[DERUMIER, Alexandre]

Le vendredi 09 juin 2023 à 09:29 +0200, Thomas Lamprecht a écrit :
> On 09/06/2023 09:14, DERUMIER, Alexandre wrote:
> > Or Maybe, we should simply warn && remove the netX from the restore
> > config ? (I'm thinking about old backup with older non existing
> > bridge anymore or coming from another cluster, where user couldn't
> > have any permissions)
> 
> I think the whole UX of this new check can definitively be improved
> for,
> at least for restore and clone.
> 
> Once we expose nicely overriding also network for the original
> config,
> we can highlight vnets/briges that either do not exist, or the user
> has no access to, already as invalid in the web UI.
> Then the user can override it with one that exists and that they have
> access too in the UI.

Yes, I was thinking about that too. 

I just send a patch to warn/remove the net from restore config , I
think it's a good compromise until the gui is implemented ?
https://lists.proxmox.com/pipermail/pve-devel/2023-June/057392.html


> 

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

[Thomas Lamprecht]

On 09/06/2023 09:00, DERUMIER, Alexandre wrote:
> Le jeudi 08 juin 2023 à 18:02 +0200, Thomas Lamprecht a écrit :
>> On 07/06/2023 14:03, Alexandre Derumier wrote:
>>> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
>>> ---
>>>  PVE/API2/Qemu.pm | 33 +++++++++++++++++++++++++++++----
>>>  1 file changed, 29 insertions(+), 4 deletions(-)
>>>
>>>
>>
>> applied, with Fabians R-b, thanks.
>>
>> Made a follow-up moving the checker method to QemuServer and
>> replacing getting
>> the config fromthe archive twice by checking after the config from
>> the backup
>> and the override pa<rameters passed on restore got merged into the
>> actual target
>> config, so this wasn't only a inefficiency thing IIUC, but actually
>> wrong, i.e.,
>> if one passed a override for a netX property the one from the backup
>> got checked,
>> not the effective one.
>>
> Thanks Thomas.
> 
> Just wonder, could it be done before disk restore ?  (That's what I was
> trying to do)> 
> it seem to be inefficiency too to check it after disk restore (if for
> example, user restore a big backup, taking hours)

yes, sure, but as mentioned in the commit message, if it's checked
to late other things happen to early, as doing stuff before having
the merged config seems odd.

And I did not wanted to re-work that part in a hurry, we can improve
that still in the next week(s).

> 
> I have done a test from the gui
> "
> ...
> progress 98% (read 21045379072 bytes, duration 14 sec)
> progress 99% (read 21260140544 bytes, duration 14 sec)
> progress 100% (read 21474836480 bytes, duration 14 sec)
> total bytes read 21474836480, sparse bytes 18656022528 (86.9%)
> space reduction due to 4K zero blocks 4.54%
> no lock found trying to remove 'create'  lock
> error before or during data restore, some or all disks were not
> completely restored. VM 249 state is NOT cleaned up.
> TASK ERROR: 403 Permission check failed
> (/sdn/zones/localnetwork/vmbr0/96, SDN.Use)
> 
> "
> 
> The vm config file is created, mostly empty:
> /etc/pve/qemu-server/<vmid>.conf
> memory:128
> 
> and the restored disk are not removed too
> 
> 


Yes, that's not ideal, but the check is now actually correct; the existing
order of restore and config merging needs the fixing.

[pve-devel] [PATCH v3 pve-manager 1/4] api2: network: check permissions for local bridges

[Alexandre Derumier]

always check permissions, also when not filtered

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Network.pm | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index a43579fa..8dc56482 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -209,7 +209,7 @@ __PACKAGE__->register_method({
 	    type => {
 		description => "Only list specific interface types.",
 		type => 'string',
-		enum => [ @$network_type_enum, 'any_bridge' ],
+		enum => [ @$network_type_enum, 'any_bridge', 'any_local_bridge' ],
 		optional => 1,
 	    },
 	},
@@ -240,22 +240,17 @@ __PACKAGE__->register_method({
 
 	if (my $tfilter = $param->{type}) {
 	    my $vnets;
-	    my $vnet_cfg;
-	    my $can_access_vnet = sub { # only matters for the $have_sdn case, checked implict
-		return 1 if $authuser eq 'root@pam' || !defined($vnets);
-		return 1 if !defined(PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $_[0], 1)); # not a vnet
-		$rpcenv->check_any($authuser, "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1)
-	    };
 
 	    if ($have_sdn && $param->{type} eq 'any_bridge') {
 		$vnets = PVE::Network::SDN::get_local_vnets(); # returns already access-filtered
-		$vnet_cfg = PVE::Network::SDN::Vnets::config();
 	    }
 
 	    for my $k (sort keys $ifaces->%*) {
 		my $type = $ifaces->{$k}->{type};
-		my $match = $tfilter eq $type || ($tfilter eq 'any_bridge' && ($type eq 'bridge' || $type eq 'OVSBridge'));
-		delete $ifaces->{$k} if !($match && $can_access_vnet->($k));
+		my $match = ($param->{type} eq $type) || (
+		    ($param->{type} =~ /^any(_local)?_bridge$/) &&
+		    ($type eq 'bridge' || $type eq 'OVSBridge'));
+		delete $ifaces->{$k} if !$match;
 	    }
 
 	    if (defined($vnets)) {
@@ -263,6 +258,16 @@ __PACKAGE__->register_method({
 	    }
 	}
 
+	#always check bridge access
+	my $can_access_vnet = sub {
+	    return 1 if $authuser eq 'root@pam';
+	    return 1 if $rpcenv->check_sdn_bridge($authuser, "localnetwork", $_[0], ['SDN.Audit', 'SDN.Use'], 1);
+	};
+	for my $k (sort keys $ifaces->%*) {
+	    my $type = $ifaces->{$k}->{type};
+	    delete $ifaces->{$k} if ($type eq 'bridge' || $type eq 'OVSBridge') && !$can_access_vnet->($k);
+	}
+
 	return PVE::RESTHandler::hash_to_array($ifaces, 'iface');
    }});
 
-- 
2.30.2

[pve-devel] applied: [PATCH v3 pve-manager 1/4] api2: network: check permissions for local bridges

[Fabian Grünbichler]

applied this one with a bit of followups to make it more readable, but
no semantic changes intended.

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> always check permissions, also when not filtered
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Network.pm | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> index a43579fa..8dc56482 100644
> --- a/PVE/API2/Network.pm
> +++ b/PVE/API2/Network.pm
> @@ -209,7 +209,7 @@ __PACKAGE__->register_method({
>  	    type => {
>  		description => "Only list specific interface types.",
>  		type => 'string',
> -		enum => [ @$network_type_enum, 'any_bridge' ],
> +		enum => [ @$network_type_enum, 'any_bridge', 'any_local_bridge' ],
>  		optional => 1,
>  	    },
>  	},
> @@ -240,22 +240,17 @@ __PACKAGE__->register_method({
>  
>  	if (my $tfilter = $param->{type}) {
>  	    my $vnets;
> -	    my $vnet_cfg;
> -	    my $can_access_vnet = sub { # only matters for the $have_sdn case, checked implict
> -		return 1 if $authuser eq 'root@pam' || !defined($vnets);
> -		return 1 if !defined(PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $_[0], 1)); # not a vnet
> -		$rpcenv->check_any($authuser, "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1)
> -	    };
>  
>  	    if ($have_sdn && $param->{type} eq 'any_bridge') {
>  		$vnets = PVE::Network::SDN::get_local_vnets(); # returns already access-filtered
> -		$vnet_cfg = PVE::Network::SDN::Vnets::config();
>  	    }
>  
>  	    for my $k (sort keys $ifaces->%*) {
>  		my $type = $ifaces->{$k}->{type};
> -		my $match = $tfilter eq $type || ($tfilter eq 'any_bridge' && ($type eq 'bridge' || $type eq 'OVSBridge'));
> -		delete $ifaces->{$k} if !($match && $can_access_vnet->($k));
> +		my $match = ($param->{type} eq $type) || (
> +		    ($param->{type} =~ /^any(_local)?_bridge$/) &&
> +		    ($type eq 'bridge' || $type eq 'OVSBridge'));
> +		delete $ifaces->{$k} if !$match;
>  	    }
>  
>  	    if (defined($vnets)) {
> @@ -263,6 +258,16 @@ __PACKAGE__->register_method({
>  	    }
>  	}
>  
> +	#always check bridge access
> +	my $can_access_vnet = sub {
> +	    return 1 if $authuser eq 'root@pam';
> +	    return 1 if $rpcenv->check_sdn_bridge($authuser, "localnetwork", $_[0], ['SDN.Audit', 'SDN.Use'], 1);
> +	};
> +	for my $k (sort keys $ifaces->%*) {
> +	    my $type = $ifaces->{$k}->{type};
> +	    delete $ifaces->{$k} if ($type eq 'bridge' || $type eq 'OVSBridge') && !$can_access_vnet->($k);
> +	}
> +
>  	return PVE::RESTHandler::hash_to_array($ifaces, 'iface');
>     }});
>  
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH pve-network 1/1] get_local_vnets: fix permission path && perm

[Alexandre Derumier]

new path is /zones/<zone>/<vnetid>

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/Network/SDN.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/Network/SDN.pm b/PVE/Network/SDN.pm
index b95dd5b..1ad85e5 100644
--- a/PVE/Network/SDN.pm
+++ b/PVE/Network/SDN.pm
@@ -190,10 +190,10 @@ sub get_local_vnets {
 	my $zoneid = $vnet->{zone};
 	my $comments = $vnet->{alias};
 
-	my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
+	my $privs = [ 'SDN.Audit', 'SDN.Use' ];
 
 	next if !$zoneid;
-	next if !$rpcenv->check_any($authuser, "/sdn/zones/$zoneid", $privs, 1) && !$rpcenv->check_any($authuser, "/sdn/vnets/$vnetid", $privs, 1);
+	next if !$rpcenv->check_sdn_bridge($authuser, $zoneid, $vnetid, $privs, 1);
 
 	my $zone_config = PVE::Network::SDN::Zones::sdn_zones_config($zones_cfg, $zoneid);
 
-- 
2.30.2

Re: [pve-devel] [PATCH pve-network 1/1] get_local_vnets: fix permission path && perm

[Fabian Grünbichler]

pve-network requires more work:

- there is a lot of /sdn/vnets/.. permission checks leftover (all of the vnet/subnet code!)
- there are /sdn/vnets/../subnets/.. ACL paths that need to be dropped,
  or they clash with /sdn/zones/<zone>/<vnet>[/<vlan>]
- the GUI seems to be broken when "Advanced" is not ticked

I started off, but then I realized we might also want to re-evaluate:
- whether we even care about potentially leaking the vnet<->zone binding
  in case the ACL checks fail
- whether we want to move the whole API tree as well to have vnets below
  zones instead of next to eachother, so we always have the zone as
  (path) parameter?

anyhow, here's a half-diff of some potentially relevant changes ;)

```
diff --git a/src/PVE/API2/Network/SDN/Subnets.pm b/src/PVE/API2/Network/SDN/Subnets.pm
index 377a568..fbe2c46 100644
--- a/src/PVE/API2/Network/SDN/Subnets.pm
+++ b/src/PVE/API2/Network/SDN/Subnets.pm
@@ -39,7 +39,7 @@ __PACKAGE__->register_method ({
     method => 'GET',
     description => "SDN subnets index.",
     permissions => {
-	description => "Only list entries where you have 'SDN.Audit' or 'SDN.Allocate' permissions on '/sdn/subnets/<subnet>'",
+	description => "Only list entries where you have 'SDN.Audit', 'SDN.Use' or 'SDN.Allocate' permissions on '/sdn/subnets/<subnet>'",
 	user => 'all',
     },
     parameters => {
@@ -89,7 +89,7 @@ __PACKAGE__->register_method ({
 	my @sids = PVE::Network::SDN::Subnets::sdn_subnets_ids($cfg);
 	my $res = [];
 	foreach my $id (@sids) {
-	    my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
+	    my $privs = [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ];
 	    next if !$rpcenv->check_any($authuser, "/sdn/vnets/$vnetid/subnets/$id", $privs, 1);
 
 	    my $scfg = &$api_sdn_subnets_config($cfg, $id);
diff --git a/src/PVE/API2/Network/SDN/Vnets.pm b/src/PVE/API2/Network/SDN/Vnets.pm
index 811a2e8..eaa3a04 100644
--- a/src/PVE/API2/Network/SDN/Vnets.pm
+++ b/src/PVE/API2/Network/SDN/Vnets.pm
@@ -50,6 +50,13 @@ my $api_sdn_vnets_deleted_config = sub {
     }
 };
 
+# checks access, but masks zone to avoid info leak..
+my $check_vnet_access = sub {
+    sub ($rpcenv, $authuser, $zone, $vnet, $privs) = @_;
+    $rpcenv->check_any($authuser, "/sdn/zones/<zone>/$vnet", $privs)
+	if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$vnet", $privs, 1);
+}
+
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',
@@ -57,7 +64,7 @@ __PACKAGE__->register_method ({
     description => "SDN vnets index.",
     permissions => {
 	description => "Only list entries where you have 'SDN.Audit' or 'SDN.Allocate'"
-	    ." permissions on '/sdn/vnets/<vnet>'",
+	    ." permissions on '/sdn/zones/<zone>/<vnet>'",
 	user => 'all',
     },
     parameters => {
@@ -104,8 +111,10 @@ __PACKAGE__->register_method ({
 	my @sids = PVE::Network::SDN::Vnets::sdn_vnets_ids($cfg);
 	my $res = [];
 	foreach my $id (@sids) {
-	    my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
-	    next if !$rpcenv->check_any($authuser, "/sdn/vnets/$id", $privs, 1);
+	    my $privs = [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ];
+	    my $zone = $cfg->{$id}->{zone};
+	    next if !$zone;
+	    next if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$id", $privs, 1);
 
 	    my $scfg = &$api_sdn_vnets_config($cfg, $id);
 	    push @$res, $scfg;
@@ -120,8 +129,9 @@ __PACKAGE__->register_method ({
     method => 'GET',
     description => "Read sdn vnet configuration.",
     permissions => {
-	check => ['perm', '/sdn/vnets/{vnet}', ['SDN.Allocate']],
-   },
+	description => "Requires 'SDN.Allocate' permission on '/sdn/zones/<zone>/<vnet>'",
+	user => 'all',
+    },
     parameters => {
 	additionalProperties => 0,
 	properties => {
@@ -144,6 +154,9 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
+
 	my $cfg = {};
 	if($param->{pending}) {
 	    my $running_cfg = PVE::Network::SDN::running_config();
@@ -156,6 +169,11 @@ __PACKAGE__->register_method ({
 	    $cfg = PVE::Network::SDN::Vnets::config();
 	}
 
+	my $zone = $cfg->{$vnet}->{zone};
+	return if !$zone;
+
+	$check_vnet_access($rpcenv, $authuser, $zone, $vnet, ['SDN.Allocate']);
+
 	return $api_sdn_vnets_config->($cfg, $param->{vnet});
     }});
 
@@ -166,7 +184,7 @@ __PACKAGE__->register_method ({
     method => 'POST',
     description => "Create a new sdn vnet object.",
     permissions => {
-	check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
+	check => ['perm', '/sdn/zones/{zone}', ['SDN.Allocate']],
     },
     parameters => PVE::Network::SDN::VnetPlugin->createSchema(),
     returns => { type => 'null' },
@@ -210,24 +228,36 @@ __PACKAGE__->register_method ({
     method => 'PUT',
     description => "Update sdn vnet object configuration.",
     permissions => {
-	check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
+	description => "Requires 'SDN.Allocate' permission on '/sdn/zones/<zone>/<vnet>'",
+	user => 'all',
     },
     parameters => PVE::Network::SDN::VnetPlugin->updateSchema(),
     returns => { type => 'null' },
     code => sub {
 	my ($param) = @_;
 
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
+
 	my $id = extract_param($param, 'vnet');
 	my $digest = extract_param($param, 'digest');
 
 	PVE::Network::SDN::lock_sdn_config(sub {
 	    my $cfg = PVE::Network::SDN::Vnets::config();
 
-	    PVE::SectionConfig::assert_if_modified($cfg, $digest);
+	    my $zone = $cfg->{ids}->{$id}->{zone} // $params->{zone};
+	    # TODO can this even happen?
+	    raise_param_exc({ zone => "missing zone" }) if !$zone;
+
+	    $check_vnet_access($rpcenv, $authuser, $zone, $id, ['SDN.Allocate']);
+
+	    if (my $new_zone = $params->{zone}) {
+		$rpcenv->check($authuser, "/sdn/zones/$new_zone/$id", ['SDN.Allocate']);
+	    }
 
+	    PVE::SectionConfig::assert_if_modified($cfg, $digest);
 
 	    my $opts = PVE::Network::SDN::VnetPlugin->check_config($id, $param, 0, 1);
-	    raise_param_exc({ zone => "missing zone"}) if !$opts->{zone};
 	    my $subnets = PVE::Network::SDN::Vnets::get_subnets($id);
 	    raise_param_exc({ zone => "can't change zone if subnets exists"}) if($subnets && $opts->{zone} ne $cfg->{ids}->{$id}->{zone});
 
@@ -256,7 +286,8 @@ __PACKAGE__->register_method ({
     method => 'DELETE',
     description => "Delete sdn vnet object configuration.",
     permissions => {
-	check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
+	description => "Requires 'SDN.Allocate' permission on '/sdn/zones/<zone>/<vnet>'",
+	user => 'all',
     },
     parameters => {
 	additionalProperties => 0,
@@ -270,10 +301,19 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
+
 	my $id = extract_param($param, 'vnet');
 
         PVE::Network::SDN::lock_sdn_config(sub {
 	    my $cfg = PVE::Network::SDN::Vnets::config();
+	    my $zone = $cfg->{ids}->{$id}->{zone};
+	    # TODO can this even happen?
+	    raise_param_exc({ zone => "missing zone" }) if !$zone;
+
+	    $check_vnet_access($rpcenv, $authuser, $zone, $id, ['SDN.Allocate']);
+
 	    my $scfg = PVE::Network::SDN::Vnets::sdn_vnets_config($cfg, $id); # check if exists
 	    my $vnet_cfg = PVE::Network::SDN::Vnets::config();
 
```

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> new path is /zones/<zone>/<vnetid>
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/Network/SDN.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Network/SDN.pm b/PVE/Network/SDN.pm
> index b95dd5b..1ad85e5 100644
> --- a/PVE/Network/SDN.pm
> +++ b/PVE/Network/SDN.pm
> @@ -190,10 +190,10 @@ sub get_local_vnets {
>  	my $zoneid = $vnet->{zone};
>  	my $comments = $vnet->{alias};
>  
> -	my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> +	my $privs = [ 'SDN.Audit', 'SDN.Use' ];
>  
>  	next if !$zoneid;
> -	next if !$rpcenv->check_any($authuser, "/sdn/zones/$zoneid", $privs, 1) && !$rpcenv->check_any($authuser, "/sdn/vnets/$vnetid", $privs, 1);
> +	next if !$rpcenv->check_sdn_bridge($authuser, $zoneid, $vnetid, $privs, 1);
>  
>  	my $zone_config = PVE::Network::SDN::Zones::sdn_zones_config($zones_cfg, $zoneid);
>  
> -- 
> 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 pve-network 1/1] get_local_vnets: fix permission path && perm

[DERUMIER, Alexandre]

Le mercredi 07 juin 2023 à 16:56 +0200, Fabian Grünbichler a écrit :
> pve-network requires more work:
> 
> - there is a lot of /sdn/vnets/.. permission checks leftover (all of
> the vnet/subnet code!)
> - there are /sdn/vnets/../subnets/.. ACL paths that need to be
> dropped,
>   or they clash with /sdn/zones/<zone>/<vnet>[/<vlan>]

I'll look at it . permissions management on /sdn/vnets was never
exposed to gui (in zone permission manage, on main permissions panel
combo). So I really don't think that somebody has ever used it.

I don't think we need permission on subnets anyway. 
Permission on vnet with SDN.use should allow to use all subnets
of the vnet.


> - the GUI seems to be broken when "Advanced" is not ticked
> 
Yes, some user reported this some weeks ago (without any change in pve-
network). I think something have change in pve-manager, I need to check
that.


> I started off, but then I realized we might also want to re-evaluate:
> - whether we even care about potentially leaking the vnet<->zone
> binding
>   in case the ACL checks fail

I think for SDN.Allocate (create vnets/change zone option), permission
on zone should be enough.

SDN.Allocate on a vnet to create subnets or change vnet option.


So, yes, using /sdn/zones/<zone>/<vnet> make sense. (and propagate from
the zone is great too).







>- whether we want to move the whole API tree as well to have vnets
>below
>  zones instead of next to eachother, so we always have the zone as
>  (path) parameter?

I'll try to have a look at this too.


>>anyhow, here's a half-diff of some potentially relevant changes ;)


ok. thanks. I'll work on it tomorrow !





> ```
> diff --git a/src/PVE/API2/Network/SDN/Subnets.pm
> b/src/PVE/API2/Network/SDN/Subnets.pm
> index 377a568..fbe2c46 100644
> --- a/src/PVE/API2/Network/SDN/Subnets.pm
> +++ b/src/PVE/API2/Network/SDN/Subnets.pm
> @@ -39,7 +39,7 @@ __PACKAGE__->register_method ({
>      method => 'GET',
>      description => "SDN subnets index.",
>      permissions => {
> -       description => "Only list entries where you have 'SDN.Audit'
> or 'SDN.Allocate' permissions on '/sdn/subnets/<subnet>'",
> +       description => "Only list entries where you have 'SDN.Audit',
> 'SDN.Use' or 'SDN.Allocate' permissions on '/sdn/subnets/<subnet>'",
>         user => 'all',
>      },
>      parameters => {
> @@ -89,7 +89,7 @@ __PACKAGE__->register_method ({
>         my @sids = PVE::Network::SDN::Subnets::sdn_subnets_ids($cfg);
>         my $res = [];
>         foreach my $id (@sids) {
> -           my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> +           my $privs = [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ];
>             next if !$rpcenv->check_any($authuser,
> "/sdn/vnets/$vnetid/subnets/$id", $privs, 1);
>  
>             my $scfg = &$api_sdn_subnets_config($cfg, $id);
> diff --git a/src/PVE/API2/Network/SDN/Vnets.pm
> b/src/PVE/API2/Network/SDN/Vnets.pm
> index 811a2e8..eaa3a04 100644
> --- a/src/PVE/API2/Network/SDN/Vnets.pm
> +++ b/src/PVE/API2/Network/SDN/Vnets.pm
> @@ -50,6 +50,13 @@ my $api_sdn_vnets_deleted_config = sub {
>      }
>  };
>  
> +# checks access, but masks zone to avoid info leak..
> +my $check_vnet_access = sub {
> +    sub ($rpcenv, $authuser, $zone, $vnet, $privs) = @_;
> +    $rpcenv->check_any($authuser, "/sdn/zones/<zone>/$vnet", $privs)
> +       if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$vnet",
> $privs, 1);
> +}
> +
>  __PACKAGE__->register_method ({
>      name => 'index',
>      path => '',
> @@ -57,7 +64,7 @@ __PACKAGE__->register_method ({
>      description => "SDN vnets index.",
>      permissions => {
>         description => "Only list entries where you have 'SDN.Audit'
> or 'SDN.Allocate'"
> -           ." permissions on '/sdn/vnets/<vnet>'",
> +           ." permissions on '/sdn/zones/<zone>/<vnet>'",
>         user => 'all',
>      },
>      parameters => {
> @@ -104,8 +111,10 @@ __PACKAGE__->register_method ({
>         my @sids = PVE::Network::SDN::Vnets::sdn_vnets_ids($cfg);
>         my $res = [];
>         foreach my $id (@sids) {
> -           my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> -           next if !$rpcenv->check_any($authuser, "/sdn/vnets/$id",
> $privs, 1);
> +           my $privs = [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ];
> +           my $zone = $cfg->{$id}->{zone};
> +           next if !$zone;
> +           next if !$rpcenv->check_any($authuser,
> "/sdn/zones/$zone/$id", $privs, 1);
>  
>             my $scfg = &$api_sdn_vnets_config($cfg, $id);
>             push @$res, $scfg;
> @@ -120,8 +129,9 @@ __PACKAGE__->register_method ({
>      method => 'GET',
>      description => "Read sdn vnet configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets/{vnet}', ['SDN.Allocate']],
> -   },
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
> +    },
>      parameters => {
>         additionalProperties => 0,
>         properties => {
> @@ -144,6 +154,9 @@ __PACKAGE__->register_method ({
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $cfg = {};
>         if($param->{pending}) {
>             my $running_cfg = PVE::Network::SDN::running_config();
> @@ -156,6 +169,11 @@ __PACKAGE__->register_method ({
>             $cfg = PVE::Network::SDN::Vnets::config();
>         }
>  
> +       my $zone = $cfg->{$vnet}->{zone};
> +       return if !$zone;
> +
> +       $check_vnet_access($rpcenv, $authuser, $zone, $vnet,
> ['SDN.Allocate']);
> +
>         return $api_sdn_vnets_config->($cfg, $param->{vnet});
>      }});
>  
> @@ -166,7 +184,7 @@ __PACKAGE__->register_method ({
>      method => 'POST',
>      description => "Create a new sdn vnet object.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       check => ['perm', '/sdn/zones/{zone}', ['SDN.Allocate']],
>      },
>      parameters => PVE::Network::SDN::VnetPlugin->createSchema(),
>      returns => { type => 'null' },
> @@ -210,24 +228,36 @@ __PACKAGE__->register_method ({
>      method => 'PUT',
>      description => "Update sdn vnet object configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
>      },
>      parameters => PVE::Network::SDN::VnetPlugin->updateSchema(),
>      returns => { type => 'null' },
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $id = extract_param($param, 'vnet');
>         my $digest = extract_param($param, 'digest');
>  
>         PVE::Network::SDN::lock_sdn_config(sub {
>             my $cfg = PVE::Network::SDN::Vnets::config();
>  
> -           PVE::SectionConfig::assert_if_modified($cfg, $digest);
> +           my $zone = $cfg->{ids}->{$id}->{zone} // $params->{zone};
> +           # TODO can this even happen?
> +           raise_param_exc({ zone => "missing zone" }) if !$zone;
> +
> +           $check_vnet_access($rpcenv, $authuser, $zone, $id,
> ['SDN.Allocate']);
> +
> +           if (my $new_zone = $params->{zone}) {
> +               $rpcenv->check($authuser, "/sdn/zones/$new_zone/$id",
> ['SDN.Allocate']);
> +           }
>  
> +           PVE::SectionConfig::assert_if_modified($cfg, $digest);
>  
>             my $opts = PVE::Network::SDN::VnetPlugin-
> >check_config($id, $param, 0, 1);
> -           raise_param_exc({ zone => "missing zone"}) if !$opts-
> >{zone};
>             my $subnets = PVE::Network::SDN::Vnets::get_subnets($id);
>             raise_param_exc({ zone => "can't change zone if subnets
> exists"}) if($subnets && $opts->{zone} ne $cfg->{ids}->{$id}-
> >{zone});
>  
> @@ -256,7 +286,8 @@ __PACKAGE__->register_method ({
>      method => 'DELETE',
>      description => "Delete sdn vnet object configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
>      },
>      parameters => {
>         additionalProperties => 0,
> @@ -270,10 +301,19 @@ __PACKAGE__->register_method ({
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $id = extract_param($param, 'vnet');
>  
>          PVE::Network::SDN::lock_sdn_config(sub {
>             my $cfg = PVE::Network::SDN::Vnets::config();
> +           my $zone = $cfg->{ids}->{$id}->{zone};
> +           # TODO can this even happen?
> +           raise_param_exc({ zone => "missing zone" }) if !$zone;
> +
> +           $check_vnet_access($rpcenv, $authuser, $zone, $id,
> ['SDN.Allocate']);
> +
>             my $scfg =
> PVE::Network::SDN::Vnets::sdn_vnets_config($cfg, $id); # check if
> exists
>             my $vnet_cfg = PVE::Network::SDN::Vnets::config();
>  
> ```
> 
> On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> > new path is /zones/<zone>/<vnetid>
> > 
> > Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> > ---
> >  PVE/Network/SDN.pm | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/PVE/Network/SDN.pm b/PVE/Network/SDN.pm
> > index b95dd5b..1ad85e5 100644
> > --- a/PVE/Network/SDN.pm
> > +++ b/PVE/Network/SDN.pm
> > @@ -190,10 +190,10 @@ sub get_local_vnets {
> >         my $zoneid = $vnet->{zone};
> >         my $comments = $vnet->{alias};
> >  
> > -       my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> > +       my $privs = [ 'SDN.Audit', 'SDN.Use' ];
> >  
> >         next if !$zoneid;
> > -       next if !$rpcenv->check_any($authuser,
> > "/sdn/zones/$zoneid", $privs, 1) && !$rpcenv->check_any($authuser,
> > "/sdn/vnets/$vnetid", $privs, 1);
> > +       next if !$rpcenv->check_sdn_bridge($authuser, $zoneid,
> > $vnetid, $privs, 1);
> >  
> >         my $zone_config =
> > PVE::Network::SDN::Zones::sdn_zones_config($zones_cfg, $zoneid);
> >  
> > -- 
> > 2.30.2
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://antiphishing.cetsi.fr/proxy/v3?i=MUo0RzFIRTVvbFhYVGloQoloZQj6tvqyhpERsn5z8Z4&r=cFdGNHFjVENnWDEzUVliSYiK92A-8tPjy0OkrQBFKsNtwFQPVFVwvPagaFXOdIvK&f=ODlJNFRJTjZBcWFlaWxQaCCCfKFEiPqnNIdA-OFeRWhRkYGvCokAsY8PdoPF8z-Ieq4V3WNpzo4Gr8nE76YOxQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=b1p5
> > 
> > 
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=MUo0RzFIRTVvbFhYVGloQoloZQj6tvqyhpERsn5z8Z4&r=cFdGNHFjVENnWDEzUVliSYiK92A-8tPjy0OkrQBFKsNtwFQPVFVwvPagaFXOdIvK&f=ODlJNFRJTjZBcWFlaWxQaCCCfKFEiPqnNIdA-OFeRWhRkYGvCokAsY8PdoPF8z-Ieq4V3WNpzo4Gr8nE76YOxQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=b1p5
> 

Re: [pve-devel] [PATCH pve-network 1/1] get_local_vnets: fix permission path && perm

[DERUMIER, Alexandre]

Le mercredi 07 juin 2023 à 16:56 +0200, Fabian Grünbichler a écrit :
> pve-network requires more work:
> 
> - there is a lot of /sdn/vnets/.. permission checks leftover (all of
> the vnet/subnet code!)
> - there are /sdn/vnets/../subnets/.. ACL paths that need to be
> dropped,
>   or they clash with /sdn/zones/<zone>/<vnet>[/<vlan>]
> 
I have send separated patch to fix permissions
https://lists.proxmox.com/pipermail/pve-devel/2023-June/057374.html

I don't have changed api for now, as I should need to rework the gui a
little bit.
(and I'm not sure about api path like /sdn/zones/zone1/vnet1  ,
/sdn/zones/zone2/vnet1  , as vnet should really be unique)


But for now, acl are working fine with this patch.


> - the GUI seems to be broken when "Advanced" is not ticked
> 

I really don't see where is the problem (don't happen with
bgpcontroller advanced, and it's exactly the same code....)


> I started off, but then I realized we might also want to re-evaluate:
> - whether we even care about potentially leaking the vnet<->zone
> binding
>   in case the ACL checks fail
> - whether we want to move the whole API tree as well to have vnets
> below
>   zones instead of next to eachother, so we always have the zone as
>   (path) parameter?
> 
Maybe later ^_^, I need a little bit more time to think about it.
 

> anyhow, here's a half-diff of some potentially relevant changes ;)
> 
> ```
> diff --git a/src/PVE/API2/Network/SDN/Subnets.pm
> b/src/PVE/API2/Network/SDN/Subnets.pm
> index 377a568..fbe2c46 100644
> --- a/src/PVE/API2/Network/SDN/Subnets.pm
> +++ b/src/PVE/API2/Network/SDN/Subnets.pm
> @@ -39,7 +39,7 @@ __PACKAGE__->register_method ({
>      method => 'GET',
>      description => "SDN subnets index.",
>      permissions => {
> -       description => "Only list entries where you have 'SDN.Audit'
> or 'SDN.Allocate' permissions on '/sdn/subnets/<subnet>'",
> +       description => "Only list entries where you have 'SDN.Audit',
> 'SDN.Use' or 'SDN.Allocate' permissions on '/sdn/subnets/<subnet>'",
>         user => 'all',
>      },
>      parameters => {
> @@ -89,7 +89,7 @@ __PACKAGE__->register_method ({
>         my @sids = PVE::Network::SDN::Subnets::sdn_subnets_ids($cfg);
>         my $res = [];
>         foreach my $id (@sids) {
> -           my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> +           my $privs = [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ];
>             next if !$rpcenv->check_any($authuser,
> "/sdn/vnets/$vnetid/subnets/$id", $privs, 1);
>  
>             my $scfg = &$api_sdn_subnets_config($cfg, $id);
> diff --git a/src/PVE/API2/Network/SDN/Vnets.pm
> b/src/PVE/API2/Network/SDN/Vnets.pm
> index 811a2e8..eaa3a04 100644
> --- a/src/PVE/API2/Network/SDN/Vnets.pm
> +++ b/src/PVE/API2/Network/SDN/Vnets.pm
> @@ -50,6 +50,13 @@ my $api_sdn_vnets_deleted_config = sub {
>      }
>  };
>  
> +# checks access, but masks zone to avoid info leak..
> +my $check_vnet_access = sub {
> +    sub ($rpcenv, $authuser, $zone, $vnet, $privs) = @_;
> +    $rpcenv->check_any($authuser, "/sdn/zones/<zone>/$vnet", $privs)
> +       if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$vnet",
> $privs, 1);
> +}
> +
>  __PACKAGE__->register_method ({
>      name => 'index',
>      path => '',
> @@ -57,7 +64,7 @@ __PACKAGE__->register_method ({
>      description => "SDN vnets index.",
>      permissions => {
>         description => "Only list entries where you have 'SDN.Audit'
> or 'SDN.Allocate'"
> -           ." permissions on '/sdn/vnets/<vnet>'",
> +           ." permissions on '/sdn/zones/<zone>/<vnet>'",
>         user => 'all',
>      },
>      parameters => {
> @@ -104,8 +111,10 @@ __PACKAGE__->register_method ({
>         my @sids = PVE::Network::SDN::Vnets::sdn_vnets_ids($cfg);
>         my $res = [];
>         foreach my $id (@sids) {
> -           my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> -           next if !$rpcenv->check_any($authuser, "/sdn/vnets/$id",
> $privs, 1);
> +           my $privs = [ 'SDN.Audit', 'SDN.Use', 'SDN.Allocate' ];
> +           my $zone = $cfg->{$id}->{zone};
> +           next if !$zone;
> +           next if !$rpcenv->check_any($authuser,
> "/sdn/zones/$zone/$id", $privs, 1);
>  
>             my $scfg = &$api_sdn_vnets_config($cfg, $id);
>             push @$res, $scfg;
> @@ -120,8 +129,9 @@ __PACKAGE__->register_method ({
>      method => 'GET',
>      description => "Read sdn vnet configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets/{vnet}', ['SDN.Allocate']],
> -   },
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
> +    },
>      parameters => {
>         additionalProperties => 0,
>         properties => {
> @@ -144,6 +154,9 @@ __PACKAGE__->register_method ({
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $cfg = {};
>         if($param->{pending}) {
>             my $running_cfg = PVE::Network::SDN::running_config();
> @@ -156,6 +169,11 @@ __PACKAGE__->register_method ({
>             $cfg = PVE::Network::SDN::Vnets::config();
>         }
>  
> +       my $zone = $cfg->{$vnet}->{zone};
> +       return if !$zone;
> +
> +       $check_vnet_access($rpcenv, $authuser, $zone, $vnet,
> ['SDN.Allocate']);
> +
>         return $api_sdn_vnets_config->($cfg, $param->{vnet});
>      }});
>  
> @@ -166,7 +184,7 @@ __PACKAGE__->register_method ({
>      method => 'POST',
>      description => "Create a new sdn vnet object.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       check => ['perm', '/sdn/zones/{zone}', ['SDN.Allocate']],
>      },
>      parameters => PVE::Network::SDN::VnetPlugin->createSchema(),
>      returns => { type => 'null' },
> @@ -210,24 +228,36 @@ __PACKAGE__->register_method ({
>      method => 'PUT',
>      description => "Update sdn vnet object configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
>      },
>      parameters => PVE::Network::SDN::VnetPlugin->updateSchema(),
>      returns => { type => 'null' },
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $id = extract_param($param, 'vnet');
>         my $digest = extract_param($param, 'digest');
>  
>         PVE::Network::SDN::lock_sdn_config(sub {
>             my $cfg = PVE::Network::SDN::Vnets::config();
>  
> -           PVE::SectionConfig::assert_if_modified($cfg, $digest);
> +           my $zone = $cfg->{ids}->{$id}->{zone} // $params->{zone};
> +           # TODO can this even happen?
> +           raise_param_exc({ zone => "missing zone" }) if !$zone;
> +
> +           $check_vnet_access($rpcenv, $authuser, $zone, $id,
> ['SDN.Allocate']);
> +
> +           if (my $new_zone = $params->{zone}) {
> +               $rpcenv->check($authuser, "/sdn/zones/$new_zone/$id",
> ['SDN.Allocate']);
> +           }
>  
> +           PVE::SectionConfig::assert_if_modified($cfg, $digest);
>  
>             my $opts = PVE::Network::SDN::VnetPlugin-
> >check_config($id, $param, 0, 1);
> -           raise_param_exc({ zone => "missing zone"}) if !$opts-
> >{zone};
>             my $subnets = PVE::Network::SDN::Vnets::get_subnets($id);
>             raise_param_exc({ zone => "can't change zone if subnets
> exists"}) if($subnets && $opts->{zone} ne $cfg->{ids}->{$id}-
> >{zone});
>  
> @@ -256,7 +286,8 @@ __PACKAGE__->register_method ({
>      method => 'DELETE',
>      description => "Delete sdn vnet object configuration.",
>      permissions => {
> -       check => ['perm', '/sdn/vnets', ['SDN.Allocate']],
> +       description => "Requires 'SDN.Allocate' permission on
> '/sdn/zones/<zone>/<vnet>'",
> +       user => 'all',
>      },
>      parameters => {
>         additionalProperties => 0,
> @@ -270,10 +301,19 @@ __PACKAGE__->register_method ({
>      code => sub {
>         my ($param) = @_;
>  
> +       my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
> +
>         my $id = extract_param($param, 'vnet');
>  
>          PVE::Network::SDN::lock_sdn_config(sub {
>             my $cfg = PVE::Network::SDN::Vnets::config();
> +           my $zone = $cfg->{ids}->{$id}->{zone};
> +           # TODO can this even happen?
> +           raise_param_exc({ zone => "missing zone" }) if !$zone;
> +
> +           $check_vnet_access($rpcenv, $authuser, $zone, $id,
> ['SDN.Allocate']);
> +
>             my $scfg =
> PVE::Network::SDN::Vnets::sdn_vnets_config($cfg, $id); # check if
> exists
>             my $vnet_cfg = PVE::Network::SDN::Vnets::config();
>  
> ```
> 
> On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> > new path is /zones/<zone>/<vnetid>
> > 
> > Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> > ---
> >  PVE/Network/SDN.pm | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/PVE/Network/SDN.pm b/PVE/Network/SDN.pm
> > index b95dd5b..1ad85e5 100644
> > --- a/PVE/Network/SDN.pm
> > +++ b/PVE/Network/SDN.pm
> > @@ -190,10 +190,10 @@ sub get_local_vnets {
> >         my $zoneid = $vnet->{zone};
> >         my $comments = $vnet->{alias};
> >  
> > -       my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> > +       my $privs = [ 'SDN.Audit', 'SDN.Use' ];
> >  
> >         next if !$zoneid;
> > -       next if !$rpcenv->check_any($authuser,
> > "/sdn/zones/$zoneid", $privs, 1) && !$rpcenv->check_any($authuser,
> > "/sdn/vnets/$vnetid", $privs, 1);
> > +       next if !$rpcenv->check_sdn_bridge($authuser, $zoneid,
> > $vnetid, $privs, 1);
> >  
> >         my $zone_config =
> > PVE::Network::SDN::Zones::sdn_zones_config($zones_cfg, $zoneid);
> >  
> > -- 
> > 2.30.2
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://antiphishing.cetsi.fr/proxy/v3?i=MUo0RzFIRTVvbFhYVGloQoloZQj6tvqyhpERsn5z8Z4&r=cFdGNHFjVENnWDEzUVliSYiK92A-8tPjy0OkrQBFKsNtwFQPVFVwvPagaFXOdIvK&f=ODlJNFRJTjZBcWFlaWxQaCCCfKFEiPqnNIdA-OFeRWhRkYGvCokAsY8PdoPF8z-Ieq4V3WNpzo4Gr8nE76YOxQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=b1p5
> > 
> > 
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=MUo0RzFIRTVvbFhYVGloQoloZQj6tvqyhpERsn5z8Z4&r=cFdGNHFjVENnWDEzUVliSYiK92A-8tPjy0OkrQBFKsNtwFQPVFVwvPagaFXOdIvK&f=ODlJNFRJTjZBcWFlaWxQaCCCfKFEiPqnNIdA-OFeRWhRkYGvCokAsY8PdoPF8z-Ieq4V3WNpzo4Gr8nE76YOxQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=b1p5
> 

[pve-devel] [PATCH v2 pve-guest-common 1/1] helpers : add check_vnet_access

[Alexandre Derumier]

if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl or zone)
if trunks is defined, we check permissions for each vlan of the trunks
if no tag, test if user have access to full bridge.

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/GuestHelpers.pm | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
index b4ccbaa..d22be1e 100644
--- a/src/PVE/GuestHelpers.pm
+++ b/src/PVE/GuestHelpers.pm
@@ -10,10 +10,17 @@ use PVE::Storage;
 use POSIX qw(strftime);
 use Scalar::Util qw(weaken);
 
+my $have_sdn;
+eval {
+    require PVE::Network::SDN;
+    $have_sdn = 1;
+};
+
 use base qw(Exporter);
 
 our @EXPORT_OK = qw(
 assert_tag_permissions
+check_vnet_access
 get_allowed_tags
 safe_boolean_ne
 safe_num_ne
@@ -366,4 +373,46 @@ sub get_unique_tags {
     return !$no_join_result ? join(';', $res->@*) : $res;
 }
 
+sub get_tags_from_trunk {
+    my ($trunks) = @_;
+
+    my $res = {};
+    my @trunks_array = split /;/, $trunks;
+    for my $trunk (@trunks_array) {
+	my ($tag, $tag_end) = split /-/, $trunk;
+	if($tag_end && $tag_end > $tag) {
+	    my @tags = ($tag..$tag_end);
+	    $res->{$_} = 1 for @tags;
+	} else {
+	    $res->{$tag} = 1;
+	}
+    }
+    return $res;
+}
+
+sub check_vnet_access {
+    my ($rpcenv, $authuser, $vnet, $tag, $trunks) = @_;
+
+    my $zone = 'localnetwork';
+
+    if ($have_sdn) {
+	my $vnet_cfg = PVE::Network::SDN::Vnets::config();
+	if (defined(my $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $vnet, 1))) {
+	    $zone = $vnet->{zone};
+	}
+    }
+
+    # if a tag is defined, test if user have a specific access to the vlan (or propagated from full bridge acl)
+    $rpcenv->check($authuser, "/sdn/zones/$zone/$vnet/$tag", ['SDN.Use']) if $tag;
+    # check each vlan access from trunk
+    if ($trunks) {
+	my $tags = get_tags_from_trunk($trunks);
+	for my $tag (sort keys %$tags) {
+	    $rpcenv->check($authuser, "/sdn/zones/$zone/$vnet/$tag", ['SDN.Use']);
+	}
+    }
+    # if no tag, test if user have access to full bridge.
+    $rpcenv->check($authuser, "/sdn/zones/$zone/$vnet", ['SDN.Use']);
+}
+
 1;
-- 
2.30.2

[pve-devel] applied: [PATCH v2 pve-guest-common 1/1] helpers : add check_vnet_access

[Fabian Grünbichler]

applied with a small fixup, I'll write the corresponding pve-container
patch on Friday.

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> if a tag is defined, test if user have a specific access to the vlan (or propagate from full bridge acl or zone)
> if trunks is defined, we check permissions for each vlan of the trunks
> if no tag, test if user have access to full bridge.
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  src/PVE/GuestHelpers.pm | 49 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
> index b4ccbaa..d22be1e 100644
> --- a/src/PVE/GuestHelpers.pm
> +++ b/src/PVE/GuestHelpers.pm
> @@ -10,10 +10,17 @@ use PVE::Storage;
>  use POSIX qw(strftime);
>  use Scalar::Util qw(weaken);
>  
> +my $have_sdn;
> +eval {
> +    require PVE::Network::SDN;
> +    $have_sdn = 1;
> +};
> +
>  use base qw(Exporter);
>  
>  our @EXPORT_OK = qw(
>  assert_tag_permissions
> +check_vnet_access
>  get_allowed_tags
>  safe_boolean_ne
>  safe_num_ne
> @@ -366,4 +373,46 @@ sub get_unique_tags {
>      return !$no_join_result ? join(';', $res->@*) : $res;
>  }
>  
> +sub get_tags_from_trunk {
> +    my ($trunks) = @_;
> +
> +    my $res = {};
> +    my @trunks_array = split /;/, $trunks;
> +    for my $trunk (@trunks_array) {
> +	my ($tag, $tag_end) = split /-/, $trunk;
> +	if($tag_end && $tag_end > $tag) {
> +	    my @tags = ($tag..$tag_end);
> +	    $res->{$_} = 1 for @tags;
> +	} else {
> +	    $res->{$tag} = 1;
> +	}
> +    }
> +    return $res;
> +}
> +
> +sub check_vnet_access {
> +    my ($rpcenv, $authuser, $vnet, $tag, $trunks) = @_;
> +
> +    my $zone = 'localnetwork';
> +
> +    if ($have_sdn) {
> +	my $vnet_cfg = PVE::Network::SDN::Vnets::config();
> +	if (defined(my $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnet_cfg, $vnet, 1))) {
> +	    $zone = $vnet->{zone};
> +	}
> +    }
> +
> +    # if a tag is defined, test if user have a specific access to the vlan (or propagated from full bridge acl)
> +    $rpcenv->check($authuser, "/sdn/zones/$zone/$vnet/$tag", ['SDN.Use']) if $tag;
> +    # check each vlan access from trunk
> +    if ($trunks) {
> +	my $tags = get_tags_from_trunk($trunks);
> +	for my $tag (sort keys %$tags) {
> +	    $rpcenv->check($authuser, "/sdn/zones/$zone/$vnet/$tag", ['SDN.Use']);
> +	}
> +    }
> +    # if no tag, test if user have access to full bridge.
> +    $rpcenv->check($authuser, "/sdn/zones/$zone/$vnet", ['SDN.Use']);
> +}
> +
>  1;
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH v3 pve-manager 2/4] api2: cluster: ressources: add "localnetwork" zone

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Cluster.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 2e942368..a7224d7f 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -474,6 +474,20 @@ __PACKAGE__->register_method({
 	    }
 	}
 
+	#add default "localnetwork" zone
+	if ($rpcenv->check($authuser, "/sdn/zones/localnetwork", [ 'SDN.Audit' ], 1)) {
+	    foreach my $node (@$nodelist) {
+		my $local_sdn = {
+		    id => "sdn/$node/localnetwork",
+		    sdn => 'localnetwork',
+		    node => $node,
+		    type => 'sdn',
+		    status => 'ok',
+	        };
+	        push @$res, $local_sdn;
+	    }
+	}
+
 	if ($have_sdn) {
 	    if (!$param->{type} || $param->{type} eq 'sdn') {
 
-- 
2.30.2

Re: [pve-devel] [PATCH v3 pve-manager 2/4] api2: cluster: ressources: add "localnetwork" zone

[Fabian Grünbichler]

I had the following fixup for this locally:

```
diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index a7224d7f3..07e5261f2 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -474,23 +474,22 @@ __PACKAGE__->register_method({
 	    }
 	}
 
-	#add default "localnetwork" zone
-	if ($rpcenv->check($authuser, "/sdn/zones/localnetwork", [ 'SDN.Audit' ], 1)) {
-	    foreach my $node (@$nodelist) {
-		my $local_sdn = {
-		    id => "sdn/$node/localnetwork",
-		    sdn => 'localnetwork',
-		    node => $node,
-		    type => 'sdn',
-		    status => 'ok',
-	        };
-	        push @$res, $local_sdn;
+	if (!$param->{type} || $param->{type} eq 'sdn') {
+	    #add default "localnetwork" zone
+	    if ($rpcenv->check($authuser, "/sdn/zones/localnetwork", [ 'SDN.Audit' ], 1)) {
+	      	foreach my $node (@$nodelist) {
+		    my $local_sdn = {
+		      	id => "sdn/$node/localnetwork",
+		      	sdn => 'localnetwork',
+		      	node => $node,
+		      	type => 'sdn',
+		      	status => 'ok',
+	            };
+	            push @$res, $local_sdn;
+	      	}
 	    }
-	}
-
-	if ($have_sdn) {
-	    if (!$param->{type} || $param->{type} eq 'sdn') {
 
+	    if ($have_sdn) {
 		my $nodes = PVE::Cluster::get_node_kv("sdn");
 
 		for my $node (sort keys %{$nodes}) {
```

but then when testing I realized that this makes a 'localnetwork' entry
appear in the tree in the GUI, but it's not possible to open it? or
maybe it needs the GUI patches that I haven't had the time to look at
yet ;) but maybe something to take into account in the next version,
didn't apply this one for now..

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Cluster.pm | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
> index 2e942368..a7224d7f 100644
> --- a/PVE/API2/Cluster.pm
> +++ b/PVE/API2/Cluster.pm
> @@ -474,6 +474,20 @@ __PACKAGE__->register_method({
>  	    }
>  	}
>  
> +	#add default "localnetwork" zone
> +	if ($rpcenv->check($authuser, "/sdn/zones/localnetwork", [ 'SDN.Audit' ], 1)) {
> +	    foreach my $node (@$nodelist) {
> +		my $local_sdn = {
> +		    id => "sdn/$node/localnetwork",
> +		    sdn => 'localnetwork',
> +		    node => $node,
> +		    type => 'sdn',
> +		    status => 'ok',
> +	        };
> +	        push @$res, $local_sdn;
> +	    }
> +	}
> +
>  	if ($have_sdn) {
>  	    if (!$param->{type} || $param->{type} eq 'sdn') {
>  
> -- 
> 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 v3 pve-manager 2/4] api2: cluster: ressources: add "localnetwork" zone

[DERUMIER, Alexandre]

Le mercredi 07 juin 2023 à 16:44 +0200, Fabian Grünbichler a écrit :
> 
> ```
> 
> but then when testing I realized that this makes a 'localnetwork'
> entry
> appear in the tree in the GUI, but it's not possible to open it? or
> maybe it needs the GUI patches that I haven't had the time to look at
> yet ;) but maybe something to take into account in the next version,
> didn't apply this one for now.

Yes, it need the gui patches ;)

because the "localnetwork" zone content api is not the same than sdn.


Note that this patch add it in cluster ressource, so expose the
"localnetwork" zone in the permissions paths combobox for permissions
management at datacenter level.

[pve-devel] [PATCH v2 pve-access-control 2/3] rpcenvironnment: add check_sdn_bridge

[Alexandre Derumier]

check if user have access to 1 vlan of the bridge
or the bridge itself

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/RPCEnvironment.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index 8586938..e0a101f 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -324,6 +324,24 @@ sub check_full {
     }
 }
 
+sub check_sdn_bridge {
+    my ($self, $username, $zone, $bridge, $privs, $noerr) = @_;
+
+    my $path = "/sdn/zones/$zone/$bridge";
+    my $cfg = $self->{user_cfg};
+    my $bridge_acl = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root}, $path);
+    if ($bridge_acl) {
+	my $vlans = $bridge_acl->{children};
+	for my $vlan (keys %$vlans) {
+	    my $vlanpath = "$path/$vlan";
+	    return 1 if $self->check_any($username, $vlanpath, $privs, $noerr);
+	}
+	# check access to bridge itself
+	return 1 if $self->check_any($username, $path, $privs, $noerr);
+    }
+    return;
+}
+
 sub check_user_enabled {
     my ($self, $user, $noerr) = @_;
 
-- 
2.30.2

[pve-devel] applied: [PATCH v2 pve-access-control 2/3] rpcenvironnment: add check_sdn_bridge

[Fabian Grünbichler]

applied this one with a bit of follow-ups, please check them out!

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> check if user have access to 1 vlan of the bridge
> or the bridge itself
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  src/PVE/RPCEnvironment.pm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 8586938..e0a101f 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -324,6 +324,24 @@ sub check_full {
>      }
>  }
>  
> +sub check_sdn_bridge {
> +    my ($self, $username, $zone, $bridge, $privs, $noerr) = @_;
> +
> +    my $path = "/sdn/zones/$zone/$bridge";
> +    my $cfg = $self->{user_cfg};
> +    my $bridge_acl = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root}, $path);
> +    if ($bridge_acl) {
> +	my $vlans = $bridge_acl->{children};
> +	for my $vlan (keys %$vlans) {
> +	    my $vlanpath = "$path/$vlan";
> +	    return 1 if $self->check_any($username, $vlanpath, $privs, $noerr);
> +	}
> +	# check access to bridge itself
> +	return 1 if $self->check_any($username, $path, $privs, $noerr);
> +    }
> +    return;
> +}
> +
>  sub check_user_enabled {
>      my ($self, $user, $noerr) = @_;
>  
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH v2 pve-access-control 3/3] add new SDN.use privilege in PVESDNUser role

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/AccessControl.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 6a3d203..326ed5c 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1131,6 +1131,9 @@ my $privgroups = {
 	    'SDN.Allocate',
 	    'SDN.Audit',
 	],
+	user => [
+	    'SDN.Use',
+	],
 	audit => [
 	    'SDN.Audit',
 	],
-- 
2.30.2

[pve-devel] applied: [PATCH v2 pve-access-control 3/3] add new SDN.use privilege in PVESDNUser role

[Fabian Grünbichler]

applied this one as well (with a small fixup for a test case)

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  src/PVE/AccessControl.pm | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 6a3d203..326ed5c 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1131,6 +1131,9 @@ my $privgroups = {
>  	    'SDN.Allocate',
>  	    'SDN.Audit',
>  	],
> +	user => [
> +	    'SDN.Use',
> +	],
>  	audit => [
>  	    'SDN.Audit',
>  	],
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH v3 pve-manager 3/4] ui: add vnet permissions panel

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 www/manager6/Makefile                |   2 +
 www/manager6/sdn/Browser.js          |  17 +-
 www/manager6/sdn/VnetACLView.js      | 289 +++++++++++++++++++++++++++
 www/manager6/sdn/ZoneContentPanel.js |  41 ++++
 www/manager6/sdn/ZoneContentView.js  |  25 ++-
 5 files changed, 356 insertions(+), 18 deletions(-)
 create mode 100644 www/manager6/sdn/VnetACLView.js
 create mode 100644 www/manager6/sdn/ZoneContentPanel.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 2b577c8e..fb9930af 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -251,10 +251,12 @@ JSSRC= 							\
 	sdn/StatusView.js				\
 	sdn/VnetEdit.js					\
 	sdn/VnetView.js					\
+	sdn/VnetACLView.js					\
 	sdn/VnetPanel.js				\
 	sdn/SubnetEdit.js				\
 	sdn/SubnetView.js				\
 	sdn/ZoneContentView.js				\
+	sdn/ZoneContentPanel.js				\
 	sdn/ZoneView.js					\
 	sdn/OptionsPanel.js				\
 	sdn/controllers/Base.js				\
diff --git a/www/manager6/sdn/Browser.js b/www/manager6/sdn/Browser.js
index 09b0c4fe..3dc5a5ad 100644
--- a/www/manager6/sdn/Browser.js
+++ b/www/manager6/sdn/Browser.js
@@ -25,14 +25,15 @@ Ext.define('PVE.sdn.Browser', {
 
 	const caps = Ext.state.Manager.get('GuiCap');
 
-	if (caps.sdn['SDN.Audit']) {
-	    me.items.push({
-		xtype: 'pveSDNZoneContentView',
-		title: gettext('Content'),
-		iconCls: 'fa fa-th',
-		itemId: 'content',
-	    });
-	}
+	me.items.push({
+	    nodename: nodename,
+	    zone: sdnId,
+	    xtype: 'pveSDNZoneContentPanel',
+	    title: gettext('Content'),
+	    iconCls: 'fa fa-th',
+	    itemId: 'content',
+	});
+
 	if (caps.sdn['Permissions.Modify']) {
 	    me.items.push({
 		xtype: 'pveACLView',
diff --git a/www/manager6/sdn/VnetACLView.js b/www/manager6/sdn/VnetACLView.js
new file mode 100644
index 00000000..af10d954
--- /dev/null
+++ b/www/manager6/sdn/VnetACLView.js
@@ -0,0 +1,289 @@
+Ext.define('PVE.sdn.VnetACLAdd', {
+    extend: 'Proxmox.window.Edit',
+    alias: ['widget.pveSDNVnetACLAdd'],
+
+    url: '/access/acl',
+    method: 'PUT',
+    isAdd: true,
+    isCreate: true,
+
+    width: 400,
+    initComponent: function() {
+        let me = this;
+
+	let items = [
+	    {
+		xtype: 'hiddenfield',
+		name: 'path',
+		value: me.path,
+		allowBlank: false,
+		fieldLabel: gettext('Path'),
+	    },
+	];
+
+	if (me.aclType === 'group') {
+	    me.subject = gettext("Group Permission");
+	    items.push({
+		xtype: 'pveGroupSelector',
+		name: 'groups',
+		fieldLabel: gettext('Group'),
+	    });
+	} else if (me.aclType === 'user') {
+	    me.subject = gettext("User Permission");
+	    items.push({
+		xtype: 'pmxUserSelector',
+		name: 'users',
+		fieldLabel: gettext('User'),
+	    });
+	} else if (me.aclType === 'token') {
+	    me.subject = gettext("API Token Permission");
+	    items.push({
+		xtype: 'pveTokenSelector',
+		name: 'tokens',
+		fieldLabel: gettext('API Token'),
+	    });
+	} else {
+	    throw "unknown ACL type";
+	}
+
+	items.push({
+	    xtype: 'pmxRoleSelector',
+	    name: 'roles',
+	    value: 'NoAccess',
+	    fieldLabel: gettext('Role'),
+	});
+
+	items.push({
+	    xtype: 'proxmoxintegerfield',
+	    name: 'vlan',
+	    minValue: 1,
+	    maxValue: 4096,
+            allowBlank: true,
+	    fieldLabel: 'Vlan',
+	    emptyText: gettext('All'),
+	});
+
+	let ipanel = Ext.create('Proxmox.panel.InputPanel', {
+	    items: items,
+	    onlineHelp: 'pveum_permission_management',
+	    onGetValues: function(values) {
+		if (values.vlan) {
+		    values.path = values.path + "/" + values.vlan;
+		    delete values.vlan;
+		}
+		return values;
+	    },
+	});
+
+	Ext.apply(me, {
+	    items: [ipanel],
+	});
+
+	me.callParent();
+    },
+});
+
+Ext.define('PVE.sdn.VnetACLView', {
+    extend: 'Ext.grid.GridPanel',
+
+    alias: ['widget.pveSDNVnetACLView'],
+
+    onlineHelp: 'chapter_user_management',
+
+    stateful: true,
+    stateId: 'grid-acls',
+
+    // use fixed path
+    path: undefined,
+
+    setPath: function(path) {
+        let me = this;
+
+        me.path = path;
+
+        if (path === undefined) {
+	    me.down('#groupmenu').setDisabled(true);
+	    me.down('#usermenu').setDisabled(true);
+	    me.down('#tokenmenu').setDisabled(true);
+        } else {
+	    me.down('#groupmenu').setDisabled(false);
+	    me.down('#usermenu').setDisabled(false);
+	    me.down('#tokenmenu').setDisabled(false);
+            me.store.load();
+        }
+    },
+    initComponent: function() {
+	let me = this;
+
+	let store = Ext.create('Ext.data.Store', {
+	    model: 'pve-acl',
+	    proxy: {
+                type: 'proxmox',
+		url: "/api2/json/access/acl",
+	    },
+	    sorters: {
+		property: 'path',
+		direction: 'ASC',
+	    },
+	});
+
+	store.addFilter(Ext.create('Ext.util.Filter', {
+	    filterFn: item => item.data.path.replace(/(\/sdn\/zones\/(.*)\/(.*))\/[0-9]*$/, '$1') === me.path,
+	}));
+
+	let render_ugid = function(ugid, metaData, record) {
+	    if (record.data.type === 'group') {
+		return '@' + ugid;
+	    }
+
+	    return Ext.String.htmlEncode(ugid);
+	};
+
+	let render_vlan = function(path, metaData, record) {
+	    let vlan = 'any';
+	    const match = path.match(/(\/sdn\/zones\/)(.*)\/(.*)\/([0-9]*)$/);
+	    if (match) {
+		vlan = match[4];
+	    }
+
+	    return Ext.String.htmlEncode(vlan);
+	};
+
+	let columns = [
+	    {
+		header: gettext('User') + '/' + gettext('Group') + '/' + gettext('API Token'),
+		flex: 1,
+		sortable: true,
+		renderer: render_ugid,
+		dataIndex: 'ugid',
+	    },
+	    {
+		header: gettext('Role'),
+		flex: 1,
+		sortable: true,
+		dataIndex: 'roleid',
+	    },
+	    {
+		header: gettext('Vlan'),
+		flex: 1,
+		sortable: true,
+		renderer: render_vlan,
+		dataIndex: 'path',
+	    },
+	];
+
+
+	let sm = Ext.create('Ext.selection.RowModel', {});
+
+	let remove_btn = new Proxmox.button.Button({
+	    text: gettext('Remove'),
+	    disabled: true,
+	    selModel: sm,
+	    confirmMsg: gettext('Are you sure you want to remove this entry'),
+	    handler: function(btn, event, rec) {
+		var params = {
+		    'delete': 1,
+		    path: rec.data.path,
+		    roles: rec.data.roleid,
+		};
+		if (rec.data.type === 'group') {
+		    params.groups = rec.data.ugid;
+		} else if (rec.data.type === 'user') {
+		    params.users = rec.data.ugid;
+		} else if (rec.data.type === 'token') {
+		    params.tokens = rec.data.ugid;
+		} else {
+		    throw 'unknown data type';
+		}
+
+		Proxmox.Utils.API2Request({
+		    url: '/access/acl',
+		    params: params,
+		    method: 'PUT',
+		    waitMsgTarget: me,
+		    callback: () => store.load(),
+		    failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
+		});
+	    },
+	});
+
+	Proxmox.Utils.monStoreErrors(me, store);
+
+	Ext.apply(me, {
+	    store: store,
+	    selModel: sm,
+	    tbar: [
+		{
+		    text: gettext('Add'),
+		    menu: {
+			xtype: 'menu',
+			items: [
+			    {
+				text: gettext('Group Permission'),
+				disabled: !me.path,
+				itemId: 'groupmenu',
+				iconCls: 'fa fa-fw fa-group',
+				handler: function() {
+				    var win = Ext.create('PVE.sdn.VnetACLAdd', {
+					aclType: 'group',
+					path: me.path,
+				    });
+				    win.on('destroy', () => store.load());
+				    win.show();
+				},
+			    },
+			    {
+				text: gettext('User Permission'),
+				disabled: !me.path,
+				itemId: 'usermenu',
+				iconCls: 'fa fa-fw fa-user',
+				handler: function() {
+				    var win = Ext.create('PVE.sdn.VnetACLAdd', {
+					aclType: 'user',
+					path: me.path,
+				    });
+				    win.on('destroy', () => store.load());
+				    win.show();
+				},
+			    },
+			    {
+				text: gettext('API Token Permission'),
+				disabled: !me.path,
+				itemId: 'tokenmenu',
+				iconCls: 'fa fa-fw fa-user-o',
+				handler: function() {
+				    let win = Ext.create('PVE.sdn.VnetACLAdd', {
+					aclType: 'token',
+					path: me.path,
+				    });
+				    win.on('destroy', () => store.load());
+				    win.show();
+				},
+			    },
+			],
+		    },
+		},
+		remove_btn,
+	    ],
+	    viewConfig: {
+		trackOver: false,
+	    },
+	    columns: columns,
+	    listeners: {
+	    },
+	});
+
+	me.callParent();
+    },
+}, function() {
+    Ext.define('pve-acl-vnet', {
+	extend: 'Ext.data.Model',
+	fields: [
+	    'path', 'type', 'ugid', 'roleid',
+	    {
+		name: 'propagate',
+		type: 'boolean',
+	    },
+	],
+    });
+});
diff --git a/www/manager6/sdn/ZoneContentPanel.js b/www/manager6/sdn/ZoneContentPanel.js
new file mode 100644
index 00000000..5bb081bb
--- /dev/null
+++ b/www/manager6/sdn/ZoneContentPanel.js
@@ -0,0 +1,41 @@
+Ext.define('PVE.sdn.ZoneContentPanel', {
+    extend: 'Ext.panel.Panel',
+    alias: 'widget.pveSDNZoneContentPanel',
+
+    title: 'Vnet',
+
+    onlineHelp: 'pvesdn_config_vnet',
+
+    initComponent: function() {
+	var me = this;
+
+	var permissions_panel = Ext.createWidget('pveSDNVnetACLView', {
+	    title: gettext('Vnet Permissions'),
+	    region: 'center',
+	    border: false,
+	});
+
+	var vnetview_panel = Ext.createWidget('pveSDNZoneContentView', {
+	    title: 'Vnets',
+	    region: 'west',
+	    permissions_panel: permissions_panel,
+	    nodename: me.nodename,
+	    zone: me.zone,
+	    width: '50%',
+	    border: false,
+	    split: true,
+	});
+
+	Ext.apply(me, {
+	    layout: 'border',
+	    items: [vnetview_panel, permissions_panel],
+	    listeners: {
+		show: function() {
+		    permissions_panel.fireEvent('show', permissions_panel);
+		},
+	    },
+	});
+
+	me.callParent();
+    },
+});
diff --git a/www/manager6/sdn/ZoneContentView.js b/www/manager6/sdn/ZoneContentView.js
index 1ea65450..4bc92718 100644
--- a/www/manager6/sdn/ZoneContentView.js
+++ b/www/manager6/sdn/ZoneContentView.js
@@ -17,17 +17,15 @@ Ext.define('PVE.sdn.ZoneContentView', {
     initComponent: function() {
 	var me = this;
 
-	var nodename = me.pveSelNode.data.node;
-	if (!nodename) {
+	if (!me.nodename) {
 	    throw "no node name specified";
 	}
 
-	var zone = me.pveSelNode.data.sdn;
-	if (!zone) {
+	if (!me.zone) {
 	    throw "no zone ID specified";
 	}
 
-	var baseurl = "/nodes/" + nodename + "/sdn/zones/" + zone + "/content";
+	var baseurl = "/nodes/" + me.nodename + "/sdn/zones/" + me.zone + "/content";
 	var store = Ext.create('Ext.data.Store', {
 	    model: 'pve-sdnzone-content',
 	    groupField: 'content',
@@ -48,7 +46,6 @@ Ext.define('PVE.sdn.ZoneContentView', {
 	};
 
 	Proxmox.Utils.monStoreErrors(me, store);
-
 	Ext.apply(me, {
 	    store: store,
 	    selModel: sm,
@@ -79,11 +76,19 @@ Ext.define('PVE.sdn.ZoneContentView', {
 		    dataIndex: 'statusmsg',
 		},
 	    ],
-	    listeners: {
-		activate: reload,
-	    },
+            listeners: {
+                activate: reload,
+                show: reload,
+                select: function(_sm, rec) {
+                    let path = `/sdn/zones/${me.zone}/${rec.data.vnet}`;
+                    me.permissions_panel.setPath(path);
+                },
+                deselect: function() {
+                    me.permissions_panel.setPath(undefined);
+                },
+            },
 	});
-
+	store.load();
 	me.callParent();
     },
 }, function() {
-- 
2.30.2

[pve-devel] [PATCH v3 pve-manager 4/4] ui: add permissions management for "localnetwork" zone

[Alexandre Derumier]

add a default virtual zone called 'localnetwork' in the ressource tree,
and handle permissions like a true sdn zone

(no conflict with true sdn zone is possible, as they have 8 characters max)

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 www/manager6/sdn/ZoneContentView.js | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/www/manager6/sdn/ZoneContentView.js b/www/manager6/sdn/ZoneContentView.js
index 4bc92718..2a3cbf52 100644
--- a/www/manager6/sdn/ZoneContentView.js
+++ b/www/manager6/sdn/ZoneContentView.js
@@ -26,6 +26,9 @@ Ext.define('PVE.sdn.ZoneContentView', {
 	}
 
 	var baseurl = "/nodes/" + me.nodename + "/sdn/zones/" + me.zone + "/content";
+	if (me.zone === 'localnetwork') {
+	    baseurl = "/nodes/" + me.nodename + "/network?type=any_local_bridge";
+	}
 	var store = Ext.create('Ext.data.Store', {
 	    model: 'pve-sdnzone-content',
 	    groupField: 'content',
@@ -95,7 +98,29 @@ Ext.define('PVE.sdn.ZoneContentView', {
     Ext.define('pve-sdnzone-content', {
 	extend: 'Ext.data.Model',
 	fields: [
-	    'vnet', 'status', 'statusmsg',
+	    {
+		name: 'iface',
+		convert: function(value, record) {
+		    //map local vmbr to vnet
+		    if (record.data.iface) {
+			record.data.vnet = record.data.iface;
+		    }
+		    return value;
+		},
+	    },
+	    {
+		name: 'comments',
+		convert: function(value, record) {
+		    //map local vmbr comments to vnet alias
+		    if (record.data.comments) {
+			record.data.alias = record.data.comments;
+		    }
+		    return value;
+		},
+	    },
+	    'vnet',
+	    'status',
+	    'statusmsg',
 	    {
 		name: 'text',
 		convert: function(value, record) {
-- 
2.30.2

[pve-devel] applied-series: [PATCH-SERIE pve-access-control/pve-manager/pve-guest-common/qemu-server/pve-network] check permissions on local bridge

[Fabian Grünbichler]

applied the pve-manager and pve-network patches (and your pve-network
follow-up, plus some fixes of my own).

some more things that might be worthy of a follow-up:
- for the ACL panel of a zone, also displaying the vnet + vlan ACLs
  might be nice
- for the ACL panel fo a vnet, also displaying zone ACLs of the vnet
  zone might be nice

this would be similar to how we display datastore permissions in the PBS
GUI (I know that other specific permission views in PVE are also lacking
this, but for VNETs it's especially visible ;))

- the ACL Add button is not correctly initialized with a disabled state
  before the first vnet is selected

On June 7, 2023 2:03 pm, Alexandre Derumier wrote:
> add vnet/localbridge permissions management
> 
> Hi,
> as we has discuted some weeks ago,
> this patche serie introduce management of acl for vnets && local bridges
> 
> The permission path is:
> 
> /sdn/zones/<zone>/<vnet>
> 
> where the local vmbr are in a virtual "localnetwork" zone
> 
> /sdn/zones/localnetwork/<vnet>
> 
> Vlans permissions  are also handled with
> /sdn/zones/<zone>/<vnet>/<tag>
> 
> if user have permissions on the vnet/tag, he have access to only the specific vlan.
> if user have permissions on the vnet with propagate, he have access to all vlans of the vnet
> if user have permissions on the vnet without propagate, he have access to bridge only without any vlan
> 
> 
> I have reworked the sdn zone panel from the tree, to manage permissions
> on displayed vnets. (patch 3 && 4 pve-manager)
> 
> some screenshots:
> 
> https://mutulin1.odiso.net/sdnzone-perm.png
> https://mutulin1.odiso.net/localzone-perm.png
> 
> 
> 
> changelog v2:
>  - use /vnets/vlan instead /vnets.vlan
>  - rework the bridge filtering when user have access only to a specific vlan
>  - api2 network: always check bridge access if no filter is defined
> 
> changelog v3:
>  - use /sdn/zones/<zone>/vnets/vlan instead /sdn/vnets/vnets.vlan
>  - add SDN.Use permission
>  - pve-manager: split ui code (could be applied later)
>  - remove check on zone (it's now propagate with new path)
>  - move check_vnet_access to pve-guest-common for lxc reuse
>  - pve-network: fix vnet/tag perm check
> 
> changelog v4:
>  - qemu-server: check permissions on backup restore
>  - guest-common: check trunks permissions
> 
> todo:
>  - implement lxc check permissions
> 
> 
> 
> pve-access-control:
> 
> Alexandre Derumier (3):
>   access control: add /sdn/zones/<zone>/<vnet>/<vlan> path
>   rpcenvironnment: add check_sdn_bridge
>   add new SDN.use privilege in PVESDNUser role
> 
>  src/PVE/AccessControl.pm  |  6 +++++-
>  src/PVE/RPCEnvironment.pm | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> pve-manager:
> 
> Alexandre Derumier (4):
>   api2: network: check permissions for local bridges
>   api2: cluster: ressources: add "localnetwork" zone
>   ui: add vnet permissions panel
>   ui: add permissions management for "localnetwork" zone
> 
>  PVE/API2/Cluster.pm                  |  14 ++
>  PVE/API2/Network.pm                  |  25 ++-
>  www/manager6/Makefile                |   2 +
>  www/manager6/sdn/Browser.js          |  17 +-
>  www/manager6/sdn/VnetACLView.js      | 289 +++++++++++++++++++++++++++
>  www/manager6/sdn/ZoneContentPanel.js |  41 ++++
>  www/manager6/sdn/ZoneContentView.js  |  52 ++++-
>  7 files changed, 411 insertions(+), 29 deletions(-)
>  create mode 100644 www/manager6/sdn/VnetACLView.js
>  create mode 100644 www/manager6/sdn/ZoneContentPanel.js
> 
> pve-guest-common:
> 
> Alexandre Derumier (1):
>   helpers : add check_vnet_access
> 
>  src/PVE/GuestHelpers.pm | 49 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> qemu-server:
> 
> Alexandre Derumier (1):
>   api2: add check_bridge_access for create/update/clone/restore vm
> 
>  PVE/API2/Qemu.pm | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> 
> pve-network:
> 
> Alexandre Derumier (1):
>   get_local_vnets: fix permission path && perm
> 
>  PVE/Network/SDN.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH-SERIE pve-access-control/pve-manager/pve-guest-common/qemu-server/pve-network] check permissions on local bridge

[Alexandre Derumier]

add vnet/localbridge permissions management

Hi,
as we has discuted some weeks ago,
this patche serie introduce management of acl for vnets && local bridges

The permission path is:

/sdn/zones/<zone>/<vnet>

where the local vmbr are in a virtual "localnetwork" zone

/sdn/zones/localnetwork/<vnet>

Vlans permissions  are also handled with
/sdn/zones/<zone>/<vnet>/<tag>

if user have permissions on the vnet/tag, he have access to only the specific vlan.
if user have permissions on the vnet with propagate, he have access to all vlans of the vnet
if user have permissions on the vnet without propagate, he have access to bridge only without any vlan


I have reworked the sdn zone panel from the tree, to manage permissions
on displayed vnets. (patch 3 && 4 pve-manager)

some screenshots:

https://mutulin1.odiso.net/sdnzone-perm.png
https://mutulin1.odiso.net/localzone-perm.png



changelog v2:
 - use /vnets/vlan instead /vnets.vlan
 - rework the bridge filtering when user have access only to a specific vlan
 - api2 network: always check bridge access if no filter is defined

changelog v3:
 - use /sdn/zones/<zone>/vnets/vlan instead /sdn/vnets/vnets.vlan
 - add SDN.Use permission
 - pve-manager: split ui code (could be applied later)
 - remove check on zone (it's now propagate with new path)
 - move check_vnet_access to pve-guest-common for lxc reuse
 - pve-network: fix vnet/tag perm check

todo:
 - implement check permissions on trunks
 - qemu : check bridge permissions on restore. (how to check restore config before restore ?)
 - lxc: check bridge permissions


pve-access-control:

Alexandre Derumier (3):
  access control: add /sdn/zones/<zone>/<vnet>/<vlan> path
  rpcenvironnment: add check_sdn_bridge
  add new SDN.use privilege in PVESDNUser role

 src/PVE/AccessControl.pm  |  6 +++++-
 src/PVE/RPCEnvironment.pm | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

pve-manager:

Alexandre Derumier (4):
  api2: network: check permissions for local bridges
  api2: cluster: ressources: add "localnetwork" zone
  ui: add vnet permissions panel
  ui: add permissions management for "localnetwork" zone

 PVE/API2/Cluster.pm                  |  14 ++
 PVE/API2/Network.pm                  |  25 ++-
 www/manager6/Makefile                |   2 +
 www/manager6/sdn/Browser.js          |  17 +-
 www/manager6/sdn/VnetACLView.js      | 289 +++++++++++++++++++++++++++
 www/manager6/sdn/ZoneContentPanel.js |  41 ++++
 www/manager6/sdn/ZoneContentView.js  |  52 ++++-
 7 files changed, 411 insertions(+), 29 deletions(-)
 create mode 100644 www/manager6/sdn/VnetACLView.js
 create mode 100644 www/manager6/sdn/ZoneContentPanel.js


pve-guest-common:

Alexandre Derumier (1):
  helpers : add check_vnet_access

 src/PVE/GuestHelpers.pm | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

qemu-server:

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

 PVE/API2/Qemu.pm | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

pve-network:

Alexandre Derumier (1):
  get_local_vnets: fix permission path && perm

 PVE/Network/SDN.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH pve-network 1/1] get_local_vnets: fix permission path && perm

[Alexandre Derumier]

new path is /zones/<zone>/<vnetid>

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/Network/SDN.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/Network/SDN.pm b/PVE/Network/SDN.pm
index b95dd5b..1ad85e5 100644
--- a/PVE/Network/SDN.pm
+++ b/PVE/Network/SDN.pm
@@ -190,10 +190,10 @@ sub get_local_vnets {
 	my $zoneid = $vnet->{zone};
 	my $comments = $vnet->{alias};
 
-	my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
+	my $privs = [ 'SDN.Audit', 'SDN.Use' ];
 
 	next if !$zoneid;
-	next if !$rpcenv->check_any($authuser, "/sdn/zones/$zoneid", $privs, 1) && !$rpcenv->check_any($authuser, "/sdn/vnets/$vnetid", $privs, 1);
+	next if !$rpcenv->check_sdn_bridge($authuser, $zoneid, $vnetid, $privs, 1);
 
 	my $zone_config = PVE::Network::SDN::Zones::sdn_zones_config($zones_cfg, $zoneid);
 
-- 
2.30.2