From: Hunter Haugen Date: Wed, 30 Jul 2014 23:32:31 +0000 (-0700) Subject: (MODULES-450) Enable rule inversion X-Git-Tag: 1.3.0~1^2~23^2 X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=d5312a51edb43ac4b90d1228fe60a3b63050b6a6;p=puppet-modules%2Fpuppetlabs-firewall.git (MODULES-450) Enable rule inversion iptables has many rule arguments that may be inverted by prefixing with an exclamation mark. This commit enables inversion for most every property currently in the firewall provider that supports inversion by prefixing the value with a bang+space. Array elements must have all array elements prefixed with a bang+space otherwise a warning will be raised, as it would look confusing to negate a single value and then have iptables negate all of them. --- diff --git a/README.markdown b/README.markdown index 843e5d6..24417f8 100644 --- a/README.markdown +++ b/README.markdown @@ -198,6 +198,28 @@ class profile::apache { } ``` +###Rule inversion +Firewall rules may be inverted by prefixing the value of a parameter by "! ". If the value is an array, then every item in the array must be prefixed as iptables does not understand inverting a single value. + +Parameters that understand inversion are: connmark, ctstate, destination, dport, dst\_range, dst\_type, port, proto, source, sport, src\_range, src\_type, and state. + +Examples: + +```puppet +firewall { '001 disallow esp protocol': + action => 'accept', + proto => '! esp', +} +firewall { '002 drop NEW external website packets with FIN/RST/ACK set and SYN unset': + chain => 'INPUT', + state => 'NEW', + action => 'drop', + proto => 'tcp', + sport => ['! http', '! 443'], + source => '! 10.0.0.0/8', + tcp_flags => '! FIN,SYN,RST,ACK SYN', +} +``` ###Additional Uses for the Firewall Module diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 231568f..43e2484 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -216,13 +216,13 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir # --tcp-flags takes two values; we cheat by adding " around it # so it behaves like --comment - values = values.sub(/--tcp-flags (\S*) (\S*)/, '--tcp-flags "\1 \2"') + values = values.gsub(/(!\s+)?--tcp-flags (\S*) (\S*)/, '--tcp-flags "\1\2 \3"') # we do a similar thing for negated address masks (source and destination). - values = values.sub(/(-\S+) (!)\s?(\S*)/,'\1 "\2 \3"') + values = values.gsub(/(-\S+) (!)\s?(\S*)/,'\1 "\2 \3"') # the actual rule will have the ! mark before the option. - values = values.sub(/(!)\s*(-\S+)\s*(\S*)/, '\2 "\1 \3"') + values = values.gsub(/(!)\s*(-\S+)\s*(\S*)/, '\2 "\1 \3"') # The match extension for tcp & udp are optional and throws off the @resource_map. - values = values.sub(/-m (tcp|udp) (--(s|d)port|-m multiport)/, '\2') + values = values.gsub(/-m (tcp|udp) (--(s|d)port|-m multiport)/, '\2') # '--pol ipsec' takes many optional arguments; we cheat again by adding " around them values = values.sub(/ --pol\sipsec @@ -291,14 +291,6 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir # POST PARSE CLUDGING ##################### - # Normalise all rules to CIDR notation. - [:source, :destination].each do |prop| - next if hash[prop].nil? - m = hash[prop].match(/(!?)\s?(.*)/) - neg = "! " if m[1] == "!" - hash[prop] = "#{neg}#{Puppet::Util::IPCidr.new(m[2]).cidr}" - end - [:dport, :sport, :port, :state, :ctstate].each do |prop| hash[prop] = hash[prop].split(',') if ! hash[prop].nil? end @@ -322,6 +314,43 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir end end + # Invert any rules that are prefixed with a '!' + [ + :connmark, + :ctstate, + :destination, + :dport, + :dst_range, + :dst_type, + :port, + :proto, + :source, + :sport, + :src_range, + :src_type, + :state, + ].each do |prop| + if hash[prop] and hash[prop].is_a?(Array) + # find if any are negated, then negate all if so + should_negate = hash[prop].index do |value| + value.match(/^(!)\s+/) + end + hash[prop] = hash[prop].collect { |v| + "! #{v.sub(/^!\s+/,'')}" + } if should_negate + elsif hash[prop] + m = hash[prop].match(/^(!?)\s?(.*)/) + neg = "! " if m[1] == "!" + if [:source,:destination].include?(prop) + p hash if hash[prop] == "udp" + # Normalise all rules to CIDR notation. + hash[prop] = "#{neg}#{Puppet::Util::IPCidr.new(m[2]).cidr}" + else + hash[prop] = "#{neg}#{m[2]}" + end + end + end + # States should always be sorted. This ensures that the output from # iptables-save and user supplied resources is consistent. hash[:state] = hash[:state].sort unless hash[:state].nil? @@ -424,12 +453,37 @@ Puppet::Type.type(:firewall).provide :iptables, :parent => Puppet::Provider::Fir end args << [resource_map[res]].flatten.first.split(' ') + args = args.flatten # On negations, the '!' has to be before the option (eg: "! -d 1.2.3.4") if resource_value.is_a?(String) and resource_value.sub!(/^!\s*/, '') then # we do this after adding the 'dash' argument because of ones like "-m multiport --dports", where we want it before the "--dports" but after "-m multiport". # so we insert before whatever the last argument is args.insert(-2, '!') + elsif resource_value.is_a?(Symbol) and resource_value.to_s.match(/^!/) then + #ruby 1.8.7 can't .match Symbols ------------------ ^ + resource_value = resource_value.to_s.sub!(/^!\s*/, '').to_sym + args.insert(-2, '!') + elsif resource_value.is_a?(Array) + should_negate = resource_value.index do |value| + #ruby 1.8.7 can't .match symbols + value.to_s.match(/^(!)\s+/) + end + if should_negate + resource_value, wrong_values = resource_value.collect do |value| + if value.is_a?(String) + wrong = value if ! value.match(/^!\s+/) + [value.sub(/^!\s*/, ''),wrong] + else + [value,nil] + end + end.transpose + wrong_values = wrong_values.compact + if ! wrong_values.empty? + fail "All values of the '#{res}' property must be prefixed with a '!' when inverting, but '#{wrong_values.join("', '")}' #{wrong_values.length>1?"are":"is"} not prefixed; aborting" + end + args.insert(-2, '!') + end end diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb index aeaabdb..cffab48 100644 --- a/lib/puppet/type/firewall.rb +++ b/lib/puppet/type/firewall.rb @@ -53,6 +53,7 @@ Puppet::Type.newtype(:firewall) do feature :isfirstfrag, "Match the first fragment of a fragmented ipv6 packet" feature :ipsec_policy, "Match IPsec policy" feature :ipsec_dir, "Match IPsec policy direction" + feature :mask, "Ability to match recent rules based on the ipv4 mask" # provider specific features feature :iptables, "The provider provides iptables features." @@ -323,7 +324,9 @@ Puppet::Type.newtype(:firewall) do *tcp*. EOS - newvalues(:tcp, :udp, :icmp, :"ipv6-icmp", :esp, :ah, :vrrp, :igmp, :ipencap, :ospf, :gre, :cbt, :all) + newvalues(*[:tcp, :udp, :icmp, :"ipv6-icmp", :esp, :ah, :vrrp, :igmp, :ipencap, :ospf, :gre, :cbt, :all].collect do |proto| + [proto, "! #{proto}".to_sym] + end.flatten) defaultto "tcp" end diff --git a/lib/puppet/util/firewall.rb b/lib/puppet/util/firewall.rb index aa26d3b..0f8bfdf 100644 --- a/lib/puppet/util/firewall.rb +++ b/lib/puppet/util/firewall.rb @@ -77,14 +77,11 @@ module Puppet::Util::Firewall proto = 'tcp' end - if value.kind_of?(String) - if value.match(/^\d+(-\d+)?$/) - return value - else - return Socket.getservbyname(value, proto).to_s - end + m = value.to_s.match(/^(!\s+)?(\S+)/) + if m[2].match(/^\d+(-\d+)?$/) + return "#{m[1]}#{m[2]}" else - Socket.getservbyname(value.to_s, proto).to_s + return "#{m[1]}#{Socket.getservbyname(m[2], proto).to_s}" end end diff --git a/spec/acceptance/invert_spec.rb b/spec/acceptance/invert_spec.rb new file mode 100644 index 0000000..aa04912 --- /dev/null +++ b/spec/acceptance/invert_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper_acceptance' + +describe 'firewall type', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do + before(:all) do + iptables_flush_all_tables + end + + context "inverting rules" do + it 'applies' do + pp = <<-EOS + class { '::firewall': } + firewall { '601 disallow esp protocol': + action => 'accept', + proto => '! esp', + } + firewall { '602 drop NEW external website packets with FIN/RST/ACK set and SYN unset': + chain => 'INPUT', + state => 'NEW', + action => 'drop', + proto => 'tcp', + sport => ['! http', '! 443'], + source => '! 10.0.0.0/8', + tcp_flags => '! FIN,SYN,RST,ACK SYN', + } + EOS + + apply_manifest(pp, :catch_failures => true) + apply_manifest(pp, :catch_changes => true) + end + + it 'should contain the rules' do + shell('iptables-save') do |r| + expect(r.stdout).to match(/-A INPUT ! -p esp -m comment --comment "601 disallow esp protocol" -j ACCEPT/) + expect(r.stdout).to match(/-A INPUT ! -s 10\.0\.0\.0\/8 -p tcp -m tcp ! --tcp-flags FIN,SYN,RST,ACK SYN -m multiport ! --sports 80,443 -m comment --comment "602 drop NEW external website packets with FIN\/RST\/ACK set and SYN unset" -m state --state NEW -j DROP/) + end + end + end + context "inverting partial array rules" do + it 'raises a failure' do + pp = <<-EOS + class { '::firewall': } + firewall { '603 drop 80,443 traffic': + chain => 'INPUT', + action => 'drop', + proto => 'tcp', + sport => ['! http', '443'], + } + EOS + + apply_manifest(pp, :expect_failures => true) do |r| + expect(r.stderr).to match(/is not prefixed/) + end + end + end +end diff --git a/spec/fixtures/iptables/conversion_hash.rb b/spec/fixtures/iptables/conversion_hash.rb index abe47a1..3024e80 100644 --- a/spec/fixtures/iptables/conversion_hash.rb +++ b/spec/fixtures/iptables/conversion_hash.rb @@ -473,6 +473,40 @@ ARGS_TO_HASH = { :action => 'reject', }, }, + 'disallow_esp_protocol' => { + :line => '-t filter ! -p esp -m comment --comment "063 disallow esp protocol" -j ACCEPT', + :table => 'filter', + :params => { + :name => '063 disallow esp protocol', + :action => 'accept', + :proto => '! esp', + }, + }, + 'drop_new_packets_without_syn' => { + :line => '-t filter ! -s 10.0.0.0/8 ! -p tcp -m tcp ! --tcp-flags FIN,SYN,RST,ACK SYN -m comment --comment "064 drop NEW non-tcp external packets with FIN/RST/ACK set and SYN unset" -m state --state NEW -j DROP', + :table => 'filter', + :params => { + :name => '064 drop NEW non-tcp external packets with FIN/RST/ACK set and SYN unset', + :state => ['NEW'], + :action => 'drop', + :proto => '! tcp', + :source => '! 10.0.0.0/8', + :tcp_flags => '! FIN,SYN,RST,ACK SYN', + }, + }, + 'negate_dport_and_sport' => { + :line => '-A nova-compute-FORWARD -s 0.0.0.0/32 -d 255.255.255.255/32 -p udp -m udp ! --sport 68,69 ! --dport 67,66 -j ACCEPT', + :table => 'filter', + :params => { + :action => 'accept', + :chain => 'nova-compute-FORWARD', + :source => '0.0.0.0/32', + :destination => '255.255.255.255/32', + :sport => ['! 68','! 69'], + :dport => ['! 67','! 66'], + :proto => 'udp', + }, + }, } # This hash is for testing converting a hash to an argument line. @@ -940,4 +974,40 @@ HASH_TO_ARGS = { }, :args => ["-t", :filter, "-p", :all, "-m", "comment", "--comment", "062 REJECT connmark", "-j", "REJECT", "-m", "connmark", "--mark", "0x1"], }, + 'disallow_esp_protocol' => { + :params => { + :name => '063 disallow esp protocol', + :table => 'filter', + :action => 'accept', + :proto => '! esp', + }, + :args => ["-t", :filter, "!", "-p", :esp, "-m", "comment", "--comment", "063 disallow esp protocol", "-j", "ACCEPT"], + }, + 'drop_new_packets_without_syn' => { + :params => { + :name => '064 drop NEW non-tcp external packets with FIN/RST/ACK set and SYN unset', + :table => 'filter', + :chain => 'INPUT', + :state => ['NEW'], + :action => 'drop', + :proto => '! tcp', + :source => '! 10.0.0.0/8', + :tcp_flags => '! FIN,SYN,RST,ACK SYN', + }, + :args => ["-t", :filter, "!", "-s", "10.0.0.0/8", "!", "-p", :tcp, "-m", "tcp", "!", "--tcp-flags", "FIN,SYN,RST,ACK", "SYN", "-m", "comment", "--comment", "064 drop NEW non-tcp external packets with FIN/RST/ACK set and SYN unset", "-m", "state", "--state", "NEW", "-j", "DROP"] + }, + 'negate_dport_and_sport' => { + :params => { + :name => '065 negate dport and sport', + :table => 'filter', + :action => 'accept', + :chain => 'nova-compute-FORWARD', + :source => '0.0.0.0/32', + :destination => '255.255.255.255/32', + :sport => ['! 68','! 69'], + :dport => ['! 67','! 66'], + :proto => 'udp', + }, + :args => ["-t", :filter, "-s", "0.0.0.0/32", "-d", "255.255.255.255/32", "-p", :udp, "-m", "multiport", "!", "--sports", "68,69", "-m", "multiport", "!", "--dports", "67,66", "-m", "comment", "--comment", "065 negate dport and sport", "-j", "ACCEPT"], + }, } diff --git a/spec/unit/puppet/provider/iptables_spec.rb b/spec/unit/puppet/provider/iptables_spec.rb index ad13fbe..e73bf84 100644 --- a/spec/unit/puppet/provider/iptables_spec.rb +++ b/spec/unit/puppet/provider/iptables_spec.rb @@ -315,6 +315,27 @@ describe 'iptables provider' do end end + describe 'when inverting rules' do + let(:resource) { + Puppet::Type.type(:firewall).new({ + :name => '040 partial invert', + :table => 'filter', + :action => 'accept', + :chain => 'nova-compute-FORWARD', + :source => '0.0.0.0/32', + :destination => '255.255.255.255/32', + :sport => ['! 78','79','http'], + :dport => ['77','! 76'], + :proto => 'udp', + }) + } + let(:instance) { provider.new(resource) } + + it 'fails when not all array items are inverted' do + expect { instance.insert }.to raise_error Puppet::Error, /but '79', '80' are not prefixed/ + end + end + describe 'when deleting resources' do let(:sample_rule) { '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061,7062 -j ACCEPT'