Provide a mechanism to flush un-managed rules #115
Conversation
nbarrientos
force-pushed
the
issue113
branch
2 times, most recently
from
abd95ac
to
06ff347
Compare
nbarrientos
force-pushed
the
issue113
branch
3 times, most recently
from
1192571
to
7615dc8
Compare
manifests/init.pp
Outdated
| @@ -164,10 +175,33 @@ | |||
| restart => '/usr/bin/systemctl reload nftables', | |||
| } | |||
|
|
|||
| if $allow_unmanaged_rules { | |||
| file { $inmem_rules_hash_file: | |||
There was a problem hiding this comment.
do we really need to remove it by default?
There was a problem hiding this comment.
It's mainly to remove garbage if the user switches the setting. It can go if it's okay to live with the file until the next reboot. It's not consumed anyway if this mechanism is switched off. I'm happy with both options. Not having the resource makes the Puppet run faster of course.
| exec { 'Reload nftables if there are un-managed rules': | ||
| command => '/usr/bin/systemctl reload nftables', | ||
| refreshonly => false, | ||
| unless => "/usr/bin/test -s ${inmem_rules_hash_file} -a \"$(nft -s list ruleset | sha1sum)\" = \"$(cat ${inmem_rules_hash_file})\"", |
There was a problem hiding this comment.
The nft -s list ruleset will also contain any chains that are present in the nftables::noflush_chains parameter.
Taking the typical use case of fail2ban maintaining a chain we expect that chain to drift almost constantly so ideally we want to ignore those chains when making the comparison to avoid unnecessary reloads.
The reloads themselves will be fine since these noflush chains are already ignored by the reload.
There was a problem hiding this comment.
A hard one. The easy exit is to make them mutually exclusive, otherwise we'd have to find a way to remove the dynamic rules when Puppet takes a snapshot of what's in memory. However, having an option that purges unmanaged rules allowing unmanaged rules sounds odd to me so I'm in favour of the mutual exclusivity.
There was a problem hiding this comment.
If we added --json to the dump filtering out the noflush chains to the dump should be doable?
There was a problem hiding this comment.
I would certainly use both options if they were not mutually exclusive.
There was a problem hiding this comment.
If we added --json to the dump filtering out the noflush chains to the dump should be doable?
It's doable, however this would introduce a dependency on a JSON processor, like jq. Unfortunately the objects are not nested when dumped with --json, so the script would have to iterate over nftables::noflush_tables and for each element attach a filter like:
| del(.[] | select(.rule.table == "table_name" or .chain.table == "table_name" or .table.name == "table_name"))
The filter would probably have to filter by table name and family, as it's legal to have two tables with the same name and different families, making it even uglier.
Ugly but it should work. The script to hash the rules could be used for both dumping (to be called by Systemd as currently proposed) and comparing (to be called by the Exec).
Another option is to do something similar to puppet.nft.epp and generate an array of managed tables by removing nftables::noflush_tables from $facts['nftables']['tables']. Then sort the array, hash each table independently and then join the hashes and use that string to detect changes. Something like (pseudobash):
tables=("inet filter" "ip nat" "ip6 nat") # To be generated by Puppet
hashes=""
for table in "${tables[@]}"
do
sha1=$(/sbin/nft -s list table $table | /usr/bin/sha1sum | cut -f 1 -d " ")
hashes+=$sha1
done
if [ $2 == "store" ];
then
/bin/echo $hashes > $1
elif [ $2 == "compare" ];
then
/usr/bin/test -s $1 -a "$(cat $1)" = "$hashes"
else
echo "Dunno what to do"
exit 1
fiWe'd have to think here if the boostrapping makes sense as $facts['nftables']['tables'] won't be available during the first run.
| Hash $rules = {}, | ||
| Hash $sets = {}, | ||
| String $log_prefix = '[nftables] %<chain>s %<comment>s', | ||
| String[1] $nat_table_name = 'nat', | ||
| Stdlib::Unixpath $inmem_rules_hash_file = '/run/puppet-nft-memhash', |
There was a problem hiding this comment.
Apologies not to think of earlier. Should we create a directory via a systemd::tmpfile to stick this file in.
Also question if it needs to be a parameter? Why would you want to change it?
There was a problem hiding this comment.
Apologies not to think of earlier. Should we create a directory via a systemd::tmpfile to stick this file in.
No worries. Sounds good to me, no objection. We're already depending on the systemd module so all good. /run/puppet-nft?
Also question if it needs to be a parameter? Why would you want to change it?
Thought it was a good idea to offer it but initially it could be hardcoded. If somebody ever needs to make the path customisable then merge requests are always welcome :)
This patchset adds a new parameter to the main class to activate a mechanism that will invoke `systemctl reload nftables` during the Puppet run if manual changes to the in-memory ruleset are detected. To accomplish this, the systemd unit in charge of nftables is configured to write a hash of the in-memory ruleset right after starting/reloading. During the Puppet run, the hash of the current rule set is compared to the one previously stored. If the hash differ then `systemctl reload nftables` is executed to flush manual changes. Fixes voxpupuli#113
nbarrientos
force-pushed
the
issue113
branch
from
67013db
to
019bde3
Compare
|
Can this be considered again? |
|
@JohnHolmesII if you are interested in this PR you can pick it up and rebase it. |
|
Yeah the OA has other things on his mind these days. |
This patchset adds a new parameter to the main class to activate a mechanism that will invoke
systemctl reload nftablesduring the Puppet run if manual changes to the in-memory ruleset are detected.To accomplish this, the systemd unit in charge of nftables is configured to write a hash of the in-memory ruleset right after starting/reloading. During the Puppet run, the hash of the current rule set is compared to the one previously stored. If the hash differs then
systemctl reload nftablesis executed to flush manual changes.Fixes #113