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

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

On Tue, Aug 18, 2026 at 03:34:04PM +0200, Arthur Bied-Charreton wrote:
> 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.
> 
[...]
> 
Talked to Stefan off-list, we came to the conclusion that it would make
sense to also allow just disabling rules referencing deleted objects
(as opposed to just removing them completely, which is what the current
version does).

I am also looking into handling references left dangling by the deletion
of SDN objects, i.e. VNets and guests with IPAM entries, will also 
include that in v3. 

This raises a question about the ACL policy this series should follow.
Now the reference rewriting performs no permission check at all and
runs as root. Editing the cluster firewall config requires Sys.Modify 
on /, but the endpoint will also edit guest firewall configs, which 
normally requires VM.Config.Network on /vms/<vmid>, and VNet firewall 
configs, which requires SDN.Allocate on the zone. Conversely, handling
references to a guest's IPAM IPSet on destruction means potentially 
editing the cluster firewall config from an endpoint that only requires 
VM.Allocate on the guest. 

There is precedent for this. destroy_vm requires VM.Allocate on
/vms/<vmid>, and with purge set it removes the VMID from the
cluster-wide HA config, from the replication config, and from backup
jobs, without checking any further privileges. The current position in
PVE seems to be that the privilege on the object is enough and that
cleaning up references to it somewhere else is part of the operation.

Dropping referencing rules is the one case that destroys configuration
the caller has no rights to. A rule also had an action, another 
endpoint, ports and comments, whcih would all be dropped as well as a 
result.

My proposition is to follow the precedent and require no additional
privileges for any of the three, on the grounds that the caller has
power over these things already. The one place where I would understand
an objection is dropping, for the reason above. The purge precedent
however also covers dropping, so singling it out would be a deviation.

If we do want a gate on dropping, I would suggest making it a single
check for Sys.Modify on / at the top of the endpoint rather than a
per-config check. Requiring the exact union of privileges means either
iterating over every affected config twice or implementing rollback,
and a best-effort approach that only edits what the caller may edit
produces split states. 

In all cases the new parameters default to keeping references as they
are.

Thoughts?
> 
>