[nft PATCH v4 0/6] add input flags and "no-dns"/"json" flags

[nft PATCH v4 0/6] add input flags and "no-dns"/"json" flags

[Thomas Haller]

Changes since v3:

- set-input() now returns the old value (both for Python and C API)
- python: API follows the style of existing set_debug()/get_debug()
  methods.
- nft_input_no_dns()/nft_input_json() helper functions added and used.
- python: new patch to better handle exception while creating Nftables
  instance.

Thomas Haller (6):
  src: add input flags for nft_ctx
  src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking
  src: add input flag NFT_CTX_INPUT_JSON to enable JSON parsing
  py: fix exception during cleanup of half-initialized Nftables
  py: extract flags helper functions for set_debug()/get_debug()
  py: add Nftables.{get,set}_input() API

 doc/libnftables.adoc           |  30 +++++++++-
 include/datatype.h             |   1 +
 include/nftables.h             |  15 +++++
 include/nftables/libnftables.h |   8 +++
 py/src/nftables.py             | 101 ++++++++++++++++++++++++++-------
 src/datatype.c                 |  68 +++++++++++++---------
 src/evaluate.c                 |  10 +++-
 src/libnftables.c              |  20 ++++++-
 src/libnftables.map            |   5 ++
 9 files changed, 203 insertions(+), 55 deletions(-)

-- 
2.41.0

[nft PATCH v4 1/6] src: 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.

One difference to nft_ctx_output_set_flags(), is that the setter for
input flags returns the previously set flags.

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

diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
index 7ea0d56e9b1d..a0d3521e5e7a 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'*);
+unsigned int 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,16 @@ 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 no 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'
+and returns the previous flags.
+
 === 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..9a05d3c4b90d 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);
