[iptables PATCH 0/5] xtables-monitor enhancements

[iptables PATCH 0/5] xtables-monitor enhancements

[Phil Sutter]

This series introduces family-specific xtables-monitor aliases as
suggested during NFWS, the relevant code is in patches four and five.

The first three patches are fallout from the above, mostly minor fixes
found along the way.

Phil Sutter (5):
  doc: Clean generated *-restore-translate man pages
  doc: Fix xtables-monitor man page
  xtables-monitor: Improve error messages
  xtables-monitor: Support ARP and bridge families
  xtables-monitor: Add family-specific aliases

 iptables/Makefile.am          |  21 ++++--
 iptables/xtables-monitor.8.in |  32 +++++++--
 iptables/xtables-monitor.c    | 124 ++++++++++++++++++++++++++++------
 iptables/xtables-multi.h      |   4 ++
 iptables/xtables-nft-multi.c  |   4 ++
 5 files changed, 154 insertions(+), 31 deletions(-)

-- 
2.22.0

[iptables PATCH 1/5] doc: Clean generated *-restore-translate man pages

[Phil Sutter]

Since they are generated, one has to specify them in CLEANFILES. While
being at it, introduce a variable holding them to improve readability a
bit.

Fixes: 3dfb01cf14d72 ("doc: Install ip{6,}tables-restore-translate.8 man pages")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/Makefile.am | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/iptables/Makefile.am b/iptables/Makefile.am
index 21ac7f08b7c1f..da07b9a4b5a2f 100644
--- a/iptables/Makefile.am
+++ b/iptables/Makefile.am
@@ -47,6 +47,9 @@ xtables_nft_multi_SOURCES += xshared.c
 xtables_nft_multi_LDADD   += ../libxtables/libxtables.la -lm
 endif
 
+XTABLES_XLATE_8_LINKS = iptables-translate.8 ip6tables-translate.8 \
+			iptables-restore-translate.8 ip6tables-restore-translate.8
+
 sbin_PROGRAMS    = xtables-legacy-multi
 if ENABLE_NFTABLES
 sbin_PROGRAMS	+= xtables-nft-multi
@@ -56,14 +59,13 @@ man_MANS         = iptables.8 iptables-restore.8 iptables-save.8 \
                    ip6tables-save.8 iptables-extensions.8
 if ENABLE_NFTABLES
 man_MANS	+= xtables-nft.8 xtables-translate.8 xtables-legacy.8 \
-                   iptables-translate.8 ip6tables-translate.8 \
-		   iptables-restore-translate.8 ip6tables-restore-translate.8 \
+                   ${XTABLES_XLATE_8_LINKS} \
                    xtables-monitor.8 \
                    arptables-nft.8 arptables-nft-restore.8 arptables-nft-save.8 \
                    ebtables-nft.8
 endif
 CLEANFILES       = iptables.8 xtables-monitor.8 \
-		   iptables-translate.8 ip6tables-translate.8
+		   ${XTABLES_XLATE_8_LINKS}
 
 vx_bin_links   = iptables-xml
 if ENABLE_IPV4
@@ -93,7 +95,7 @@ iptables-extensions.8: iptables-extensions.8.tmpl ../extensions/matches.man ../e
 		-e '/@MATCH@/ r ../extensions/matches.man' \
 		-e '/@TARGET@/ r ../extensions/targets.man' $< >$@;
 
-iptables-translate.8 ip6tables-translate.8 iptables-restore-translate.8 ip6tables-restore-translate.8:
+${XTABLES_XLATE_8_LINKS}:
 	${AM_VERBOSE_GEN} echo '.so man8/xtables-translate.8' >$@
 
 pkgconfig_DATA = xtables.pc
-- 
2.22.0

[iptables PATCH 2/5] doc: Fix xtables-monitor man page

[Phil Sutter]

Fix some syntax errors, also document -4 and -6 long options.

Fixes: d26c538b9a549 ("xtables: add xtables-monitor")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-monitor.8.in | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/iptables/xtables-monitor.8.in b/iptables/xtables-monitor.8.in
index b647a79eb64ed..19eb729c51240 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\fB]
+\fBxtables\-monitor\fP [\fB\-t\fP] [\fB\-e\fP] [\fB\-4\fP|\fB\-6\fP]
 .PP
 \
 .SH DESCRIPTION
@@ -14,8 +14,8 @@ for packets tagged using the TRACE target.
 will run until the user aborts execution, typically by using CTRL-C.
 .RE
 .SH OPTIONS
-\fB\-e\fP, \fB\-\-event\fP
 .TP
+\fB\-e\fP, \fB\-\-event\fP
 Watch for updates to the rule set.
 Updates include creation of new tables, chains and rules and
 the name of the program that caused the rule update.
