From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 19E071FF0B3 for ; Fri, 11 Sep 2026 10:44:24 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id CCFEA2156F; Fri, 11 Sep 2026 10:44:20 +0200 (CEST) Date: Fri, 11 Sep 2026 10:44:13 +0200 From: Arthur Bied-Charreton To: pve-devel@lists.proxmox.com, s.hanreich@proxmox.com Subject: Re: [PATCH firewall/manager v2 0/9] allow updating references when renaming/deleting firewall objects Message-ID: References: <20260818133413.450776-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260818133413.450776-1-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1789116244762 X-SPAM-LEVEL: Spam detection results: 0 AWL 1.490 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 3ZPT5UTBXKRTB6FIVDRI6VWFFG6INHTI X-Message-ID-Hash: 3ZPT5UTBXKRTB6FIVDRI6VWFFG6INHTI X-MailFrom: a.bied-charreton@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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/, 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/, 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? > >