Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide a mechanism to flush un-managed rules #115

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Provide a mechanism to flush un-managed rules #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
78d8286
Provide a mechanism to flush un-managed rules
nbarrientos Dec 8, 2021
93077e9
Use /run/puppet-nft-memhash as default path
nbarrientos Dec 8, 2021
95636ba
allow_unmanaged_rules -> purge_unmanaged_rules
nbarrientos Dec 8, 2021
3f640a7
Reword sentence
nbarrientos Dec 8, 2021
019bde3
Look for literal dots
nbarrientos Dec 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@ Initially puppet deploys all configuration to
If and only if successful the configuration will be copied to
the real locations before the service is reloaded.

## Un-managed rules

By default, rules added manually by the administrator to the in-memory
ruleset will be left untouched. However,
`nftables::purge_unmanaged_rules` can be set to `true` to revert this
behaviour and force a reload of the ruleset during the Puppet run if
non-managed changes are detected.

## Basic types

### nftables::config
Expand Down
5 changes: 5 additions & 0 deletions files/systemd/nft-hash-ruleset.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/bin/bash
# This file is Puppet managed

nbarrientos marked this conversation as resolved.
Show resolved Hide resolved
umask 0377
/sbin/nft -s list ruleset | /usr/bin/sha1sum > $1
7 changes: 0 additions & 7 deletions files/systemd/puppet_nft.conf
View file
Open in desktop

This file was deleted.

36 changes: 35 additions & 1 deletion manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,18 @@
# @param nat
# Add default tables and chains to process NAT traffic.
#
# @param purge_unmanaged_rules
# Prohibits in-memory rules that are not declared in Puppet
# code. Setting this to true activates a check that reloads nftables
# if the rules in memory have been modified outwith Puppet.
#
# @param nat_table_name
# The name of the 'nat' table.
#
# @param inmem_rules_hash_file
# The name of the file where the hash of the in-memory rules
# will be stored.
#
# @param sets
# Allows sourcing set definitions directly from Hiera.
#
Expand Down Expand Up @@ -99,10 +108,12 @@
Boolean $fwd_conntrack = false,
Boolean $inet_filter = true,
Boolean $nat = true,
Boolean $purge_unmanaged_rules = false,
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',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Variant[Boolean[false], String] $log_limit = '3/minute burst 5 packets',
Variant[Boolean[false], Pattern[/icmp(v6|x)? type .+|tcp reset/]] $reject_with = 'icmpx type port-unreachable',
Variant[Boolean[false], Enum['mask']] $firewalld_enable = 'mask',
Expand Down Expand Up @@ -164,10 +175,33 @@
restart => '/usr/bin/systemctl reload nftables',
}

if $purge_unmanaged_rules {
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})\"",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we added --json to the dump filtering out the noflush chains to the dump should be doable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would certainly use both options if they were not mutually exclusive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
fi

We'd have to think here if the boostrapping makes sense as $facts['nftables']['tables'] won't be available during the first run.

require => Service['nftables'],
}

file { '/usr/local/sbin/nft-hash-ruleset.sh' :
ensure => file,
mode => '0755',
content => file('nftables/systemd/nft-hash-ruleset.sh'),
before => Systemd::Dropin_file['puppet_nft.conf'],
}
} else {
file { $inmem_rules_hash_file:
ensure => absent,
}
}

systemd::dropin_file { 'puppet_nft.conf':
ensure => present,
unit => 'nftables.service',
content => file('nftables/systemd/puppet_nft.conf'),
content => epp('nftables/systemd/puppet_nft.conf.epp', {
'purge_unmanaged' => $purge_unmanaged_rules,
'hash_file' => $inmem_rules_hash_file,
}),
notify => Service['nftables'],
}

Expand Down
38 changes: 38 additions & 0 deletions spec/classes/nftables_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@
expect(subject).to contain_systemd__dropin_file('puppet_nft.conf').with(
content: %r{^ExecReload=/sbin/nft -I /etc/nftables/puppet -f /etc/sysconfig/nftables.conf$}
)
is_expected.not_to contain_systemd__dropin_file('puppet_nft.conf').with(
content: %r{^ExecReload=.*nft-hash-ruleset\.sh.*$}
)
is_expected.not_to contain_systemd__dropin_file('puppet_nft.conf').with(
content: %r{^ExecStartPost.*$}
)
}

it {
Expand All @@ -94,6 +100,9 @@
it { is_expected.to contain_class('nftables::rules::out::chrony') }
it { is_expected.not_to contain_class('nftables::rules::out::all') }
it { is_expected.not_to contain_nftables__rule('default_out-all') }
it { is_expected.not_to contain_exec('Reload nftables if there are un-managed rules') }
it { is_expected.to contain_file('/run/puppet-nft-memhash') }
it { is_expected.not_to contain_file('/usr/local/sbin/nft-hash-ruleset.sh') }

context 'with out_all set true' do
let(:params) do
Expand Down Expand Up @@ -207,6 +216,35 @@
it { is_expected.to have_nftables__set_resource_count(0) }
end

context 'with not allowing un-managed changes' do
let(:params) do
{
'purge_unmanaged_rules' => true,
'inmem_rules_hash_file' => '/foo/bar',
}
end

it { is_expected.not_to contain_file('/foo/bar') }
it { is_expected.to contain_file('/usr/local/sbin/nft-hash-ruleset.sh') }
it {
is_expected.to contain_exec('Reload nftables if there are un-managed rules').with(
command: '/usr/bin/systemctl reload nftables',
refreshonly: false,
unless: '/usr/bin/test -s /foo/bar -a "$(nft -s list ruleset | sha1sum)" = "$(cat /foo/bar)"'
)
}
it {
is_expected.to contain_systemd__dropin_file('puppet_nft.conf').with(
content: %r{^ExecReload=/bin/bash /usr/local/sbin/nft-hash-ruleset\.sh /foo/bar$}
)
}
it {
is_expected.to contain_systemd__dropin_file('puppet_nft.conf').with(
content: %r{^ExecStartPost=/bin/bash /usr/local/sbin/nft-hash-ruleset\.sh /foo/bar$}
)
}
end

context 'with with noflush_tables parameter' do
let(:params) do
{
Expand Down
16 changes: 16 additions & 0 deletions templates/systemd/puppet_nft.conf.epp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<%- |
Boolean $purge_unmanaged,
Stdlib::Unixpath $hash_file,
|-%>
# Puppet Deployed
[Service]
ExecStart=
ExecStart=/sbin/nft -I /etc/nftables/puppet -f /etc/sysconfig/nftables.conf
<% if $purge_unmanaged { -%>
ExecStartPost=/bin/bash /usr/local/sbin/nft-hash-ruleset.sh <%= $hash_file %>
<% } -%>
ExecReload=
ExecReload=/sbin/nft -I /etc/nftables/puppet -f /etc/sysconfig/nftables.conf
<% if $purge_unmanaged { -%>
ExecReload=/bin/bash /usr/local/sbin/nft-hash-ruleset.sh <%= $hash_file -%>
<% } -%>