@@ -24,10 +24,10 @@ 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\-4\fP
+\fB\-4\fP, \fB--ipv4\fP
 Restrict output to IPv4.
 .TP
-\fB\-6\fP
+\fB\-6\fP, \fB--ipv6\fP
 Restrict output to IPv6.
 .SH EXAMPLE OUTPUT
 .TP
-- 
2.22.0

[iptables PATCH 3/5] xtables-monitor: Improve error messages

[Phil Sutter]

Print a line explaining what was wrong before the general help text.
Also catch multiple family selectors, they overwrite each other and
hence could cause unexpected behaviour.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-monitor.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c
index eb80bac81c645..02e8e446b1c8c 100644
--- a/iptables/xtables-monitor.c
+++ b/iptables/xtables-monitor.c
@@ -588,6 +588,16 @@ static void print_usage(void)
 	exit(EXIT_FAILURE);
 }
 
+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");
+		print_usage();
+		exit(PARAMETER_PROBLEM);
+	}
+	arg->nfproto = val;
+}
+
 int xtables_monitor_main(int argc, char *argv[])
 {
 	struct mnl_socket *nl;
@@ -626,10 +636,10 @@ int xtables_monitor_main(int argc, char *argv[])
 			print_usage();
 			exit(0);
 		case '4':
-			cb_arg.nfproto = NFPROTO_IPV4;
+			set_nfproto(&cb_arg, NFPROTO_IPV4);
 			break;
 		case '6':
-			cb_arg.nfproto = NFPROTO_IPV6;
+			set_nfproto(&cb_arg, NFPROTO_IPV6);
 			break;
 		case 'V':
 			printf("xtables-monitor %s\n", PACKAGE_VERSION);
@@ -647,6 +657,7 @@ int xtables_monitor_main(int argc, char *argv[])
 		nfgroup |= 1 << (NFNLGRP_NFTABLES - 1);
 
 	if (nfgroup == 0) {
+		fprintf(stderr, "Missing mandatory argument, specify either '-t' or '-e' (or both).\n\n");
 		print_usage();
 		exit(EXIT_FAILURE);
 	}
-- 
2.22.0

[iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Phil Sutter]

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(-)

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;
-- 
2.22.0

[iptables PATCH 5/5] xtables-monitor: Add family-specific aliases

[Phil Sutter]

Allow for more intuitive xtables-monitor use, e.g. 'ebtables-monitor'
instead of 'xtables-monitor --bridge'. This needs separate main
functions to call from xtables-nft-multi.c and in turn allows to
properly initialize for each family. The latter is required to correctly
print e.g. rules using ebtables extensions.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/Makefile.am          | 13 ++++--
 iptables/xtables-monitor.8.in | 14 ++++++
 iptables/xtables-monitor.c    | 88 ++++++++++++++++++++++++++++-------
 iptables/xtables-multi.h      |  4 ++
 iptables/xtables-nft-multi.c  |  4 ++
 5 files changed, 104 insertions(+), 19 deletions(-)

diff --git a/iptables/Makefile.am b/iptables/Makefile.am
index da07b9a4b5a2f..0af9f8dc7738e 100644
--- a/iptables/Makefile.am
+++ b/iptables/Makefile.am
@@ -49,6 +49,8 @@ endif
 
 XTABLES_XLATE_8_LINKS = iptables-translate.8 ip6tables-translate.8 \
 			iptables-restore-translate.8 ip6tables-restore-translate.8
+XTABLES_MONITOR_8_LINKS = iptables-monitor.8 ip6tables-monitor.8 \
+			  arptables-monitor.8 ebtables-monitor.8
 
 sbin_PROGRAMS    = xtables-legacy-multi
 if ENABLE_NFTABLES
@@ -60,12 +62,13 @@ man_MANS         = iptables.8 iptables-restore.8 iptables-save.8 \
 if ENABLE_NFTABLES
 man_MANS	+= xtables-nft.8 xtables-translate.8 xtables-legacy.8 \
                    ${XTABLES_XLATE_8_LINKS} \
-                   xtables-monitor.8 \
+                   xtables-monitor.8 ${XTABLES_MONITOR_8_LINKS} \
                    arptables-nft.8 arptables-nft-restore.8 arptables-nft-save.8 \
                    ebtables-nft.8
 endif
 CLEANFILES       = iptables.8 xtables-monitor.8 \
-		   ${XTABLES_XLATE_8_LINKS}
+		   ${XTABLES_XLATE_8_LINKS} \
+		   ${XTABLES_MONITOR_8_LINKS}
 
 vx_bin_links   = iptables-xml
 if ENABLE_IPV4
@@ -87,7 +90,8 @@ x_sbin_links  = iptables-nft iptables-nft-restore iptables-nft-save \
 		ebtables-nft ebtables \
 		ebtables-nft-restore ebtables-restore \
 		ebtables-nft-save ebtables-save \
-		xtables-monitor
+		xtables-monitor arptables-monitor ebtables-monitor \
+		iptables-monitor ip6tables-monitor
 endif
 
 iptables-extensions.8: iptables-extensions.8.tmpl ../extensions/matches.man ../extensions/targets.man
@@ -98,6 +102,9 @@ iptables-extensions.8: iptables-extensions.8.tmpl ../extensions/matches.man ../e
 ${XTABLES_XLATE_8_LINKS}:
 	${AM_VERBOSE_GEN} echo '.so man8/xtables-translate.8' >$@
 
+${XTABLES_MONITOR_8_LINKS}:
+	${AM_VERBOSE_GEN} echo '.so man8/xtables-monitor.8' >$@
+
 pkgconfig_DATA = xtables.pc
 
 # Using if..fi avoids an ugly "error (ignored)" message :)
diff --git a/iptables/xtables-monitor.8.in b/iptables/xtables-monitor.8.in
index 6bde54fa4a359..b91b81489d1b9 100644
--- a/iptables/xtables-monitor.8.in
+++ b/iptables/xtables-monitor.8.in
@@ -3,6 +3,14 @@
 xtables-monitor \(em show changes to rule set and trace-events
 .SH SYNOPSIS
 \fBxtables\-monitor\fP [\fB\-t\fP] [\fB\-e\fP] [\fB\-0\fP|\fB-1\fP|\fB\-4\fP|\fB\-6\fP]
+.br
+\fBiptables-monitor\fP [\fB\-t\fP] [\fB\-e\fP]
+.br
+\fBip6tables-monitor\fP [\fB\-t\fP] [\fB\-e\fP]
+.br
+\fBarptables-monitor\fP [\fB\-t\fP] [\fB\-e\fP]
+.br
+\fBebtables-monitor\fP [\fB\-t\fP] [\fB\-e\fP]
 .PP
 \
 .SH DESCRIPTION
@@ -12,6 +20,12 @@ is used to monitor changes to the ruleset or to show rule evaluation events
 for packets tagged using the TRACE target.
 .B xtables-monitor
 will run until the user aborts execution, typically by using CTRL-C.
+.PP
+.BR iptables-monitor ", " ip6tables-monitor ", "
+.BR arptables-monitor " and " ebtables-monitor
+are aliases to calling
+.B xtables-monitor
+with a family filtering flag.
 .RE
 .SH OPTIONS
 .TP
diff --git a/iptables/xtables-monitor.c b/iptables/xtables-monitor.c
index 9be8ce9de6b5f..71e9de45a34a3 100644
--- a/iptables/xtables-monitor.c
+++ b/iptables/xtables-monitor.c
@@ -37,6 +37,7 @@
 #include "xtables-multi.h"
 #include "nft.h"
 #include "nft-arp.h"
+#include "nft-bridge.h"
 
 struct cb_arg {
 	uint32_t nfproto;
@@ -611,28 +612,16 @@ static void set_nfproto(struct cb_arg *arg, uint32_t val)
 	arg->nfproto = val;
 }
 
-int xtables_monitor_main(int argc, char *argv[])
+static int __xtables_monitor_main(uint32_t family, int argc, char *argv[])
 {
 	struct mnl_socket *nl;
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	uint32_t nfgroup = 0;
-	struct cb_arg cb_arg = {};
+	struct cb_arg cb_arg = {
+		.nfproto = family,
+	};
 	int ret, c;
 
-	xtables_globals.program_name = "xtables-monitor";
-	/* XXX xtables_init_all does several things we don't want */
-	c = xtables_init_all(&xtables_globals, NFPROTO_IPV4);
-	if (c < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
-				xtables_globals.program_name,
-				xtables_globals.program_version);
-		exit(1);
-	}
-#if defined(ALL_INCLUSIVE) || defined(NO_SHARED_LIBS)
-	init_extensions();
-	init_extensions4();
-#endif
-
 	opterr = 0;
 	while ((c = getopt_long(argc, argv, "ceht0146V", options, NULL)) != -1) {
 		switch (c) {
@@ -708,3 +697,70 @@ int xtables_monitor_main(int argc, char *argv[])
 	return EXIT_SUCCESS;
 }
 
+static void common_xtables_init(const char *argv0, uint8_t family)
+{
+	xtables_globals.program_name = basename(argv0);
+
+	/* XXX xtables_init_all does several things we don't want */
+	if (xtables_init_all(&xtables_globals, family) < 0) {
+		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
+				xtables_globals.program_name,
+				xtables_globals.program_version);
+		exit(1);
+	}
+}
+
+int xtables_monitor_main(int argc, char *argv[])
+{
+	/* Can't pass NFPROTO_UNSPEC, xtables_set_nfproto() would complain. */
+	common_xtables_init(argv[0], NFPROTO_IPV4);
+
+	return __xtables_monitor_main(NFPROTO_UNSPEC, argc, argv);
+}
+
+int iptables_monitor_main(int argc, char *argv[])
+{
+	common_xtables_init(argv[0], NFPROTO_IPV4);
+#if defined(ALL_INCLUSIVE) || defined(NO_SHARED_LIBS)
+	init_extensions();
+	init_extensions4();
+#endif
+
+	return __xtables_monitor_main(NFPROTO_IPV4, argc, argv);
+}
+
+int ip6tables_monitor_main(int argc, char *argv[])
+{
+	common_xtables_init(argv[0], NFPROTO_IPV6);
+#if defined(ALL_INCLUSIVE) || defined(NO_SHARED_LIBS)
+	init_extensions();
+	init_extensions4();
+#endif
+
+	return __xtables_monitor_main(NFPROTO_IPV6, argc, argv);
+}
+
+int arptables_monitor_main(int argc, char *argv[])
+{
+	common_xtables_init(argv[0], NFPROTO_ARP);
+#if defined(ALL_INCLUSIVE) || defined(NO_SHARED_LIBS)
+	init_extensionsa();
+#endif
+
+	return __xtables_monitor_main(NFPROTO_ARP, argc, argv);
+}
+
+int ebtables_monitor_main(int argc, char *argv[])
+{
+	common_xtables_init(argv[0], NFPROTO_BRIDGE);
+#if defined(ALL_INCLUSIVE) || defined(NO_SHARED_LIBS)
+	init_extensionsb();
+#endif
+	/* manually registering ebt matches, given the original ebtables parser
+	 * don't use '-m matchname' and the match can't be loaded dynamically when
+	 * the user calls it.
+	 */
+	ebt_load_match_extensions();
+
+	return __xtables_monitor_main(NFPROTO_BRIDGE, argc, argv);
+}
diff --git a/iptables/xtables-multi.h b/iptables/xtables-multi.h
index 0fedb430e1a28..11364ba5b731e 100644
--- a/iptables/xtables-multi.h
+++ b/iptables/xtables-multi.h
@@ -22,6 +22,10 @@ extern int xtables_eb_restore_main(int, char **);
 extern int xtables_eb_save_main(int, char **);
 extern int xtables_config_main(int, char **);
 extern int xtables_monitor_main(int, char **);
+extern int iptables_monitor_main(int, char **);
+extern int ip6tables_monitor_main(int, char **);
+extern int arptables_monitor_main(int, char **);
+extern int ebtables_monitor_main(int, char **);
 #endif
 
 #endif /* _XTABLES_MULTI_H */
diff --git a/iptables/xtables-nft-multi.c b/iptables/xtables-nft-multi.c
index e2b7c641f85dd..3b13d89dffcb3 100644
--- a/iptables/xtables-nft-multi.c
+++ b/iptables/xtables-nft-multi.c
@@ -44,6 +44,10 @@ static const struct subcommand multi_subcommands[] = {
 	{"ebtables-nft-restore",	xtables_eb_restore_main},
 	{"ebtables-nft-save",		xtables_eb_save_main},
 	{"xtables-monitor",		xtables_monitor_main},
+	{"iptables-monitor",		iptables_monitor_main},
+	{"ip6tables-monitor",		ip6tables_monitor_main},
+	{"arptables-monitor",		arptables_monitor_main},
+	{"ebtables-monitor",		ebtables_monitor_main},
 	{NULL},
 };
 
-- 
2.22.0

Re: [iptables PATCH 5/5] xtables-monitor: Add family-specific aliases

[Phil Sutter]

On Wed, Jul 31, 2019 at 06:39:15PM +0200, Phil Sutter wrote:
> Allow for more intuitive xtables-monitor use, e.g. 'ebtables-monitor'
> instead of 'xtables-monitor --bridge'. This needs separate main
> functions to call from xtables-nft-multi.c and in turn allows to
> properly initialize for each family. The latter is required to correctly
> print e.g. rules using ebtables extensions.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

This one needs a v2, I forgot to update iptables/.gitignore.

Sorry, Phil

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Pablo Neira Ayuso]

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.

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Phil Sutter]

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

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Pablo Neira Ayuso]

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!

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Pablo Neira Ayuso]

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.

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Phil Sutter]

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

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Pablo Neira Ayuso]

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!

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Phil Sutter]

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

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Pablo Neira Ayuso]

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.

Re: [iptables PATCH 4/5] xtables-monitor: Support ARP and bridge families

[Phil Sutter]

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