| Message ID | 20190731163915.22232-5-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | xtables-monitor enhancements | expand |
On Wed, Jul 31, 2019 at 06:39:14PM +0200, Phil Sutter wrote:
@@ -565,6 +574,8 @@ static const struct option options[] = {
> {.name = "counters", .has_arg = false, .val = 'c'},
> {.name = "trace", .has_arg = false, .val = 't'},
> {.name = "event", .has_arg = false, .val = 'e'},
> + {.name = "arp", .has_arg = false, .val = '0'},
> + {.name = "bridge", .has_arg = false, .val = '1'},
Probably?
-A for arp.
-B for bridge.
so users don't have to remember? -4 and -6 are intuitive, I'd like
these are sort of intuitive too in its compact definition.
Apart from that, patchset looks good to me.
Thanks.
On Thu, Aug 01, 2019 at 01:20:50PM +0200, Pablo Neira Ayuso wrote: > On Wed, Jul 31, 2019 at 06:39:14PM +0200, Phil Sutter wrote: > @@ -565,6 +574,8 @@ static const struct option options[] = { > > {.name = "counters", .has_arg = false, .val = 'c'}, > > {.name = "trace", .has_arg = false, .val = 't'}, > > {.name = "event", .has_arg = false, .val = 'e'}, > > + {.name = "arp", .has_arg = false, .val = '0'}, > > + {.name = "bridge", .has_arg = false, .val = '1'}, > > Probably? > > -A for arp. > -B for bridge. > > so users don't have to remember? -4 and -6 are intuitive, I'd like > these are sort of intuitive too in its compact definition. > > Apart from that, patchset looks good to me. I had something like that (-a and -b should still be free), but then discovered that for rules there was '-0' prefix in use when printing arp family rules. Should I change these prefixes also or leave them as -0 and -1? I guess most importantly they must not clash with real parameters. Thanks, Phil
On Thu, Aug 01, 2019 at 02:00:48PM +0200, Phil Sutter wrote: > On Thu, Aug 01, 2019 at 01:20:50PM +0200, Pablo Neira Ayuso wrote: > > On Wed, Jul 31, 2019 at 06:39:14PM +0200, Phil Sutter wrote: > > @@ -565,6 +574,8 @@ static const struct option options[] = { > > > {.name = "counters", .has_arg = false, .val = 'c'}, > > > {.name = "trace", .has_arg = false, .val = 't'}, > > > {.name = "event", .has_arg = false, .val = 'e'}, > > > + {.name = "arp", .has_arg = false, .val = '0'}, > > > + {.name = "bridge", .has_arg = false, .val = '1'}, > > > > Probably? > > > > -A for arp. > > -B for bridge. > > > > so users don't have to remember? -4 and -6 are intuitive, I'd like > > these are sort of intuitive too in its compact definition. > > > > Apart from that, patchset looks good to me. > > I had something like that (-a and -b should still be free), but then > discovered that for rules there was '-0' prefix in use when printing arp > family rules. Should I change these prefixes also or leave them as -0 > and -1? I guess most importantly they must not clash with real > parameters. You can just leave them as is if this is the way this is exposed in rules. Not sure what the logic behing -0 and -1 is, this is not mapping to NFPROTO_* definitions, so it looks like something it's been pulled out of someone's hat :-) I think users will end up using --arp and --bridge for this. I myself will not remember this -0 and -1 thing. Feel free to explore any possibility, probably leaving the existing -0 and -1 in place if you're afraid of breaking anything, add aliases and only document the more intuitive one. If you think this is worth exploring, of course. Thanks!
On Thu, Aug 01, 2019 at 02:30:40PM +0200, Pablo Neira Ayuso wrote: > On Thu, Aug 01, 2019 at 02:00:48PM +0200, Phil Sutter wrote: > > On Thu, Aug 01, 2019 at 01:20:50PM +0200, Pablo Neira Ayuso wrote: > > > On Wed, Jul 31, 2019 at 06:39:14PM +0200, Phil Sutter wrote: > > > @@ -565,6 +574,8 @@ static const struct option options[] = { > > > > {.name = "counters", .has_arg = false, .val = 'c'}, > > > > {.name = "trace", .has_arg = false, .val = 't'}, > > > > {.name = "event", .has_arg = false, .val = 'e'}, > > > > + {.name = "arp", .has_arg = false, .val = '0'}, > > > > + {.name = "bridge", .has_arg = false, .val = '1'}, > > > > > > Probably? > > > > > > -A for arp. > > > -B for bridge. > > > > > > so users don't have to remember? -4 and -6 are intuitive, I'd like > > > these are sort of intuitive too in its compact definition. > > > > > > Apart from that, patchset looks good to me. > > > > I had something like that (-a and -b should still be free), but then > > discovered that for rules there was '-0' prefix in use when printing arp > > family rules. Should I change these prefixes also or leave them as -0 > > and -1? I guess most importantly they must not clash with real > > parameters. > > You can just leave them as is if this is the way this is exposed in > rules. Not sure what the logic behing -0 and -1 is, this is not > mapping to NFPROTO_* definitions, so it looks like something it's been > pulled out of someone's hat :-) > > I think users will end up using --arp and --bridge for this. I myself > will not remember this -0 and -1 thing. Probably exposing: iptables-monitor ip6tables-monitor arptables-monitor ebtables-monitor although this will not solve the problem that we are discussing here, I think having those around would be nice. The xtables-monitor variant still will need to sort out the -0 and -1 thing that we're discussing here. > Feel free to explore any possibility, probably leaving the existing -0 > and -1 in place if you're afraid of breaking anything, add aliases and > only document the more intuitive one. If you think this is worth > exploring, of course.
Hi, On Thu, Aug 01, 2019 at 02:30:40PM +0200, Pablo Neira Ayuso wrote: > On Thu, Aug 01, 2019 at 02:00:48PM +0200, Phil Sutter wrote: > > On Thu, Aug 01, 2019 at 01:20:50PM +0200, Pablo Neira Ayuso wrote: > > > On Wed, Jul 31, 2019 at 06:39:14PM +0200, Phil Sutter wrote: > > > @@ -565,6 +574,8 @@ static const struct option options[] = { > > > > {.name = "counters", .has_arg = false, .val = 'c'}, > > > > {.name = "trace", .has_arg = false, .val = 't'}, > > > > {.name = "event", .has_arg = false, .val = 'e'}, > > > > + {.name = "arp", .has_arg = false, .val = '0'}, > > > > + {.name = "bridge", .has_arg = false, .val = '1'}, > > > > > > Probably? > > > > > > -A for arp. > > > -B for bridge. > > > > > > so users don't have to remember? -4 and -6 are intuitive, I'd like > > > these are sort of intuitive too in its compact definition. > > > > > > Apart from that, patchset looks good to me. > > > > I had something like that (-a and -b should still be free), but then > > discovered that for rules there was '-0' prefix in use when printing arp > > family rules. Should I change these prefixes also or leave them as -0 > > and -1? I guess most importantly they must not clash with real > > parameters. > > You can just leave them as is if this is the way this is exposed in > rules. Not sure what the logic behing -0 and -1 is, this is not > mapping to NFPROTO_* definitions, so it looks like something it's been > pulled out of someone's hat :-) Well, the '-1' certainly was! :D In ss tool, '-0' is used to select packet sockets. Maybe that's where it came from. > I think users will end up using --arp and --bridge for this. I myself > will not remember this -0 and -1 thing. That's correct. So I guess changing cmdline flags to -a/-b makes sense either way. > Feel free to explore any possibility, probably leaving the existing -0 > and -1 in place if you're afraid of breaking anything, add aliases and > only document the more intuitive one. If you think this is worth > exploring, of course. I would omit the prefix from output if a family was selected. For unfiltered xtables-monitor output, I would change the prefix to something more readable, e.g.: 'ip: ', 'ip6: ', 'arp: ', 'eb: ' What do you think? Thanks for the input, Phil
On Thu, Aug 01, 2019 at 02:41:07PM +0200, Phil Sutter wrote: > Hi, > > On Thu, Aug 01, 2019 at 02:30:40PM +0200, Pablo Neira Ayuso wrote: > > On Thu, Aug 01, 2019 at 02:00:48PM +0200, Phil Sutter wrote: [...] > > I think users will end up using --arp and --bridge for this. I myself > > will not remember this -0 and -1 thing. > > That's correct. So I guess changing cmdline flags to -a/-b makes sense > either way. In the rule side, getopt_long() is already pretty overloaded, just double check these are spare. > > Feel free to explore any possibility, probably leaving the existing -0 > > and -1 in place if you're afraid of breaking anything, add aliases and > > only document the more intuitive one. If you think this is worth > > exploring, of course. > > I would omit the prefix from output if a family was selected. For > unfiltered xtables-monitor output, I would change the prefix to > something more readable, e.g.: > > 'ip: ', > 'ip6: ', > 'arp: ', > 'eb: ' > > What do you think? Probably use the long option name, which seems more readable to me: EVENT: --ipv4 -t filter -A INPUT -j ACCEPT I like that the event is printed using the {ip,...}tables syntax. Thanks!
On Thu, Aug 01, 2019 at 02:47:38PM +0200, Pablo Neira Ayuso wrote: > On Thu, Aug 01, 2019 at 02:41:07PM +0200, Phil Sutter wrote: > > Hi, > > > > On Thu, Aug 01, 2019 at 02:30:40PM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Aug 01, 2019 at 02:00:48PM +0200, Phil Sutter wrote: > [...] > > > I think users will end up using --arp and --bridge for this. I myself > > > will not remember this -0 and -1 thing. > > > > That's correct. So I guess changing cmdline flags to -a/-b makes sense > > either way. > > In the rule side, getopt_long() is already pretty overloaded, just > double check these are spare. This is only about xtables-monitor cmdline, or am I missing something? > > > Feel free to explore any possibility, probably leaving the existing -0 > > > and -1 in place if you're afraid of breaking anything, add aliases and > > > only document the more intuitive one. If you think this is worth > > > exploring, of course. > > > > I would omit the prefix from output if a family was selected. For > > unfiltered xtables-monitor output, I would change the prefix to > > something more readable, e.g.: > > > > 'ip: ', > > 'ip6: ', > > 'arp: ', > > 'eb: ' > > > > What do you think? > > Probably use the long option name, which seems more readable to me: > > EVENT: --ipv4 -t filter -A INPUT -j ACCEPT Ah, good idea! > I like that the event is printed using the {ip,...}tables syntax. OK. --arp/--bridge won't work there, obviously. We could of course try to change that, but I guess it's not feasible. Also, IIRC 'iptables -6' was buggy in that it should fail but does not. This is a compatibility issue I didn't get to fix yet. Cheers, Phil
On Thu, Aug 01, 2019 at 02:58:00PM +0200, Phil Sutter wrote: > On Thu, Aug 01, 2019 at 02:47:38PM +0200, Pablo Neira Ayuso wrote: > > On Thu, Aug 01, 2019 at 02:41:07PM +0200, Phil Sutter wrote: > > > Hi, > > > > > > On Thu, Aug 01, 2019 at 02:30:40PM +0200, Pablo Neira Ayuso wrote: > > > > On Thu, Aug 01, 2019 at 02:00:48PM +0200, Phil Sutter wrote: > > [...] > > > > I think users will end up using --arp and --bridge for this. I myself > > > > will not remember this -0 and -1 thing. > > > > > > That's correct. So I guess changing cmdline flags to -a/-b makes sense > > > either way. > > > > In the rule side, getopt_long() is already pretty overloaded, just > > double check these are spare. > > This is only about xtables-monitor cmdline, or am I missing something? I was referring to the iptables rule command. Not sure it's worth there the alias. I think you mentioned that there's already -0 and -1 in the rule command line, hence the -0 and -1 for xtables-monitor. > > > > Feel free to explore any possibility, probably leaving the existing -0 > > > > and -1 in place if you're afraid of breaking anything, add aliases and > > > > only document the more intuitive one. If you think this is worth > > > > exploring, of course. > > > > > > I would omit the prefix from output if a family was selected. For > > > unfiltered xtables-monitor output, I would change the prefix to > > > something more readable, e.g.: > > > > > > 'ip: ', > > > 'ip6: ', > > > 'arp: ', > > > 'eb: ' > > > > > > What do you think? > > > > Probably use the long option name, which seems more readable to me: > > > > EVENT: --ipv4 -t filter -A INPUT -j ACCEPT > > Ah, good idea! > > > I like that the event is printed using the {ip,...}tables syntax. > > OK. --arp/--bridge won't work there, obviously. We could of course try > to change that, but I guess it's not feasible. I think we would need a common parser, and that's not feasible. Unless there is some preparsing, just to check if the family option is in place, ie. -4, -6, --arp and --bridge, then route the parsing to the corresponding parser. It's a bit of extra glue code, not sure it's worth, just an idea / future work if helping all these tooling converge might be of interest. > Also, IIRC 'iptables -6' was buggy in that it should fail but does > not. This is a compatibility issue I didn't get to fix yet. Noted. I have seen the recent patch to fix this.
On Thu, Aug 01, 2019 at 03:03:03PM +0200, Pablo Neira Ayuso wrote: > On Thu, Aug 01, 2019 at 02:58:00PM +0200, Phil Sutter wrote: > > On Thu, Aug 01, 2019 at 02:47:38PM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Aug 01, 2019 at 02:41:07PM +0200, Phil Sutter wrote: > > > > Hi, > > > > > > > > On Thu, Aug 01, 2019 at 02:30:40PM +0200, Pablo Neira Ayuso wrote: > > > > > On Thu, Aug 01, 2019 at 02:00:48PM +0200, Phil Sutter wrote: > > > [...] > > > > > I think users will end up using --arp and --bridge for this. I myself > > > > > will not remember this -0 and -1 thing. > > > > > > > > That's correct. So I guess changing cmdline flags to -a/-b makes sense > > > > either way. > > > > > > In the rule side, getopt_long() is already pretty overloaded, just > > > double check these are spare. > > > > This is only about xtables-monitor cmdline, or am I missing something? > > I was referring to the iptables rule command. Not sure it's worth > there the alias. I think you mentioned that there's already -0 and -1 > in the rule command line, hence the -0 and -1 for xtables-monitor. Why should xtables-monitor print something that can be used as input to iptables? > > > > > Feel free to explore any possibility, probably leaving the existing -0 > > > > > and -1 in place if you're afraid of breaking anything, add aliases and > > > > > only document the more intuitive one. If you think this is worth > > > > > exploring, of course. > > > > > > > > I would omit the prefix from output if a family was selected. For > > > > unfiltered xtables-monitor output, I would change the prefix to > > > > something more readable, e.g.: > > > > > > > > 'ip: ', > > > > 'ip6: ', > > > > 'arp: ', > > > > 'eb: ' > > > > > > > > What do you think? > > > > > > Probably use the long option name, which seems more readable to me: > > > > > > EVENT: --ipv4 -t filter -A INPUT -j ACCEPT > > > > Ah, good idea! > > > > > I like that the event is printed using the {ip,...}tables syntax. > > > > OK. --arp/--bridge won't work there, obviously. We could of course try > > to change that, but I guess it's not feasible. > > I think we would need a common parser, and that's not feasible. Unless > there is some preparsing, just to check if the family option is in > place, ie. -4, -6, --arp and --bridge, then route the parsing to the > corresponding parser. It's a bit of extra glue code, not sure it's > worth, just an idea / future work if helping all these tooling > converge might be of interest. Given the large differences in ebtables cmdline syntax to the other tools, I consider it a plus to have different commands (and hence separate "main" functions). > > Also, IIRC 'iptables -6' was buggy in that it should fail but does > > not. This is a compatibility issue I didn't get to fix yet. > > Noted. I have seen the recent patch to fix this. That was only for iptables-nft-restore. I am talking about plain iptables: | % iptables-legacy -6 -A FORWARD -j ACCEPT | This is the IPv4 version of iptables. | Try `iptables -h' or 'iptables --help' for more information. iptables-nft accepts this but the result seems to be identical to just omitting '-6'. Cheers, Phil
diff --git a/iptables/xtables-monitor.8.in b/iptables/xtables-monitor.8.in index 19eb729c51240..6bde54fa4a359 100644 --- a/iptables/xtables-monitor.8.in +++ b/iptables/xtables-monitor.8.in @@ -2,7 +2,7 @@ .SH NAME xtables-monitor \(em show changes to rule set and trace-events .SH SYNOPSIS -\fBxtables\-monitor\fP [\fB\-t\fP] [\fB\-e\fP] [\fB\-4\fP|\fB\-6\fP] +\fBxtables\-monitor\fP [\fB\-t\fP] [\fB\-e\fP] [\fB\-0\fP|\fB-1\fP|\fB\-4\fP|\fB\-6\fP] .PP \ .SH DESCRIPTION @@ -24,11 +24,17 @@ the name of the program that caused the rule update. Watch for trace events generated by packets that have been tagged using the TRACE target. .TP +\fB\-0\fP, \fB--arp\fP +Restrict output to ARP (i.e., events caused by arptables-nft). +.TP +\fB\-1\fP, \fB--bridge\fP +Restrict output to bridge (i.e., events caused by ebtables-nft). +.TP \fB\-4\fP, \fB--ipv4\fP -Restrict output to IPv4. +Restrict output to IPv4 (i.e., events caused by iptables-nft). .TP \fB\-6\fP, \fB--ipv6\fP -Restrict output to IPv6. +Restrict output to IPv6 (i.e., events caused by ip6tables-nft). .SH EXAMPLE OUTPUT .TP .B xtables-monitor \-\-trace diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c index 02e8e446b1c8c..9be8ce9de6b5f 100644 --- a/iptables/xtables-monitor.c +++ b/iptables/xtables-monitor.c @@ -101,6 +101,9 @@ static int rule_cb(const struct nlmsghdr *nlh, void *data) case NFPROTO_ARP: printf("-0 "); break; + case NFPROTO_BRIDGE: + printf("-1 "); + break; default: goto err_free; } @@ -139,6 +142,12 @@ static int chain_cb(const struct nlmsghdr *nlh, void *data) printf(" EVENT: "); switch (family) { + case NFPROTO_ARP: + family = 0; + break; + case NFPROTO_BRIDGE: + family = 1; + break; case NFPROTO_IPV4: family = 4; break; @@ -565,6 +574,8 @@ static const struct option options[] = { {.name = "counters", .has_arg = false, .val = 'c'}, {.name = "trace", .has_arg = false, .val = 't'}, {.name = "event", .has_arg = false, .val = 'e'}, + {.name = "arp", .has_arg = false, .val = '0'}, + {.name = "bridge", .has_arg = false, .val = '1'}, {.name = "ipv4", .has_arg = false, .val = '4'}, {.name = "ipv6", .has_arg = false, .val = '6'}, {.name = "version", .has_arg = false, .val = 'V'}, @@ -580,6 +591,8 @@ static void print_usage(void) " --trace -t trace ruleset traversal of packets tagged via -j TRACE rule\n" " --event -e show events that modify the ruleset\n" "Optional arguments:\n" + " --arp -0 only monitor ARP\n" + " --bridge -1 only monitor bridge\n" " --ipv4 -4 only monitor IPv4\n" " --ipv6 -6 only monitor IPv6\n" " --counters -c show counters in rules\n" @@ -591,7 +604,7 @@ static void print_usage(void) static void set_nfproto(struct cb_arg *arg, uint32_t val) { if (arg->nfproto != NFPROTO_UNSPEC && arg->nfproto != val) { - fprintf(stderr, "Only one of '-4' or '-6' may be specified at once.\n\n"); + fprintf(stderr, "Only one of '-0', '-1', '-4' or '-6' may be specified at once.\n\n"); print_usage(); exit(PARAMETER_PROBLEM); } @@ -621,8 +634,14 @@ int xtables_monitor_main(int argc, char *argv[]) #endif opterr = 0; - while ((c = getopt_long(argc, argv, "ceht46V", options, NULL)) != -1) { + while ((c = getopt_long(argc, argv, "ceht0146V", options, NULL)) != -1) { switch (c) { + case '0': + set_nfproto(&cb_arg, NFPROTO_ARP); + break; + case '1': + set_nfproto(&cb_arg, NFPROTO_BRIDGE); + break; case 'c': counters = true; break;
Apart from allowing to filter by these families, add missing switch() cases in chain and rule callbacks. Signed-off-by: Phil Sutter <phil@nwl.cc> --- iptables/xtables-monitor.8.in | 12 +++++++++--- iptables/xtables-monitor.c | 23 +++++++++++++++++++++-- 2 files changed, 30 insertions(+), 5 deletions(-)