[PATCH firewall/manager v2 0/9] allow updating references when renaming/deleting firewall objects

[PATCH firewall/manager v2 0/9] allow updating references when renaming/deleting firewall objects

[Arthur Bied-Charreton]

Renaming or deleting a firewall object (IPSet or alias) that is still
referenced currently leaves dangling references behind. The firewall
then fails to parse the affected rules, which become inactive without
any error visible from the UI.

This is especially dangerous on renames, where a user might reasonably
expect references to follow the rename, and could walk away believing
a set of rules is active while it has been effectively disabled.

This series introduces opt-in functionality allowing the related
firewall endpoints to keep references consistent.

- rename (POST /ipset/, PUT /aliases/{name}) gains an
  'update-references' option to rewrite all references to the new name.
- delete (DELETE /ipset/{name}, DELETE /aliases/{name}) gains a
  'delete-references' option to drop the referencing rules along with
  the object.

In the UI, 'update-references' is represented as a segmented button in
the edit panels allowing to set 'no', 'yes' and 'force' (see below).
'delete-references' is a check box in the remove confirm dialog.

The reference rewriting is centralized in a new helper that scans rules
(also in security groups) and IPSet members. When called for a cluster
object, it also updates references in downstream configs (guest, host,
vnet) across the cluster, since those may reference cluster-level
objects.

On a cluster rename, the new object is persisted before its references
are rewritten, and the old object is removed only afterwards. At no
point does an on-disk state contain a reference that is not defined,
which would otherwise make a concurrent firewall compilation drop the
affected rule. The trade-off is that an interrupted cluster rename
leaves both the old and new object defined, possibly with references
to both in downstream configs.

In order to have something standing between the admin and manually
fixing references across a whole cluster's worth of configs in case of
mid-rename failure, the rename can be finished by re-issuing it. The
'update-references' option is an enum, 'no' for opting out, 'yes' for
opting in, and 'force' for forcing the rename even if an object already
exists with the target name, which is the state a partial cluster rename
would leave the config in.

Only cluster objects fan out, host and vnet configs have no IPSet/alias
section of their own, and SDN-generated (sdn/) IPSets are regenerated
from the SDN config and never persisted, so neither is touched.

Future work:

- As noted by Stefan in the v1 review [0], IPSets are auto-generated for
  SDN objects. Those IPSets can also be referenced in user-managed
  configs and would therefore benefit from a similar feature for the
  SDN object edit/delete paths. It should be relatively straightforward
  to extend the shared helper (update_refs, commit 1 in pve-firewall)
  to also handle SDN references.

Changes since v1 (thanks @Stefan):

- Fix timing issue where on rename, the cluster fan-out would update
  downstream references to use the new name before persisting it in the
  cluster config. This would cause concurrent firewall compilations to
  fail to parse some rules and drop them. [2]

- Scan vnet configs for references as well [1]

- Generalize the cluster fan-out logic for renaming/deleting object
  references, and move all shared functionality into the Helpers
  module. [2]

- Make the 'update-references' option an enum to allow retrying a
  partial rename caused by a failure during cluster fan-out.

- Rename the filter_map helper to map_items. [1]

- Use POD for doc comments. [1]

[0] https://lore.proxmox.com/all/e7348b76-a215-4189-ade9-a7c5bbbd26cf@proxmox.com/
[1] https://lore.proxmox.com/all/c161fc57-1d8e-4340-9d2c-6621d1c39af8@proxmox.com/
[2] https://lore.proxmox.com/all/055351eb-f575-46c3-ac37-01925306c449@proxmox.com/


pve-firewall:

Arthur Bied-Charreton (6):
  api: helpers: add helper to update firewall object references
  api: ipset: add option to update references on edit
  api: ipset: add option to GC references on delete
  api: aliases: add option to update references on edit
  api: aliases: add option to GC references on delete
  firewall: tests: add tests for object reference update logic

 src/PVE/API2/Firewall/Aliases.pm |  51 +++++++-
 src/PVE/API2/Firewall/IPSet.pm   |  71 +++++++++--
 src/PVE/Firewall/Helpers.pm      | 195 +++++++++++++++++++++++++++++++
 test/Makefile                    |   1 +
 test/referenceupdatetests.pl     | 178 ++++++++++++++++++++++++++++
 5 files changed, 482 insertions(+), 14 deletions(-)
 create mode 100755 test/referenceupdatetests.pl


pve-manager:

Arthur Bied-Charreton (3):
  ui: firewall: add common widgets for deleting and updating references
  ui: firewall: ipset: add controls to update/delete references on edit
  ui: firewall: aliases: add controls to update/delete references on
    edit

 www/manager6/Makefile                     |   1 +
 www/manager6/grid/FirewallAliases.js      |  74 +++++++++++-----
 www/manager6/grid/FirewallObjectCommon.js | 100 ++++++++++++++++++++++
 www/manager6/panel/IPSet.js               |  31 ++++++-
 4 files changed, 181 insertions(+), 25 deletions(-)
 create mode 100644 www/manager6/grid/FirewallObjectCommon.js


Summary over all repositories:
  9 files changed, 663 insertions(+), 39 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH pve-firewall v2 1/9] api: helpers: add helper to update firewall object references

[Arthur Bied-Charreton]

Renaming or deleting a firewall object (ipset or alias) that is still
referenced by rules or ipset members leaves dangling references. The
firewall fails to parse the affected rules and drops them, so an edit
can effectively disable a whole set of rules.

Add a shared helper that finds and rewrites (on rename) or removes (on
delete) all such references. When operating on the cluster config it
also does so for every downstream config (guest, host and vnet) across
the cluster. Matching is case-insensitive and rewritten references are
normalized to lowercase.

