Re: [pve-devel] [PATCH pve-firewall v2 10/17] sdn: add vnet firewall configuration

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Oct 10, 2024 at 05:56:43PM GMT, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  src/PVE/Firewall.pm         | 122 ++++++++++++++++++++++++++++++++++--
>  src/PVE/Firewall/Helpers.pm |  12 ++++
>  2 files changed, 128 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 9943f2e..e8096aa 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -29,6 +29,7 @@ use PVE::RS::Firewall::SDN;
>  
>  my $pvefw_conf_dir = "/etc/pve/firewall";
>  my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
> +my $vnetfw_conf_dir = "/etc/pve/sdn/firewall";
>  
>  # dynamically include PVE::QemuServer and PVE::LXC
>  # to avoid dependency problems
> @@ -1290,6 +1291,12 @@ our $cluster_option_properties = {
>  	optional => 1,
>  	enum => ['ACCEPT', 'REJECT', 'DROP'],
>      },
> +    policy_forward => {
> +	description => "Forward policy.",
> +	type => 'string',
> +	optional => 1,
> +	enum => ['ACCEPT', 'DROP'],
> +    },
>      log_ratelimit => {
>  	description => "Log ratelimiting settings",
>  	type => 'string', format => {
> @@ -1329,6 +1336,8 @@ our $host_option_properties = {
>  	description => "Log level for incoming traffic." }),
>      log_level_out =>  get_standard_option('pve-fw-loglevel', {
>  	description => "Log level for outgoing traffic." }),
> +    log_level_forward =>  get_standard_option('pve-fw-loglevel', {
> +	description => "Log level for forwarded traffic." }),
>      tcp_flags_log_level =>  get_standard_option('pve-fw-loglevel', {
>  	description => "Log level for illegal tcp flags filter." }),
>      smurf_log_level =>  get_standard_option('pve-fw-loglevel', {
> @@ -1476,6 +1485,23 @@ our $vm_option_properties = {
>  
>  };
>  
> +our $vnet_option_properties = {
> +    enable => {
> +	description => "Enable/disable firewall rules.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    policy_forward => {
> +	description => "Forward policy.",
> +	type => 'string',
> +	optional => 1,
> +	enum => ['ACCEPT', 'DROP'],
> +    },
> +    log_level_forward =>  get_standard_option('pve-fw-loglevel', {
> +	description => "Log level for forwarded traffic." }),
> +};
> +
>  
>  my $addr_list_descr = "This can refer to a single IP address, an IP set ('+ipsetname') or an IP alias definition. You can also specify an address range like '20.34.101.207-201.3.9.99', or a list of IP addresses and networks (entries are separated by comma). Please do not mix IPv4 and IPv6 addresses inside such lists.";
>  
> @@ -1493,7 +1519,7 @@ my $rule_properties = {
>  	description => "Rule type.",
>  	type => 'string',
>  	optional => 1,
> -	enum => ['in', 'out', 'group'],
> +	enum => ['in', 'out', 'forward', 'group'],
>      },
>      action => {
>  	description => "Rule action ('ACCEPT', 'DROP', 'REJECT') or security group name.",
> @@ -1651,10 +1677,20 @@ my $rule_env_iface_lookup = {
>      'ct' => 1,
>      'vm' => 1,
>      'group' => 0,
> +    'vnet' => 0,
>      'cluster' => 1,
>      'host' => 1,
>  };
>  
> +my $rule_env_direction_lookup = {
> +    'ct' => ['in', 'out', 'group'],
> +    'vm' => ['in', 'out', 'group'],
> +    'group' => ['in', 'out', 'forward'],
> +    'cluster' => ['in', 'out', 'forward', 'group'],
> +    'host' => ['in', 'out', 'forward', 'group'],
> +    'vnet' => ['forward', 'group'],
> +};
> +
>  sub verify_rule {
>      my ($rule, $cluster_conf, $fw_conf, $rule_env, $noerr) = @_;
>  
> @@ -1725,7 +1761,13 @@ sub verify_rule {
>      &$add_error('action', "missing property") if !$action;
>  
>      if ($type) {
> -	if ($type eq  'in' || $type eq 'out') {
> +	my $valid_types = $rule_env_direction_lookup->{$rule_env}
> +	    or die "unknown rule_env '$rule_env'\n";
> +
> +	&$add_error('type', "invalid rule type '$type' for rule_env '$rule_env'")
> +	    if !grep (/^$type$/, @$valid_types);

^ Please use `$_ eq $type` instead of turning the thing you're supposed
to be verifying into a regex.

> +
> +	if ($type eq  'in' || $type eq 'out' || $type eq 'forward') {
>  	    &$add_error('action', "unknown action '$action'")
>  		if $action && ($action !~ m/^(ACCEPT|DROP|REJECT)$/);
>  	} elsif ($type eq 'group') {
> @@ -2829,7 +2871,7 @@ sub parse_fw_rule {
>      $rule->{type} = lc($1);
>      $rule->{action} = $2;
>  
> -    if ($rule->{type} eq  'in' || $rule->{type} eq 'out') {
> +    if ($rule->{type} eq  'in' || $rule->{type} eq 'out' || $rule->{type} eq 'forward') {
>  	if ($rule->{action} =~ m/^(\S+)\((ACCEPT|DROP|REJECT)\)$/) {
>  	    $rule->{macro} = $1;
>  	    $rule->{action} = $2;
> @@ -2943,7 +2985,7 @@ sub parse_hostfw_option {
>      if ($line =~ m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack|nf_conntrack_allow_invalid|protection_synflood|nftables):\s*(0|1)\s*$/i) {
>  	$opt = lc($1);
>  	$value = int($2);
> -    } elsif ($line =~ m/^(log_level_in|log_level_out|tcp_flags_log_level|smurf_log_level):\s*(($loglevels)\s*)?$/i) {
> +    } elsif ($line =~ m/^(log_level_in|log_level_out|log_level_forward|tcp_flags_log_level|smurf_log_level):\s*(($loglevels)\s*)?$/i) {

This is getting quite long. I know we already have such massive lines,
but I wonder if we should at least do something like
    (log_level_(?:in|out|forward)|...)
(tbh I wouldn't even condense the `(tcp_flags|smurf)_log_level`, but
somehow `log_level_(in|out|forward)` IMO looks fine...

No strong feelings though, if you think this doesn't help the readability anyway.

>  	$opt = lc($1);
>  	$value = $2 ? lc($3) : '';
>      } elsif ($line =~ m/^(nf_conntrack_helpers):\s*(((\S+)[,]?)+)\s*$/i) {
> @@ -2974,7 +3016,7 @@ sub parse_clusterfw_option {
>      } elsif ($line =~ m/^(ebtables):\s*(0|1)\s*$/i) {
>  	$opt = lc($1);
>  	$value = int($2);
> -    } elsif ($line =~ m/^(policy_(in|out)):\s*(ACCEPT|DROP|REJECT)\s*$/i) {
> +    } elsif ($line =~ m/^(policy_(in|out|forward)):\s*(ACCEPT|DROP|REJECT)\s*$/i) {

^ I mean we do it here :)

>  	$opt = lc($1);
>  	$value = uc($3);
>      } elsif ($line =~ m/^(log_ratelimit):\s*(\S+)\s*$/) {
> @@ -2987,6 +3029,24 @@ sub parse_clusterfw_option {
>      return ($opt, $value);
>  }
>  
> +sub parse_vnetfw_option {
> +    my ($line) = @_;
> +
> +    my ($opt, $value);
> +
> +    if ($line =~ m/^(enable):\s*(\d+)\s*$/i) {
> +	$opt = lc($1);
> +	$value = int($2);
> +    } elsif ($line =~ m/^(policy_forward):\s*(ACCEPT|DROP)\s*$/i) {
> +	$opt = lc($1);
> +	$value = uc($2);
> +    } else {
> +	die "can't parse option '$line'\n"
> +    }
> +
> +    return ($opt, $value);
> +}
> +
>  sub resolve_alias {
>      my ($clusterfw_conf, $fw_conf, $cidr, $scope) = @_;
>  
> @@ -3153,6 +3213,8 @@ sub generic_fw_config_parser {
>  		    ($opt, $value) = parse_clusterfw_option($line);
>  		} elsif ($rule_env eq 'host') {
>  		    ($opt, $value) = parse_hostfw_option($line);
> +		} elsif ($rule_env eq 'vnet') {
> +		    ($opt, $value) = parse_vnetfw_option($line);
>  		} else {
>  		    ($opt, $value) = parse_vmfw_option($line);
>  		}
> @@ -3292,6 +3354,10 @@ sub lock_vmfw_conf {
>      return PVE::Firewall::Helpers::lock_vmfw_conf(@_);
>  }
>  
> +sub lock_vnetfw_conf {
> +    return PVE::Firewall::Helpers::lock_vnetfw_conf(@_);
> +}
> +
>  sub load_vmfw_conf {
>      my ($cluster_conf, $rule_env, $vmid, $dir) = @_;
>  
> @@ -3318,7 +3384,7 @@ my $format_rules = sub {
>      my $raw = '';
>  
>      foreach my $rule (@$rules) {
> -	if ($rule->{type} eq  'in' || $rule->{type} eq 'out' || $rule->{type} eq 'group') {
> +	if ($rule->{type} eq  'in' || $rule->{type} eq 'out' || $rule->{type} eq 'forward' || $rule->{type} eq 'group') {

could drop the double-space after the first `eq` ;-)

and maybe it's worth switching to

    grep { $rule->{type} eq $_ } qw(in out forward group)

>  	    $raw .= '|' if defined($rule->{enable}) && !$rule->{enable};
>  	    $raw .= uc($rule->{type});
>  	    if ($rule->{macro}) {
> @@ -3794,6 +3860,50 @@ sub save_hostfw_conf {
>      }
>  }
>  
> +sub load_vnetfw_conf {
> +    my ($cluster_conf, $rule_env, $vnet, $dir) = @_;
> +
> +    $rule_env = 'vnet' if !defined($rule_env);
> +
> +    my $filename = "$vnetfw_conf_dir/$vnet.fw";
> +
> +    my $empty_conf = {
> +	rules => [],
> +	options => {},
> +    };
> +
> +    my $vnetfw_conf = generic_fw_config_parser($filename, $cluster_conf, $empty_conf, $rule_env);
> +    $vnetfw_conf->{vnet} = $vnet;
> +
> +    return $vnetfw_conf;
> +}
> +
> +sub save_vnetfw_conf {
> +    my ($vnet, $conf) = @_;
> +
> +    my $filename = "$vnetfw_conf_dir/$vnet.fw";
> +
> +    my $raw = '';
> +
> +    my $options = $conf->{options};
> +    $raw .= &$format_options($options) if $options && scalar(keys %$options);
> +
> +    my $rules = $conf->{rules};
> +    if ($rules && scalar(@$rules)) {
> +	$raw .= "[RULES]\n\n";
> +	$raw .= &$format_rules($rules, 1);
> +	$raw .= "\n";
> +    }
> +
> +    mkdir($vnetfw_conf_dir, 0755) if !-d $vnetfw_conf_dir;
> +
> +    if ($raw) {
> +	PVE::Tools::file_set_contents($filename, $raw);
> +    } else {
> +	unlink $filename;
> +    }
> +}
> +
>  sub compile {
>      my ($cluster_conf, $hostfw_conf, $vmdata, $corosync_conf) = @_;
>  
> diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm
> index 7dcbca3..0b465ae 100644
> --- a/src/PVE/Firewall/Helpers.pm
> +++ b/src/PVE/Firewall/Helpers.pm
> @@ -32,6 +32,18 @@ sub lock_vmfw_conf {
>      return $res;
>  }
>  
> +sub lock_vnetfw_conf {
> +    my ($vnet, $timeout, $code, @param) = @_;
> +
> +    die "can't lock vnet firewall config for undefined vnet\n"
> +	if !defined($vnet);
> +
> +    my $res = PVE::Cluster::cfs_lock_firewall("vnet-$vnet", $timeout, $code, @param);
> +    die $@ if $@;
> +
> +    return $res;
> +}
> +
>  sub remove_vmfw_conf {
>      my ($vmid) = @_;
>  
> -- 
> 2.39.5


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