[pve-devel] [PATCH pve-firewall v5 2/5] add support for loading sdn firewall configuration

[Stefan Hanreich]

On 11/18/24 12:41, Stefan Hanreich wrote:
> +sub load_sdn_conf {
> +    my $rpcenv = eval { PVE::RPCEnvironment::get() };

After some additional consideration and testing, I think it is a bad
idea to have the permission filtering in the core firewall code.
Particularly because loading and validating is a single step in
pve-firewall and you cannot opt out of it. As soon as we limit the
amount of IPSets returned here, we run into the possibility of
validation errors. Also, it's generally bad w.r.t. separation of concerns.

I think we should *always* load the whole configuration here and filter
which IPSets we output in the API methods instead (by invoking
load_sdn_conf there explicitly and updating the cluster_conf hash).

Consider the following situation:

A user has permissions for one (out of many) SDN subnet and one (out of
many) VM and wants to edit the VM firewall config from the Web UI. When
editing the VM Firewall configuration, as soon as we load the cluster
config in the code of that endpoint and the cluster configuration
references an IPSet the user cannot see, there are validation errors in
the syslog (although they do not pose any functional issue - the
firewall works correctly).

The only way to avoid this I see is:
* always load the *full* SDN IPsets initially, so cfg validation always
  sees the big pictrue
* filter the output of IPSets in the API endpoints returning valid
  IPsets for rule creation
* use the filtered IPSet list when validating rules on creation / update

When returning a firewall configuration from the API, and it contains
IPSets that actually exist, but cannot be seen by the user, we're in a
bit of a pickle.
Currently we show them as errors in the UI (since the referenced IPSet
is not loaded because of the filtering we do currently load_sdn_conf).
With the proposed changes, there would be no errors displayed in the
frontend unless the user tries to edit the rule (since then we're
validating against the filtered IPSet list). I'm not sure what would be
the best behavior here.
I don't think filtering the rules is a good option, since it can lead to
unexpected behavior of the firewall rules and is generally just
intransparent. I think just showing them as valid but not letting the
user actually use them in rules is fine.

Currently we show errors in the syslog in this situation (could not find
ipset ...), but the functionality of the firewall is not affected in any
way! So we can also decide that this is okay for now and fix this with a
subsequent patch.

> +
> +    if ($@) {
> +	warn "could not load SDN configuration because RPCEnvironment is not initialized.";
> +	return {};
> +    }
> +
> +    my $authuser = $rpcenv->get_user();
> +
> +    my $guests = PVE::Cluster::get_vmlist();
> +    my $allowed_vms = [
> +	grep { $rpcenv->check($authuser, "/vms/$_", [ 'VM.Audit' ], 1) } sort keys $guests->{ids}->%*
> +    ];
> +
> +    my $vnets = PVE::Network::SDN::Vnets::config(1);
> +    my $privs = [ 'SDN.Audit', 'SDN.Allocate' ];
> +    my $allowed_vnets = [];
> +    foreach my $vnet (sort keys %{$vnets->{ids}}) {
> +	my $zone = $vnets->{ids}->{$vnet}->{zone};
> +	next if !$rpcenv->check_any($authuser, "/sdn/zones/$zone/$vnet", $privs, 1);
> +	push @$allowed_vnets, $vnet;
> +    }
> +
> +    my $empty_sdn_config = { ipset => {} , ipset_comments => {} };
> +
> +    my $sdn_config = eval {
> +	PVE::RS::Firewall::SDN::config($allowed_vnets, $allowed_vms)
> +    };
> +    warn $@ if $@;
> +
> +    return $sdn_config // $empty_sdn_config;
> +}
> +