[pve-devel] [PATCH firewall/manager/proxmox{-ve-rs, -firewall} 0/5] fix #7068: show rule comments in iptables and nftables

[pve-devel] [PATCH firewall/manager/proxmox{-ve-rs, -firewall} 0/5] fix #7068: show rule comments in iptables and nftables

[Robert Obkircher]

Pass firewall rule comments from the UI to the underlying firewall
configuration.

Combines and changes two previously separate patches:
New:
    - added a preserve_comments firewall option to api and UI
[1] fix #7068: show rule comments in iptables output
    - shortened PVECOMMENT: prefix to PVE:
    - fixed escape logic regex
    - added tests for print_ipt_command function
[2] fix #7068: show rule comments in nftables output
    - test truncation logic

Note that for testing you likely want the fix for '#' symbols inside
comments [3] and the fix for "make check" in pve-firewall [4].

At some point I ran into a strange issue, where the options grid
displayed "Yes" for boolean options which were clearly 0 in the network
response, while the edit dialog displayed the correct value. I'm not
sure what caused this but system updates and rebuilding pve-firewall
+pve-manager fixed it.

[1] https://lore.proxmox.com/pve-devel/20251201123424.94742-1-r.obkircher@proxmox.com/#r
[2] https://lore.proxmox.com/pve-devel/75f887c2-fce6-444e-829c-977784dca365@proxmox.com/#r
[3] https://lore.proxmox.com/pve-devel/20251215123925.168475-1-r.obkircher@proxmox.com/T/#u
[4] https://lore.proxmox.com/pve-devel/20251211141201.143435-1-r.obkircher@proxmox.com/

*** MURPP HERE ***

pve-firewall:

Robert Obkircher (2):
  api: firewall: add option to preserve comments
  fix #7068: show rule comments in iptables output

 src/PVE/Firewall.pm   | 36 +++++++++++++++++-
 test/Makefile         |  1 +
 test/test_comments.pl | 86 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 2 deletions(-)
 create mode 100755 test/test_comments.pl


pve-manager:

Robert Obkircher (1):
  ui: firewall: add preserve comments option

 www/manager6/grid/FirewallOptions.js | 1 +
 1 file changed, 1 insertion(+)


proxmox-ve-rs:

Robert Obkircher (1):
  firewall: parse preserve_comments host firewall option

 proxmox-ve-config/src/firewall/host.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)


proxmox-firewall:

Robert Obkircher (2):
  fix #7068: show rule comments in nftables output
  firewall: add rule comments to snapshot tests

 proxmox-firewall/src/rule.rs                  | 56 ++++++++++++++++++-
 proxmox-firewall/tests/input/host.fw          |  4 +-
 .../integration_tests__firewall.snap          | 44 ++++++++++++++-
 3 files changed, 100 insertions(+), 4 deletions(-)


Summary over all repositories:
  8 files changed, 236 insertions(+), 6 deletions(-)

-- 
Generated by git-murpp 0.8.1


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-firewall 1/2] api: firewall: add option to preserve comments

[Robert Obkircher]

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/PVE/Firewall.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 93f8c34..06384b4 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1451,6 +1451,13 @@ our $host_option_properties = {
         default => 0,
         optional => 1,
     },
+    preserve_comments => {
+        description => "Pass comments from the UI to the underlying firewall configuration. "
+            . "May involve truncation.",
+        type => 'boolean',
+        default => 0,
+        optional => 1,
+    },
 };
 
 our $vm_option_properties = {
@@ -3288,7 +3295,7 @@ sub parse_hostfw_option {
     my $loglevels = "emerg|alert|crit|err|warning|notice|info|debug|nolog";
 
     if ($line =~
-        m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack|nf_conntrack_allow_invalid|protection_synflood|nftables):\s*(0|1)\s*$/i
+        m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack|nf_conntrack_allow_invalid|protection_synflood|nftables|preserve_comments):\s*(0|1)\s*$/i
     ) {
         $opt = lc($1);
         $value = int($2);
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-firewall 2/2] fix #7068: show rule comments in iptables output

