[pve-devel] [PATCH v2 firewall] fix #4414: automatically rename usages of aliases and IPsets

[pve-devel] [PATCH v2 firewall] fix #4414: automatically rename usages of aliases and IPsets

[Leo Nunner]

This patch depends on my previous patch removing capitalization
restrictions [1].

Changes from v1:
    - factor in feedback from v1 
    - fix issues that arose from renaming objects and changing their
      capitalization. This is a separate patch, since it would exceed
      the scope of this one. Rebased accordingly.

      Note that this patch depends on _all_ the patches in [1], so also
      the optional one. In case that isn't needed, I'll rebase this
      patch.

[1] https://lists.proxmox.com/pipermail/pve-devel/2023-January/055596.html

firewall:

Leo Nunner (2):
  allow updating an alias without providing a CIDR
  fix #4414: automatically rename usages of aliases and IPsets

 src/PVE/API2/Firewall/Aliases.pm | 53 ++++++++++++++++++++++++++++---
 src/PVE/API2/Firewall/Groups.pm  | 54 ++++++--------------------------
 src/PVE/API2/Firewall/IPSet.pm   | 47 +++++++++++++++++++++++++--
 src/PVE/Firewall/Helpers.pm      | 33 +++++++++++++++++++
 4 files changed, 136 insertions(+), 51 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v2 firewall 1/2] allow updating an alias without providing a CIDR

[Leo Nunner]

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 src/PVE/API2/Firewall/Aliases.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 9f8e71e..c84c348 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -214,6 +214,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');
 
@@ -248,7 +249,7 @@ sub register_update_alias {
 
 		raise_param_exc({ name => "no such alias" }) if !$aliases->{$rename_from};
 
-		my $data = { name => $rename_from, cidr => $param->{cidr} };
+		my $data = { name => $rename_from, cidr => $param->{cidr} // $aliases->{$rename_from}->{cidr} };
 		$data->{comment} = $param->{comment} if $param->{comment};
 
 		$aliases->{$rename_from} = $data;
-- 
2.30.2

[pve-devel] [PATCH v2 firewall 2/2] fix #4414: automatically rename usages of aliases and IPsets

[Leo Nunner]

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 <l.nunner@proxmox.com>
---
 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