[nft PATCH] List handles of added rules if requested

[nft PATCH] List handles of added rules if requested

[Phil Sutter]

Being able to retrieve an added rule's handle atomically is a crucial
feature for scripts invoking nft command: Without it, there is no way to
be sure a handle extracted from 'nft list ruleset' command actually
refers to the rule one has added before or that of another process which
ran in between.

Extracting an added rule's handle itself is not an easy task already,
since there is a chance that a given rule is printed differently than
when it was added before. A simple example is port number vs. service
name:

| nft add rule ip t c tcp dport { ssh, 80 } accept

There is no way to make 'nft list ruleset' return the rule just like
this as depending on whether '-nn' was given or not, it either prints
the set as '{ ssh, http }' or '{ 22, 80 }' but never in the mixed form
that was used when adding it.

This patch prints an identifying string for each added rule which may be
used as single parameter to a later 'nft delete rule' command. So a
simple scripting example looks like this:

| handle=$(nft add rule ip t c counter)
| ...
| nft delete rule $handle

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/mnl.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/mnl.c b/src/mnl.c
index 295dd84a58406..d5b261aff0aee 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -26,9 +26,11 @@
 #include <string.h>
 #include <sys/socket.h>
 #include <arpa/inet.h>
+#include <inttypes.h>
 #include <errno.h>
 #include <utils.h>
 #include <nftables.h>
+#include <rule.h>
 
 static int seq;
 
@@ -242,6 +244,31 @@ static ssize_t mnl_nft_socket_sendmsg(const struct mnl_socket *nl)
 	return sendmsg(mnl_socket_get_fd(nl), &msg, 0);
 }
 
+static int mnl_callback(const struct nlmsghdr *nlh, void *data)
+{
+	struct nftnl_rule *nlr;
+
+	if (!handle_output)
+		return MNL_CB_OK;
+
+	nlr = nftnl_rule_alloc();
+	if (!nlr)
+		memory_allocation_error();
+
+	if (nftnl_rule_nlmsg_parse(nlh, nlr) < 0)
+		goto out_free;
+
+	printf("%s %s %s handle %" PRIu64 "\n",
+	       family2str(nftnl_rule_get_u32(nlr, NFTNL_RULE_FAMILY)),
+	       nftnl_rule_get_str(nlr, NFTNL_RULE_TABLE),
+	       nftnl_rule_get_str(nlr, NFTNL_RULE_CHAIN),
+	       nftnl_rule_get_u64(nlr, NFTNL_RULE_HANDLE));
+
+out_free:
+	nftnl_rule_free(nlr);
+	return MNL_CB_OK;
+}
+
 int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list)
 {
 	int ret, fd = mnl_socket_get_fd(nl), portid = mnl_socket_get_portid(nl);
@@ -271,11 +298,12 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list)
 		if (ret == -1)
 			return -1;
 
-		ret = mnl_cb_run(rcv_buf, ret, 0, portid, NULL, NULL);
+		ret = mnl_cb_run(rcv_buf, ret, 0, portid, &mnl_callback, NULL);
 		/* Continue on error, make sure we get all acknowledgments */
 		if (ret == -1)
 			mnl_err_list_node_add(err_list, errno, nlh->nlmsg_seq);
 
+
 		ret = select(fd+1, &readfds, NULL, NULL, &tv);
 		if (ret == -1)
 			return -1;
-- 
2.11.0

Re: [nft PATCH] List handles of added rules if requested

[Pablo Neira Ayuso]

