Puppet repeatedly attempts to correct firewall rules when source has a prefix length of zero

[nabertrand]

If the firewall type source has a prefix length of zero, Puppet attempts to correct the source on every run.

Environment details

• Puppet version: 7.28.0
• OS: Rocky Linux 8.9
• Module version: 7.0.2

Steps to reproduce:

# puppet resource firewall '000 allow icmp from all' proto=icmp source=0.0.0.0/0
Notice: /Firewall[000 allow icmp from all]/source: source changed  to '0.0.0.0/0'
Notice: firewall[000 allow icmp from all]: Updating: Updating Rule '000 allow icmp from all' with {:name=>"000 allow icmp from all", :source=>nil, :proto=>"icmp", :ensure=>"present", :protocol=>"IPv4", :table=>"filter", :chain=>"INPUT"}
Notice: firewall[000 allow icmp from all]: Updating: Ensuring changes to '000 allow icmp from all' persist
Notice: firewall[000 allow icmp from all]: Updating: Finished in 0.819169 seconds
firewall { '000 allow icmp from all': 
  proto => 'icmp',
  ensure => 'present',
  protocol => 'IPv4',
  table => 'filter',
  chain => 'INPUT',
}
# puppet resource firewall '000 allow icmp from all' proto=icmp source=0.0.0.0/0
Notice: /Firewall[000 allow icmp from all]/source: source changed  to '0.0.0.0/0'
Notice: firewall[000 allow icmp from all]: Updating: Updating Rule '000 allow icmp from all' with {:name=>"000 allow icmp from all", :source=>nil, :proto=>"icmp", :ensure=>"present", :protocol=>"IPv4", :table=>"filter", :chain=>"INPUT"}
Notice: firewall[000 allow icmp from all]: Updating: Ensuring changes to '000 allow icmp from all' persist
Notice: firewall[000 allow icmp from all]: Updating: Finished in 0.844126 seconds
firewall { '000 allow icmp from all': 
  proto => 'icmp',
  ensure => 'present',
  protocol => 'IPv4',
  table => 'filter',
  chain => 'INPUT',
}
...

Additional Information

When Puppet checks if source is in sync, it immediately returns nil because is is nil:

puppetlabs-firewall/lib/puppet/provider/firewall/firewall.rb

Lines 328 to 329 in 403fed3

	# If either value is nil, no custom logic is required
	return nil if is_hash[property_name].nil? || should_hash[property_name].nil?

But when Puppet goes to apply the changes, it modifies the should to be nil because of the zero prefix length:

puppetlabs-firewall/lib/puppet/provider/firewall/firewall.rb

Lines 871 to 873 in 403fed3

	# `source` and `destination` must be put through host_to_mask
	should[:source] = PuppetX::Firewall::Utility.host_to_mask(should[:source], should[:protocol]) if should[:source]
	should[:destination] = PuppetX::Firewall::Utility.host_to_mask(should[:destination], should[:protocol]) if should[:destination]

puppetlabs-firewall/lib/puppet_x/puppetlabs/firewall/utility.rb

Lines 89 to 90 in 403fed3

	# - Any address with a resulting prefix length of zero:
	# It will return nil which is equivilent to not specifying an address

Potential Fix

Allow the insync? method to process the source and destination parameters before comparing so zero prefix length values become nil:

--- lib/puppet/provider/firewall/firewall.rb   2024-02-02 12:46:57.759555325 -0600
+++ lib/puppet/provider/firewall/firewall.rb.patched   2024-02-02 13:10:22.039261534 -0600
@@ -326,6 +326,6 @@
     context.debug("Checking whether '#{property_name}' is out of sync")
 
-    # If either value is nil, no custom logic is required
-    return nil if is_hash[property_name].nil? || should_hash[property_name].nil?
+    # If either value is nil, no custom logic is required unless property is source or destination
+    return nil if (is_hash[property_name].nil? || should_hash[property_name].nil?) && ! [:source, :destination].include?(property_name)
 
     case property_name

[corporate-gadfly]

With Puppet 8.5.1, and module 8.0.0, firewall rules were not idempotent as well.

To compensate, wherever state was used, I had to replace:

state  => ['NEW'],

with

state  => 'NEW',

In addition, anywhere where I had used ranges in sport or dport, I had to replace - with :, e.g.:

dport  => '7937-9936',

with

dport  => '7937:9936',

After that, firewall rules were idempotent. In that regards PR #1189 was not necessary, in my case.

