From ff2e35584d6b5565dc80e41d8bb436a2c09a7251 Mon Sep 17 00:00:00 2001 From: Sharif Nassar Date: Wed, 30 May 2012 16:12:21 -0700 Subject: [PATCH] (#14755) Fix mark to not repeat rules with iptables 1.4.1+. --- LICENSE | 1 + .../{iptables.rb => ip6tables_version.rb} | 12 ------ lib/facter/iptables_version.rb | 11 +++++ lib/puppet/provider/firewall/iptables.rb | 11 ++++- lib/puppet/type/firewall.rb | 36 ++++++++++++++-- lib/puppet/util/firewall.rb | 14 +++++++ spec/fixtures/iptables/conversion_hash.rb | 40 +++++++++++++++--- spec/unit/facter/iptables_spec.rb | 2 +- spec/unit/puppet/type/firewall_spec.rb | 42 +++++++++++++++---- spec/unit/puppet/util/firewall_spec.rb | 11 +++++ 10 files changed, 148 insertions(+), 32 deletions(-) rename lib/facter/{iptables.rb => ip6tables_version.rb} (50%) create mode 100644 lib/facter/iptables_version.rb diff --git a/LICENSE b/LICENSE index e43c1c5..b66330d 100644 --- a/LICENSE +++ b/LICENSE @@ -2,6 +2,7 @@ Puppet Firewall Module - Puppet module for managing Firewalls Copyright (C) 2011 Puppet Labs, Inc. Copyright (C) 2011 Jonathan Boyett +Copyright (C) 2011 Media Temple, Inc. Some of the iptables code was taken from puppet-iptables which was: diff --git a/lib/facter/iptables.rb b/lib/facter/ip6tables_version.rb similarity index 50% rename from lib/facter/iptables.rb rename to lib/facter/ip6tables_version.rb index d6ee3fc..3dce27f 100644 --- a/lib/facter/iptables.rb +++ b/lib/facter/ip6tables_version.rb @@ -1,15 +1,3 @@ -Facter.add(:iptables_version) do - confine :kernel => :linux - setcode do - version = Facter::Util::Resolution.exec('iptables --version') - if version - version.match(/\d+\.\d+\.\d+/).to_s - else - nil - end - end -end - Facter.add(:ip6tables_version) do confine :kernel => :linux setcode do diff --git a/lib/facter/iptables_version.rb b/lib/facter/iptables_version.rb new file mode 100644 index 0000000..6f7ae56 --- /dev/null +++ b/lib/facter/iptables_version.rb @@ -0,0 +1,11 @@ +Facter.add(:iptables_version) do + confine :kernel => :linux + setcode do + version = Facter::Util::Resolution.exec('iptables --version') + if version + version.match(/\d+\.\d+\.\d+/).to_s + else + nil + end + end +end diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 2d1f40a..d8538ed 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -26,6 +26,13 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir defaultfor :kernel => :linux + iptables_version = Facter.fact('iptables_version').value + if (iptables_version and Puppet::Util::Package.versioncmp(iptables_version, '1.4.1') < 0) + mark_flag = '--set-mark' + else + mark_flag = '--set-xmark' + end + @resource_map = { :burst => "--limit-burst", :destination => "-d", @@ -42,16 +49,16 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir :port => '-m multiport --ports', :proto => "-p", :reject => "--reject-with", + :set_mark => mark_flag, :source => "-s", - :state => "-m state --state", :sport => "-m multiport --sports", + :state => "-m state --state", :table => "-t", :tcp_flags => "-m tcp --tcp-flags", :todest => "--to-destination", :toports => "--to-ports", :tosource => "--to-source", :uid => "-m owner --uid-owner", - :set_mark => "--set-mark", :pkttype => "-m pkttype --pkt-type" } diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb index 47603bb..47aa992 100644 --- a/lib/puppet/type/firewall.rb +++ b/lib/puppet/type/firewall.rb @@ -477,15 +477,43 @@ Puppet::Type.newtype(:firewall) do newproperty(:set_mark, :required_features => :mark) do desc <<-EOS - Set the Netfilter mark value associated with the packet. + Set the Netfilter mark value associated with the packet. Accepts either of: + mark/mask or mark. These will be converted to hex if they are not already. EOS munge do |value| - if ! value.to_s.include?("0x") - "0x" + value.to_i.to_s(16) + int_or_hex = '[a-fA-F0-9x]' + match = value.to_s.match("(#{int_or_hex}+)(/)?(#{int_or_hex}+)?") + mark = @resource.to_hex32(match[1]) + + # Values that can't be converted to hex. + # Or contain a trailing slash with no mask. + if mark.nil? or (mark and match[2] and match[3].nil?) + raise ArgumentError, "MARK value must be integer or hex between 0 and 0xffffffff" + end + + # Old iptables does not support a mask. New iptables will expect one. + iptables_version = Facter.fact('iptables_version').value + mask_required = (iptables_version and Puppet::Util::Package.versioncmp(iptables_version, '1.4.1') >= 0) + + if mask_required + if match[3].nil? + value = "#{mark}/0xffffffff" + else + mask = @resource.to_hex32(match[3]) + if mask.nil? + raise ArgumentError, "MARK mask must be integer or hex between 0 and 0xffffffff" + end + value = "#{mark}/#{mask}" + end else - super(value) + unless match[3].nil? + raise ArgumentError, "iptables version #{iptables_version} does not support masks on MARK rules" + end + value = mark end + + value end end diff --git a/lib/puppet/util/firewall.rb b/lib/puppet/util/firewall.rb index e36c4b1..d6dd90d 100644 --- a/lib/puppet/util/firewall.rb +++ b/lib/puppet/util/firewall.rb @@ -96,4 +96,18 @@ module Puppet::Util::Firewall return nil if value.prefixlen == 0 value.cidr end + + # Validates the argument is int or hex, and returns valid hex + # conversion of the value or nil otherwise. + def to_hex32(value) + begin + value = Integer(value) + if value.between?(0, 0xffffffff) + return '0x' + value.to_s(16) + end + rescue ArgumentError + # pass + end + return nil + end end diff --git a/spec/fixtures/iptables/conversion_hash.rb b/spec/fixtures/iptables/conversion_hash.rb index 563e2a3..c3c0c8c 100644 --- a/spec/fixtures/iptables/conversion_hash.rb +++ b/spec/fixtures/iptables/conversion_hash.rb @@ -219,12 +219,12 @@ ARGS_TO_HASH = { }, }, 'mark_set-mark' => { - :line => '-t mangle -A PREROUTING -j MARK --set-mark 1000', + :line => '-t mangle -A PREROUTING -j MARK --set-xmark 0x3e8/0xffffffff', :table => 'mangle', :params => { :jump => 'MARK', :chain => 'PREROUTING', - :set_mark => '1000', + :set_mark => '0x3e8/0xffffffff', } }, 'iniface_1' => { @@ -532,7 +532,7 @@ HASH_TO_ARGS = { }, :args => ['-t', :mangle, '-p', :all, '-m', 'owner', '--gid-owner', 'root', '-m', 'comment', '--comment', '057 POSTROUTING gid root only', '-j', 'ACCEPT'], }, - 'mark_set-mark' => { + 'mark_set-mark_int' => { :params => { :name => '058 set-mark 1000', :table => 'mangle', @@ -540,9 +540,39 @@ HASH_TO_ARGS = { :chain => 'PREROUTING', :set_mark => '1000', }, - :args => ['-t', :mangle, '-p', :tcp, '-m', 'comment', '--comment', '058 set-mark 1000', '-j', 'MARK', '--set-mark', '0x3e8'], + :args => ['-t', :mangle, '-p', :tcp, '-m', 'comment', '--comment', '058 set-mark 1000', '-j', 'MARK', '--set-xmark', '0x3e8/0xffffffff'], }, - 'iniface_1' => { + 'mark_set-mark_hex' => { + :params => { + :name => '058 set-mark 0x32', + :table => 'mangle', + :jump => 'MARK', + :chain => 'PREROUTING', + :set_mark => '0x32', + }, + :args => ['-t', :mangle, '-p', :tcp, '-m', 'comment', '--comment', '058 set-mark 0x32', '-j', 'MARK', '--set-xmark', '0x32/0xffffffff'], + }, + 'mark_set-mark_hex_with_hex_mask' => { + :params => { + :name => '058 set-mark 0x32/0xffffffff', + :table => 'mangle', + :jump => 'MARK', + :chain => 'PREROUTING', + :set_mark => '0x32/0xffffffff', + }, + :args => ['-t', :mangle, '-p', :tcp, '-m', 'comment', '--comment', '058 set-mark 0x32/0xffffffff', '-j', 'MARK', '--set-xmark', '0x32/0xffffffff'], + }, + 'mark_set-mark_hex_with_mask' => { + :params => { + :name => '058 set-mark 0x32/4', + :table => 'mangle', + :jump => 'MARK', + :chain => 'PREROUTING', + :set_mark => '0x32/4', + }, + :args => ['-t', :mangle, '-p', :tcp, '-m', 'comment', '--comment', '058 set-mark 0x32/4', '-j', 'MARK', '--set-xmark', '0x32/0x4'], + }, + 'iniface_1' => { :params => { :name => '060 iniface', :table => 'filter', diff --git a/spec/unit/facter/iptables_spec.rb b/spec/unit/facter/iptables_spec.rb index f0dd81d..1733f13 100644 --- a/spec/unit/facter/iptables_spec.rb +++ b/spec/unit/facter/iptables_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' -require 'facter/iptables' describe "Facter::Util::Fact" do before { + Facter.clear Facter.fact(:kernel).stubs(:value).returns("Linux") Facter.fact(:kernelrelease).stubs(:value).returns("2.6") } diff --git a/spec/unit/puppet/type/firewall_spec.rb b/spec/unit/puppet/type/firewall_spec.rb index dbf0dde..af7d272 100755 --- a/spec/unit/puppet/type/firewall_spec.rb +++ b/spec/unit/puppet/type/firewall_spec.rb @@ -34,7 +34,7 @@ describe firewall 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 @@ -277,10 +277,10 @@ describe firewall do describe ':action and :jump' do it 'should allow only 1 to be set at a time' do - expect { + expect { @class.new( - :name => "001-test", - :action => "accept", + :name => "001-test", + :action => "accept", :jump => "custom_chain" ) }.should raise_error(Puppet::Error, /^Only one of the parameters 'action' and 'jump' can be set$/) @@ -307,12 +307,38 @@ describe firewall do describe ':set_mark' do it 'should allow me to set set-mark' do - @resource[:set_mark] = '0x3e8' - @resource[:set_mark].should == '0x3e8' + @resource[:set_mark] = '0x3e8/0xffffffff' + @resource[:set_mark].should == '0x3e8/0xffffffff' end - it 'should convert int to hex' do + it 'should convert int to hex and add a 32 bit mask' do @resource[:set_mark] = '1000' - @resource[:set_mark].should == '0x3e8' + @resource[:set_mark].should == '0x3e8/0xffffffff' + end + it 'should add a 32 bit mask' do + @resource[:set_mark] = '0x32' + @resource[:set_mark].should == '0x32/0xffffffff' + end + it 'should use the mask provided' do + @resource[:set_mark] = '0x32/0x4' + @resource[:set_mark].should == '0x32/0x4' + end + it 'should use the mask provided and convert int to hex' do + @resource[:set_mark] = '1000/0x4' + @resource[:set_mark].should == '0x3e8/0x4' + end + ['/', '1000/', 'pwnie'].each do |bad_mark| + it "should fail with malformed mark '#{bad_mark}'" do + lambda { @resource[:set_mark] = bad_mark}.should raise_error(Puppet::Error) + end + end + it 'should fail if mask is malformed' do + lambda { @resource[:set_mark] = '1000/0xq4'}.should raise_error(Puppet::Error) + end + it 'should fail if mark value is more than 32 bits' do + lambda { @resource[:set_mark] = '4294967296'}.should raise_error(Puppet::Error) + end + it 'should fail if mask value is more than 32 bits' do + lambda { @resource[:set_mark] = '1/4294967296'}.should raise_error(Puppet::Error) end end diff --git a/spec/unit/puppet/util/firewall_spec.rb b/spec/unit/puppet/util/firewall_spec.rb index 822a3f8..e847b3c 100644 --- a/spec/unit/puppet/util/firewall_spec.rb +++ b/spec/unit/puppet/util/firewall_spec.rb @@ -44,4 +44,15 @@ describe 'Puppet::Util::Firewall' do specify { subject.string_to_port('80').should == '80' } specify { subject.string_to_port('http').should == '80' } end + + describe '#to_hex32' do + subject { resource } + specify { subject.to_hex32('0').should == '0x0' } + specify { subject.to_hex32('0x32').should == '0x32' } + specify { subject.to_hex32('42').should == '0x2a' } + specify { subject.to_hex32('4294967295').should == '0xffffffff' } + specify { subject.to_hex32('4294967296').should == nil } + specify { subject.to_hex32('-1').should == nil } + specify { subject.to_hex32('bananas').should == nil } + end end -- 2.45.2