[Robert Obkircher]

Use the iptables comment extension to include comments from the UI.
Prefix them with "PVE:" to avoid interfering with "PVESIG:$sig"
comments, which are used to store signatures for change detection.

The total length of the (unescaped) comments is limited to 255 utf8
bytes.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/PVE/Firewall.pm   | 27 +++++++++++++-
 test/Makefile         |  1 +
 test/test_comments.pl | 86 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100755 test/test_comments.pl

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 06384b4..2533e9c 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -2278,6 +2278,29 @@ sub ipt_gen_src_or_dst_match {
     return $match;
 }
 
+sub print_ipt_comment {
+    my ($comment) = @_;
+    return "" if !defined($comment) || $comment eq "";
+    $comment = "PVE:$comment"; # Disambiguate from PVESIG: comments
+
+    # Mimic iptables-save and limit the length to 255 bytes. Since
+    # iptables-restore seems to accept up to 1023 (unescaped) bytes
+    # it wouldn't be a huge problem if this was accidentally
+    # re-encoded to a longer length later.
+    $comment = encode("UTF-8", $comment, Encode::FB_WARN | Encode::LEAVE_SRC);
+    $comment = substr($comment, 0, 255);
+
+    # Clean up invalid bytes at the end.
+    $comment = decode("UTF-8", $comment, Encode::FB_QUIET | Encode::LEAVE_SRC);
+
+    # iptables_chain_digest can't process wide characters.
+    $comment = encode("UTF-8", $comment);
+
+    # Escape like xtables_save_string. Always quote because of colon.
+    $comment =~ s/([\\"'])/\\$1/g;
+    return " -m comment --comment \"$comment\"";
+}
+
 # convert a %rule to an array of iptables commands
 sub ipt_rule_to_cmds {
     my ($rule, $chain, $ipversion, $cluster_conf, $fw_conf, $vmid) = @_;
@@ -2382,7 +2405,9 @@ sub ipt_rule_to_cmds {
         my $logaction = get_log_rule_base($chain, $vmid, $rule->{logmsg}, $loglevel);
         push @iptcmds, "-A $chain $matchstr $logaction";
     }
-    push @iptcmds, "-A $chain $matchstr $targetstr";
+    my $comment =
+        $fw_conf->{options}->{preserve_comments} ? print_ipt_comment($rule->{comment}) : "";
+    push @iptcmds, "-A $chain $matchstr $targetstr$comment";
     return @iptcmds;
 }
 
diff --git a/test/Makefile b/test/Makefile
index fea9c21..3880b57 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -4,6 +4,7 @@ all:
 .PHONY: check
 check:
 	./fwtester.pl
+	./test_comments.pl
 
 .PHONY: install
 install: check
diff --git a/test/test_comments.pl b/test/test_comments.pl
new file mode 100755
index 0000000..3f1d065
--- /dev/null
+++ b/test/test_comments.pl
@@ -0,0 +1,86 @@
+#!/usr/bin/env perl
+
+use lib '../src';
+
+use strict;
+use warnings;
+
+use utf8;
+
+use Encode qw(encode);
+use Test::More;
+
+use PVE::Firewall;
+
+die if length('🦀') != 1;
+die if length(encode('UTF-8', '🦀')) != 4;
+
+my $tests = [
+    {
+        desc => 'empty for empty undef',
+        param => undef,
+        expected => '',
+    },
+    {
+        desc => 'empty for empty string',
+        param => '',
+        expected => '',
+    },
+    {
+        desc => 'escape single/double quote and backslash',
+        param => q{x"x\\x'x escape ""''\\\\"'\\},
+        expected =>
+            q{ -m comment --comment "PVE:x\\"x\\\\x\\'x escape \\"\\"\\'\\'\\\\\\\\\\"\\'\\\\"},
+    },
+    {
+        desc => 'other special characters',
+        param => q{@$#'\\"🦀\\t=(	)},
+        expected => q{ -m comment --comment "PVE:@$#\\'\\\\\\"🦀\\\\t=(	)"},
+    },
+    {
+        desc => 'prevent conflict with signature prefix',
+        param => 'PVESIG:abc',
+        expected => ' -m comment --comment "PVE:PVESIG:abc"',
+    },
+    {
+        desc => 'truncate ascii',
+        param => 'a' x 300,
+        expected => ' -m comment --comment "PVE:' . ('a' x 251) . '"',
+    },
+    {
+        desc => 'truncate 0/4 emoji bytes',
+        param => ('a' x 247) . '🦀',
+        expected => ' -m comment --comment "PVE:' . ('a' x 247) . '🦀"',
+    },
+    {
+        desc => 'truncate 1/4 emoji bytes',
+        param => ('a' x 248) . '🦀',
+        expected => ' -m comment --comment "PVE:' . ('a' x 248) . '"',
+    },
+    {
+        desc => 'truncate 2/4 emoji bytes',
+        param => ('a' x 249) . '🦀',
+        expected => ' -m comment --comment "PVE:' . ('a' x 249) . '"',
+    },
+    {
+        desc => 'truncate 3/4 emoji bytes',
+        param => ('a' x 250) . '🦀',
+        expected => ' -m comment --comment "PVE:' . ('a' x 250) . '"',
+    },
+    {
+        desc => 'truncate 4/4 emoji bytes',
+        param => ('a' x 251) . '🦀',
+        expected => ' -m comment --comment "PVE:' . ('a' x 251) . '"',
+    },
+];
+
+plan(tests => scalar($tests->@*));
+
+for my $case ($tests->@*) {
+    my $result = PVE::Firewall::print_ipt_comment($case->{param});
+
+    my $expected = encode('UTF-8', $case->{expected});
+    is($result, $expected, $case->{desc});
+}
+
+done_testing();
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-manager 1/1] ui: firewall: add preserve comments option

[Robert Obkircher]

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 www/manager6/grid/FirewallOptions.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/manager6/grid/FirewallOptions.js b/www/manager6/grid/FirewallOptions.js
index 6d68a939..7ee6196f 100644
--- a/www/manager6/grid/FirewallOptions.js
+++ b/www/manager6/grid/FirewallOptions.js
@@ -85,6 +85,7 @@ Ext.define('PVE.FirewallOptions', {
             add_log_row('log_level_forward');
             add_log_row('tcp_flags_log_level', 120);
             add_log_row('smurf_log_level');
+            add_boolean_row('preserve_comments', gettext('Preserve Comments'), 0);
             add_boolean_row('nftables', gettext('nftables (tech preview)'), 0);
         } else if (me.fwtype === 'vm') {
             me.rows.enable = {
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-ve-rs 1/1] firewall: parse preserve_comments host firewall option

[Robert Obkircher]

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-ve-config/src/firewall/host.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/proxmox-ve-config/src/firewall/host.rs b/proxmox-ve-config/src/firewall/host.rs
index d749442..01ffca2 100644
--- a/proxmox-ve-config/src/firewall/host.rs
+++ b/proxmox-ve-config/src/firewall/host.rs
@@ -34,6 +34,8 @@ pub const HOST_BLOCK_INVALID_TCP_DEFAULT: bool = false;
 pub const HOST_BLOCK_INVALID_CONNTRACK: bool = false;
 /// default setting for logging of invalid conntrack entries
 pub const HOST_LOG_INVALID_CONNTRACK: bool = false;
+/// default setting for preserve_comments
+pub const HOST_PRESERVE_COMMENTS_DEFAULT: bool = false;
 
 #[derive(Debug, Default, Deserialize)]
 #[cfg_attr(test, derive(Eq, PartialEq))]
@@ -82,6 +84,9 @@ pub struct Options {
 
     #[serde(default, deserialize_with = "proxmox_serde::perl::deserialize_bool")]
     tcpflags: Option<bool>,
+
+    #[serde(default, deserialize_with = "proxmox_serde::perl::deserialize_bool")]
+    preserve_comments: Option<bool>,
 }
 
 #[derive(Debug, Default)]
@@ -274,6 +279,13 @@ impl Config {
             Direction::Forward => self.config.options.log_level_forward.unwrap_or_default(),
         }
     }
+
+    pub fn preserve_comments(&self) -> bool {
+        self.config
+            .options
+            .preserve_comments
+            .unwrap_or(HOST_PRESERVE_COMMENTS_DEFAULT)
+    }
 }
 
 #[cfg(test)]
@@ -309,6 +321,7 @@ protection_synflood_rate: 300
 smurf_log_level: notice
 tcp_flags_log_level: nolog
 tcpflags: yes
+preserve_comments: 1
 
 [RULES]
 
@@ -342,6 +355,7 @@ IN ACCEPT -p udp -dport 33 -sport 22 -log warning
                 smurf_log_level: Some(LogLevel::Notice),
                 tcp_flags_log_level: Some(LogLevel::Nolog),
                 tcpflags: Some(true),
+                preserve_comments: Some(true),
             }
         );
 
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-firewall 1/2] fix #7068: show rule comments in nftables output

[Robert Obkircher]

Include rule comments from the UI in the generated nftables rules if
the preserve_comments option is enabled. Truncate them to at most 128
bytes to match the limit in libnftnl.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-firewall/src/rule.rs | 56 +++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
index b79f91c..6be6720 100644
--- a/proxmox-firewall/src/rule.rs
+++ b/proxmox-firewall/src/rule.rs
@@ -36,14 +36,19 @@ pub(crate) struct NftRule {
     family: Option<Family>,
     statements: Vec<Statement>,
     terminal_statements: Vec<Statement>,
+    comment: Option<String>,
 }
 
 impl NftRule {
+    /// from NFTNL_UDATA_COMMENT_MAXLEN
+    pub const MAX_COMMENT_LEN: usize = 128;
+
     pub fn from_terminal_statements(terminal_statements: Vec<Statement>) -> Self {
         Self {
             family: None,
             statements: Vec::new(),
             terminal_statements,
+            comment: None,
         }
     }
 
@@ -52,6 +57,7 @@ impl NftRule {
             family: None,
             statements: Vec::new(),
             terminal_statements: vec![terminal_statement],
+            comment: None,
         }
     }
 
@@ -81,6 +87,41 @@ impl NftRule {
         ipfilter.to_nft_rules(&mut rules, env)?;
         Ok(rules)
     }
+
+    pub fn set_comment(&mut self, comment: &str) {
+        self.comment = Some(Self::truncate_comment(comment).to_string())
+    }
+
+    fn truncate_comment(comment: &str) -> &str {
+        &comment[..my_floor_char_boundary(comment, Self::MAX_COMMENT_LEN)]
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::NftRule;
+
+    #[test]
+    fn test_truncate_129() {
+        let comment = "Mid character trucation of 129 byte comment: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa🦀🦀";
+        assert_eq!(comment.len(), 129);
+        let truncated = NftRule::truncate_comment(comment);
+        assert_eq!(truncated.len(), 125);
+        assert!(comment.starts_with(truncated));
+    }
+}
+
+// TODO: replace with str::floor_char_boundary once rustc 1.91.0 is available
+fn my_floor_char_boundary(s: &str, index: usize) -> usize {
+    if index >= s.len() {
+        s.len()
+    } else {
+        s.char_indices()
+            .map(|(i, _)| i)
+            .take_while(|i| *i <= index)
+            .last()
+            .unwrap_or(0)
+    }
 }
 
 impl Deref for NftRule {
@@ -101,7 +142,12 @@ impl NftRule {
     pub fn into_add_rule(self, chain: ChainPart) -> AddRule {
         let statements = self.statements.into_iter().chain(self.terminal_statements);
 
-        AddRule::from_statements(chain, statements)
+        let result = AddRule::from_statements(chain, statements);
+        if let Some(comment) = self.comment {
+            result.with_comment(comment)
+        } else {
+            result
+        }
     }
 
     pub fn family(&self) -> Option<Family> {
@@ -175,11 +221,19 @@ impl ToNftRules for Rule {
     fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
         log::trace!("generating nft rules for config rule {self:?}");
 
+        let before = rules.len();
         match self.kind() {
             Kind::Match(rule) => rule.to_nft_rules(rules, env)?,
             Kind::Group(group) => group.to_nft_rules(rules, env)?,
         };
 
+        if env.firewall_config.host().preserve_comments() {
+            if let Some(comment) = self.comment() {
+                for nft_rule in &mut rules[before..] {
+                    nft_rule.set_comment(comment);
+                }
+            }
+        }
         Ok(())
     }
 }
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-firewall 2/2] firewall: add rule comments to snapshot tests

[Robert Obkircher]

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-firewall/tests/input/host.fw          |  4 +-
 .../integration_tests__firewall.snap          | 44 ++++++++++++++++++-
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/proxmox-firewall/tests/input/host.fw b/proxmox-firewall/tests/input/host.fw
index 7b89aad..56c8054 100644
--- a/proxmox-firewall/tests/input/host.fw
+++ b/proxmox-firewall/tests/input/host.fw
@@ -13,15 +13,17 @@ protection_synflood_burst: 1337
 protection_synflood_rate: 400
 nosmurfs: 1
 nf_conntrack_helpers: amanda,ftp,irc,netbios-ns,pptp,sane,sip,snmp,tftp
+preserve_comments: 1
 
 
 [RULES]
 
-IN DNS(ACCEPT) -source dc/network1 -log nolog
+IN DNS(ACCEPT) -source dc/network1 -log nolog # prevent DNS issues
 IN DHCPv6(ACCEPT) -log nolog
 IN DHCPfwd(ACCEPT) -log nolog
 IN ACCEPT --icmp-type neighbor-solicitation --proto ipv6-icmp --log info
 IN Ping(REJECT)
 IN REJECT -p udp --dport 443
 OUT REJECT -p udp --dport 443
+IN REJECT -p udp --dport 1000 # Mid character trucation of 129 byte comment: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa🦀🦀
 
diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
index 79cb882..0fb44f4 100644
--- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
+++ b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
@@ -1,8 +1,6 @@
 ---
 source: proxmox-firewall/tests/integration_tests.rs
-assertion_line: 127
 expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
-snapshot_kind: text
 ---
 {
   "nftables": [
@@ -3657,6 +3655,7 @@ snapshot_kind: text
           "family": "inet",
           "table": "proxmox-firewall",
           "chain": "host-in",
+          "comment": "prevent DNS issues",
           "expr": [
             {
               "match": {
@@ -3711,6 +3710,7 @@ snapshot_kind: text
           "family": "inet",
           "table": "proxmox-firewall",
           "chain": "host-in",
+          "comment": "prevent DNS issues",
           "expr": [
             {
               "match": {
@@ -4034,6 +4034,46 @@ snapshot_kind: text
         }
       }
     },
+    {
+      "add": {
+        "rule": {
+          "family": "inet",
+          "table": "proxmox-firewall",
+          "chain": "host-in",
+          "comment": "Mid character trucation of 129 byte comment: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa🦀",
+          "expr": [
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "meta": {
+                    "key": "l4proto"
+                  }
+                },
+                "right": "udp"
+              }
+            },
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "payload": {
+                    "protocol": "th",
+                    "field": "dport"
+                  }
+                },
+                "right": 1000
+              }
+            },
+            {
+              "jump": {
+                "target": "do-reject"
+              }
+            }
+          ]
+        }
+      }
+    },
     {
       "add": {
         "rule": {
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel