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

[Fabian Grünbichler]

On April 2, 2025 2:20 pm, Stefan Hanreich wrote:
> 
> 
> 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.

can be fine, the rest of the GET api calls only take allocate though ;)

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

that's okay, we don't really have that many per-node ACL paths in
general..