[iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[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

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Florian Westphal]

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?

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Erik Skultety]

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

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Phil Sutter]

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

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Florian Westphal]

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.

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Jan Engelhardt]

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.

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Erik Skultety]

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

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Jan Engelhardt]

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.

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Florian Westphal]

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.

Re: [iptables PATCH] iptables: xshared: Ouptut '--' in the opt field in ipv6's fake mode

[Erik Skultety]

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