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 126761FF0E9 for ; Thu, 16 Jul 2026 15:17:52 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 98EC721445; Thu, 16 Jul 2026 15:17:50 +0200 (CEST) Message-ID: Date: Thu, 16 Jul 2026 15:17:40 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH firewall/manager 0/5] Allow updating references to firewall objects when editing them To: pve-devel@lists.proxmox.com References: <20260407073658.90818-1-a.bied-charreton@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20260407073658.90818-1-a.bied-charreton@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.250 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: YAWTVZV7KAXJ26M4UPHRTK67G4CX6FOR X-Message-ID-Hash: YAWTVZV7KAXJ26M4UPHRTK67G4CX6FOR X-MailFrom: s.hanreich@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: Thanks for tackling this! I've added some comments on the individual patches and did some testing 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. 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(-) >