(#10002) Change to dport and sport to handle ranges, and fix handling of name to...

We hadn't been allowing ranges of the kind 22:1000 for ranges. This patch
fixes that. Thanks to Jason Hancock for finding this issue and providing a sample
patch.

Instead of using colon though, it was decided we would use a hyphen to specify a range
as its more agnostic. This patch does the filtering for both writing the rule and
reading the rule.

Also - the way we were doing name to port conversion had been broken. I found
this out while fixing the ranges, and have now fixed it and added tests.

diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb
index d4c7faa5b04d0608f19d3e6870b2043228effafe..adc3d0d49b584e50154bacb7ec8ba915405db479 100644 (file)
--- a/lib/puppet/provider/firewall/iptables.rb
+++ b/lib/puppet/provider/firewall/iptables.rb
@@ -121,6 +121,16 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir
       hash[prop] = hash[prop].split(',') if ! hash[prop].nil?
     end
 
+    # Our type prefers hyphens over colons for ranges so ...
+    # Iterate across all ports replacing colons with hyphens so that ranges match
+    # the types expectations.
+    [:dport, :sport].each do |prop|
+      next unless hash[prop]
+      hash[prop] = hash[prop].collect do |elem|
+        elem.gsub(/:/,'-')
+      end
+    end
+
     # This forces all existing, commentless rules to be moved to the bottom of the stack.
     # Puppet-firewall requires that all rules have comments (resource names) and will fail if
     # a rule in iptables does not have a comment. We get around this by appending a high level
@@ -207,6 +217,14 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir
 
       args << resource_map[res].split(' ')
 
+      # For sport and dport, convert hyphens to colons since the type
+      # expects hyphens for ranges of ports.
+      if [:sport, :dport].include?(res) then
+        resource_value = resource_value.collect do |elem|
+          elem.gsub(/-/, ':')
+        end
+      end
+
       if resource_value.is_a?(Array)
         args << resource_value.join(',')
       else

diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb
index 2eb87517f73551058d9b69e571909752a1dbe5b3..6396ae9cb4e79553712e98733c9207ae87cc029e 100644 (file)
--- a/lib/puppet/type/firewall.rb
+++ b/lib/puppet/type/firewall.rb
@@ -92,8 +92,18 @@ Puppet::Type.newtype(:firewall) do
 
   newproperty(:sport, :array_matching => :all) do
     desc <<-EOS
-      For protocols that support ports, this is a list of source ports 
-      to filter on.
+      The source port to match for this filter (if the protocol supports 
+      ports). Will accept a single element or an array.
+
+      For some firewall providers you can pass a range of ports in the format:
+
+          <start_number>-<ending_number>
+
+      For example:
+
+          1-1024
+
+      This would cover ports 1 to 1024.
     EOS
 
     munge do |value|
@@ -108,8 +118,18 @@ Puppet::Type.newtype(:firewall) do
 
   newproperty(:dport, :array_matching => :all) do
     desc <<-EOS
-      For protocols that support ports, this is a list of destination 
-      ports to filter on.
+      The destination port to match for this filter (if the protocol supports 
+      ports). Will accept a single element or an array.
+
+      For some firewall providers you can pass a range of ports in the format:
+
+          <start_number>-<ending_number>
+
+      For example:
+
+          1-1024
+
+      This would cover ports 1 to 1024.
     EOS
     
     munge do |value|

diff --git a/lib/puppet/util/firewall.rb b/lib/puppet/util/firewall.rb
index d49aeb484b14dbb8e2274f11cd6b1410b588dcb7..7e21d50112840b69765f90c162426b834b87e86c 100644 (file)
--- a/lib/puppet/util/firewall.rb
+++ b/lib/puppet/util/firewall.rb
@@ -45,13 +45,19 @@ module Puppet::Util::Firewall
     end
   end
 
+  # This method takes a string and attempts to convert it to a port number
+  # if valid.
+  # 
+  # If the string already contains a port number or perhaps a range of ports
+  # in the format 22:1000 for example, it simply returns the string and does
+  # nothing.
   def string_to_port(value)
-    if value.kind_of?(Array)
-      ports = []
-      value.each do |port|
-        ports << Socket.getservbyname(port) unless port.kind_of?(Integer)
+    if value.kind_of?(String)
+      if value.match(/^\d+(-\d+)?$/)
+        return value
+      else
+        return Socket.getservbyname(value).to_s
       end
-      ports
     else
       Socket.getservbyname(value)
     end

diff --git a/spec/fixtures/iptables/conversion_hash.rb b/spec/fixtures/iptables/conversion_hash.rb
index d3e64ee53718904e16b1bbc9a5fb7671d4af5ab5..c4d290a86bcd35f2e52bc91cce2521a9fbc5ef99 100644 (file)
--- a/spec/fixtures/iptables/conversion_hash.rb
+++ b/spec/fixtures/iptables/conversion_hash.rb
@@ -57,6 +57,34 @@ ARGS_TO_HASH = {
       :action => nil,
     },
   },
