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

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

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