[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 at 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?