Downstream configs are locked and saved individually as they are
visited, so on a cluster rename the caller must persist the config with
the new object already present before calling update_refs (keep both the
old and new object until all references are migrated). Otherwise a
concurrent firewall compilation could encounter a reference to an object
that does not exist yet and drop the rule.

Object references are not guaranteed to be scoped (dc/, guest/). This is
not an issue for cluster, host and vnet configs, as in those cases the
reference can only point to an object defined in the cluster config.
Guest configs can however define their own objects. Unscoped references
in guest rules are therefore resolved by first checking for a definition
in the relevant guest config, and only then in the cluster config, to
prevent overwriting the wrong reference.

The ipset and alias endpoints build on this in the following commits.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Firewall/Helpers.pm | 195 ++++++++++++++++++++++++++++++++++++
 1 file changed, 195 insertions(+)

diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm
index fa3646c..3bdb7dc 100644
--- a/src/PVE/Firewall/Helpers.pm
+++ b/src/PVE/Firewall/Helpers.pm
@@ -18,8 +18,12 @@ our @EXPORT_OK = qw(
     clone_vmfw_conf
     collect_refs
     flush_fw_ct_entries_by_mark
+    update_refs
+    get_object_spec
 );
 
+require PVE::Firewall;
+
 my $pvefw_conf_dir = "/etc/pve/firewall";
 
 sub lock_vmfw_conf {
@@ -234,4 +238,195 @@ sub flush_fw_ct_entries_by_mark {
     );
 }
 
