[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 at 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