[nft PATCH] nftables: add flag for nft context to avoid blocking getaddrinfo()

[nft PATCH] nftables: add flag for nft context to avoid blocking getaddrinfo()

[Thomas Haller]

getaddrinfo() blocks while trying to resolve the name. Blocking the
caller of the library is bad in some cases. Especially, while
reconfiguring the firewall, it's not clear that we can access the
network to resolve names.

Add a way to opt out from getaddrinfo() and only accept plain IP addresses.

The opt-out is per nft_ctx instance and cannot be changed after the
context is created. I think that is sufficient.

We could also use AI_NUMERICHOST and getaddrinfo() instead of
inet_pton(). But it seems we can do a better job of generating an error
message, when we try to parse via inet_pton(). Then our error message
can clearly indicate that the string is not a valid IP address.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/datatype.h             |  1 +
 include/nftables/libnftables.h |  1 +
 py/nftables.py                 | 12 +++++-
 src/datatype.c                 | 68 ++++++++++++++++++++--------------
 src/evaluate.c                 | 16 +++++++-
 5 files changed, 66 insertions(+), 32 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index 4b59790b67f9..108bf03ad0ed 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -182,6 +182,7 @@ struct datatype *dtype_clone(const struct datatype *orig_dtype);
 
 struct parse_ctx {
 	struct symbol_tables	*tbl;
+	bool			no_block;
 };
 
 extern struct error_record *symbol_parse(struct parse_ctx *ctx,
diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
index 85e08c9bc98b..d75aff05dec8 100644
--- a/include/nftables/libnftables.h
+++ b/include/nftables/libnftables.h
@@ -34,6 +34,7 @@ enum nft_debug_level {
  * Possible flags to pass to nft_ctx_new()
  */
 #define NFT_CTX_DEFAULT		0
+#define NFT_CTX_NO_BLOCK	1
 
 struct nft_ctx *nft_ctx_new(uint32_t flags);
 void nft_ctx_free(struct nft_ctx *ctx);
diff --git a/py/nftables.py b/py/nftables.py
index 6daeafc231f4..59798b1ecf0c 100644
--- a/py/nftables.py
+++ b/py/nftables.py
@@ -62,9 +62,13 @@ class Nftables:
         "terse":          (1 << 11),
     }
 
+    # Allow to easily indicate to the caller, whether Nftables() supports
+    # the recently added "no_block" argument.
+    _supports_no_block = True
+
     validator = None
 
-    def __init__(self, sofile="libnftables.so.1"):
+    def __init__(self, sofile="libnftables.so.1", no_block=False):
         """Instantiate a new Nftables class object.
 
         Accepts a shared object file to open, by default standard search path
@@ -144,8 +148,12 @@ class Nftables:
         self.nft_ctx_free = lib.nft_ctx_free
         lib.nft_ctx_free.argtypes = [c_void_p]
 
+        ctx_flags = 0  # NFT_CTX_DEFAULT
+        if no_block:
+            ctx_flags |= 1  # NFT_CTX_NO_BLOCK
+
         # initialize libnftables context
-        self.__ctx = self.nft_ctx_new(0)
+        self.__ctx = self.nft_ctx_new(ctx_flags)
         self.nft_ctx_buffer_output(self.__ctx)
         self.nft_ctx_buffer_error(self.__ctx)
 
diff --git a/src/datatype.c b/src/datatype.c
index da802a18bccd..4ceae14f4bd4 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -599,27 +599,33 @@ static struct error_record *ipaddr_type_parse(struct parse_ctx *ctx,
 					      const struct expr *sym,
 					      struct expr **res)
 {
-	struct addrinfo *ai, hints = { .ai_family = AF_INET,
-				       .ai_socktype = SOCK_DGRAM};
-	struct in_addr *addr;
-	int err;
+	struct in_addr addr;
 
-	err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
-	if (err != 0)
-		return error(&sym->location, "Could not resolve hostname: %s",
-			     gai_strerror(err));
+	if (ctx->no_block) {
+		if (inet_pton(AF_INET, sym->identifier, &addr) != 1)
+			return error(&sym->location, "Invalid IPv4 address");
+	} else {
+		struct addrinfo *ai, hints = { .ai_family = AF_INET,
+					       .ai_socktype = SOCK_DGRAM};
+		int err;
 
-	if (ai->ai_next != NULL) {
+		err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
+		if (err != 0)
+			return error(&sym->location, "Could not resolve hostname: %s",
+				     gai_strerror(err));
+
+		if (ai->ai_next != NULL) {
+			freeaddrinfo(ai);
+			return error(&sym->location,
+				     "Hostname resolves to multiple addresses");
+		}
+		addr = ((struct sockaddr_in *)ai->ai_addr)->sin_addr;
 		freeaddrinfo(ai);
-		return error(&sym->location,
-			     "Hostname resolves to multiple addresses");
 	}
 
-	addr = &((struct sockaddr_in *)ai->ai_addr)->sin_addr;
 	*res = constant_expr_alloc(&sym->location, &ipaddr_type,
 				   BYTEORDER_BIG_ENDIAN,
-				   sizeof(*addr) * BITS_PER_BYTE, addr);
-	freeaddrinfo(ai);
+				   sizeof(addr) * BITS_PER_BYTE, &addr);
 	return NULL;
 }
 
@@ -658,27 +664,33 @@ static struct error_record *ip6addr_type_parse(struct parse_ctx *ctx,
 					       const struct expr *sym,
 					       struct expr **res)
 {
-	struct addrinfo *ai, hints = { .ai_family = AF_INET6,
-				       .ai_socktype = SOCK_DGRAM};
-	struct in6_addr *addr;
-	int err;
+	struct in6_addr addr;
 
-	err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
-	if (err != 0)
-		return error(&sym->location, "Could not resolve hostname: %s",
-			     gai_strerror(err));
+	if (ctx->no_block) {
+		if (inet_pton(AF_INET6, sym->identifier, &addr) != 1)
+			return error(&sym->location, "Invalid IPv6 address");
+	} else {
+		struct addrinfo *ai, hints = { .ai_family = AF_INET6,
+					       .ai_socktype = SOCK_DGRAM};
+		int err;
 
-	if (ai->ai_next != NULL) {
+		err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
+		if (err != 0)
+			return error(&sym->location, "Could not resolve hostname: %s",
+				     gai_strerror(err));
+
+		if (ai->ai_next != NULL) {
+			freeaddrinfo(ai);
+			return error(&sym->location,
+				     "Hostname resolves to multiple addresses");
+		}
+		addr = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
 		freeaddrinfo(ai);
-		return error(&sym->location,
-			     "Hostname resolves to multiple addresses");
 	}
 
-	addr = &((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
 	*res = constant_expr_alloc(&sym->location, &ip6addr_type,
 				   BYTEORDER_BIG_ENDIAN,
-				   sizeof(*addr) * BITS_PER_BYTE, addr);
-	freeaddrinfo(ai);
+				   sizeof(addr) * BITS_PER_BYTE, &addr);
 	return NULL;
 }
 
diff --git a/src/evaluate.c b/src/evaluate.c
index 678ad9b8907d..9249e89bec58 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -46,6 +46,14 @@ struct proto_ctx *eval_proto_ctx(struct eval_ctx *ctx)
 	return &ctx->_pctx[idx];
 }
 
+static void parse_ctx_init(struct parse_ctx *parse_ctx, const struct eval_ctx *ctx)
+{
+	*parse_ctx = (struct parse_ctx) {
+		.tbl      = &ctx->nft->output.tbl,
+		.no_block = !!(ctx->nft->flags & NFT_CTX_NO_BLOCK),
+	};
+}
+
 static int expr_evaluate(struct eval_ctx *ctx, struct expr **expr);
 
 static const char * const byteorder_names[] = {
@@ -277,12 +285,14 @@ static int flowtable_not_found(struct eval_ctx *ctx, const struct location *loc,
  */
 static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 {
-	struct parse_ctx parse_ctx = { .tbl = &ctx->nft->output.tbl, };
+	struct parse_ctx parse_ctx;
 	struct error_record *erec;
 	struct table *table;
 	struct set *set;
 	struct expr *new;
 
+	parse_ctx_init(&parse_ctx, ctx);
+
 	switch ((*expr)->symtype) {
 	case SYMBOL_VALUE:
 		datatype_set(*expr, ctx->ectx.dtype);
@@ -3432,10 +3442,12 @@ static int stmt_evaluate_reject_default(struct eval_ctx *ctx,
 
 static int stmt_evaluate_reject_icmp(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	struct parse_ctx parse_ctx = { .tbl = &ctx->nft->output.tbl, };
+	struct parse_ctx parse_ctx;
 	struct error_record *erec;
 	struct expr *code;
 
+	parse_ctx_init(&parse_ctx, ctx);
+
 	erec = symbol_parse(&parse_ctx, stmt->reject.expr, &code);
 	if (erec != NULL) {
 		erec_queue(erec, ctx->msgs);
-- 
2.41.0

Re: [nft PATCH] nftables: add flag for nft context to avoid blocking getaddrinfo()

[Pablo Neira Ayuso]

On Mon, Jul 10, 2023 at 07:46:30PM +0200, Thomas Haller wrote:
> getaddrinfo() blocks while trying to resolve the name. Blocking the
> caller of the library is bad in some cases. Especially, while
> reconfiguring the firewall, it's not clear that we can access the
> network to resolve names.
> 
> Add a way to opt out from getaddrinfo() and only accept plain IP addresses.
> 
> The opt-out is per nft_ctx instance and cannot be changed after the
> context is created. I think that is sufficient.
> 
> We could also use AI_NUMERICHOST and getaddrinfo() instead of
> inet_pton(). But it seems we can do a better job of generating an error
> message, when we try to parse via inet_pton(). Then our error message
> can clearly indicate that the string is not a valid IP address.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/datatype.h             |  1 +
>  include/nftables/libnftables.h |  1 +
>  py/nftables.py                 | 12 +++++-
>  src/datatype.c                 | 68 ++++++++++++++++++++--------------
>  src/evaluate.c                 | 16 +++++++-
>  5 files changed, 66 insertions(+), 32 deletions(-)
> 
> diff --git a/include/datatype.h b/include/datatype.h
> index 4b59790b67f9..108bf03ad0ed 100644
> --- a/include/datatype.h
> +++ b/include/datatype.h
> @@ -182,6 +182,7 @@ struct datatype *dtype_clone(const struct datatype *orig_dtype);
>  
>  struct parse_ctx {
>  	struct symbol_tables	*tbl;
> +	bool			no_block;
>  };
>  
>  extern struct error_record *symbol_parse(struct parse_ctx *ctx,
> diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
> index 85e08c9bc98b..d75aff05dec8 100644
> --- a/include/nftables/libnftables.h
> +++ b/include/nftables/libnftables.h
> @@ -34,6 +34,7 @@ enum nft_debug_level {
>   * Possible flags to pass to nft_ctx_new()
>   */
>  #define NFT_CTX_DEFAULT		0
> +#define NFT_CTX_NO_BLOCK	1

Could you add this flag instead?

        NFT_CTX_INPUT_NO_DNS

there are NFT_CTX_OUTPUT_* flags already in place that determine how
the output is done, but better not to (ab)use them.

And add:

        nft_ctx_input_set_flags(...)

to allow users to set it on.

>  struct nft_ctx *nft_ctx_new(uint32_t flags);
>  void nft_ctx_free(struct nft_ctx *ctx);

[nft v2 PATCH 1/3] nftables: add input flags for nft_ctx

[Thomas Haller]

Similar to the existing output flags, add input flags. No flags are yet
implemented, that will follow.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 doc/libnftables.adoc           | 12 ++++++++++++
 include/nftables.h             |  5 +++++
 include/nftables/libnftables.h |  3 +++
 py/nftables.py                 |  7 +++++++
 src/libnftables.c              | 12 ++++++++++++
 src/libnftables.map            |  5 +++++
 6 files changed, 44 insertions(+)

diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
index 7ea0d56e9b1d..96a580469ee0 100644
--- a/doc/libnftables.adoc
+++ b/doc/libnftables.adoc
@@ -18,6 +18,9 @@ void nft_ctx_free(struct nft_ctx* '\*ctx'*);
 bool nft_ctx_get_dry_run(struct nft_ctx* '\*ctx'*);
 void nft_ctx_set_dry_run(struct nft_ctx* '\*ctx'*, bool* 'dry'*);
 
+unsigned int nft_ctx_input_get_flags(struct nft_ctx* '\*ctx'*);
+void nft_ctx_input_set_flags(struct nft_ctx* '\*ctx'*, unsigned int* 'flags'*);
+
 unsigned int nft_ctx_output_get_flags(struct nft_ctx* '\*ctx'*);
 void nft_ctx_output_set_flags(struct nft_ctx* '\*ctx'*, unsigned int* 'flags'*);
 
@@ -78,6 +81,15 @@ The *nft_ctx_get_dry_run*() function returns the dry-run setting's value contain
 
 The *nft_ctx_set_dry_run*() function sets the dry-run setting in 'ctx' to the value of 'dry'.
 
+=== nft_ctx_input_get_flags() and nft_ctx_input_set_flags()
+The flags setting controls the input format.
+
+Currently not flags are implemented.
+
+The *nft_ctx_input_get_flags*() function returns the input flags setting's value in 'ctx'.
+
+The *nft_ctx_input_set_flags*() function sets the input flags setting in 'ctx' to the value of 'val'.
+
 === nft_ctx_output_get_flags() and nft_ctx_output_set_flags()
 The flags setting controls the output format.
 
diff --git a/include/nftables.h b/include/nftables.h
index d49eb579dc04..7d35a95a89de 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -23,6 +23,10 @@ struct symbol_tables {
 	const struct symbol_table	*realm;
 };
 
+struct input_ctx {
+	unsigned int flags;
+};
+
 struct output_ctx {
 	unsigned int flags;
 	union {
@@ -119,6 +123,7 @@ struct nft_ctx {
 	unsigned int		num_vars;
 	unsigned int		parser_max_errors;
 	unsigned int		debug_mask;
+	struct input_ctx	input;
 	struct output_ctx	output;
 	bool			check;
 	struct nft_cache	cache;
diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
index 85e08c9bc98b..7fb621be1f12 100644
--- a/include/nftables/libnftables.h
+++ b/include/nftables/libnftables.h
@@ -48,6 +48,9 @@ enum nft_optimize_flags {
 uint32_t nft_ctx_get_optimize(struct nft_ctx *ctx);
 void nft_ctx_set_optimize(struct nft_ctx *ctx, uint32_t flags);
 
+unsigned int nft_ctx_input_get_flags(struct nft_ctx *ctx);
+void nft_ctx_input_set_flags(struct nft_ctx *ctx, unsigned int flags);
+
 enum {
 	NFT_CTX_OUTPUT_REVERSEDNS	= (1 << 0),
 	NFT_CTX_OUTPUT_SERVICE		= (1 << 1),
diff --git a/py/nftables.py b/py/nftables.py
index 6daeafc231f4..b9fa63bb8789 100644
--- a/py/nftables.py
+++ b/py/nftables.py
@@ -82,6 +82,13 @@ class Nftables:
         self.nft_ctx_new.restype = c_void_p
         self.nft_ctx_new.argtypes = [c_int]
 
+        self.nft_ctx_input_get_flags = lib.nft_ctx_input_get_flags
+        self.nft_ctx_input_get_flags.restype = c_uint
+        self.nft_ctx_input_get_flags.argtypes = [c_void_p]
+
+        self.nft_ctx_input_set_flags = lib.nft_ctx_input_set_flags
+        self.nft_ctx_input_set_flags.argtypes = [c_void_p, c_uint]
+
         self.nft_ctx_output_get_flags = lib.nft_ctx_output_get_flags
         self.nft_ctx_output_get_flags.restype = c_uint
         self.nft_ctx_output_get_flags.argtypes = [c_void_p]
diff --git a/src/libnftables.c b/src/libnftables.c
index 6fc4f7db6760..6832f0486d6d 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -401,6 +401,18 @@ void nft_ctx_set_optimize(struct nft_ctx *ctx, uint32_t flags)
 	ctx->optimize_flags = flags;
 }
 
+EXPORT_SYMBOL(nft_ctx_input_get_flags);
+unsigned int nft_ctx_input_get_flags(struct nft_ctx *ctx)
+{
+	return ctx->input.flags;
+}
+
+EXPORT_SYMBOL(nft_ctx_input_set_flags);
+void nft_ctx_input_set_flags(struct nft_ctx *ctx, unsigned int flags)
+{
+	ctx->input.flags = flags;
+}
+
 EXPORT_SYMBOL(nft_ctx_output_get_flags);
 unsigned int nft_ctx_output_get_flags(struct nft_ctx *ctx)
 {
diff --git a/src/libnftables.map b/src/libnftables.map
index a46a3ad53ff6..9369f44f3536 100644
--- a/src/libnftables.map
+++ b/src/libnftables.map
@@ -33,3 +33,8 @@ LIBNFTABLES_3 {
   nft_ctx_set_optimize;
   nft_ctx_get_optimize;
 } LIBNFTABLES_2;
+
+LIBNFTABLES_4 {
+  nft_ctx_input_get_flags;
+  nft_ctx_input_set_flags;
+} LIBNFTABLES_3;
-- 
2.41.0

[nft v2 PATCH 2/3] nftables: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking getaddrinfo()

[Thomas Haller]

getaddrinfo() blocks while trying to resolve the name. Blocking the
caller of the library is in many cases undesirable. Also, while
reconfiguring the firewall, it's not clear that resolving names via
the network works or makes sense.

Add a new input flag NFT_CTX_INPUT_NO_DNS to opt-out from getaddrinfo()
and only accept plain IP addresses.

We could also use AI_NUMERICHOST with getaddrinfo() instead of
inet_pton(). But it seems we can do a better job of generating an error
message, when we try to parse via inet_pton(). Then our error message
can clearly indicate that the string is not a valid IP address.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 doc/libnftables.adoc           | 10 ++++-
 include/datatype.h             |  1 +
 include/nftables/libnftables.h |  4 ++
 src/datatype.c                 | 68 ++++++++++++++++++++--------------
 src/evaluate.c                 | 16 +++++++-
 5 files changed, 68 insertions(+), 31 deletions(-)

diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
index 96a580469ee0..77f3a0fd5659 100644
--- a/doc/libnftables.adoc
+++ b/doc/libnftables.adoc
@@ -84,7 +84,15 @@ The *nft_ctx_set_dry_run*() function sets the dry-run setting in 'ctx' to the va
 === nft_ctx_input_get_flags() and nft_ctx_input_set_flags()
 The flags setting controls the input format.
 
-Currently not flags are implemented.
+----
+enum {
+        NFT_CTX_INPUT_NO_DNS = (1 << 0),
+};
+----
+
+NFT_CTX_INPUT_NO_DNS::
+	Avoid resolving IP addresses with blocking getaddrinfo(). In that case,
+	only plain IP addresses are accepted.
 
 The *nft_ctx_input_get_flags*() function returns the input flags setting's value in 'ctx'.
 
diff --git a/include/datatype.h b/include/datatype.h
index 4b59790b67f9..be5c6d1b4011 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -182,6 +182,7 @@ struct datatype *dtype_clone(const struct datatype *orig_dtype);
 
 struct parse_ctx {
 	struct symbol_tables	*tbl;
+	const struct input_ctx	*input;
 };
 
 extern struct error_record *symbol_parse(struct parse_ctx *ctx,
diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
index 7fb621be1f12..2f5f079efff0 100644
--- a/include/nftables/libnftables.h
+++ b/include/nftables/libnftables.h
@@ -48,6 +48,10 @@ enum nft_optimize_flags {
 uint32_t nft_ctx_get_optimize(struct nft_ctx *ctx);
 void nft_ctx_set_optimize(struct nft_ctx *ctx, uint32_t flags);
 
+enum {
+	NFT_CTX_INPUT_NO_DNS		= (1 << 0),
+};
+
 unsigned int nft_ctx_input_get_flags(struct nft_ctx *ctx);
 void nft_ctx_input_set_flags(struct nft_ctx *ctx, unsigned int flags);
 
diff --git a/src/datatype.c b/src/datatype.c
index da802a18bccd..8629a38da56a 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -599,27 +599,33 @@ static struct error_record *ipaddr_type_parse(struct parse_ctx *ctx,
 					      const struct expr *sym,
 					      struct expr **res)
 {
-	struct addrinfo *ai, hints = { .ai_family = AF_INET,
-				       .ai_socktype = SOCK_DGRAM};
-	struct in_addr *addr;
-	int err;
+	struct in_addr addr;
 
-	err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
-	if (err != 0)
-		return error(&sym->location, "Could not resolve hostname: %s",
-			     gai_strerror(err));
+	if (ctx->input->flags & NFT_CTX_INPUT_NO_DNS) {
+		if (inet_pton(AF_INET, sym->identifier, &addr) != 1)
+			return error(&sym->location, "Invalid IPv4 address");
+	} else {
+		struct addrinfo *ai, hints = { .ai_family = AF_INET,
+					       .ai_socktype = SOCK_DGRAM};
+		int err;
 
-	if (ai->ai_next != NULL) {
+		err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
+		if (err != 0)
+			return error(&sym->location, "Could not resolve hostname: %s",
+				     gai_strerror(err));
+
+		if (ai->ai_next != NULL) {
+			freeaddrinfo(ai);
+			return error(&sym->location,
+				     "Hostname resolves to multiple addresses");
+		}
+		addr = ((struct sockaddr_in *)ai->ai_addr)->sin_addr;
 		freeaddrinfo(ai);
-		return error(&sym->location,
-			     "Hostname resolves to multiple addresses");
 	}
 
-	addr = &((struct sockaddr_in *)ai->ai_addr)->sin_addr;
 	*res = constant_expr_alloc(&sym->location, &ipaddr_type,
 				   BYTEORDER_BIG_ENDIAN,
-				   sizeof(*addr) * BITS_PER_BYTE, addr);
-	freeaddrinfo(ai);
+				   sizeof(addr) * BITS_PER_BYTE, &addr);
 	return NULL;
 }
 
@@ -658,27 +664,33 @@ static struct error_record *ip6addr_type_parse(struct parse_ctx *ctx,
 					       const struct expr *sym,
 					       struct expr **res)
 {
-	struct addrinfo *ai, hints = { .ai_family = AF_INET6,
-				       .ai_socktype = SOCK_DGRAM};
-	struct in6_addr *addr;
-	int err;
+	struct in6_addr addr;
 
-	err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
-	if (err != 0)
-		return error(&sym->location, "Could not resolve hostname: %s",
-			     gai_strerror(err));
+	if (ctx->input->flags & NFT_CTX_INPUT_NO_DNS) {
+		if (inet_pton(AF_INET6, sym->identifier, &addr) != 1)
+			return error(&sym->location, "Invalid IPv6 address");
+	} else {
+		struct addrinfo *ai, hints = { .ai_family = AF_INET6,
+					       .ai_socktype = SOCK_DGRAM};
+		int err;
 
-	if (ai->ai_next != NULL) {
+		err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
+		if (err != 0)
+			return error(&sym->location, "Could not resolve hostname: %s",
+				     gai_strerror(err));
+
+		if (ai->ai_next != NULL) {
+			freeaddrinfo(ai);
+			return error(&sym->location,
+				     "Hostname resolves to multiple addresses");
+		}
+		addr = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
 		freeaddrinfo(ai);
-		return error(&sym->location,
-			     "Hostname resolves to multiple addresses");
 	}
 
-	addr = &((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
 	*res = constant_expr_alloc(&sym->location, &ip6addr_type,
 				   BYTEORDER_BIG_ENDIAN,
-				   sizeof(*addr) * BITS_PER_BYTE, addr);
-	freeaddrinfo(ai);
+				   sizeof(addr) * BITS_PER_BYTE, &addr);
 	return NULL;
 }
 
diff --git a/src/evaluate.c b/src/evaluate.c
index 33e4ac93e89a..a904714acd48 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -46,6 +46,14 @@ struct proto_ctx *eval_proto_ctx(struct eval_ctx *ctx)
 	return &ctx->_pctx[idx];
 }
 
+static void parse_ctx_init(struct parse_ctx *parse_ctx, const struct eval_ctx *ctx)
+{
+	*parse_ctx = (struct parse_ctx) {
+		.tbl	= &ctx->nft->output.tbl,
+		.input	= &ctx->nft->input,
+	};
+}
+
 static int expr_evaluate(struct eval_ctx *ctx, struct expr **expr);
 
 static const char * const byteorder_names[] = {
@@ -277,12 +285,14 @@ static int flowtable_not_found(struct eval_ctx *ctx, const struct location *loc,
  */
 static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 {
-	struct parse_ctx parse_ctx = { .tbl = &ctx->nft->output.tbl, };
+	struct parse_ctx parse_ctx;
 	struct error_record *erec;
 	struct table *table;
 	struct set *set;
 	struct expr *new;
 
+	parse_ctx_init(&parse_ctx, ctx);
+
 	switch ((*expr)->symtype) {
 	case SYMBOL_VALUE:
 		datatype_set(*expr, ctx->ectx.dtype);
@@ -3432,10 +3442,12 @@ static int stmt_evaluate_reject_default(struct eval_ctx *ctx,
 
 static int stmt_evaluate_reject_icmp(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	struct parse_ctx parse_ctx = { .tbl = &ctx->nft->output.tbl, };
+	struct parse_ctx parse_ctx;
 	struct error_record *erec;
 	struct expr *code;
 
+	parse_ctx_init(&parse_ctx, ctx);
+
 	erec = symbol_parse(&parse_ctx, stmt->reject.expr, &code);
 	if (erec != NULL) {
 		erec_queue(erec, ctx->msgs);
-- 
2.41.0

[nft v2 PATCH 3/3] py: add input_{set,get}_flags() API to helpers

[Thomas Haller]

Note that the corresponding API for output flags does not expose the
plain numeric flags. Instead, it exposes the underlying, flag-based C
API more directly.

Reasons:

- a flags property has the benefits that adding new flags is very light
  weight. Otherwise, every addition of a flag requires new API. That new
  API increases the documentation and what the user needs to understand.
  With a flag API, we just need new documentation what the new flag is.
  It's already clear how to use it.

- opinionated, also the usage of "many getter/setter API" is not have
  better usability. Its convenient when we can do similar things (setting
  a boolean flag) depending on an argument of a function, instead of
  having different functions.

  Compare

     ctx.set_reversedns_output(True)
     ctx.set_handle_output(True)

  with

     ctx.ouput_set_flags(NFT_CTX_OUTPUT_REVERSEDNS | NFT_CTX_OUTPUT_HANDLE)

  Note that the vast majority of users of this API will just create one
  nft_ctx instance and set the flags once. Each user application
  probably has only one place where they call the setter once. So
  while I think flags have better usability, it doesn't matter much
  either way.

- if individual properties are preferable over flags, then the C API
  should also do that. In other words, the Python API should be similar
  to the underlying C API.

- I don't understand how to do this best. Is Nftables.output_flags
  public API? It appears to be, as it has no underscore. Why does this
  additional mapping from function (get_reversedns_output()) to name
  ("reversedns") to number (1<<0) exist?

Downside is the inconsistency with the existing output flags API.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---

This is probably a controversial approach :)

 py/nftables.py | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/py/nftables.py b/py/nftables.py
index b9fa63bb8789..795700db45ef 100644
--- a/py/nftables.py
+++ b/py/nftables.py
@@ -21,6 +21,16 @@ import os
 
 NFTABLES_VERSION = "0.1"
 
+"""Prevent blocking DNS lookups for IP addresses.
+
+By default, nftables will try to resolve IP addresses with blocking getaddrinfo() API.
+By setting this flag, only literal IP adddresses are supported in input.
+
+This numeric flag can be passed to Nftables.input_get_flags() and Nftables.input_set_flags().
+"""
+NFT_CTX_INPUT_NO_DNS = 1
+
+
 class SchemaValidator:
     """Libnftables JSON validator using jsonschema"""
 
@@ -159,6 +169,27 @@ class Nftables:
     def __del__(self):
         self.nft_ctx_free(self.__ctx)
 
+    def input_get_flags(self):
+        """Query input flags for the nft context.
+
+        See input_get_flags() for supported flags.
+
+        Returns the currently set input flags as number.
+        """
+        return self.nft_ctx_input_get_flags(self.__ctx)
+
+    def input_set_flags(self, flags):
+        """Set input flags for the nft context as number.
+
+        By default, a new context objects has no flags set.
+
+        Supported flags are NFT_CTX_INPUT_NO_DNS (0x1) to disable blocking address
+        lookup via getaddrinfo.
+
+        Returns nothing.
+        """
+        self.nft_ctx_input_set_flags(self.__ctx, flags)
+
     def __get_output_flag(self, name):
         flag = self.output_flags[name]
         return self.nft_ctx_output_get_flags(self.__ctx) & flag
-- 
2.41.0

Re: [nft v2 PATCH 3/3] py: add input_{set,get}_flags() API to helpers

[Phil Sutter]

Hi Thomas,

On Fri, Jul 14, 2023 at 10:48:53AM +0200, Thomas Haller wrote:
> Note that the corresponding API for output flags does not expose the
> plain numeric flags. Instead, it exposes the underlying, flag-based C
> API more directly.
> 
> Reasons:
> 
> - a flags property has the benefits that adding new flags is very light
>   weight. Otherwise, every addition of a flag requires new API. That new
>   API increases the documentation and what the user needs to understand.
>   With a flag API, we just need new documentation what the new flag is.
>   It's already clear how to use it.
> 
> - opinionated, also the usage of "many getter/setter API" is not have
>   better usability. Its convenient when we can do similar things (setting
>   a boolean flag) depending on an argument of a function, instead of
>   having different functions.
> 
>   Compare
> 
>      ctx.set_reversedns_output(True)
>      ctx.set_handle_output(True)
> 
>   with
> 
>      ctx.ouput_set_flags(NFT_CTX_OUTPUT_REVERSEDNS | NFT_CTX_OUTPUT_HANDLE)
> 
>   Note that the vast majority of users of this API will just create one
>   nft_ctx instance and set the flags once. Each user application
>   probably has only one place where they call the setter once. So
>   while I think flags have better usability, it doesn't matter much
>   either way.
> 
> - if individual properties are preferable over flags, then the C API
>   should also do that. In other words, the Python API should be similar
>   to the underlying C API.
> 
> - I don't understand how to do this best. Is Nftables.output_flags
>   public API? It appears to be, as it has no underscore. Why does this
>   additional mapping from function (get_reversedns_output()) to name
>   ("reversedns") to number (1<<0) exist?

I don't recall why I chose to add setters/getters for individual output
flags instead of expecting users to do bit-fiddling. Maybe the latter is
not as common among Python users. :)

On the other hand, things are a bit inconsistent already, see
set_debug() method. 

Maybe we could turn __{get,set}_output_flag() public and make them
accept an array of strings or numbers just like set_debug()? If you
then adjust your input flag API accordingly, things become consistent
(enough?), without breaking existing users.

FWIW, I find

| ctx.set_output_flags(["reversedns", "stateless"])

nicer than

| ctx.set_output_flags(REVERSEDNS | STATELESS)

at least with a Python hat on. WDYT?

Cheers, Phil

Re: [nft v2 PATCH 2/3] nftables: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking getaddrinfo()

[Phil Sutter]

On Fri, Jul 14, 2023 at 10:48:52AM +0200, Thomas Haller wrote:
> getaddrinfo() blocks while trying to resolve the name. Blocking the
> caller of the library is in many cases undesirable. Also, while
> reconfiguring the firewall, it's not clear that resolving names via
> the network works or makes sense.
> 
> Add a new input flag NFT_CTX_INPUT_NO_DNS to opt-out from getaddrinfo()
> and only accept plain IP addresses.
> 
> We could also use AI_NUMERICHOST with getaddrinfo() instead of
> inet_pton(). But it seems we can do a better job of generating an error
> message, when we try to parse via inet_pton(). Then our error message
> can clearly indicate that the string is not a valid IP address.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  doc/libnftables.adoc           | 10 ++++-
>  include/datatype.h             |  1 +
>  include/nftables/libnftables.h |  4 ++
>  src/datatype.c                 | 68 ++++++++++++++++++++--------------
>  src/evaluate.c                 | 16 +++++++-
>  5 files changed, 68 insertions(+), 31 deletions(-)
> 
> diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
> index 96a580469ee0..77f3a0fd5659 100644
> --- a/doc/libnftables.adoc
> +++ b/doc/libnftables.adoc
> @@ -84,7 +84,15 @@ The *nft_ctx_set_dry_run*() function sets the dry-run setting in 'ctx' to the va
>  === nft_ctx_input_get_flags() and nft_ctx_input_set_flags()
>  The flags setting controls the input format.
>  
> -Currently not flags are implemented.
> +----
> +enum {
> +        NFT_CTX_INPUT_NO_DNS = (1 << 0),
> +};
> +----
> +
> +NFT_CTX_INPUT_NO_DNS::
> +	Avoid resolving IP addresses with blocking getaddrinfo(). In that case,
> +	only plain IP addresses are accepted.
>  
>  The *nft_ctx_input_get_flags*() function returns the input flags setting's value in 'ctx'.
>  
> diff --git a/include/datatype.h b/include/datatype.h
> index 4b59790b67f9..be5c6d1b4011 100644
> --- a/include/datatype.h
> +++ b/include/datatype.h
> @@ -182,6 +182,7 @@ struct datatype *dtype_clone(const struct datatype *orig_dtype);
>  
>  struct parse_ctx {
>  	struct symbol_tables	*tbl;
> +	const struct input_ctx	*input;
>  };
>  
>  extern struct error_record *symbol_parse(struct parse_ctx *ctx,
> diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
> index 7fb621be1f12..2f5f079efff0 100644
> --- a/include/nftables/libnftables.h
> +++ b/include/nftables/libnftables.h
> @@ -48,6 +48,10 @@ enum nft_optimize_flags {
>  uint32_t nft_ctx_get_optimize(struct nft_ctx *ctx);
>  void nft_ctx_set_optimize(struct nft_ctx *ctx, uint32_t flags);
>  
> +enum {
> +	NFT_CTX_INPUT_NO_DNS		= (1 << 0),
> +};
> +
>  unsigned int nft_ctx_input_get_flags(struct nft_ctx *ctx);
>  void nft_ctx_input_set_flags(struct nft_ctx *ctx, unsigned int flags);
>  
> diff --git a/src/datatype.c b/src/datatype.c
> index da802a18bccd..8629a38da56a 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -599,27 +599,33 @@ static struct error_record *ipaddr_type_parse(struct parse_ctx *ctx,
>  					      const struct expr *sym,
>  					      struct expr **res)
>  {
> -	struct addrinfo *ai, hints = { .ai_family = AF_INET,
> -				       .ai_socktype = SOCK_DGRAM};
> -	struct in_addr *addr;
> -	int err;
> +	struct in_addr addr;
>  
> -	err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
> -	if (err != 0)
> -		return error(&sym->location, "Could not resolve hostname: %s",
> -			     gai_strerror(err));
> +	if (ctx->input->flags & NFT_CTX_INPUT_NO_DNS) {
> +		if (inet_pton(AF_INET, sym->identifier, &addr) != 1)
> +			return error(&sym->location, "Invalid IPv4 address");
> +	} else {
> +		struct addrinfo *ai, hints = { .ai_family = AF_INET,
> +					       .ai_socktype = SOCK_DGRAM};
> +		int err;
>  
> -	if (ai->ai_next != NULL) {
> +		err = getaddrinfo(sym->identifier, NULL, &hints, &ai);
> +		if (err != 0)
> +			return error(&sym->location, "Could not resolve hostname: %s",
> +				     gai_strerror(err));
> +
> +		if (ai->ai_next != NULL) {
> +			freeaddrinfo(ai);
> +			return error(&sym->location,
> +				     "Hostname resolves to multiple addresses");
> +		}
> +		addr = ((struct sockaddr_in *)ai->ai_addr)->sin_addr;
>  		freeaddrinfo(ai);
> -		return error(&sym->location,
> -			     "Hostname resolves to multiple addresses");
>  	}
>  
> -	addr = &((struct sockaddr_in *)ai->ai_addr)->sin_addr;
>  	*res = constant_expr_alloc(&sym->location, &ipaddr_type,
>  				   BYTEORDER_BIG_ENDIAN,
> -				   sizeof(*addr) * BITS_PER_BYTE, addr);
> -	freeaddrinfo(ai);
> +				   sizeof(addr) * BITS_PER_BYTE, &addr);
>  	return NULL;
>  }

Maybe introduce a common

| struct error_record *do_getaddrinfo(const char *buf, int family, struct in_addr *out);

for both *_type_parse() to call?

[...]
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 33e4ac93e89a..a904714acd48 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -46,6 +46,14 @@ struct proto_ctx *eval_proto_ctx(struct eval_ctx *ctx)
>  	return &ctx->_pctx[idx];
>  }
>  
> +static void parse_ctx_init(struct parse_ctx *parse_ctx, const struct eval_ctx *ctx)
> +{
> +	*parse_ctx = (struct parse_ctx) {
> +		.tbl	= &ctx->nft->output.tbl,
> +		.input	= &ctx->nft->input,
> +	};
> +}

This is interesting coding style, but looks more complicated than

| parse_ctx->tbl	= &ctx->nft->output.tbl;
| parse_ctx->input	= &ctx->nft->input;

though I would just keep the extra assignment inline like so:

> -	struct parse_ctx parse_ctx = { .tbl = &ctx->nft->output.tbl, };
> +	struct parse_ctx parse_ctx = {
> +		.tbl	= &ctx->nft->output.tbl,
> +		.input	= &ctx->nft->input,
> +	};

Cheers, Phil

Re: [nft v2 PATCH 1/3] nftables: add input flags for nft_ctx

[Phil Sutter]

On Fri, Jul 14, 2023 at 10:48:51AM +0200, Thomas Haller wrote:
[...]
> +=== nft_ctx_input_get_flags() and nft_ctx_input_set_flags()
> +The flags setting controls the input format.

Note how we turn on JSON input parsing if JSON output is enabled.

I think we could tidy things up by introducing NFT_CTX_INPUT_JSON and
flip it from nft_ctx_output_set_flags() to match NFT_CTX_OUTPUT_JSON for
compatibility?

Cheers, Phil

Re: [nft v2 PATCH 1/3] nftables: add input flags for nft_ctx

[Thomas Haller]

On Fri, 2023-07-14 at 12:16 +0200, Phil Sutter wrote:
> On Fri, Jul 14, 2023 at 10:48:51AM +0200, Thomas Haller wrote:
> [...]
> > +=== nft_ctx_input_get_flags() and nft_ctx_input_set_flags()
> > +The flags setting controls the input format.
> 
> Note how we turn on JSON input parsing if JSON output is enabled.
> 
> I think we could tidy things up by introducing NFT_CTX_INPUT_JSON and
> flip it from nft_ctx_output_set_flags() to match NFT_CTX_OUTPUT_JSON
> for
> compatibility?


Hi Phil,


The doc says:

doc/libnftables.adoc:NFT_CTX_OUTPUT_JSON::
doc/libnftables.adoc-    If enabled at compile-time, libnftables accepts input in JSON format and is able to print output in JSON format as well.
doc/libnftables.adoc-    See *libnftables-json*(5) for a description of the supported schema.
doc/libnftables.adoc-    This flag controls JSON output format, input is auto-detected.

which is a bit inaccurate, as JSON is auto-detect if-and-only-if
NFT_CTX_OUTPUT_JSON is set.


src/libnftables.c:  if (nft_output_json(&nft->output))
src/libnftables.c-       rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
src/libnftables.c-  if (rc == -EINVAL)
src/libnftables.c-       rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &cmds,
src/libnftables.c-                          &indesc_cmdline);


I think, that toggling the input flag when setting an output flag
should not be done. It's confusing to behave differently depending on
the order in which nft_ctx_output_set_flags() and
nft_ctx_input_set_flags() are called. And it's confusing that setting
output flags would mangle input flags.

And for the sake of backwards compatibility, the current behavior must
be kept anyway. So there is only a place for overruling the current
automatism via some NFT_CTX_INPUT_NO_JSON (aka
NFT_CTX_INOUT_FORCE_BISON) or NFT_CTX_INPUT_FORCE_JSON flags, like

    try_json = TRUE;
    try_bison = TRUE;
    if (nft_ctx_input_get_flags(ctx) & NFT_CTX_INPUT_NO_JSON)
        try_json = FALSE;
    else if (nft_ctx_input_get_flags(ctx) & NFT_CTX_INPUT_FORCE_JSON)
        try_bison = FALSE;
    else if (nft_output_json(&ctx->output)) {
        /* try both, JSON first */
    } else
        try_json = FALSE;

    if (try_json)
        rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
    if (try_bison && (!try_json || rc == -EINVAL))
        rc = nft_parse_bison_buffer(nft, nlbuf, ...);


This would not require to mangle input flags during
nft_ctx_output_set_flags().

 
But I find those two flags unnecessary. Both can be added any time
later when needed. The addition of nft_ctx_input_set_flags() does not
force a resolution now.


Also, depending on the semantics, I don't understand how
NFT_CTX_INPUT_JSON extends the current behavior in a backward
compatible way. The default would be to not have that flag set, which
already means to enable JSON parsing depending on NFT_CTX_OUTPUT_JSON.
If NFT_CTX_INPUT_JSON merely means to explicitly enable JSON input,
then that is already fully configurable today. Having this flag does
not provide something new (unlike NO_JSON/FORCE_BISON or FORCE_JSON
flags would).



Thomas

Re: [nft v2 PATCH 2/3] nftables: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking getaddrinfo()

[Thomas Haller]

On Fri, 2023-07-14 at 12:07 +0200, Phil Sutter wrote:
> On Fri, Jul 14, 2023 at 10:48:52AM +0200, Thomas Haller wrote:
> > 
> > +static void parse_ctx_init(struct parse_ctx *parse_ctx, const
> > struct eval_ctx *ctx)
> > +{
> > +       *parse_ctx = (struct parse_ctx) {
> > +               .tbl    = &ctx->nft->output.tbl,
> > +               .input  = &ctx->nft->input,
> > +       };
> > +}
> 
> This is interesting coding style, but looks more complicated than

> 
> > parse_ctx->tbl  = &ctx->nft->output.tbl;
> > parse_ctx->input        = &ctx->nft->input;
> 
> though I would just keep the extra assignment inline like so:

> 
> > -       struct parse_ctx parse_ctx = { .tbl = &ctx->nft-
> > >output.tbl, };
> > +       struct parse_ctx parse_ctx = {
> > +               .tbl    = &ctx->nft->output.tbl,
> > +               .input  = &ctx->nft->input,
> > +       };
> 
> Cheers, Phil

Hi,

I will address this in an update.

Thomas

Re: [nft v2 PATCH 1/3] nftables: add input flags for nft_ctx

[Phil Sutter]

On Tue, Jul 18, 2023 at 11:05:45AM +0200, Thomas Haller wrote:
> On Fri, 2023-07-14 at 12:16 +0200, Phil Sutter wrote:
> > On Fri, Jul 14, 2023 at 10:48:51AM +0200, Thomas Haller wrote:
> > [...]
> > > +=== nft_ctx_input_get_flags() and nft_ctx_input_set_flags()
> > > +The flags setting controls the input format.
> > 
> > Note how we turn on JSON input parsing if JSON output is enabled.
> > 
> > I think we could tidy things up by introducing NFT_CTX_INPUT_JSON and
> > flip it from nft_ctx_output_set_flags() to match NFT_CTX_OUTPUT_JSON
> > for
> > compatibility?
> 
> The doc says:
> 
> doc/libnftables.adoc:NFT_CTX_OUTPUT_JSON::
> doc/libnftables.adoc-    If enabled at compile-time, libnftables accepts input in JSON format and is able to print output in JSON format as well.
> doc/libnftables.adoc-    See *libnftables-json*(5) for a description of the supported schema.
> doc/libnftables.adoc-    This flag controls JSON output format, input is auto-detected.
> 
> which is a bit inaccurate, as JSON is auto-detect if-and-only-if
> NFT_CTX_OUTPUT_JSON is set.

Yes, I'd even call it incorrect. :)

> src/libnftables.c:  if (nft_output_json(&nft->output))
> src/libnftables.c-       rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
> src/libnftables.c-  if (rc == -EINVAL)
> src/libnftables.c-       rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &cmds,
> src/libnftables.c-                          &indesc_cmdline);
> 
> 
> I think, that toggling the input flag when setting an output flag
> should not be done. It's confusing to behave differently depending on
> the order in which nft_ctx_output_set_flags() and
> nft_ctx_input_set_flags() are called. And it's confusing that setting
> output flags would mangle input flags.

That's a valid point, indeed.

> And for the sake of backwards compatibility, the current behavior must
> be kept anyway. So there is only a place for overruling the current
> automatism via some NFT_CTX_INPUT_NO_JSON (aka
> NFT_CTX_INOUT_FORCE_BISON) or NFT_CTX_INPUT_FORCE_JSON flags, like
> 
>     try_json = TRUE;
>     try_bison = TRUE;
>     if (nft_ctx_input_get_flags(ctx) & NFT_CTX_INPUT_NO_JSON)
>         try_json = FALSE;
>     else if (nft_ctx_input_get_flags(ctx) & NFT_CTX_INPUT_FORCE_JSON)
>         try_bison = FALSE;
>     else if (nft_output_json(&ctx->output)) {
>         /* try both, JSON first */
>     } else
>         try_json = FALSE;
> 
>     if (try_json)
>         rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
>     if (try_bison && (!try_json || rc == -EINVAL))
>         rc = nft_parse_bison_buffer(nft, nlbuf, ...);
> 
> 
> This would not require to mangle input flags during
> nft_ctx_output_set_flags().
> 
>  
> But I find those two flags unnecessary. Both can be added any time
> later when needed. The addition of nft_ctx_input_set_flags() does not
> force a resolution now.
> 
> 
> Also, depending on the semantics, I don't understand how
> NFT_CTX_INPUT_JSON extends the current behavior in a backward
> compatible way. The default would be to not have that flag set, which
> already means to enable JSON parsing depending on NFT_CTX_OUTPUT_JSON.
> If NFT_CTX_INPUT_JSON merely means to explicitly enable JSON input,
> then that is already fully configurable today. Having this flag does
> not provide something new (unlike NO_JSON/FORCE_BISON or FORCE_JSON
> flags would).

The use-case I had in mind was to enable JSON parsing while keeping
standard output. This was possible with setting NFT_CTX_INPUT_JSON and
keeping NFT_CTX_OUTPUT_JSON unset.

The reason for libnftables' odd behaviour probably stems from nft using
just a single flag ('-j') to control JSON "mode" and I wanted to still
support non-JSON input - I tend to (mis-)use it as JSON-translator in
the testsuite and personally. ;)

You're right, we may just leave JSON input/output toggles alone for now.
I also didn't intend to block the patches - giving it a thought (as you
did) is fine from my side.

Thanks, Phil

Re: [nft v2 PATCH 3/3] py: add input_{set,get}_flags() API to helpers

[Thomas Haller]

On Fri, 2023-07-14 at 11:59 +0200, Phil Sutter wrote:
> Hi Thomas,
> 
> On Fri, Jul 14, 2023 at 10:48:53AM +0200, Thomas Haller wrote:
> > Note that the corresponding API for output flags does not expose
> > the
> > plain numeric flags. Instead, it exposes the underlying, flag-based
> > C
> > API more directly.
> > 
> > Reasons:
> > 
> > - a flags property has the benefits that adding new flags is very
> > light
> >   weight. Otherwise, every addition of a flag requires new API.
> > That new
> >   API increases the documentation and what the user needs to
> > understand.
> >   With a flag API, we just need new documentation what the new flag
> > is.
> >   It's already clear how to use it.
> > 
> > - opinionated, also the usage of "many getter/setter API" is not
> > have
> >   better usability. Its convenient when we can do similar things
> > (setting
> >   a boolean flag) depending on an argument of a function, instead
> > of
> >   having different functions.
> > 
> >   Compare
> > 
> >      ctx.set_reversedns_output(True)
> >      ctx.set_handle_output(True)
> > 
> >   with
> > 
> >      ctx.ouput_set_flags(NFT_CTX_OUTPUT_REVERSEDNS |
> > NFT_CTX_OUTPUT_HANDLE)
> > 
> >   Note that the vast majority of users of this API will just create
> > one
> >   nft_ctx instance and set the flags once. Each user application
> >   probably has only one place where they call the setter once. So
> >   while I think flags have better usability, it doesn't matter much
> >   either way.
> > 
> > - if individual properties are preferable over flags, then the C
> > API
> >   should also do that. In other words, the Python API should be
> > similar
> >   to the underlying C API.
> > 
> > - I don't understand how to do this best. Is Nftables.output_flags
> >   public API? It appears to be, as it has no underscore. Why does
> > this
> >   additional mapping from function (get_reversedns_output()) to
> > name
> >   ("reversedns") to number (1<<0) exist?
> 
> I don't recall why I chose to add setters/getters for individual
> output
> flags instead of expecting users to do bit-fiddling. Maybe the latter
> is
> not as common among Python users. :)
> 
> On the other hand, things are a bit inconsistent already, see
> set_debug() method. 
> 
> Maybe we could turn __{get,set}_output_flag() public and make them
> accept an array of strings or numbers just like set_debug()? If you
> then adjust your input flag API accordingly, things become consistent
> (enough?), without breaking existing users.
> 
> FWIW, I find
> 
> > ctx.set_output_flags(["reversedns", "stateless"])
> 
> nicer than
> 
> > ctx.set_output_flags(REVERSEDNS | STATELESS)
> 
> at least with a Python hat on. WDYT?

Hi Phil,


I see set_debug().

So we can do:

   nft.set_debug("netlink")

or

   nft.set_debug(("netlink", "scanner"))

but to me, that is not an improvement over plain

   nft.output_set_debug(nftables.NFT_DEBUG_NETLINK | nftables.NFT_DEBUG_SCANNER)

(which would be a thin layer over the underlying, documented C API).


I like set_debug() better than the __set_output_flag() approach,
because the flags are an argument of one function, instead of multiple
set-flag-xyz() functions. I don't like very much that

  - the "set_debug()" name does not resemble the underlying 
    nft_ctx_output_set_debug() name.
  - it encourages using string literals as arguments (instead of 
    Python constants which I can grep for and find with ctags).
  - it requires extra some code to translate from one domain 
    (the list of names/ints)) to another (plain integer flags), when 
    the user could just as well use the flags directly.

Anyway. I don't really mind either way. I will do whatever we agree
upon.


Thanks,
Thomas

Re: [nft v2 PATCH 1/3] nftables: add input flags for nft_ctx

[Thomas Haller]

On Tue, 2023-07-18 at 11:33 +0200, Phil Sutter wrote:
> On Tue, Jul 18, 2023 at 11:05:45AM +0200, Thomas Haller wrote:
> > On Fri, 2023-07-14 at 12:16 +0200, Phil Sutter wrote:
> > > On Fri, Jul 14, 2023 at 10:48:51AM +0200, Thomas Haller wrote:
> > > [...]
> > > > +=== nft_ctx_input_get_flags() and nft_ctx_input_set_flags()
> > > > +The flags setting controls the input format.
> > > 
> > > Note how we turn on JSON input parsing if JSON output is enabled.
> > > 
> > > I think we could tidy things up by introducing NFT_CTX_INPUT_JSON
> > > and
> > > flip it from nft_ctx_output_set_flags() to match
> > > NFT_CTX_OUTPUT_JSON
> > > for
> > > compatibility?
> > 
> > The doc says:
> > 
> > doc/libnftables.adoc:NFT_CTX_OUTPUT_JSON::
> > doc/libnftables.adoc-    If enabled at compile-time, libnftables
> > accepts input in JSON format and is able to print output in JSON
> > format as well.
> > doc/libnftables.adoc-    See *libnftables-json*(5) for a
> > description of the supported schema.
> > doc/libnftables.adoc-    This flag controls JSON output format,
> > input is auto-detected.
> > 
> > which is a bit inaccurate, as JSON is auto-detect if-and-only-if
> > NFT_CTX_OUTPUT_JSON is set.
> 
> Yes, I'd even call it incorrect. :)
> 
> > src/libnftables.c:  if (nft_output_json(&nft->output))
> > src/libnftables.c-       rc = nft_parse_json_buffer(nft, nlbuf,
> > &msgs, &cmds);
> > src/libnftables.c-  if (rc == -EINVAL)
> > src/libnftables.c-       rc = nft_parse_bison_buffer(nft, nlbuf,
> > &msgs, &cmds,
> > src/libnftables.c-                          &indesc_cmdline);
> > 
> > 
> > I think, that toggling the input flag when setting an output flag
> > should not be done. It's confusing to behave differently depending
> > on
> > the order in which nft_ctx_output_set_flags() and
> > nft_ctx_input_set_flags() are called. And it's confusing that
> > setting
> > output flags would mangle input flags.
> 
> That's a valid point, indeed.
> 
> > And for the sake of backwards compatibility, the current behavior
> > must
> > be kept anyway. So there is only a place for overruling the current
> > automatism via some NFT_CTX_INPUT_NO_JSON (aka
> > NFT_CTX_INOUT_FORCE_BISON) or NFT_CTX_INPUT_FORCE_JSON flags, like
> > 
> >     try_json = TRUE;
> >     try_bison = TRUE;
> >     if (nft_ctx_input_get_flags(ctx) & NFT_CTX_INPUT_NO_JSON)
> >         try_json = FALSE;
> >     else if (nft_ctx_input_get_flags(ctx) &
> > NFT_CTX_INPUT_FORCE_JSON)
> >         try_bison = FALSE;
> >     else if (nft_output_json(&ctx->output)) {
> >         /* try both, JSON first */
> >     } else
> >         try_json = FALSE;
> > 
> >     if (try_json)
> >         rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
> >     if (try_bison && (!try_json || rc == -EINVAL))
> >         rc = nft_parse_bison_buffer(nft, nlbuf, ...);
> > 
> > 
> > This would not require to mangle input flags during
> > nft_ctx_output_set_flags().
> > 
> >  
> > But I find those two flags unnecessary. Both can be added any time
> > later when needed. The addition of nft_ctx_input_set_flags() does
> > not
> > force a resolution now.
> > 
> > 
> > Also, depending on the semantics, I don't understand how
> > NFT_CTX_INPUT_JSON extends the current behavior in a backward
> > compatible way. The default would be to not have that flag set,
> > which
> > already means to enable JSON parsing depending on
> > NFT_CTX_OUTPUT_JSON.
> > If NFT_CTX_INPUT_JSON merely means to explicitly enable JSON input,
> > then that is already fully configurable today. Having this flag
> > does
> > not provide something new (unlike NO_JSON/FORCE_BISON or FORCE_JSON
> > flags would).
> 
> The use-case I had in mind was to enable JSON parsing while keeping
> standard output. This was possible with setting NFT_CTX_INPUT_JSON
> and
> keeping NFT_CTX_OUTPUT_JSON unset.

you are right. A NFT_CTX_INPUT_JSON (or TRY_JSON?) flag makes sense
beside NO_JSON/FORCE_BISON and FORCE_JSON/ALWAYS_JSON to enable to try
both.

> 
> The reason for libnftables' odd behaviour probably stems from nft
> using
> just a single flag ('-j') to control JSON "mode" and I wanted to
> still
> support non-JSON input - I tend to (mis-)use it as JSON-translator in
> the testsuite and personally. ;)
> 
> You're right, we may just leave JSON input/output toggles alone for
> now.
> I also didn't intend to block the patches - giving it a thought (as
> you
> did) is fine from my side.


makes sense. But doesn't block the addition of
nft_ctx_input_set_flags(), because I would not try to automatically add
the NFT_CTX_INPUT_JSON flag (based on the output flag). Setting the
input flag should still be an explicit user action, so the flag can be
added any time later.


Thomas