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 9B01196C46 for ; Thu, 26 Jan 2023 15:32:25 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7B45024C05 for ; Thu, 26 Jan 2023 15:31:55 +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:31:54 +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 03D56465A6 for ; Thu, 26 Jan 2023 15:31:54 +0100 (CET) From: Leo Nunner To: pve-devel@lists.proxmox.com Date: Thu, 26 Jan 2023 15:31:27 +0100 Message-Id: <20230126143127.151018-3-l.nunner@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230126143127.151018-1-l.nunner@proxmox.com> References: <20230126143127.151018-1-l.nunner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 2.376 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 RCVD_IN_DNSWL_HI -5 Sender listed at https://www.dnswl.org/, high trust 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. [ipset.pm, groups.pm, helpers.pm, aliases.pm] Subject: [pve-devel] [PATCH v2 firewall 2/2] fix #4414: automatically rename usages of aliases and IPsets 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:32:25 -0000 Renaming an alias or an IPset broke existing firewall rules, both standalone and as part of a security group. This was already fixed with security groups (see issue #4204), so it was only a matter of factoring out the relevant code and providing a more general implementation, so that it could be reused for the other components. The mechanism works the same for all three components: first, the existing object is duplicated with a new name. Then, all the occurences inside rules are replaced with the new name, and, when this is done, the old object is deleted. This ensures that a failure while updating (such as a timeout due to the inability to lock a config file) does not break the rules that could not be updated yet. In PVE::Firewall::Helpers, a function is now provided to globally update all configs (nodes, VMs, CTs) using a custom function that is passed as a parameter. Each config is locked, passed to the function and then written. Signed-off-by: Leo Nunner --- src/PVE/API2/Firewall/Aliases.pm | 52 +++++++++++++++++++++++++++--- src/PVE/API2/Firewall/Groups.pm | 54 ++++++-------------------------- src/PVE/API2/Firewall/IPSet.pm | 47 +++++++++++++++++++++++++-- src/PVE/Firewall/Helpers.pm | 33 +++++++++++++++++++ 4 files changed, 135 insertions(+), 51 deletions(-) diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm index c84c348..4c0163f 100644 --- a/src/PVE/API2/Firewall/Aliases.pm +++ b/src/PVE/API2/Firewall/Aliases.pm @@ -249,15 +249,59 @@ sub register_update_alias { raise_param_exc({ name => "no such alias" }) if !$aliases->{$rename_from}; - my $data = { name => $rename_from, cidr => $param->{cidr} // $aliases->{$rename_from}->{cidr} }; - $data->{comment} = $param->{comment} if $param->{comment}; - - $aliases->{$rename_from} = $data; + my $alias = $aliases->{$rename_from}; + $alias->{cidr} = $param->{cidr} if defined($param->{cidr}); + $alias->{comment} = $param->{comment} if defined($param->{comment}); if ($rename_to && ($rename_from ne $rename_to)) { raise_param_exc({ name => "alias '$param->{rename}' already exists" }) if grep { lc($_) eq lc($rename_to) } (keys $aliases->%*); $aliases->{$rename_to} = $aliases->{$rename_from}; + $class->save_aliases($param, $fw_conf, $aliases); + + my $rename_fw_rules = sub { + my ($config) = @_; + + for my $rule ($config->{rules}->@*) { + next if $rule->{type} eq "group"; + $rule->{source} = $rename_to if lc($rule->{source}) eq lc($rename_from); + $rule->{dest} = $rename_to if lc($rule->{dest}) eq lc($rename_from); + } + }; + + my $rename_ipsets = sub { + my ($config) = @_; + + for my $ipset (values $config->{ipset}->%*) { + for my $ip ($ipset->{entries}->@*) { + $ip->{cidr} = $rename_to if lc($ip->{cidr}) eq lc($rename_from); + } + } + }; + + # Figure out scope + if ($class->rule_env() eq "cluster") { + # Update rules and IPsets + PVE::Firewall::Helpers::global_update_configs($fw_conf, sub { + my ($config) = @_; + + $rename_fw_rules->($config); + $rename_ipsets->($config); + }); + + # Update group definitions + for my $group (values $fw_conf->{groups}->%*) { + for my $rule ($group->{rules}->@*) { + $rule->{source} = $rename_to if lc($rule->{source}) eq lc($rename_from); + $rule->{dest} = $rename_to if lc($rule->{dest}) eq lc($rename_from); + } + } + } + + $rename_fw_rules->($fw_conf); + $rename_ipsets->($fw_conf); + + # Now we can delete the old one delete $aliases->{$rename_from}; } diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm index 3bf01ac..524fd46 100644 --- a/src/PVE/API2/Firewall/Groups.pm +++ b/src/PVE/API2/Firewall/Groups.pm @@ -28,15 +28,6 @@ my $get_security_group_list = sub { return wantarray ? ($list, $digest) : $list; }; -my $rename_fw_rules = sub { - my ($old, $new, $rules) = @_; - - for my $rule (@{$rules}) { - next if ($rule->{type} ne "group" || $rule->{action} ne $old); - $rule->{action} = $new; - } -}; - __PACKAGE__->register_method({ name => 'list_security_groups', path => '', @@ -131,44 +122,19 @@ __PACKAGE__->register_method({ # rules won't be broken when the new name is referenced PVE::Firewall::save_clusterfw_conf($cluster_conf); - # Update all the host configs to the new copy - my $hosts = PVE::Cluster::get_nodelist(); - foreach my $host (@$hosts) { - PVE::Firewall::lock_hostfw_conf($host, 10, sub { - my $host_conf_path = "/etc/pve/nodes/$host/host.fw"; - my $host_conf = PVE::Firewall::load_hostfw_conf($cluster_conf, $host_conf_path); - - if (defined($host_conf)) { - &$rename_fw_rules($rename_from, - $rename_to, - $host_conf->{rules}); - PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path); - } - }); - } + my $rename_fw_rules = sub { + my ($config) = @_; - # Update all the VM configs - my $vms = PVE::Cluster::get_vmlist(); - foreach my $vm (keys %{$vms->{ids}}) { - PVE::Firewall::lock_vmfw_conf($vm, 10, sub { - my $vm_type = $vms->{ids}->{$vm}->{type} eq "lxc" ? "ct" : "vm"; - my $vm_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, $vm_type, $vm, "/etc/pve/firewall"); - - if (defined($vm_conf)) { - &$rename_fw_rules($rename_from, - $rename_to, - $vm_conf->{rules}); - PVE::Firewall::save_vmfw_conf($vm, $vm_conf); - } - }); - } + for my $rule (@{$config->{rules}}) { + next if ($rule->{type} ne "group" || $rule->{action} ne $rename_from); + $rule->{action} = $rename_to; + } + }; - # And also update the cluster itself - &$rename_fw_rules($rename_from, - $rename_to, - $cluster_conf->{rules}); + # Update VM/CT/cluster rules + PVE::Firewall::Helpers::global_update_configs($cluster_conf, $rename_fw_rules); + $rename_fw_rules->($cluster_conf); - # Now that everything has been updated, the old rule can be deleted delete $cluster_conf->{groups}->{$rename_from}; } else { # In this context, $rename_to is the name for the new group diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm index aae9006..6e0829a 100644 --- a/src/PVE/API2/Firewall/IPSet.pm +++ b/src/PVE/API2/Firewall/IPSet.pm @@ -642,6 +642,10 @@ sub register_create { code => sub { my ($param) = @_; + my $rename_to = $param->{name}; + my $rename_from = $param->{rename}; + my $comment = $param->{comment}; + $class->lock_config($param, sub { my ($param) = @_; @@ -663,9 +667,46 @@ sub register_create { if (grep { lc($_) eq lc($rename_to) } (keys $fw_conf->{ipset}->%*)) && $rename_to ne $rename_from; - my $data = delete $fw_conf->{ipset}->{$rename_from}; - $fw_conf->{ipset}->{$rename_to} = $data; - $fw_conf->{ipset}->{$rename_to}->{comment} = $param->{comment} if defined($param->{comment}); + if ($rename_from eq $rename_to) { + $fw_conf->{ipset}->{$rename_from}->{comment} = $comment if defined($comment); + $class->save_config($param, $fw_conf); + return; + } + + # Create a reference to the old IPset, so that both the old and the new name point to the same set + $fw_conf->{ipset}->{$rename_to} = $fw_conf->{ipset}->{$rename_from}; + + # Update comment if provided + $fw_conf->{ipset}->{$rename_to}->{comment} = $comment if defined($comment); + + $class->save_config($param, $fw_conf); + + my $rename_fw_rules = sub { + my ($config) = @_; + + for my $rule (@{$config->{rules}}) { + next if $rule->{type} eq "group"; + $rule->{source} = "+$rename_to" if $rule->{source} eq "+$rename_from"; + $rule->{dest} = "+$rename_to" if $rule->{dest} eq "+$rename_from"; + } + }; + + # Figure out scope + if ($class->rule_env() eq "cluster") { + PVE::Firewall::Helpers::global_update_configs($fw_conf, $rename_fw_rules); + + # Update group definitions + for my $group (values $fw_conf->{groups}->%*) { + for my $rule ($group->{rules}->@*) { + $rule->{source} = "+$rename_to" if $rule->{source} eq "+$rename_from"; + $rule->{dest} = "+$rename_to" if $rule->{dest} eq "+$rename_from"; + } + } + } + + $rename_fw_rules->($fw_conf); + + delete $fw_conf->{ipset}->{$rename_from}; } else { # In this context, $rename_to is the name for the new IPSet raise_param_exc({ name => "IPSet '$rename_to' already exists" }) diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm index 154fca5..3137b33 100644 --- a/src/PVE/Firewall/Helpers.pm +++ b/src/PVE/Firewall/Helpers.pm @@ -11,6 +11,7 @@ our @EXPORT_OK = qw( lock_vmfw_conf remove_vmfw_conf clone_vmfw_conf +global_update_configs ); my $pvefw_conf_dir = "/etc/pve/firewall"; @@ -52,4 +53,36 @@ sub clone_vmfw_conf { }); } +# Updates all VM and CT configs. For every host/VM/CT, a custom function +# $fun is called with the config object as a parameter. +sub global_update_configs { + my ($cluster_conf, $fun) = @_; + + my $hosts = PVE::Cluster::get_nodelist(); + foreach my $host (@$hosts) { + PVE::Firewall::lock_hostfw_conf($host, 10, sub { + my $host_conf_path = "/etc/pve/nodes/$host/host.fw"; + my $host_conf = PVE::Firewall::load_hostfw_conf($cluster_conf, $host_conf_path); + + if (defined($host_conf)) { + $fun->($host_conf); + PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path); + } + }); + } + + my $vms = PVE::Cluster::get_vmlist(); + foreach my $vm (keys %{$vms->{ids}}) { + PVE::Firewall::lock_vmfw_conf($vm, 10, sub { + my $vm_type = $vms->{ids}->{$vm}->{type} eq "lxc" ? "ct" : "vm"; + my $vm_conf = PVE::Firewall::load_vmfw_conf($cluster_conf, $vm_type, $vm, "/etc/pve/firewall"); + + if (defined($vm_conf)) { + $fun->($vm_conf); + PVE::Firewall::save_vmfw_conf($vm, $vm_conf); + } + }); + } +} + 1; -- 2.30.2