+unsigned int 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/src/libnftables.c b/src/libnftables.c
index e214abb69cf2..17438b5330cb 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -401,6 +401,22 @@ 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);
+unsigned int nft_ctx_input_set_flags(struct nft_ctx *ctx, unsigned int flags)
+{
+	unsigned int old_flags;
+
+	old_flags = ctx->input.flags;
+	ctx->input.flags = flags;
+	return old_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

Re: [nft PATCH v4 1/6] src: add input flags for nft_ctx

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:35:14PM +0200, Thomas Haller wrote:
> Similar to the existing output flags, add input flags. No flags are yet
> implemented, that will follow.
> 
> One difference to nft_ctx_output_set_flags(), is that the setter for
> input flags returns the previously set flags.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>

Reviewed-by: Phil Sutter <phil@nwl.cc>

[nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[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 will work 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(). By parsing via inet_pton(), we are better aware of
what we expect and can generate a better error message in case of
failure.

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

diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
index a0d3521e5e7a..62de75f3fa22 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 no 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.h b/include/nftables.h
index 7d35a95a89de..666a17ae4dab 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -27,6 +27,11 @@ struct input_ctx {
 	unsigned int flags;
 };
 
+static inline bool nft_input_no_dns(const struct input_ctx *ictx)
+{
+	return ictx->flags & NFT_CTX_INPUT_NO_DNS;
+}
+
 struct output_ctx {
 	unsigned int flags;
 	union {
diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
index 9a05d3c4b90d..e109805f32a1 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);
 unsigned int nft_ctx_input_set_flags(struct nft_ctx *ctx, unsigned int flags);
 
diff --git a/src/datatype.c b/src/datatype.c
index da802a18bccd..de4fbd776df5 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 (nft_input_no_dns(ctx->input)) {
+		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 (nft_input_no_dns(ctx->input)) {
+		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 8fc1ca7e4b4f..e87177729cc3 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -277,7 +277,10 @@ 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 = {
+		.tbl	= &ctx->nft->output.tbl,
+		.input	= &ctx->nft->input,
+	};
 	struct error_record *erec;
 	struct table *table;
 	struct set *set;
@@ -3432,7 +3435,10 @@ 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 = {
+		.tbl	= &ctx->nft->output.tbl,
+		.input	= &ctx->nft->input,
+	};
 	struct error_record *erec;
 	struct expr *code;
 
-- 
2.41.0

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:35:16PM +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 will work or makes sense.
> 
> Add a new input flag NFT_CTX_INPUT_NO_DNS to opt-out from getaddrinfo()
> and only accept plain IP addresses.

This sounds like user input validation via backend. Another way to solve
the problem at hand is to not insert host names into the rules(et) fed
into libnftables, right?

Cheers, Phil

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Thomas Haller]

On Tue, 2023-08-08 at 15:24 +0200, Phil Sutter wrote:
> On Thu, Aug 03, 2023 at 09:35:16PM +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 will work or makes sense.
> > 
> > Add a new input flag NFT_CTX_INPUT_NO_DNS to opt-out from
> > getaddrinfo()
> > and only accept plain IP addresses.
> 
> This sounds like user input validation via backend. Another way to
> solve
> the problem at hand is to not insert host names into the rules(et)
> fed
> into libnftables, right?

Right. More generally, ensure not to pass any non-addresses in JSON
that would be resolved.

Which requires that the user application is keenly aware, understands
and validates the input data. For example, there couldn't be a "expert
option" where the admin configures arbitrary JSON.

And that the application doesn't make a mistake with that ([1]).

[1] https://github.com/firewalld/firewalld/commit/4db89e316f2d60f3cf856a7025a96a61e40b1e5a

Thomas

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Phil Sutter]

On Tue, Aug 08, 2023 at 10:05:19PM +0200, Thomas Haller wrote:
> On Tue, 2023-08-08 at 15:24 +0200, Phil Sutter wrote:
> > On Thu, Aug 03, 2023 at 09:35:16PM +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 will work or makes sense.
> > > 
> > > Add a new input flag NFT_CTX_INPUT_NO_DNS to opt-out from
> > > getaddrinfo()
> > > and only accept plain IP addresses.
> > 
> > This sounds like user input validation via backend. Another way to
> > solve
> > the problem at hand is to not insert host names into the rules(et)
> > fed
> > into libnftables, right?
> 
> Right. More generally, ensure not to pass any non-addresses in JSON
> that would be resolved.

Well, detecting if a string constitutes a valid IP address is rather
trivial. In Python, there's even 'ipaddress' module for that job.

> Which requires that the user application is keenly aware, understands
> and validates the input data. For example, there couldn't be a "expert
> option" where the admin configures arbitrary JSON.

Why is host resolution a problem in such scenario? The fact that using
host names instead of IP addresses may result in significant delays due
to the required DNS queries is pretty common knowledge among system
administrators.

> And that the application doesn't make a mistake with that ([1]).
> 
> [1] https://github.com/firewalld/firewalld/commit/4db89e316f2d60f3cf856a7025a96a61e40b1e5a

This is just a bug in firewall-cmd, missing to convert ranges into JSON
format. I don't see the benefit for users which no longer may use host
names in that spot.

Cheers, Phil

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Thomas Haller]

Hi,

On Wed, 2023-08-09 at 12:20 +0200, Phil Sutter wrote:
> On Tue, Aug 08, 2023 at 10:05:19PM +0200, Thomas Haller wrote:
> > On Tue, 2023-08-08 at 15:24 +0200, Phil Sutter wrote:
> > > On Thu, Aug 03, 2023 at 09:35:16PM +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 will work or makes sense.
> > > > 
> > > > Add a new input flag NFT_CTX_INPUT_NO_DNS to opt-out from
> > > > getaddrinfo()
> > > > and only accept plain IP addresses.
> > > 
> > > This sounds like user input validation via backend. Another way
> > > to
> > > solve
> > > the problem at hand is to not insert host names into the
> > > rules(et)
> > > fed
> > > into libnftables, right?
> > 
> > Right. More generally, ensure not to pass any non-addresses in JSON
> > that would be resolved.
> 
> Well, detecting if a string constitutes a valid IP address is rather
> trivial. In Python, there's even 'ipaddress' module for that job.

firewalld messed it up, showing that it can happen.

> 
> > Which requires that the user application is keenly aware,
> > understands
> > and validates the input data. For example, there couldn't be a
> > "expert
> > option" where the admin configures arbitrary JSON.
> 
> Why is host resolution a problem in such scenario? The fact that
> using
> host names instead of IP addresses may result in significant delays
> due
> to the required DNS queries is pretty common knowledge among system
> administrators.


It seems prudent that libnftables provides a mode of operation so that
it doesn't block the calling application. Otherwise, it is a problem
for applications that care about that.


> 
> > And that the application doesn't make a mistake with that ([1]).
> > 
> > [1]
> > https://github.com/firewalld/firewalld/commit/4db89e316f2d60f3cf856a7025a96a61e40b1e5a
> 
> This is just a bug in firewall-cmd, missing to convert ranges into
> JSON
> format. I don't see the benefit for users which no longer may use
> host
> names in that spot.

Which spot do you mean? /sbin/nft is not affected, unless it opts-in to
the new flag. firewalld never supported hostnames at that spot anyway
(or does it?).


Thomas

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Phil Sutter]

On Wed, Aug 09, 2023 at 09:17:36PM +0200, Thomas Haller wrote:
> On Wed, 2023-08-09 at 12:20 +0200, Phil Sutter wrote:
> > On Tue, Aug 08, 2023 at 10:05:19PM +0200, Thomas Haller wrote:
> > > On Tue, 2023-08-08 at 15:24 +0200, Phil Sutter wrote:
> > > > On Thu, Aug 03, 2023 at 09:35:16PM +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 will work or makes sense.
> > > > > 
> > > > > Add a new input flag NFT_CTX_INPUT_NO_DNS to opt-out from
> > > > > getaddrinfo()
> > > > > and only accept plain IP addresses.
> > > > 
> > > > This sounds like user input validation via backend. Another way
> > > > to
> > > > solve
> > > > the problem at hand is to not insert host names into the
> > > > rules(et)
> > > > fed
> > > > into libnftables, right?
> > > 
> > > Right. More generally, ensure not to pass any non-addresses in JSON
> > > that would be resolved.
> > 
> > Well, detecting if a string constitutes a valid IP address is rather
> > trivial. In Python, there's even 'ipaddress' module for that job.
> 
> firewalld messed it up, showing that it can happen.

I'm just saying it's possible. From libnftables' PoV, hostname input is
supported and works as expected.

> > > Which requires that the user application is keenly aware,
> > > understands
> > > and validates the input data. For example, there couldn't be a
> > > "expert
> > > option" where the admin configures arbitrary JSON.
> > 
> > Why is host resolution a problem in such scenario? The fact that
> > using
> > host names instead of IP addresses may result in significant delays
> > due
> > to the required DNS queries is pretty common knowledge among system
> > administrators.
> 
> 
> It seems prudent that libnftables provides a mode of operation so that
> it doesn't block the calling application. Otherwise, it is a problem
> for applications that care about that.

Hmm. In that case, one might also have to take care of calls to
getprotobyname() and maybe others (getaddrinfo()?). Depending on
nsswitch.conf it may block, too, right?

> > > And that the application doesn't make a mistake with that ([1]).
> > > 
> > > [1]
> > > https://github.com/firewalld/firewalld/commit/4db89e316f2d60f3cf856a7025a96a61e40b1e5a
> > 
> > This is just a bug in firewall-cmd, missing to convert ranges into
> > JSON
> > format. I don't see the benefit for users which no longer may use
> > host
> > names in that spot.
> 
> Which spot do you mean? /sbin/nft is not affected, unless it opts-in to
> the new flag. firewalld never supported hostnames at that spot anyway
> (or does it?).

I'm pretty sure it does, albeit maybe not officially.

Cheers, Phil

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Thomas Haller]

Hi Phil,

On Thu, 2023-08-10 at 09:45 +0200, Phil Sutter wrote:
> On Wed, Aug 09, 2023 at 09:17:36PM +0200, Thomas Haller wrote:
> 
> 
> > 
> > It seems prudent that libnftables provides a mode of operation so
> > that
> > it doesn't block the calling application. Otherwise, it is a
> > problem
> > for applications that care about that.
> 
> Hmm. In that case, one might also have to take care of calls to
> getprotobyname() and maybe others (getaddrinfo()?). Depending on
> nsswitch.conf it may block, too, right?

getaddrinfo() is avoided by NFT_CTX_INPUT_NO_DNS.

... except at one place in `inet_service_type_parse()`, where
getaddrinfo() is used to parse the service. I don't think that has any
reason to block(*), has it?.

getprotobyname() also should not block(*)  as it merely reads
/etc/protocols (in musl it's even hard-coded).


(*) reading from /etc or talking netlink to kernel is sufficiently fast
so I consider it "non-blocking".


In the first version, the flag was called NFT_CTX_NO_BLOCK. It had the
goal to avoid any significant blocking. The flag got renamed to
NFT_CTX_INPUT_NO_DNS, which on the surface has the different goal to
only accept plain IP addresses.

If there are other places that still can block, they should be
identified and addressed. But that's then separate from NO_DNS flag.


> 
> > > > And that the application doesn't make a mistake with that
> > > > ([1]).
> > > > 
> > > > [1]
> > > > https://github.com/firewalld/firewalld/commit/4db89e316f2d60f3cf856a7025a96a61e40b1e5a
> > > 
> > > This is just a bug in firewall-cmd, missing to convert ranges
> > > into
> > > JSON
> > > format. I don't see the benefit for users which no longer may use
> > > host
> > > names in that spot.
> > 
> > Which spot do you mean? /sbin/nft is not affected, unless it opts-
> > in to
> > the new flag. firewalld never supported hostnames at that spot
> > anyway
> > (or does it?).
> 
> I'm pretty sure it does, albeit maybe not officially.

That would be important to verify. I will check, thank you.



Thomas

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Phil Sutter]

On Thu, Aug 10, 2023 at 10:43:38AM +0200, Thomas Haller wrote:
> Hi Phil,
> 
> On Thu, 2023-08-10 at 09:45 +0200, Phil Sutter wrote:
> > On Wed, Aug 09, 2023 at 09:17:36PM +0200, Thomas Haller wrote:
> > 
> > 
> > > 
> > > It seems prudent that libnftables provides a mode of operation so
> > > that
> > > it doesn't block the calling application. Otherwise, it is a
> > > problem
> > > for applications that care about that.
> > 
> > Hmm. In that case, one might also have to take care of calls to
> > getprotobyname() and maybe others (getaddrinfo()?). Depending on
> > nsswitch.conf it may block, too, right?
> 
> getaddrinfo() is avoided by NFT_CTX_INPUT_NO_DNS.
> 
> ... except at one place in `inet_service_type_parse()`, where
> getaddrinfo() is used to parse the service. I don't think that has any
> reason to block(*), has it?.
> 
> getprotobyname() also should not block(*)  as it merely reads
> /etc/protocols (in musl it's even hard-coded).
> 
> 
> (*) reading from /etc or talking netlink to kernel is sufficiently fast
> so I consider it "non-blocking".

I think these functions support /etc/nsswitch.conf, so feeding them
names is not necessarily only a local lookup. But anyway, this doesn't
quite matter: /etc/nsswitch.conf is in control of the user, just as is
the input fed into run_cmd_from_*(). So for the sake of the discussion
here, it doesn't make a difference.

> In the first version, the flag was called NFT_CTX_NO_BLOCK. It had the
> goal to avoid any significant blocking. The flag got renamed to
> NFT_CTX_INPUT_NO_DNS, which on the surface has the different goal to
> only accept plain IP addresses.
> 
> If there are other places that still can block, they should be
> identified and addressed. But that's then separate from NO_DNS flag.

Yes, I believe the various name to number lookups are the only potential
blockers.

> > > > > And that the application doesn't make a mistake with that
> > > > > ([1]).
> > > > > 
> > > > > [1]
> > > > > https://github.com/firewalld/firewalld/commit/4db89e316f2d60f3cf856a7025a96a61e40b1e5a
> > > > 
> > > > This is just a bug in firewall-cmd, missing to convert ranges
> > > > into
> > > > JSON
> > > > format. I don't see the benefit for users which no longer may use
> > > > host
> > > > names in that spot.
> > > 
> > > Which spot do you mean? /sbin/nft is not affected, unless it opts-
> > > in to
> > > the new flag. firewalld never supported hostnames at that spot
> > > anyway
> > > (or does it?).
> > 
> > I'm pretty sure it does, albeit maybe not officially.
> 
> That would be important to verify. I will check, thank you.

Did you find time for it already?

Cheers, Phil

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Thomas Haller]

On Wed, 2023-08-16 at 18:01 +0200, Phil Sutter wrote:
> > > 
> > > I'm pretty sure it does, albeit maybe not officially.
> > 
> > That would be important to verify. I will check, thank you.
> 
> Did you find time for it already?

Hi Phil,


Not yet. Will do.

Note that the new behavior is opt-in, and firewalld will have to change
to get it.


Thomas

Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:35:16PM +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 will work 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(). By parsing via inet_pton(), we are better aware of
> what we expect and can generate a better error message in case of
> failure.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>

Reviewed-by: Phil Sutter <phil@nwl.cc>

[nft PATCH v4 3/6] src: add input flag NFT_CTX_INPUT_JSON to enable JSON parsing

[Thomas Haller]

By default, the input is parsed using the nftables grammar. When setting
NFT_CTX_OUTPUT_JSON flag, nftables will first try to parse the input as
JSON before falling back to the nftables grammar.

But NFT_CTX_OUTPUT_JSON flag also turns on JSON for the output. Add a
flag NFT_CTX_INPUT_JSON which allows to treat only the input as JSON,
but keep the output mode unchanged.

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

diff --git a/doc/libnftables.adoc b/doc/libnftables.adoc
index 62de75f3fa22..2cf78d7ae536 100644
--- a/doc/libnftables.adoc
+++ b/doc/libnftables.adoc
@@ -87,6 +87,7 @@ The flags setting controls the input format.
 ----
 enum {
         NFT_CTX_INPUT_NO_DNS = (1 << 0),
+        NFT_CTX_INPUT_JSON   = (1 << 1),
 };
 ----
 
@@ -94,6 +95,11 @@ NFT_CTX_INPUT_NO_DNS::
 	Avoid resolving IP addresses with blocking getaddrinfo(). In that case,
 	only plain IP addresses are accepted.
 
+NFT_CTX_INPUT_JSON:
+	When parsing the input, first try to interpret the input as JSON before
+	falling back to the nftables format. This behavior is implied when setting
+	the NFT_CTX_OUTPUT_JSON flag.
+
 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'
@@ -139,7 +145,8 @@ NFT_CTX_OUTPUT_HANDLE::
 NFT_CTX_OUTPUT_JSON::
 	If enabled at compile-time, libnftables accepts input in JSON format and is able to print output in JSON format as well.
 	See *libnftables-json*(5) for a description of the supported schema.
-	This flag controls JSON output format, input is auto-detected.
+	This flag enables JSON output format. If the flag is set, the input will first be tried as JSON format,
+	before falling back to nftables format. This flag implies NFT_CTX_INPUT_JSON.
 NFT_CTX_OUTPUT_ECHO::
 	The echo setting makes libnftables print the changes once they are committed to the kernel, just like a running instance of *nft monitor* would.
 	Amongst other things, this allows one to retrieve an added rule's handle atomically.
diff --git a/include/nftables.h b/include/nftables.h
index 666a17ae4dab..f073fa95a60d 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -32,6 +32,11 @@ static inline bool nft_input_no_dns(const struct input_ctx *ictx)
 	return ictx->flags & NFT_CTX_INPUT_NO_DNS;
 }
 
