Merge pull request #1030 from puppetlabs/SEC-944-handle_duplicate_rule_comments_v2

(SEC-944) Handle duplicate system rules

Author: david22swan

README.md

@@ -14,7 +14,10 @@
 4. [Usage - Configuration and customization options](#usage)
     * [Default rules - Setting up general configurations for all firewalls](#default-rules)
     * [Application-specific rules - Options for configuring and managing firewalls across applications](#application-specific-rules)
-    * [Additional ses for the firewall module](#other-rules)
+    * [Rule inversion](#rule-inversion)
+    * [Additional uses for the firewall module](#additional-uses-for-the-firewal-module)
+    * [Duplicate rule behaviour](#duplicate-rule-behaviour)
+    * [Additional information](#additional-information)
 5. [Reference - An under-the-hood peek at what the module is doing](#reference)
 6. [Limitations - OS compatibility, etc.](#limitations)
 7. [Firewall_multi - Arrays for certain parameters](#firewall_multi)
@@ -387,6 +390,21 @@ firewall {'666 for NFLOG':
 }
 ```
 
+### Duplicate rule behaviour
+
+It is possible for an unmanaged rule to exist on the target system that has the same comment as the rule specified in the manifest. This configuration is not supported by the firewall module.
+
+In the event of a duplicate rule, the module will by default display a warning message notifying the user that it has found a duplicate but will continue to update the resource.
+
+This behaviour is configurable via the `onduplicaterulebehaviour` parameter. Users can choose from the following behaviours:
+
+* `ignore` - The duplicate rule is ignored and any updates to the resource will continue unaffected.
+* `warn` - The duplicate rule is logged as a warning and any updates to the resource will continue unaffected.
+* `error` - The duplicate rule is logged as an error and any updates to the resource will be skipped.
+
+With either the `ignore` or `warn` (default) behaviour, Puppet may create another duplicate rule.
+To prevent this behavior and report the resource as failing during the Puppet run, specify the `error` behaviour.
+
 ### Additional information
 
 Access the inline documentation:

lib/puppet/provider/firewall/iptables.rb

@@ -401,9 +401,29 @@ def delete
   end
 
   def exists?
+    if duplicate_rule?(@property_hash[:name])
+      core_message = "Duplicate rule found for #{@property_hash[:name]}."
+      case @resource.parameters[:onduplicaterulebehaviour].value
+      when :error
+        raise   "#{core_message} Skipping update."
+      when :warn
+        warning "#{core_message}. This may add an additional rule to the system."
+      when :ignore
+        debug "#{core_message}. This may add an additional rule to the system."
+      end
+    end
+
     properties[:ensure] != :absent
   end
 
+  def duplicate_rule?(rule)
+    rules = self.class.instances
+    system_rule_count = Hash.new(0)
+    rules.each { |r| system_rule_count[r.name] += 1 }
+    duplicate_rules = rules.select { |r| system_rule_count[r.name] > 1 }
+    duplicate_rules.select { |r| r.name == rule }.any?
+  end
+
   # Flush the property hash once done.
   def flush
     debug('[flush]')

lib/puppet/type/firewall.rb

@@ -234,6 +234,24 @@
     newvalues(%r{^\d+[[:graph:][:space:]]+$})
   end
 
+  newparam(:onduplicaterulebehaviour) do
+    desc <<-PUPPETCODE
+      In certain situations it is possible for an unmanaged rule to exist
+      on the target system that has the same comment as the rule
+      specified in the manifest.
+
+      This setting determines what happens when such a duplicate is found.
+
+      It offers three options:
+
+        * ignore - The duplicate rule is ignored and any updates to the resource will continue unaffected.
+        * warn - The duplicate rule is logged as a warning and any updates to the resource will continue unaffected.
+        * error - The duplicate rule is logged as an error and any updates to the resource will be skipped.
+    PUPPETCODE
+    newvalues(:ignore, :warn, :error)
+    defaultto :warn
+  end
+
   newproperty(:action) do
     desc <<-PUPPETCODE
       This is the action to perform on a match. Can be one of:

spec/acceptance/firewall_duplicate_comment_spec.rb

@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+require 'spec_helper_acceptance'
+
+def make_manifest(behaviour)
+  pp = <<-PUPPETCODE
+    class { 'firewall': }
+    resources { 'firewall':
+      purge => true,
+    }
+
+    firewall { '550 destination':
+      proto  => tcp,
+      dport   => '550',
+      action => accept,
+      destination => '192.168.2.0/24',
+      onduplicaterulebehaviour => #{behaviour}
+    }
+    PUPPETCODE
+
+  pp
+end
+
+describe 'firewall - duplicate comments' do
+  before(:all) do
+    if os[:family] == 'ubuntu' || os[:family] == 'debian'
+      update_profile_file
+    end
+  end
+
+  before(:each) do
+    run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"')
+  end
+
+  after(:each) do
+    iptables_flush_all_tables
+  end
+
+  context 'when onduplicateerrorhevent is set to error' do
+    it 'raises an error' do
+      run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"')
+      pp = make_manifest('error')
+
+      apply_manifest(pp) do |r|
+        expect(r.stderr).to include('Error: /Stage[main]/Main/Firewall[550 destination]: Could not evaluate: Duplicate rule found for 550 destination. Skipping update.')
+      end
+    end
+  end
+
+  context 'when onduplicateerrorhevent is set to warn' do
+    run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"')
+
+    it 'warns and continues' do
+      run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"')
+      pp = make_manifest('warn')
+
+      apply_manifest(pp) do |r|
+        expect(r.stderr).to include('Warning: Firewall[550 destination](provider=iptables): Duplicate rule found for 550 destination.. This may add an additional rule to the system.')
+      end
+    end
+  end
+
+  context 'when onduplicateerrorhevent is set to ignore' do
+    run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"')
+
+    it 'continues silently' do
+      run_shell('iptables -I INPUT -m state --state NEW -m tcp -p tcp --dport 551 -j ACCEPT -m comment --comment "550 destination"')
+      pp = make_manifest('ignore')
+
+      apply_manifest(pp) do |r|
+        expect(r.stderr).to be_empty
+      end
+    end
+  end
+end

spec/spec_helper_acceptance_local.rb

@@ -111,5 +111,12 @@ def fetch_os_name
       }
     PUPPETCODE
     LitmusHelper.instance.apply_manifest(pp)
+
+    # Ensure that policycoreutils is present. In the future we could probably refactor
+    # this so that policycoreutils is installed on platform where the os.family fact
+    # is set to 'redhat'
+    if ['almalinux-8', 'rocky-8'].include?("#{fetch_os_name}-#{os[:release].to_i}")
+      LitmusHelper.instance.run_shell('yum install policycoreutils -y')
+    end
   end
 end