Re: [pve-devel] [PATCH pve-network 11/17] api: add fabrics subfolder

[Stefan Hanreich <s.hanreich@proxmox.com>]

On 4/2/25 12:41, Fabian Grünbichler wrote:
>> +    code => sub {
>> +	my ($param) = @_;
>> +	my $rpcenv = PVE::RPCEnvironment::get();
>> +
>> +	my $running = extract_param($param, 'running');
>> +	my $pending = extract_param($param, 'pending');
>> +
>> +	my $fabric_config = PVE::Network::SDN::Fabrics::config();
>> +	my $running_config = PVE::Network::SDN::running_config();
>> +	my $config;
>> +
>> +	my $authuser = $rpcenv->get_user();
>> +	my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> 
> I wonder whether it would make sense to check whether there are any
> privs below the /sdn/fabrics root here, and move the config loading
> below that check, to avoid leaking things via error messages if
> something is misconfigured?

Yes, probably sensible to check for each protocol independently and only
start loading the configuration / filtering if the user has at least
some permissions for that protocol. We should then also eval everything
and return a generic error in case anything goes wrong - just to be sure?

> also, doesn't this return quite a lot of information for an "index"
> call that just requires SDN.Audit? it might make sense to filter the
> information below based on whether we have Audit or Allocate privs?

Wouldn't that be applicable for almost any SDN endpoint then? Audit has
always been read and Allocate modify. Not sure which properties we could
actually filter in the case of the user having only Audit permissions.

We decided against additionally introducing a host-level in the
permissions, because with a fabric spanning the whole cluster it doesn't
really make sense to have a partial view of only some nodes.

>> +
>> +	my $res = {};
>> +	foreach my $protocol (keys %$fabric_config) {
>> +	    $res->{$protocol} = [];
>> +
>> +	    if ($pending) {
>> +		# pending_config expects the section config to be under the ids
>> +		# key, but get_inner() returns it without that key
>> +		my $section_config = {
>> +		    ids => $fabric_config->{$protocol}->get_inner(),
>> +		};
>> +
>> +		$config = PVE::Network::SDN::pending_config(
>> +		    $running_config,
>> +		    $section_config,
>> +		    $protocol
>> +		);
>> +
>> +		$config = $config->{ids};
>> +	    } elsif ($running) {
>> +		$config = $running_config->{$protocol}->{ids};
>> +	    } else {
>> +		$config = $fabric_config->{$protocol}->get_inner();
>> +	    }
>> +
>> +	    foreach my $id (sort keys %$config) {
>> +		my $entry = $config->{$id};
>> +		next if !$rpcenv->check_any($authuser, "/sdn/fabrics/$protocol/$entry->{name}", $privs, 1);
> 
> this is a new ACL path, but it's not possible to configure it because
> there is no pve-access-control patch allowing it - did you test the
> permissions part? ;)

will be included in a follow-up - sorry!

>> +
>> +		push @{$res->{$protocol}}, dclone($entry);
>> +	    }
>> +	}
>> +	return $res;
>> +    },
>> +});
>> +
>> +1;
>> diff --git a/src/PVE/API2/Network/SDN/Makefile b/src/PVE/API2/Network/SDN/Makefile
>> index abd1bfae020e..4dbb6c92fd82 100644
>> --- a/src/PVE/API2/Network/SDN/Makefile
>> +++ b/src/PVE/API2/Network/SDN/Makefile
>> @@ -1,4 +1,4 @@
>> -SOURCES=Vnets.pm Zones.pm Controllers.pm Subnets.pm Ipams.pm Dns.pm Ips.pm
>> +SOURCES=Vnets.pm Zones.pm Controllers.pm Subnets.pm Ipams.pm Dns.pm Ips.pm Fabrics.pm
>>  
>>  
>>  PERL5DIR=${DESTDIR}/usr/share/perl5
>> diff --git a/src/PVE/Network/SDN.pm b/src/PVE/Network/SDN.pm
>> index 24879dc0e76a..b35767b667b4 100644
>> --- a/src/PVE/Network/SDN.pm
>> +++ b/src/PVE/Network/SDN.pm
>> @@ -344,7 +344,7 @@ sub generate_dhcp_config {
>>  sub encode_value {
>>      my ($type, $key, $value) = @_;
>>  
>> -    if ($key eq 'nodes' || $key eq 'exitnodes' || $key eq 'dhcp-range') {
>> +    if ($key eq 'nodes' || $key eq 'exitnodes' || $key eq 'dhcp-range' || $key eq 'interface') {
> 
> I hope this doesn't ever bite us, 'interface' (and 'nodes' for that matter) is quite generic..

Yes, it's far from optimal - the whole pending_config / encode_value
could use some serious redoing imo.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel