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 <aderum...@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
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