+=head3 map_items($items, $action, $matches)
+
+Apply C<$action> to each item C<$item> in C<$items> for which C<$matches->($item)> is true. Remove
+C<$item> from C<$items> if C<$action->($item)> returns C<undef>.
+
+Return the updated items arrayref and a boolean indicating whether any item was matched.
+
+=cut
+
+sub map_items {
+    my ($items, $action, $matches) = @_;
+    my @result;
+    my $modified = 0;
+    for my $item (($items // [])->@*) {
+        if ($matches->($item)) {
+            $modified = 1;
+            my $new = $action->($item);
+            push @result, $new if defined $new;
+        } else {
+            push @result, $item;
+        }
+    }
+    return (\@result, $modified);
+}
+
+=head3 foreach_conf_in_env($conf, $rule_env, $rewrite)
+
+Apply C<$rewrite> to the main firewall configs and, if C<$rule_env> is 'cluster', to all guest, host
+and vnet firewall configs across the cluster. Configs where C<$rewrite> returns true are saved. The
+caller is responsible for locking and saving the cluster config (C<$conf>).
+
+=cut
+
+sub foreach_conf_in_env {
+    my ($conf, $rule_env, $rewrite) = @_;
+
+    $rewrite->($conf, $rule_env, 0);
+
+    return if $rule_env ne 'cluster';
+
+    my $vmlist = PVE::Cluster::get_vmlist();
+    my $vmids = ($vmlist // {})->{ids} // {};
+    for my $vmid (keys $vmids->%*) {
+        PVE::Firewall::lock_vmfw_conf(
+            $vmid,
+            10,
+            sub {
+                my $env = $vmlist->{ids}->{$vmid}->{type} eq 'lxc' ? 'ct' : 'vm';
+                my $guest_conf = PVE::Firewall::load_vmfw_conf($conf, $env, $vmid);
+                if ($rewrite->($guest_conf, 'cluster', 1)) {
+                    PVE::Firewall::save_vmfw_conf($vmid, $guest_conf);
+                }
+            },
+        );
+    }
+
+    for my $node (PVE::Cluster::get_nodelist()->@*) {
+        my $host_conf_path = "/etc/pve/nodes/$node/host.fw";
+        PVE::Firewall::lock_hostfw_conf(
+            $node,
+            10,
+            sub {
+                my $host_conf = PVE::Firewall::load_hostfw_conf($conf, $host_conf_path);
+                if ($rewrite->($host_conf, 'cluster', 0)) {
+                    PVE::Firewall::save_hostfw_conf($host_conf, $host_conf_path);
+                }
+            },
+        );
+    }
+
+    my $vnets = (PVE::Network::SDN::Vnets::config(1) // {})->{ids} // {};
+    for my $vnet (keys $vnets->%*) {
+        PVE::Firewall::lock_vnetfw_conf(
+            $vnet,
+            10,
+            sub {
+                my $vnet_conf = PVE::Firewall::load_vnetfw_conf($conf, 'vnet', $vnet);
+                if ($rewrite->($vnet_conf, 'cluster', 0)) {
+                    PVE::Firewall::save_vnetfw_conf($vnet, $vnet_conf);
+                }
+            },
+        );
+    }
+}
+
+my $object_ref_specs = {
+    ipset => { prefix => '+', self => 'ipset' },
+    aliases => { prefix => '', self => 'aliases' },
+};
+
+=head3 get_object_spec($kind)
+
+Get the spec hash for C<$kind>. Refer to the C<update_refs> POD for details.
+
+=cut
+
+sub get_object_spec {
+    my ($kind) = @_;
+    return $object_ref_specs->{$kind};
+}
+
+=head3 rewrite_refs_in_conf($conf, $spec, $old, $new, $env, $is_guest)
+
+Rename all references (or with C<$new> undef, delete referencing rules) to C<$old> across C<$conf>.
+
+Only exposed for testing, see POD for C<update_refs> for details.
+
+=cut
+
+sub rewrite_refs_in_conf {
+    my ($conf, $spec, $old, $new, $env, $is_guest) = @_;
+
+    my $ref_fields = ['source', 'dest', 'cidr'];
+    my $shadowed = $is_guest && $conf->{ $spec->{self} }->{$old};
+
+    my $scopes = [];
+    if ($env eq 'cluster') {
+        push $scopes->@*, 'dc/';
+        push $scopes->@*, '' if !$shadowed;
+    } else {
+        push $scopes->@*, '';
+        push $scopes->@*, 'guest/';
+    }
+
+    my $prefix = $spec->{prefix};
+    my $repl = { map { ("$prefix$_$old" => defined($new) ? "$prefix$_$new" : undef) } $scopes->@* };
+
+    my $matches = sub {
+        my ($obj) = @_;
+        grep { exists($repl->{ lc($obj->{$_} // '') }) } $ref_fields->@*;
+    };
+
+    my $rewrite = sub {
+        my ($obj) = @_;
+        return undef if !defined($new);
+        for my $f ($ref_fields->@*) {
+            my $r = lc($obj->{$f} // '');
+            $obj->{$f} = $repl->{$r} if exists($repl->{$r});
+        }
+        return $obj;
+    };
+
+    my $modified = 0;
+    my ($rules, $ch) = map_items($conf->{rules}, $rewrite, $matches);
+    $conf->{rules} = $rules;
+    $modified ||= $ch;
+
+    for my $section (qw(groups ipset)) {
+        my $map = $conf->{$section} // {};
+        for my $key (keys $map->%*) {
+            ($map->{$key}, my $c) = map_items($map->{$key}, $rewrite, $matches);
+            $modified ||= $c;
+        }
+    }
+
+    return $modified;
+}
+
+=head3 update_refs($conf, $spec, $old, $new, $rule_env)
+
+Rename (or, with C<$new> undef, delete) all references to a firewall object across the environment.
+C<$spec> describes the object kind:
+
+    { prefix => '+' | '', self => 'ipset' | 'aliases' }
+
+C<prefix> is the prefix a reference carries, C<self> is the section a downstream config may use to
+shadow a same-named cluster object. References are matched in rules, security groups and IPSet
+members. Matching is case-insensitive and renames are written back lowercased.
+
+The caller is responsible for locking and saving C<$conf>.
+
+If C<$conf> is the cluster config, i.e. C<$rule_env eq 'cluster'>, guest, host and vnet configs will
+be sequentially locked, updated and saved. Therefore, if this function is called for the cluster
+environment, a I<renaming> caller must first persist C<$conf> with the new (renamed) object present,
+so references rewritten in those downstream configs do not point at a not-yet-saved object during
+concurrent compilations.
+
+=cut
+
+sub update_refs {
+    my ($conf, $spec, $old, $new, $rule_env) = @_;
+
+    my ($lc_old, $lc_new) = (lc($old), defined($new) ? lc($new) : undef);
+
+    my $code = sub {
+        my ($fw_conf, $env, $is_guest) = @_;
+        return rewrite_refs_in_conf($fw_conf, $spec, $lc_old, $lc_new, $env, $is_guest);
+    };
+
+    return foreach_conf_in_env($conf, $rule_env, $code);
+}
 1;
-- 
2.47.3

[PATCH pve-firewall v2 2/9] api: ipset: add option to update references on edit

[Arthur Bied-Charreton]

Renaming an ipset still referenced by rules leaves dangling references.
The firewall then fails to parse those rules during compilation and
drops them. The errors, while logged to the journal, are not visible
from the GUI - a rename can therefore effectively disable a whole set
of rules.

Add an 'update-references' option to the rename path to rewrite them to
the new name. For cluster ipsets, this also covers references in
downstream configs (host, guest and vnet).

The new ipset is persisted before its references are rewritten, so a
concurrent firewall compilation never observes a dangling reference. If
the cluster-wide rewrite is interrupted, it can be retried by passing
'update-references=force'.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/IPSet.pm | 59 +++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index ca7228b..0d802aa 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -528,6 +528,7 @@ package PVE::API2::Firewall::BaseIPSetList;
 
 use strict;
 use warnings;
+use PVE::Firewall::Helpers qw(update_refs get_object_spec);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Exception qw(raise_param_exc);
 use PVE::Firewall;
@@ -655,6 +656,16 @@ sub register_create {
         },
     );
 
+    $properties->{'update-references'} = {
+        type => 'string',
+        enum => ['no', 'yes', 'force'],
+        optional => 1,
+        description =>
+            "Update all references to the IPSet when renaming it. Use 'force' to also "
+            . "overwrite an existing target IPSet, e.g. to resume an interrupted rename.",
+        default => 'no',
+    };
+
     $class->register_method({
         name => 'create_ipset',
         path => '',
@@ -675,6 +686,8 @@ sub register_create {
                 sub {
                     my ($param) = @_;
 
+                    my $update_references = $param->{'update-references'} // 'no';
+
                     my ($cluster_conf, $fw_conf) = $class->load_config($param);
 
                     if ($param->{rename}) {
@@ -685,18 +698,42 @@ sub register_create {
                             if !$fw_conf->{ipset}->{ $param->{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;
+
+                        if ($param->{name} ne $param->{rename}) {
+                            raise_param_exc({
+                                name => "IPSet '$param->{name}' does already exist" })
+                                if $fw_conf->{ipset}->{ $param->{name} }
+                                && ($update_references // '') ne 'force';
+
+                            $fw_conf->{ipset}->{ $param->{name} } =
+                                $fw_conf->{ipset}->{ $param->{rename} };
+
+                            if ($update_references ne 'no') {
+                                my $env = $class->rule_env();
+                                my $spec = get_object_spec('ipset');
+                                my $old = $param->{rename};
+                                my $new = $param->{name};
+
+                                # persist the new ipset before rewriting references so a concurrent
+                                # compilation never sees a reference to a not-yet-saved ipset.
+                                $class->save_config($param, $fw_conf) if $env eq 'cluster';
+
+                                eval { update_refs($fw_conf, $spec, $old, $new, $env) };
+                                die "rename interrupted, references may be partially updated; "
+                                    . "retry with 'force' to finish: $@"
+                                    if $@;
+                            }
+
+                            delete $fw_conf->{ipset}->{ $param->{rename} };
+
+                            if (
+                                my $comment =
+                                delete $fw_conf->{ipset_comments}->{ $param->{rename} }
+                            ) {
+                                $fw_conf->{ipset_comments}->{ $param->{name} } = $comment;
+                            }
                         }
+
                         $fw_conf->{ipset_comments}->{ $param->{name} } = $param->{comment}
                             if defined($param->{comment});
                     } else {
-- 
2.47.3

[PATCH pve-firewall v2 3/9] api: ipset: add option to GC references on delete

[Arthur Bied-Charreton]

Deleting an ipset referenced by rules or security groups leaves dangling
references, which the firewall fails to parse and drops.

Add a 'delete-references' option to the delete endpoint to allow
removing such references. For cluster ipsets, this also covers all
downstream configs (host, guest and vnet).

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/IPSet.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index 0d802aa..ba233b7 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -2,6 +2,7 @@ package PVE::API2::Firewall::IPSetBase;
 
 use strict;
 use warnings;
+use PVE::Firewall::Helpers qw(update_refs get_object_spec);
 use PVE::Exception qw(raise raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -140,6 +141,12 @@ sub register_delete_ipset {
         optional => 1,
         description => 'Delete all members of the IPSet, if there are any.',
     };
+    $properties->{'delete-references'} = {
+        type => 'boolean',
+        optional => 1,
+        description => 'Delete dangling references after deleting the IPSet.',
+        default => 0,
+    };
 
     $class->register_method({
         name => 'delete_ipset',
@@ -166,6 +173,11 @@ sub register_delete_ipset {
                     die "IPSet '$param->{name}' is not empty\n"
                         if scalar(@$ipset) && !$param->{force};
 
+                    if ($param->{'delete-references'}) {
+                        my $spec = get_object_spec('ipset');
+                        update_refs($fw_conf, $spec, $param->{name}, undef, $class->rule_env());
+                    }
+
                     $class->save_ipset($param, $fw_conf, undef);
 
                 },
-- 
2.47.3

[PATCH pve-firewall v2 4/9] api: aliases: add option to update references on edit

[Arthur Bied-Charreton]

Renaming an alias still referenced by rules leaves dangling references.
The firewall then fails to parse those rules during compilation and
drops them. The errors, while logged to the journal, are not visible
from the GUI - a rename can therefore effectively disable a whole set
of rules.

Add an 'update-references' option to the rename path to rewrite them to
the new name. For cluster aliases, this also covers references in
downstream configs (host, guest and vnet).

The new alias is persisted before its references are rewritten, so a
concurrent firewall compilation never observes a dangling reference. If
the cluster-wide rewrite is interrupted, it can be retried by passing
'update-references=force'.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/Aliases.pm | 40 +++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 4f6960d..657f333 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -2,6 +2,8 @@ package PVE::API2::Firewall::AliasesBase;
 
 use strict;
 use warnings;
+
+use PVE::Firewall::Helpers qw(update_refs get_object_spec);
 use PVE::Exception qw(raise raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -219,6 +221,16 @@ sub register_update_alias {
     $properties->{comment} = $api_properties->{comment};
     $properties->{digest} = get_standard_option('pve-config-digest');
 
+    $properties->{'update-references'} = {
+        type => 'string',
+        enum => ['no', 'yes', 'force'],
+        optional => 1,
+        description =>
+            "Update all references to the alias when renaming it. Use 'force' to also "
+            . "overwrite an existing target alias, e.g. to resume an interrupted rename.",
+        default => 'no',
+    };
+
     $class->register_method({
         name => 'update_alias',
         path => '{name}',
@@ -239,6 +251,8 @@ sub register_update_alias {
                 sub {
                     my ($param) = @_;
 
+                    my $update_references = $param->{'update-references'} // 'no';
+
                     my ($fw_conf, $aliases) = $class->load_config($param);
 
                     my $list = &$aliases_to_list($aliases);
@@ -261,9 +275,28 @@ sub register_update_alias {
 
                     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};
+                            if defined($aliases->{$rename})
+                            && $update_references ne 'force';
+
+                        $aliases->{$rename} =
+                            { $aliases->{$name}->%*, name => $param->{rename} };
+
+                        if ($update_references ne 'no') {
+                            my $env = $class->rule_env();
+                            my $spec = get_object_spec('aliases');
+                            my $new_name = $param->{rename};
+
+                            # persist the new alias before rewriting references so a concurrent
+                            # compilation never sees a reference to a not-yet-saved alias.
+                            $class->save_aliases($param, $fw_conf, $aliases)
+                                if $env eq 'cluster';
+
+                            eval { update_refs($fw_conf, $spec, $name, $new_name, $env) };
+                            die "rename interrupted, references may be partially updated; "
+                                . "retry with 'force' to finish: $@"
+                                if $@;
+                        }
+
                         delete $aliases->{$name};
                     }
 
@@ -311,6 +344,7 @@ sub register_delete_alias {
                     PVE::Tools::assert_if_modified($digest, $param->{digest});
 
                     my $name = lc($param->{name});
+
                     delete $aliases->{$name};
 
                     $class->save_aliases($param, $fw_conf, $aliases);
-- 
2.47.3

[PATCH pve-firewall v2 5/9] api: aliases: add option to GC references on delete

[Arthur Bied-Charreton]

Deleting an alias referenced by rules or security groups leaves dangling
references, which the firewall fails to parse and drops.

Add a 'delete-references' option to the delete endpoint to allow
removing such references. For cluster aliases, this also covers all
downstream configs (host, guest and vnet).

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/API2/Firewall/Aliases.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/PVE/API2/Firewall/Aliases.pm b/src/PVE/API2/Firewall/Aliases.pm
index 657f333..b8ddb53 100644
--- a/src/PVE/API2/Firewall/Aliases.pm
+++ b/src/PVE/API2/Firewall/Aliases.pm
@@ -316,6 +316,12 @@ sub register_delete_alias {
 
     $properties->{name} = $api_properties->{name};
     $properties->{digest} = get_standard_option('pve-config-digest');
+    $properties->{'delete-references'} = {
+        type => 'boolean',
+        optional => 1,
+        description => 'Delete dangling references after deleting the alias.',
+        default => 0,
+    };
 
     $class->register_method({
         name => 'remove_alias',
@@ -345,6 +351,11 @@ sub register_delete_alias {
 
                     my $name = lc($param->{name});
 
+                    if ($param->{'delete-references'}) {
+                        my $spec = get_object_spec('aliases');
+                        update_refs($fw_conf, $spec, $param->{name}, undef, $class->rule_env());
+                    }
+
                     delete $aliases->{$name};
 
                     $class->save_aliases($param, $fw_conf, $aliases);
-- 
2.47.3

[PATCH pve-firewall v2 6/9] firewall: tests: add tests for object reference update logic

[Arthur Bied-Charreton]

The reference updating logic has quite a few edge cases, especially
regarding the fact that cluster objects may be shadowed by guest
objects.

Add a few tests to consolidate the intended functionality. Note that
these tests do not cover the actual iteration over the different
configs in the cluster, rather they are focused on the core reference
rewriting logic.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 test/Makefile                |   1 +
 test/referenceupdatetests.pl | 178 +++++++++++++++++++++++++++++++++++
 2 files changed, 179 insertions(+)
 create mode 100755 test/referenceupdatetests.pl

diff --git a/test/Makefile b/test/Makefile
index fea9c21..15d2ae3 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -4,6 +4,7 @@ all:
 .PHONY: check
 check:
 	./fwtester.pl
+	./referenceupdatetests.pl
 
 .PHONY: install
 install: check
diff --git a/test/referenceupdatetests.pl b/test/referenceupdatetests.pl
new file mode 100755
index 0000000..e80cb50
--- /dev/null
+++ b/test/referenceupdatetests.pl
@@ -0,0 +1,178 @@
+#!/usr/bin/perl
+
+# tests for the alias/ipset reference updating logic
+
+use lib '../src';
+
+use v5.36;
+
+use Test::More;
+
+use PVE::Firewall::Helpers;
+
+my $ipset_spec = PVE::Firewall::Helpers::get_object_spec('ipset');
+my $alias_spec = PVE::Firewall::Helpers::get_object_spec('aliases');
+
+sub rewrite($conf, $spec, $old, $new, $env, $is_guest) {
+    return PVE::Firewall::Helpers::rewrite_refs_in_conf($conf, $spec, $old, $new, $env, $is_guest);
+}
+
+subtest 'ipset: cluster rename rewrites dc/ and bare refs' => sub {
+    my $conf = {
+        rules => [
+            { source => '+dc/foo' },
+            { dest => '+foo' },
+            { source => '+bar' },
+            { source => '+dc/bar' },
+        ],
+        groups => {
+            grp => [{ source => '+dc/foo' }, { dest => '+foo' }],
+        },
+        ipset => {
+            set => [{ cidr => '10.0.0.0/8' }],
+        },
+    };
+
+    my $modified = rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 0);
+
+    ok($modified, 'reports modified');
+    is($conf->{rules}->[0]->{source}, '+dc/baz', 'dc/ ref rewritten');
+    is($conf->{rules}->[1]->{dest}, '+baz', 'bare ref rewritten');
+    is($conf->{rules}->[2]->{source}, '+bar', 'unrelated bare ref kept');
+    is($conf->{rules}->[3]->{source}, '+dc/bar', 'unrelated dc/ ref kept');
+    is($conf->{groups}->{grp}->[0]->{source}, '+dc/baz', 'group dc/ ref rewritten');
+    is($conf->{groups}->{grp}->[1]->{dest}, '+baz', 'group bare ref rewritten');
+    is($conf->{ipset}->{set}->[0]->{cidr}, '10.0.0.0/8', 'ipset member not touched');
+};
+
+subtest 'ipset: cluster delete drops matching rules' => sub {
+    my $conf = {
+        rules => [
+            { source => '+dc/foo' }, { source => '+foo' }, { source => '+other' },
+        ],
+        groups => { grp => [{ dest => '+dc/foo' }, { dest => '+keep' }] },
+    };
+
+    my $modified = rewrite($conf, $ipset_spec, 'foo', undef, 'cluster', 0);
+
+    ok($modified, 'reports modified');
+    is(scalar($conf->{rules}->@*), 1, 'matching rules dropped');
+    is($conf->{rules}->[0]->{source}, '+other', 'unrelated rule kept');
+    is(scalar($conf->{groups}->{grp}->@*), 1, 'matching group rule dropped');
+    is($conf->{groups}->{grp}->[0]->{dest}, '+keep', 'unrelated group rule kept');
+};
+
+subtest 'ipset: guest shadows cluster object' => sub {
+    my $conf = {
+        rules => [
+            { source => '+foo' }, { dest => '+dc/foo' },
+        ],
+        ipset => { foo => [] },
+    };
+
+    my $modified = rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 1);
+
+    ok($modified, 'reports modified');
+    is($conf->{rules}->[0]->{source}, '+foo', 'shadowed bare ref not touched');
+    is($conf->{rules}->[1]->{dest}, '+dc/baz', 'explicit cluster ref rewritten');
+};
+
+subtest 'ipset: guest without own object' => sub {
+    my $conf = {
+        rules => [{ source => '+foo' }, { dest => '+dc/foo' }],
+        ipset => {},
+    };
+
+    rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 1);
+
+    is($conf->{rules}->[0]->{source}, '+baz', 'implicit cluster ref rewritten');
+    is($conf->{rules}->[1]->{dest}, '+dc/baz', 'explicit cluster ref rewritten');
+};
+
+subtest 'ipset: guest-level rename' => sub {
+    my $conf = {
+        rules => [
+            { source => '+foo' }, { dest => '+guest/foo' }, { source => '+dc/foo' },
+        ],
+    };
+
+    rewrite($conf, $ipset_spec, 'foo', 'baz', 'vm', 0);
+
+    is($conf->{rules}->[0]->{source}, '+baz', 'implicit guest ref rewritten');
+    is($conf->{rules}->[1]->{dest}, '+guest/baz', 'guest/ ref rewritten');
+    is($conf->{rules}->[2]->{source}, '+dc/foo', 'dc/ ref not touched in guest env');
+};
+
+subtest 'ipset: not confused with alias' => sub {
+    my $conf = { rules => [{ source => 'foo' }, { dest => '+foo' }] };
+
+    my $modified = rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 0);
+
+    ok($modified, 'reports modified');
+    is($conf->{rules}->[0]->{source}, 'foo', 'bare alias ref not touched');
+    is($conf->{rules}->[1]->{dest}, '+baz', 'ipset ref rewritten');
+};
+
+subtest 'alias: cluster rename rewrites rules and ipset members' => sub {
+    my $conf = {
+        rules => [{ source => 'al' }, { dest => 'dc/al' }, { source => 'other' }],
+        groups => { grp => [{ source => 'dc/al' }] },
+        ipset => {
+            set => [{ cidr => 'al' }, { cidr => 'dc/al' }, { cidr => '10.0.0.1' }],
+        },
+    };
+
+    my $modified = rewrite($conf, $alias_spec, 'al', 'new', 'cluster', 0);
+
+    ok($modified, 'reports modified');
+    is($conf->{rules}->[0]->{source}, 'new', 'bare alias in rule rewritten');
+    is($conf->{rules}->[1]->{dest}, 'dc/new', 'dc/ alias in rule rewritten');
+    is($conf->{rules}->[2]->{source}, 'other', 'unrelated rule kept');
+    is($conf->{groups}->{grp}->[0]->{source}, 'dc/new', 'alias in group rewritten');
+    is($conf->{ipset}->{set}->[0]->{cidr}, 'new', 'bare alias in ipset member rewritten');
+    is($conf->{ipset}->{set}->[1]->{cidr}, 'dc/new', 'dc/ alias in ipset member rewritten');
+    is($conf->{ipset}->{set}->[2]->{cidr}, '10.0.0.1', 'literal cidr member kept');
+};
+
+subtest 'alias: cluster delete drops rules and ipset members' => sub {
+    my $conf = {
+        rules => [{ source => 'al' }, { source => 'keep' }],
+        ipset => { set => [{ cidr => 'dc/al' }, { cidr => '10.0.0.1' }] },
+    };
+
+    rewrite($conf, $alias_spec, 'al', undef, 'cluster', 0);
+
+    is(scalar($conf->{rules}->@*), 1, 'matching rule dropped');
+    is($conf->{rules}->[0]->{source}, 'keep', 'unrelated rule kept');
+    is(scalar($conf->{ipset}->{set}->@*), 1, 'matching member dropped');
+    is($conf->{ipset}->{set}->[0]->{cidr}, '10.0.0.1', 'literal member kept');
+};
+
+subtest 'alias: not confused with ipset' => sub {
+    my $conf = { rules => [{ source => '+al' }, { dest => 'al' }] };
+
+    rewrite($conf, $alias_spec, 'al', 'new', 'cluster', 0);
+
+    is($conf->{rules}->[0]->{source}, '+al', 'ipset ref (+) not touched');
+    is($conf->{rules}->[1]->{dest}, 'new', 'alias ref rewritten');
+};
+
+subtest 'case-insensitive match, written back lowercase' => sub {
+    my $conf = { rules => [{ source => '+DC/FOO' }, { dest => '+Foo' }] };
+
+    rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 0);
+
+    is($conf->{rules}->[0]->{source}, '+dc/baz', 'dc/ ref matched and lowercased');
+    is($conf->{rules}->[1]->{dest}, '+baz', 'bare ref matched and lowercased');
+};
+
+subtest 'no match reports not modified' => sub {
+    my $conf = { rules => [{ source => '+other' }, { dest => 'somealias' }] };
+
+    my $modified = rewrite($conf, $ipset_spec, 'foo', 'baz', 'cluster', 0);
+
+    ok(!$modified, 'nothing matched -> not modified');
+    is($conf->{rules}->[0]->{source}, '+other', 'unrelated refs not touched');
+};
+
+done_testing();
-- 
2.47.3

[PATCH pve-manager v2 7/9] ui: firewall: add common widgets for deleting and updating references

[Arthur Bied-Charreton]

The ipset and alias grids need the same two UI pieces to let users
update/delete referencing rules when removing or renaming an object. Add
them as reusable components.

FirewallDeleteReferences extends ConfirmRemoveDialog with a checkbox
that sets 'delete-references', FirewallUpdateReferences is a
no/yes/force selector that submits 'update-references'.

Callers will be added in subsequent commits.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/Makefile                     |   1 +
 www/manager6/grid/FirewallObjectCommon.js | 100 ++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 www/manager6/grid/FirewallObjectCommon.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index eb0e9d9c..0a0a2792 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -98,6 +98,7 @@ JSSRC= 							\
 	form/TagFieldSet.js				\
 	form/IsoSelector.js				\
 	grid/BackupView.js				\
+	grid/FirewallObjectCommon.js		\
 	grid/FirewallAliases.js				\
 	grid/FirewallOptions.js				\
 	grid/FirewallRules.js				\
diff --git a/www/manager6/grid/FirewallObjectCommon.js b/www/manager6/grid/FirewallObjectCommon.js
new file mode 100644
index 00000000..67e16634
--- /dev/null
+++ b/www/manager6/grid/FirewallObjectCommon.js
@@ -0,0 +1,100 @@
+Ext.define('PVE.window.FirewallDeleteReferences', {
+    extend: 'Proxmox.window.ConfirmRemoveDialog',
+    alias: 'widget.pveFirewallDeleteReferences',
+
+    width: 350,
+
+    refsFieldLabel: gettext('Delete references'),
+
+    refsTip: undefined,
+
+    initComponent: function () {
+        let me = this;
+
+        let refsTip = me.refsTip;
+        if (refsTip === undefined) {
+            throw new Error('refsTip is undefined, cannot initialize component');
+        }
+
+        me.additionalItems = [
+            {
+                xtype: 'proxmoxcheckbox',
+                reference: 'deleteRefsCheckbox',
+                fieldLabel: me.refsFieldLabel,
+                labelWidth: 260,
+                autoEl: { 'data-qtip': refsTip },
+            },
+        ];
+
+        me.callParent();
+    },
+
+    getParams: function () {
+        let me = this;
+        if (me.lookupReference('deleteRefsCheckbox').getValue()) {
+            me.params['delete-references'] = 1;
+        }
+
+        return me.callParent();
+    },
+});
+
+Ext.define('PVE.form.FirewallUpdateReferences', {
+    extend: 'Ext.form.FieldContainer',
+    alias: 'widget.pveFirewallUpdateReferences',
+
+    fieldLabel: gettext('Update references'),
+    labelWidth: 150,
+    layout: { type: 'hbox', pack: 'end' },
+
+    // submitted parameter
+    name: 'update-references',
+    value: 'no',
+
+    noTip: undefined,
+    yesTip: undefined,
+    forceTip: undefined,
+
+    initComponent: function () {
+        let me = this;
+
+        let noTip = me.noTip;
+        if (noTip === undefined) {
+            throw new Error('noTip is undefined, cannot initialize component');
+        }
+
+        let yesTip = me.yesTip;
+        if (yesTip === undefined) {
+            throw new Error('yesTip is undefined, cannot initialize component');
+        }
+
+        let forceTip = me.forceTip;
+        if (forceTip === undefined) {
+            throw new Error('forceTip is undefined, cannot initialize component');
+        }
+
+        me.items = [
+            {
+                xtype: 'segmentedbutton',
+                allowMultiple: false,
+                value: me.value,
+                items: [
+                    { text: gettext('No'), value: 'no', tooltip: me.noTip },
+                    { text: gettext('Yes'), value: 'yes', tooltip: me.yesTip },
+                    { text: gettext('Force'), value: 'force', tooltip: me.forceTip },
+                ],
+                listeners: {
+                    change: (btn, val) => me.down('hiddenfield').setValue(val),
+                },
+            },
+            {
+                xtype: 'hiddenfield',
+                name: me.name,
+                value: me.value,
+                isDirty: () => false,
+            },
+        ];
+
+        me.callParent();
+    },
+});
-- 
2.47.3

[PATCH pve-manager v2 8/9] ui: firewall: ipset: add controls to update/delete references on edit

[Arthur Bied-Charreton]

Add the shared 'update-references' selector to the update dialog, so a
rename can (force-)update all references to the ipset, and replace the
plain remove button with a confirmation dialog offering to delete
referencing rules along with the ipset.

StdRemoveButton cannot pass the extra 'delete-references' parameter, so
the remove button is switched to a plain button that opens the shared
FirewallDeleteReferences dialog.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/panel/IPSet.js | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/www/manager6/panel/IPSet.js b/www/manager6/panel/IPSet.js
index 9e0203a0..5c7340a7 100644
--- a/www/manager6/panel/IPSet.js
+++ b/www/manager6/panel/IPSet.js
@@ -91,6 +91,16 @@ Ext.define('PVE.IPSetList', {
                         value: rec.data.comment,
                         fieldLabel: gettext('Comment'),
                     },
+                    {
+                        xtype: 'pveFirewallUpdateReferences',
+                        noTip: gettext(
+                            'Do not update referencing rules when renaming this IPSet. Note that this will render any reference to this IPSet invalid.',
+                        ),
+                        yesTip: gettext('Update referencing rules to point to the new IPSet name.'),
+                        forceTip: gettext(
+                            'Update referencing rules to point to the new IPSet name, overwriting any pre-existing IPSet with the target name. Can be used in case a failed rename left the cluster configuration in a partially-updated state.',
+                        ),
+                    },
                 ],
             });
             win.show();
@@ -134,11 +144,26 @@ Ext.define('PVE.IPSetList', {
             },
         });
 
-        me.removeBtn = Ext.create('Proxmox.button.StdRemoveButton', {
+        me.removeBtn = Ext.create('Proxmox.button.Button', {
+            text: gettext('Remove'),
+            disabled: true,
+            dangerous: true,
             enableFn: (rec) => canEdit,
             selModel: sm,
-            baseurl: me.base_url + '/',
-            callback: reload,
+            handler: function (btn, event, rec) {
+                Ext.create('PVE.window.FirewallDeleteReferences', {
+                    item: { id: rec.data.name },
+                    url: me.base_url + '/' + rec.data.name,
+                    text: Ext.String.format(
+                        gettext("Are you sure you want to remove IPSet '{0}'?"),
+                        rec.data.name,
+                    ),
+                    refsTip: gettext(
+                        'Delete all firewall rules referencing this IPSet. If left unchecked, some firewall configurations may end up with dangling references.',
+                    ),
+                    apiCallDone: reload,
+                }).show();
+            },
         });
 
         Ext.apply(me, {
-- 
2.47.3

[PATCH pve-manager v2 9/9] ui: firewall: aliases: add controls to update/delete references on edit

[Arthur Bied-Charreton]

Add the shared 'update-references' selector to the update dialog, so a
rename can (force-)update all references to the alias, and replace the
plain remove button with a confirmation dialog to delete referencing
rules along with the alias.

StdRemoveButton cannot pass the extra 'delete-references' parameter, so
the remove button is switched to a plain button that opens the shared
FirewallDeleteReferences dialog.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 www/manager6/grid/FirewallAliases.js | 74 +++++++++++++++++++---------
 1 file changed, 52 insertions(+), 22 deletions(-)

diff --git a/www/manager6/grid/FirewallAliases.js b/www/manager6/grid/FirewallAliases.js
index 06801d33..f3a20fc5 100644
--- a/www/manager6/grid/FirewallAliases.js
+++ b/www/manager6/grid/FirewallAliases.js
@@ -20,27 +20,43 @@ Ext.define('PVE.FirewallAliasEdit', {
             me.method = 'PUT';
         }
 
+        let items = [
+            {
+                xtype: 'textfield',
+                name: me.isCreate ? 'name' : 'rename',
+                fieldLabel: gettext('Name'),
+                allowBlank: false,
+            },
+            {
+                xtype: 'textfield',
+                name: 'cidr',
+                fieldLabel: gettext('IP/CIDR'),
+                allowBlank: false,
+            },
+            {
+                xtype: 'textfield',
+                name: 'comment',
+                fieldLabel: gettext('Comment'),
+            },
+        ];
+        if (!me.isCreate) {
+            items.push({
+                xtype: 'pveFirewallUpdateReferences',
+                noTip: gettext(
+                    'Do not update referencing rules and IPSets when renaming this alias. Note that this will render any reference to this alias invalid.',
+                ),
+                yesTip: gettext(
+                    'Update referencing rules and IPSets to point to the new alias name.',
+                ),
+                forceTip: gettext(
+                    'Update referencing rules and IPSets to point to the new alias name, overwriting any pre-existing alias with the target name. Can be used in case a failed rename left the cluster configuration in a partially-updated state.',
+                ),
+            });
+        }
+
         let ipanel = Ext.create('Proxmox.panel.InputPanel', {
             isCreate: me.isCreate,
-            items: [
-                {
-                    xtype: 'textfield',
-                    name: me.isCreate ? 'name' : 'rename',
-                    fieldLabel: gettext('Name'),
-                    allowBlank: false,
-                },
-                {
-                    xtype: 'textfield',
-                    name: 'cidr',
-                    fieldLabel: gettext('IP/CIDR'),
-                    allowBlank: false,
-                },
-                {
-                    xtype: 'textfield',
-                    name: 'comment',
-                    fieldLabel: gettext('Comment'),
-                },
-            ],
+            items,
         });
 
         Ext.apply(me, {
@@ -158,15 +174,29 @@ Ext.define('PVE.FirewallAliases', {
             },
         });
 
-        me.removeBtn = Ext.create('Proxmox.button.StdRemoveButton', {
+        me.removeBtn = Ext.create('Proxmox.button.Button', {
+            text: gettext('Remove'),
             disabled: true,
+            dangerous: true,
             selModel: sm,
             enableFn: (rec) =>
                 !!caps.vms['VM.Config.Network'] ||
                 !!caps.dc['Sys.Modify'] ||
                 !!caps.nodes['Sys.Modify'],
-            baseurl: me.base_url + '/',
-            callback: reload,
+            handler: function (btn, event, rec) {
+                Ext.create('PVE.window.FirewallDeleteReferences', {
+                    item: { id: rec.data.name },
+                    url: me.base_url + '/' + rec.data.name,
+                    text: Ext.String.format(
+                        gettext("Are you sure you want to remove alias '{0}'"),
+                        rec.data.name,
+                    ),
+                    refsTip: gettext(
+                        'Delete all firewall rules and IPSets referencing this alias. If left unchecked, some firewall configurations may end with dangling references.',
+                    ),
+                    apiCallDone: reload,
+                }).show();
+            },
         });
 
         Ext.apply(me, {
-- 
2.47.3