[pve-devel] [PATCH pve-firewall v4 4/9] add support for loading sdn firewall configuration

[Thomas Lamprecht]

Am 15.11.24 um 13:09 schrieb Stefan Hanreich:
> This also includes support for parsing rules referencing IPSets in the
> new SDN scope and generating those IPSets in the firewall.
> 
> Loading SDN configuration is optional, since loading it requires root
> privileges which we do not have in all call sites. Adding the flag
> allows us to selectively load the SDN configuration only where
> required and at the same time allows us to only elevate privileges in
> the API where absolutely needed.
> 
> Signed-off-by: Stefan Hanreich <s.hanreich at proxmox.com>
> Reviewed-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> Tested-By: Gabriel Goller <g.goller at proxmox.com>
> Tested-By: Hannes Dürr <h.duerr at proxmox.com>
> ---
>  src/PVE/Firewall.pm             | 62 +++++++++++++++++++++++++++++----
>  src/PVE/Service/pve_firewall.pm |  4 +--
>  2 files changed, 57 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 09544ba..7642bf6 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -25,6 +25,7 @@ use PVE::Tools qw($IPV4RE $IPV6RE);
>  use PVE::Tools qw(run_command lock_file dir_glob_foreach);
>  
>  use PVE::Firewall::Helpers;
> +use PVE::RS::Firewall::SDN;
>  
>  my $pvefw_conf_dir = "/etc/pve/firewall";
>  my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
> @@ -1689,9 +1690,12 @@ sub verify_rule {
>  
>  	if (my $value = $rule->{$name}) {
>  	    if ($value =~ m/^\+/) {
> -		if ($value =~ m@^\+(guest/|dc/)?(${ipset_name_pattern})$@) {
> +		if ($value =~ m@^\+(guest/|dc/|sdn/)?(${ipset_name_pattern})$@) {
>  		    &$add_error($name, "no such ipset '$2'")
> -			if !($cluster_conf->{ipset}->{$2} || ($fw_conf && $fw_conf->{ipset}->{$2}));
> +			if !($cluster_conf->{ipset}->{$2}
> +			    || ($fw_conf && $fw_conf->{ipset}->{$2})
> +			    || ($cluster_conf->{sdn} && $cluster_conf->{sdn}->{ipset}->{$2})
> +			    || ($fw_conf->{sdn} && $fw_conf->{sdn}->{ipset}->{$2}));

Avoid multi-line post-ifs:

https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If

rather use something like

if (
    !$cluster_conf->{ipset}->{$2}
    && !($fw_conf && $fw_conf->{ipset}->{$2})
    && !($cluster_conf->{sdn} && $cluster_conf->{sdn}->{ipset}->{$2})
    && !($fw_conf->{sdn} && $fw_conf->{sdn}->{ipset}->{$2}))
) {
    $add_error->($name, "no such ipset '$2'")
}

>  
>  		} else {
>  		    &$add_error($name, "invalid ipset name '$value'");
> @@ -2108,13 +2112,15 @@ sub ipt_gen_src_or_dst_match {
>  
>      my $match;
>      if ($adr =~ m/^\+/) {
> -	if ($adr =~ m@^\+(guest/|dc/)?(${ipset_name_pattern})$@) {
> +	if ($adr =~ m@^\+(guest/|dc/|sdn/)?(${ipset_name_pattern})$@) {
>  	    my $scope = $1 // "";
>  	    my $name = $2;

A small closer could help to reduce bewlow if expressions, e.g. like:

my $is_scope = sub { return !$scope || $scope eq "$_[0]/" };

#...

if ($is_scope->('guest') && $fw_conf && $fw_conf->{ipset}->{$name}) {
#...

>  	    my $ipset_chain;
> -	    if ($scope ne 'dc/' && $fw_conf && $fw_conf->{ipset}->{$name}) {
> +	    if ((!$scope || $scope eq 'guest/') && $fw_conf && $fw_conf->{ipset}->{$name}) {
>  		$ipset_chain = compute_ipset_chain_name($fw_conf->{vmid}, $name, $ipversion);
> -	    } elsif ($scope ne 'guest/' && $cluster_conf && $cluster_conf->{ipset}->{$name}) {
> +	    } elsif ((!$scope || $scope eq 'dc/') && $cluster_conf && $cluster_conf->{ipset}->{$name}) {
> +		$ipset_chain = compute_ipset_chain_name(0, $name, $ipversion);
> +	    } elsif ((!$scope || $scope eq 'sdn/') && $cluster_conf->{sdn} && $cluster_conf->{sdn}->{ipset}->{$name}) {
>  		$ipset_chain = compute_ipset_chain_name(0, $name, $ipversion);
>  	    } else {
>  		die "no such ipset '$name'\n";
> @@ -3644,7 +3650,8 @@ sub lock_clusterfw_conf {
>  }
>  
>  sub load_clusterfw_conf {
> -    my ($filename) = @_;
> +    my ($filename, $options) = @_;
> +
>  
>      $filename = $clusterfw_conf_filename if !defined($filename);
>      my $empty_conf = {
> @@ -3655,6 +3662,7 @@ sub load_clusterfw_conf {
>  	group_comments => {},
>  	ipset => {} ,
>  	ipset_comments => {},
> +	sdn => load_sdn_conf(),

it's a bit odd to assign the full SDN related config to a variable named
$empty_config, but assigning it after the parser will cause a semantic difference
for the case where the firewall config is empty, not sure if that is fine.

>      };
>  
>      my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster');
> @@ -3663,6 +3671,45 @@ sub load_clusterfw_conf {
>      return $cluster_conf;
>  }
>  
> +sub load_sdn_conf {
> +    my $rpcenv = eval { PVE::RPCEnvironment::get(); };

Missing use statement for that module, also the semicolon inside the eval can be dropped.

> +
> +    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 = [];
> +    foreach my $vmid (sort keys %{$guests->{ids}}) {
> +	next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ], 1);
> +	push @$allowed_vms, $vmid;
> +    }

Above one could be:

$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 $sdn_config = {
> +	ipset => {} ,
> +	ipset_comments => {},
> +    };
> +
> +    eval {
> +	$sdn_config = PVE::RS::Firewall::SDN::config($allowed_vnets, $allowed_vms);
> +    };
> +    warn $@ if $@;

above might be more readable by doing:

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;

> +
> +    return $sdn_config;
> +}
> +
>  sub save_clusterfw_conf {
>      my ($cluster_conf) = @_;
>  
> @@ -3768,7 +3815,7 @@ sub compile {
>  
>  	$vmfw_configs = read_vm_firewall_configs($cluster_conf, $vmdata, $testdir);
>      } else { # normal operation
> -	$cluster_conf = load_clusterfw_conf(undef) if !$cluster_conf;
> +	$cluster_conf = load_clusterfw_conf() if !$cluster_conf;
>  
>  	$hostfw_conf = load_hostfw_conf($cluster_conf, undef) if !$hostfw_conf;
>  
> @@ -4043,6 +4090,7 @@ sub compile_ipsets {
>      }
>  
>      generate_ipset_chains($ipset_ruleset, undef, $cluster_conf, undef, $cluster_conf->{ipset});
> +    generate_ipset_chains($ipset_ruleset, undef, $cluster_conf, undef, $cluster_conf->{sdn}->{ipset});
>  
>      return $ipset_ruleset;
>  }
> diff --git a/src/PVE/Service/pve_firewall.pm b/src/PVE/Service/pve_firewall.pm
> index 65cb2b8..02b507a 100755
> --- a/src/PVE/Service/pve_firewall.pm
> +++ b/src/PVE/Service/pve_firewall.pm
> @@ -158,7 +158,7 @@ __PACKAGE__->register_method ({
>  
>  	    PVE::Firewall::set_verbose(1); # show syntax errors
>  
> -	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef);
> +	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();

just to be sure, above and below does not change anything, or?

>  	    $res->{enable} = $cluster_conf->{options}->{enable} ? 1 : 0;
>  
>  	    if ($status eq 'running') {
> @@ -202,7 +202,7 @@ __PACKAGE__->register_method ({
>  
>  	    PVE::Firewall::set_verbose(1);
>  
> -	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef);
> +	    my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
>  	    my ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset) = PVE::Firewall::compile($cluster_conf, undef, undef);
>  
>  	    print "ipset cmdlist:\n";