On Thu, May 04, 2017 at 02:34:21PM +0200, Phil Sutter wrote:
> Being able to retrieve an added rule's handle atomically is a crucial
> feature for scripts invoking nft command: Without it, there is no way to
> be sure a handle extracted from 'nft list ruleset' command actually
> refers to the rule one has added before or that of another process which
> ran in between.
> 
> Extracting an added rule's handle itself is not an easy task already,
> since there is a chance that a given rule is printed differently than
> when it was added before. A simple example is port number vs. service
> name:
> 
> | nft add rule ip t c tcp dport { ssh, 80 } accept
> 
> There is no way to make 'nft list ruleset' return the rule just like
> this as depending on whether '-nn' was given or not, it either prints
> the set as '{ ssh, http }' or '{ 22, 80 }' but never in the mixed form
> that was used when adding it.
> 
> This patch prints an identifying string for each added rule which may be
> used as single parameter to a later 'nft delete rule' command. So a
> simple scripting example looks like this:
> 
> | handle=$(nft add rule ip t c counter)

This is a hack.

We should follow the rule description path.

Re: [nft PATCH] List handles of added rules if requested

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, May 04, 2017 at 02:34:21PM +0200, Phil Sutter wrote:
> > Being able to retrieve an added rule's handle atomically is a crucial
> > feature for scripts invoking nft command: Without it, there is no way to
> > be sure a handle extracted from 'nft list ruleset' command actually
> > refers to the rule one has added before or that of another process which
> > ran in between.
> > 
> > Extracting an added rule's handle itself is not an easy task already,
> > since there is a chance that a given rule is printed differently than
> > when it was added before. A simple example is port number vs. service
> > name:
> > 
> > | nft add rule ip t c tcp dport { ssh, 80 } accept
> > 
> > There is no way to make 'nft list ruleset' return the rule just like
> > this as depending on whether '-nn' was given or not, it either prints
> > the set as '{ ssh, http }' or '{ 22, 80 }' but never in the mixed form
> > that was used when adding it.
> > 
> > This patch prints an identifying string for each added rule which may be
> > used as single parameter to a later 'nft delete rule' command. So a
> > simple scripting example looks like this:
> > 
> > | handle=$(nft add rule ip t c counter)
> 
> This is a hack.
> 
> We should follow the rule description path.

You mean delete-by-name?

Its just as ugly, just a different kind of ugly.

Will you delete the first match?  The last one?  All of them?

Re: [nft PATCH] List handles of added rules if requested

[Pablo Neira Ayuso]

On Thu, May 04, 2017 at 03:44:19PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, May 04, 2017 at 02:34:21PM +0200, Phil Sutter wrote:
> > > Being able to retrieve an added rule's handle atomically is a crucial
> > > feature for scripts invoking nft command: Without it, there is no way to
> > > be sure a handle extracted from 'nft list ruleset' command actually
> > > refers to the rule one has added before or that of another process which
> > > ran in between.
> > > 
> > > Extracting an added rule's handle itself is not an easy task already,
> > > since there is a chance that a given rule is printed differently than
> > > when it was added before. A simple example is port number vs. service
> > > name:
> > > 
> > > | nft add rule ip t c tcp dport { ssh, 80 } accept
> > > 
> > > There is no way to make 'nft list ruleset' return the rule just like
> > > this as depending on whether '-nn' was given or not, it either prints
> > > the set as '{ ssh, http }' or '{ 22, 80 }' but never in the mixed form
> > > that was used when adding it.
> > > 
> > > This patch prints an identifying string for each added rule which may be
> > > used as single parameter to a later 'nft delete rule' command. So a
> > > simple scripting example looks like this:
> > > 
> > > | handle=$(nft add rule ip t c counter)
> > 
> > This is a hack.
> > 
> > We should follow the rule description path.
> 
> You mean delete-by-name?
> 
> Its just as ugly, just a different kind of ugly.

Ugly?

This kernel patch is seriouly broken. It's sending a message to
userspace from the preparation phase of the commit protocol, where
things are not even confirmed at all...

> Will you delete the first match?  The last one?  All of them?

I already explained this Florian. Please, look at the mail archive.

Re: [nft PATCH] List handles of added rules if requested

[Phil Sutter]

On Thu, May 04, 2017 at 04:00:42PM +0200, Pablo Neira Ayuso wrote:
> On Thu, May 04, 2017 at 03:44:19PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Thu, May 04, 2017 at 02:34:21PM +0200, Phil Sutter wrote:
> > > > Being able to retrieve an added rule's handle atomically is a crucial
> > > > feature for scripts invoking nft command: Without it, there is no way to
> > > > be sure a handle extracted from 'nft list ruleset' command actually
> > > > refers to the rule one has added before or that of another process which
> > > > ran in between.
> > > > 
> > > > Extracting an added rule's handle itself is not an easy task already,
> > > > since there is a chance that a given rule is printed differently than
> > > > when it was added before. A simple example is port number vs. service
> > > > name:
> > > > 
> > > > | nft add rule ip t c tcp dport { ssh, 80 } accept
> > > > 
> > > > There is no way to make 'nft list ruleset' return the rule just like
> > > > this as depending on whether '-nn' was given or not, it either prints
> > > > the set as '{ ssh, http }' or '{ 22, 80 }' but never in the mixed form
> > > > that was used when adding it.
> > > > 
> > > > This patch prints an identifying string for each added rule which may be
> > > > used as single parameter to a later 'nft delete rule' command. So a
> > > > simple scripting example looks like this:
> > > > 
> > > > | handle=$(nft add rule ip t c counter)
> > > 
> > > This is a hack.
> > > 
> > > We should follow the rule description path.
> > 
> > You mean delete-by-name?
> > 
> > Its just as ugly, just a different kind of ugly.
> 
> Ugly?
> 
> This kernel patch is seriouly broken. It's sending a message to
> userspace from the preparation phase of the commit protocol, where
> things are not even confirmed at all...

Oh, I indeed ignored the transactional behaviour at all. But looking at
the code, if I use NLM_F_ECHO that should be fine, right?

> > Will you delete the first match?  The last one?  All of them?
> 
> I already explained this Florian. Please, look at the mail archive.

While I think it's not a bad idea to allow users experienced with
iptables to apply their muscle memory to nftables as well, I don't quite
get what should hold us back from leveraging this feature nftables
provides over iptables. The existence of a unique identifier is a big
plus in my point of view, it's just not really useful yet since users
have no safe way to get that handle for the rule they added.

Are you OK with providing both alternatives in parallel? If not, why?

Thanks, Phil

Re: [nft PATCH] List handles of added rules if requested

[Pablo Neira Ayuso]

On Fri, May 05, 2017 at 12:26:12AM +0200, Phil Sutter wrote:
> On Thu, May 04, 2017 at 04:00:42PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, May 04, 2017 at 03:44:19PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Thu, May 04, 2017 at 02:34:21PM +0200, Phil Sutter wrote:
> > > > > Being able to retrieve an added rule's handle atomically is a crucial
> > > > > feature for scripts invoking nft command: Without it, there is no way to
> > > > > be sure a handle extracted from 'nft list ruleset' command actually
> > > > > refers to the rule one has added before or that of another process which
> > > > > ran in between.
> > > > > 
> > > > > Extracting an added rule's handle itself is not an easy task already,
> > > > > since there is a chance that a given rule is printed differently than
> > > > > when it was added before. A simple example is port number vs. service
> > > > > name:
> > > > > 
> > > > > | nft add rule ip t c tcp dport { ssh, 80 } accept
> > > > > 
> > > > > There is no way to make 'nft list ruleset' return the rule just like
> > > > > this as depending on whether '-nn' was given or not, it either prints
> > > > > the set as '{ ssh, http }' or '{ 22, 80 }' but never in the mixed form
> > > > > that was used when adding it.
> > > > > 
> > > > > This patch prints an identifying string for each added rule which may be
> > > > > used as single parameter to a later 'nft delete rule' command. So a
> > > > > simple scripting example looks like this:
> > > > > 
> > > > > | handle=$(nft add rule ip t c counter)
> > > > 
> > > > This is a hack.
> > > > 
> > > > We should follow the rule description path.
> > > 
> > > You mean delete-by-name?
> > > 
> > > Its just as ugly, just a different kind of ugly.
> > 
> > Ugly?
> > 
> > This kernel patch is seriouly broken. It's sending a message to
> > userspace from the preparation phase of the commit protocol, where
> > things are not even confirmed at all...
> 
> Oh, I indeed ignored the transactional behaviour at all. But looking at
> the code, if I use NLM_F_ECHO that should be fine, right?
> 
> > > Will you delete the first match?  The last one?  All of them?
> > 
> > I already explained this Florian. Please, look at the mail archive.
> 
> While I think it's not a bad idea to allow users experienced with
> iptables to apply their muscle memory to nftables as well, I don't quite
> get what should hold us back from leveraging this feature nftables
> provides over iptables. The existence of a unique identifier is a big
> plus in my point of view, it's just not really useful yet since users
> have no safe way to get that handle for the rule they added.
>
> Are you OK with providing both alternatives in parallel? If not, why?

