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 C178F92416 for ; Wed, 28 Dec 2022 10:37:22 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A428422F7C for ; Wed, 28 Dec 2022 10:37:22 +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 ; Wed, 28 Dec 2022 10:37:19 +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 644CA45231 for ; Wed, 28 Dec 2022 10:37:19 +0100 (CET) Message-ID: Date: Wed, 28 Dec 2022 10:37:16 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Content-Language: en-US To: Wolfgang Bumiller Cc: pve-devel@lists.proxmox.com References: <20221222155232.149115-1-l.nunner@proxmox.com> <20221223103525.4kth7kuzllqzlnnv@fwblub> From: Leo Nunner In-Reply-To: <20221223103525.4kth7kuzllqzlnnv@fwblub> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.561 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 NICE_REPLY_A -1.147 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [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: Wed, 28 Dec 2022 09:37:22 -0000 On 2022-12-23 11:36, Wolfgang Bumiller wrote: > On Thu, Dec 22, 2022 at 04:52:32PM +0100, Leo Nunner wrote: >> 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? > Better than duplicate code, though I'm beginning to have a strong > aversion to modules named 'Helpers' or 'Tools' and would rather have > this in `PVE::Firewall` even if it's quite full. > BUT, that's for another time, for now, keep it that way, maybe at some > later point we can find a better way to organize this for the whole > Firewall.pm. Alright, I think for a future patch it shouldn't be too hard to merge this back into another file, since it's basically just a collection of functions to manipulate config files… >> - 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. > Generally speaking, yes, separating patches are usually better since we > can also apply partial series to move forward a bit more easily, making > future versions of series containing fewer changes. > > If I'm seeing this right it'll amount to a single line change in this > case, (2, but the 2nd patch would have to touch the same other yet > again), so one would think it doesn't matter much, yet I did end up > looking for remaining `$param->{cidr}` uses in the file, (which were > unlikely given the change), which would end up being one thing less to > worry about... > > So yeah... if it's easy, I'd prefer separate patches, but there can of > course be situations where that's more work than it's worth. I'll try to see how I can best separate this into two patches. >> 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 }; > A quicker way to make a shallow clone of a hash is to just put the `%` > expansion inside braces: > $aliases->{$rename} = { %$alias }; > > or actually with arrow-dereferencing it looks a bit more explicit IMO, > but optional (in this case ;-), see below): > > $aliases->{$rename} = { $alias->%* }; I considered going with the first solution actually, but thought it might be more readable the other way ^^ I'll change it to the dereferencing style. >> + $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}}) { > For anything other than a simple `%$something` code is much easier to > follow when sticking to consistent arrows: > $config->{ipset}->%* > instead of > %{$config->{ipset}} > since the latter creates "spirals" akin to cdecls (just do an > image-search for cdecl spiral :-P) > Or compare: > - You a $config, you take its `ipset` member, and of that you take > the content. > - You take the content of the thing you get when you take the config and > then fetch its `ipset` member. That does seem like a cleaner approach, thanks! >> + map { $_->{cidr} = $rename if $_->{cidr} eq $name } @{$ipset}; > Don't use `map` as a replacement for `for` loops just to cram it into a > single line with a suffix-`if` ;-) > Use `for`. Will do. >> + } >> + }; >> + >> + # Figure out scope >> + if ($class->rule_env() eq "cluster") { > Do we need to go through the files twice here? Can we not do all 3 > replacements in 1 run? > Since this can be quite expensive as it'll take cluster-wide locks of > potentially lots of files... twice. True, calling rename_fw_rules and rename_ipsets in a single run does make way more sense. The group replacement needs to be separate since groups can only be declared on a cluster-wide level, but it doesn't do any locking on its own anyway. >> + 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" }) > Okay I'm confused. > Do we really have `rename` mean "rename-TO" in one case and > "rename-FROM" in the other? 😒 > If so, can we please use `$rename_from` and `$rename_to` as variable > names, because this is highly annoying. > Also maybe add a patch to expand the `rename` parameter documentation to > include what it actually does... Yep, with groups and IPsets, "rename" specifies the object to be renamed, while with aliases, it's the new name… I'll send a patch to update the documentation, and will rename the variables accordingly. >> - 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 > ^ Maybe note that we're only referencing it and not actually copying > since we don't need to touch the contents, unless I'm missing something > and this should instead perform a copy like in the alias case? Will do, a copying operation isn't necessary here. >> + $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; > Hold on... 🧐 > So either I'm missing something or this should just not be a loop? > just use > raise_... > if $fw_conf->{ipset}->{$name}; > like in the other branch? The same loop seems to exist in the groups endpoint too [1] 🤔. But I agree, it doesn't make much sense to have a loop here… [1] https://git.proxmox.com/?p=pve-firewall.git;a=blob;f=src/PVE/API2/Firewall/Groups.pm;h=ffdc45c1889e3d2af1906e412bca2601601c595f;hb=HEAD#l180 >> } >> >> - $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