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

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

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

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

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

> +
>  	    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://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>