* [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode @ 2022-07-20 13:06 Erik Skultety 2022-07-20 14:20 ` Florian Westphal ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Erik Skultety @ 2022-07-20 13:06 UTC (permalink / raw) To: netfilter-devel; +Cc: Erik Skultety The fact that the 'opt' table field reports spaces instead of '--' for IPv6 as it would have been the case with IPv4 has a bit of an unfortunate side effect that it completely confuses the 'jc' JSON formatter tool (which has an iptables formatter module). Consider: # ip6tables -L test Chain test (0 references) target prot opt source destination ACCEPT all a:b:c:: anywhere MAC01:02:03:04:05:06 Then: # ip6tables -L test | jc --iptables [{"chain":"test", "rules":[ {"target":"ACCEPT", "prot":"all", "opt":"a:b:c::", "source":"anywhere", "destination":"MAC01:02:03:04:05:06" }] }] which as you can see is wrong simply because whitespaces are considered as a column delimiter. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- iptables/xshared.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iptables/xshared.c b/iptables/xshared.c index bd4e1022..b1088c82 100644 --- a/iptables/xshared.c +++ b/iptables/xshared.c @@ -731,7 +731,7 @@ void print_fragment(unsigned int flags, unsigned int invflags, fputs("opt ", stdout); if (fake) { - fputs(" ", stdout); + fputs("--", stdout); } else { fputc(invflags & IPT_INV_FRAG ? '!' : '-', stdout); fputc(flags & IPT_F_FRAG ? 'f' : '-', stdout); -- 2.36.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-20 13:06 [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode Erik Skultety @ 2022-07-20 14:20 ` Florian Westphal 2022-07-20 16:11 ` Erik Skultety 2022-07-20 16:07 ` Jan Engelhardt 2022-07-25 21:39 ` Florian Westphal 2 siblings, 1 reply; 10+ messages in thread From: Florian Westphal @ 2022-07-20 14:20 UTC (permalink / raw) To: Erik Skultety; +Cc: netfilter-devel Erik Skultety <eskultet@redhat.com> wrote: > The fact that the 'opt' table field reports spaces instead of '--' for > IPv6 as it would have been the case with IPv4 has a bit of an > unfortunate side effect that it completely confuses the 'jc' JSON > formatter tool (which has an iptables formatter module). > Consider: > # ip6tables -L test > Chain test (0 references) > target prot opt source destination > ACCEPT all a:b:c:: anywhere MAC01:02:03:04:05:06 > > Then: > # ip6tables -L test | jc --iptables > [{"chain":"test", > "rules":[ > {"target":"ACCEPT", > "prot":"all", > "opt":"a:b:c::", > "source":"anywhere", > "destination":"MAC01:02:03:04:05:06" > }] > }] > > which as you can see is wrong simply because whitespaces are considered > as a column delimiter. Looks like ip6tables and iptables had this behaviour since day 1. original iptables: if (format & FMT_OPTIONS) { if (format & FMT_NOTABLE) fputs("opt ", stdout); fputc(fw->ip.invflags & IPT_INV_FRAG ? '!' : '-', stdout); fputc(flags & IPT_F_FRAG ? 'f' : '-', stdout); fputc(' ', stdout); } original ip6tables (5eed48af2516ebce0412121713d285bc30edb10d, June 2000): if (format & FMT_OPTIONS) { if (format & FMT_NOTABLE) fputs("opt ", stdout); fputc(' ', stdout); fputc(' ', stdout); fputc(' ', stdout); } While I like the idea of making those two identical I'm not sure its worh the risk, we've hit bugs for a myriad of other reasons when making seemingly innocent changes like this. What do others think? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-20 14:20 ` Florian Westphal @ 2022-07-20 16:11 ` Erik Skultety 2022-07-23 9:47 ` Phil Sutter 0 siblings, 1 reply; 10+ messages in thread From: Erik Skultety @ 2022-07-20 16:11 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Wed, Jul 20, 2022 at 04:20:02PM +0200, Florian Westphal wrote: > Erik Skultety <eskultet@redhat.com> wrote: > > The fact that the 'opt' table field reports spaces instead of '--' for > > IPv6 as it would have been the case with IPv4 has a bit of an > > unfortunate side effect that it completely confuses the 'jc' JSON > > formatter tool (which has an iptables formatter module). > > Consider: > > # ip6tables -L test > > Chain test (0 references) > > target prot opt source destination > > ACCEPT all a:b:c:: anywhere MAC01:02:03:04:05:06 > > > > Then: > > # ip6tables -L test | jc --iptables > > [{"chain":"test", > > "rules":[ > > {"target":"ACCEPT", > > "prot":"all", > > "opt":"a:b:c::", > > "source":"anywhere", > > "destination":"MAC01:02:03:04:05:06" > > }] > > }] > > > > which as you can see is wrong simply because whitespaces are considered > > as a column delimiter. > > Looks like ip6tables and iptables had this behaviour since day 1. > original iptables: > > if (format & FMT_OPTIONS) { > if (format & FMT_NOTABLE) > fputs("opt ", stdout); > fputc(fw->ip.invflags & IPT_INV_FRAG ? '!' : > '-', stdout); > fputc(flags & IPT_F_FRAG ? 'f' : '-', stdout); > fputc(' ', stdout); > } > > original ip6tables (5eed48af2516ebce0412121713d285bc30edb10d, June 2000): > if (format & FMT_OPTIONS) { > if (format & FMT_NOTABLE) > fputs("opt ", stdout); > fputc(' ', stdout); > fputc(' ', stdout); > fputc(' ', stdout); > } > > While I like the idea of making those two identical I'm not sure its > worh the risk, we've hit bugs for a myriad of other reasons when making > seemingly innocent changes like this. Hmm, the only reason why I submitted this change is because our libvirt test suite suddenly started failing on CentOS Stream 9 and only on CS9. Now, the test suite is flawed in its own way checking libvirt actions against iptables CLI output (yes, very fragile), but at the time the tests were written there essentially wasn't a programatic way of checking the changes like we could do today with the nftables library and its JSON formatter. So I investigated what's changed on CentOS Stream 9 compared to CS8 or Fedora 35/36 and it turned out that CS9 ships iptables-nft 1.8.8 while e.g. Fedora 36 ships 1.8.7 (so we're bound to failures there as well in the future). Let me describe the output difference in between the 2 versions of iptables: < v1.8.8 # ip6tables -L FI-tck-7081731 Chain FI-tck-7081731 (1 references) target prot opt source destination RETURN icmpv6 f:e:d::c:b:a/127 a:b:c::d:e:f MAC01:02:03:04:05:06 DSCP match 0x02 ipv6-icmptype 12 code 11 ctstate NEW,ESTABLISHED *** NOTE ^^HERE *** >= v1.8.8 ip6tables -L FI-tck-7081731 Chain FI-tck-7081731 (1 references) target prot opt source destination RETURN ipv6-icmp f:e:d::c:b:a/127 a:b:c::d:e:f MAC01:02:03:04:05:06 DSCP match 0x02 ipv6-icmptype 12 code 11 ctstate NEW,ESTABLISHED *** NOTE ^^HERE *** If my detective work was correct it was caused by commit b6196c7504d4d41827cea86c167926125cdbf1f3 which swapped the order of the protocol keys in the 'xtables_chain_protos'. If the protocol key order could be reverted back to its previous state then essentially it doesn't matter much to libvirt whether this patch lands in or not (well, it would have to be downstreamed to both CS9 and Fedora 36 anyway), I'm just looking for ways to fix our test suite even though I'd like to make use of a more programatic way of checking the chain rules, e.g. the JSON output as we've had many breakages in this area over the past couple of years. Note that the 'nft's JSON parser is currently not an answer for us in ^^this regard as for some reason it doesn't format the iptables' module extension data like the source MAC 01:02... you see above, but it does so when the rule was created with the nft frontend - I'm going to file a bug about this. Regards, Erik ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-20 16:11 ` Erik Skultety @ 2022-07-23 9:47 ` Phil Sutter 2022-07-23 12:35 ` Florian Westphal 0 siblings, 1 reply; 10+ messages in thread From: Phil Sutter @ 2022-07-23 9:47 UTC (permalink / raw) To: Florian Westphal, Jan Engelhardt, Erik Skultety; +Cc: netfilter-devel Hi, On Wed, Jul 20, 2022 at 06:11:19PM +0200, Erik Skultety wrote: [...] > Hmm, the only reason why I submitted this change is because our libvirt test > suite suddenly started failing on CentOS Stream 9 and only on CS9. Now, the > test suite is flawed in its own way checking libvirt actions against iptables > CLI output (yes, very fragile), but at the time the tests were written there > essentially wasn't a programatic way of checking the changes like we could do > today with the nftables library and its JSON formatter. > So I investigated what's changed on CentOS Stream 9 compared to CS8 or Fedora > 35/36 and it turned out that CS9 ships iptables-nft 1.8.8 while e.g. Fedora 36 > ships 1.8.7 (so we're bound to failures there as well in the future). > > Let me describe the output difference in between the 2 versions of iptables: > > < v1.8.8 > # ip6tables -L FI-tck-7081731 > Chain FI-tck-7081731 (1 references) > target prot opt source destination > RETURN icmpv6 f:e:d::c:b:a/127 a:b:c::d:e:f MAC01:02:03:04:05:06 DSCP match 0x02 ipv6-icmptype 12 code 11 ctstate NEW,ESTABLISHED > *** NOTE ^^HERE *** > > >= v1.8.8 > ip6tables -L FI-tck-7081731 > Chain FI-tck-7081731 (1 references) > target prot opt source destination > RETURN ipv6-icmp f:e:d::c:b:a/127 a:b:c::d:e:f MAC01:02:03:04:05:06 DSCP match 0x02 ipv6-icmptype 12 code 11 ctstate NEW,ESTABLISHED > *** NOTE ^^HERE *** > > If my detective work was correct it was caused by commit > b6196c7504d4d41827cea86c167926125cdbf1f3 which swapped the order of the > protocol keys in the 'xtables_chain_protos'. Yes, the goal was to avoid changes in output given typical /etc/protocol contents - it prefers "ipv6-icmp" over "icmpv6" for protocol 58 at least on my systems. I would suggest to not rely upon human-readable names for protocol numbers, but in fact there's no way out: iptables consolidates its internal protocol names list even if --numeric was given. Another bug I found while playing around is this: | # iptables -A FORWARD -p icmpv6 | # iptables -vnL FORWARD | Chain FORWARD (policy ACCEPT 0 packets, 0 bytes) | pkts bytes target prot opt in out source destination | 0 0 ipv6-icmp-- * * 0.0.0.0/0 0.0.0.0/0 print_rule_details() does not append a space after the protocol name if it is longer or equal to five characters. Both bugs seem to exist since day 1, I'm still tempted to fix them, i.e.: - Print protocol numbers with --numeric - Adjust the protocol format string from "%-5s" to "%-4s " for protocol names and from "%-5hu" to "%-4hu " for protocol numbers to force a single white space Objections anyone? Thanks, Phil ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-23 9:47 ` Phil Sutter @ 2022-07-23 12:35 ` Florian Westphal 0 siblings, 0 replies; 10+ messages in thread From: Florian Westphal @ 2022-07-23 12:35 UTC (permalink / raw) To: Phil Sutter, Florian Westphal, Jan Engelhardt, Erik Skultety, netfilter-devel Phil Sutter <phil@nwl.cc> wrote: > Another bug I found while playing around is this: > > | # iptables -A FORWARD -p icmpv6 > | # iptables -vnL FORWARD > | Chain FORWARD (policy ACCEPT 0 packets, 0 bytes) > | pkts bytes target prot opt in out source destination > | 0 0 ipv6-icmp-- * * 0.0.0.0/0 0.0.0.0/0 > > print_rule_details() does not append a space after the protocol name if it is > longer or equal to five characters. > > Both bugs seem to exist since day 1, I'm still tempted to fix them, i.e.: > > - Print protocol numbers with --numeric > - Adjust the protocol format string from "%-5s" to "%-4s " for protocol > names and from "%-5hu" to "%-4hu " for protocol numbers to force a > single white space > > Objections anyone? No, go ahead. Also, I think that the proposed "--" change is the least intrusive option so I'm inclined to apply the patch as-is. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-20 13:06 [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode Erik Skultety 2022-07-20 14:20 ` Florian Westphal @ 2022-07-20 16:07 ` Jan Engelhardt 2022-07-20 16:56 ` Erik Skultety 2022-07-25 21:39 ` Florian Westphal 2 siblings, 1 reply; 10+ messages in thread From: Jan Engelhardt @ 2022-07-20 16:07 UTC (permalink / raw) To: Erik Skultety; +Cc: netfilter-devel On Wednesday 2022-07-20 15:06, Erik Skultety wrote: >The fact that the 'opt' table field reports spaces instead of '--' for >IPv6 as it would have been the case with IPv4 has a bit of an >unfortunate side effect that it completely confuses the 'jc' JSON >formatter tool (which has an iptables formatter module). >Consider: > # ip6tables -L test > Chain test (0 references) > target prot opt source destination > ACCEPT all a:b:c:: anywhere MAC01:02:03:04:05:06 > >Then: > # ip6tables -L test | jc --iptables > [{"chain":"test", > "rules":[ > {"target":"ACCEPT", > "prot":"all", > "opt":"a:b:c::", > "source":"anywhere", > "destination":"MAC01:02:03:04:05:06" > }] > }] > >which as you can see is wrong simply because whitespaces are considered >as a column delimiter. Even if you beautify the opt column with a dash, you still have problems elsewhere. "MAC01" for example is not the destination at all. If you or jc is to parse anything, it must only be done with the iptables -S output form. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-20 16:07 ` Jan Engelhardt @ 2022-07-20 16:56 ` Erik Skultety 2022-07-21 7:22 ` Jan Engelhardt 0 siblings, 1 reply; 10+ messages in thread From: Erik Skultety @ 2022-07-20 16:56 UTC (permalink / raw) To: Jan Engelhardt; +Cc: netfilter-devel On Wed, Jul 20, 2022 at 06:07:34PM +0200, Jan Engelhardt wrote: > > On Wednesday 2022-07-20 15:06, Erik Skultety wrote: > > >The fact that the 'opt' table field reports spaces instead of '--' for > >IPv6 as it would have been the case with IPv4 has a bit of an > >unfortunate side effect that it completely confuses the 'jc' JSON > >formatter tool (which has an iptables formatter module). > >Consider: > > # ip6tables -L test > > Chain test (0 references) > > target prot opt source destination > > ACCEPT all a:b:c:: anywhere MAC01:02:03:04:05:06 > > > >Then: > > # ip6tables -L test | jc --iptables > > [{"chain":"test", > > "rules":[ > > {"target":"ACCEPT", > > "prot":"all", > > "opt":"a:b:c::", > > "source":"anywhere", > > "destination":"MAC01:02:03:04:05:06" > > }] > > }] > > > >which as you can see is wrong simply because whitespaces are considered > >as a column delimiter. > > Even if you beautify the opt column with a dash, you still have > problems elsewhere. "MAC01" for example is not the destination > at all. That's incorrect - this is what it would look like after this patch: [{"chain":"test", "rules":[ {"target":"ACCEPT", "prot":"all", "opt": , "source": "a:b:c::", "destination":"anywhere", "options":"MAC01:02:03:04:05:06" }] }] which actually makes more sense. I may have not been completely clear about it. With the column "beautifying" we could keep the current shape of tests, i.e. not trying to use 'jc' to get a JSON output and instead it would give me time to try address the nature of the checks in the test suite with nft's native JSON formatter instead which is IMO a more future-proof design of these old tests. > > If you or jc is to parse anything, it must only be done with the > iptables -S output form. > Well, that would be a problem because 'jc' iptables plugin doesn't understand the -S output (isn't -S considered deprecated or I'm just halucinating?). Erik ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-20 16:56 ` Erik Skultety @ 2022-07-21 7:22 ` Jan Engelhardt 0 siblings, 0 replies; 10+ messages in thread From: Jan Engelhardt @ 2022-07-21 7:22 UTC (permalink / raw) To: Erik Skultety; +Cc: netfilter-devel On Wednesday 2022-07-20 18:56, Erik Skultety wrote: >> >> If you or jc is to parse anything, it must only be done with the >> iptables -S output form. > >Well, that would be a problem because 'jc' iptables plugin doesn't understand >the -S output (isn't -S considered deprecated or I'm just halucinating?). iptables-save loops over all tables, and its output can be fed back to iptables-restore. That has existed for a long time. Then at some point, -S was added, which is a subset of save-style output for just one table or chain, but otherwise unchanged. Another way of looking at it is that the -S command is like -L, but in re-parsable syntax. If anything, -L would be deprecated. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-20 13:06 [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode Erik Skultety 2022-07-20 14:20 ` Florian Westphal 2022-07-20 16:07 ` Jan Engelhardt @ 2022-07-25 21:39 ` Florian Westphal 2022-07-26 6:55 ` Erik Skultety 2 siblings, 1 reply; 10+ messages in thread From: Florian Westphal @ 2022-07-25 21:39 UTC (permalink / raw) To: Erik Skultety; +Cc: netfilter-devel Erik Skultety <eskultet@redhat.com> wrote: > The fact that the 'opt' table field reports spaces instead of '--' for > IPv6 as it would have been the case with IPv4 has a bit of an > unfortunate side effect that it completely confuses the 'jc' JSON > formatter tool (which has an iptables formatter module). > Consider: > # ip6tables -L test > Chain test (0 references) > target prot opt source destination > ACCEPT all a:b:c:: anywhere MAC01:02:03:04:05:06 > > Then: > # ip6tables -L test | jc --iptables > [{"chain":"test", > "rules":[ > {"target":"ACCEPT", > "prot":"all", > "opt":"a:b:c::", > "source":"anywhere", > "destination":"MAC01:02:03:04:05:06" > }] > }] > > which as you can see is wrong simply because whitespaces are considered > as a column delimiter. Applied. I amended the commit message to include a Link to this thread on lore.kernel.org so in case something else breaks because of this change. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode 2022-07-25 21:39 ` Florian Westphal @ 2022-07-26 6:55 ` Erik Skultety 0 siblings, 0 replies; 10+ messages in thread From: Erik Skultety @ 2022-07-26 6:55 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Mon, Jul 25, 2022 at 11:39:14PM +0200, Florian Westphal wrote: > Erik Skultety <eskultet@redhat.com> wrote: > > The fact that the 'opt' table field reports spaces instead of '--' for > > IPv6 as it would have been the case with IPv4 has a bit of an > > unfortunate side effect that it completely confuses the 'jc' JSON > > formatter tool (which has an iptables formatter module). > > Consider: > > # ip6tables -L test > > Chain test (0 references) > > target prot opt source destination > > ACCEPT all a:b:c:: anywhere MAC01:02:03:04:05:06 > > > > Then: > > # ip6tables -L test | jc --iptables > > [{"chain":"test", > > "rules":[ > > {"target":"ACCEPT", > > "prot":"all", > > "opt":"a:b:c::", > > "source":"anywhere", > > "destination":"MAC01:02:03:04:05:06" > > }] > > }] > > > > which as you can see is wrong simply because whitespaces are considered > > as a column delimiter. > > Applied. I amended the commit message to include a Link to this thread > on lore.kernel.org so in case something else breaks because of this > change. > Thanks! However, given Phil's findings in his reply to the patch I think my patch is an incomplete fix without his suggested/proposed follow-ups, so hopefully those could land upstream as well. Regards, Erik ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-07-26 6:55 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-20 13:06 [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode Erik Skultety 2022-07-20 14:20 ` Florian Westphal 2022-07-20 16:11 ` Erik Skultety 2022-07-23 9:47 ` Phil Sutter 2022-07-23 12:35 ` Florian Westphal 2022-07-20 16:07 ` Jan Engelhardt 2022-07-20 16:56 ` Erik Skultety 2022-07-21 7:22 ` Jan Engelhardt 2022-07-25 21:39 ` Florian Westphal 2022-07-26 6:55 ` Erik Skultety
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).