]> review.fuel-infra Code Review - puppet-modules/puppetlabs-firewall.git/commitdiff
(#14755) Fix mark to not repeat rules with iptables 1.4.1+.
authorSharif Nassar <sharif@mediatemple.net>
Wed, 30 May 2012 23:12:21 +0000 (16:12 -0700)
committerKen Barber <ken@bob.sh>
Wed, 20 Jun 2012 17:34:52 +0000 (18:34 +0100)
LICENSE
lib/facter/ip6tables_version.rb [moved from lib/facter/iptables.rb with 50% similarity]
lib/facter/iptables_version.rb [new file with mode: 0644]
lib/puppet/provider/firewall/iptables.rb
lib/puppet/type/firewall.rb
lib/puppet/util/firewall.rb
spec/fixtures/iptables/conversion_hash.rb
spec/unit/facter/iptables_spec.rb
spec/unit/puppet/type/firewall_spec.rb
spec/unit/puppet/util/firewall_spec.rb

diff --git a/LICENSE b/LICENSE
index e43c1c5cc95d0dc820a0a80374d3e4b8de9dd69b..b66330d45273389d2dad32fa5c951e1894f1d15b 100644 (file)
--- 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:
 
similarity index 50%
rename from lib/facter/iptables.rb
rename to lib/facter/ip6tables_version.rb
index d6ee3fc0c65128bdcb4eff556ccb5635b51d6b92..3dce27f70cbbd0f544d5aab1228b4db369fac13d 100644 (file)
@@ -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 (file)
index 0000000..6f7ae56
--- /dev/null
@@ -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
index 2d1f40a4f5d46aa1376b816b981cda339864ef4e..d8538edd9ed8cc803caf19fa1d6e0346f09fba3f 100644 (file)
@@ -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"
   }
 
index 47603bbb8d9f4897d974a3e9472eef2cf8e636e5..47aa99200a89bff3f85b3a685621b0831f8735f0 100644 (file)
@@ -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
 
index e36c4b15c1d95a0d290c4eb67b6d7f87a6aa5005..d6dd90d5577cdd2bc59dea1ce7ed59b7b5d205b3 100644 (file)
@@ -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
index 563e2a389eec3a7c1ff4caabadb2db0f6c27e460..c3c0c8c52fbeb586fceb9daa102bb653a5184c53 100644 (file)
@@ -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',
index f0dd81d72dcaaa7d70cc39b5cf9c921a7cd66629..1733f135916e28903b2f4c0a00fd401c2e17c47c 100644 (file)
@@ -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")
   }
index dbf0dde8e737cf66b9d61af166579f42643b532d..af7d272261cffefe0d66970c5f43d02cc78bfd20 100755 (executable)
@@ -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
 
index 822a3f8fd337065db50da57ae05562c8f80df5bd..e847b3cf47f01f0447b0a0526289cc9ea5af1ade 100644 (file)
@@ -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