[pve-devel] [PATCH manager] api2: network: anybridge: re-add regular bridges

[pve-devel] [PATCH manager] api2: network: anybridge: re-add regular bridges

[Fabian Grünbichler]

commit 052fbb2a4d1bdeb490b2e3b67cd7555e460ebe93 introduced permission
checks here that caused all regular bridges to be removed from the
returned list as soon as the SDN package is installed, unless the user
is root@pam or there exists a VNET with the same ID.

this is arguably a breaking change, so limit the priv check to actually
defined VNETs for the time being, and add ALL regular bridges
uncondtionally like before.

get_local_vnets already filters by the same prvs, so we need to get the
full config to find out which IDs are VNETs and which are not.

once/iff we introduce ACL paths for *all* bridges in the future, we can
limit accordingly here.

CC: Alexandre Derumier <aderumier@odiso.com>
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 PVE/API2/Network.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
index 214ab50a..a43579fa 100644
--- a/PVE/API2/Network.pm
+++ b/PVE/API2/Network.pm
@@ -240,13 +240,16 @@ __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->%*) {
-- 
2.30.2

[pve-devel] applied: [PATCH manager] api2: network: anybridge: re-add regular bridges

[Thomas Lamprecht]

On 27.04.22 13:36, Fabian Grünbichler wrote:
> commit 052fbb2a4d1bdeb490b2e3b67cd7555e460ebe93 introduced permission
> checks here that caused all regular bridges to be removed from the
> returned list as soon as the SDN package is installed, unless the user
> is root@pam or there exists a VNET with the same ID.
> 
> this is arguably a breaking change, so limit the priv check to actually
> defined VNETs for the time being, and add ALL regular bridges
> uncondtionally like before.
> 
> get_local_vnets already filters by the same prvs, so we need to get the
> full config to find out which IDs are VNETs and which are not.
> 
> once/iff we introduce ACL paths for *all* bridges in the future, we can
> limit accordingly here.
> 
> CC: Alexandre Derumier <aderumier@odiso.com>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  PVE/API2/Network.pm | 3 +++
>  1 file changed, 3 insertions(+)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH manager] api2: network: anybridge: re-add regular bridges

[DERUMIER, Alexandre]

Hi Fabian
Le mercredi 27 avril 2022 à 13:36 +0200, Fabian Grünbichler a écrit :
> commit 052fbb2a4d1bdeb490b2e3b67cd7555e460ebe93 introduced permission
> > checks here that caused all regular bridges to be removed from the
> > returned list as soon as the SDN package is installed, unless the
> > user
> > is root@pam or there exists a VNET with the same ID.
> > 
mmm, this is unexpected.
with my original patch :

if user don't have any permissions on vnets:
  - all vmbrX bridges are displayed
  - no vnets is displayed

if user have a permission on at least 1vnet:
  - vmbrX bridge are no more displayed  (untie you add a specific
permissions with /sdn/vnets/vmbrX)
  - only vnet with permissions are displayed


I didn' see, but Thomas have reworked it:
https://git.proxmox.com/?p=pve-manager.git;a=commit;h=640c0b26891c408d0456c355b3724c1be18cc75f

and the behaviour seem to be different:

if user don't have any permissions on vnets:
  - no vmbrX displayed   ----> different behaviour
  - no vnets is displayed

if user have a permission on at least 1vnet:
  - vmbrX bridge are no more displayed  (untile you add a specific
permissions with /sdn/vnets/vmbrX)
  - only vnet with permissions are displayed


with your patch, it seem to be different too:

if user don't have any permissions on vnets:
  - vmbrX displayed  
  - no vnets is displayed

if user have a permission on at least 1vnet:
  - vmbrX brige are still displayed ----> different behaviour
  - only vnet with permissions are displayed


> this is arguably a breaking change, so limit the priv check to
> actually
> > defined VNETs for the time being, and add ALL regular bridges
> > uncondtionally like before.
> > 
> get_local_vnets already filters by the same prvs, so we need to get
> the
> > full config to find out which IDs are VNETs and which are not.
> > 
> once/iff we introduce ACL paths for all bridges in the future, we can
> > limit accordingly here.
> > 
> CC: Alexandre Derumier <aderumier@odiso.com>
> > Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> > ---
> >  PVE/API2/Network.pm | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> > index 214ab50a..a43579fa 100644
> > --- a/PVE/API2/Network.pm
> > +++ b/PVE/API2/Network.pm
> > @@ -240,13 +240,16 @@ 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->%*) {

Re: [pve-devel] [PATCH manager] api2: network: anybridge: re-add regular bridges

[Fabian Grünbichler]

On April 27, 2022 2:32 pm, DERUMIER, Alexandre wrote:
> Hi Fabian
> Le mercredi 27 avril 2022 à 13:36 +0200, Fabian Grünbichler a écrit :
>> commit 052fbb2a4d1bdeb490b2e3b67cd7555e460ebe93 introduced permission
>> > checks here that caused all regular bridges to be removed from the
>> > returned list as soon as the SDN package is installed, unless the
>> > user
>> > is root@pam or there exists a VNET with the same ID.
>> > 
> mmm, this is unexpected.
> with my original patch :
> 
> if user don't have any permissions on vnets:
>   - all vmbrX bridges are displayed
>   - no vnets is displayed
> 
> if user have a permission on at least 1vnet:
>   - vmbrX bridge are no more displayed  (untie you add a specific
> permissions with /sdn/vnets/vmbrX)
>   - only vnet with permissions are displayed
> 
> 
> I didn' see, but Thomas have reworked it:
> https://git.proxmox.com/?p=pve-manager.git;a=commit;h=640c0b26891c408d0456c355b3724c1be18cc75f
> 
> and the behaviour seem to be different:
> 
> if user don't have any permissions on vnets:
>   - no vmbrX displayed   ----> different behaviour
>   - no vnets is displayed
> 
> if user have a permission on at least 1vnet:
>   - vmbrX bridge are no more displayed  (untile you add a specific
> permissions with /sdn/vnets/vmbrX)
>   - only vnet with permissions are displayed
> 
> 
> with your patch, it seem to be different too:
> 
> if user don't have any permissions on vnets:
>   - vmbrX displayed  
>   - no vnets is displayed
> 
> if user have a permission on at least 1vnet:
>   - vmbrX brige are still displayed ----> different behaviour
>   - only vnet with permissions are displayed

yeah that's true. I missed which commit was the culprit (relying on the 
commit messages)

given that
- we can't require some new ACL path/priv for regular bridges until the 
  next major release (as that would be quite the breaking change ;))
- removing access to the last VNET would suddenly make all regular 
  bridges available (again) with your original patch, which is strange 
  behaviour for an ACL (removing a positive ACL shouldn't give me more 
  access ;))
- with the original patch it's impossible to say "user FOO can play 
  around with SDN but also use regular bridges" without 'faking' ACLs 
  for vnets that aren't actually vnets, but regular bridges

I think the current variant is what we can have for the time being - and 
then with 8.0 either say
- 'regular bridges also require FOO on /sdn/vnets/vmbrX' (identical to 
  vnets, but a bit ugly as the ACL path is technically 'wrong')
- 'regular bridges require FOO on /network/bridge/vmbrX' (like vnets, 
  but different ACL path/namespace, probably with some helper that 'does 
  the right thing' in places where we don't care what kind it is)
and then filter here and in other places accordingly?

Re: [pve-devel] [PATCH manager] api2: network: anybridge: re-add regular bridges

[DERUMIER, Alexandre]

> given that
> - we can't require some new ACL path/priv for regular bridges until
> the 
>   next major release (as that would be quite the breaking change ;))
> - removing access to the last VNET would suddenly make all regular 
>   bridges available (again) with your original patch, which is
> strange 
>   behaviour for an ACL (removing a positive ACL shouldn't give me
> more 
>   access ;))
> - with the original patch it's impossible to say "user FOO can play 
>   around with SDN but also use regular bridges" without 'faking' ACLs
>   for vnets that aren't actually vnets, but regular bridges
> 

yes, I known it's a big strange/ugly.

I have tried to find a way to hide vmbrX because multiples users have
requested it.
https://forum.proxmox.com/threads/sdn-group-pool-permissions.93872/

But it's still beta, so, we can wait for next major release to have a
clean implementation.

>I think the current variant is what we can have for the time being -
> and 
> then with 8.0 either say
> - 'regular bridges also require FOO on /sdn/vnets/vmbrX' (identical
> to 
>   vnets, but a bit ugly as the ACL path is technically 'wrong')
> - 'regular bridges require FOO on /network/bridge/vmbrX' (like vnets,
>   but different ACL path/namespace, probably with some helper that
> 'does 
>   the right thing' in places where we don't care what kind it is)
> and then filter here and in other places accordingly?
> 

Yes, that's seem fine.
I could be great to have a true acl management to vmbrX :)


Le mercredi 27 avril 2022 à 14:45 +0200, Fabian Grünbichler a écrit :
> On April 27, 2022 2:32 pm, DERUMIER, Alexandre wrote:
> > Hi Fabian
> > Le mercredi 27 avril 2022 à 13:36 +0200, Fabian Grünbichler a
> > écrit :
> > > commit 052fbb2a4d1bdeb490b2e3b67cd7555e460ebe93 introduced
> > > permission
> > > > checks here that caused all regular bridges to be removed from
> > > > the
> > > > returned list as soon as the SDN package is installed, unless
> > > > the
> > > > user
> > > > is root@pam or there exists a VNET with the same ID.
> > > > 
> > mmm, this is unexpected.
> > with my original patch :
> > 
> > if user don't have any permissions on vnets:
> >   - all vmbrX bridges are displayed
> >   - no vnets is displayed
> > 
> > if user have a permission on at least 1vnet:
> >   - vmbrX bridge are no more displayed  (untie you add a specific
> > permissions with /sdn/vnets/vmbrX)
> >   - only vnet with permissions are displayed
> > 
> > 
> > I didn' see, but Thomas have reworked it:
> > https://antiphishing.cetsi.fr/proxy/v3?i=SHV0Y1JZQjNyckJFa3dUQiblhF5YcUqtiWCaK_ri0kk&r=T0hnMlUyVEgwNmlmdHc1NQqeTQ1pLQVNn4UPLJn2W6e9Hh50epHxcxJAGCrIHvKB1souhZXB265bSkydEfNuQg&f=V3p0eFlQOUZ4czh2enpJS729B9MwEV8JxHeeNPHyzEMH7fBkB3EUDz0PUKSsrg4P&u=https%3A//git.proxmox.com/%3Fp%3Dpve-manager.git%3Ba%3Dcommit%3Bh%3D640c0b26891c408d0456c355b3724c1be18cc75f&k=ZVd0
> > 
> > and the behaviour seem to be different:
> > 
> > if user don't have any permissions on vnets:
> >   - no vmbrX displayed   ----> different behaviour
> >   - no vnets is displayed
> > 
> > if user have a permission on at least 1vnet:
> >   - vmbrX bridge are no more displayed  (untile you add a specific
> > permissions with /sdn/vnets/vmbrX)
> >   - only vnet with permissions are displayed
> > 
> > 
> > with your patch, it seem to be different too:
> > 
> > if user don't have any permissions on vnets:
> >   - vmbrX displayed  
> >   - no vnets is displayed
> > 
> > if user have a permission on at least 1vnet:
> >   - vmbrX brige are still displayed ----> different behaviour
> >   - only vnet with permissions are displayed
> 
> yeah that's true. I missed which commit was the culprit (relying on
> the 
> commit messages)
> 
> given that
> - we can't require some new ACL path/priv for regular bridges until
> the 
>   next major release (as that would be quite the breaking change ;))
> - removing access to the last VNET would suddenly make all regular 
>   bridges available (again) with your original patch, which is
> strange 
>   behaviour for an ACL (removing a positive ACL shouldn't give me
> more 
>   access ;))
> - with the original patch it's impossible to say "user FOO can play 
>   around with SDN but also use regular bridges" without 'faking' ACLs
>   for vnets that aren't actually vnets, but regular bridges
> 
> I think the current variant is what we can have for the time being -
> and 
> then with 8.0 either say
> - 'regular bridges also require FOO on /sdn/vnets/vmbrX' (identical
> to 
>   vnets, but a bit ugly as the ACL path is technically 'wrong')
> - 'regular bridges require FOO on /network/bridge/vmbrX' (like vnets,
>   but different ACL path/namespace, probably with some helper that
> 'does 
>   the right thing' in places where we don't care what kind it is)
> and then filter here and in other places accordingly?
> 

Re: [pve-devel] [PATCH manager] api2: network: anybridge: re-add regular bridges

[Thomas Lamprecht]

On 27.04.22 14:32, DERUMIER, Alexandre wrote:
> I didn' see, but Thomas have reworked it:
> https://git.proxmox.com/?p=pve-manager.git;a=commit;h=640c0b26891c408d0456c355b3724c1be18cc75f
> 
> and the behaviour seem to be different:

argh, sorry, the can_access_vnet sub should also had an:

return 1 if !($have_sdn && $param->{type} eq 'any_bridge');

to make it equivalent with what you had before... :/