From: Ken Barber Date: Tue, 11 Oct 2011 17:29:17 +0000 (+0100) Subject: (#10002) Change to dport and sport to handle ranges, and fix handling of name to... X-Git-Tag: v0.0.2~2^2 X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=3991b7621c10254b2ad7508375bc93e68133fa12;p=puppet-modules%2Fpuppetlabs-firewall.git (#10002) Change to dport and sport to handle ranges, and fix handling of name to port. 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 d4c7faa..adc3d0d 100644 --- 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 2eb8751..6396ae9 100644 --- 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: + + - + + 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: + + - + + 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 d49aeb4..7e21d50 100644 --- 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 d3e64ee..c4d290a 100644 --- 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 d442c44..3b19327 100644 --- 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