From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id EEFCD9D0A1 for ; Fri, 2 Jun 2023 13:40:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D3A332970F for ; Fri, 2 Jun 2023 13:40:03 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 2 Jun 2023 13:40:02 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 17FAC48486 for ; Fri, 2 Jun 2023 13:40:02 +0200 (CEST) Date: Fri, 02 Jun 2023 13:39:55 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230526072724.1605613-1-aderumier@odiso.com> <20230526072724.1605613-5-aderumier@odiso.com> In-Reply-To: <20230526072724.1605613-5-aderumier@odiso.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1685703684.s6g3qc3qqp.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.074 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH pve-manager 4/4] api2: network: check vlan permissions for local bridges X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Jun 2023 11:40:34 -0000 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. >=20 > 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. >=20 > (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). >=20 > Signed-off-by: Alexandre Derumier > --- > PVE/API2/Network.pm | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) >=20 > 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({ > =20 > if (my $tfilter =3D $param->{type}) { > my $vnets; > + my $bridges_vlans_acl =3D {}; > #check access for local bridges > my $can_access_vnet =3D sub { > + my $bridge =3D $_[0]; > return 1 if $authuser eq 'root@pam'; > return 1 if $rpcenv->check_any($authuser, "/sdn/zones/local", ['SDN.Au= dit', 'SDN.Allocate'], 1); > - return 1 if $rpcenv->check_any($authuser, "/sdn/vnets/$_[0]", ['SDN.Au= dit', 'SDN.Allocate'], 1); > + return 1 if $rpcenv->check($authuser, "/sdn/vnets/$bridge", ['SDN.Audi= t'], 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 =3D $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 =3D PVE::AccessControl::find_acl_tree_node($cfg->{acl_root}, "/s= dn/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; > + } > }; > =20 > if ($have_sdn && $param->{type} eq 'any_bridge') { > $vnets =3D PVE::Network::SDN::get_local_vnets(); # returns already acc= ess-filtered > } > =20 > + #find all vlans where we have specific acls > + if ($tfilter =3D~ /^any(_local)?_bridge$/) { > + my $cfg =3D $rpcenv->{user_cfg}; > + my $vnets_acl_root =3D $cfg->{acl_root}->{children}->{sdn}->{children}= ->{vnets}; > + PVE::AccessControl::iterate_acl_tree("/", $vnets_acl_root, sub { > + my ($path, $node) =3D @_; > + if ($path =3D~ /\/(.*)\.(\d+)$/) { > + $bridges_vlans_acl->{$1}->{$2} =3D 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 =3D $ifaces->{$k}->{type}; > my $match =3D $tfilter eq $type || ($tfilter =3D~ /^any(_local)?_bridg= e$/ && ($type eq 'bridge' || $type eq 'OVSBridge')); > --=20 > 2.30.2 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20