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 32F2091A35 for ; Thu, 22 Dec 2022 16:55:22 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 119161DD45 for ; Thu, 22 Dec 2022 16:54:52 +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, 22 Dec 2022 16:54: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 9B32043965 for ; Thu, 22 Dec 2022 16:54:49 +0100 (CET) From: Leo Nunner To: pve-devel@lists.proxmox.com Date: Thu, 22 Dec 2022 16:52:32 +0100 Message-Id: <20221222155232.149115-1-l.nunner@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.012 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 Subject: [pve-devel] [PATCH firewall] 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, 22 Dec 2022 15:55:22 -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. The update endpoint for aliases was also cleaned up to make the needed parameters more logical, i.e. just updating the name/comment does not require a CIDR to be passed along as well anymore. Signed-off-by: Leo Nunner --- RFC: - I'm not really sure if the approach with the function in PVE::Firewall::Helpers is the best option, but it might also be useful in other places? - Should the cleanup in the update endpoint for aliases go in a separate patch? I figured I might as well do that now, since I was changing quite a few things around anyway in that function. src/PVE/API2/Firewall/Aliases.pm | 59 ++++++++++++++++++++++---- src/PVE/API2/Firewall/Groups.pm | 54 +++++------------------- src/PVE/API2/Firewall/IPSet.pm | 72 +++++++++++++++++++++++++------- src/PVE/Firewall/Helpers.pm | 33 +++++++++++++++ 4 files changed, 149 insertions(+), 69 deletions(-) diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm index 33ac669..b154454 100644 --- a/src/PVE/API2/Firewall/Aliases.pm +++ b/src/PVE/API2/Firewall/Aliases.pm @@ -205,6 +205,7 @@ sub register_update_alias { $properties->{name} = $api_properties->{name}; $properties->{rename} = $api_properties->{rename}; $properties->{cidr} = $api_properties->{cidr}; + $properties->{cidr}->{optional} = 1; $properties->{comment} = $api_properties->{comment}; $properties->{digest} = get_standard_option('pve-config-digest'); @@ -235,22 +236,62 @@ sub register_update_alias { PVE::Tools::assert_if_modified($digest, $param->{digest}); my $name = lc($param->{name}); + my $rename = lc($param->{rename}) if defined($param->{rename}); raise_param_exc({ name => "no such alias" }) if !$aliases->{$name}; - my $data = { name => $param->{name}, cidr => $param->{cidr} }; - $data->{comment} = $param->{comment} if $param->{comment}; - - $aliases->{$name} = $data; - - my $rename = $param->{rename}; - $rename = lc($rename) if $rename; + my $alias = $aliases->{$name}; + $alias->{cidr} = $param->{cidr} if defined($param->{cidr}); + $alias->{comment} = $param->{comment} if defined($param->{comment}); if ($rename && ($name ne $rename)) { raise_param_exc({ name => "alias '$param->{rename}' already exists" }) if defined($aliases->{$rename}); - $aliases->{$name}->{name} = $param->{rename}; - $aliases->{$rename} = $aliases->{$name}; + + # Don't delete old alias yet, only copy it, change the name, and save. + $aliases->{$rename}->{$_} = $alias->{$_} for keys %{ $alias }; + $aliases->{$rename}->{name} = $param->{rename}; + + $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 if $rule->{source} eq $name; + $rule->{dest} = $rename if $rule->{dest} eq $name; + } + }; + + my $rename_ipsets = sub { + my ($config) = @_; + + for my $ipset (values %{$config->{ipset}}) { + map { $_->{cidr} = $rename if $_->{cidr} eq $name } @{$ipset}; + } + }; + + # 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}) { + $rule->{source} = $rename if $rule->{source} eq $name; + $rule->{dest} = $rename if $rule->{dest} eq $name; + } + } + + # Update IPsets, since aliases can also be used here + PVE::Firewall::Helpers::global_update_configs($fw_conf, $rename_ipsets); + } + + $rename_fw_rules->($fw_conf); + $rename_ipsets->($fw_conf); + + # Now we can delete the old one delete $aliases->{$name}; } diff --git a/src/PVE/API2/Firewall/Groups.pm b/src/PVE/API2/Firewall/Groups.pm index ffdc45c..6080c6d 100644 --- a/src/PVE/API2/Firewall/Groups.pm +++ b/src/PVE/API2/Firewall/Groups.pm @@ -30,15 +30,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 => '', @@ -136,44 +127,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, - $group, - $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, - $group, - $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); + $rule->{action} = $group; + } + }; - # And also update the cluster itself - &$rename_fw_rules($rename, - $group, - $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}; delete $cluster_conf->{group_comments}->{$rename}; } else { diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm index 14bcfcb..aeb462d 100644 --- a/src/PVE/API2/Firewall/IPSet.pm +++ b/src/PVE/API2/Firewall/IPSet.pm @@ -642,37 +642,77 @@ sub register_create { code => sub { my ($param) = @_; + my $name = $param->{name}; + my $rename = $param->{rename}; + my $comment = $param->{comment}; + $class->lock_config($param, sub { my ($param) = @_; my ($cluster_conf, $fw_conf) = $class->load_config($param); - if ($param->{rename}) { + if ($rename) { my (undef, $digest) = &$get_ipset_list($fw_conf); PVE::Tools::assert_if_modified($digest, $param->{digest}); - raise_param_exc({ name => "IPSet '$param->{rename}' does not exist" }) - if !$fw_conf->{ipset}->{$param->{rename}}; + raise_param_exc({ name => "IPSet '$rename' does not exist" }) + if !$fw_conf->{ipset}->{$rename}; # prevent overwriting existing ipset - raise_param_exc({ name => "IPSet '$param->{name}' does already exist"}) - if $fw_conf->{ipset}->{$param->{name}} && - $param->{name} ne $param->{rename}; - - my $data = delete $fw_conf->{ipset}->{$param->{rename}}; - $fw_conf->{ipset}->{$param->{name}} = $data; - if (my $comment = delete $fw_conf->{ipset_comments}->{$param->{rename}}) { - $fw_conf->{ipset_comments}->{$param->{name}} = $comment; + raise_param_exc({ name => "IPSet '$name' does already exist"}) + if $fw_conf->{ipset}->{$name} && + $name ne $rename; + + if ($rename eq $name) { + $fw_conf->{ipset_comments}->{$rename} = $comment if defined($comment); + $class->save_config($param, $fw_conf); + return; } - $fw_conf->{ipset_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment}); + + # Create a copy of the old IPset + $fw_conf->{ipset}->{$name} = $fw_conf->{ipset}->{$rename}; + $fw_conf->{ipset_comments}->{$name} = $fw_conf->{ipset_comments}->{$rename}; + + # Update comment if provided + $fw_conf->{ipset_comments}->{$name} = $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} = "+$name" if $rule->{source} eq "+$rename"; + $rule->{dest} = "+$name" if $rule->{dest} eq "+$rename"; + } + }; + + # 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}) { + $rule->{source} = "+$name" if $rule->{source} eq "+$rename"; + $rule->{dest} = "+$name" if $rule->{dest} eq "+$rename"; + } + } + } + + $rename_fw_rules->($fw_conf); + + delete $fw_conf->{ipset}->{$rename}; + delete $fw_conf->{ipset_comments}->{$rename}; } else { - foreach my $name (keys %{$fw_conf->{ipset}}) { + foreach my $set (keys %{$fw_conf->{ipset}}) { raise_param_exc({ name => "IPSet '$name' already exists" }) - if $name eq $param->{name}; + if $set eq $name; } - $fw_conf->{ipset}->{$param->{name}} = []; - $fw_conf->{ipset_comments}->{$param->{name}} = $param->{comment} if defined($param->{comment}); + $fw_conf->{ipset}->{$name} = []; + $fw_conf->{ipset_comments}->{$name} = $comment if defined($comment); } $class->save_config($param, $fw_conf); 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