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