public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets
@ 2022-12-22 15:52 Leo Nunner
  2022-12-23 10:36 ` Wolfgang Bumiller
  0 siblings, 1 reply; 3+ messages in thread
From: Leo Nunner @ 2022-12-22 15:52 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-12-28  9:37 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-22 15:52 [pve-devel] [PATCH firewall] fix 4414: automatically rename usages of aliases and IPsets Leo Nunner
2022-12-23 10:36 ` Wolfgang Bumiller
2022-12-28  9:37   ` Leo Nunner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal