Re-Architect Firewall

[david22swan]

Hey, just laying this out for everyone so there are no surprises.
As part of a larger discussion, we have decided that the Firewall module is in need of a major restructuring, with more and more issues arising from it and compile times that seem to regularly take upwards of thirty minutes.
In order to try and solve this issue the team is working to put together a plan on how the module should best be restructured to not only resolve the issues currently facing it but to help ensure that similar ones do not occur.
As part of this however we would like to hear the communities feedback and what ideas that they might have on the best way to accomplish this, so that we can reach our goal and provide the best quality module that we can.
As a start there are several questions that must be answered and choices to be made, these including:

• Should we keep to the current provider style module or convert it to run with templates?
  Converting the module may take more time to accomplish but could help keep the modules run time down and make the module more easily modified and maintained.

• Should we refactor the current module or create a fresh one and leave the current one as is?
  Depending on the size of the refactor it may be easier to simply start fresh, especially if we convert to a template style, as we can leave the old module as is for people to use until we get the new one fully fleshed out.
  As a side note however, this could also be accomplished simply by cutting a major release and informing the community beforehand.

• In the event that we create a new module, should we split it into two separate ones, one for iptables and one for ip6tables?
  Although there is overlap in how they are managed they are in fact separate and outright keeping them as such may be more convenient for people who simply use on or the other.

• Finally, should the module/modules be expanded to include nftables?
  Although it has been around for some time now it is only recently that nftables seems to have gained traction, as such it may be best to add support for it to the firewall module/modules to ease people into converting over, as both it and iptables/ip6tables are both maintained by the same organisation, with the prior being actively developed as a successor and poised to take over.

Finally, to anyone worried about the changes coming to the module, know that this is merely an initial investigation and that the actual changes are still some time away. We are well aware of how important this module is and are committed to getting this right.

This message has also been posted in the puppet community slack and the puppet-users@ mailing list.

[canihavethisone]

In a nutshell, i think:

• same module but major version bump
• move to templates,
• Ipv6 in same module
• add nftables so it does everything! (Would you merge in the existing puppet/nftables module logic?)

And if feasible, retain as much of the current hiera structure as possible to ease migration

[rtib]

Personal opinion:

• Firewall rules are volatile kernel settings which might change in runtime without any change in persisted state on the filesystem. To my experience, providers fit best for this kind of configuration. Controlling just the persisted state, which templates are limited to, would miss changes made locally by any other entity. Applying the persisted state each time ... well, that just sucks.
• I prefer small modules, precisely focused on a target. Thus, distinct modules for iptables, ip6tables (which might grow obsolete?), nftables, ebtables, etc. with the outlook to get support for PF, IPFW, UFW and others is all fine for me. However, the demand to have similar APIs from PuppetDSL point of view could grow challenging.
• New modules should be designed really attractive for operators to change. Archiving a widespread module or braking its behaviour or API hardly earns any applause.

[canihavethisone]

@rtib good point re what's active not necessarily being what's on disk. If templates were used the state could be compared, though you do raise a good consideration there.

I get your point about discrete modules, i lean towards modules that manage all facets of a feature, in this case most major firewall options though firewalld is not present here (?). IMO one disadvantage of discrete modules is they can make dependency hell worse.

[rtib]

Indeed, a module named firewall suggests supporting all kinds of firewalls, which is Netfilter for Linux, PacketFilter for BSD, don't know what its called on Windows, etc.

A module supporting many of the above depends on a common API defining the archetypes of all aspects of firewalls. The type/providers architecture is made for this.

[bastelfreak]

I think supporting different implementations, like iptables and nftables, in a single type but with multiple providers, is really tricky. This module is fine for managing iptables. for nftables, https://forge.puppet.com/puppet/nftables exists and is fine. Instead of multiple modules implementing nftables, I think we should focus on puppet/nftables. On the long term, puppetlabs/firewall could be archived and people switch to nftables.

[yakatz]

