(#14755) Fix mark to not repeat rules with iptables 1.4.1+.

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:
 

lib/facter/iptables.rb → 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

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

lib/puppet/provider/firewall/iptables.rb

@@ -26,6 +26,13 @@
 
   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 @@
     :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"
   }
 

lib/puppet/type/firewall.rb

@@ -477,15 +477,43 @@ def should_to_s(value)
 
   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
 

lib/puppet/util/firewall.rb

@@ -96,4 +96,18 @@ def host_to_ip(value)
     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

spec/fixtures/iptables/conversion_hash.rb

@@ -219,12 +219,12 @@
     },
   },
   '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,17 +532,47 @@
     },
     :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',
       :jump     => 'MARK',
       :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',

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")
   }

spec/unit/puppet/type/firewall_spec.rb

@@ -34,7 +34,7 @@
       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 ':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 ':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
 

spec/unit/puppet/util/firewall_spec.rb

@@ -44,4 +44,15 @@
     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