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

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

On Fri, Sep 11, 2026 at 10:44:13AM +0200, Arthur Bied-Charreton wrote:
> 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?
> > 
> > 
For the record, Stefan and I talked about this again and we decided to
keep the current behavior, i.e. no additional privileges are required
for rewriting, disabling or dropping references to firewall objects.
Requiring the more fine-grained privileges leads to awkward cases. 

For example, IPSets are auto-generated for guests with IPAM entries, and
those can be referenced from any firewall config. Deleting a guest whose
IPSet is referenced in the cluster config would then require Sys.Modify
on / as well, which we probably don't want to give to every VM admin.
Deleting a cluster IPSet would require VM.Config.Network on every guest 
that references it. 

A user allowed to delete a guest or VNet can already invalidate every
rule referencing its IPSets, so requiring nothing beyond the privilege
on the object itself seems appropriate. Dropping goes further, since it
removes the rest of the rule as well, but it matches what destroy_vm
already does with purge for HA, replication and backup jobs. 

v3 will follow this approach.