+static inline bool nft_input_json(const struct input_ctx *ictx)
+{
+	return ictx->flags & NFT_CTX_INPUT_JSON;
+}
+
 struct output_ctx {
 	unsigned int flags;
 	union {
diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
index e109805f32a1..cc05969215bc 100644
--- a/include/nftables/libnftables.h
+++ b/include/nftables/libnftables.h
@@ -50,6 +50,7 @@ void nft_ctx_set_optimize(struct nft_ctx *ctx, uint32_t flags);
 
 enum {
 	NFT_CTX_INPUT_NO_DNS		= (1 << 0),
+	NFT_CTX_INPUT_JSON		= (1 << 1),
 };
 
 unsigned int nft_ctx_input_get_flags(struct nft_ctx *ctx);
diff --git a/src/libnftables.c b/src/libnftables.c
index 17438b5330cb..69ea9d4135b7 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -582,7 +582,7 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 	nlbuf = xzalloc(strlen(buf) + 2);
 	sprintf(nlbuf, "%s\n", buf);
 
-	if (nft_output_json(&nft->output))
+	if (nft_output_json(&nft->output) || nft_input_json(&nft->input))
 		rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
 	if (rc == -EINVAL)
 		rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &cmds,
@@ -683,7 +683,7 @@ static int __nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename
 		goto err;
 
 	rc = -EINVAL;
-	if (nft_output_json(&nft->output))
+	if (nft_output_json(&nft->output) || nft_input_json(&nft->input))
 		rc = nft_parse_json_filename(nft, filename, &msgs, &cmds);
 	if (rc == -EINVAL)
 		rc = nft_parse_bison_filename(nft, filename, &msgs, &cmds);
-- 
2.41.0

Re: [nft PATCH v4 3/6] src: add input flag NFT_CTX_INPUT_JSON to enable JSON parsing

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:35:18PM +0200, Thomas Haller wrote:
> By default, the input is parsed using the nftables grammar. When setting
> NFT_CTX_OUTPUT_JSON flag, nftables will first try to parse the input as
> JSON before falling back to the nftables grammar.
> 
> But NFT_CTX_OUTPUT_JSON flag also turns on JSON for the output. Add a
> flag NFT_CTX_INPUT_JSON which allows to treat only the input as JSON,
> but keep the output mode unchanged.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>

Reviewed-by: Phil Sutter <phil@nwl.cc>

[nft PATCH v4 4/6] py: fix exception during cleanup of half-initialized Nftables

[Thomas Haller]

When we create a Nftables instance against an older library version,
we might not find a symbol and fail with an exception when initializing
the context object.

Then, __del__() is still called, but resulting in a second exception
because self.__ctx is not set. Avoid that second exception.

    $ python -c 'import nftables; nftables.Nftables()'
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/data/src/nftables/py/nftables.py", line 90, in __init__
        self.nft_ctx_input_get_flags = lib.nft_ctx_input_get_flags
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib64/python3.11/ctypes/__init__.py", line 389, in __getattr__
        func = self.__getitem__(name)
               ^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib64/python3.11/ctypes/__init__.py", line 394, in __getitem__
        func = self._FuncPtr((name_or_ordinal, self))
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    AttributeError: /lib64/libnftables.so.1: undefined symbol: nft_ctx_input_get_flags
    Exception ignored in: <function Nftables.__del__ at 0x7f6315a2c540>
    Traceback (most recent call last):
      File "/data/src/nftables/py/nftables.py", line 166, in __del__
        self.nft_ctx_free(self.__ctx)
        ^^^^^^^^^^^^^^^^^
    AttributeError: 'Nftables' object has no attribute 'nft_ctx_free'

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 py/src/nftables.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/py/src/nftables.py b/py/src/nftables.py
index 68fcd7dd103c..b1186781ab5c 100644
--- a/py/src/nftables.py
+++ b/py/src/nftables.py
@@ -74,6 +74,8 @@ class Nftables:
         is requested from the library and buffering of output and error streams
         is turned on.
         """
+        self.__ctx = None
+
         lib = cdll.LoadLibrary(sofile)
 
         ### API function definitions
@@ -150,7 +152,9 @@ class Nftables:
         self.nft_ctx_buffer_error(self.__ctx)
 
     def __del__(self):
-        self.nft_ctx_free(self.__ctx)
+        if self.__ctx is not None:
+            self.nft_ctx_free(self.__ctx)
+            self.__ctx = None
 
     def __get_output_flag(self, name):
         flag = self.output_flags[name]
-- 
2.41.0

Re: [nft PATCH v4 4/6] py: fix exception during cleanup of half-initialized Nftables

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:35:20PM +0200, Thomas Haller wrote:
> When we create a Nftables instance against an older library version,
> we might not find a symbol and fail with an exception when initializing
> the context object.
> 
> Then, __del__() is still called, but resulting in a second exception
> because self.__ctx is not set. Avoid that second exception.
> 
>     $ python -c 'import nftables; nftables.Nftables()'
>     Traceback (most recent call last):
>       File "<string>", line 1, in <module>
>       File "/data/src/nftables/py/nftables.py", line 90, in __init__
>         self.nft_ctx_input_get_flags = lib.nft_ctx_input_get_flags
>                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>       File "/usr/lib64/python3.11/ctypes/__init__.py", line 389, in __getattr__
>         func = self.__getitem__(name)
>                ^^^^^^^^^^^^^^^^^^^^^^
>       File "/usr/lib64/python3.11/ctypes/__init__.py", line 394, in __getitem__
>         func = self._FuncPtr((name_or_ordinal, self))
>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>     AttributeError: /lib64/libnftables.so.1: undefined symbol: nft_ctx_input_get_flags
>     Exception ignored in: <function Nftables.__del__ at 0x7f6315a2c540>
>     Traceback (most recent call last):
>       File "/data/src/nftables/py/nftables.py", line 166, in __del__
>         self.nft_ctx_free(self.__ctx)
>         ^^^^^^^^^^^^^^^^^
>     AttributeError: 'Nftables' object has no attribute 'nft_ctx_free'
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>

Reviewed-by: Phil Sutter <phil@nwl.cc>

[nft PATCH v4 5/6] py: extract flags helper functions for set_debug()/get_debug()

[Thomas Haller]

Will be re-used for nft_ctx_input_set_flags() and
nft_ctx_input_get_flags().

There are changes in behavior here.

- when passing an unrecognized string (e.g. `ctx.set_debug('foo')` or
  `ctx.set_debug(['foo'])`), a ValueError is now raised instead of a
  KeyError.

- when passing an out-of-range integer, now a ValueError is no raised.
  Previously the integer was truncated to 32bit.

Changing the exception is an API change, but most likely nobody will
care or try to catch a KeyError to find out whether a flag is supported.
Especially, since such a check would be better performed via `'foo' in
ctx.debug_flags`.

In other cases, a TypeError is raised as before.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 py/src/nftables.py | 52 +++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/py/src/nftables.py b/py/src/nftables.py
index b1186781ab5c..95c65cde69c4 100644
--- a/py/src/nftables.py
+++ b/py/src/nftables.py
@@ -156,6 +156,35 @@ class Nftables:
             self.nft_ctx_free(self.__ctx)
             self.__ctx = None
 
+    def _flags_from_numeric(self, flags_dict, val):
+        names = []
+        for n, v in flags_dict.items():
+            if val & v:
+                names.append(n)
+                val &= ~v
+        if val:
+            names.append(val)
+        return names
+
+    def _flags_to_numeric(self, flags_dict, values):
+        if isinstance(values, (str, int)):
+            values = (values,)
+
+        val = 0
+        for v in values:
+            if isinstance(v, str):
+                v = flags_dict.get(v)
+                if v is None:
+                    raise ValueError("Invalid argument")
+            elif isinstance(v, int):
+                if v < 0 or v > 0xFFFFFFFF:
+                    raise ValueError("Invalid argument")
+            else:
+                raise TypeError("Not a valid flag")
+            val |= v
+
+        return val
+
     def __get_output_flag(self, name):
         flag = self.output_flags[name]
         return (self.nft_ctx_output_get_flags(self.__ctx) & flag) != 0
@@ -375,16 +404,7 @@ class Nftables:
         Returns a set of flag names. See set_debug() for details.
         """
         val = self.nft_ctx_output_get_debug(self.__ctx)
-
-        names = []
-        for n,v in self.debug_flags.items():
-            if val & v:
-                names.append(n)
-                val &= ~v
-        if val:
-            names.append(val)
-
-        return names
+        return self._flags_from_numeric(self.debug_flags, val)
 
     def set_debug(self, values):
         """Set debug output flags.
@@ -406,19 +426,9 @@ class Nftables:
         Returns a set of previously active debug flags, as returned by
         get_debug() method.
         """
+        val = self._flags_to_numeric(self.debug_flags, values)
         old = self.get_debug()
-
-        if type(values) in [str, int]:
-            values = [values]
-
-        val = 0
-        for v in values:
-            if type(v) is str:
-                v = self.debug_flags[v]
-            val |= v
-
         self.nft_ctx_output_set_debug(self.__ctx, val)
-
         return old
 
     def cmd(self, cmdline):
-- 
2.41.0

Re: [nft PATCH v4 5/6] py: extract flags helper functions for set_debug()/get_debug()

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:35:22PM +0200, Thomas Haller wrote:
> Will be re-used for nft_ctx_input_set_flags() and
> nft_ctx_input_get_flags().
> 
> There are changes in behavior here.
> 
> - when passing an unrecognized string (e.g. `ctx.set_debug('foo')` or
>   `ctx.set_debug(['foo'])`), a ValueError is now raised instead of a
>   KeyError.
> 
> - when passing an out-of-range integer, now a ValueError is no raised.
>   Previously the integer was truncated to 32bit.
> 
> Changing the exception is an API change, but most likely nobody will
> care or try to catch a KeyError to find out whether a flag is supported.
> Especially, since such a check would be better performed via `'foo' in
> ctx.debug_flags`.
> 
> In other cases, a TypeError is raised as before.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>

Reviewed-by: Phil Sutter <phil@nwl.cc>

[nft PATCH v4 6/6] py: add Nftables.{get,set}_input() API

[Thomas Haller]

Similar to the existing Nftables.{get,set}_debug() API.

Only notable (internal) difference is that nft_ctx_input_set_flags()
returns the old value already, so we don't need to call
Nftables.get_input() first.

The benefit of this API, is that it follows the existing API for debug
flags. Also, when future flags are added it requires few changes to the
python code.

The disadvantage is that it looks different from the underlying C API,
which is confusing when reading the C API. Also, it's a bit cumbersome
to reset only one flag. For example:

     def _drop_flag_foo(flag):
        if isinstance(flag, int):
            return flag & ~FOO_NUM
        if flag == 'foo':
            return 0
        return flag

     ctx.set_input(_drop_flag_foo(v) for v in ctx.get_input())

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 py/src/nftables.py | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/py/src/nftables.py b/py/src/nftables.py
index 95c65cde69c4..2b68fe4184cb 100644
--- a/py/src/nftables.py
+++ b/py/src/nftables.py
@@ -37,6 +37,11 @@ class SchemaValidator:
 class Nftables:
     """A class representing libnftables interface"""
 
+    input_flags = {
+        "no-dns": 0x1,
+        "json": 0x2,
+    }
+
     debug_flags = {
         "scanner":   0x1,
         "parser":    0x2,
@@ -84,6 +89,14 @@ 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.restype = c_uint
+        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]
@@ -185,6 +198,36 @@ class Nftables:
 
         return val
 
+    def get_input(self):
+        """Get currently active input flags.
+
+        Returns a set of flag names. See set_input() for details.
+        """
+        val = self.nft_ctx_input_get_flags(self.__ctx)
+        return self._flags_from_numeric(self.input_flags, val)
+
+    def set_input(self, values):
+        """Set input flags.
+
+        Resets all input flags to values. Accepts either a single flag or a list
+        of flags. Each flag might be given either as string or integer value as
+        shown in the following table:
+
+        Name      | Value (hex)
+        -----------------------
+        "no-dns"  | 0x1
+        "json"    | 0x2
+
+        "no-dns" disables blocking address lookup.
+        "json" enables JSON mode for input.
+
+        Returns a set of previously active input flags, as returned by
+        get_input() method.
+        """
+        val = self._flags_to_numeric(self.input_flags, values)
+        old = self.nft_ctx_input_set_flags(self.__ctx, val)
+        return self._flags_from_numeric(self.input_flags, old)
+
     def __get_output_flag(self, name):
         flag = self.output_flags[name]
         return (self.nft_ctx_output_get_flags(self.__ctx) & flag) != 0
-- 
2.41.0

Re: [nft PATCH v4 6/6] py: add Nftables.{get,set}_input() API

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:35:24PM +0200, Thomas Haller wrote:
> Similar to the existing Nftables.{get,set}_debug() API.
> 
> Only notable (internal) difference is that nft_ctx_input_set_flags()
> returns the old value already, so we don't need to call
> Nftables.get_input() first.
> 
> The benefit of this API, is that it follows the existing API for debug
> flags. Also, when future flags are added it requires few changes to the
> python code.
> 
> The disadvantage is that it looks different from the underlying C API,
> which is confusing when reading the C API. Also, it's a bit cumbersome
> to reset only one flag. For example:
> 
>      def _drop_flag_foo(flag):
>         if isinstance(flag, int):
>             return flag & ~FOO_NUM
>         if flag == 'foo':
>             return 0
>         return flag
> 
>      ctx.set_input(_drop_flag_foo(v) for v in ctx.get_input())

Which would be easier if there were dedicated setter/getter pairs for
each flag. The code for debug flags optimizes for setting multiple flags
at once ("get me all the debugging now!"). Not a veto from my side
though, adding getter/setter pairs after the fact is still possible
without breaking anything.

Thanks, Phil

Re: [nft PATCH v4 6/6] py: add Nftables.{get,set}_input() API

[Thomas Haller]

On Tue, 2023-08-08 at 16:04 +0200, Phil Sutter wrote:
> On Thu, Aug 03, 2023 at 09:35:24PM +0200, Thomas Haller wrote:
> > Similar to the existing Nftables.{get,set}_debug() API.
> > 
> > Only notable (internal) difference is that
> > nft_ctx_input_set_flags()
> > returns the old value already, so we don't need to call
> > Nftables.get_input() first.
> > 
> > The benefit of this API, is that it follows the existing API for
> > debug
> > flags. Also, when future flags are added it requires few changes to
> > the
> > python code.
> > 
> > The disadvantage is that it looks different from the underlying C
> > API,
> > which is confusing when reading the C API. Also, it's a bit
> > cumbersome
> > to reset only one flag. For example:
> > 
> >      def _drop_flag_foo(flag):
> >         if isinstance(flag, int):
> >             return flag & ~FOO_NUM
> >         if flag == 'foo':
> >             return 0
> >         return flag
> > 
> >      ctx.set_input(_drop_flag_foo(v) for v in ctx.get_input())
> 
> Which would be easier if there were dedicated setter/getter pairs for
> each flag. The code for debug flags optimizes for setting multiple
> flags
> at once ("get me all the debugging now!"). Not a veto from my side
> though, adding getter/setter pairs after the fact is still possible
> without breaking anything.

Or

  ctx.set_input(ctx.get_input(numeric=True) & ~FOO_NUM)


Thomas

Re: [nft PATCH v4 6/6] py: add Nftables.{get,set}_input() API

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:35:24PM +0200, Thomas Haller wrote:
> Similar to the existing Nftables.{get,set}_debug() API.
> 
> Only notable (internal) difference is that nft_ctx_input_set_flags()
> returns the old value already, so we don't need to call
> Nftables.get_input() first.
> 
> The benefit of this API, is that it follows the existing API for debug
> flags. Also, when future flags are added it requires few changes to the
> python code.
> 
> The disadvantage is that it looks different from the underlying C API,
> which is confusing when reading the C API. Also, it's a bit cumbersome
> to reset only one flag. For example:
> 
>      def _drop_flag_foo(flag):
>         if isinstance(flag, int):
>             return flag & ~FOO_NUM
>         if flag == 'foo':
>             return 0
>         return flag
> 
>      ctx.set_input(_drop_flag_foo(v) for v in ctx.get_input())

IMO the name is too short. While I find it works with debug ("set_debug"
as in "enable_debugging") but with input I expect something to follow.
So I suggest renaming to (get|set)_input_flags(), similar to
__(get|set)_output_flag() (which get/set a single flag instead of
multiple).

Cheers, Phil

Re: [nft PATCH v4 6/6] py: add Nftables.{get,set}_input() API

[Thomas Haller]

On Wed, 2023-08-16 at 18:10 +0200, Phil Sutter wrote:
> On Thu, Aug 03, 2023 at 09:35:24PM +0200, Thomas Haller wrote:
> > Similar to the existing Nftables.{get,set}_debug() API.
> > 
> > Only notable (internal) difference is that
> > nft_ctx_input_set_flags()
> > returns the old value already, so we don't need to call
> > Nftables.get_input() first.
> > 
> > The benefit of this API, is that it follows the existing API for
> > debug
> > flags. Also, when future flags are added it requires few changes to
> > the
> > python code.
> > 
> > The disadvantage is that it looks different from the underlying C
> > API,
> > which is confusing when reading the C API. Also, it's a bit
> > cumbersome
> > to reset only one flag. For example:
> > 
> >      def _drop_flag_foo(flag):
> >         if isinstance(flag, int):
> >             return flag & ~FOO_NUM
> >         if flag == 'foo':
> >             return 0
> >         return flag
> > 
> >      ctx.set_input(_drop_flag_foo(v) for v in ctx.get_input())
> 
> IMO the name is too short. While I find it works with debug
> ("set_debug"
> as in "enable_debugging") but with input I expect something to
> follow.
> So I suggest renaming to (get|set)_input_flags(), similar to
> __(get|set)_output_flag() (which get/set a single flag instead of
> multiple).


Hi Phil,

changed in v5.

thank you!
Thomas