We have systems using iptables, firewalld, csf, Windows, and probably a few others that I am not remembering. When new people see firewall { in the manifest, they think it works everywhere and wonder why the manifest won't apply on RHEL 9. We ended up writing a wrapper (which uses hiera to decide which other module to invoke based on operating system and with per-node overrides).

I would argue for either:

1. Have separate modules for each firewall type to make it clear what is supported, or
2. Include as many types as possible built in and look at ways to make it possible to add providers in the local environment.

[joshcooper]

Why do compile times take upwards of 30 minutes? How does using type/provider or templates change that?

[tskirvin]

Just to add to the urgency, my understanding is that EL9 systems (e.g. RHEL9) are going to drop iptables in the not-too-distant future. This is enough of an issue that we're looking at switching to firewalld internally.

[bastelfreak]

while I hate firewalld; we still have a module for it as well: https://forge.puppet.com/modules/puppet/firewalld/readme

[david22swan]

@joshcooper This issue here gives a good explanation of at least one of the modules issues: #1053

............. no pun was intended

[david22swan]

Just gonna post my current thoughts regarding each point:

• In regards to swapping to templates, an opinion was offered that since the rules are volatile settings that can change during runtime that a provider setup works better. However in my opinion, since the module as it stands now adds all rules via iptables-persistant then we are already working directly with the persisted state which puppet by it’s very nature is constantly re-applying.
• • One thought I did have was to mix a template style with a small dedicated provider that would call iptables-restore if it detected any difference between the persistant and current state or even on a dedicated schedule.
• On whether or not a fresh module should be created I am leaning on keeping with the old one, while this may mean that we have less time to get the work out, as we would be unable to simply keep maintaining/fixing the current one while the new one is developed, I feel that people would be less inclined to swap over.
• • This also answers whether the module should be split.
• Finally in regards to adding nftables support, qhile this is something that may be done, I consider it is a question for the further future, to be considered properly once the module is fixed up.

[Jayfrown]

My two cents: I would vote against a system of managing persistent state and reapplying it in an iptables-restore-type fashion as this would for example wipe chains and rules managed by fail2ban.

I'm currently able to define my "normal" firewall ruleset using Firewall[] resources, tell the chain to purge rules except those matching f2b-.*, and let fail2ban do its own thing on top of that. Same thing goes for stuff like docker or lxd.

[david22swan]

@Jayfrown Thank you for bringing this to my attention. Had missed this bit of existing functionality when going over the module.
Currently re-doing my plan for going forward, kinda happy that this got caught before I started the actual process so thank you again.

[bastelfreak]

@david22swan are there any new plans? I'm still voting for deprecating the module and putting the effort into puppet/nftables (or puppet/firewalld if people like to use that).

[david22swan]

@bastelfreak Sorry for the late reply, currently working on rewriting the module in the style of the resource_api.
At this point have gotten the basic functionality of the module working and am now working on implementing the firewallchain purge/ignore/ignore_foreign functionality

[jantman]

I completely understand the desire, from a module development and support perspective, to just split these out into separate modules per firewall type. But for those of us who maintain multi-distro/multi-version environments, where we currently have (at least) a mix of iptables and nftables, that's going to result in quite a bit of effort on the user side (i.e. defined types for everything to try and bridge the gap??)...

[greatflyingsteve]

Serious question: if I re-wrote the parser in Ruby optparse, would you guys be more happy to see that, or angry to see that? I'm over in PR 1127 making -j SYNPROXY and its arguments work, and also over in PR 1126 adding support for -m tcp --tcp-option <blah>, so I've spent a couple of WEEKS deep-diving the existing Ruby code, and I think I can honestly say I would have been time ahead at this point to gut the entire Regexp abomination that is the current parsing engine and start from scratch in optparse.

It would probably require re-writing a lot of the current tests just to correct small things like argument order in static test-case strings, but I highly, highly doubt that it would break more things in anyone's environment than the current implementation holds in unknown surprises - for example, did you know that given the right rule, the current design would happily have output you a rule that ends with -j CT -m hashlimit --hashlimit-name foo --hashlimit-upto 20/second --notrack, where --notrack is only valid as an argument to -j CT --notrack?

[bastelfreak]

Instead of rewriting the opt parsing, I still think that the time should be invested in existing nftables modules. puppetlabs/firewall is okay for iptables, but iptables is deprecated. nftables is the successor, there are approved and established module for nftables around and I think they should be used instead.

[greatflyingsteve]

...iptables is deprecated. nftables is the successor...and I think [that] should be used instead.

Okay so I was GOING to say "that cuts off anyone using a RHEL 7 derivative, where the mainline kernel is still 3.10.x," because:

You require the following software in order to run the nft command line tool:
Linux kernel since 3.13, although newer kernel versions are recommended.

...but then I hopped onto one of my development machines, and there sure does seem to be an nftables package available. No rules appear in the nft tool, though, even though I have an iptables ruleset configured currently. Maybe RedHat back-ported nftables into their bizarre ancient kernels?

[bastelfreak]

EL7 is quite old. EL8 and EL9 are available. People sould move on. For those who can't, they can still use the existing firewall module on EL7 :) and yes, nftables is supported in RHEL 7.6 I think

[david22swan]

To anyone who is interested, PR up to convert the module to the resource_api: #1145
Not as massive of a change as I originally envisioned, but think it will help with maintenance and improvement in the future.
In the process of re-adding the automated testing but all my manual checks throughout the process came back clear.

[david22swan]

@here One more shoutout to anyone who is interested, my Firewall pr is officially done and ready for review!
Anyone who has a few free moments please come over and give it a look, want to catch anything I've missed before it's merged and released.
#1145

[david22swan]

That's my pr merged in and once the followup rubocop fixes are in they will both be released.
It was fun.