+  'dport_range_1' => {
+    :line => '-A INPUT -m multiport --dports 1:1024 -m comment --comment "000 allow foo"',
+    :table => 'filter',
+    :params => {
+      :dport => ["1-1024"],
+    },
+  },
+  'dport_range_2' => {
+    :line => '-A INPUT -m multiport --dports 15,512:1024 -m comment --comment "000 allow foo"',
+    :table => 'filter',
+    :params => {
+      :dport => ["15","512-1024"],
+    },
+  },
+  'sport_range_1' => {
+    :line => '-A INPUT -m multiport --sports 1:1024 -m comment --comment "000 allow foo"',
+    :table => 'filter',
+    :params => {
+      :sport => ["1-1024"],
+    },
+  },
+  'sport_range_2' => {
+    :line => '-A INPUT -m multiport --sports 15,512:1024 -m comment --comment "000 allow foo"',
+    :table => 'filter',
+    :params => {
+      :sport => ["15","512-1024"],
+    },
+  },
 }
 
 # This hash is for testing converting a hash to an argument line.
@@ -98,5 +126,37 @@ HASH_TO_ARGS = {
     },  
     :args => ["-t", :filter, "-p", :tcp, "-m", "comment", "--comment", 
       "100 no action"],
-  }
+  },
+  'sport_range_1' => {
+    :params => {
+      :name => "100 sport range",
+      :sport => ["1-1024"],
+      :table => "filter",
+    },  
+    :args => ["-t", :filter, "-p", :tcp, "-m", "multiport", "--sports", "1:1024", "-m", "comment", "--comment", "100 sport range"],
+  },
+  'sport_range_2' => {
+    :params => {
+      :name => "100 sport range",
+      :sport => ["15","512-1024"],
+      :table => "filter",
+    },  
+    :args => ["-t", :filter, "-p", :tcp, "-m", "multiport", "--sports", "15,512:1024", "-m", "comment", "--comment", "100 sport range"],
+  },
+  'dport_range_1' => {
+    :params => {
+      :name => "100 sport range",
+      :dport => ["1-1024"],
+      :table => "filter",
+    },  
+    :args => ["-t", :filter, "-p", :tcp, "-m", "multiport", "--dports", "1:1024", "-m", "comment", "--comment", "100 sport range"],
+  },
+  'dport_range_2' => {
+    :params => {
+      :name => "100 sport range",
+      :dport => ["15","512-1024"],
+      :table => "filter",
+    },  
+    :args => ["-t", :filter, "-p", :tcp, "-m", "multiport", "--dports", "15,512:1024", "-m", "comment", "--comment", "100 sport range"],
+  },
 }

diff --git a/spec/unit/puppet/type/firewall_spec.rb b/spec/unit/puppet/type/firewall_spec.rb
index d442c4476c3645b5f4880c94f7863492bc071fa4..3b19327f1a979e12d3fcfce925040758014f3247 100644 (file)
--- a/spec/unit/puppet/type/firewall_spec.rb
+++ b/spec/unit/puppet/type/firewall_spec.rb
@@ -123,12 +123,37 @@ describe firewall do
     describe port do
       it "should accept a #{port} as string" do
         @resource[port] = '22'
-        @resource[port].should == [22]
+        @resource[port].should == ['22']
       end
 
       it "should accept a #{port} as an array" do
         @resource[port] = ['22','23']
-        @resource[port].should == [22,23]
+        @resource[port].should == ['22','23']
+      end
+
+      it "should accept a #{port} as a hyphen separated range" do
+        @resource[port] = ['22-1000']
+        @resource[port].should == ['22-1000']
+      end
+
+      it "should accept a #{port} as a combination of arrays of single and " \
+        "hyphen separated ranges" do
+
+        @resource[port] = ['22-1000','33','3000-4000']
+        @resource[port].should == ['22-1000','33','3000-4000']
+      end
+
+      it "should convert a port name for #{port} to its number" do
+        @resource[port] = 'ssh'
+        @resource[port].should == ['22']
+      end
+
+      it "should not accept something invalid for #{port}" do
+        expect { @resource[port] = 'something odd' }.should raise_error(Puppet::Error, /^Parameter .+ failed: Munging failed for value ".+" in class .+: no such service/)
+      end
+
+      it "should not accept something invalid in an array for #{port}" do
+        expect { @resource[port] = ['something odd','something even odder'] }.should raise_error(Puppet::Error, /^Parameter .+ failed: Munging failed for value ".+" in class .+: no such service/)
       end
     end
   end