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 5F9661FF0E6 for ; Fri, 24 Jul 2026 10:54:23 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 02EC5214A6; Fri, 24 Jul 2026 10:54:23 +0200 (CEST) Date: Fri, 24 Jul 2026 10:53:49 +0200 From: Arthur Bied-Charreton To: Stefan Hanreich Subject: Re: [PATCH firewall/manager 0/5] Allow updating references to firewall objects when editing them Message-ID: References: <20260407073658.90818-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784883199767 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.837 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) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low 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: PR7GC2UKM42Q7ESA6XOC5J3WGXROAUBL X-Message-ID-Hash: PR7GC2UKM42Q7ESA6XOC5J3WGXROAUBL 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 CC: pve-devel@lists.proxmox.com 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 Thu, Jul 16, 2026 at 03:17:40PM +0200, Stefan Hanreich wrote: > Thanks for tackling this! I've added some comments on the individual > patches and did some testing > thanks for the review! > An idea for an additional feature I noticed while testing: We > auto-generate IPSets for SDN objects. It might make sense to add similar > functionality when deleting SDN objects, so there are no dangling IPSet > references after deleting an SDN object. This could be done in a v2, or > even in a completely separate patch series. good point, will look into that. > > > On 4/7/26 9:36 AM, Arthur Bied-Charreton wrote: > > Renaming/deleting ipsets or aliases that are referenced by other rules, > > security groups, or ipsets currently leaves dangling references behind. > > This creates broken firewall configurations where the rules with those > > dangling references fail to parse, meaning they are ignored by the > > firewall. > > > > This is especially dangerous in the renaming case, where a user might > > reasonably expect references to update automatically, meaning they may > > go about their day after renaming an object without realizing a whole > > set of firewall rules is now inactive. > > > > This patch series addresses this by adding optional parameters to the > > related firewall endpoints that allow updating/deleting all references > > to an object when editing/removing it. The UI patches add checkboxes to > > the edit/remove windows to optionally pass this parameter. > > > > The casing in the firewall API is inconsistent: ipset names are stored > > as-is in the SectionConfig (e.g. 'FOO'), but lowercased when the config > > is loaded. This means a rule referencing '+dc/FOO' will fail to match > > its own definition after a load. In practice this is self-correcting: > > lowercasing is applied in-place, so definitions are written back in > > lowercase on the next read-write cycle, and the API does not allow > > creating rules with references that do not match anything. There is > > however a window between object creation and that first cycle where > > references can be broken. To avoid false negatives in that window, > > this series uses case-insensitive matching when scanning for > > references, and renamed references are always written back lowercased. > > > > Stefan and I talked about this off-list and came to the conclusion that > > enforcing either casing may break existing configs that have been > > manually edited, so a more general fix would be better folded into a > > major release. > > > > The UI patches introduce some code duplication. I tried to generalize > > the remove button with an optional extra URL parameter, but I did not > > find a good name for it, nor did I find a nice common ground that did > > not involve making the call sites even more verbose with configuration > > options, which is why I ended up leaving it like this. I am of course > > open to suggestions. > > > > The original plan was to have an upfront check for would-be-dangling > > references before allowing any edit to an object, however this can be a > > very expensive operation, since an object defined at the cluster level > > may be referenced by guest and host configurations in the whole cluster. > > A check like this would trigger an unconditional scan of all those > > configs when clicking the remove buttons, which is why it is kept > > opt-in in the UI. > > > > > > pve-firewall: > > > > Arthur Bied-Charreton (3): > > Add helpers for updating alias and ipset references > > ipset: Add option to update references on rename/delete > > aliases: Add option to update references on rename/delete > > > > src/PVE/API2/Firewall/Aliases.pm | 115 +++++++++++++++++++++++++++++++ > > src/PVE/API2/Firewall/Helpers.pm | 66 ++++++++++++++++++ > > src/PVE/API2/Firewall/IPSet.pm | 108 ++++++++++++++++++++++++++++- > > 3 files changed, 288 insertions(+), 1 deletion(-) > > > > > > pve-manager: > > > > Arthur Bied-Charreton (2): > > ipset: Add option to update references on rename/delete > > aliases: Add option to update references on rename/delete > > > > www/manager6/grid/FirewallAliases.js | 101 +++++++++++++++++++++------ > > www/manager6/panel/IPSet.js | 60 +++++++++++++++- > > 2 files changed, 136 insertions(+), 25 deletions(-) > > > > > > Summary over all repositories: > > 5 files changed, 424 insertions(+), 26 deletions(-) > > > > > > >