From 4d64dc0ee27a4b8b2e3c8ef5ed8bfb86e5d23acc Mon Sep 17 00:00:00 2001 From: Ken Barber Date: Mon, 10 Oct 2011 08:11:27 +0100 Subject: [PATCH] (#9362) Create action property and perform transformation for accept, drop, reject value for iptables jump parameter. This commit introduces the new 'action' parameter which is designed to designate the action to take when a match succeeds. This is a cross-platform parameter and for the values 'accept','drop','reject' it will take the place of the existing jump parameter. The jump parameter is deemed as an iptables specific parameter so by splitting out this parameter for common actions it allows us to extend the firewall resource to include other providers much more easily in the future. By having such a common parameter we will be able to compare resources between boxes that may have different firewall implementations. The new behaviour is to force the usage for action parameter, and using 'accept', 'drop' or 'reject' for jump will now no longer work. Also - the default of 'accept' for jump has been removed which means you MUST specify an action if you want your rule to do something. Without an action the rule will match, but do nothing (so only useful for keeping counters generally). To aid in the testing of this new property I've added new ways to test converting iptables rules to hashes and hashes to general_args. This should simplify the testing of new bugs as well. --- README.markdown | 41 ++++--- examples/iptables/test.pp | 32 +++--- lib/puppet/provider/firewall.rb | 1 + lib/puppet/provider/firewall/iptables.rb | 41 +++++-- lib/puppet/type/firewall.rb | 36 ++++-- spec/fixtures/iptables/conversion_hash.rb | 102 +++++++++++++++++ spec/unit/puppet/provider/iptables_spec.rb | 125 +++++++++++++-------- spec/unit/puppet/type/firewall_spec.rb | 56 +++++++-- 8 files changed, 330 insertions(+), 104 deletions(-) create mode 100644 spec/fixtures/iptables/conversion_hash.rb diff --git a/README.markdown b/README.markdown index fa2b7cb..5803df1 100644 --- a/README.markdown +++ b/README.markdown @@ -35,13 +35,13 @@ Basic accept ICMP request example: firewall { "000 accept all icmp requests": proto => "icmp", - jump => "ACCEPT", + action => "accept", } Deny all: - firewall { "999 deny all other requests": - jump => "DENY", + firewall { "999 drop all other requests": + action => "drop", } Source NAT example (perfect for a virtualization host): @@ -72,18 +72,18 @@ If you wish to ensure any reject rules are executed last, try using stages. The following example shows the creation of a class which is where your last rules should run, this however should belong in a puppet module. - class my_fw::deny { - iptables { "999 deny all": - jump => "DENY" + class my_fw::drop { + iptables { "999 drop all": + action => "drop" } } stage { pre: before => Stage[main] } stage { post: require => Stage[main] } - class { "my_fw::deny": stage => "post" } + class { "my_fw::drop": stage => "post" } -By placing the 'my_fw::deny' class in the post stage it will always be inserted +By placing the 'my_fw::drop' class in the post stage it will always be inserted last thereby avoiding locking you out before the accept rules are inserted. ### Supported firewalls @@ -120,6 +120,17 @@ common practice to prefix all rules with numbers to force ordering. For example: This will occur very early. +#### action + +This is the action to perform on a match. Can be one of: + +* accept - the packet is accepted +* reject - the packet is rejected with a suitable ICMP response +* drop - the packet is dropped + +If you specify no value it will simply match the rule but perform no +action unless you provide a provider specific parameter (such as 'jump'). + #### proto Protocol to filter. By default this is 'tcp'. @@ -174,22 +185,24 @@ By default the setting is 'filter'. #### jump -Action to perform when filter is matched. Can be one of: +Action to perform when filter is matched for iptables. Can be one of: -* ACCEPT -* DROP * QUEUE * RETURN -* REJECT * DNAT * SNAT * LOG * MASQUERADE * REDIRECT -Or this can be a user defined chain. +But any valid chain name is allowed. + +For the values ACCEPT, DROP and REJECT you must use the generic +'action' parameter. This is to enfore the use of generic parameters where +possible for maximum cross-platform modelling. -The default value is 'ACCEPT'. +If you set both 'accept' and 'jump' parameters, you will get an error as +only one of the options should be set. ### Interface Matching Properties diff --git a/examples/iptables/test.pp b/examples/iptables/test.pp index 57eec86..bb8921f 100644 --- a/examples/iptables/test.pp +++ b/examples/iptables/test.pp @@ -1,11 +1,11 @@ firewall { '000 allow foo': dport => [7061, 7062], - jump => "ACCEPT", + action => accept, proto => "tcp", } firewall { '001 allow boo': - jump => "ACCEPT", + action => accept, iniface => "eth0", sport => "123", dport => "123", @@ -24,84 +24,84 @@ firewall { '100 snat for network foo2': } firewall { '999 bar': + action => accept, dport => "1233", proto => "tcp", - jump => "DROP", } firewall { '002 foo': + action => drop, dport => "1233", proto => "tcp", - jump => "DROP", } firewall { '010 icmp': + action => accept, proto => "icmp", icmp => "echo-reply", - jump => "ACCEPT", } firewall { '010 INPUT allow loopback': + action => accept, iniface => 'lo', chain => 'INPUT', - jump => 'ACCEPT' } firewall { '005 INPUT disregard DHCP': + action => drop, dport => ['bootpc', 'bootps'], - jump => 'DROP', proto => 'udp' } firewall { '006 INPUT disregard netbios': + action => drop, proto => 'udp', dport => ['netbios-ns', 'netbios-dgm', 'netbios-ssn'], - jump => 'DROP' } firewall { '006 Disregard CIFS': + action => drop, dport => 'microsoft-ds', - jump => 'DROP', proto => 'tcp' } firewall { '050 INPUT drop invalid': + action => drop, state => 'INVALID', - jump => 'DROP' } firewall { '051 INPUT allow related and established': + action => accept, state => ['RELATED', 'ESTABLISHED'], - jump => 'ACCEPT' } firewall { '053 INPUT allow ICMP': + action => accept, icmp => '8', proto => 'icmp', - jump => 'ACCEPT' } firewall { '055 INPUT allow DNS': + action => accept, proto => 'udp', - jump => 'ACCEPT', sport => 'domain' } firewall { '999 FORWARD drop': + action => drop, chain => 'FORWARD', - jump => 'DROP' } firewall { '001 OUTPUT allow loopback': + action => accept, chain => 'OUTPUT', outiface => 'lo', - jump => 'ACCEPT' } firewall { '100 OUTPUT drop invalid': + action => drop, chain => 'OUTPUT', state => 'INVALID', - jump => 'DROP' } resources { 'firewall': diff --git a/lib/puppet/provider/firewall.rb b/lib/puppet/provider/firewall.rb index 345893f..e6ea598 100644 --- a/lib/puppet/provider/firewall.rb +++ b/lib/puppet/provider/firewall.rb @@ -38,6 +38,7 @@ class Puppet::Provider::Firewall < Puppet::Provider dynamic_methods = self.class.instance_variable_get('@resource_map').keys dynamic_methods << :chain dynamic_methods << :table + dynamic_methods << :action if dynamic_methods.include?(meth.to_sym) then if @property_hash[meth.to_sym] then diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 50f366e..d4c7faa 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -134,8 +134,17 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir hash[:ensure] = :present # Munge some vars here ... - # proto should equal 'all' if undefined + + # Proto should equal 'all' if undefined hash[:proto] = "all" if !hash.include?(:proto) + + # If the jump parameter is set to one of: ACCEPT, REJECT or DROP then + # we should set the action parameter instead. + if ['ACCEPT','REJECT','DROP'].include?(hash[:jump]) then + hash[:action] = hash[:jump].downcase + hash.delete(:jump) + end + hash end @@ -180,17 +189,31 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir def general_args debug "Current resource: %s" % resource.class + args = [] - self.class.instance_variable_get('@resource_list').each do |res| - if(resource.value(res)) - args << self.class.instance_variable_get('@resource_map')[res].split(' ') - if resource[res].is_a?(Array) - args << resource[res].join(',') - else - args << resource[res] - end + resource_list = self.class.instance_variable_get('@resource_list') + resource_map = self.class.instance_variable_get('@resource_map') + + resource_list.each do |res| + resource_value = nil + if (resource[res]) then + resource_value = resource[res] + elsif res == :jump and resource[:action] then + # In this case, we are substituting jump for action + resource_value = resource[:action].to_s.upcase + else + next + end + + args << resource_map[res].split(' ') + + if resource_value.is_a?(Array) + args << resource_value.join(',') + else + args << resource_value end end + args end diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb index 522b3fd..52844d9 100644 --- a/lib/puppet/type/firewall.rb +++ b/lib/puppet/type/firewall.rb @@ -42,10 +42,9 @@ Puppet::Type.newtype(:firewall) do newvalues(/^\d+[a-zA-Z0-9\s\-_]+$/) end - newparam(:action) do + newproperty(:action) do desc "Action to perform on this rule." newvalues(:accept, :reject, :drop) - defaultto :accept end # Generic matching properties @@ -108,11 +107,30 @@ Puppet::Type.newtype(:firewall) do end newproperty(:jump, :required_features => :iptables) do - desc "The value for the iptables --jump parameter. Normal values are: - 'ACCEPT', 'DROP', 'QUEUE', 'RETURN', REJECT', 'DNAT', 'SNAT', 'LOG', - 'MASQUERADE', 'REDIRECT'. But any valid chain name is allowed." - newvalues(/^[a-zA-Z0-9\-_]+$/) - defaultto "ACCEPT" + desc < { + :line => '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061,7062 -m comment --comment "000 allow foo" -j ACCEPT', + :table => 'filter', + :compare_all => true, + :params => { + :action => "accept", + :chain => "INPUT", + :destination => "1.1.1.1", + :dport => ["7061","7062"], + :ensure => :present, + :line => '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061,7062 -m comment --comment "000 allow foo" -j ACCEPT', + :name => "000 allow foo", + :proto => "tcp", + :provider => "iptables", + :source => "1.1.1.1", + :sport => ["7061","7062"], + :table => "filter", + }, + }, + 'action_drop_1' => { + :line => '-A INPUT -m comment --comment "000 allow foo" -j DROP', + :table => 'filter', + :params => { + :jump => nil, + :action => "drop", + }, + }, + 'action_reject_1' => { + :line => '-A INPUT -m comment --comment "000 allow foo" -j REJECT', + :table => 'filter', + :params => { + :jump => nil, + :action => "reject", + }, + }, + 'action_nil_1' => { + :line => '-A INPUT -m comment --comment "000 allow foo"', + :table => 'filter', + :params => { + :jump => nil, + :action => nil, + }, + }, + 'jump_custom_chain_1' => { + :line => '-A INPUT -m comment --comment "000 allow foo" -j custom_chain', + :table => 'filter', + :params => { + :jump => "custom_chain", + :action => nil, + }, + }, +} + +# This hash is for testing converting a hash to an argument line. +HASH_TO_ARGS = { + 'long_rule_1' => { + :params => { + :action => "accept", + :chain => "INPUT", + :destination => "1.1.1.1", + :dport => ["7061","7062"], + :ensure => :present, + :name => "000 allow foo", + :proto => "tcp", + :source => "1.1.1.1", + :sport => ["7061","7062"], + :table => "filter", + }, + :args => ["-t", :filter, "-s", "1.1.1.1", "-d", "1.1.1.1", "-p", :tcp, "-m", "multiport", "--sports", "7061,7062", "-m", "multiport", "--dports", "7061,7062", "-m", "comment", "--comment", "000 allow foo", "-j", "ACCEPT"], + }, + 'long_rule_2' => { + :params => { + :chain => "INPUT", + :destination => "2.10.13.3/24", + :dport => ["7061"], + :ensure => :present, + :jump => "my_custom_chain", + :name => "700 allow bar", + :proto => "udp", + :source => "1.1.1.1", + :sport => ["7061","7062"], + :table => "filter", + }, + :args => ["-t", :filter, "-s", "1.1.1.1", "-d", "2.10.13.3/24", "-p", :udp, "-m", "multiport", "--sports", "7061,7062", "-m", "multiport", "--dports", "7061", "-m", "comment", "--comment", "700 allow bar", "-j", "my_custom_chain"], + }, + 'no_action' => { + :params => { + :name => "100 no action", + :table => "filter", + }, + :args => ["-t", :filter, "-p", :tcp, "-m", "comment", "--comment", + "100 no action"], + } +} diff --git a/spec/unit/puppet/provider/iptables_spec.rb b/spec/unit/puppet/provider/iptables_spec.rb index d27fc14..4da37ed 100644 --- a/spec/unit/puppet/provider/iptables_spec.rb +++ b/spec/unit/puppet/provider/iptables_spec.rb @@ -1,24 +1,28 @@ +#!/usr/bin/env rspec + require 'spec_helper' +require 'puppet/provider/confine/exists' describe 'iptables provider detection' do - before :each do - require 'puppet/provider/confine/exists' - @exists = Puppet::Provider::Confine::Exists + let(:exists) { + Puppet::Provider::Confine::Exists + } + before :each do # Reset the default provider Puppet::Type.type(:firewall).defaultprovider = nil end it "should default to iptables provider if /sbin/iptables[-save] exists" do # Stub lookup for /sbin/iptables & /sbin/iptables-save - @exists.any_instance.stubs(:which).with("/sbin/iptables"). + exists.any_instance.stubs(:which).with("/sbin/iptables"). returns "/sbin/iptables" - @exists.any_instance.stubs(:which).with("/sbin/iptables-save"). + exists.any_instance.stubs(:which).with("/sbin/iptables-save"). returns "/sbin/iptables-save" # Every other command should return false so we don't pick up any # other providers - @exists.any_instance.stubs(:which).with() { |value| + exists.any_instance.stubs(:which).with() { |value| ! ["/sbin/iptables","/sbin/iptables-save"].include?(value) }.returns false @@ -31,7 +35,7 @@ describe 'iptables provider detection' do it "should raise a default provider error when there are no commands" do # Stub all commands lookups so they return nothing - @exists.any_instance.stubs(:which).returns false + exists.any_instance.stubs(:which).returns false # Instantiate a resource instance and make sure it raises an exception lambda { resource = Puppet::Type.type(:firewall).new({ @@ -41,96 +45,121 @@ describe 'iptables provider detection' do end describe 'iptables provider' do - before :each do - @provider = Puppet::Type.type(:firewall).provider(:iptables) - Puppet::Type::Firewall.stubs(:defaultprovider).returns @provider - @provider.stubs(:command).with(:iptables_save).returns "/sbin/iptables-save" - @resource = Puppet::Type.type(:firewall).new({ + let(:provider) { Puppet::Type.type(:firewall).provider(:iptables) } + let(:resource) { + Puppet::Type.type(:firewall).new({ :name => '000 test foo', - :chain => 'INPUT', - :jump => 'ACCEPT' + :action => 'accept', }) + } + + before :each do + Puppet::Type::Firewall.stubs(:defaultprovider).returns provider + provider.stubs(:command).with(:iptables_save).returns "/sbin/iptables-save" end it 'should be able to get a list of existing rules' do # Pretend to return nil from iptables - @provider.expects(:execute).with(['/sbin/iptables-save']).returns("") + provider.expects(:execute).with(['/sbin/iptables-save']).returns("") - @provider.instances.each do |rule| - rule.should be_instance_of(@provider) - rule.properties[:provider].to_s.should == @provider.name.to_s + provider.instances.each do |rule| + rule.should be_instance_of(provider) + rule.properties[:provider].to_s.should == provider.name.to_s end end - describe 'when converting rules to resources' do - before :each do - @resource = @provider.rule_to_hash('-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061,7062 -m comment --comment "000 allow foo" -j ACCEPT', 'filter', 0) - end + # Load in ruby hash for test fixtures. + load 'spec/fixtures/iptables/conversion_hash.rb' - [:name, :table, :chain, :proto, :jump, :source, :destination].each do |param| - it "#{param} should be a string" do - @resource[param].class.should == String + describe 'when converting rules to resources' do + ARGS_TO_HASH.each do |test_name,data| + describe "for test data '#{test_name}'" do + let(:resource) { provider.rule_to_hash(data[:line], data[:table], 0) } + + # If this option is enabled, make sure the parameters exactly match + if data[:compare_all] then + it "the parameter hash keys should be the same as returned by rules_to_hash" do + resource.keys.sort.should == data[:params].keys.sort + end + end + + # Iterate across each parameter, creating an example for comparison + data[:params].each do |param_name, param_value| + it "the parameter '#{param_name.to_s}' should match #{param_value.inspect}" do + resource[param_name].should == data[:params][param_name] + end + end end end + end + + describe 'when working out general_args' do + HASH_TO_ARGS.each do |test_name,data| + describe "for test data '#{test_name}'" do + let(:resource) { Puppet::Type.type(:firewall).new(data[:params]) } + let(:provider) { Puppet::Type.type(:firewall).provider(:iptables) } + let(:instance) { provider.new(resource) } - [:dport, :sport].each do |param| - it "#{param} should be an array" do - @resource[param].class.should == Array + it 'general_args should be valid' do + instance.general_args.flatten.should == data[:args] + end end end end describe 'when converting rules without comments to resources' do - before :each do - @rule = '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT' - @resource = @provider.rule_to_hash(@rule, 'filter', 0) - @instance = @provider.new(@resource) - end + let(:sample_rule) { + '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT' + } + let(:resource) { provider.rule_to_hash(sample_rule, 'filter', 0) } + let(:instance) { provider.new(resource) } it 'rule name contains a MD5 sum of the line' do - @resource[:name].should == "9999 #{Digest::MD5.hexdigest(@resource[:line])}" + resource[:name].should == "9999 #{Digest::MD5.hexdigest(resource[:line])}" end end describe 'when creating resources' do + let(:instance) { provider.new(resource) } + before :each do - @instance = @provider.new(@resource) - @provider.expects(:execute).with(['/sbin/iptables-save']).returns("") + provider.expects(:execute).with(['/sbin/iptables-save']).returns("") end it 'insert_args should be an array' do - @instance.insert_args.class.should == Array + instance.insert_args.class.should == Array end end describe 'when modifying resources' do + let(:instance) { provider.new(resource) } + before :each do - @instance = @provider.new(@resource) - @provider.expects(:execute).with(['/sbin/iptables-save']).returns "" + provider.expects(:execute).with(['/sbin/iptables-save']).returns "" end it 'update_args should be an array' do - @instance.update_args.class.should == Array + instance.update_args.class.should == Array end end describe 'when deleting resources' do - before :each do - @rule = '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT' - @resource = @provider.rule_to_hash(@rule, 'filter', 0) - @instance = @provider.new(@resource) - end + let(:sample_rule) { + '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT' + } + let(:resource) { provider.rule_to_hash(sample_rule, 'filter', 0) } + let(:instance) { provider.new(resource) } it 'resource[:line] looks like the original rule' do - @resource[:line] == @rule + resource[:line] == sample_rule end it 'delete_args is an array' do - @instance.delete_args.class.should == Array + instance.delete_args.class.should == Array end it 'delete_args is the same as the rule string when joined' do - @instance.delete_args.join(' ').should == @rule.gsub(/\-A/, '-D') + instance.delete_args.join(' ').should == sample_rule.gsub(/\-A/, '-D') end end end diff --git a/spec/unit/puppet/type/firewall_spec.rb b/spec/unit/puppet/type/firewall_spec.rb index cc88010..d442c44 100644 --- a/spec/unit/puppet/type/firewall_spec.rb +++ b/spec/unit/puppet/type/firewall_spec.rb @@ -1,3 +1,5 @@ +#!/usr/bin/env rspec + require 'spec_helper' firewall = Puppet::Type.type(:firewall) @@ -9,16 +11,9 @@ describe firewall do @provider.stubs(:name).returns(:iptables) Puppet::Type::Firewall.stubs(:defaultprovider).returns @provider - @resource = @class.new({ - :name => '000 test foo', - :chain => 'INPUT', - :jump => 'ACCEPT' - }) + @resource = @class.new({:name => '000 test foo'}) end - # Example on how to test and have the full validation kick in - #lambda { @class.new(:name => "001-test", :chain => chain) }.should_not raise_error - it 'should have :name be its namevar' do @class.key_attributes.should == [:name] end @@ -34,6 +29,24 @@ describe firewall do end end + describe ':action' do + it "should have no default" do + res = @class.new(:name => "000 test") + res.parameters[:action].should == nil + end + + [:accept, :drop, :reject].each do |action| + it "should accept value #{action}" do + @resource[:action] = action + @resource[:action].should == action + end + end + + it 'should fail when value is not recognized' do + lambda { @resource[:action] = 'not valid' }.should raise_error(Puppet::Error) + end + end + describe ':chain' do [:INPUT, :FORWARD, :OUTPUT, :PREROUTING, :POSTROUTING].each do |chain| it "should accept chain value #{chain}" do @@ -74,15 +87,26 @@ describe firewall do end describe ':jump' do - [:ACCEPT, :DROP, :QUEUE, :RETURN, :REJECT, :DNAT, :SNAT, :LOG, :MASQUERADE, :REDIRECT].each do |jump| + it "should have no default" do + res = @class.new(:name => "000 test") + res.parameters[:jump].should == nil + end + + ['QUEUE', 'RETURN', 'DNAT', 'SNAT', 'LOG', 'MASQUERADE', 'REDIRECT'].each do |jump| it "should accept jump value #{jump}" do @resource[:jump] = jump @resource[:jump].should == jump end end + ['ACCEPT', 'DROP', 'REJECT'].each do |jump| + it "should now fail when value #{jump}" do + lambda { @resource[:jump] = jump }.should raise_error(Puppet::Error) + end + end + it "should fail when jump value is not recognized" do - lambda { @resource[:proto] = 'jump' }.should raise_error(Puppet::Error) + lambda { @resource[:jump] = '%^&*' }.should raise_error(Puppet::Error) end end @@ -181,4 +205,16 @@ describe firewall do lambda { @resource[:burst] = 'foo' }.should raise_error(Puppet::Error) end end + + describe ':action and :jump' do + it 'should allow only 1 to be set at a time' do + expect { + @class.new( + :name => "001-test", + :action => "accept", + :jump => "custom_chain" + ) + }.should raise_error(Puppet::Error, /^Only one of the parameters 'action' and 'jump' can be set$/) + end + end end -- 2.45.2