From fd2c345d0d3b2b5a0c088fcbc49690dd529bd29a Mon Sep 17 00:00:00 2001 From: Jean-Francois Roche Date: Fri, 20 Apr 2018 14:11:43 +0200 Subject: [PATCH] Add validation for rich rule action It can be a string or a hash. We validate string content and hash content. +tests --- .../firewalld_rich_rule/firewall_cmd.rb | 4 +- lib/puppet/type/firewalld_rich_rule.rb | 3 +- .../firewalld/property/rich_rule_action.rb | 21 +++++++ .../provider/firewalld_rich_rule_spec.rb | 62 +++++++++++++++++++ .../puppet/type/firewalld_rich_rule_spec.rb | 22 +++++++ 5 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 lib/puppet_x/firewalld/property/rich_rule_action.rb create mode 100644 spec/unit/puppet/provider/firewalld_rich_rule_spec.rb diff --git a/lib/puppet/provider/firewalld_rich_rule/firewall_cmd.rb b/lib/puppet/provider/firewalld_rich_rule/firewall_cmd.rb index 2cb078d6..451c1faa 100644 --- a/lib/puppet/provider/firewalld_rich_rule/firewall_cmd.rb +++ b/lib/puppet/provider/firewalld_rich_rule/firewall_cmd.rb @@ -117,8 +117,8 @@ def build_rich_rule eval_audit, eval_action, ] - @resource[:raw_rule] = rule.flatten.reject { |r| r.empty? }.join(" ") - @resource[:raw_rule] + @resource[:raw_rule] = raw_rule = rule.flatten.reject { |r| r.empty? }.join(" ") + raw_rule end def create diff --git a/lib/puppet/type/firewalld_rich_rule.rb b/lib/puppet/type/firewalld_rich_rule.rb index 5f62ff45..73c6365b 100644 --- a/lib/puppet/type/firewalld_rich_rule.rb +++ b/lib/puppet/type/firewalld_rich_rule.rb @@ -1,4 +1,5 @@ require 'puppet' +require_relative '../../puppet_x/firewalld/property/rich_rule_action' Puppet::Type.newtype(:firewalld_rich_rule) do @@ -99,7 +100,7 @@ desc "doc" end - newparam(:action) do + newparam(:action, :parent => PuppetX::Firewalld::Property::RichRuleAction) do desc "doc" end diff --git a/lib/puppet_x/firewalld/property/rich_rule_action.rb b/lib/puppet_x/firewalld/property/rich_rule_action.rb new file mode 100644 index 00000000..5849ca9e --- /dev/null +++ b/lib/puppet_x/firewalld/property/rich_rule_action.rb @@ -0,0 +1,21 @@ +module PuppetX + module Firewalld + module Property + class RichRuleAction < Puppet::Property + def _validate_action(value) + raise Puppet::Error, "Authorized action values are `accept`, `reject`, `drop` or `mark`, got #{value}" unless %w[accept drop reject mark].include? value + end + validate do |value| + if value.is_a?(Hash) + if value.keys.sort != %i[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]) + elsif value.is_a?(String) + _validate_action(value) + end + end + end + end + end +end diff --git a/spec/unit/puppet/provider/firewalld_rich_rule_spec.rb b/spec/unit/puppet/provider/firewalld_rich_rule_spec.rb new file mode 100644 index 00000000..b5512e4f --- /dev/null +++ b/spec/unit/puppet/provider/firewalld_rich_rule_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +provider_class = Puppet::Type.type(:firewalld_rich_rule).provider(:firewall_cmd) + +describe provider_class do + let(:resource) do + @resource = Puppet::Type.type(:firewalld_rich_rule).new( + ensure: :present, + name: 'Accept ssh from barny', + zone: 'restricted', + service: 'ssh', + source: '192.168.1.2/32', + action: 'accept', + provider: described_class.name + ) + end + let(:provider) { resource.provider } + + before :each do + provider.class.stubs(:execute_firewall_cmd).returns(Object.any_instance.stubs(:exitstatus => 0)) + provider.class.stubs(:execute_firewall_cmd).with(['--list-interfaces']).returns(Object.any_instance.stubs(:exitstatus => 0, :chomp => "")) + end + + describe 'when creating' do + context 'with basic parameters' do + it "should build the rich rule" do + resource.expects(:[]).with(:source).returns('192.168.1.2/32').at_least_once + resource.expects(:[]).with(:service).returns('ssh').at_least_once + resource.expects(:[]).with("family").returns('ipv4').at_least_once + resource.expects(:[]).with(:dest).returns(nil) + resource.expects(:[]).with(:port).returns(nil) + resource.expects(:[]).with(:protocol).returns(nil) + resource.expects(:[]).with(:icmp_block).returns(nil) + resource.expects(:[]).with(:masquerade).returns(nil) + resource.expects(:[]).with(:forward_port).returns(nil) + resource.expects(:[]).with(:log).returns(nil) + resource.expects(:[]).with(:audit).returns(nil) + resource.expects(:[]).with(:raw_rule).returns(nil) + resource.expects(:[]).with(:action).returns('accept') + expect(provider.build_rich_rule).to eq("rule family=\"ipv4\" source service name=\"ssh\" accept") + end + end + context 'with reject type' do + it "should build the rich rule" do + resource.expects(:[]).with(:source).returns(nil).at_least_once + resource.expects(:[]).with(:service).returns('ssh').at_least_once + resource.expects(:[]).with("family").returns('ipv4').at_least_once + resource.expects(:[]).with(:dest).returns({'address' => '192.168.0.1/32'}) + resource.expects(:[]).with(:port).returns(nil) + resource.expects(:[]).with(:protocol).returns(nil) + resource.expects(:[]).with(:icmp_block).returns(nil) + resource.expects(:[]).with(:masquerade).returns(nil) + resource.expects(:[]).with(:forward_port).returns(nil) + 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'}) + expect(provider.build_rich_rule).to eq("rule family=\"ipv4\" destination service name=\"ssh\" reject type=\"icmp-admin-prohibited\"") + end + end + end +end diff --git a/spec/unit/puppet/type/firewalld_rich_rule_spec.rb b/spec/unit/puppet/type/firewalld_rich_rule_spec.rb index f8e4134c..93cdf02d 100644 --- a/spec/unit/puppet/type/firewalld_rich_rule_spec.rb +++ b/spec/unit/puppet/type/firewalld_rich_rule_spec.rb @@ -28,6 +28,28 @@ end end + describe 'action validation' do + it 'raises an error if wrong action string' do + expect do described_class.new( + title: 'SSH from barny', + action: 'accepted', + ) end.to raise_error(/Authorized action values are `accept`, `reject`, `drop` or `mark`/) + end + it 'raises an error if wrong action hash keys' do + expect do described_class.new( + title: 'SSH from barny', + action: {'type' => 'accepted', 'foo' => 'bar'}, + ) end.to raise_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`/) + 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'}, + ) end.to raise_error(/Authorized action values are `accept`, `reject`, `drop` or `mark`/) + end + + end + describe 'namevar validation' do let(:attrs) {{ :title => 'SSH from barny',