Rule API

[LionelCons]

This is not to report a bug but to trigger a discussion on the rule API.

Looking at http versus ssh, we see that the API is not consistent. With http, one can only allow a single http server running on a fixed port. With ssh, one can allow multiple ssh servers on any ports (thanks to the ports parameter). It would probably be good to have a common set of parameters (like the one to change the port) for all the service specific rules. Note: ssh currently uses ports while masquerade uses dport.

Also, nftables::rules::http is a class while nftables::rule is a defined type. So one cannot instantiate nftables::rules::http multiple times, as it would be natural to do to declare multiple http servers.

So, changing to a defined type and adding parameters, one could declare multiple http servers this way:

nftables::rules::http { 'foo':
    # note: port not needed as we use the default
    comment => 'foo web server',
}
nftables::rules::http { 'bar':
    port    => 8000,
    comment => 'bar web server',
}

When using nftables features like comments or counter or logging (i.e. log prefix), it could make sense to have multiple individual rules instead of a single rule with multiple ports. This would provide more flexibility.

Comments?

[keachi]

I think it's a good idea to unify the interface, as long as it makes sense. Using a define is a good idea, and I also guess that multiple ports should be possible. Then one can choose to instantiate it once or multiple times for multiple ports.

[duritong]

The reason is more historically:

• This mostly copies what our shorewall module does, since we migrate over
• in shorewall we also started with rule files, that were able to be included from other modules (and so a class to be able to be included multiple times)
• At some point somebody added alternative ports to the sshd module (same server listens on multiple ports) and this required multiple ports to be opened, thus the change to that class
• This got copied over to nftables module

