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

["DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>]

Le vendredi 02 juin 2023 à 13:39 +0200, Fabian Grünbichler a écrit :
> On May 26, 2023 9:27 am, Alexandre Derumier wrote:
> > We need to display the bridge is the user have a permission
> > on any vlan on the bridge.
> > 
> > to avoid to check permissions on 4096 vlans for each bridge
> > (could be slow with a lot of bridges),
> > we first list vlans where acls are defined.
> > 
> > (4000 check took 60ms on 10year xeon server, should be enough
> > for any network where the total number of vlans is limited)
> 
> some sort of spec here for how the ACL looks like would be nice to
> have
> (while it's possible to reverse it from the code, it's always nicer
> to
> have the expection explicit as well).
> 
> > 
> > Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> > ---
> >  PVE/API2/Network.pm | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> > index ba3b3e0e..39f17d14 100644
> > --- a/PVE/API2/Network.pm
> > +++ b/PVE/API2/Network.pm
> > @@ -240,17 +240,35 @@ __PACKAGE__->register_method({
> >  
> >         if (my $tfilter = $param->{type}) {
> >             my $vnets;
> > +           my $bridges_vlans_acl = {};
> >             #check access for local bridges
> >             my $can_access_vnet = sub {
> > +               my $bridge = $_[0];
> >                 return 1 if $authuser eq 'root@pam';
> >                 return 1 if $rpcenv->check_any($authuser,
> > "/sdn/zones/local", ['SDN.Audit', 'SDN.Allocate'], 1);
> > -               return 1 if $rpcenv->check_any($authuser,
> > "/sdn/vnets/$_[0]", ['SDN.Audit', 'SDN.Allocate'], 1);
> > +               return 1 if $rpcenv->check($authuser,
> > "/sdn/vnets/$bridge", ['SDN.Audit'], 1);
> 
> why does this drop the Allocate? we usually have the "more
> privileged"
> privilege in addition to Audit (if there is one).

I think allocate maybe sense on zone. (as what's we allocate are
vnets).
(it should be removed on /sdn/vnets/$_[0]).


But I don't, maybe if user add a simple acl like "/" + propagate with
SDN.allocate only, we want to give it access everywhere ?


> 
> > +               my $bridge_vlan = $bridges_vlans_acl->{$bridge};
> > +               for my $tag (sort keys %$bridge_vlan) {
> > +                   return 1 if $rpcenv->check($authuser,
> > "/sdn/vnets/$bridge.$tag", ['SDN.Audit'], 1);
> 
> wouldn't $bridge/$tag be more natural? it would allow propagation
> from a
> bridge to all tags using the usual propagate flag as well..
> 
> this could also live in pve-access-control as a special helper, then
> we
> wouldn't need to do this dance here (it would be the first
> iterate_acl_tree call site outside of pve-access-control!).
> 
> something like this in PVE::RPCEnvironment:
> 
> sub check_sdn_vlan(.., $bridge, $priv) {
>   .. iterate over all vlans and check while iterating, returning
> early for first one with access
> }
> 
> basically:
> 
> my $bridge = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root},
> "/sdn/vnets/$bridge");
> if ($bridge) {
>   for my $vlan (keys $bridge->{children}->%$) {
>     return 1 if $self->check_any(...);
>   }
>   return 1 if # check propagate on bridge itself
> }
> return 0;
> 
> > +               }
> >             };
> >  
> >             if ($have_sdn && $param->{type} eq 'any_bridge') {
> >                 $vnets = PVE::Network::SDN::get_local_vnets(); #
> > returns already access-filtered
> >             }
> >  
> > +           #find all vlans where we have specific acls
> > +           if ($tfilter =~ /^any(_local)?_bridge$/) {
> > +               my $cfg = $rpcenv->{user_cfg};
> > +               my $vnets_acl_root = $cfg->{acl_root}->{children}-
> > >{sdn}->{children}->{vnets};
> > +               PVE::AccessControl::iterate_acl_tree("/",
> > $vnets_acl_root, sub {
> > +                   my ($path, $node) = @_;
> > +                   if ($path =~ /\/(.*)\.(\d+)$/) {
> > +                       $bridges_vlans_acl->{$1}->{$2} = 1;
> > +                   }
> > +               });
> > +           }
> 
> because this iterates over *all* ACLs, only to then return upon the
> first match above in $can_access_vnet..
> 
> it should also be
> 
> iterate_acl_tree("/sdn/vnets", ..) or "/sdn/vnets/$bridge" if vlan
> tags
> live as children of the bridge (path and the node should match!).
> there
> also is "find_acl_tree_node" so you don't need to make assumptions
> about
> how the nodes are laid out.
> 

Thanks ! I was really banging my head to implement this properly &&
fast ^_^


> I do wonder whether something that supports ranges would be more
> appropriate rather then encoding this in ACLs directly though..
>  could
> always be added later on though (e.g., named vlan objects that define
> a
> set of vlans).

yes, I was thinking the same to avoid too much acl when you need to
access to many vlans.

> 
> > +
> >             for my $k (sort keys $ifaces->%*) {
> >                 my $type = $ifaces->{$k}->{type};
> >                 my $match = $tfilter eq $type || ($tfilter =~
> > /^any(_local)?_bridge$/ && ($type eq 'bridge' || $type eq
> > 'OVSBridge'));
> > -- 
> > 2.30.2
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://antiphishing.cetsi.fr/proxy/v3?i=MlZSTzBhZFZ6Nzl4c3EyN7fbSKDePLMxi5u5_onpAoI&r=cm1qVmRYUWk2WXhYZVFHWA0PXtTaYxz7-FIOTkZBm34_dHdSch-gXn7ST9eGhQLN&f=S1Zkd042VWdrZG5qQUxxWk5ps4t67kNuHsBZzdzhpquLKuXqTZLIq2K1DfKr9N61yBafm7AuAITd6bHtRU4zEQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=F1is
> > 
> > 
> > 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://antiphishing.cetsi.fr/proxy/v3?i=MlZSTzBhZFZ6Nzl4c3EyN7fbSKDePLMxi5u5_onpAoI&r=cm1qVmRYUWk2WXhYZVFHWA0PXtTaYxz7-FIOTkZBm34_dHdSch-gXn7ST9eGhQLN&f=S1Zkd042VWdrZG5qQUxxWk5ps4t67kNuHsBZzdzhpquLKuXqTZLIq2K1DfKr9N61yBafm7AuAITd6bHtRU4zEQ&u=https%3A//lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel&k=F1is
>