This does not integrate at all into the scripting features we have in
nftables. We don't want people to use bash (or like) shell scripts
anymore, they are bad, they break atomicity for us. We should extend
native nftables scripting capabilities to support what user need,
natively. Look, this will not work with nft -i either...

And this also will not work for robots using incremental updates via
nft -f. And we very much want to support such transaction like scheme,
ie. place a bunch of incremental updates in one single file and apply
that in one single transaction.

This is just covering one very specific usecase, that is, users have a
quick way to delete the rule that just added. And we have better ways
to achieve this, and that will work from all the scenarios that I
described above.

Re: [nft PATCH] List handles of added rules if requested

[Puustinen, Ismo]

On Fri, 2017-05-05 at 12:49 +0200, Pablo Neira Ayuso wrote:
> This does not integrate at all into the scripting features we have in
> 
> nftables. We don't want people to use bash (or like) shell scripts
> anymore, they are bad, they break atomicity for us. We should extend
> native nftables scripting capabilities to support what user need,
> natively. Look, this will not work with nft -i either...
> 
> And this also will not work for robots using incremental updates via
> nft -f. And we very much want to support such transaction like
> scheme,
> ie. place a bunch of incremental updates in one single file and apply
> that in one single transaction.
> 
> This is just covering one very specific usecase, that is, users have
> a
> quick way to delete the rule that just added. And we have better ways
> to achieve this, and that will work from all the scenarios that I
> described above.

I would be interested in any documentation you might have concerning
the "better ways" of creating modular rulesets, that is, automatically
adding and deleting rules and rule groups. This is a real issue that
I'm currently struggling with.

My use case is  pretty simple: when a service is loaded, it also loads
the firewall rules associated with it. When a service is stopped, the
rules corresponding to the service are unloaded. There is no
interactive user present in the system.

Having "nft add rule ..." return a rule handle would fix this, so I was
happy to see Phil's patches. Currently I have to load the rules in
their own chains (one chain per service) and then use a packet marking
scheme to see if the packet has been marked to be accepted by at least
one service chain. When a service is stopped, I then remove the
corresponding chain. The packet marking scheme is quite ugly though,
and the service chains need to know how the packets need to be marked
so that they get accepted by the final chain.

I can't use the "jump" facility to jump to the service chains, because
then I would need to be able to remove the "jump" rule when the service
is unloaded, which is again not possible without the rule handle.

Ismo

Re: [nft PATCH] List handles of added rules if requested

[Pablo Neira Ayuso]

