[pve-devel] [PATCH pve-manager 0/2] sdn: permissions improvments

[pve-devel] [PATCH pve-manager 0/2] sdn: permissions improvments

[Alexandre Derumier]

- display zones list in global permissions management pathselector
- remove vmbrX bridges from bridgeselector if user have permissions on vnets

Alexandre Derumier (2):
  permpathstore: add sdn zones
  api2 : network: anybridge: don't display bridges if user have access
    to vnets.

 PVE/API2/Network.pm                | 19 ++++++++++++-------
 www/manager6/data/PermPathStore.js |  3 +++
 2 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH pve-manager 1/1] api2 : network: anybridge: don't display bridges if user have access to vnets.

[Alexandre Derumier]

This remove vmbr* from bridgeselector if user have access to vnets.
(as currently, we don't have have permission management on vmbr$)

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

diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index a26f36d2..02bd3bdb 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -226,6 +226,7 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
 
 	my $tmp = PVE::INotify::read_file('interfaces', 1);
 	my $config = $tmp->{data};
@@ -238,20 +239,24 @@ __PACKAGE__->register_method({
 	delete $ifaces->{lo}; # do not list the loopback device
 
 	if ($param->{type}) {
+	    my $vnets = {};
+	    my $filtered_sdn = undef;
+	    if ($have_sdn && $param->{type} eq 'any_bridge') {
+		$vnets = PVE::Network::SDN::get_local_vnets();
+		$filtered_sdn = 1 if $authuser ne 'root@pam' && keys %{$vnets} > 0;
+	    }
+
 	    foreach my $k (keys %$ifaces) {
 		my $type = $ifaces->{$k}->{type};
 		my $match =  ($param->{type} eq $type) || (
 		    ($param->{type} eq 'any_bridge') && 
 		    ($type eq 'bridge' || $type eq 'OVSBridge'));
-		delete $ifaces->{$k} if !$match;
+		delete $ifaces->{$k} if !$match || $filtered_sdn;
 	    }
 
-	    if ($have_sdn && $param->{type} eq 'any_bridge') {
-		my $vnets = PVE::Network::SDN::get_local_vnets();
-		map {
-		    $ifaces->{$_} = $vnets->{$_};
-		} keys %$vnets;
-	    }
+	    map {
+	    	$ifaces->{$_} = $vnets->{$_};
+	    } keys %$vnets;
 	}
 
 	return PVE::RESTHandler::hash_to_array($ifaces, 'iface');
-- 
2.30.2

[pve-devel] [PATCH pve-manager 1/2] permpathstore: add sdn zones

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 www/manager6/data/PermPathStore.js | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/www/manager6/data/PermPathStore.js b/www/manager6/data/PermPathStore.js
index 1dc276b6..cf702c03 100644
--- a/www/manager6/data/PermPathStore.js
+++ b/www/manager6/data/PermPathStore.js
@@ -10,6 +10,7 @@ Ext.define('PVE.data.PermPathStore', {
 	{ 'value': '/access/realm' },
 	{ 'value': '/nodes' },
 	{ 'value': '/pool' },
+	{ 'value': '/sdn/zones' },
 	{ 'value': '/storage' },
 	{ 'value': '/vms' },
     ],
@@ -32,6 +33,8 @@ Ext.define('PVE.data.PermPathStore', {
 		    break;
 		case 'lxc': path = '/vms/' + record.get('vmid');
 		    break;
+		case 'sdn': path = '/sdn/zones/' + record.get('sdn');
+		    break;
 		case 'storage': path = '/storage/' + record.get('storage');
 		    break;
 		case 'pool': path = '/pool/' + record.get('pool');
-- 
2.30.2

Re: [pve-devel] [PATCH pve-manager 1/2] permpathstore: add sdn zones

[Thomas Lamprecht]

On 05/08/2021 16:58, Alexandre Derumier wrote:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  www/manager6/data/PermPathStore.js | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/www/manager6/data/PermPathStore.js b/www/manager6/data/PermPathStore.js
> index 1dc276b6..cf702c03 100644
> --- a/www/manager6/data/PermPathStore.js
> +++ b/www/manager6/data/PermPathStore.js
> @@ -10,6 +10,7 @@ Ext.define('PVE.data.PermPathStore', {
>  	{ 'value': '/access/realm' },
>  	{ 'value': '/nodes' },
>  	{ 'value': '/pool' },
> +	{ 'value': '/sdn/zones' },

Would need guarding similar to how we hide other SDN stuff for now, else it could be
confusing to some. I.e., adding something like the following stanza in the constructor
between the suspendEvents and resumeEvents calls:

if (PVE.SDNInfo) {
  me.add({ value: '/sdn/zones' });
}



>  	{ 'value': '/storage' },
>  	{ 'value': '/vms' },
>      ],
> @@ -32,6 +33,8 @@ Ext.define('PVE.data.PermPathStore', {
>  		    break;
>  		case 'lxc': path = '/vms/' + record.get('vmid');
>  		    break;
> +		case 'sdn': path = '/sdn/zones/' + record.get('sdn');
> +		    break;
>  		case 'storage': path = '/storage/' + record.get('storage');
>  		    break;
>  		case 'pool': path = '/pool/' + record.get('pool');
> 

[pve-devel] [PATCH pve-manager 2/2] api2 : network: anybridge: don't display bridges if user have access to vnets.

[Alexandre Derumier]

This remove vmbr* from bridgeselector if user have access to vnets.
(as currently, we don't have have permission management on vmbr$)

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

diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index a26f36d2..02bd3bdb 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -226,6 +226,7 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
 
 	my $tmp = PVE::INotify::read_file('interfaces', 1);
 	my $config = $tmp->{data};
@@ -238,20 +239,24 @@ __PACKAGE__->register_method({
 	delete $ifaces->{lo}; # do not list the loopback device
 
 	if ($param->{type}) {
+	    my $vnets = {};
+	    my $filtered_sdn = undef;
+	    if ($have_sdn && $param->{type} eq 'any_bridge') {
+		$vnets = PVE::Network::SDN::get_local_vnets();
+		$filtered_sdn = 1 if $authuser ne 'root@pam' && keys %{$vnets} > 0;
+	    }
+
 	    foreach my $k (keys %$ifaces) {
 		my $type = $ifaces->{$k}->{type};
 		my $match =  ($param->{type} eq $type) || (
 		    ($param->{type} eq 'any_bridge') && 
 		    ($type eq 'bridge' || $type eq 'OVSBridge'));
-		delete $ifaces->{$k} if !$match;
+		delete $ifaces->{$k} if !$match || $filtered_sdn;
 	    }
 
-	    if ($have_sdn && $param->{type} eq 'any_bridge') {
-		my $vnets = PVE::Network::SDN::get_local_vnets();
-		map {
-		    $ifaces->{$_} = $vnets->{$_};
-		} keys %$vnets;
-	    }
+	    map {
+	    	$ifaces->{$_} = $vnets->{$_};
+	    } keys %$vnets;
 	}
 
 	return PVE::RESTHandler::hash_to_array($ifaces, 'iface');
-- 
2.30.2

Re: [pve-devel] [PATCH pve-manager 2/2] api2 : network: anybridge: don't display bridges if user have access to vnets.

[alexandre derumier]

Hi,
abou this patch, I'm not sure it's the right way, a forum user request
that also sdnadmin can view vmbrX.

I don't known how to hide correctly vmbrX bridge, as currently , we
don't have any permissions management,
and I don't want to break current users setup.

maybe could we add a special permission like "noaccess" with path like
/bridge/vmbrX  ?
(we currently have a role "noaccess", but it's simply a role without
any permission.


Le jeudi 05 août 2021 à 16:59 +0200, Alexandre Derumier a écrit :
> This remove vmbr* from bridgeselector if user have access to vnets.
> (as currently, we don't have have permission management on vmbr$)
> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/API2/Network.pm | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> index a26f36d2..02bd3bdb 100644
> --- a/PVE/API2/Network.pm
> +++ b/PVE/API2/Network.pm
> @@ -226,6 +226,7 @@ __PACKAGE__->register_method({
>         my ($param) = @_;
>  
>         my $rpcenv = PVE::RPCEnvironment::get();
> +       my $authuser = $rpcenv->get_user();
>  
>         my $tmp = PVE::INotify::read_file('interfaces', 1);
>         my $config = $tmp->{data};
> @@ -238,20 +239,24 @@ __PACKAGE__->register_method({
>         delete $ifaces->{lo}; # do not list the loopback device
>  
>         if ($param->{type}) {
> +           my $vnets = {};
> +           my $filtered_sdn = undef;
> +           if ($have_sdn && $param->{type} eq 'any_bridge') {
> +               $vnets = PVE::Network::SDN::get_local_vnets();
> +               $filtered_sdn = 1 if $authuser ne 'root@pam' && keys
> %{$vnets} > 0;
> +           }
> +
>             foreach my $k (keys %$ifaces) {
>                 my $type = $ifaces->{$k}->{type};
>                 my $match =  ($param->{type} eq $type) || (
>                     ($param->{type} eq 'any_bridge') && 
>                     ($type eq 'bridge' || $type eq 'OVSBridge'));
> -               delete $ifaces->{$k} if !$match;
> +               delete $ifaces->{$k} if !$match || $filtered_sdn;
>             }
>  
> -           if ($have_sdn && $param->{type} eq 'any_bridge') {
> -               my $vnets = PVE::Network::SDN::get_local_vnets();
> -               map {
> -                   $ifaces->{$_} = $vnets->{$_};
> -               } keys %$vnets;
> -           }
> +           map {
> +               $ifaces->{$_} = $vnets->{$_};
> +           } keys %$vnets;
>         }
>  
>         return PVE::RESTHandler::hash_to_array($ifaces, 'iface');

Re: [pve-devel] [PATCH pve-manager 2/2] api2 : network: anybridge: don't display bridges if user have access to vnets.

[Thomas Lamprecht]

On 07/08/2021 18:00, alexandre derumier wrote:
> Hi,
> abou this patch, I'm not sure it's the right way, a forum user request
> that also sdnadmin can view vmbrX.
> 
> I don't known how to hide correctly vmbrX bridge, as currently , we
> don't have any permissions management,
> and I don't want to break current users setup.
> 
> maybe could we add a special permission like "noaccess" with path like
> /bridge/vmbrX  ?
> (we currently have a role "noaccess", but it's simply a role without
> any permission.
> 

We do not have "revoking privileges", only empty roles so adding that would
change the whole ACL system quite a bit and IMO make it a bit more confusing that
it already can be.

Currently we handle giving more permissions in general but less permissions
on a specific ACL object by just always using the more restrictive ACL
configurations for specific paths.
I.e., if one configures Admin on / with propagate and an empty role (like
NoAccess) on /node/foo then they have no Admin privs on the foo node.

I.e., you do not need to invent a negative priv. simply setting a empty or less
privileged role on a more specific path is enough.

So rather the IMO a bit more integrated way of achieving that would be adding
a `/nodes/<node-id>/iface/<iface-id>` ACL path or the like. I mean, if we'd
be sure it would be only always a bridge then we could avoid making that path
node specific, as for them we assume that the one with the same ID are network
wise "the same" already on different nodes in a cluster.

I mean, we could also just see bridges as a vnet and handle them with the
`/sdn/vnets/<id>` path? Checking for every bridge/vnet if some priv. is
available, we probably would need two privileges, one for "allowed to use"
and a "allowed to modify" priv.

(disclaimer, fresh back from vacation and did not really looked into this in
detail, so just holler if something is stupid or not fitting the existing
permission stuff of SDN :))

> 
> Le jeudi 05 août 2021 à 16:59 +0200, Alexandre Derumier a écrit :
>> This remove vmbr* from bridgeselector if user have access to vnets.
>> (as currently, we don't have have permission management on vmbr$)
>>
>> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
>> ---
>>  PVE/API2/Network.pm | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
>> index a26f36d2..02bd3bdb 100644
>> --- a/PVE/API2/Network.pm
>> +++ b/PVE/API2/Network.pm
>> @@ -226,6 +226,7 @@ __PACKAGE__->register_method({
>>         my ($param) = @_;
>>  
>>         my $rpcenv = PVE::RPCEnvironment::get();
>> +       my $authuser = $rpcenv->get_user();
>>  
>>         my $tmp = PVE::INotify::read_file('interfaces', 1);
>>         my $config = $tmp->{data};
>> @@ -238,20 +239,24 @@ __PACKAGE__->register_method({
>>         delete $ifaces->{lo}; # do not list the loopback device
>>  
>>         if ($param->{type}) {
>> +           my $vnets = {};
>> +           my $filtered_sdn = undef;
>> +           if ($have_sdn && $param->{type} eq 'any_bridge') {
>> +               $vnets = PVE::Network::SDN::get_local_vnets();
>> +               $filtered_sdn = 1 if $authuser ne 'root@pam' && keys
>> %{$vnets} > 0;
>> +           }
>> +
>>             foreach my $k (keys %$ifaces) {
>>                 my $type = $ifaces->{$k}->{type};
>>                 my $match =  ($param->{type} eq $type) || (
>>                     ($param->{type} eq 'any_bridge') && 
>>                     ($type eq 'bridge' || $type eq 'OVSBridge'));
>> -               delete $ifaces->{$k} if !$match;
>> +               delete $ifaces->{$k} if !$match || $filtered_sdn;
>>             }
>>  
>> -           if ($have_sdn && $param->{type} eq 'any_bridge') {
>> -               my $vnets = PVE::Network::SDN::get_local_vnets();
>> -               map {
>> -                   $ifaces->{$_} = $vnets->{$_};
>> -               } keys %$vnets;
>> -           }
>> +           map {
>> +               $ifaces->{$_} = $vnets->{$_};
>> +           } keys %$vnets;
>>         }
>>  
>>         return PVE::RESTHandler::hash_to_array($ifaces, 'iface');
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 

Re: [pve-devel] [PATCH pve-manager 2/2] api2 : network: anybridge: don't display bridges if user have access to vnets.

[alexandre derumier]

Le lundi 23 août 2021 à 18:39 +0200, Thomas Lamprecht a écrit :
> I mean, we could also just see bridges as a vnet and handle them with
> the
> `/sdn/vnets/<id>` path? Checking for every bridge/vnet if some priv.
> is
> available, we probably would need two privileges, one for "allowed to
> use"
> and a "allowed to modify" priv.

yes, I was thinking about this way too.

I'll rework this patch, thanks for the review.