From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D0AA096C9F for ; Thu, 26 Jan 2023 15:31:24 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EEA4924A01 for ; Thu, 26 Jan 2023 15:30:53 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 26 Jan 2023 15:30:49 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A803B465B2 for ; Thu, 26 Jan 2023 15:30:48 +0100 (CET) From: Leo Nunner To: pve-devel@lists.proxmox.com Date: Thu, 26 Jan 2023 15:30:19 +0100 Message-Id: <20230126143020.150338-5-l.nunner@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230126143020.150338-1-l.nunner@proxmox.com> References: <20230126143020.150338-1-l.nunner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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. [rules.pm, cluster.pm, vm.pm, groups.pm, ipset.pm] Subject: [pve-devel] [PATCH firewall 4/4] config: combine group/ipset and their comments 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: , X-List-Received-Date: Thu, 26 Jan 2023 14:31:24 -0000 This patch restructures the parsed config structure a bit to be more consistent across objects. group_comments and ipset_comments were removed from the config structure and are now stored directly within the group/ipset objects themselves. They now follow the same structure as aliases, with => { comment => <...>, [entries|rules] => { <...> }, } We don't need to store separate instances of the original + the lower-case name for aliases anymore, so the structure was changed to => { comment => <...>, cidr => <...>, ipversion => <...>, } Signed-off-by: Leo Nunner --- RFC: This one is optional, it's just that while experimenting with the capitalization issue I also looked into using a "name" property for everything (like for aliases), and while I was at it, I also transfered the comments into the main object… I feel like this structure is nicer, but we don't _need_ it. My main worry is that there might still be some calls to $conf->{ipset}->{foo} instead of $conf->{ipset}->{foo}->{entries}, but I couldn't find any aside from the ones modified in this patch ^^ src/PVE/API2/Firewall/Cluster.pm | 2 +- src/PVE/API2/Firewall/Groups.pm | 22 +++++++------- src/PVE/API2/Firewall/IPSet.pm | 23 +++++++------- src/PVE/API2/Firewall/Rules.pm | 4 +-- src/PVE/API2/Firewall/VM.pm | 2 +- src/PVE/Firewall.pm | 51 +++++++++++++++++--------------- 6 files changed, 53 insertions(+), 51 deletions(-) diff --git a/src/PVE/API2/Firewall/Cluster.pm b/src/PVE/API2/Firewall/Cluster.pm index c9c3e67..63cfb98 100644 --- a/src/PVE/API2/Firewall/Cluster.pm +++ b/src/PVE/API2/Firewall/Cluster.pm @@ -261,7 +261,7 @@ __PACKAGE__->register_method({ name => $name, ref => "+$name", }; - if (my $comment = $conf->{ipset_comments}->{$name}) { + if (my $comment = $conf->{ipset}->{$name}->{comment}) { $data->{comment} = $comment; } push @$res, $data; diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm index 6d30c53..3bf01ac 100644 --- a/src/PVE/API2/Firewall/Groups.pm +++ b/src/PVE/API2/Firewall/Groups.pm @@ -18,10 +18,8 @@ my $get_security_group_list = sub { foreach my $group (sort keys %{$cluster_conf->{groups}}) { my $data = { group => $group, + comment => $cluster_conf->{groups}->{$group}->{comment}, }; - if (my $comment = $cluster_conf->{group_comments}->{$group}) { - $data->{comment} = $comment; - } push @$res, $data; } @@ -120,17 +118,14 @@ __PACKAGE__->register_method({ $rename_to ne $rename_from; if ($rename_from eq $rename_to) { - $cluster_conf->{group_comments}->{$rename_from} = $comment if defined($comment); + $cluster_conf->{groups}->{$rename_from}->{comment} = $comment if defined($comment); PVE::Firewall::save_clusterfw_conf($cluster_conf); return; } # Create an exact copy of the old security group - $cluster_conf->{groups}->{$rename_to} = $cluster_conf->{groups}->{$rename_from}; - $cluster_conf->{group_comments}->{$rename_to} = $cluster_conf->{group_comments}->{$rename_from}; - - # Update comment if provided - $cluster_conf->{group_comments}->{$rename_to} = $comment if defined($comment); + $cluster_conf->{groups}->{$rename_to} = { $cluster_conf->{groups}->{$rename_from}->%* }; + $cluster_conf->{groups}->{$rename_to}->{comment} = $comment if defined($comment); # Write the copy to the cluster config, so that if something fails inbetween, the new firewall # rules won't be broken when the new name is referenced @@ -175,14 +170,17 @@ __PACKAGE__->register_method({ # Now that everything has been updated, the old rule can be deleted delete $cluster_conf->{groups}->{$rename_from}; - delete $cluster_conf->{group_comments}->{$rename_from}; } else { # In this context, $rename_to is the name for the new group raise_param_exc({ group => "Security group '$rename_to' already exists" }) if grep { lc($_) eq lc($rename_to) } (keys $cluster_conf->{groups}->%*); - $cluster_conf->{groups}->{$rename_to} = []; - $cluster_conf->{group_comments}->{$rename_to} = $comment if defined($comment); + my $data = { + comment => $comment, + rules => [], + }; + + $cluster_conf->{groups}->{$rename_to} = $data; } PVE::Firewall::save_clusterfw_conf($cluster_conf); diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm index bb7f098..aae9006 100644 --- a/src/PVE/API2/Firewall/IPSet.pm +++ b/src/PVE/API2/Firewall/IPSet.pm @@ -57,7 +57,7 @@ sub save_ipset { if (!defined($ipset)) { delete $fw_conf->{ipset}->{$param->{name}}; } else { - $fw_conf->{ipset}->{$param->{name}} = $ipset; + $fw_conf->{ipset}->{$param->{name}}->{entries} = $ipset; } $class->save_config($param, $fw_conf); @@ -400,7 +400,7 @@ sub load_config { my ($class, $param) = @_; my $fw_conf = PVE::Firewall::load_clusterfw_conf(); - my $ipset = $fw_conf->{ipset}->{$param->{name}}; + my $ipset = $fw_conf->{ipset}->{$param->{name}}->{entries}; die "no such IPSet '$param->{name}'\n" if !defined($ipset); return (undef, $fw_conf, $ipset); @@ -444,7 +444,7 @@ sub load_config { my $cluster_conf = PVE::Firewall::load_clusterfw_conf(); my $fw_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, 'vm', $param->{vmid}); - my $ipset = $fw_conf->{ipset}->{$param->{name}}; + my $ipset = $fw_conf->{ipset}->{$param->{name}}->{entries}; die "no such IPSet '$param->{name}'\n" if !defined($ipset); return ($cluster_conf, $fw_conf, $ipset); @@ -488,7 +488,7 @@ sub load_config { my $cluster_conf = PVE::Firewall::load_clusterfw_conf(); my $fw_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, 'ct', $param->{vmid}); - my $ipset = $fw_conf->{ipset}->{$param->{name}}; + my $ipset = $fw_conf->{ipset}->{$param->{name}}->{entries}; die "no such IPSet '$param->{name}'\n" if !defined($ipset); return ($cluster_conf, $fw_conf, $ipset); @@ -562,7 +562,7 @@ my $get_ipset_list = sub { my $data = { name => $name, }; - if (my $comment = $fw_conf->{ipset_comments}->{$name}) { + if (my $comment = $fw_conf->{ipset}->{$name}->{comment}) { $data->{comment} = $comment; } push @$res, $data; @@ -665,17 +665,18 @@ sub register_create { my $data = delete $fw_conf->{ipset}->{$rename_from}; $fw_conf->{ipset}->{$rename_to} = $data; - if (my $comment = delete $fw_conf->{ipset_comments}->{$rename_from}) { - $fw_conf->{ipset_comments}->{$rename_to} = $comment; - } - $fw_conf->{ipset_comments}->{$rename_to} = $param->{comment} if defined($param->{comment}); + $fw_conf->{ipset}->{$rename_to}->{comment} = $param->{comment} if defined($param->{comment}); } else { # In this context, $rename_to is the name for the new IPSet raise_param_exc({ name => "IPSet '$rename_to' already exists" }) if grep { lc($_) eq lc($rename_to) } (keys $fw_conf->{ipset}->%*); - $fw_conf->{ipset}->{$rename_to} = []; - $fw_conf->{ipset_comments}->{$rename_to} = $param->{comment} if defined($param->{comment}); + my $data = { + comment => $comment, + entries => [], + }; + + $fw_conf->{ipset}->{$rename_to} = $data; } $class->save_config($param, $fw_conf); diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm index 9fcfb20..30e1050 100644 --- a/src/PVE/API2/Firewall/Rules.pm +++ b/src/PVE/API2/Firewall/Rules.pm @@ -413,7 +413,7 @@ sub load_config { my ($class, $param) = @_; my $fw_conf = PVE::Firewall::load_clusterfw_conf(); - my $rules = $fw_conf->{groups}->{$param->{group}}; + my $rules = $fw_conf->{groups}->{$param->{group}}->{rules}; die "no such security group '$param->{group}'\n" if !defined($rules); return (undef, $fw_conf, $rules); @@ -425,7 +425,7 @@ sub save_rules { if (!defined($rules)) { delete $fw_conf->{groups}->{$param->{group}}; } else { - $fw_conf->{groups}->{$param->{group}} = $rules; + $fw_conf->{groups}->{$param->{group}}->{rules} = $rules; } PVE::Firewall::save_clusterfw_conf($fw_conf); diff --git a/src/PVE/API2/Firewall/VM.pm b/src/PVE/API2/Firewall/VM.pm index 48b8c5f..5bd1944 100644 --- a/src/PVE/API2/Firewall/VM.pm +++ b/src/PVE/API2/Firewall/VM.pm @@ -269,7 +269,7 @@ sub register_handlers { name => $name, ref => "+$name", }; - if (my $comment = $conf->{ipset_comments}->{$name}) { + if (my $comment = $conf->{ipset}->{$name}->{comment}) { $data->{comment} = $comment; } $ipsets->{$name} = $data; diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm index cbb72f5..e04fc15 100644 --- a/src/PVE/Firewall.pm +++ b/src/PVE/Firewall.pm @@ -2686,7 +2686,7 @@ sub enable_host_firewall { sub generate_group_rules { my ($ruleset, $cluster_conf, $group, $ipversion) = @_; - my $rules = $cluster_conf->{groups}->{$group}; + my $rules = $cluster_conf->{groups}->{$group}->{rules}; if (!$rules) { warn "no such security group '$group'\n"; @@ -3028,9 +3028,12 @@ sub generic_fw_config_parser { next; } - $res->{$section}->{$group} = []; - $res->{group_comments}->{$group} = decode('utf8', $comment) - if $comment; + my $data = { + rules => [], + }; + $data->{comment} = decode('utf8', $comment) if $comment; + + $res->{$section}->{$group} = $data; next; } @@ -3053,11 +3056,14 @@ sub generic_fw_config_parser { next; } - $res->{$section}->{$group} = []; + my $data = { + entries => [], + }; + $data->{comment} = decode('utf8', $comment) if $comment; + + $res->{$section}->{$group} = $data; $curr_group_keys = {}; - $res->{ipset_comments}->{$group} = decode('utf8', $comment) - if $comment; next; } @@ -3100,7 +3106,7 @@ sub generic_fw_config_parser { warn "$prefix: $err"; next; } - push @{$res->{$section}->{$group}}, $rule; + push @{$res->{$section}->{$group}->{rules}}, $rule; } elsif ($section eq 'ipset') { # we can add single line comments to the end of the rule my $comment = decode('utf8', $1) if $line =~ s/#\s*(.*?)\s*$//; @@ -3144,7 +3150,7 @@ sub generic_fw_config_parser { } } - push @{$res->{$section}->{$group}}, $entry; + push @{$res->{$section}->{$group}->{entries}}, $entry; $curr_group_keys->{$cidr} = 1; } else { warn "$prefix: skip line - unknown section\n"; @@ -3221,7 +3227,6 @@ sub load_vmfw_conf { options => {}, aliases => {}, ipset => {} , - ipset_comments => {}, }; my $vmfw_conf = generic_fw_config_parser($filename, $cluster_conf, $empty_conf, $rule_env); @@ -3307,13 +3312,14 @@ my $format_ipsets = sub { my $raw = ''; foreach my $ipset (sort keys %{$fw_conf->{ipset}}) { - if (my $comment = $fw_conf->{ipset_comments}->{$ipset}) { + my $data = $fw_conf->{ipset}->{$ipset}; + if (my $comment = $data->{comment}) { my $utf8comment = encode('utf8', $comment); $raw .= "[IPSET $ipset] # $utf8comment\n\n"; } else { $raw .= "[IPSET $ipset]\n\n"; } - my $options = $fw_conf->{ipset}->{$ipset}; + my $options = $data->{entries}; my $nethash = {}; foreach my $entry (@$options) { @@ -3453,7 +3459,7 @@ sub generate_ipset_chains { foreach my $ipset (keys %{$ipsets}) { - my $options = $ipsets->{$ipset}; + my $options = $ipsets->{$ipset}->{entries}; if ($device_ips && $ipset =~ /^ipfilter-(net\d+)$/) { if (my $ips = $device_ips->{$1}) { @@ -3573,9 +3579,7 @@ sub load_clusterfw_conf { options => {}, aliases => {}, groups => {}, - group_comments => {}, ipset => {} , - ipset_comments => {}, }; my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster'); @@ -3606,8 +3610,8 @@ sub save_clusterfw_conf { if ($cluster_conf->{groups}) { foreach my $group (sort keys %{$cluster_conf->{groups}}) { - my $rules = $cluster_conf->{groups}->{$group}; - if (my $comment = $cluster_conf->{group_comments}->{$group}) { + my $rules = $cluster_conf->{groups}->{$group}->{rules}; + if (my $comment = $cluster_conf->{groups}->{$group}->{comment}) { my $utf8comment = encode('utf8', $comment); $raw .= "[group $group] # $utf8comment\n\n"; } else { @@ -3714,7 +3718,7 @@ sub compile { name => 'local_network', cidr => $localnet, ipversion => $localnet_ver }; } - push @{$cluster_conf->{ipset}->{management}}, { cidr => $localnet }; + push @{$cluster_conf->{ipset}->{management}->{entries}}, { cidr => $localnet }; my $ruleset = {}; my $rulesetv6 = {}; @@ -3866,8 +3870,7 @@ sub compile_ipsets { name => 'local_network', cidr => $localnet, ipversion => $localnet_ver }; } - push @{$cluster_conf->{ipset}->{management}}, { cidr => $localnet }; - + push @{$cluster_conf->{ipset}->{management}->{entries}}, { cidr => $localnet }; my $ipset_ruleset = {}; @@ -3893,7 +3896,7 @@ sub compile_ipsets { next if !$net->{firewall}; if ($vmfw_conf->{options}->{ipfilter} && !$ipsets->{"ipfilter-$netid"}) { - $implicit_sets->{"ipfilter-$netid"} = []; + $implicit_sets->{"ipfilter-$netid"}->{entries} = []; } my $macaddr = $net->{macaddr}; @@ -3933,7 +3936,7 @@ sub compile_ipsets { next if !$net->{firewall}; if ($vmfw_conf->{options}->{ipfilter} && !$ipsets->{"ipfilter-$netid"}) { - $implicit_sets->{"ipfilter-$netid"} = []; + $implicit_sets->{"ipfilter-$netid"}->{entries} = []; } my $macaddr = $net->{hwaddr}; @@ -3994,7 +3997,7 @@ sub compile_ebtables_filter { my $macaddr = $net->{macaddr}; my $arpfilter = []; if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) { - foreach my $ipaddr (@$ipset) { + foreach my $ipaddr ($ipset->{entries}->@*) { my($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr}); next if !$ip || ($version && $version != 4); push(@$arpfilter, $ip); @@ -4023,7 +4026,7 @@ sub compile_ebtables_filter { my $macaddr = $net->{hwaddr}; my $arpfilter = []; if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) { - foreach my $ipaddr (@$ipset) { + foreach my $ipaddr ($ipset->{entries}->@*) { my($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr}); next if !$ip || ($version && $version != 4); push(@$arpfilter, $ip); -- 2.30.2