Merge pull request #329 from bmagistro/bugfix-193-rich-rule-reject-type

Fix rich rule with typed action
voxpupuli · Aug 24, 2023 · fd1173c · fd1173c
2 parents 321acce + 19f4b15
commit fd1173c
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 11 deletions.
diff --git a/lib/puppet/provider/firewalld_rich_rule/firewall_cmd.rb b/lib/puppet/provider/firewalld_rich_rule/firewall_cmd.rb
@@ -113,8 +113,8 @@ def eval_action
     return [] unless (action = @resource[:action])
     args = []
     if action.is_a?(Hash)
-      args << action[:action]
-      args << quote_keyval('type', action[:type])
+      args << action['action']
+      args << quote_keyval('type', action['type'])
     else
       args << action
     end

diff --git a/lib/puppet/type/firewalld_rich_rule.rb b/lib/puppet/type/firewalld_rich_rule.rb
@@ -122,10 +122,10 @@ def _validate_action(value)
     end
     validate do |value|
       if value.is_a?(Hash)
-        if value.keys.sort != [:action, :type]
+        if value.keys.sort != ['action', 'type']
           raise Puppet::Error, "Rule action hash should contain `action` and `type` keys. Use a string if you only want to declare the action to be `accept` or `reject`. Got #{value}"
         end
-        _validate_action(value[:action])
+        _validate_action(value['action'])
       elsif value.is_a?(String)
         _validate_action(value)
       end

diff --git a/spec/unit/puppet/provider/firewalld_rich_rule_spec.rb b/spec/unit/puppet/provider/firewalld_rich_rule_spec.rb
@@ -60,7 +60,7 @@
         resource.expects(:[]).with(:log).returns(nil)
         resource.expects(:[]).with(:audit).returns(nil)
         resource.expects(:[]).with(:raw_rule).returns(nil)
-        resource.expects(:[]).with(:action).returns(action: 'reject', type: 'icmp-admin-prohibited')
+        resource.expects(:[]).with(:action).returns('action' => 'reject', 'type' => 'icmp-admin-prohibited')
         expect(provider.build_rich_rule).to eq('rule family="ipv4" destination address="192.168.0.1/32" service name="ssh" reject type="icmp-admin-prohibited"')
       end
     end

diff --git a/spec/unit/puppet/type/firewalld_rich_rule_spec.rb b/spec/unit/puppet/type/firewalld_rich_rule_spec.rb
@@ -42,15 +42,15 @@
       expect do
         described_class.new(
           title: 'SSH from barny',
-          action: { type: 'accepted', foo: 'bar' }
+          action: { 'type' => 'accepted', 'foo' => 'bar' }
         )
       end.to raise_error(%r{Rule action hash should contain `action` and `type` keys. Use a string if you only want to declare the action to be `accept` or `reject`})
     end
     it 'raises an error if wrong action hash values' do
       expect do
         described_class.new(
           title: 'SSH from barny',
-          action: { type: 'icmp-admin-prohibited', action: 'accepted' }
+          action: { 'type' => 'icmp-admin-prohibited', 'action' => 'accepted' }
         )
       end.to raise_error(%r{Authorized action values are `accept`, `reject`, `drop` or `mark`})
     end
@@ -219,7 +219,31 @@
         icmp_type: 'echo',
         log: { 'level' => 'debug' },
         action: 'accept'
-      } => 'rule family="ipv4" destination address="10.0.1.2/24" icmp-type name="echo" log level="debug" accept'
+      } => 'rule family="ipv4" destination address="10.0.1.2/24" icmp-type name="echo" log level="debug" accept',
+
+      ## test reject
+      {
+        name: 'reject ssh',
+        ensure: 'present',
+        family: 'ipv4',
+        zone: 'restricted',
+        source: { 'address' => '10.0.1.2/24' },
+        service: 'ssh',
+        log: { 'level' => 'debug' },
+        action: 'reject'
+      } => 'rule family="ipv4" source address="10.0.1.2/24" service name="ssh" log level="debug" reject',
+
+      ## test reject + type (#193)
+      {
+        name: 'reject ssh tcp reset',
+        ensure: 'present',
+        family: 'ipv4',
+        zone: 'restricted',
+        source: { 'address' => '10.0.1.2/24' },
+        service: 'ssh',
+        log: { 'level' => 'debug' },
+        action: { 'action' => 'reject', 'type' => 'tcp-reset' }
+      } => 'rule family="ipv4" source address="10.0.1.2/24" service name="ssh" log level="debug" reject type="tcp-reset"',
 
     }
 
@@ -230,9 +254,6 @@
         end
         let(:fakeclass) { Class.new }
         let(:provider) { resource.provider }
-        let(:rawrule) do
-          'rule family="ipv4" source address="10.0.1.2/24" service name="ssh" log level="debug" accept'
-        end
 
         it 'queries the status' do
           fakeclass.stubs(:exitstatus).returns(0)