From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 1CA9E1FF164 for ; Fri, 8 Nov 2024 15:00:27 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C32B9EF63; Fri, 8 Nov 2024 15:00:25 +0100 (CET) Date: Fri, 8 Nov 2024 14:59:51 +0100 From: Wolfgang Bumiller To: Stefan Hanreich Message-ID: References: <20241010155650.255698-1-s.hanreich@proxmox.com> <20241010155650.255698-11-s.hanreich@proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20241010155650.255698-11-s.hanreich@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.079 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [helpers.pm, firewall.pm] Subject: Re: [pve-devel] [PATCH pve-firewall v2 10/17] sdn: add vnet firewall configuration X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: pve-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Thu, Oct 10, 2024 at 05:56:43PM GMT, Stefan Hanreich wrote: > Signed-off-by: Stefan Hanreich > --- > 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