Hope that helps someone.

[corporate-gadfly]

firewallchain resources with Puppet 8.5.1 and module version 8.0.0 are behaving non-idempotent for me, at the moment.

To reproduce, try:

puppet apply -e 'firewallchain {"PREROUTING:mangle:IPv4": ensure=>"present"}'

[corporate-gadfly]

Here is the difference in debug output when executing:

puppet apply -e 'firewallchain {"PREROUTING:mangle:IPv4": ensure=>"present"}' --debug

With firewall v6.0.0:

Debug: Found in cache :production (ttl = 0 sec)
Debug: /Firewallchain[PREROUTING:mangle:IPv4]: [validate]
Debug: Creating default schedules
Debug: Loaded state in 0.02 seconds
Info: Using environment 'production'
Debug: Loaded state in 0.02 seconds
Info: Applying configuration version '1710342690'
Debug: Prefetching iptables_chain resources for firewallchain
Debug: Puppet::Type::Firewallchain::ProviderIptables_chain: [prefetch(resources)]
Debug: Puppet::Type::Firewallchain::ProviderIptables_chain: [instances]
Debug: Executing: '/sbin/iptables-save'
Debug: Puppet::Type::Firewallchain::ProviderIptables_chain: [instance] 'INPUT:filter:IPv4' accept
Debug: Puppet::Type::Firewallchain::ProviderIptables_chain: [instance] 'FORWARD:filter:IPv4' accept
Debug: Puppet::Type::Firewallchain::ProviderIptables_chain: [instance] 'OUTPUT:filter:IPv4' accept
Debug: Executing: '/sbin/ip6tables-save'
Debug: Executing: '/sbin/ebtables-save'
Debug: Finishing transaction 12460
Debug: Storing state

With firewall v8.0.0:

Debug: Found in cache :production (ttl = 0 sec)
Debug: Creating default schedules
Debug: Loaded state in 0.03 seconds
Info: Using environment 'production'
Debug: Loaded state in 0.02 seconds
Debug: Executing: 'iptables-save'
Debug: Executing: 'ip6tables-save'
Info: Applying configuration version '1710342515'
Debug: Current State: {:title=>"PREROUTING:mangle:IPv4", :ensure=>:absent}
Debug: firewallchain: Checking whether 'ensure' is out of sync
Notice: /Stage[main]/Main/Firewallchain[PREROUTING:mangle:IPv4]/ensure: defined 'ensure' as 'present'
Debug: Target State: {:name=>"PREROUTING:mangle:IPv4", :ensure=>"present", :purge=>false, :ignore_foreign=>false}
Debug: firewallchain[PREROUTING:mangle:IPv4]: Updating: Start
Notice: firewallchain[PREROUTING:mangle:IPv4]: Updating: Finished in 0.000071 seconds
Debug: /Stage[main]/Main/Firewallchain[PREROUTING:mangle:IPv4]: The container Class[Main] will propagate my refresh event
Debug: Class[Main]: The container Stage[main] will propagate my refresh event
Debug: Finishing transaction 12600
Debug: Storing state

[corporate-gadfly]

@Ramesh7 : Any chance this issue can be looked at? I'm not sure how specs work in this module, but shouldn't idempotent_apply calls catch these issues?

[Ramesh7]

@Ramesh7 : Any chance this issue can be looked at? I'm not sure how specs work in this module, but shouldn't idempotent_apply calls catch these issues?

Hey @corporate-gadfly, sorry for delay in response, I will have look in upcoming week on idempotent_apply.

[Ramesh7]

Hi @corporate-gadfly, I have tried on RHEL/Ubuntu and was able to reproduce the issue with given steps. Have dig more into that and here is my observations :

• The firewall module uses ip(6)tables-save command to check the current set of chains
• The expectation of command output is to return all default firewall chains but it return nothing

# iptables-save
#

• So when the firewallchain resource converging (applying) it checks for current state, as the command returns no output so it assumes the chains are absent and it tries to create it on every run.
• So the issue seems to be with command which doesn't return the expected output.

Later I tried to create a random chain and re-run the iptables-save command, it return respective table's chain :

• Creating random chain

# puppet apply -e 'firewallchain {"TEST:mangle:IPv4": ensure=>"present"}'
Notice: Compiled catalog for litmus-e6912e0100115ac3.c.ia-content.internal in environment production in 0.05 seconds
Notice: /Stage[main]/Main/Firewallchain[TEST:mangle:IPv4]/ensure: defined 'ensure' as 'present'
Notice: firewallchain[TEST:mangle:IPv4]: Creating: Creating Chain 'TEST:mangle:IPv4' with {:name=>"TEST:mangle:IPv4", :ensure=>"present", :purge=>false, :ignore_foreign=>false, :chain=>"TEST", :table=>"mangle", :protocol=>"IPv4"}
Notice: firewallchain[TEST:mangle:IPv4]: Creating: Ensuring changes to 'TEST:mangle:IPv4' persist
Unable to persist firewall rules: Execution of '/usr/libexec/iptables/iptables.init save' returned 1: Error: Could not execute posix command: No such file or directory - /usr/libexec/iptables/iptables.init
Notice: firewallchain[TEST:mangle:IPv4]: Creating: Finished in 0.014551 seconds
Notice: Applied catalog in 0.04 seconds

After creating custom chain tried running same command :

# iptables-save
# Generated by iptables-save v1.8.10 (nf_tables) on Tue May 28 13:19:36 2024
*mangle
:PREROUTING ACCEPT [0:0]
:INPUT ACCEPT [0:0]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
:POSTROUTING ACCEPT [0:0]
:TEST - [0:0]
COMMIT
# Completed on Tue May 28 13:19:36 2024

Then tried rerunning same manifest which turn out to be idempotent :

# puppet apply -e 'firewallchain {"PREROUTING:mangle:IPv4": ensure=>"present"}'
Notice: Compiled catalog for litmus-e6912e0100115ac3.c.ia-content.internal in environment production in 0.05 seconds
Notice: Applied catalog in 0.04 seconds
...
# puppet apply -e 'firewallchain {"TEST:mangle:IPv4": ensure=>"present"}'
Notice: Compiled catalog for litmus-e6912e0100115ac3.c.ia-content.internal in environment production in 0.05 seconds
Notice: Applied catalog in 0.04 seconds

Later I tried same on CentOS where have observed things are working as expected.

Conclusion :

• I think the behaviour of ip(6)tables-save misleading the current state of machine because of that firewallchain tries to create it and makes firewallchain resource non-i idempotent
• For some machines (CentOS) have observed things are working fine
• Don't think so we need to create the default chains, as these are created when the package gets installed. So we can ignore default chain and can create custom chain if needed.

Please let me know if any questions or thoughts?
Thanks

[nabertrand]

@Ramesh7 thanks for looking into this, but I don't think @corporate-gadfly's firewallchain issue is related to the original PR issue of zero-prefix-length source or destination parameters on firewall resources causing the resource to be corrected on every agent run. Just wanted to make sure the original issue was also being looked into.

[trevorrea]

@Ramesh7 Is there any update on this please? As per what @nabertrand said above this issue is separate to the issue that was fixed at #1206

[trevorrea]

I tested the latest version of 8.x today and this issue is still occurring. Can it be looked at and fixed please? There's an MR at #1189

[corporate-gadfly]

Coming back to this issue, which still exists in the 8.x versions on Ubuntu 22, I asked, horror of all horrors, ChatGPT if there was a way to have the rules in the default tables show up.

The helpful hint was to add placeholder rules and then remove them. E.g.:

iptables -t nat -A POSTROUTING -j ACCEPT

followed by:

iptables -t nat -D POSTROUTING 1

After this, iptables-save showed me both filter and nat tables in the output.

Now, I know zilch about the nat tables, let alone the chains and the default rules inside them. Could someone more knowledgeable comment on the above hint.

Also, for reference, there is a netfilter bugzilla which mentions the sparseness of iptables-save with respect to default tables.

[trevorrea]

I did this as a workaround (I only have a few rules so it's not a big deal)

profile::firewall::rules:
  '100 allow HTTP access':
    jump: 'accept'
    dport: '80'
    proto: 'tcp'
    source: '0.0.0.0/1' # Workaround for https://github.com/puppetlabs/puppetlabs-firewall/issues/1188
  '101 allow HTTP access':
    jump: 'accept'
    dport: '80'
    proto: 'tcp'
    source: '128.0.0.0/1' # Workaround for https://github.com/puppetlabs/puppetlabs-firewall/issues/1188

[trevorrea]

MR at #1189 has finally been merged so hopefully a release will be forthcoming also.

[corporate-gadfly]

@trevorrea : 8.1.3 has the fix. Are you able to test? My puppet control repository branch structure is preventing me from trying this, at the moment.