Anyway, what I fear with a unique interface is, that we just have many defines, that exactly does the same (what would be the difference between ssh and http in the end? not much that I see, except the default port(s), which is mostly already the case. And on the other hand we would loose to include possibility if we move it to a define. What is the advantage of a define?

And what would we do with double rules? E.g. both opening port 80?

Just coming up with some questions, not necessary, that they are important.

[LionelCons]

I fully agree that there is not much difference between the ssh and http classes in the end.

IMHO, the main question is who would use such classes. Only internally (via $in_ssh and $in_http) or by the end user?

Will end users rather use nftables::rule for the added flexibility (counter, log, comment...)?

I like very much the nftables::set API because it exposes what can be done via strictly controlled parameters. Typos can be caught early on.

This set API is similar to the puppetlabs-firewall rule API:

firewall { '100 allow http and https access':
    dport  => [80, 443],
    proto  => 'tcp',
    action => 'accept',
}

We could imagine the same kind of API for nftables::rule...

[duritong]

Other modules will definitely use them, since e.g. an apache module just want to have http(s) opened and done.

But we can definitely rather advocate to use more the nftables::rule or nftables::set functionality by exposing them more prominently in the README etc.

[nbarrientos]

I like the idea of seeing classes in the rules directory as public interfaces people can use. I'd rather not clutter nftables with zillions of in_* parameters. As discussed in other MR, I agree with you on leaving there just flags for rather widely used services like sshd. Actually in our internal documentation we're encouraging people to look in rules before writing their own nftables::rule as it's less error prone.

I agree though that perhaps it makes sense indeed to make the interface homogenous across rule classes, where it makes sense. For instance, I don't see the point in allowing the user to customise the ports in nftables::rules::dhcpv6_client. If you really have to do that perhaps it's when it's time to use nftables::rule instead. I don't have a preference over using a defined resources to allow multiple instances, other than saying that this approach would break the current interface and that's rather bad. In short, as a middle-ground, I'd just leave them as classes providing at least a port parameter allowing several of them when it makes sense.

[nbarrientos]

Giving it another thought, perhaps it does not even make sense to allow customising ports for the rules pre-encapsulated in classes.

I personally see classes in rules/ and services/ to have a mapping to /etc/services. So for instance if I wanted to allow access to the http service, it'd be safe to assume that there is a nftables::services::http (to be used if I'm not allowing all outgoing traffic) and a nftables::rules::http (if I'm).

On the other hand, if I wanted to run a http daemon on a non-standard port I'd use nftables::rule directly.

That's my point of view of course.

[LionelCons]

IMHO, there are two different decisions to be made here.

First, do we improve the nftables::rule API to be similar to nftables::set? Pros: avoids typos with typed parameters. Cons: quite a few parameters to add.

Second, what to do with nftables::rules::* and nftables::servcies::*. They can stay as they are, be changed from classes to defined types, get more consistency (e.g. wrt ports), get more "options" like counter or log, get less options (so removing the port to have a simple mapping with /etc/services).

Note: nftables allows named ports so nftables::rules::http could simply contain tcp dport http accept .

[nbarrientos]

My opinion:

First, do we improve the nftables::rule API to be similar to nftables::set? Pros: avoids typos with typed parameters. Cons: quite a few parameters to add.

I wouldn't do anything. It's not just adding parameters but adding validation to make sure that the combination can be translated into the syntax of the rules. More complex maintainability and testing for maybe a low gain. Perhaps a nftables::richrule or similar in the future. We're working on better validating and showing errors in case of wrongly formulated rules which should help better visualising human mistakes.

Second, what to do with nftables::rules::* and nftables::servcies::*. They can stay as they are, be changed from classes to defined types, get more consistency (e.g. wrt ports), get more "options" like counter or log, get less options (so removing the port to have a simple mapping with /etc/services).

Basically, https://github.com/duritong/puppet-nftables/issues/17#issuecomment-733047927. No ports but perhaps a common parameter to enable counters sounds like a sweet spot :) Anything beyond basic functionality that we can easily encapsulate (like log rules) should be left to the user who can always do complex and custom stuff with nftables::rule.

Just my 2 cents.

[LionelCons]

In addition to counter, there is also a very cool feature that could be useful for these "standard" services under nftables::rules::* and nftables::servcies::*.

For instance for ssh, instead of having:

tcp dport 22 accept

one can use:

tcp dport 22 ct state new log prefix "[nftables] Accepted: " accept

This assumes a ct state established,related accept somewhere and will log every incoming connection (new connection only, not every packet).

[keachi]

I think it's a good idea to add logging for certain rules, but I think it should be optionally.
As we have a ct state established,related accept rule on top of our rule set those rules will only handle ct state new. But declaring this explicitly is also a good idea.

[traylenator]

It's true that with.

firewall { '100 allow http and https access':
    dport  => [80, 443],
    proto  => 'tcp',
    action => 'accept',
}

there has allways been parameters missing or not quite correct.

Having said that I like the nftables::richrule or nftables::match maybe.

Validation is so much better and easier in puppet these days that its easy to do something that is useful.
Given it will be compatiable with nftables::rule nftables::richrule will not have cover every obscure corner case.

[traylenator]

Another option perhaps is another module that really does try to support the abstraction of nftables, iptables, ....
A full time job maintaining that however.

[LionelCons]

I fully agree that a 100% coverage of all the possible nftables rules in a high-level API is a huge amount of work.

What I have in mind is a reduced API for a subset of all the possible rules since we can always give the complete rule line as a string when needed

What is the minimum API that would be needed to cover, for instance, 90% of all the rules used in realistic scenarios?

Let's take for instance an API with the following parameters: protocol (as tcp or icmp), type (for icmp), saddr, daddr, sport, dport, counter (boolean), fate (as accept or drop) and comment. Would it be useful?

[nbarrientos]

If we agreed on a simple interface like the one proposed by Lionel, kind of covering simple uses of the firewall type and respecting whenever possible the name of the parameters, I'd be happy to propose a nftables::simplerule. It could just encapsulate generating a content using a template to be fed to a nftables::rule and use Puppet types for input validation. We could also state clearly in the documentation of the class that the aim is to provide a firewall-like interface covering basic cases and that for more advanced configurations the way to go is to use nftables::rule directly.

[keachi]

IMHO iifname should be covered as well. And if we don't cover NATing with this interface I propose nftables::simpleinetrule or something like that, then it's clear that the rule doesn't cover NATing. Else I would propose to support simple NAT possibilities too (even if a simple server doesn't use that).

[nbarrientos]

I've written a simple PoC and sent is as a merge request so we can discuss a bit about the interface, etc. It misses parameters of course :)

It does not take into account your latest comment, @keachi. I'd be happy to focus only on inet rules to be honest. I doubt somebody configuring NAT stuff would even profit from a simplified interface. Dunno. The more complexity we put on the class the more maintenance we'll have to support.

[duritong]

I am absolutely fine with multiple interfaces covering different aspects (inet vs. NAT). We don't need one to rule them all, for that we can pass in the rule string.

And if folks are seeing to repeat themselves with certain interfaces, they will come up with (another) abstraction layer, which is fine. Keeping the single interface simple and focused is much more important.