[PATCH lnfct-ct 1/2] conntrack: snprintf: add labels in hex representation by default

[PATCH lnfct-ct 1/2] conntrack: snprintf: add labels in hex representation by default

[Florian Westphal]

udp      17 24 [..] dport=62277 mark=0 use=1 labels=0x28000000000000008
or
[..]<mark>0</mark><labels>0x28000000000000008</labels> [..] </meta></flow>

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 I'm interested if there are any objections on putting
 this in by default, one alternative would be to toss this and only
 print label info with special formating request (see next patch,
 which adds NFCT_OF_ specifier (which will then print the resovled names
 instead of bitmask).

 include/internal/prototypes.h    |  1 +
 src/conntrack/snprintf_default.c | 36 ++++++++++++++++++++++++++++++++++++
 src/conntrack/snprintf_xml.c     | 13 +++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/include/internal/prototypes.h b/include/internal/prototypes.h
index 484deea..cc8de1d 100644
--- a/include/internal/prototypes.h
+++ b/include/internal/prototypes.h
@@ -15,6 +15,7 @@ int __snprintf_protocol(char *buf, unsigned int len, const struct nf_conntrack *
 int __snprintf_proto(char *buf, unsigned int len, const struct __nfct_tuple *tuple);
 int __snprintf_conntrack_default(char *buf, unsigned int len, const struct nf_conntrack *ct, const unsigned int msg_type, const unsigned int flags);
 int __snprintf_conntrack_xml(char *buf, unsigned int len, const struct nf_conntrack *ct, const unsigned int msg_type, const unsigned int flags);
+int __snprintf_connlabels(char *buf, unsigned int len, const struct nf_conntrack *ct, const char *prefix, const char *end);
 
 enum __nfct_addr {
 	__ADDR_SRC = 0,
diff --git a/src/conntrack/snprintf_default.c b/src/conntrack/snprintf_default.c
index 911faea..a1edef4 100644
--- a/src/conntrack/snprintf_default.c
+++ b/src/conntrack/snprintf_default.c
@@ -288,6 +288,37 @@ __snprintf_helper_name(char *buf, unsigned int len, const struct nf_conntrack *c
 	return (snprintf(buf, len, "helper=%s ", ct->helper_name));
 }
 
+int
+__snprintf_connlabels(char *buf, unsigned int len,
+		      const struct nf_conntrack *ct,
+		      const char *prefix, const char *end)
+{
+	const struct nfct_bitmask *b = nfct_get_attr(ct, ATTR_CONNLABELS);
+	unsigned int i;
+	int ret, size = 0, offset = 0;
+
+	if (!b)
+		return 0;
+	for (i = b->words - 1; i && b->bits[i] == 0; i--)
+		/* nothing */;
+	ret = snprintf(buf, len, "%s0x%x", prefix, b->bits[i]);
+	BUFFER_SIZE(ret, size, len, offset);
+	while (i > 0) {
+		i--;
+		ret = snprintf(buf + offset, len, "%08x", b->bits[i]);
+		BUFFER_SIZE(ret, size, len, offset);
+	}
+	ret = snprintf(buf + offset, len, "%s", end);
+	BUFFER_SIZE(ret, size, len, offset);
+	return size;
+}
+
+static int
+__snprintf_clabels(char *buf, unsigned int len, const struct nf_conntrack *ct)
+{
+	return __snprintf_connlabels(buf, len, ct, "labels=", " ");
+}
+
 int __snprintf_conntrack_default(char *buf, 
 				 unsigned int len,
 				 const struct nf_conntrack *ct,
@@ -426,6 +457,11 @@ int __snprintf_conntrack_default(char *buf,
 		BUFFER_SIZE(ret, size, len, offset);
 	}
 
+	if (test_bit(ATTR_CONNLABELS, ct->head.set)) {
+		ret = __snprintf_clabels(buf+offset, len, ct);
+		BUFFER_SIZE(ret, size, len, offset);
+	}
+
 	/* Delete the last blank space */
 	size--;
 
diff --git a/src/conntrack/snprintf_xml.c b/src/conntrack/snprintf_xml.c
index ad53075..f373f5b 100644
--- a/src/conntrack/snprintf_xml.c
+++ b/src/conntrack/snprintf_xml.c
@@ -348,6 +348,12 @@ static int __snprintf_tuple_xml(char *buf,
 	return size;
 }
 
+static int
+__snprintf_clabels_xml(char *buf, unsigned int len, const struct nf_conntrack *ct)
+{
+	return __snprintf_connlabels(buf, len, ct, "<labels>", "</labels>");
+}
+
 int __snprintf_conntrack_xml(char *buf,
 			     unsigned int len,
 			     const struct nf_conntrack *ct,
@@ -390,6 +396,7 @@ int __snprintf_conntrack_xml(char *buf,
 	    test_bit(ATTR_USE, ct->head.set) ||
 	    test_bit(ATTR_STATUS, ct->head.set) ||
 	    test_bit(ATTR_ID, ct->head.set) ||
+	    test_bit(ATTR_CONNLABELS, ct->head.set) ||
 	    test_bit(ATTR_TIMESTAMP_START, ct->head.set) ||
 	    test_bit(ATTR_TIMESTAMP_STOP, ct->head.set)) {
 		ret = snprintf(buf+offset, len, 
@@ -432,6 +439,11 @@ int __snprintf_conntrack_xml(char *buf,
 		BUFFER_SIZE(ret, size, len, offset);
 	}
 
+	if (test_bit(ATTR_CONNLABELS, ct->head.set)) {
+		ret = __snprintf_clabels_xml(buf+offset, len, ct);
+		BUFFER_SIZE(ret, size, len, offset);
+	}
+
 	if (test_bit(ATTR_SECMARK, ct->head.set)) {
 		ret = snprintf(buf+offset, len, 
 				"<secmark>%u</secmark>", ct->secmark);
@@ -510,6 +522,7 @@ int __snprintf_conntrack_xml(char *buf,
 	    test_bit(ATTR_USE, ct->head.set) ||
 	    test_bit(ATTR_STATUS, ct->head.set) ||
 	    test_bit(ATTR_ID, ct->head.set) ||
+	    test_bit(ATTR_CONNLABELS, ct->head.set) ||
 	    test_bit(ATTR_TIMESTAMP_START, ct->head.set) ||
 	    test_bit(ATTR_TIMESTAMP_STOP, ct->head.set)) {
 	    	ret = snprintf(buf+offset, len, "</meta>");
-- 
1.8.1.5

[PATCH 2/2] conntrack: snprintf: add connlabel format specifier

[Florian Westphal]

by default, nfct_snprintf will not resolve connlabel names, as they're
system specific. This adds a fmt attribute to resolve names accordingly.

output looks like this:
... mark=0 use=1 labels=eth0-in (0),eth1-in (1)
or
<labels>
<label name="eth0-in" bit="0"/>
<label name="eth1-in" bit="1"/>
</labels>

Major stupidity:
As names can be anything, we need to be careful, especially when
creating XML output. Only alphanumerical label names are printed at
this time.

Else you get
labels=(0), .. or <labels><label name="" bit="0"/> in xml output.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 I am interested in feedback wrt.

 a) the chosen xml representation
 b) wheter it makes sense to limit valid charactes in the label
    names this way (e.g., should we care? should be disallow that
    in iptables as well, etc).
 c) if this patch makes sense in the first place, e.g, can't use
     alternate mapping file for labels with nfct_snprintf

 include/internal/internal.h                        |  1 +
 include/internal/prototypes.h                      |  1 +
 .../libnetfilter_conntrack.h                       |  3 +
 src/conntrack/snprintf_default.c                   | 84 +++++++++++++++++++++-
 src/conntrack/snprintf_xml.c                       |  8 ++-
 5 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/include/internal/internal.h b/include/internal/internal.h
index aaf6bd4..a5c7923 100644
--- a/include/internal/internal.h
+++ b/include/internal/internal.h
@@ -6,6 +6,7 @@
 #ifndef __LIBNETFILTER_CONNTRACK_INTERNAL__
 #define __LIBNETFILTER_CONNTRACK_INTERNAL__
 
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
diff --git a/include/internal/prototypes.h b/include/internal/prototypes.h
index cc8de1d..577270b 100644
--- a/include/internal/prototypes.h
+++ b/include/internal/prototypes.h
@@ -16,6 +16,7 @@ int __snprintf_proto(char *buf, unsigned int len, const struct __nfct_tuple *tup
 int __snprintf_conntrack_default(char *buf, unsigned int len, const struct nf_conntrack *ct, const unsigned int msg_type, const unsigned int flags);
 int __snprintf_conntrack_xml(char *buf, unsigned int len, const struct nf_conntrack *ct, const unsigned int msg_type, const unsigned int flags);
 int __snprintf_connlabels(char *buf, unsigned int len, const struct nf_conntrack *ct, const char *prefix, const char *end);
+int __snprintf_connlabels_names(char *buf, unsigned int len, const struct nf_conntrack *ct, const char *prefix, const char *end, bool xml);
 
 enum __nfct_addr {
 	__ADDR_SRC = 0,
diff --git a/include/libnetfilter_conntrack/libnetfilter_conntrack.h b/include/libnetfilter_conntrack/libnetfilter_conntrack.h
index 39dc24c..eedf85e 100644
--- a/include/libnetfilter_conntrack/libnetfilter_conntrack.h
+++ b/include/libnetfilter_conntrack/libnetfilter_conntrack.h
@@ -389,6 +389,9 @@ enum {
 
 	NFCT_OF_TIMESTAMP_BIT = 3,
 	NFCT_OF_TIMESTAMP = (1 << NFCT_OF_TIMESTAMP_BIT),
+
+	NFCT_OF_CONNLABELS_BIT = 4,
+	NFCT_OF_CONNLABELS = (1 << NFCT_OF_CONNLABELS_BIT),
 };
 
 extern int nfct_snprintf(char *buf, 
diff --git a/src/conntrack/snprintf_default.c b/src/conntrack/snprintf_default.c
index a1edef4..7befbd7 100644
--- a/src/conntrack/snprintf_default.c
+++ b/src/conntrack/snprintf_default.c
@@ -313,9 +313,89 @@ __snprintf_connlabels(char *buf, unsigned int len,
 	return size;
 }
 
+/*
+ * Labels can have any name, there are no restrictions.
+ * We will only print alpha numerical ones; else parsers
+ * could choke when putinng "&>" and friends into output
+ * (especially when using XML format).  ASCII machines only.
+ */
+static bool label_is_sane(const char *label)
+{
+	for (;*label; label++) {
+		if (*label >= 'a' && *label <= 'z')
+			continue;
+		if (*label >= 'A' && *label <= 'Z')
+			continue;
+		if (*label >= '0' && *label <= '9')
+			continue;
+		if (*label == ' ' || *label == '-')
+			continue;
+		return false;
+	}
+	return true;
+}
+
+int
+__snprintf_connlabels_names(char *buf, unsigned int len,
+			    const struct nf_conntrack *ct,
+			    const char *prefix, const char *end, bool xml)
+{
+	const struct nfct_bitmask *b = nfct_get_attr(ct, ATTR_CONNLABELS);
+	unsigned int i, max;
+	int ret, size = 0, offset = 0;
+	struct nfct_labelmap *map;
+
+	if (!b)
+		return 0;
+	map = nfct_labelmap_new(NULL);
+	if (!map)
+		return __snprintf_connlabels(buf, len, ct, prefix, end);
+
+	ret = snprintf(buf, len, "%s", prefix);
+	BUFFER_SIZE(ret, size, len, offset);
+	max = nfct_bitmask_maxbit(b);
+	for (i = 0; i <= max && len; i++) {
+		const char *name;
+		if (!nfct_bitmask_test_bit(b, i))
+			continue;
+		name = nfct_labelmap_get_name(map, i);
+		if (!name || !label_is_sane(name))
+			name = "";
+
+		if (xml)
+			ret = snprintf(buf + offset, len, "<label name=\"%s\" ", name);
+		else if (strcmp(name, "") != 0)
+			ret = snprintf(buf + offset, len, "%s ", name);
+		else
+			ret = 0;
+		BUFFER_SIZE(ret, size, len, offset);
+
+		if (xml)
+			ret = snprintf(buf + offset, len, "bit=\"%u\"/>", i);
+		else
+			ret = snprintf(buf + offset, len, "(%u),", i);
+		BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	nfct_labelmap_destroy(map);
+	if (offset && end) {
+		if (!xml) {
+			offset--; /* remove last , */
+			size--;
+		}
+		ret = snprintf(buf + offset, len, "%s", end);
+		BUFFER_SIZE(ret, size, len, offset);
+	}
+	return size;
+}
+
 static int
-__snprintf_clabels(char *buf, unsigned int len, const struct nf_conntrack *ct)
+__snprintf_clabels(char *buf, unsigned int len,
+		   const struct nf_conntrack *ct, bool names)
 {
+	if (names)
+		return  __snprintf_connlabels_names(buf, len, ct,
+					   "labels=", " ", false);
 	return __snprintf_connlabels(buf, len, ct, "labels=", " ");
 }
 
@@ -458,7 +538,7 @@ int __snprintf_conntrack_default(char *buf,
 	}
 
 	if (test_bit(ATTR_CONNLABELS, ct->head.set)) {
-		ret = __snprintf_clabels(buf+offset, len, ct);
+		ret = __snprintf_clabels(buf+offset, len, ct, flags & NFCT_OF_CONNLABELS );
 		BUFFER_SIZE(ret, size, len, offset);
 	}
 
diff --git a/src/conntrack/snprintf_xml.c b/src/conntrack/snprintf_xml.c
index f373f5b..1c2440a 100644
--- a/src/conntrack/snprintf_xml.c
+++ b/src/conntrack/snprintf_xml.c
@@ -349,8 +349,12 @@ static int __snprintf_tuple_xml(char *buf,
 }
 
 static int
-__snprintf_clabels_xml(char *buf, unsigned int len, const struct nf_conntrack *ct)
+__snprintf_clabels_xml(char *buf, unsigned int len,
+		       const struct nf_conntrack *ct, bool names)
 {
+	if (names)
+		return __snprintf_connlabels_names(buf, len, ct,
+				"<labels>", "</labels>", true);
 	return __snprintf_connlabels(buf, len, ct, "<labels>", "</labels>");
 }
 
@@ -440,7 +444,7 @@ int __snprintf_conntrack_xml(char *buf,
 	}
 
 	if (test_bit(ATTR_CONNLABELS, ct->head.set)) {
-		ret = __snprintf_clabels_xml(buf+offset, len, ct);
+		ret = __snprintf_clabels_xml(buf+offset, len, ct, flags & NFCT_OF_CONNLABELS);
 		BUFFER_SIZE(ret, size, len, offset);
 	}
 
-- 
1.8.1.5

Re: [PATCH 2/2] conntrack: snprintf: add connlabel format specifier

[Pablo Neira Ayuso]

On Tue, Jun 18, 2013 at 10:54:22PM +0200, Florian Westphal wrote:
> by default, nfct_snprintf will not resolve connlabel names, as they're
> system specific. This adds a fmt attribute to resolve names accordingly.
> 
> output looks like this:
> ... mark=0 use=1 labels=eth0-in (0),eth1-in (1)
> or
> <labels>
> <label name="eth0-in" bit="0"/>
> <label name="eth1-in" bit="1"/>
> </labels>

I think that the bit field is meaningless, so I would just export the
strings. I'd use:

<labels>
<label>eth0-in</label>
<label>eth1-in</label>
<labels>

I'd rather use elements, attributes are not easily extensible IMO.

> Major stupidity:
> As names can be anything, we need to be careful, especially when
> creating XML output. Only alphanumerical label names are printed at
> this time.

Hm, the string is enclosed between commas, isn't that enough to avoid
troubles?

> Else you get
> labels=(0), .. or <labels><label name="" bit="0"/> in xml output.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  I am interested in feedback wrt.
> 
>  a) the chosen xml representation
>  b) wheter it makes sense to limit valid charactes in the label
>     names this way (e.g., should we care? should be disallow that
>     in iptables as well, etc).
>  c) if this patch makes sense in the first place, e.g, can't use
>      alternate mapping file for labels with nfct_snprintf

Regarding c) I think it makes sense. You can add some
nfct_set_connlabel_file interface to set where to find it, it's a bit
cheating but should resolve the issue.

>  include/internal/internal.h                        |  1 +
>  include/internal/prototypes.h                      |  1 +
>  .../libnetfilter_conntrack.h                       |  3 +
>  src/conntrack/snprintf_default.c                   | 84 +++++++++++++++++++++-
>  src/conntrack/snprintf_xml.c                       |  8 ++-
>  5 files changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/include/internal/internal.h b/include/internal/internal.h
> index aaf6bd4..a5c7923 100644
> --- a/include/internal/internal.h
> +++ b/include/internal/internal.h
> @@ -6,6 +6,7 @@
>  #ifndef __LIBNETFILTER_CONNTRACK_INTERNAL__
>  #define __LIBNETFILTER_CONNTRACK_INTERNAL__
>  
> +#include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stdarg.h>
> diff --git a/include/internal/prototypes.h b/include/internal/prototypes.h
> index cc8de1d..577270b 100644
> --- a/include/internal/prototypes.h
> +++ b/include/internal/prototypes.h
> @@ -16,6 +16,7 @@ int __snprintf_proto(char *buf, unsigned int len, const struct __nfct_tuple *tup
>  int __snprintf_conntrack_default(char *buf, unsigned int len, const struct nf_conntrack *ct, const unsigned int msg_type, const unsigned int flags);
>  int __snprintf_conntrack_xml(char *buf, unsigned int len, const struct nf_conntrack *ct, const unsigned int msg_type, const unsigned int flags);
>  int __snprintf_connlabels(char *buf, unsigned int len, const struct nf_conntrack *ct, const char *prefix, const char *end);
> +int __snprintf_connlabels_names(char *buf, unsigned int len, const struct nf_conntrack *ct, const char *prefix, const char *end, bool xml);
>  
>  enum __nfct_addr {
>  	__ADDR_SRC = 0,
> diff --git a/include/libnetfilter_conntrack/libnetfilter_conntrack.h b/include/libnetfilter_conntrack/libnetfilter_conntrack.h
> index 39dc24c..eedf85e 100644
> --- a/include/libnetfilter_conntrack/libnetfilter_conntrack.h
> +++ b/include/libnetfilter_conntrack/libnetfilter_conntrack.h
> @@ -389,6 +389,9 @@ enum {
>  
>  	NFCT_OF_TIMESTAMP_BIT = 3,
>  	NFCT_OF_TIMESTAMP = (1 << NFCT_OF_TIMESTAMP_BIT),
> +
> +	NFCT_OF_CONNLABELS_BIT = 4,
> +	NFCT_OF_CONNLABELS = (1 << NFCT_OF_CONNLABELS_BIT),
>  };
>  
>  extern int nfct_snprintf(char *buf, 
> diff --git a/src/conntrack/snprintf_default.c b/src/conntrack/snprintf_default.c
> index a1edef4..7befbd7 100644
> --- a/src/conntrack/snprintf_default.c
> +++ b/src/conntrack/snprintf_default.c
> @@ -313,9 +313,89 @@ __snprintf_connlabels(char *buf, unsigned int len,
>  	return size;
>  }
>  
> +/*
> + * Labels can have any name, there are no restrictions.
> + * We will only print alpha numerical ones; else parsers
> + * could choke when putinng "&>" and friends into output
> + * (especially when using XML format).  ASCII machines only.
> + */
> +static bool label_is_sane(const char *label)
> +{
> +	for (;*label; label++) {
> +		if (*label >= 'a' && *label <= 'z')
> +			continue;
> +		if (*label >= 'A' && *label <= 'Z')
> +			continue;
> +		if (*label >= '0' && *label <= '9')
> +			continue;

is isalnum() instead?

> +		if (*label == ' ' || *label == '-')
> +			continue;i
> +		return false;
> +	}
> +	return true;
> +}
> +
> +int
> +__snprintf_connlabels_names(char *buf, unsigned int len,
> +			    const struct nf_conntrack *ct,
> +			    const char *prefix, const char *end, bool xml)
> +{
> +	const struct nfct_bitmask *b = nfct_get_attr(ct, ATTR_CONNLABELS);
> +	unsigned int i, max;
> +	int ret, size = 0, offset = 0;
> +	struct nfct_labelmap *map;
> +
> +	if (!b)
> +		return 0;
> +	map = nfct_labelmap_new(NULL);
> +	if (!map)
> +		return __snprintf_connlabels(buf, len, ct, prefix, end);
> +
> +	ret = snprintf(buf, len, "%s", prefix);
> +	BUFFER_SIZE(ret, size, len, offset);
> +	max = nfct_bitmask_maxbit(b);
> +	for (i = 0; i <= max && len; i++) {
> +		const char *name;
> +		if (!nfct_bitmask_test_bit(b, i))
> +			continue;
> +		name = nfct_labelmap_get_name(map, i);
> +		if (!name || !label_is_sane(name))
> +			name = "";
> +
> +		if (xml)
> +			ret = snprintf(buf + offset, len, "<label name=\"%s\" ", name);
> +		else if (strcmp(name, "") != 0)
> +			ret = snprintf(buf + offset, len, "%s ", name);
> +		else
> +			ret = 0;
> +		BUFFER_SIZE(ret, size, len, offset);
> +
> +		if (xml)
> +			ret = snprintf(buf + offset, len, "bit=\"%u\"/>", i);
> +		else
> +			ret = snprintf(buf + offset, len, "(%u),", i);
> +		BUFFER_SIZE(ret, size, len, offset);
> +	}
> +
> +	nfct_labelmap_destroy(map);
> +	if (offset && end) {
> +		if (!xml) {
> +			offset--; /* remove last , */
> +			size--;
> +		}
> +		ret = snprintf(buf + offset, len, "%s", end);
> +		BUFFER_SIZE(ret, size, len, offset);
> +	}
> +	return size;
> +}
> +
>  static int
> -__snprintf_clabels(char *buf, unsigned int len, const struct nf_conntrack *ct)
> +__snprintf_clabels(char *buf, unsigned int len,
> +		   const struct nf_conntrack *ct, bool names)
>  {
> +	if (names)
> +		return  __snprintf_connlabels_names(buf, len, ct,
> +					   "labels=", " ", false);
>  	return __snprintf_connlabels(buf, len, ct, "labels=", " ");
>  }
>  
> @@ -458,7 +538,7 @@ int __snprintf_conntrack_default(char *buf,
>  	}
>  
>  	if (test_bit(ATTR_CONNLABELS, ct->head.set)) {
> -		ret = __snprintf_clabels(buf+offset, len, ct);
> +		ret = __snprintf_clabels(buf+offset, len, ct, flags & NFCT_OF_CONNLABELS );
>  		BUFFER_SIZE(ret, size, len, offset);
>  	}
>  
> diff --git a/src/conntrack/snprintf_xml.c b/src/conntrack/snprintf_xml.c
> index f373f5b..1c2440a 100644
> --- a/src/conntrack/snprintf_xml.c
> +++ b/src/conntrack/snprintf_xml.c
> @@ -349,8 +349,12 @@ static int __snprintf_tuple_xml(char *buf,
>  }
>  
>  static int
> -__snprintf_clabels_xml(char *buf, unsigned int len, const struct nf_conntrack *ct)
> +__snprintf_clabels_xml(char *buf, unsigned int len,
> +		       const struct nf_conntrack *ct, bool names)
>  {
> +	if (names)
> +		return __snprintf_connlabels_names(buf, len, ct,
> +				"<labels>", "</labels>", true);
>  	return __snprintf_connlabels(buf, len, ct, "<labels>", "</labels>");
>  }
>  
> @@ -440,7 +444,7 @@ int __snprintf_conntrack_xml(char *buf,
>  	}
>  
>  	if (test_bit(ATTR_CONNLABELS, ct->head.set)) {
> -		ret = __snprintf_clabels_xml(buf+offset, len, ct);
> +		ret = __snprintf_clabels_xml(buf+offset, len, ct, flags & NFCT_OF_CONNLABELS);
>  		BUFFER_SIZE(ret, size, len, offset);
>  	}
>  
> -- 
> 1.8.1.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] conntrack: snprintf: add connlabel format specifier

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jun 18, 2013 at 10:54:22PM +0200, Florian Westphal wrote:
> > by default, nfct_snprintf will not resolve connlabel names, as they're
> > system specific. This adds a fmt attribute to resolve names accordingly.
> > 
> > output looks like this:
> > ... mark=0 use=1 labels=eth0-in (0),eth1-in (1)
> > or
> > <labels>
> > <label name="eth0-in" bit="0"/>
> > <label name="eth1-in" bit="1"/>
> > </labels>
> 
> I think that the bit field is meaningless, so I would just export the
> strings. I'd use:
> 
> <labels>
> <label>eth0-in</label>
> <label>eth1-in</label>
> <labels>
> 
> I'd rather use elements, attributes are not easily extensible IMO.

Okay, I'll change it accordingly.  In case translation fails i'll put
the number in there, so you'd get <label>(42)</label> if bit 42 is set
but has no mapping entry.

> > Major stupidity:
> > As names can be anything, we need to be careful, especially when
> > creating XML output. Only alphanumerical label names are printed at
> > this time.
> 
> Hm, the string is enclosed between commas, isn't that enough to avoid
> troubles?

What if connlabel.conf contains line like
0  foo<bar>&;!"

> >  a) the chosen xml representation
> >  b) wheter it makes sense to limit valid charactes in the label
> >     names this way (e.g., should we care? should be disallow that
> >     in iptables as well, etc).
> >  c) if this patch makes sense in the first place, e.g, can't use
> >      alternate mapping file for labels with nfct_snprintf
> 
> Regarding c) I think it makes sense. You can add some
> nfct_set_connlabel_file interface to set where to find it, it's a bit
> cheating but should resolve the issue.

Yuck ;-)

Lets ignore it for now, adding that would break thread-safety.
I guess one alternative would be to add

nfct_labelmap_snprintf(char *buf,
	       	       size_t sz,
		       const struct nfct_labelmap *,
		       const struct nfct_bitmask *, int flags);

When a use case presents itself.

> > +/*
> > + * Labels can have any name, there are no restrictions.
> > + * We will only print alpha numerical ones; else parsers
> > + * could choke when putinng "&>" and friends into output
> > + * (especially when using XML format).  ASCII machines only.
> > + */
> > +static bool label_is_sane(const char *label)
> > +{
> > +	for (;*label; label++) {
> > +		if (*label >= 'a' && *label <= 'z')
> > +			continue;
> > +		if (*label >= 'A' && *label <= 'Z')
> > +			continue;
> > +		if (*label >= '0' && *label <= '9')
> > +			continue;
> 
> is isalnum() instead?

I was intimidated by locale interactions of isalnum...

Cheers,
Florian

Re: [PATCH 2/2] conntrack: snprintf: add connlabel format specifier

[Pablo Neira Ayuso]

On Fri, Jun 21, 2013 at 12:31:14AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Jun 18, 2013 at 10:54:22PM +0200, Florian Westphal wrote:
> > > by default, nfct_snprintf will not resolve connlabel names, as they're
> > > system specific. This adds a fmt attribute to resolve names accordingly.
> > > 
> > > output looks like this:
> > > ... mark=0 use=1 labels=eth0-in (0),eth1-in (1)
> > > or
> > > <labels>
> > > <label name="eth0-in" bit="0"/>
> > > <label name="eth1-in" bit="1"/>
> > > </labels>
> > 
> > I think that the bit field is meaningless, so I would just export the
> > strings. I'd use:
> > 
> > <labels>
> > <label>eth0-in</label>
> > <label>eth1-in</label>
> > <labels>
> > 
> > I'd rather use elements, attributes are not easily extensible IMO.
> 
> Okay, I'll change it accordingly.  In case translation fails i'll put
> the number in there, so you'd get <label>(42)</label> if bit 42 is set
> but has no mapping entry.

I think we should not show it in case no mapping file is found. It
would be similar to the unclear usecase we discussed in the previous
patch.

> > > Major stupidity:
> > > As names can be anything, we need to be careful, especially when
> > > creating XML output. Only alphanumerical label names are printed at
> > > this time.
> > 
> > Hm, the string is enclosed between commas, isn't that enough to avoid
> > troubles?
> 
> What if connlabel.conf contains line like
> 0  foo<bar>&;!"

OK, let's just stick to alphanum for simplicity, that's so insane that
we'll make a better world if we prevent people using that ;-)

> > >  a) the chosen xml representation
> > >  b) wheter it makes sense to limit valid charactes in the label
> > >     names this way (e.g., should we care? should be disallow that
> > >     in iptables as well, etc).
> > >  c) if this patch makes sense in the first place, e.g, can't use
> > >      alternate mapping file for labels with nfct_snprintf
> > 
> > Regarding c) I think it makes sense. You can add some
> > nfct_set_connlabel_file interface to set where to find it, it's a bit
> > cheating but should resolve the issue.
> 
> Yuck ;-)
> 
> Lets ignore it for now, adding that would break thread-safety.
> I guess one alternative would be to add
> 
> nfct_labelmap_snprintf(char *buf,
> 	       	       size_t sz,
> 		       const struct nfct_labelmap *,
> 		       const struct nfct_bitmask *, int flags);
> 
> When a use case presents itself.

That's fine with me. The first use case would be the conntrack
utility, which should output the connlabel strings in they are being
used and the mapping file is available.

> > > +/*
> > > + * Labels can have any name, there are no restrictions.
> > > + * We will only print alpha numerical ones; else parsers
> > > + * could choke when putinng "&>" and friends into output
> > > + * (especially when using XML format).  ASCII machines only.
> > > + */
> > > +static bool label_is_sane(const char *label)
> > > +{
> > > +	for (;*label; label++) {
> > > +		if (*label >= 'a' && *label <= 'z')
> > > +			continue;
> > > +		if (*label >= 'A' && *label <= 'Z')
> > > +			continue;
> > > +		if (*label >= '0' && *label <= '9')
> > > +			continue;
> > 
> > is isalnum() instead?
> 
> I was intimidated by locale interactions of isalnum...

Ah, then just add a comment, it should be fine.

Re: [PATCH lnfct-ct 1/2] conntrack: snprintf: add labels in hex representation by default

[Pablo Neira Ayuso]

On Tue, Jun 18, 2013 at 10:54:21PM +0200, Florian Westphal wrote:
> udp      17 24 [..] dport=62277 mark=0 use=1 labels=0x28000000000000008
> or
> [..]<mark>0</mark><labels>0x28000000000000008</labels> [..] </meta></flow>
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  I'm interested if there are any objections on putting
>  this in by default, one alternative would be to toss this and only
>  print label info with special formating request (see next patch,
>  which adds NFCT_OF_ specifier (which will then print the resovled names
>  instead of bitmask).

I'm not sure about the utility of this without the translation to
strings.

Re: [PATCH lnfct-ct 1/2] conntrack: snprintf: add labels in hex representation by default

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I'm not sure about the utility of this without the translation to
> strings.

OK, no objections.  Lets just drop it until a use case shows up.

Re: [PATCH lnfct-ct 1/2] conntrack: snprintf: add labels in hex representation by default

[Pablo Neira Ayuso]

On Fri, Jun 21, 2013 at 12:20:58AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I'm not sure about the utility of this without the translation to
> > strings.
> 
> OK, no objections.  Lets just drop it until a use case shows up.

Thanks. In my experience, it's just better not to add a new interface
whose use case is unclear rather than adding it and wait for people to
(ab)use it.