diff --git a/lib/puppet/provider/firewall/firewall.rb b/lib/puppet/provider/firewall/firewall.rb index d2c77c1ec..58b82b909 100644 --- a/lib/puppet/provider/firewall/firewall.rb +++ b/lib/puppet/provider/firewall/firewall.rb @@ -200,8 +200,7 @@ class Puppet::Provider::Firewall::Firewall # => multiport: (For some reason, the multiport arguments can't be) # specified within the same "-m multiport", but works in seperate # ones. - # => addrtype: Each instance of src_type/dst_type requires its own preface - # + # => addrtype: Each instance of src_type/dst_type requires it's own preface @module_to_argument_mapping = { physdev: [:physdev_in, :physdev_out, :physdev_is_bridged, :physdev_is_in, :physdev_is_out], iprange: [:src_range, :dst_range], @@ -339,7 +338,7 @@ def insync?(context, _name, property_name, is_hash, should_hash) is == should when :source, :destination - # Ensure source/destination has its valid mask before you compare it + # Ensure source/destination has it's valid mask before you compare it is_hash[property_name] == PuppetX::Firewall::Utility.host_to_mask(should_hash[property_name], should_hash[:protocol]) when :tcp_option, :ctproto, :hop_limit # Ensure that the values are compared as strings @@ -503,12 +502,14 @@ def self.rule_to_name(_context, rule, table_name, protocol) rule_hash[:table] = table_name rule_hash[:protocol] = protocol - name_regex = Regexp.new("#{$fw_resource_map[:name]}\\s+(?:\"(.+?(? 1 end when :state, :ctstate, :ctstatus, :month_days, :week_days - if rule.match(Regexp.new("#{value}\\s")) - value_regex = Regexp.new("(?:(!)\\s)?#{value}\\s(\\S+)") - key_value = rule.scan(value_regex) + if parse_rule.match(Regexp.new("#{token_prefix}#{Regexp.escape(value)}\\s")) + value_regex = Regexp.new("#{token_prefix}(?:(!)\\s)?#{Regexp.escape(value)}\\s(\\S+)") + key_value = parse_rule.scan(value_regex) split_value = key_value[0][1].split(%r{,}) - # If negated add to first value split_value[0] = [key_value[0][0], split_value[0]].join(' ') unless key_value[0][0].nil? - # If value is meant to be Int, return as such int_attr = [:month_days] split_value = split_value.map(&:to_i) if int_attr.include?(key) - # If only a single value is found, strip the Array wrapping split_value = split_value[0] if split_value.length == 1 rule_hash[key] = split_value end @@ -623,24 +627,22 @@ def self.rule_to_hash(_context, rule, table_name, protocol) proto = 1 end - if rule.match(Regexp.new("#{value[proto]}\\s")) - value_regex = Regexp.new("#{value[proto]}\\s(\\S+)") - key_value = rule.scan(value_regex)[0] + if parse_rule.match(Regexp.new("#{token_prefix}#{Regexp.escape(value[proto])}\\s")) + value_regex = Regexp.new("#{token_prefix}#{Regexp.escape(value[proto])}\\s(\\S+)") + key_value = parse_rule.scan(value_regex)[0] rule_hash[key] = key_value[0] end when :recent - if rule.match(Regexp.new("#{value}\\s")) - value_regex = Regexp.new("#{value}\\s(!\\s)?--(\\S+)") - key_value = rule.scan(value_regex)[0] - # If it has, combine the retrieved '!' with the actual value to make one string + if parse_rule.match(Regexp.new("#{token_prefix}#{Regexp.escape(value)}\\s")) + value_regex = Regexp.new("#{token_prefix}#{Regexp.escape(value)}\\s(!\\s)?--(\\S+)") + key_value = parse_rule.scan(value_regex)[0] key_value[1] = [key_value[0], key_value[1]].join unless key_value[0].nil? rule_hash[key] = key_value[1] if key_value end when :rpfilter - if rule.match(Regexp.new("#{value}\\s--")) - # Since the values are their own flags we can simply look for them directly - value_regex = Regexp.new("(?:\s--(invert|validmark|loose|accept-local))") - key_value = rule.scan(value_regex) + if parse_rule.match(Regexp.new("#{token_prefix}#{Regexp.escape(value)}\\s--")) + value_regex = Regexp.new('(?:\\s--(invert|validmark|loose|accept-local))') + key_value = parse_rule.scan(value_regex) return_value = [] key_value.each do |val| return_value << val[0] @@ -651,32 +653,17 @@ def self.rule_to_hash(_context, rule, table_name, protocol) when :proto, :source, :destination, :iniface, :outiface, :physdev_in, :physdev_out, :src_range, :dst_range, :tcp_option, :uid, :gid, :mac_source, :pkttype, :ctproto, :ctorigsrc, :ctorigdst, :ctreplsrc, :ctrepldst, :ctorigsrcport, :ctorigdstport, :ctreplsrcport, :ctrepldstport, :ctexpire, :cgroup, :hop_limit - # Values where negation is prior to the flag - # First find if flag is present, add a space to ensure accuracy with the more simplistic flags; i.e. `-i` - if rule.match(Regexp.new("#{value}\\s")) - value_regex = Regexp.new("(?:(!)\\s)?#{value}\\s(\\S+)") - key_value = rule.scan(value_regex)[0] - # If it has, combine the retrieved '!' with the actual value to make one string + if parse_rule.match(Regexp.new("#{token_prefix}#{Regexp.escape(value)}\\s")) + value_regex = Regexp.new("#{token_prefix}(?:(!)\\s)?#{Regexp.escape(value)}\\s(\\S+)") + key_value = parse_rule.scan(value_regex)[0] key_value[1] = [key_value[0], key_value[1]].join(' ') unless key_value[0].nil? rule_hash[key] = key_value[1] if key_value end - else # stat_mode, stat_every, stat_packet, stat_probability, socket, ipsec_dir, ipsec_policy, :ctdir, - # :limit, :burst, :length, :rseconds, :rhitcount, :rname, :mask, :string_algo, :string_from, :string_to, - # :jump, :goto, :clusterip_hashmode, :clusterip_clustermac, :clusterip_total_nodes, :clusterip_local_node, - # :clusterip_hash_init, :queue_num, :nflog_group, :nflog_range, :nflog_size, :nflog_threshold, - # :gateway, :set_mss, :set_dscp, :set_dscp_class, :todest, :tosource, :toports, :to, :log_level, - # :reject, :set_mark, :connlimit_upto, :connlimit_above, :connlimit_mask, :time_start, :time_stop, :date_start, - # :date_stop, :src_cc, :dst_cc, :hashlimit_upto, :hashlimit_above, :hashlimit_name, :hashlimit_burst, :hashlimit_mode, - # :hashlimit_srcmask, :hashlimit_dstmask, :hashlimit_htable_size, :hashlimit_htable_max, :hashlimit_htable_expire, - # :hashlimit_htable_gcinterval, :zone, :helper, :condition - # Default return, retrieve first complete block following the key value - # First find if flag is present, add a space to ensure accuracy with the more simplistic flags; i.e. `-j`, `--to` - if rule.match(Regexp.new("#{value}\\s")) - value_regex = Regexp.new("#{value}(?:\\s(!)\\s|\\s{1,2})(\\S+)") - key_value = rule.scan(value_regex)[0] - # If it has, combine the retrieved '!' with the actual value to make one string + else + if parse_rule.match(Regexp.new("#{token_prefix}#{Regexp.escape(value)}\\s")) + value_regex = Regexp.new("#{token_prefix}#{Regexp.escape(value)}(?:\\s(!)\\s|\\s{1,2})(\\S+)") + key_value = parse_rule.scan(value_regex)[0] key_value[1] = [key_value[0], key_value[1]].join(' ') unless key_value[0].nil? - # If value is meant to return as an integer/float ensure it does int_attr = [:stat_every, :stat_packet, :burst, :rseconds, :rhitcount, :string_from, :string_to, :clusterip_total_nodes, :clusterip_local_nodes, :nflog_group, :nflog_range, :nflog_size, :nflog_threshold, :set_mss, :connlimit_upto, :connlimit_above, :connlimit_mask, :hashlimit_burst, :hashlimit_srcmask, :hashlimit_dstmask, :hashlimit_htable_size, @@ -725,7 +712,7 @@ def self.process_get(_context, rule_hash, rule, counter) # Certain OS can return the proto as it;s equivalent number and we make sure to convert it in that case rule_hash[:proto] = PuppetX::Firewall::Utility.proto_number_to_name(rule_hash[:proto]) - # If a dscp numer is found, also return it as its valid class name + # If a dscp numer is found, also return it as it's valid class name rule_hash[:set_dscp_class] = PuppetX::Firewall::Utility.dscp_number_to_class(rule_hash[:set_dscp]) if rule_hash[:set_dscp] rule_hash @@ -975,7 +962,7 @@ def self.hash_to_rule(_context, _name, rule) arguments += " #{[$fw_resource_map[key][1], rule[key]].join(' ')}" end when :src_type, :dst_type, :ipset, :match_mark, :mss, :connmark - # Code for if value requires its own flag each time it is applied + # Code for if value requires it's own flag each time it is applied split_command = $fw_resource_map[key].split(%r{ }) negated_command = [split_command[0], split_command[1], '!', split_command[2]].join(' ') @@ -1057,7 +1044,7 @@ def self.insert_order(context, name, chain, table, protocol) # If the rule already exists, use it as the offset offset_rule = name else - # If it doesn't add it to the list and find its ordered location + # If it doesn't add it to the list and find it's ordered location rules << name new_rule_location = rules.sort.uniq.index(name) offset_rule = if new_rule_location.zero? diff --git a/spec/unit/puppet/provider/firewall/firewall_output_parsing_spec.rb b/spec/unit/puppet/provider/firewall/firewall_output_parsing_spec.rb index d5bbe235c..6fcecb67b 100644 --- a/spec/unit/puppet/provider/firewall/firewall_output_parsing_spec.rb +++ b/spec/unit/puppet/provider/firewall/firewall_output_parsing_spec.rb @@ -93,7 +93,7 @@ rttl: false, socket: false, table: 'filter', - time_contiguous: false + time_contiguous: false, }, { chain: 'INPUT', @@ -128,7 +128,7 @@ rttl: false, socket: false, table: 'filter', - time_contiguous: false + time_contiguous: false, }, { chain: 'INPUT', @@ -163,7 +163,7 @@ rttl: false, socket: false, table: 'filter', - time_contiguous: false + time_contiguous: false, }, { chain: 'TEST-ANOTHER', @@ -198,7 +198,7 @@ rttl: false, socket: false, table: 'filter', - time_contiguous: false + time_contiguous: false, }, { chain: 'TEST_ONE', @@ -233,7 +233,7 @@ rttl: false, socket: false, table: 'filter', - time_contiguous: false + time_contiguous: false, }, { chain: 'OUTPUT', @@ -268,7 +268,7 @@ rttl: false, socket: false, table: 'raw', - time_contiguous: false + time_contiguous: false, }, { chain: 'OUTPUT', @@ -303,7 +303,7 @@ rttl: false, socket: false, table: 'filter', - time_contiguous: false + time_contiguous: false, }, { chain: 'TEST_TWO', @@ -338,7 +338,7 @@ rttl: false, socket: false, table: 'raw', - time_contiguous: false + time_contiguous: false, }, ] end @@ -349,6 +349,138 @@ expect(provider.get(context)).to eq(returned_data) end + + context 'when comments contain proto-like flags' do + let(:iptables) do + <<~IPTABLES + # Generated by iptables-save v1.8.4 on Thu Aug 10 10:15:14 2023 + *filter + :INPUT ACCEPT [62:3308] + :FORWARD ACCEPT [0:0] + :OUTPUT ACCEPT [39:3092] + COMMIT + -A INPUT -m comment --comment "008 parser should ignore -p -p" + -A INPUT -m comment --comment '009 parser should ignore -p -p' + -A INPUT -p tcp -m comment --comment "010 parser should keep real proto despite -p -p" + -A INPUT -p tcp -m comment --comment '011 parser should keep real proto despite -p -p' + # Completed on Thu Aug 10 10:15:14 2023 + IPTABLES + end + + let(:ip6tables) do + <<~IP6TABLES + # Generated by ip6tables-save v1.8.4 on Thu Aug 10 10:21:55 2023 + *filter + :INPUT ACCEPT [0:0] + :FORWARD ACCEPT [0:0] + :OUTPUT ACCEPT [13:824] + COMMIT + # Completed on Thu Aug 10 10:21:55 2023 + IP6TABLES + end + + it 'processes single-quoted and double-quoted comments without mis-parsing proto' do + allow(Puppet::Util::Execution).to receive(:execute).with('iptables-save', { combine: false, failonfail: true }).and_return(iptables) + allow(Puppet::Util::Execution).to receive(:execute).with('ip6tables-save', { combine: false, failonfail: true }).and_return(ip6tables) + + parsed = provider.get(context) + + expect(parsed).to include( + a_hash_including( + chain: 'INPUT', + line: '-A INPUT -m comment --comment "008 parser should ignore -p -p"', + name: '008 parser should ignore -p -p', + protocol: 'IPv4', + table: 'filter', + proto: 'all', + ), + a_hash_including( + chain: 'INPUT', + line: "-A INPUT -m comment --comment '009 parser should ignore -p -p'", + name: '009 parser should ignore -p -p', + protocol: 'IPv4', + table: 'filter', + proto: 'all', + ), + a_hash_including( + chain: 'INPUT', + line: '-A INPUT -p tcp -m comment --comment "010 parser should keep real proto despite -p -p"', + name: '010 parser should keep real proto despite -p -p', + protocol: 'IPv4', + table: 'filter', + proto: 'tcp', + ), + a_hash_including( + chain: 'INPUT', + line: "-A INPUT -p tcp -m comment --comment '011 parser should keep real proto despite -p -p'", + name: '011 parser should keep real proto despite -p -p', + protocol: 'IPv4', + table: 'filter', + proto: 'tcp', + ), + ) + + expect(parsed.map { |rule| rule[:proto] }).not_to include('-p', '! -p') + end + end + + context 'when chain names contain option-like substrings' do + let(:iptables) do + <<~IPTABLES + # Generated by iptables-save v1.8.4 on Thu Aug 10 10:15:14 2023 + *filter + :INPUT ACCEPT [62:3308] + :FORWARD ACCEPT [0:0] + :OUTPUT ACCEPT [39:3092] + :COLDFRONT-ACCESS - [0:0] + :cali-po-_M_pLJ-KDF1NA7XPQJ-p - [0:0] + COMMIT + -A cali-po-_M_pLJ-KDF1NA7XPQJ-p -p udp -m comment --comment "012 chain name ends with -p" + -A COLDFRONT-ACCESS -p tcp -m comment --comment "013 chain name contains -A" + # Completed on Thu Aug 10 10:15:14 2023 + IPTABLES + end + + let(:ip6tables) do + <<~IP6TABLES + # Generated by ip6tables-save v1.8.4 on Thu Aug 10 10:21:55 2023 + *filter + :INPUT ACCEPT [0:0] + :FORWARD ACCEPT [0:0] + :OUTPUT ACCEPT [13:824] + COMMIT + # Completed on Thu Aug 10 10:21:55 2023 + IP6TABLES + end + + it 'parses proto flags using token boundaries instead of substrings in the chain name' do + allow(Puppet::Util::Execution).to receive(:execute).with('iptables-save', { combine: false, failonfail: true }).and_return(iptables) + allow(Puppet::Util::Execution).to receive(:execute).with('ip6tables-save', { combine: false, failonfail: true }).and_return(ip6tables) + + parsed = provider.get(context) + + expect(parsed).to include( + a_hash_including( + chain: 'cali-po-_M_pLJ-KDF1NA7XPQJ-p', + line: '-A cali-po-_M_pLJ-KDF1NA7XPQJ-p -p udp -m comment --comment "012 chain name ends with -p"', + name: '012 chain name ends with -p', + protocol: 'IPv4', + table: 'filter', + proto: 'udp', + ), + a_hash_including( + chain: 'COLDFRONT-ACCESS', + line: '-A COLDFRONT-ACCESS -p tcp -m comment --comment "013 chain name contains -A"', + name: '013 chain name contains -A', + protocol: 'IPv4', + table: 'filter', + proto: 'tcp', + ), + ) + + expect(parsed.map { |rule| rule[:proto] }).not_to include('-p', '! -p') + end + end end end end diff --git a/spec/unit/puppet/provider/firewall/firewall_private_get_spec.rb b/spec/unit/puppet/provider/firewall/firewall_private_get_spec.rb index 8f1bce61f..9bd193684 100644 --- a/spec/unit/puppet/provider/firewall/firewall_private_get_spec.rb +++ b/spec/unit/puppet/provider/firewall/firewall_private_get_spec.rb @@ -29,6 +29,21 @@ table_name: 'raw', protocol: 'IPv6', result: { ensure: 'present', table: 'raw', protocol: 'IPv6', name: '002 test rule', chain: 'OUTPUT' } }, + { + rule: "-A INPUT -p tcp -m comment --comment '003 single quoted test rule'", + table_name: 'filter', protocol: 'IPv4', + result: { ensure: 'present', table: 'filter', protocol: 'IPv4', name: '003 single quoted test rule', chain: 'INPUT' } + }, + { + rule: '-A cali-po-_M_pLJ-KDF1NA7XPQJ-p -p udp -m comment --comment "004 chain suffix test rule"', + table_name: 'filter', protocol: 'IPv4', + result: { ensure: 'present', table: 'filter', protocol: 'IPv4', name: '004 chain suffix test rule', chain: 'cali-po-_M_pLJ-KDF1NA7XPQJ-p' } + }, + { + rule: '-A COLDFRONT-ACCESS -p tcp -m comment --comment "005 chain contains -A test rule"', + table_name: 'filter', protocol: 'IPv4', + result: { ensure: 'present', table: 'filter', protocol: 'IPv4', name: '005 chain contains -A test rule', chain: 'COLDFRONT-ACCESS' } + }, ].each do |test| it "parses the rule: '#{test[:rule]}'" do expect(provider.rule_to_name(context, test[:rule], test[:table_name], test[:protocol])).to eq(test[:result]) @@ -339,6 +354,104 @@ end end end + + context 'when quoted comments contain proto-like flags' do + it 'does not parse -p from a double-quoted comment as proto' do + rule = '-A INPUT -m comment --comment "008 parser should ignore -p -p"' + + expect(provider.rule_to_hash(context, rule, 'filter', 'IPv4').sort).to eq( + boolean_block.merge( + line: rule, + ensure: 'present', + table: 'filter', + protocol: 'IPv4', + chain: 'INPUT', + name: '008 parser should ignore -p -p', + ).sort, + ) + end + + it 'does not parse -p from a single-quoted comment as proto' do + rule = "-A INPUT -m comment --comment '009 parser should ignore -p -p'" + + expect(provider.rule_to_hash(context, rule, 'filter', 'IPv4').sort).to eq( + boolean_block.merge( + line: rule, + ensure: 'present', + table: 'filter', + protocol: 'IPv4', + chain: 'INPUT', + name: '009 parser should ignore -p -p', + ).sort, + ) + end + + it 'still parses the real proto outside a double-quoted comment' do + rule = '-A INPUT -p tcp -m comment --comment "010 parser should keep real proto despite -p -p"' + + expect(provider.rule_to_hash(context, rule, 'filter', 'IPv4').sort).to eq( + boolean_block.merge( + line: rule, + ensure: 'present', + table: 'filter', + protocol: 'IPv4', + chain: 'INPUT', + name: '010 parser should keep real proto despite -p -p', + proto: 'tcp', + ).sort, + ) + end + + it 'still parses the real proto outside a single-quoted comment' do + rule = "-A INPUT -p tcp -m comment --comment '011 parser should keep real proto despite -p -p'" + + expect(provider.rule_to_hash(context, rule, 'filter', 'IPv4').sort).to eq( + boolean_block.merge( + line: rule, + ensure: 'present', + table: 'filter', + protocol: 'IPv4', + chain: 'INPUT', + name: '011 parser should keep real proto despite -p -p', + proto: 'tcp', + ).sort, + ) + end + end + + context 'when chain names contain option-like substrings' do + it 'parses the real proto when the chain name ends with -p' do + rule = '-A cali-po-_M_pLJ-KDF1NA7XPQJ-p -p udp -m comment --comment "012 chain name ends with -p"' + + expect(provider.rule_to_hash(context, rule, 'filter', 'IPv4').sort).to eq( + boolean_block.merge( + line: rule, + ensure: 'present', + table: 'filter', + protocol: 'IPv4', + chain: 'cali-po-_M_pLJ-KDF1NA7XPQJ-p', + name: '012 chain name ends with -p', + proto: 'udp', + ).sort, + ) + end + + it 'parses the chain correctly when the chain name contains -A' do + rule = '-A COLDFRONT-ACCESS -p tcp -m comment --comment "013 chain name contains -A"' + + expect(provider.rule_to_hash(context, rule, 'filter', 'IPv4').sort).to eq( + boolean_block.merge( + line: rule, + ensure: 'present', + table: 'filter', + protocol: 'IPv4', + chain: 'COLDFRONT-ACCESS', + name: '013 chain name contains -A', + proto: 'tcp', + ).sort, + ) + end + end end describe 'self.validate_get(_context, rules)' do