On Fri, May 05, 2017 at 11:18:37AM +0000, Puustinen, Ismo wrote:
> On Fri, 2017-05-05 at 12:49 +0200, Pablo Neira Ayuso wrote:
> > This does not integrate at all into the scripting features we have in
> > 
> > nftables. We don't want people to use bash (or like) shell scripts
> > anymore, they are bad, they break atomicity for us. We should extend
> > native nftables scripting capabilities to support what user need,
> > natively. Look, this will not work with nft -i either...
> > 
> > And this also will not work for robots using incremental updates via
> > nft -f. And we very much want to support such transaction like
> > scheme,
> > ie. place a bunch of incremental updates in one single file and apply
> > that in one single transaction.
> > 
> > This is just covering one very specific usecase, that is, users have
> > a
> > quick way to delete the rule that just added. And we have better ways
> > to achieve this, and that will work from all the scenarios that I
> > described above.
> 
> I would be interested in any documentation you might have concerning
> the "better ways" of creating modular rulesets, that is, automatically
> adding and deleting rules and rule groups. This is a real issue that
> I'm currently struggling with.
> 
> My use case is  pretty simple: when a service is loaded, it also loads
> the firewall rules associated with it. When a service is stopped, the
> rules corresponding to the service are unloaded. There is no
> interactive user present in the system.

Would it be better if you do this programmatically? ie. from a
library. Do you have some example code of how you're doing things
there?

> Having "nft add rule ..." return a rule handle would fix this, so I was
> happy to see Phil's patches.

But this is basically pushing nft to the domain of using non-native
shell scripts.

I guess you call several commands there in a row and that is defeating
the existing commit protocol we have.

Re: [nft PATCH] List handles of added rules if requested

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:

[ CC Eric ]

> On Fri, May 05, 2017 at 12:26:12AM +0200, Phil Sutter wrote:
> > While I think it's not a bad idea to allow users experienced with
> > iptables to apply their muscle memory to nftables as well, I don't quite
> > get what should hold us back from leveraging this feature nftables
> > provides over iptables. The existence of a unique identifier is a big
> > plus in my point of view, it's just not really useful yet since users
> > have no safe way to get that handle for the rule they added.
> >
> > Are you OK with providing both alternatives in parallel? If not, why?
> 
> This does not integrate at all into the scripting features we have in
> nftables. We don't want people to use bash (or like) shell scripts
> anymore, they are bad, they break atomicity for us. We should extend
> native nftables scripting capabilities to support what user need,
> natively. Look, this will not work with nft -i either...

Yes.  OTOH I don't think we want programs to parse nft frontend
text output either...

Eric, whats the status wrt libnft?

> And this also will not work for robots using incremental updates via
> nft -f. And we very much want to support such transaction like scheme,
> ie. place a bunch of incremental updates in one single file and apply
> that in one single transaction.

Right.

> This is just covering one very specific usecase, that is, users have a
> quick way to delete the rule that just added. And we have better ways
> to achieve this, and that will work from all the scenarios that I
> described above.

What about (as first step) to extend nft monitor?

f.e. afaics kernels update notifications already contain the netlink
portid of the peer that added a rule, perhaps we can display that?

nft monitor
add rule ip saddr 1.2.3.4 # nlport 123

We could use that to then also display the process that currently owns
this portid, e.g.:

add rule ip saddr 1.2.3.4 # nlportid 123 # nlport 123 (nft?)
delete rule inet filter input handle 5 # nlport 42 (firewalld?)

We might also consider extending it to display/group transaction ids
to the user so its easier to identify batches.

What do you think?

FWIW, I believe that deletion by handle and by textual description
both have their use-cases so its more of a question on how to implement
this in a sensible way.

Re: [nft PATCH] List handles of added rules if requested

[Thomas Woerner]

On 05/04/2017 03:44 PM, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> On Thu, May 04, 2017 at 02:34:21PM +0200, Phil Sutter wrote:
>>> Being able to retrieve an added rule's handle atomically is a crucial
>>> feature for scripts invoking nft command: Without it, there is no way to
>>> be sure a handle extracted from 'nft list ruleset' command actually
>>> refers to the rule one has added before or that of another process which
>>> ran in between.
>>>
>>> Extracting an added rule's handle itself is not an easy task already,
>>> since there is a chance that a given rule is printed differently than
>>> when it was added before. A simple example is port number vs. service
>>> name:
>>>
>>> | nft add rule ip t c tcp dport { ssh, 80 } accept
>>>
>>> There is no way to make 'nft list ruleset' return the rule just like
>>> this as depending on whether '-nn' was given or not, it either prints
>>> the set as '{ ssh, http }' or '{ 22, 80 }' but never in the mixed form
>>> that was used when adding it.
>>>
>>> This patch prints an identifying string for each added rule which may be
>>> used as single parameter to a later 'nft delete rule' command. So a
>>> simple scripting example looks like this:
>>>
>>> | handle=$(nft add rule ip t c counter)
>>
>> This is a hack.
>>
>> We should follow the rule description path.
> 
> You mean delete-by-name?
> 
> Its just as ugly, just a different kind of ugly.
> 
Delete by name for rules would be an atomic operation. The solution above is 
not as two calls are needed to remove one rule. Saving the handles while the 
rule has been added is not possible all the time and will most likely require 
bigger changes in several projects.

I also do not understand why delete by name is possible for tables, chains and 
sets but not for rules.

Delete by name for rules is also something that would help to be able to create 
an nftables backend for firewalld easily. firewalld is using a transaction 
model for iptables, that I'd also like to use for nftables. With this it is 
possible to add and remove rules in one -restore call for IPv4 and one for 
IPv6. Using this with gathering handles will take more time and will not be atomic.

Though: Flushing the whole rule set and recreation of the new rule set could be 
done in an atomic operation. But it will destroy everything else.

> Will you delete the first match?  The last one?  All of them? > --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [nft PATCH] List handles of added rules if requested

[Phil Sutter]

On Thu, May 04, 2017 at 05:37:25PM +0200, Thomas Woerner wrote:
> On 05/04/2017 03:44 PM, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> On Thu, May 04, 2017 at 02:34:21PM +0200, Phil Sutter wrote:
> >>> Being able to retrieve an added rule's handle atomically is a crucial
> >>> feature for scripts invoking nft command: Without it, there is no way to
> >>> be sure a handle extracted from 'nft list ruleset' command actually
> >>> refers to the rule one has added before or that of another process which
> >>> ran in between.
> >>>
> >>> Extracting an added rule's handle itself is not an easy task already,
> >>> since there is a chance that a given rule is printed differently than
> >>> when it was added before. A simple example is port number vs. service
> >>> name:
> >>>
> >>> | nft add rule ip t c tcp dport { ssh, 80 } accept
> >>>
> >>> There is no way to make 'nft list ruleset' return the rule just like
> >>> this as depending on whether '-nn' was given or not, it either prints
> >>> the set as '{ ssh, http }' or '{ 22, 80 }' but never in the mixed form
> >>> that was used when adding it.
> >>>
> >>> This patch prints an identifying string for each added rule which may be
> >>> used as single parameter to a later 'nft delete rule' command. So a
> >>> simple scripting example looks like this:
> >>>
> >>> | handle=$(nft add rule ip t c counter)
> >>
> >> This is a hack.
> >>
> >> We should follow the rule description path.
> > 
> > You mean delete-by-name?
> > 
> > Its just as ugly, just a different kind of ugly.
> > 
> Delete by name for rules would be an atomic operation. The solution above is 
> not as two calls are needed to remove one rule. Saving the handles while the 
> rule has been added is not possible all the time and will most likely require 
> bigger changes in several projects.

This was not the aspect I was talking about when mentioning missing
atomicity: I didn't have the delete by description approach in mind at
all, and looking at delete by handle the problem is that it requires a
non-atomic operation to add a rule and retrieve it's handle.

With the proposed solution in place, if you take care to collect the
handles for the rules you're adding along the way, you can be sure
you're later deleting exactly the rule you've added before.

But I agree with you in that the situation right now is really messy:
Add a rule, call 'nft -a list ruleset' and then have fun parsing the
output and searching for your rule. I guess pretty much anything is
better than that. :)

> I also do not understand why delete by name is possible for tables, chains and 
> sets but not for rules.

I didn't design all this, but I guess an explanation would be that table
names and the combination of table name and chain name are unique, while
the rule description is not.

Cheers, Phil