From cb0b412ba577ba32ae16286456ad8d3a2d3128bb Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Wed, 10 May 2023 16:42:36 -0700 Subject: [PATCH 01/19] Fix Rubocop 'Metrics/BlockLength' config Rubocop's maximum value for allowed block length needs to be incremented by hand each time the 'firewall' type gains a new feature, because the maximum block length is hand-tuned to the size of that 'newtype' block. Having the 'Metrics/BlockLength' cop instead ignore lengths for the 'newtype' and 'provider' methods (which inherently have very long blocks) instead solves the problem for all 'newtype' and 'provider' blocks at one fell swoop, and with addition of one inline disable comment for the 'validate' block in the 'firewall' type, means all remaining block length warnings are squelched by a max value of only 64 lines. --- .rubocop.yml | 6 +++++- .rubocop_todo.yml | 4 ++-- lib/puppet/type/firewall.rb | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index fec02db99..130d69f0f 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -26,6 +26,10 @@ AllCops: Layout/LineLength: Description: People have wide screens, use them. Max: 200 +Metrics/BlockLength: + AllowedMethods: + - provide + - newtype RSpec/BeforeAfterAll: Description: Beware of using after(:all) as it may cause state to leak between tests. A necessary evil in acceptance testing. @@ -83,4 +87,4 @@ Style/Documentation: - lib/puppet/parser/functions/**/* - spec/**/* Style/WordArray: - EnforcedStyle: brackets \ No newline at end of file + EnforcedStyle: brackets diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 266218cf8..108e85171 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -114,11 +114,11 @@ Lint/RedundantSafeNavigation: Metrics/AbcSize: Max: 235 -# Offense count: 23 +# Offense count: 17 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns, inherit_mode. # AllowedMethods: refine Metrics/BlockLength: - Max: 1961 + Max: 64 # Offense count: 2 # Configuration parameters: CountBlocks. diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb index 86af41b31..79fec3a71 100644 --- a/lib/puppet/type/firewall.rb +++ b/lib/puppet/type/firewall.rb @@ -2359,7 +2359,7 @@ def should_to_s(value) ['/etc/sysconfig/iptables', '/etc/sysconfig/ip6tables'] end - validate do + validate do # rubocop:disable Metrics/BlockLength debug('[validate]') # TODO: this is put here to skip validation if ensure is not set. This From f259aa1b0a5f16ab6d68dfa4ec6f9c2f4109c295 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Sat, 6 May 2023 03:49:51 -0700 Subject: [PATCH 02/19] Support parsing iptables SYNPROXY rules Add the ability to parse iptables rules we encounter that include any option for the SYNPROXY jump target (including RedHat's --ecn option), instead of issuing a "Skipping unparseable iptables rule" warning. Every Firewall resource requires a full parse of all rules, and the warning appears each time the problem rule is parsed; therefore these warning messages are noisy (especially on long rule sets), and there is no way to work around this, make the parser ignore the rule, or create other rules with Puppet in any order aside from "above the unparseable rule." --- lib/puppet/provider/firewall/iptables.rb | 8 ++++++++ spec/fixtures/iptables/conversion_hash.rb | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 9d26f03a4..fca020b92 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -172,6 +172,11 @@ string_algo: '--algo', string_from: '--from', string_to: '--to', + synproxy_ecn: '--ecn', + synproxy_mss: '--mss', + synproxy_sack_perm: '--sack-perm', + synproxy_timestamp: '--timestamp', + synproxy_wscale: '--wscale', table: '-t', tcp_flags: ['-m tcp --tcp-flags', '--tcp-flags'], todest: '--to-destination', @@ -240,6 +245,9 @@ :physdev_is_bridged, :physdev_is_in, :physdev_is_out, + :synproxy_ecn, + :synproxy_sack_perm, + :synproxy_timestamp, :time_contiguous, :kernel_timezone, :clusterip_new, diff --git a/spec/fixtures/iptables/conversion_hash.rb b/spec/fixtures/iptables/conversion_hash.rb index d593f93ca..f3ccebd3a 100644 --- a/spec/fixtures/iptables/conversion_hash.rb +++ b/spec/fixtures/iptables/conversion_hash.rb @@ -794,6 +794,27 @@ notrack: true } }, + 'parses_synproxy_rule' => { + line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --sack-perm --timestamp --wscale 9 --mss 1460 --ecn', + table: 'filter', + compare_all: true, + params: { + chain: 'INPUT', + dport: ['80'], + ensure: :present, + jump: 'SYNPROXY', + line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --sack-perm --timestamp --wscale 9 --mss 1460 --ecn', + name: '001 parses rule with synproxy target', + proto: 'tcp', + provider: 'iptables', + synproxy_ecn: true, + synproxy_mss: '1460', + synproxy_sack_perm: true, + synproxy_timestamp: true, + synproxy_wscale: '9', + table: 'filter', + }, + }, }.freeze # This hash is for testing converting a hash to an argument line. From 591dc77ea00371b362f08766f138e1c18a7a92e0 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Tue, 9 May 2023 22:13:22 -0700 Subject: [PATCH 03/19] Support creating SYNPROXY jump target rules Add support (and unit tests) for creating rules that use the SYNPROXY iptables extension and its configuration options, including the undocumented, RedHat-supported '--ecn' option. --- lib/puppet/provider/firewall/iptables.rb | 2 + lib/puppet/type/firewall.rb | 68 +++++++++++++++++++++++ spec/fixtures/iptables/conversion_hash.rb | 62 +++++++++++++++++++++ spec/unit/puppet/type/firewall_spec.rb | 60 +++++++++++++++++++- 4 files changed, 191 insertions(+), 1 deletion(-) diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index fca020b92..717df6631 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -34,6 +34,7 @@ has_feature :nflog_prefix has_feature :nflog_range has_feature :nflog_threshold + has_feature :synproxy has_feature :tcp_flags has_feature :pkttype has_feature :isfragment @@ -367,6 +368,7 @@ def munge_resource_map_from_resource(resource_map_original, compare) :nflog_group, :nflog_prefix, :nflog_range, :nflog_size, :nflog_threshold, :clamp_mss_to_pmtu, :gateway, :set_mss, :set_dscp, :set_dscp_class, :todest, :tosource, :toports, :to, :checksum_fill, :random_fully, :random, :log_prefix, :log_level, :log_uid, :log_tcp_sequence, :log_tcp_options, :log_ip_options, :reject, :set_mark, :match_mark, :mss, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, + :synproxy_sack_perm, :synproxy_timestamp, :synproxy_wscale, :synproxy_mss, :synproxy_ecn, :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone, :src_cc, :dst_cc, :hashlimit_upto, :hashlimit_above, :hashlimit_name, :hashlimit_burst, :hashlimit_mode, :hashlimit_srcmask, :hashlimit_dstmask, :hashlimit_htable_size, diff --git a/lib/puppet/type/firewall.rb b/lib/puppet/type/firewall.rb index 79fec3a71..44a6b445f 100644 --- a/lib/puppet/type/firewall.rb +++ b/lib/puppet/type/firewall.rb @@ -134,6 +134,8 @@ * string_matching: The ability to match a given string by using some pattern matching strategy. + * synproxy: The ability to use the SYNPROXY jump target. + * tcp_flags: The ability to match on particular TCP flag settings. * netmap: The ability to map entire subnets via source or destination nat rules. @@ -171,6 +173,7 @@ feature :log_ip_options, 'Add IP/IPv6 packet header to log messages' feature :mark, 'Match or Set the netfilter mark value associated with the packet' feature :mss, 'Match a given TCP MSS value or range.' + feature :synproxy, 'Use the SYNPROXY extension' feature :tcp_flags, 'The ability to match on particular TCP flag settings' feature :pkttype, 'Match a packet type' feature :rpfilter, 'Perform reverse-path filtering' @@ -2312,6 +2315,63 @@ def should_to_s(value) end end + newproperty(:synproxy_ecn, required_features: :synproxy) do + desc <<-PUPPETCODE + If the SYNPROXY jump target is being used, whether to supply the `--ecn` option to it. This + option is undocumented in the main iptables-extensions documentation, and appears to be unique + to and supported by RedHat; see RedHat's documentation for what it does. + PUPPETCODE + newvalues(:true, :false) + end + + newproperty(:synproxy_mss, required_features: :synproxy) do + desc <<-PUPPETCODE + If the SYNPROXY jump target is being used, the TCP Maxiumum Segment Size for the proxied + interface. If SYNPROXY is being used to protect another host that we forward traffic to, + then this should be the Maximum Segement Size for the interface that host uses to receive the + forwarded traffic. Integer or Stringified Integer value between 0 and 65535. For more + information, see RFC 6691. + PUPPETCODE + validate do |value| + unless value.to_i.bit_length <= 16 && value.to_i > 0 + raise ArgumentError, 'Segment size must fit within an unsigned 16-bit integer' + end + end + munge { |value| value.to_s } + end + + newproperty(:synproxy_sack_perm, required_features: :synproxy) do + desc <<-PUPPETCODE + If the SYNPROXY jump target is being used, whether or not to enable Selective Acknowledgements + on behalf of the proxied interface. This Boolean value should match the numeric truth value + of net.ipv4.tcp_sack on the protected system. Boolean, omitted by default. + PUPPETCODE + newvalues(:true, :false) + end + + newproperty(:synproxy_timestamp, required_features: :synproxy) do + desc <<-PUPPETCODE + If the SYNPROXY jump target is being used, whether to pass client timestamp option to proxied + interface. Will be disabled if not present; this option is needed for selective + acknowledgement and window scaling. Boolean, omitted by default. + PUPPETCODE + newvalues(:true, :false) + end + + newproperty(:synproxy_wscale, required_features: :synproxy) do + desc <<-PUPPETCODE + If the SYNPROXY jump target is being used, this should be the TCP Window Scaling Factor for + the proxied interface (for more information on TCP Window Scaling, see RFC7323). Integer or + Stringified Integer between 1 and 14. + PUPPETCODE + validate do |value| + unless value.to_i >= 1 && value.to_i <= 14 + raise ArgumentError, 'Window scale exponent must be between 1 and 14' + end + end + munge { |value| value.to_s } + end + autorequire(:firewallchain) do reqs = [] protocol = nil @@ -2545,5 +2605,13 @@ def should_to_s(value) raise 'Parameter jump => CT only applies to table => raw' end end + if value(:jump).to_s != 'SYNPROXY' && (value(:synproxy_ecn) || value(:synproxy_mss) || value(:synproxy_sack_perm) || value(:synproxy_timestamp) || value(:synproxy_wscale)) + raise 'Options for SYNPROXY jump target are only valid when the SYNPROXY target is used' + end + if value(:jump).to_s == 'SYNPROXY' && (value(:synproxy_sack_perm) || value(:synproxy_wscale)) && value(:synproxy_timestamp).nil? + raise <<~PUPPETCODE + When using SYNPROXY jump target and Selective Acknowledgements or TCP Window Scaliing are enabled, you must also enable timestamps + PUPPETCODE + end end end diff --git a/spec/fixtures/iptables/conversion_hash.rb b/spec/fixtures/iptables/conversion_hash.rb index f3ccebd3a..407551887 100644 --- a/spec/fixtures/iptables/conversion_hash.rb +++ b/spec/fixtures/iptables/conversion_hash.rb @@ -815,6 +815,24 @@ table: 'filter', }, }, + 'parses_synproxy_rule_without_booleans' => { + line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --wscale 9 --mss 1460', + table: 'filter', + compare_all: true, + params: { + chain: 'INPUT', + dport: ['80'], + ensure: :present, + jump: 'SYNPROXY', + line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --wscale 9 --mss 1460', + name: '001 parses rule with synproxy target', + proto: 'tcp', + provider: 'iptables', + synproxy_mss: '1460', + synproxy_wscale: '9', + table: 'filter', + }, + }, }.freeze # This hash is for testing converting a hash to an argument line. @@ -1465,4 +1483,48 @@ }, args: ['-t', :filter, '-s', '1.2.3.4/32', '-d', '4.3.2.1/32', '-p', :tcp, '-j', 'NFQUEUE', '-m', 'comment', '--comment', '003 nfqueue dont specify queue_num or queue_bypass'], }, + 'creates_synproxy_rule' => { + params: { + name: '001 creates rule with synproxy target', + chain: 'INPUT', + ensure: :present, + dport: ['80'], + jump: 'SYNPROXY', + synproxy_ecn: true, + synproxy_mss: 1460, + synproxy_sack_perm: true, + synproxy_timestamp: true, + synproxy_wscale: 9, + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '80', '-j', 'SYNPROXY', '--sack-perm', '--timestamp', '--wscale', '9', '--mss', '1460', '--ecn', '-m', 'comment', '--comment', '001 creates rule with synproxy target'], + }, + 'creates_synproxy_rule_with_stringified_integers' => { + params: { + name: '001 creates rule with synproxy target', + chain: 'INPUT', + ensure: :present, + dport: ['80'], + jump: 'SYNPROXY', + synproxy_ecn: true, + synproxy_mss: '1460', + synproxy_sack_perm: true, + synproxy_timestamp: true, + synproxy_wscale: '9', + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '80', '-j', 'SYNPROXY', '--sack-perm', '--timestamp', '--wscale', '9', '--mss', '1460', '--ecn', '-m', 'comment', '--comment', '001 creates rule with synproxy target'], + }, + 'creates_synproxy_rule_with_booleans_set_false' => { + params: { + name: '001 creates rule with synproxy target', + chain: 'INPUT', + ensure: :present, + dport: ['80'], + jump: 'SYNPROXY', + synproxy_ecn: false, + synproxy_mss: '1460', + synproxy_sack_perm: false, + synproxy_timestamp: false, + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '80', '-j', 'SYNPROXY', '--mss', '1460', '-m', 'comment', '--comment', '001 creates rule with synproxy target'], + }, }.freeze diff --git a/spec/unit/puppet/type/firewall_spec.rb b/spec/unit/puppet/type/firewall_spec.rb index a31f022c4..88def6401 100755 --- a/spec/unit/puppet/type/firewall_spec.rb +++ b/spec/unit/puppet/type/firewall_spec.rb @@ -101,7 +101,7 @@ expect(res.parameters[:jump]).to be nil end - ['QUEUE', 'RETURN', 'DNAT', 'SNAT', 'LOG', 'NFLOG', 'MASQUERADE', 'REDIRECT', 'MARK'].each do |jump| + ['QUEUE', 'RETURN', 'DNAT', 'SNAT', 'LOG', 'NFLOG', 'MASQUERADE', 'REDIRECT', 'MARK', 'SYNPROXY'].each do |jump| it "accepts jump value #{jump}" do resource[:jump] = jump expect(resource[:jump]).to eql jump @@ -538,6 +538,64 @@ end end + describe 'individual SYNPROXY options' do + describe ':synproxy_mss' do + ['1', 1, '65535', 65_535].each do |v| + it 'resolves correctly when given valid values' do + resource[:synproxy_mss] = v + expect(resource[:synproxy_mss]).to eq v.to_s + end + end + + ['0', 0, '65536', 65_536].each do |v| + it 'produces the expected error when given invalid values' do + expect { resource[:synproxy_mss] = v }.to raise_error(Puppet::Error, %r{Segment size must fit within an unsigned 16-bit integer}) + end + end + end + + describe ':synproxy_wscale' do + ['1', 1, '14', 14].each do |v| + it 'resolves correctly when given valid values' do + resource[:synproxy_wscale] = v + expect(resource[:synproxy_wscale]).to eq v.to_s + end + end + + ['0', 0, '15', 15].each do |v| + it 'produces the expected error when given invalid values' do + expect { resource[:synproxy_wscale] = v }.to raise_error(Puppet::Error, %r{Window scale exponent must be between 1 and 14}) + end + end + end + end + + describe 'SYNPROXY option combinatorics' do + it 'does not allow any SYNPROXY-related options to be set if :jump is not SYNPROXY' do + { + synproxy_ecn: true, + synproxy_mss: 1024, + synproxy_sack_perm: true, + synproxy_timestamp: true, + synproxy_wscale: 10, + }.each do |syn_param, syn_value| + expect { + firewall_class.new(:name => '001-test', :jump => 'custom_chain', syn_param => syn_value) + }.to raise_error(RuntimeError, %r{Options for SYNPROXY jump target are only valid when the SYNPROXY target is used}) + end + end + it 'does not allow either synproxy_sack_perm or synproxy_wscale to be set if synproxy_timestamp is omitted' do + { + synproxy_sack_perm: true, + synproxy_wscale: 10, + }.each do |syn_param, syn_value| + expect { + firewall_class.new(:name => '001-test', :jump => 'SYNPROXY', syn_param => syn_value) + }.to raise_error(RuntimeError, %r{SYNPROXY.*Selective Acknowledgements or TCP Window Scaliing.*enable timestamps}) + end + end + end + describe ':burst' do it 'accepts numeric values' do resource[:burst] = 12 From f0e5d6e0075f503cb34be8e7f141f795d40769b1 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 11 May 2023 17:21:34 -0700 Subject: [PATCH 04/19] Move ip6tables provider tests to correct file Move tests for the ip6tables provider into the ip6tables provider unit test file. Even if it's subclassed from iptables, it should be reasonable to expect running all tests in the ip6tables provider's tests file to include all tests for the ip6tables provider. Also update several test definitions that had fallen out of sync with the iptables provider's test framework so the test cases for ip6tables are also no longer sensitive to specific hash order and stringified 'true'. --- spec/unit/puppet/provider/ip6tables_spec.rb | 101 +++++++++++++++++++- spec/unit/puppet/provider/iptables_spec.rb | 95 ------------------ 2 files changed, 100 insertions(+), 96 deletions(-) diff --git a/spec/unit/puppet/provider/ip6tables_spec.rb b/spec/unit/puppet/provider/ip6tables_spec.rb index cd4a7fea7..ad2b987d0 100644 --- a/spec/unit/puppet/provider/ip6tables_spec.rb +++ b/spec/unit/puppet/provider/ip6tables_spec.rb @@ -5,7 +5,7 @@ require 'puppet/confine/exists' provider_class = Puppet::Type.type(:firewall).provider(:ip6tables) -describe 'ip6tables' do +describe 'ip6tables' do # rubocop:disable RSpec/MultipleDescribes let(:params) { { name: '000 test foo', action: 'accept' } } let(:provider) { provider_class } let(:resource) { Puppet::Type.type(:firewall) } @@ -56,3 +56,102 @@ def stub_iptables it_behaves_like 'run' end end + +describe 'ip6tables provider' do + let(:provider6) { Puppet::Type.type(:firewall).provider(:ip6tables) } + let(:resource) do + Puppet::Type.type(:firewall).new(name: '000 test foo', + action: 'accept', + provider: 'ip6tables') + end + + before :each do + allow(Puppet::Type::Firewall).to receive(:ip6tables).and_return provider6 + allow(provider6).to receive(:command).with(:ip6tables_save).and_return '/sbin/ip6tables-save' + + # Stub iptables version + allow(Facter.fact(:ip6tables_version)).to receive(:value).and_return '1.4.7' + + allow(Puppet::Util::Execution).to receive(:execute).and_return '' + allow(Puppet::Util).to receive(:which).with('ip6tables-save') + .and_return '/sbin/ip6tables-save' + end + + it 'is expected to be able to get a list of existing rules' do + provider6.instances.each do |rule| + expect(rule).to be_instance_of(provider6) + expect(rule.properties[:provider6].to_s).to eql provider6.name.to_s + end + end + + it 'is expected to ignore lines with fatal errors' do + allow(Puppet::Util::Execution).to receive(:execute).with(['/sbin/ip6tables-save']) + .and_return('FATAL: Could not load /lib/modules/2.6.18-028stab095.1/modules.dep: No such file or directory') + expect(provider6.instances.length).to eq 0 + end + + # Load in ruby hash for test fixtures. + load 'spec/fixtures/ip6tables/conversion_hash.rb' + + describe 'when converting rules to resources' do + ARGS_TO_HASH6.each do |test_name, data| + describe "for test data '#{test_name}'" do + let(:resource) { provider6.rule_to_hash(data[:line], data[:table], 0) } + + # If this option is enabled, make sure the parameters exactly match + if data[:compare_all] + it 'the parameter hash keys should be the same as returned by rules_to_hash' do + expect(resource.keys).to match_array(data[:params].keys) + end + end + + # Iterate across each parameter, creating an example for comparison + data[:params].each do |param_name, param_value| + it "the parameter '#{param_name}' should match #{param_value.inspect}" do + if param_value == true + expect(resource[param_name]).to be_truthy + else + expect(resource[param_name]).to eq(data[:params][param_name]) + end + end + end + end + end + end + + describe 'when working out general_args' do + HASH_TO_ARGS6.each do |test_name, data| + describe "for test data '#{test_name}'" do + let(:resource) { Puppet::Type.type(:firewall).new(data[:params]) } + let(:provider6) { Puppet::Type.type(:firewall).provider(:ip6tables) } + let(:instance) { provider6.new(resource) } + + it 'general_args should be valid' do + data[:args].unshift('--wait') if instance.general_args.flatten.include? '--wait' + expect(instance.general_args.flatten).to eql data[:args] + end + end + end + end + + describe 'when deleting ipv6 resources' do + let(:sample_rule) do + '-A INPUT -i lo -m comment --comment "001 accept all to lo interface v6" -j ACCEPT' + end + + let(:bare_sample_rule) do + '-A INPUT -i lo -m comment --comment 001 accept all to lo interface v6 -j ACCEPT' + end + + let(:resource) { provider6.rule_to_hash(sample_rule, 'filter', 0) } + let(:instance) { provider6.new(resource) } + + it 'resource[:line] looks like the original rule' do + resource[:line] == sample_rule + end + + it 'delete_args is an array' do + expect(instance.delete_args.class).to eq(Array) + end + end +end diff --git a/spec/unit/puppet/provider/iptables_spec.rb b/spec/unit/puppet/provider/iptables_spec.rb index f34f586da..ea9e86dbd 100644 --- a/spec/unit/puppet/provider/iptables_spec.rb +++ b/spec/unit/puppet/provider/iptables_spec.rb @@ -353,98 +353,3 @@ end end end - -describe 'ip6tables provider' do - let(:provider6) { Puppet::Type.type(:firewall).provider(:ip6tables) } - let(:resource) do - Puppet::Type.type(:firewall).new(name: '000 test foo', - action: 'accept', - provider: 'ip6tables') - end - - before :each do - allow(Puppet::Type::Firewall).to receive(:ip6tables).and_return provider6 - allow(provider6).to receive(:command).with(:ip6tables_save).and_return '/sbin/ip6tables-save' - - # Stub iptables version - allow(Facter.fact(:ip6tables_version)).to receive(:value).and_return '1.4.7' - - allow(Puppet::Util::Execution).to receive(:execute).and_return '' - allow(Puppet::Util).to receive(:which).with('ip6tables-save') - .and_return '/sbin/ip6tables-save' - end - - it 'is expected to be able to get a list of existing rules' do - provider6.instances.each do |rule| - expect(rule).to be_instance_of(provider6) - expect(rule.properties[:provider6].to_s).to eql provider6.name.to_s - end - end - - it 'is expected to ignore lines with fatal errors' do - allow(Puppet::Util::Execution).to receive(:execute).with(['/sbin/ip6tables-save']) - .and_return('FATAL: Could not load /lib/modules/2.6.18-028stab095.1/modules.dep: No such file or directory') - expect(provider6.instances.length).to eq 0 - end - - # Load in ruby hash for test fixtures. - load 'spec/fixtures/ip6tables/conversion_hash.rb' - - describe 'when converting rules to resources' do - ARGS_TO_HASH6.each do |test_name, data| - describe "for test data '#{test_name}'" do - let(:resource) { provider6.rule_to_hash(data[:line], data[:table], 0) } - - # If this option is enabled, make sure the parameters exactly match - if data[:compare_all] - it 'the parameter hash keys should be the same as returned by rules_to_hash' do - expect(resource.keys).to match data[:params].keys - end - end - - # Iterate across each parameter, creating an example for comparison - data[:params].each do |param_name, param_value| - it "the parameter '#{param_name}' should match #{param_value.inspect}" do - expect(resource[param_name]).to eql data[:params][param_name] - end - end - end - end - end - - describe 'when working out general_args' do - HASH_TO_ARGS6.each do |test_name, data| - describe "for test data '#{test_name}'" do - let(:resource) { Puppet::Type.type(:firewall).new(data[:params]) } - let(:provider6) { Puppet::Type.type(:firewall).provider(:ip6tables) } - let(:instance) { provider6.new(resource) } - - it 'general_args should be valid' do - data[:args].unshift('--wait') if instance.general_args.flatten.include? '--wait' - expect(instance.general_args.flatten).to eql data[:args] - end - end - end - end - - describe 'when deleting ipv6 resources' do - let(:sample_rule) do - '-A INPUT -i lo -m comment --comment "001 accept all to lo interface v6" -j ACCEPT' - end - - let(:bare_sample_rule) do - '-A INPUT -i lo -m comment --comment 001 accept all to lo interface v6 -j ACCEPT' - end - - let(:resource) { provider6.rule_to_hash(sample_rule, 'filter', 0) } - let(:instance) { provider6.new(resource) } - - it 'resource[:line] looks like the original rule' do - resource[:line] == sample_rule - end - - it 'delete_args is an array' do - expect(instance.delete_args.class).to eq(Array) - end - end -end From 9e039c424891d7b1e4ef563bd62fd0849b75c606 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 11 May 2023 18:18:47 -0700 Subject: [PATCH 05/19] Support parsing and creating IPv6 SYNPROXY rules Provide the same support for rule parse and create for IPv6 as the previous commit provided for IPv4 --- lib/puppet/provider/firewall/ip6tables.rb | 13 +++- spec/fixtures/ip6tables/conversion_hash.rb | 85 +++++++++++++++++++++- 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/lib/puppet/provider/firewall/ip6tables.rb b/lib/puppet/provider/firewall/ip6tables.rb index 77a2a79ff..6681c7218 100644 --- a/lib/puppet/provider/firewall/ip6tables.rb +++ b/lib/puppet/provider/firewall/ip6tables.rb @@ -29,6 +29,7 @@ has_feature :nflog_prefix has_feature :nflog_range has_feature :nflog_threshold + has_feature :synproxy has_feature :tcp_flags has_feature :pkttype has_feature :ishasmorefrags @@ -182,6 +183,11 @@ def self.iptables_save(*args) string_algo: '--algo', string_from: '--from', string_to: '--to', + synproxy_ecn: '--ecn', + synproxy_mss: '--mss', + synproxy_sack_perm: '--sack-perm', + synproxy_timestamp: '--timestamp', + synproxy_wscale: '--wscale', table: '-t', tcp_flags: '-m tcp --tcp-flags', todest: '--to-destination', @@ -242,6 +248,9 @@ def self.iptables_save(*args) :physdev_is_bridged, :physdev_is_in, :physdev_is_out, + :synproxy_ecn, + :synproxy_sack_perm, + :synproxy_timestamp, :time_contiguous, :kernel_timezone, :queue_bypass, @@ -323,7 +332,9 @@ def self.iptables_save(*args) :clamp_mss_to_pmtu, :gateway, :todest, :tosource, :toports, :checksum_fill, :log_level, :log_prefix, :log_uid, :log_tcp_sequence, :log_tcp_options, :log_ip_options, :random_fully, :reject, :set_mss, :set_dscp, :set_dscp_class, :mss, :queue_num, :queue_bypass, - :set_mark, :match_mark, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone, + :set_mark, :match_mark, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, + :synproxy_sack_perm, :synproxy_timestamp, :synproxy_wscale, :synproxy_mss, :synproxy_ecn, + :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone, :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, :bytecode, :zone, :helper, :rpfilter, :condition, :name, :notrack] diff --git a/spec/fixtures/ip6tables/conversion_hash.rb b/spec/fixtures/ip6tables/conversion_hash.rb index 1f27ced92..f0b045365 100644 --- a/spec/fixtures/ip6tables/conversion_hash.rb +++ b/spec/fixtures/ip6tables/conversion_hash.rb @@ -49,7 +49,46 @@ params: { random_fully: 'true', } - } + }, + 'parses_synproxy_rule' => { + line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --sack-perm --timestamp --wscale 9 --mss 1460 --ecn', + compare_all: true, + table: 'filter', + params: { + chain: 'INPUT', + dport: ['80'], + ensure: :present, + jump: 'SYNPROXY', + line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --sack-perm --timestamp --wscale 9 --mss 1460 --ecn', + name: '001 parses rule with synproxy target', + proto: 'tcp', + provider: 'ip6tables', + synproxy_ecn: true, + synproxy_mss: '1460', + synproxy_sack_perm: true, + synproxy_timestamp: true, + synproxy_wscale: '9', + table: 'filter', + }, + }, + 'parses_synproxy_rule_without_booleans' => { + line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --wscale 9 --mss 1460', + compare_all: true, + table: 'filter', + params: { + chain: 'INPUT', + dport: ['80'], + ensure: :present, + jump: 'SYNPROXY', + line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --wscale 9 --mss 1460', + name: '001 parses rule with synproxy target', + proto: 'tcp', + provider: 'ip6tables', + synproxy_mss: '1460', + synproxy_wscale: '9', + table: 'filter', + }, + }, }.freeze # This hash is for testing converting a hash to an argument line. @@ -141,4 +180,48 @@ }, args: ['-t', :filter, '-p', :tcp, '-j', 'NFLOG', '--nflog-group', 1, '--nflog-prefix', 'myprefix', '-m', 'comment', '--comment', '100 nflog'], }, + 'creates_synproxy_rule' => { + params: { + name: '001 creates rule with synproxy target', + chain: 'INPUT', + ensure: :present, + dport: ['80'], + jump: 'SYNPROXY', + synproxy_ecn: true, + synproxy_mss: 1460, + synproxy_sack_perm: true, + synproxy_timestamp: true, + synproxy_wscale: 9, + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '80', '-j', 'SYNPROXY', '--sack-perm', '--timestamp', '--wscale', '9', '--mss', '1460', '--ecn', '-m', 'comment', '--comment', '001 creates rule with synproxy target'], + }, + 'creates_synproxy_rule_with_stringified_integers' => { + params: { + name: '001 creates rule with synproxy target', + chain: 'INPUT', + ensure: :present, + dport: ['80'], + jump: 'SYNPROXY', + synproxy_ecn: true, + synproxy_mss: '1460', + synproxy_sack_perm: true, + synproxy_timestamp: true, + synproxy_wscale: '9', + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '80', '-j', 'SYNPROXY', '--sack-perm', '--timestamp', '--wscale', '9', '--mss', '1460', '--ecn', '-m', 'comment', '--comment', '001 creates rule with synproxy target'], + }, + 'creates_synproxy_rule_with_booleans_set_false' => { + params: { + name: '001 creates rule with synproxy target', + chain: 'INPUT', + ensure: :present, + dport: ['80'], + jump: 'SYNPROXY', + synproxy_ecn: false, + synproxy_mss: '1460', + synproxy_sack_perm: false, + synproxy_timestamp: false, + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '80', '-j', 'SYNPROXY', '--mss', '1460', '-m', 'comment', '--comment', '001 creates rule with synproxy target'], + }, }.freeze From cf319c5ff62edb8a5b1413f8ad678f4f56cf6cf7 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Wed, 10 May 2023 21:36:02 -0700 Subject: [PATCH 06/19] Increment module version --- metadata.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metadata.json b/metadata.json index 5ae9f4838..ed4a7254e 100644 --- a/metadata.json +++ b/metadata.json @@ -1,6 +1,6 @@ { "name": "puppetlabs-firewall", - "version": "5.0.0", + "version": "5.1.0", "author": "puppetlabs", "summary": "Manages Firewalls such as iptables", "license": "Apache-2.0", From c8de286adb7d25254f04d37721a7829489b7ba32 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Fri, 12 May 2023 16:33:26 -0700 Subject: [PATCH 07/19] Make rule parse failures show useful warnings By default the test failure output when a rule is unparseable is just `undefined method '[]' for nil:NilClass`, which does not meaningfully indicate that an error has happened in parsing, or which error. This makes any current or future warning from the parser appear in test failure output. --- spec/unit/puppet/provider/ip6tables_spec.rb | 2 ++ spec/unit/puppet/provider/iptables_spec.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/spec/unit/puppet/provider/ip6tables_spec.rb b/spec/unit/puppet/provider/ip6tables_spec.rb index ad2b987d0..95db41169 100644 --- a/spec/unit/puppet/provider/ip6tables_spec.rb +++ b/spec/unit/puppet/provider/ip6tables_spec.rb @@ -101,6 +101,7 @@ def stub_iptables # If this option is enabled, make sure the parameters exactly match if data[:compare_all] it 'the parameter hash keys should be the same as returned by rules_to_hash' do + expect(provider6).not_to receive(:warning) expect(resource.keys).to match_array(data[:params].keys) end end @@ -108,6 +109,7 @@ def stub_iptables # Iterate across each parameter, creating an example for comparison data[:params].each do |param_name, param_value| it "the parameter '#{param_name}' should match #{param_value.inspect}" do + expect(provider6).not_to receive(:warning) if param_value == true expect(resource[param_name]).to be_truthy else diff --git a/spec/unit/puppet/provider/iptables_spec.rb b/spec/unit/puppet/provider/iptables_spec.rb index ea9e86dbd..2d0e31b44 100644 --- a/spec/unit/puppet/provider/iptables_spec.rb +++ b/spec/unit/puppet/provider/iptables_spec.rb @@ -219,6 +219,7 @@ # If this option is enabled, make sure the parameters exactly match if data[:compare_all] it 'the parameter hash keys should be the same as returned by rules_to_hash' do + expect(provider).not_to receive(:warning) expect(resource.keys).to match_array(data[:params].keys) end end @@ -226,6 +227,7 @@ # Iterate across each parameter, creating an example for comparison data[:params].each do |param_name, param_value| it "the parameter '#{param_name}' should match #{param_value.inspect}" do + expect(provider).not_to receive(:warning) # booleans get cludged to string "true" if param_value == true expect(resource[param_name]).to be_truthy From 303f91782c5c50764f8e064b7e700bb29c53d660 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 18 May 2023 20:26:02 -0700 Subject: [PATCH 08/19] Address deprecation message spam in test output Take a few minutes and address the 40+ lines of deprecation warnings from the test framework, so nobody has to get constantly spammed while trying to do module development anymore. --- spec/unit/puppet/type/firewall_spec.rb | 44 +++++++++++++------------- spec/unit/puppet/util/ipcidr_spec.rb | 6 ++-- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/spec/unit/puppet/type/firewall_spec.rb b/spec/unit/puppet/type/firewall_spec.rb index 88def6401..0447fc81c 100755 --- a/spec/unit/puppet/type/firewall_spec.rb +++ b/spec/unit/puppet/type/firewall_spec.rb @@ -34,7 +34,7 @@ end it 'does not accept a name with non-ASCII chars' do - expect(-> { resource[:name] = '%*#^(#$' }).to raise_error(Puppet::Error) + expect { resource[:name] = '%*#^(#$' }.to raise_error(Puppet::Error) end end @@ -52,7 +52,7 @@ end it 'fails when value is not recognized' do - expect(-> { resource[:action] = 'not valid' }).to raise_error(Puppet::Error) + expect { resource[:action] = 'not valid' }.to raise_error(Puppet::Error) end end @@ -65,7 +65,7 @@ end it 'fails when the chain value is not recognized' do - expect(-> { resource[:chain] = 'not valid' }).to raise_error(Puppet::Error) + expect { resource[:chain] = 'not valid' }.to raise_error(Puppet::Error) end end @@ -78,7 +78,7 @@ end it 'fails when table value is not recognized' do - expect(-> { resource[:table] = 'not valid' }).to raise_error(Puppet::Error) + expect { resource[:table] = 'not valid' }.to raise_error(Puppet::Error) end end @@ -91,7 +91,7 @@ end it 'fails when proto value is not recognized' do - expect(-> { resource[:proto] = 'foo' }).to raise_error(Puppet::Error) + expect { resource[:proto] = 'foo' }.to raise_error(Puppet::Error) end end @@ -110,12 +110,12 @@ ['ACCEPT', 'DROP', 'REJECT'].each do |jump| it "nows fail when value #{jump}" do - expect(-> { resource[:jump] = jump }).to raise_error(Puppet::Error) + expect { resource[:jump] = jump }.to raise_error(Puppet::Error) end end it 'fails when jump value is not recognized' do - expect(-> { resource[:jump] = '%^&*' }).to raise_error(Puppet::Error) + expect { resource[:jump] = '%^&*' }.to raise_error(Puppet::Error) end end @@ -241,7 +241,7 @@ end it "fails when #{addrtype} value is not recognized" do - expect(-> { resource[addrtype] = 'foo' }).to raise_error(Puppet::Error) + expect { resource[addrtype] = 'foo' }.to raise_error(Puppet::Error) end end @@ -295,7 +295,7 @@ expect(resource[:log_level]).to be 3 } - it { expect(-> { resource[:log_level] = 'foo' }).to raise_error(Puppet::Error) } + it { expect { resource[:log_level] = 'foo' }.to raise_error(Puppet::Error) } end end @@ -310,7 +310,7 @@ [-3, 999_999].each do |v| it { - expect(-> { resource[:nflog_group] = v }).to raise_error(Puppet::Error, %r{2\^16\-1}) + expect { resource[:nflog_group] = v }.to raise_error(Puppet::Error, %r{2\^16\-1}) } end end @@ -325,7 +325,7 @@ } it { - expect(-> { resource[:nflog_prefix] = invalid_prefix }).to raise_error(Puppet::Error, %r{64 characters}) + expect { resource[:nflog_prefix] = invalid_prefix }.to raise_error(Puppet::Error, %r{64 characters}) } end end @@ -381,14 +381,14 @@ end it 'fails if icmp type is "any"' do - expect(-> { resource[:icmp] = 'any' }).to raise_error(Puppet::Error) + expect { resource[:icmp] = 'any' }.to raise_error(Puppet::Error) end it 'fails if icmp type is an array' do - expect(-> { resource[:icmp] = "['0', '8']" }).to raise_error(Puppet::Error) + expect { resource[:icmp] = "['0', '8']" }.to raise_error(Puppet::Error) end it 'fails if icmp type cannot be mapped to a numeric' do - expect(-> { resource[:icmp] = 'foo' }).to raise_error(Puppet::Error) + expect { resource[:icmp] = 'foo' }.to raise_error(Puppet::Error) end end @@ -603,10 +603,10 @@ end it 'fails if value is not numeric' do - expect(-> { resource[:burst] = 'foo' }).to raise_error(Puppet::Error) + expect { resource[:burst] = 'foo' }.to raise_error(Puppet::Error) end it 'fails if value contains /sec' do - expect(-> { resource[:burst] = '1500/sec' }).to raise_error(Puppet::Error) + expect { resource[:burst] = '1500/sec' }.to raise_error(Puppet::Error) end end @@ -665,7 +665,7 @@ expect(resource[:set_mark]).to eql '0x3e8' end it 'fails if mask is present' do - expect(-> { resource[:set_mark] = '0x3e8/0xffffffff' }).to raise_error( + expect { resource[:set_mark] = '0x3e8/0xffffffff' }.to raise_error( Puppet::Error, %r{iptables version #{iptables_version} does not support masks on MARK rules$} ) end @@ -693,12 +693,12 @@ expect(resource[:set_mark]).to eql '0x3e8/0x4' end it 'fails if mask value is more than 32 bits' do - expect(-> { resource[:set_mark] = '1/4294967296' }).to raise_error( + expect { resource[:set_mark] = '1/4294967296' }.to raise_error( Puppet::Error, %r{MARK mask must be integer or hex between 0 and 0xffffffff$} ) end it 'fails if mask is malformed' do - expect(-> { resource[:set_mark] = '1000/0xq4' }).to raise_error( + expect { resource[:set_mark] = '1000/0xq4' }.to raise_error( Puppet::Error, %r{MARK mask must be integer or hex between 0 and 0xffffffff$} ) end @@ -706,11 +706,11 @@ ['/', '1000/', 'pwnie'].each do |bad_mark| it "fails with malformed mark '#{bad_mark}'" do - expect(-> { resource[:set_mark] = bad_mark }).to raise_error(Puppet::Error) + expect { resource[:set_mark] = bad_mark }.to raise_error(Puppet::Error) end end it 'fails if mark value is more than 32 bits' do - expect(-> { resource[:set_mark] = '4294967296' }).to raise_error( + expect { resource[:set_mark] = '4294967296' }.to raise_error( Puppet::Error, %r{MARK value must be integer or hex between 0 and 0xffffffff$} ) end @@ -877,7 +877,7 @@ end it 'fails when the pkttype value is not recognized' do - expect(-> { resource[:pkttype] = 'not valid' }).to raise_error(Puppet::Error) + expect { resource[:pkttype] = 'not valid' }.to raise_error(Puppet::Error) end end diff --git a/spec/unit/puppet/util/ipcidr_spec.rb b/spec/unit/puppet/util/ipcidr_spec.rb index b57fa354a..73b28e998 100644 --- a/spec/unit/puppet/util/ipcidr_spec.rb +++ b/spec/unit/puppet/util/ipcidr_spec.rb @@ -40,10 +40,10 @@ let(:ipcidr) { Puppet::Util::IPCidr.new('96.126.112.20/24') } - specify { host.cidr.should == '96.126.112.0/24' } # .20 is expected to + it { expect(host.cidr).to eql '96.126.112.0/24' } # .20 is expected to # be silently dropped. - specify { host.prefixlen.should == 24 } - specify { host.netmask.should == '255.255.255.0' } + it { expect(host.prefixlen).to be 24 } + it { expect(host.netmask).to eql '255.255.255.0' } end describe 'ipv4 open range with cidr' do From 0d5a78fbdfa2dd168a9ac55abe5ffa3fb8cc9e9d Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 25 May 2023 18:29:44 -0700 Subject: [PATCH 09/19] Add multi-version support to provider unit tests Add support for testing against multiple kernel and iptables versions, to ensure that features gated by kernel or iptables version aren't rendered un-testable. Also, pull the handy construct for handling the addition of '--wait' in later iptables versions in the ip6tables test cases over to iptables tests as well, because with multi-version support, the test cases for iptables will start to need it as well. --- spec/unit/puppet/provider/ip6tables_spec.rb | 8 ++++++++ spec/unit/puppet/provider/iptables_spec.rb | 9 +++++++++ 2 files changed, 17 insertions(+) diff --git a/spec/unit/puppet/provider/ip6tables_spec.rb b/spec/unit/puppet/provider/ip6tables_spec.rb index 95db41169..f1f8b55e7 100644 --- a/spec/unit/puppet/provider/ip6tables_spec.rb +++ b/spec/unit/puppet/provider/ip6tables_spec.rb @@ -124,6 +124,14 @@ def stub_iptables describe 'when working out general_args' do HASH_TO_ARGS6.each do |test_name, data| describe "for test data '#{test_name}'" do + before :each do + # Allow examples to stub kernel and iptables versions + allow(Facter.fact(:ip6tables_version)).to receive(:value).and_return(data[:ip6tables_version]) if data[:ip6tables_version] + allow(Facter.fact(:kernelversion)).to receive(:value).and_return(data[:kernel_version]) if data[:kernel_version] + # Unload existing provider so provider features get re-assessed after we stub the determining facts + Puppet::Type.type(:firewall).unprovide(:ip6tables) + end + let(:resource) { Puppet::Type.type(:firewall).new(data[:params]) } let(:provider6) { Puppet::Type.type(:firewall).provider(:ip6tables) } let(:instance) { provider6.new(resource) } diff --git a/spec/unit/puppet/provider/iptables_spec.rb b/spec/unit/puppet/provider/iptables_spec.rb index 2d0e31b44..77df78746 100644 --- a/spec/unit/puppet/provider/iptables_spec.rb +++ b/spec/unit/puppet/provider/iptables_spec.rb @@ -243,11 +243,20 @@ describe 'when working out general_args' do HASH_TO_ARGS.each do |test_name, data| describe "for test data '#{test_name}'" do + before :each do + # Allow examples to stub kernel and iptables versions + allow(Facter.fact(:iptables_version)).to receive(:value).and_return(data[:iptables_version]) if data[:iptables_version] + allow(Facter.fact(:kernelversion)).to receive(:value).and_return(data[:kernel_version]) if data[:kernel_version] + # Unload existing provider so provider features get re-assessed after we stub the determining facts + Puppet::Type.type(:firewall).unprovide(:iptables) + end + let(:resource) { Puppet::Type.type(:firewall).new(data[:params]) } let(:provider) { Puppet::Type.type(:firewall).provider(:iptables) } let(:instance) { provider.new(resource) } it 'general_args should be valid' do + data[:args].unshift('--wait') if instance.general_args.flatten.include? '--wait' expect(instance.general_args.flatten).to eq(data[:args]) end end From a68038a0c554a43e8d7c3b90998b06084bd6d8a3 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 25 May 2023 18:38:07 -0700 Subject: [PATCH 10/19] Add cases exercised in acceptance to unit testing Add the test cases not present in unit testing, but used in acceptance testing, to the unit tests as well. Not everyone who works on the module has the ability to run the acceptance tests, but acceptance test failures should not come as a surprise to developers being diligent about passing all unit tests. --- .../firewall_attributes_exceptions_spec.rb | 2 +- spec/fixtures/ip6tables/conversion_hash.rb | 628 ++++++++++++++++++ spec/fixtures/iptables/conversion_hash.rb | 577 +++++++++++++++- 3 files changed, 1205 insertions(+), 2 deletions(-) diff --git a/spec/acceptance/firewall_attributes_exceptions_spec.rb b/spec/acceptance/firewall_attributes_exceptions_spec.rb index 4033acb48..71f48e42f 100644 --- a/spec/acceptance/firewall_attributes_exceptions_spec.rb +++ b/spec/acceptance/firewall_attributes_exceptions_spec.rb @@ -66,7 +66,7 @@ class { '::firewall': } it 'contains the rule' do run_shell('iptables-save') do |r| - expect(r.stdout).not_to match(%r{-A INPUT -p tcp -m multiport --dports 9999561-562 -m comment --comment "560 - test" -j ACCEPT}) + expect(r.stdout).not_to match(%r{-A INPUT -p tcp -m multiport --dports 9999561-562 -m comment --comment "561 - test" -j ACCEPT}) end end end diff --git a/spec/fixtures/ip6tables/conversion_hash.rb b/spec/fixtures/ip6tables/conversion_hash.rb index f0b045365..17369e4df 100644 --- a/spec/fixtures/ip6tables/conversion_hash.rb +++ b/spec/fixtures/ip6tables/conversion_hash.rb @@ -24,6 +24,31 @@ destination: '2001:db8:4321::/48', }, }, + 'frag_ishasmorefrags' => { + line: '-A INPUT -p tcp -m frag --fragid 0 --fragmore -m comment --comment "100 has more fragments"', + params: { + name: '100 has more fragments', + chain: 'INPUT', + proto: 'tcp', + ishasmorefrags: true, + }, + }, + 'frag_islastfrag' => { + line: '-A INPUT -p tcp -m frag --fragid 0 --fraglast -m comment --comment "100 last fragment"', + params: { + name: '100 last fragment', + chain: 'INPUT', + islastfrag: true, + }, + }, + 'frag_isfirstfrags' => { + line: '-A INPUT -p tcp -m frag --fragid 0 --fragfirst -m comment --comment "100 first fragment"', + params: { + name: '100 first fragment', + chain: 'INPUT', + isfirstfrag: true, + }, + }, 'udp_source_port_and_destination_port' => { line: '-A ufw6-before-input -s fe80::/10 -d fe80::/10 -p udp -m udp --sport 547 --dport 546 -j ACCEPT', table: 'filter', @@ -50,6 +75,42 @@ random_fully: 'true', } }, + 'match_mss' => { + line: '-A FORWARD -p tcp -m tcp --tcp-flags SYN,RST SYN -m tcpmss --mss 1361:1541 -m comment --comment "604 - set_mss" -j TCPMSS --set-mss 1360', + compare_all: true, + table: 'filter', + params: { + line: '-A FORWARD -p tcp -m tcp --tcp-flags SYN,RST SYN -m tcpmss --mss 1361:1541 -m comment --comment "604 - set_mss" -j TCPMSS --set-mss 1360', + name: '604 - set_mss', + provider: 'ip6tables', + ensure: :present, + table: 'filter', + chain: 'FORWARD', + proto: 'tcp', + tcp_flags: 'SYN,RST SYN', + mss: '1361:1541', + jump: 'TCPMSS', + set_mss: '1360', + }, + }, + 'match_mss_with_synproxy' => { + line: '-A FORWARD -p tcp -m tcp --tcp-flags SYN,RST SYN -m tcpmss --mss 1361:1541 -m comment --comment "604 - proxy SYN in range" -j SYNPROXY --mss 1360', + compare_all: true, + table: 'filter', + params: { + line: '-A FORWARD -p tcp -m tcp --tcp-flags SYN,RST SYN -m tcpmss --mss 1361:1541 -m comment --comment "604 - proxy SYN in range" -j SYNPROXY --mss 1360', + name: '604 - proxy SYN in range', + provider: 'ip6tables', + ensure: :present, + table: 'filter', + chain: 'FORWARD', + proto: 'tcp', + tcp_flags: 'SYN,RST SYN', + mss: '1361:1541', + jump: 'SYNPROXY', + synproxy_mss: '1360', + }, + }, 'parses_synproxy_rule' => { line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --sack-perm --timestamp --wscale 9 --mss 1460 --ecn', compare_all: true, @@ -89,6 +150,256 @@ table: 'filter', }, }, + 'nflog_options_using_range' => { + line: '-A INPUT -p all -m comment --comment "503 - test" -j NFLOG --nflog-group 3 --nflog-prefix "TEST PREFIX" --nflog-range 16 --nflog-threshold 2', + params: { + name: '503 - test', + proto: 'all', + jump: 'NFLOG', + nflog_group: '3', + nflog_prefix: 'TEST PREFIX', + nflog_range: '16', + nflog_threshold: '2', + }, + }, + 'nflog_options_using_size' => { + line: '-A INPUT -p all -m comment --comment "503 - test" -j NFLOG --nflog-group 3 --nflog-prefix "TEST PREFIX" --nflog-size 16 --nflog-threshold 2', + params: { + name: '503 - test', + proto: 'all', + jump: 'NFLOG', + nflog_group: '3', + nflog_prefix: 'TEST PREFIX', + nflog_size: '16', + nflog_threshold: '2', + }, + }, + # NETMAP's '--to' isn't implemented for the ip6tables provider + # 'netmap_to' => { + # line: '-A POSTROUTING -d 200.200.200.200/32 -p tcp -m comment --comment "569 - test" -j NETMAP --to 192.168.1.1', + # params: { + # name: '569 - test', + # chain: 'POSTROUTING', + # proto: 'tcp', + # destination: '200.200.200.200/32', + # jump: 'NETMAP', + # to: '192.168.1.1', + # }, + # }, + 'snat_to_source' => { + line: '-A POSTROUTING -p tcp -m comment --comment "568 - tosource" -j SNAT --to-source 3b71:db9:4321::3', + params: { + name: '568 - tosource', + chain: 'POSTROUTING', + proto: 'tcp', + jump: 'SNAT', + tosource: '3b71:db9:4321::3', + }, + }, + 'dnat_to_dest' => { + line: '-A PREROUTING -s 3b71:db9:1234::3 -p tcp -m comment --comment "569 - todest" -j DNAT --to-destination 2001:db8:4321::1', + params: { + name: '569 - todest', + chain: 'PREROUTING', + proto: 'tcp', + jump: 'DNAT', + source: '3b71:db9:1234::3/128', + todest: '2001:db8:4321::1', + }, + }, + 'redirect_to_ports' => { + line: '-A PREROUTING -p icmp -m comment --comment "574 - toports" -j REDIRECT --to-ports 2222', + params: { + name: '574 - toports', + chain: 'PREROUTING', + proto: 'icmp', + jump: 'REDIRECT', + toports: '2222', + }, + }, + 'tee_gateway' => { + line: '-A PREROUTING -m comment --comment "810 - tee_gateway" -j TEE --gateway 2001:db8:4321::1', + params: { + name: '810 - tee_gateway', + chain: 'PREROUTING', + proto: 'all', + jump: 'TEE', + gateway: '2001:db8:4321::1', + }, + }, + 'match_time' => { + line: '-A OUTPUT -p tcp -m multiport --dports 8080 -m time --timestart 06:00:00 --timestop 17:00:00 --monthdays 7 --weekdays Tue --datestart 2016-01-19T04:17:07 --datestop 2038-01-19T04:17:07 --kerneltz -m comment --comment "805 - test" -j ACCEPT', + params: { + name: '805 - test', + chain: 'OUTPUT', + proto: 'tcp', + dport: ['8080'], + date_start: '2016-01-19T04:17:07', + date_stop: '2038-01-19T04:17:07', + time_start: '06:00:00', + time_stop: '17:00:00', + month_days: '7', + week_days: 'Tue', + kernel_timezone: true, + action: 'accept', + }, + }, + 'checksum_fill' => { + line: '-A POSTROUTING -o virbr0 -p udp -m multiport --dports 68 -m comment --comment "576 - test" -j CHECKSUM --checksum-fill', + params: { + name: '576 - test', + chain: 'POSTROUTING', + proto: 'udp', + outiface: 'virbr0', + dport: ['68'], + jump: 'CHECKSUM', + checksum_fill: true, + }, + }, + 'hashlimit_above' => { + line: '-A INPUT -p tcp -m hashlimit --hashlimit-above 16/sec --hashlimit-mode srcip,dstip --hashlimit-name above --hashlimit-htable-gcinterval 10 -m comment --comment "805 - hashlimit_above test" -j ACCEPT', + params: { + name: '805 - hashlimit_above test', + chain: 'INPUT', + proto: 'tcp', + hashlimit_name: 'above', + hashlimit_above: '16/sec', + hashlimit_htable_gcinterval: '10', + hashlimit_mode: 'srcip,dstip', + action: 'accept', + }, + }, + 'hashlimit_upto' => { + line: '-A INPUT -p tcp -m hashlimit --hashlimit-upto 16/sec --hashlimit-burst 640 --hashlimit-name upto --hashlimit-htable-size 1000000 --hashlimit-htable-max 320000 --hashlimit-htable-expire 36000000 -m comment --comment "806 - hashlimit_upto test" -j ACCEPT', + params: { + name: '806 - hashlimit_upto test', + chain: 'INPUT', + proto: 'tcp', + hashlimit_name: 'upto', + hashlimit_upto: '16/sec', + hashlimit_burst: '640', + hashlimit_htable_size: '1000000', + hashlimit_htable_max: '320000', + hashlimit_htable_expire: '36000000', + action: 'accept', + }, + }, + 'condition' => { + line: '-A INPUT -i enp0s8 -p icmp -m condition ! --condition "isblue" -m comment --comment "010 isblue ipv4" -j DROP', + params: { + name: '010 isblue ipv4', + chain: 'INPUT', + proto: 'icmp', + condition: '! isblue', + iniface: 'enp0s8', + action: 'drop', + }, + }, + 'ipsec_out_policy_ipsec' => { + line: '-A OUTPUT -d 2001:db8:4321::/64 -m policy --dir out --pol ipsec -m comment --comment "595 - ipsec_policy ipsec and out" -j REJECT --reject-with icmp-net-unreachable', + params: { + name: '595 - ipsec_policy ipsec and out', + chain: 'OUTPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + ipsec_dir: 'out', + ipsec_policy: 'ipsec', + action: 'reject', + reject: 'icmp-net-unreachable', + }, + }, + 'ipsec_in_policy_none' => { + line: '-A INPUT -d 2001:db8:4321::/64 -m policy --dir in --pol none -m comment --comment "596 - ipsec_policy none and in" -j REJECT --reject-with icmp-net-unreachable', + params: { + name: '596 - ipsec_policy none and in', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + ipsec_dir: 'in', + ipsec_policy: 'none', + action: 'reject', + reject: 'icmp-net-unreachable', + }, + }, + 'recent_set_rdest' => { + line: '-A INPUT -d 2001:db8:4321::/64 -m recent --set --name list1 --mask 255.255.255.255 --rdest -m comment --comment "597 - recent set"', + params: { + name: '597 - recent set', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + recent: 'set', + rdest: true, + rname: 'list1', + }, + }, + 'recent_rcheck_complex' => { + line: '-A INPUT -d 2001:db8:4321::/64 -m recent --rcheck --seconds 60 --hitcount 5 --rttl --name list1 --mask 255.255.255.255 --rsource -m comment --comment "598 - recent rcheck"', + params: { + name: '598 - recent rcheck', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + recent: 'rcheck', + rsource: true, + rname: 'list1', + rseconds: '60', + rhitcount: '5', + rttl: true, + }, + }, + 'recent_update_simple' => { + line: '-A INPUT -d 2001:db8:4321::/64 -m recent --update --name DEFAULT --mask 255.255.255.255 --rsource -m comment --comment "599 - recent update"', + params: { + name: '599 - recent update', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + recent: 'update', + rname: 'DEFAULT', + }, + }, + 'recent_remove' => { + line: '-A INPUT -d 2001:db8:4321::/64 -m recent --remove --name DEFAULT --mask 255.255.255.255 --rsource -m comment --comment "600 - recent remove"', + params: { + name: '600 - recent remove', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + recent: 'remove', + rname: 'DEFAULT', + }, + }, + 'log_params' => { + line: '-A OUTPUT -p tcp -m comment --comment "701 - log_uid, tcp-sequences and options" -j LOG --log-tcp-sequence --log-tcp-options --log-ip-options --log-uid', + params: { + name: '701 - log_uid, tcp-sequences and options', + chain: 'OUTPUT', + jump: 'LOG', + log_uid: true, + log_tcp_sequence: true, + log_tcp_options: true, + log_ip_options: true, + }, + }, + 'rpfilter_string' => { + line: '-A PREROUTING -p tcp -m rpfilter --invert -m comment --comment "901 - set rpfilter" -j ACCEPT', + params: { + name: '901 - set rpfilter', + chain: 'PREROUTING', + action: 'accept', + rpfilter: ['invert'], + }, + }, + 'rpfilter_array' => { + line: '-A PREROUTING -p tcp -m rpfilter --loose --validmark --accept-local --invert -m comment --comment "900 - set rpfilter" -j ACCEPT', + params: { + name: '900 - set rpfilter', + chain: 'PREROUTING', + action: 'accept', + rpfilter: ['loose', 'validmark', 'accept-local', 'invert'], + }, + }, }.freeze # This hash is for testing converting a hash to an argument line. @@ -180,9 +491,38 @@ }, args: ['-t', :filter, '-p', :tcp, '-j', 'NFLOG', '--nflog-group', 1, '--nflog-prefix', 'myprefix', '-m', 'comment', '--comment', '100 nflog'], }, + 'match_mss' => { + params: { + name: '604 - set_mss', + provider: 'ip6tables', + table: 'filter', + chain: 'FORWARD', + proto: 'tcp', + tcp_flags: 'SYN,RST SYN', + mss: '1361:1541', + jump: 'TCPMSS', + set_mss: '1360', + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'tcp', '--tcp-flags', 'SYN,RST', 'SYN', '-j', 'TCPMSS', '--set-mss', '1360', '-m', 'tcpmss', '--mss', '1361:1541', '-m', 'comment', '--comment', '604 - set_mss'], + }, + 'match_mss_with_synproxy' => { + params: { + name: '604 - proxy SYN in range', + provider: 'ip6tables', + table: 'filter', + chain: 'FORWARD', + proto: 'tcp', + tcp_flags: 'SYN,RST SYN', + mss: '1361:1541', + jump: 'SYNPROXY', + synproxy_mss: '1360', + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'tcp', '--tcp-flags', 'SYN,RST', 'SYN', '-j', 'SYNPROXY', '--mss', '1360', '-m', 'tcpmss', '--mss', '1361:1541', '-m', 'comment', '--comment', '604 - proxy SYN in range'] + }, 'creates_synproxy_rule' => { params: { name: '001 creates rule with synproxy target', + provider: 'ip6tables', chain: 'INPUT', ensure: :present, dport: ['80'], @@ -198,6 +538,7 @@ 'creates_synproxy_rule_with_stringified_integers' => { params: { name: '001 creates rule with synproxy target', + provider: 'ip6tables', chain: 'INPUT', ensure: :present, dport: ['80'], @@ -213,6 +554,7 @@ 'creates_synproxy_rule_with_booleans_set_false' => { params: { name: '001 creates rule with synproxy target', + provider: 'ip6tables', chain: 'INPUT', ensure: :present, dport: ['80'], @@ -224,4 +566,290 @@ }, args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '80', '-j', 'SYNPROXY', '--mss', '1460', '-m', 'comment', '--comment', '001 creates rule with synproxy target'], }, + 'nflog_options_using_range' => { + params: { + name: '503 - test', + provider: 'ip6tables', + proto: 'all', + jump: 'NFLOG', + nflog_group: 3, + nflog_prefix: 'TEST PREFIX', + nflog_range: '16', + nflog_threshold: 2, + }, + args: ['-t', :filter, '-p', :all, '-j', 'NFLOG', '--nflog-group', 3, '--nflog-prefix', 'TEST PREFIX', '--nflog-range', '16', '--nflog-threshold', 2, '-m', 'comment', '--comment', '503 - test'], + }, + 'nflog_options_using_size' => { + ip6tables_version: '1.6.1', + params: { + name: '503 - test', + provider: 'ip6tables', + proto: 'all', + jump: 'NFLOG', + nflog_group: 3, + nflog_prefix: 'TEST PREFIX', + nflog_size: '16', + nflog_threshold: 2, + }, + args: ['-t', :filter, '-p', :all, '-j', 'NFLOG', '--nflog-group', 3, '--nflog-prefix', 'TEST PREFIX', '--nflog-size', '16', '--nflog-threshold', 2, '-m', 'comment', '--comment', '503 - test'], + }, + # NETMAP's '--to' isn't implemented for the ip6tables provider + # 'netmap_to' => { + # params: { + # name: '569 - test', + # provider: 'ip6tables', + # table: 'nat', + # chain: 'POSTROUTING', + # destination: '3b71:db9:4321::/64', + # jump: 'NETMAP', + # to: '2001:db8:4321::1', + # }, + # args: ['-t', :nat, '-d', '3b71:db9:4321::/64', '-p', :tcp, '-j', 'NETMAP', '--to', '2001:db8:4321::1', '-m', 'comment', '--comment', '569 - test'], + # }, + 'snat_to_source' => { + params: { + name: '568 - tosource', + provider: 'ip6tables', + table: 'nat', + chain: 'POSTROUTING', + proto: 'tcp', + jump: 'SNAT', + tosource: '3b71:db9:4321::3', + }, + args: ['-t', :nat, '-p', :tcp, '-j', 'SNAT', '--to-source', '3b71:db9:4321::3', '-m', 'comment', '--comment', '568 - tosource'], + }, + 'dnat_to_dest' => { + params: { + name: '569 - todest', + provider: 'ip6tables', + table: 'nat', + chain: 'PREROUTING', + proto: 'tcp', + jump: 'DNAT', + source: '3b71:db9:1234::3', + todest: '2001:db8:4321::1', + }, + args: ['-t', :nat, '-s', '3b71:db9:1234::3/128', '-p', :tcp, '-j', 'DNAT', '--to-destination', '2001:db8:4321::1', '-m', 'comment', '--comment', '569 - todest'], + }, + 'redirect_to_ports' => { + params: { + name: '574 - toports', + provider: 'ip6tables', + table: 'nat', + chain: 'PREROUTING', + proto: 'icmp', + jump: 'REDIRECT', + toports: '2222', + }, + args: ['-t', :nat, '-p', :icmp, '-j', 'REDIRECT', '--to-ports', '2222', '-m', 'comment', '--comment', '574 - toports'], + }, + 'tee_gateway' => { + params: { + name: '810 - tee_gateway', + provider: 'ip6tables', + chain: 'PREROUTING', + table: 'mangle', + jump: 'TEE', + gateway: '2001:db8:4321::1', + proto: 'all', + }, + args: ['-t', :mangle, '-p', :all, '-j', 'TEE', '--gateway', '2001:db8:4321::1', '-m', 'comment', '--comment', '810 - tee_gateway'], + }, + 'match_time' => { + params: { + name: '805 - test', + provider: 'ip6tables', + chain: 'OUTPUT', + proto: 'tcp', + dport: '8080', + date_start: '2016-01-19T04:17:07', + date_stop: '2038-01-19T04:17:07', + time_start: '6:00', + time_stop: '17:00:00', + month_days: '7', + week_days: 'Tue', + kernel_timezone: true, + action: 'accept', + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '8080', '-j', 'ACCEPT', '-m', 'time', '--timestart', '06:00:00', '--timestop', '17:00:00', '--monthdays', '7', '--weekdays', :Tue, '--datestart', '2016-01-19T04:17:07', '--datestop', '2038-01-19T04:17:07', '--kerneltz', '-m', 'comment', '--comment', '805 - test'], + }, + 'checksum_fill' => { + params: { + name: '576 - test', + provider: 'ip6tables', + table: 'mangle', + chain: 'POSTROUTING', + proto: 'udp', + outiface: 'virbr0', + dport: '68', + jump: 'CHECKSUM', + checksum_fill: true, + }, + args: ['-t', :mangle, '-o', 'virbr0', '-p', :udp, '-m', 'multiport', '--dports', '68', '-j', 'CHECKSUM', '--checksum-fill', '-m', 'comment', '--comment', '576 - test'], + }, + 'hashlimit_above' => { + params: { + name: '805 - hashlimit_above test', + provider: 'ip6tables', + proto: 'tcp', + hashlimit_name: 'above', + hashlimit_above: '16/sec', + hashlimit_htable_gcinterval: '10', + hashlimit_mode: 'srcip,dstip', + action: 'accept', + }, + args: ['-t', :filter, '-p', :tcp, '-j', 'ACCEPT', '-m', 'hashlimit', '--hashlimit-above', '16/sec', '--hashlimit-name', 'above', '--hashlimit-mode', 'srcip,dstip', '--hashlimit-htable-gcinterval', '10', '-m', 'comment', '--comment', '805 - hashlimit_above test'], + }, + 'hashlimit_upto' => { + params: { + name: '806 - hashlimit_upto test', + provider: 'ip6tables', + hashlimit_name: 'upto', + hashlimit_upto: '16/sec', + hashlimit_burst: '640', + hashlimit_htable_size: '1000000', + hashlimit_htable_max: '320000', + hashlimit_htable_expire: '36000000', + action: 'accept', + }, + args: ['-t', :filter, '-p', :tcp, '-j', 'ACCEPT', '-m', 'hashlimit', '--hashlimit-upto', '16/sec', '--hashlimit-name', 'upto', '--hashlimit-burst', '640', '--hashlimit-htable-size', '1000000', '--hashlimit-htable-max', '320000', '--hashlimit-htable-expire', '36000000', '-m', 'comment', '--comment', '806 - hashlimit_upto test'], + }, + 'condition' => { + params: { + name: '010 isblue ipv4', + provider: 'ip6tables', + chain: 'INPUT', + proto: 'icmp', + condition: '! isblue', + iniface: 'enp0s8', + action: 'drop', + }, + args: ['-t', :filter, '-i', 'enp0s8', '-p', :icmp, '-j', 'DROP', '-m', 'condition', '!', '--condition', 'isblue', '-m', 'comment', '--comment', '010 isblue ipv4'], + }, + 'ipsec_out_policy_ipsec' => { + params: { + name: '595 - ipsec_policy ipsec and out', + provider: 'ip6tables', + ensure: 'present', + action: 'reject', + chain: 'OUTPUT', + destination: '2001:db8:4321::/64', + ipsec_dir: 'out', + ipsec_policy: 'ipsec', + proto: 'all', + reject: 'icmp-net-unreachable', + table: 'filter', + }, + args: ['-t', :filter, '-d', '2001:db8:4321::/64', '-p', :all, '-m', 'policy', '--dir', :out, '--pol', :ipsec, '-j', 'REJECT', '--reject-with', 'icmp-net-unreachable', '-m', 'comment', '--comment', '595 - ipsec_policy ipsec and out'], + }, + 'ipsec_in_policy_none' => { + params: { + name: '596 - ipsec_policy none and in', + provider: 'ip6tables', + ensure: 'present', + action: 'reject', + chain: 'INPUT', + destination: '2001:db8:4321::/64', + ipsec_dir: 'in', + ipsec_policy: 'none', + proto: 'all', + reject: 'icmp-net-unreachable', + table: 'filter', + }, + args: ['-t', :filter, '-d', '2001:db8:4321::/64', '-p', :all, '-m', 'policy', '--dir', :in, '--pol', :none, '-j', 'REJECT', '--reject-with', 'icmp-net-unreachable', '-m', 'comment', '--comment', '596 - ipsec_policy none and in'], + }, + 'recent_set_rdest' => { + params: { + name: '597 - recent set', + provider: 'ip6tables', + table: 'filter', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + recent: 'set', + rdest: true, + rname: 'list1', + }, + args: ['-t', :filter, '-d', '2001:db8:4321::/64', '-p', :all, '-m', 'recent', '--set', '--name', 'list1', '--rdest', '-m', 'comment', '--comment', '597 - recent set'], + }, + 'recent_rcheck_complex' => { + params: { + name: '598 - recent rcheck', + provider: 'ip6tables', + table: 'filter', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + recent: 'rcheck', + rsource: true, + rname: 'list1', + rseconds: 60, + rhitcount: 5, + rttl: true, + }, + args: ['-t', :filter, '-d', '2001:db8:4321::/64', '-p', :all, '-m', 'recent', '--rcheck', '--seconds', 60, '--hitcount', 5, '--rttl', '--name', 'list1', '--rsource', '-m', 'comment', '--comment', '598 - recent rcheck'], + }, + 'recent_update_simple' => { + params: { + name: '599 - recent update', + provider: 'ip6tables', + table: 'filter', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + recent: 'update', + }, + args: ['-t', :filter, '-d', '2001:db8:4321::/64', '-p', :all, '-m', 'recent', '--update', '-m', 'comment', '--comment', '599 - recent update'], + }, + 'recent_remove' => { + params: { + name: '600 - recent remove', + provider: 'ip6tables', + table: 'filter', + chain: 'INPUT', + proto: 'all', + destination: '2001:db8:4321::/64', + recent: 'remove', + }, + args: ['-t', :filter, '-d', '2001:db8:4321::/64', '-p', :all, '-m', 'recent', '--remove', '-m', 'comment', '--comment', '600 - recent remove'], + }, + 'log_params' => { + params: { + name: '701 - log_uid, tcp-sequences and options', + provider: 'ip6tables', + chain: 'OUTPUT', + jump: 'LOG', + log_uid: true, + log_tcp_sequence: true, + log_tcp_options: true, + log_ip_options: true, + }, + args: ['-t', :filter, '-p', :tcp, '-j', 'LOG', '--log-uid', '--log-tcp-sequence', '--log-tcp-options', '--log-ip-options', '-m', 'comment', '--comment', '701 - log_uid, tcp-sequences and options'], + }, + 'rpfilter_string' => { + kernel_version: '3.10.0', + ip6tables_version: '1.5', + params: { + name: '901 - set rpfilter', + provider: 'ip6tables', + table: 'raw', + chain: 'PREROUTING', + action: 'accept', + rpfilter: 'invert', + }, + args: ['-t', :raw, '-p', :tcp, '-j', 'ACCEPT', '-m', 'rpfilter', '--invert', '-m', 'comment', '--comment', '901 - set rpfilter'], + }, + 'rpfilter_array' => { + kernel_version: '3.10.0', + ip6tables_version: '1.5', + params: { + name: '900 - set rpfilter', + provider: 'ip6tables', + table: 'raw', + chain: 'PREROUTING', + action: 'accept', + rpfilter: [ 'loose', 'validmark', 'accept-local', 'invert' ], + }, + args: ['-t', :raw, '-p', :tcp, '-j', 'ACCEPT', '-m', 'rpfilter', '--loose', '--validmark', '--accept-local', '--invert', '-m', 'comment', '--comment', '900 - set rpfilter'], + }, }.freeze diff --git a/spec/fixtures/iptables/conversion_hash.rb b/spec/fixtures/iptables/conversion_hash.rb index 407551887..11a168a08 100644 --- a/spec/fixtures/iptables/conversion_hash.rb +++ b/spec/fixtures/iptables/conversion_hash.rb @@ -669,6 +669,42 @@ action: 'reject', }, }, + 'match_mss' => { + line: '-A FORWARD -p tcp -m tcp --tcp-flags SYN,RST SYN -m tcpmss --mss 1361:1541 -m comment --comment "604 - set_mss" -j TCPMSS --set-mss 1360', + compare_all: true, + table: 'filter', + params: { + line: '-A FORWARD -p tcp -m tcp --tcp-flags SYN,RST SYN -m tcpmss --mss 1361:1541 -m comment --comment "604 - set_mss" -j TCPMSS --set-mss 1360', + name: '604 - set_mss', + provider: 'iptables', + ensure: :present, + table: 'filter', + chain: 'FORWARD', + proto: 'tcp', + tcp_flags: 'SYN,RST SYN', + mss: '1361:1541', + jump: 'TCPMSS', + set_mss: '1360', + }, + }, + 'match_mss_with_synproxy' => { + line: '-A FORWARD -p tcp -m tcp --tcp-flags SYN,RST SYN -m tcpmss --mss 1361:1541 -m comment --comment "604 - proxy SYN in range" -j SYNPROXY --mss 1360', + compare_all: true, + table: 'filter', + params: { + line: '-A FORWARD -p tcp -m tcp --tcp-flags SYN,RST SYN -m tcpmss --mss 1361:1541 -m comment --comment "604 - proxy SYN in range" -j SYNPROXY --mss 1360', + name: '604 - proxy SYN in range', + provider: 'iptables', + ensure: :present, + table: 'filter', + chain: 'FORWARD', + proto: 'tcp', + tcp_flags: 'SYN,RST SYN', + mss: '1361:1541', + jump: 'SYNPROXY', + synproxy_mss: '1360', + }, + }, 'clamp_mss_to_pmtu' => { line: '-A INPUT -p tcp -m tcp --tcp-flags SYN,RST SYN -j TCPMSS --clamp-mss-to-pmtu -m comment --comment "067 change max segment size"', table: 'filter', @@ -792,7 +828,7 @@ dport: ['53'], jump: 'CT', notrack: true - } + }, }, 'parses_synproxy_rule' => { line: '-A INPUT -p tcp -m tcp --dport 80 -m comment --comment "001 parses rule with synproxy target" -j SYNPROXY --sack-perm --timestamp --wscale 9 --mss 1460 --ecn', @@ -833,6 +869,255 @@ table: 'filter', }, }, + 'nflog_options_using_range' => { + line: '-A INPUT -p all -m comment --comment "503 - test" -j NFLOG --nflog-group 3 --nflog-prefix "TEST PREFIX" --nflog-range 16 --nflog-threshold 2', + params: { + name: '503 - test', + proto: 'all', + jump: 'NFLOG', + nflog_group: '3', + nflog_prefix: 'TEST PREFIX', + nflog_range: '16', + nflog_threshold: '2', + }, + }, + 'nflog_options_using_size' => { + line: '-A INPUT -p all -m comment --comment "503 - test" -j NFLOG --nflog-group 3 --nflog-prefix "TEST PREFIX" --nflog-size 16 --nflog-threshold 2', + params: { + name: '503 - test', + proto: 'all', + jump: 'NFLOG', + nflog_group: '3', + nflog_prefix: 'TEST PREFIX', + nflog_size: '16', + nflog_threshold: '2', + }, + }, + 'netmap_to' => { + line: '-A POSTROUTING -d 200.200.200.200/32 -p tcp -m comment --comment "569 - test" -j NETMAP --to 192.168.1.1', + params: { + name: '569 - test', + chain: 'POSTROUTING', + proto: 'tcp', + destination: '200.200.200.200/32', + jump: 'NETMAP', + to: '192.168.1.1', + }, + }, + 'snat_to_source' => { + line: '-A POSTROUTING -p tcp -m comment --comment "568 - tosource" -j SNAT --to-source 192.168.1.1', + params: { + name: '568 - tosource', + chain: 'POSTROUTING', + proto: 'tcp', + jump: 'SNAT', + tosource: '192.168.1.1', + }, + }, + 'dnat_to_dest' => { + line: '-A PREROUTING -s 200.200.200.200 -p tcp -m comment --comment "569 - todest" -j DNAT --to-destination 192.168.1.1', + params: { + name: '569 - todest', + chain: 'PREROUTING', + proto: 'tcp', + jump: 'DNAT', + source: '200.200.200.200/32', + todest: '192.168.1.1', + }, + }, + 'redirect_to_ports' => { + line: '-A PREROUTING -p icmp -m comment --comment "574 - toports" -j REDIRECT --to-ports 2222', + params: { + name: '574 - toports', + chain: 'PREROUTING', + proto: 'icmp', + jump: 'REDIRECT', + toports: '2222', + }, + }, + 'tee_gateway' => { + line: '-A PREROUTING -m comment --comment "810 - tee_gateway" -j TEE --gateway 10.0.0.2', + params: { + name: '810 - tee_gateway', + chain: 'PREROUTING', + proto: 'all', + jump: 'TEE', + gateway: '10.0.0.2', + }, + }, + 'match_time' => { + line: '-A OUTPUT -p tcp -m multiport --dports 8080 -m time --timestart 06:00:00 --timestop 17:00:00 --monthdays 7 --weekdays Tue --datestart 2016-01-19T04:17:07 --datestop 2038-01-19T04:17:07 --kerneltz -m comment --comment "805 - test" -j ACCEPT', + params: { + name: '805 - test', + chain: 'OUTPUT', + proto: 'tcp', + dport: ['8080'], + date_start: '2016-01-19T04:17:07', + date_stop: '2038-01-19T04:17:07', + time_start: '06:00:00', + time_stop: '17:00:00', + month_days: '7', + week_days: 'Tue', + kernel_timezone: true, + action: 'accept', + }, + }, + 'checksum_fill' => { + line: '-A POSTROUTING -o virbr0 -p udp -m multiport --dports 68 -m comment --comment "576 - test" -j CHECKSUM --checksum-fill', + params: { + name: '576 - test', + chain: 'POSTROUTING', + proto: 'udp', + outiface: 'virbr0', + dport: ['68'], + jump: 'CHECKSUM', + checksum_fill: true, + }, + }, + 'hashlimit_above' => { + line: '-A INPUT -p tcp -m hashlimit --hashlimit-above 16/sec --hashlimit-mode srcip,dstip --hashlimit-name above --hashlimit-htable-gcinterval 10 -m comment --comment "805 - hashlimit_above test" -j ACCEPT', + params: { + name: '805 - hashlimit_above test', + chain: 'INPUT', + proto: 'tcp', + hashlimit_name: 'above', + hashlimit_above: '16/sec', + hashlimit_htable_gcinterval: '10', + hashlimit_mode: 'srcip,dstip', + action: 'accept', + }, + }, + 'hashlimit_upto' => { + line: '-A INPUT -p tcp -m hashlimit --hashlimit-upto 16/sec --hashlimit-burst 640 --hashlimit-name upto --hashlimit-htable-size 1000000 --hashlimit-htable-max 320000 --hashlimit-htable-expire 36000000 -m comment --comment "806 - hashlimit_upto test" -j ACCEPT', + params: { + name: '806 - hashlimit_upto test', + chain: 'INPUT', + proto: 'tcp', + hashlimit_name: 'upto', + hashlimit_upto: '16/sec', + hashlimit_burst: '640', + hashlimit_htable_size: '1000000', + hashlimit_htable_max: '320000', + hashlimit_htable_expire: '36000000', + action: 'accept', + }, + }, + 'condition' => { + line: '-A INPUT -i enp0s8 -p icmp -m condition ! --condition "isblue" -m comment --comment "010 isblue ipv4" -j DROP', + params: { + name: '010 isblue ipv4', + chain: 'INPUT', + proto: 'icmp', + condition: '! isblue', + iniface: 'enp0s8', + action: 'drop', + }, + }, + 'ipsec_out_policy_ipsec' => { + line: '-A OUTPUT -d 20.0.0.0/8 -m policy --dir out --pol ipsec -m comment --comment "595 - ipsec_policy ipsec and out" -j REJECT --reject-with icmp-net-unreachable', + params: { + name: '595 - ipsec_policy ipsec and out', + chain: 'OUTPUT', + proto: 'all', + destination: '20.0.0.0/8', + ipsec_dir: 'out', + ipsec_policy: 'ipsec', + action: 'reject', + reject: 'icmp-net-unreachable', + }, + }, + 'ipsec_in_policy_none' => { + line: '-A INPUT -d 20.0.0.0/8 -m policy --dir in --pol none -m comment --comment "596 - ipsec_policy none and in" -j REJECT --reject-with icmp-net-unreachable', + params: { + name: '596 - ipsec_policy none and in', + chain: 'INPUT', + proto: 'all', + destination: '20.0.0.0/8', + ipsec_dir: 'in', + ipsec_policy: 'none', + action: 'reject', + reject: 'icmp-net-unreachable', + }, + }, + 'recent_set_rdest' => { + line: '-A INPUT -d 30.0.0.0/8 -m recent --set --name list1 --mask 255.255.255.255 --rdest -m comment --comment "597 - recent set"', + params: { + name: '597 - recent set', + chain: 'INPUT', + proto: 'all', + destination: '30.0.0.0/8', + recent: 'set', + rdest: true, + rname: 'list1', + }, + }, + 'recent_rcheck_complex' => { + line: '-A INPUT -d 30.0.0.0/8 -m recent --rcheck --seconds 60 --hitcount 5 --rttl --name list1 --mask 255.255.255.255 --rsource -m comment --comment "598 - recent rcheck"', + params: { + name: '598 - recent rcheck', + chain: 'INPUT', + proto: 'all', + destination: '30.0.0.0/8', + recent: 'rcheck', + rsource: true, + rname: 'list1', + rseconds: '60', + rhitcount: '5', + rttl: true, + }, + }, + 'recent_update_simple' => { + line: '-A INPUT -d 30.0.0.0/8 -m recent --update --name DEFAULT --mask 255.255.255.255 --rsource -m comment --comment "599 - recent update"', + params: { + name: '599 - recent update', + chain: 'INPUT', + proto: 'all', + destination: '30.0.0.0/8', + recent: 'update', + rname: 'DEFAULT', + }, + }, + 'recent_remove' => { + line: '-A INPUT -d 30.0.0.0/8 -m recent --remove --name DEFAULT --mask 255.255.255.255 --rsource -m comment --comment "600 - recent remove"', + params: { + name: '600 - recent remove', + chain: 'INPUT', + proto: 'all', + destination: '30.0.0.0/8', + recent: 'remove', + rname: 'DEFAULT', + }, + }, + 'log_params' => { + line: '-A OUTPUT -p tcp -m comment --comment "701 - log_uid, tcp-sequences and options" -j LOG --log-tcp-sequence --log-tcp-options --log-ip-options --log-uid', + params: { + name: '701 - log_uid, tcp-sequences and options', + chain: 'OUTPUT', + jump: 'LOG', + log_uid: true, + log_tcp_sequence: true, + log_tcp_options: true, + log_ip_options: true, + }, + }, + 'rpfilter_string' => { + line: '-A PREROUTING -p tcp -m rpfilter --invert -m comment --comment "901 - set rpfilter" -j ACCEPT', + params: { + name: '901 - set rpfilter', + chain: 'PREROUTING', + action: 'accept', + rpfilter: ['invert'], + }, + }, + 'rpfilter_array' => { + line: '-A PREROUTING -p tcp -m rpfilter --loose --validmark --accept-local --invert -m comment --comment "900 - set rpfilter" -j ACCEPT', + params: { + name: '900 - set rpfilter', + chain: 'PREROUTING', + action: 'accept', + rpfilter: ['loose', 'validmark', 'accept-local', 'invert'], + }, + }, }.freeze # This hash is for testing converting a hash to an argument line. @@ -1385,6 +1670,32 @@ }, args: ['-t', :filter, '-p', :tcp, '-j', 'REJECT', '-m', 'mark', '--mark', '0x1', '-m', 'connlimit', '--connlimit-above', '10', '--connlimit-mask', '32', '-m', 'comment', '--comment', '066 REJECT connlimit_above 10 with mask 32 and mark matches'], # rubocop:disable Layout/LineLength }, + 'match_mss' => { + params: { + name: '604 - set_mss', + table: 'filter', + chain: 'FORWARD', + proto: 'tcp', + tcp_flags: 'SYN,RST SYN', + mss: '1361:1541', + jump: 'TCPMSS', + set_mss: '1360', + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'tcp', '--tcp-flags', 'SYN,RST', 'SYN', '-j', 'TCPMSS', '--set-mss', '1360', '-m', 'tcpmss', '--mss', '1361:1541', '-m', 'comment', '--comment', '604 - set_mss'], + }, + 'match_mss_with_synproxy' => { + params: { + name: '604 - proxy SYN in range', + table: 'filter', + chain: 'FORWARD', + proto: 'tcp', + tcp_flags: 'SYN,RST SYN', + mss: '1361:1541', + jump: 'SYNPROXY', + synproxy_mss: '1360', + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'tcp', '--tcp-flags', 'SYN,RST', 'SYN', '-j', 'SYNPROXY', '--mss', '1360', '-m', 'tcpmss', '--mss', '1361:1541', '-m', 'comment', '--comment', '604 - proxy SYN in range'] + }, 'clamp_mss_to_pmtu' => { params: { name: '067 change max segment size', @@ -1527,4 +1838,268 @@ }, args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '80', '-j', 'SYNPROXY', '--mss', '1460', '-m', 'comment', '--comment', '001 creates rule with synproxy target'], }, + 'nflog_options_using_range' => { + params: { + name: '503 - test', + proto: 'all', + jump: 'NFLOG', + nflog_group: 3, + nflog_prefix: 'TEST PREFIX', + nflog_range: '16', + nflog_threshold: 2, + }, + args: ['-t', :filter, '-p', :all, '-j', 'NFLOG', '--nflog-group', 3, '--nflog-prefix', 'TEST PREFIX', '--nflog-range', '16', '--nflog-threshold', 2, '-m', 'comment', '--comment', '503 - test'], + }, + 'nflog_options_using_size' => { + iptables_version: '1.6.1', + params: { + name: '503 - test', + proto: 'all', + jump: 'NFLOG', + nflog_group: 3, + nflog_prefix: 'TEST PREFIX', + nflog_size: '16', + nflog_threshold: 2, + }, + args: ['-t', :filter, '-p', :all, '-j', 'NFLOG', '--nflog-group', 3, '--nflog-prefix', 'TEST PREFIX', '--nflog-size', '16', '--nflog-threshold', 2, '-m', 'comment', '--comment', '503 - test'], + }, + 'netmap_to' => { + params: { + name: '569 - test', + table: 'nat', + chain: 'POSTROUTING', + destination: '200.200.200.200/32', + jump: 'NETMAP', + to: '192.168.1.1', + }, + args: ['-t', :nat, '-d', '200.200.200.200/32', '-p', :tcp, '-j', 'NETMAP', '--to', '192.168.1.1', '-m', 'comment', '--comment', '569 - test'], + }, + 'snat_to_source' => { + params: { + name: '568 - tosource', + table: 'nat', + chain: 'POSTROUTING', + proto: 'tcp', + jump: 'SNAT', + tosource: '192.168.1.1', + }, + args: ['-t', :nat, '-p', :tcp, '-j', 'SNAT', '--to-source', '192.168.1.1', '-m', 'comment', '--comment', '568 - tosource'], + }, + 'dnat_to_dest' => { + params: { + name: '569 - todest', + table: 'nat', + chain: 'PREROUTING', + proto: 'tcp', + jump: 'DNAT', + source: '200.200.200.200', + todest: '192.168.1.1', + }, + args: ['-t', :nat, '-s', '200.200.200.200/32', '-p', :tcp, '-j', 'DNAT', '--to-destination', '192.168.1.1', '-m', 'comment', '--comment', '569 - todest'], + }, + 'redirect_to_ports' => { + params: { + name: '574 - toports', + table: 'nat', + chain: 'PREROUTING', + proto: 'icmp', + jump: 'REDIRECT', + toports: '2222', + }, + args: ['-t', :nat, '-p', :icmp, '-j', 'REDIRECT', '--to-ports', '2222', '-m', 'comment', '--comment', '574 - toports'], + }, + 'tee_gateway' => { + params: { + name: '810 - tee_gateway', + chain: 'PREROUTING', + table: 'mangle', + jump: 'TEE', + gateway: '10.0.0.2', + proto: 'all', + }, + args: ['-t', :mangle, '-p', :all, '-j', 'TEE', '--gateway', '10.0.0.2', '-m', 'comment', '--comment', '810 - tee_gateway'], + }, + 'match_time' => { + params: { + name: '805 - test', + chain: 'OUTPUT', + proto: 'tcp', + dport: '8080', + date_start: '2016-01-19T04:17:07', + date_stop: '2038-01-19T04:17:07', + time_start: '6:00', + time_stop: '17:00:00', + month_days: '7', + week_days: 'Tue', + kernel_timezone: true, + action: 'accept', + }, + args: ['-t', :filter, '-p', :tcp, '-m', 'multiport', '--dports', '8080', '-j', 'ACCEPT', '-m', 'time', '--timestart', '06:00:00', '--timestop', '17:00:00', '--monthdays', '7', '--weekdays', :Tue, '--datestart', '2016-01-19T04:17:07', '--datestop', '2038-01-19T04:17:07', '--kerneltz', '-m', 'comment', '--comment', '805 - test'], + }, + 'checksum_fill' => { + params: { + name: '576 - test', + table: 'mangle', + chain: 'POSTROUTING', + proto: 'udp', + outiface: 'virbr0', + dport: '68', + jump: 'CHECKSUM', + checksum_fill: true, + }, + args: ['-t', :mangle, '-o', 'virbr0', '-p', :udp, '-m', 'multiport', '--dports', '68', '-j', 'CHECKSUM', '--checksum-fill', '-m', 'comment', '--comment', '576 - test'], + }, + 'hashlimit_above' => { + params: { + name: '805 - hashlimit_above test', + proto: 'tcp', + hashlimit_name: 'above', + hashlimit_above: '16/sec', + hashlimit_htable_gcinterval: '10', + hashlimit_mode: 'srcip,dstip', + action: 'accept', + }, + args: ['-t', :filter, '-p', :tcp, '-j', 'ACCEPT', '-m', 'hashlimit', '--hashlimit-above', '16/sec', '--hashlimit-name', 'above', '--hashlimit-mode', 'srcip,dstip', '--hashlimit-htable-gcinterval', '10', '-m', 'comment', '--comment', '805 - hashlimit_above test'], + }, + 'hashlimit_upto' => { + params: { + name: '806 - hashlimit_upto test', + hashlimit_name: 'upto', + hashlimit_upto: '16/sec', + hashlimit_burst: '640', + hashlimit_htable_size: '1000000', + hashlimit_htable_max: '320000', + hashlimit_htable_expire: '36000000', + action: 'accept', + }, + args: ['-t', :filter, '-p', :tcp, '-j', 'ACCEPT', '-m', 'hashlimit', '--hashlimit-upto', '16/sec', '--hashlimit-name', 'upto', '--hashlimit-burst', '640', '--hashlimit-htable-size', '1000000', '--hashlimit-htable-max', '320000', '--hashlimit-htable-expire', '36000000', '-m', 'comment', '--comment', '806 - hashlimit_upto test'], + }, + 'condition' => { + params: { + name: '010 isblue ipv4', + chain: 'INPUT', + proto: 'icmp', + condition: '! isblue', + iniface: 'enp0s8', + action: 'drop', + }, + args: ['-t', :filter, '-i', 'enp0s8', '-p', :icmp, '-j', 'DROP', '-m', 'condition', '!', '--condition', 'isblue', '-m', 'comment', '--comment', '010 isblue ipv4'], + }, + 'ipsec_out_policy_ipsec' => { + params: { + name: '595 - ipsec_policy ipsec and out', + ensure: 'present', + action: 'reject', + chain: 'OUTPUT', + destination: '20.0.0.0/8', + ipsec_dir: 'out', + ipsec_policy: 'ipsec', + proto: 'all', + reject: 'icmp-net-unreachable', + table: 'filter', + }, + args: ['-t', :filter, '-d', '20.0.0.0/8', '-p', :all, '-m', 'policy', '--dir', :out, '--pol', :ipsec, '-j', 'REJECT', '--reject-with', 'icmp-net-unreachable', '-m', 'comment', '--comment', '595 - ipsec_policy ipsec and out'], + }, + 'ipsec_in_policy_none' => { + params: { + name: '596 - ipsec_policy none and in', + ensure: 'present', + action: 'reject', + chain: 'INPUT', + destination: '20.0.0.0/8', + ipsec_dir: 'in', + ipsec_policy: 'none', + proto: 'all', + reject: 'icmp-net-unreachable', + table: 'filter', + }, + args: ['-t', :filter, '-d', '20.0.0.0/8', '-p', :all, '-m', 'policy', '--dir', :in, '--pol', :none, '-j', 'REJECT', '--reject-with', 'icmp-net-unreachable', '-m', 'comment', '--comment', '596 - ipsec_policy none and in'], + }, + 'recent_set_rdest' => { + params: { + name: '597 - recent set', + table: 'filter', + chain: 'INPUT', + proto: 'all', + destination: '30.0.0.0/8', + recent: 'set', + rdest: true, + rname: 'list1', + }, + args: ['-t', :filter, '-d', '30.0.0.0/8', '-p', :all, '-m', 'recent', '--set', '--name', 'list1', '--rdest', '-m', 'comment', '--comment', '597 - recent set'], + }, + 'recent_rcheck_complex' => { + params: { + name: '598 - recent rcheck', + table: 'filter', + chain: 'INPUT', + proto: 'all', + destination: '30.0.0.0/8', + recent: 'rcheck', + rsource: true, + rname: 'list1', + rseconds: 60, + rhitcount: 5, + rttl: true, + }, + args: ['-t', :filter, '-d', '30.0.0.0/8', '-p', :all, '-m', 'recent', '--rcheck', '--seconds', 60, '--hitcount', 5, '--rttl', '--name', 'list1', '--rsource', '-m', 'comment', '--comment', '598 - recent rcheck'], + }, + 'recent_update_simple' => { + params: { + name: '599 - recent update', + table: 'filter', + chain: 'INPUT', + proto: 'all', + destination: '30.0.0.0/8', + recent: 'update', + }, + args: ['-t', :filter, '-d', '30.0.0.0/8', '-p', :all, '-m', 'recent', '--update', '-m', 'comment', '--comment', '599 - recent update'], + }, + 'recent_remove' => { + params: { + name: '600 - recent remove', + table: 'filter', + chain: 'INPUT', + proto: 'all', + destination: '30.0.0.0/8', + recent: 'remove', + }, + args: ['-t', :filter, '-d', '30.0.0.0/8', '-p', :all, '-m', 'recent', '--remove', '-m', 'comment', '--comment', '600 - recent remove'], + }, + 'log_params' => { + params: { + name: '701 - log_uid, tcp-sequences and options', + chain: 'OUTPUT', + jump: 'LOG', + log_uid: true, + log_tcp_sequence: true, + log_tcp_options: true, + log_ip_options: true, + }, + args: ['-t', :filter, '-p', :tcp, '-j', 'LOG', '--log-uid', '--log-tcp-sequence', '--log-tcp-options', '--log-ip-options', '-m', 'comment', '--comment', '701 - log_uid, tcp-sequences and options'], + }, + 'rpfilter_string' => { + kernel_version: '3.10.0', + iptables_version: '1.5', + params: { + name: '901 - set rpfilter', + table: 'raw', + chain: 'PREROUTING', + action: 'accept', + rpfilter: 'invert', + }, + args: ['-t', :raw, '-p', :tcp, '-j', 'ACCEPT', '-m', 'rpfilter', '--invert', '-m', 'comment', '--comment', '901 - set rpfilter'], + }, + 'rpfilter_array' => { + kernel_version: '3.10.0', + iptables_version: '1.5', + params: { + name: '900 - set rpfilter', + table: 'raw', + chain: 'PREROUTING', + action: 'accept', + rpfilter: [ 'loose', 'validmark', 'accept-local', 'invert' ], + }, + args: ['-t', :raw, '-p', :tcp, '-j', 'ACCEPT', '-m', 'rpfilter', '--loose', '--validmark', '--accept-local', '--invert', '-m', 'comment', '--comment', '900 - set rpfilter'], + }, }.freeze From 40887cb1d77ea756edc93f3fe11387bc97d17e2a Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 25 May 2023 20:46:54 -0700 Subject: [PATCH 11/19] Align @resource_list instances in providers The @resource_list variable is almost impossible to reason about or check sync between v4 vs. v6 provider implementations - align them! --- lib/puppet/provider/firewall/ip6tables.rb | 40 +++++++++++------------ 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/puppet/provider/firewall/ip6tables.rb b/lib/puppet/provider/firewall/ip6tables.rb index 6681c7218..7f12db7b6 100644 --- a/lib/puppet/provider/firewall/ip6tables.rb +++ b/lib/puppet/provider/firewall/ip6tables.rb @@ -318,24 +318,24 @@ def self.iptables_save(*args) # (Note: on my CentOS 6.4 ip6tables-save returns -m frag on the place # I put it when calling the command. So compability with manual changes # not provided with current parser [georg.koester]) - @resource_list = [:table, :source, :destination, :iniface, :outiface, :physdev_in, - :physdev_out, :physdev_is_bridged, :physdev_is_in, :physdev_is_out, - :proto, :ishasmorefrags, :islastfrag, :isfirstfrag, :src_range, :dst_range, - :tcp_flags, :uid, :gid, :mac_source, :sport, :dport, :port, :src_type, - :dst_type, :socket, :pkttype, :ipsec_dir, :ipsec_policy, :state, - :ctstate, :ctproto, :ctorigsrc, :ctorigdst, :ctreplsrc, :ctrepldst, - :ctorigsrcport, :ctorigdstport, :ctreplsrcport, :ctrepldstport, :ctstatus, :ctexpire, :ctdir, - :icmp, :hop_limit, :limit, :burst, :length, :recent, :rseconds, :reap, - :rhitcount, :rttl, :rname, :mask, :rsource, :rdest, :ipset, :string, :string_hex, :string_algo, - :string_from, :string_to, :jump, - :nflog_group, :nflog_prefix, :nflog_range, :nflog_size, :nflog_threshold, - :clamp_mss_to_pmtu, :gateway, :todest, - :tosource, :toports, :checksum_fill, :log_level, :log_prefix, :log_uid, :log_tcp_sequence, :log_tcp_options, :log_ip_options, :random_fully, - :reject, :set_mss, :set_dscp, :set_dscp_class, :mss, :queue_num, :queue_bypass, - :set_mark, :match_mark, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, - :synproxy_sack_perm, :synproxy_timestamp, :synproxy_wscale, :synproxy_mss, :synproxy_ecn, - :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone, - :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, :bytecode, :zone, :helper, :rpfilter, :condition, :name, :notrack] + @resource_list = [ + :table, :source, :destination, :iniface, :outiface, + :physdev_in, :physdev_out, :physdev_is_bridged, :physdev_is_in, :physdev_is_out, + :proto, :ishasmorefrags, :islastfrag, :isfirstfrag, + :src_range, :dst_range, :tcp_flags, :uid, :gid, :mac_source, :sport, :dport, :port, + :src_type, :dst_type, :socket, :pkttype, :ipsec_dir, :ipsec_policy, + :state, :ctstate, :ctproto, :ctorigsrc, :ctorigdst, :ctreplsrc, :ctrepldst, + :ctorigsrcport, :ctorigdstport, :ctreplsrcport, :ctrepldstport, :ctstatus, :ctexpire, :ctdir, + :icmp, :hop_limit, :limit, :burst, :length, :recent, :rseconds, :reap, + :rhitcount, :rttl, :rname, :mask, :rsource, :rdest, :ipset, :string, :string_hex, :string_algo, + :string_from, :string_to, :jump, :queue_num, :queue_bypass, + :nflog_group, :nflog_prefix, :nflog_range, :nflog_size, :nflog_threshold, :clamp_mss_to_pmtu, :gateway, + :set_mss, :set_dscp, :set_dscp_class, :todest, :tosource, :toports, :checksum_fill, :random_fully, :log_prefix, + :log_level, :log_uid, :log_tcp_sequence, :log_tcp_options, :log_ip_options, :reject, :set_mark, :match_mark, :mss, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, + :synproxy_sack_perm, :synproxy_timestamp, :synproxy_wscale, :synproxy_mss, :synproxy_ecn, + :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone, + :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, :bytecode, :zone, :helper, :rpfilter, :condition, :name, :notrack + ] end From 71c0ab1629fae04755d8f4a46276e46b4670a50a Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 25 May 2023 21:07:14 -0700 Subject: [PATCH 12/19] Group target match options right behind targets The @resource_list determines the order in which arguments appear on the generated commandline for ip[6]tables; if there are options which belong to a match module interleaved with options which belong to a target extension, and both the target and match extensions are used in the same rule, perverse outputs like '-j SYNPROXY -m tcpmss --mss 1360 --sack-perm' are possible - where an entire match extension and its arguments are inserted between a target extension and its arguments. Group options which belong to a target extension together immediately behind :jump, so a target extension spec and its arguments cannot be separated; and add comments in the array to ensure it remains clear why this is important. --- lib/puppet/provider/firewall/ip6tables.rb | 15 +++++++++++---- lib/puppet/provider/firewall/iptables.rb | 17 ++++++++++++----- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/lib/puppet/provider/firewall/ip6tables.rb b/lib/puppet/provider/firewall/ip6tables.rb index 7f12db7b6..4df8ea15b 100644 --- a/lib/puppet/provider/firewall/ip6tables.rb +++ b/lib/puppet/provider/firewall/ip6tables.rb @@ -320,6 +320,7 @@ def self.iptables_save(*args) # not provided with current parser [georg.koester]) @resource_list = [ :table, :source, :destination, :iniface, :outiface, + # Match module options :physdev_in, :physdev_out, :physdev_is_bridged, :physdev_is_in, :physdev_is_out, :proto, :ishasmorefrags, :islastfrag, :isfirstfrag, :src_range, :dst_range, :tcp_flags, :uid, :gid, :mac_source, :sport, :dport, :port, @@ -328,14 +329,20 @@ def self.iptables_save(*args) :ctorigsrcport, :ctorigdstport, :ctreplsrcport, :ctrepldstport, :ctstatus, :ctexpire, :ctdir, :icmp, :hop_limit, :limit, :burst, :length, :recent, :rseconds, :reap, :rhitcount, :rttl, :rname, :mask, :rsource, :rdest, :ipset, :string, :string_hex, :string_algo, - :string_from, :string_to, :jump, :queue_num, :queue_bypass, - :nflog_group, :nflog_prefix, :nflog_range, :nflog_size, :nflog_threshold, :clamp_mss_to_pmtu, :gateway, + :string_from, :string_to, + # ONLY target extension options from here to END + # otherwise a jump target spec and its options can end up separated by a match module and ITS options + :jump, + :queue_num, :queue_bypass, :nflog_group, :nflog_prefix, :nflog_range, :nflog_size, :nflog_threshold, :clamp_mss_to_pmtu, :gateway, :set_mss, :set_dscp, :set_dscp_class, :todest, :tosource, :toports, :checksum_fill, :random_fully, :log_prefix, - :log_level, :log_uid, :log_tcp_sequence, :log_tcp_options, :log_ip_options, :reject, :set_mark, :match_mark, :mss, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, + :log_level, :log_uid, :log_tcp_sequence, :log_tcp_options, :log_ip_options, :reject, :set_mark, :zone, :helper, :notrack, :synproxy_sack_perm, :synproxy_timestamp, :synproxy_wscale, :synproxy_mss, :synproxy_ecn, + # END target extension options + # Resume matcher options + :match_mark, :mss, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone, :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, :bytecode, :zone, :helper, :rpfilter, :condition, :name, :notrack + :hashlimit_htable_max, :hashlimit_htable_expire, :hashlimit_htable_gcinterval, :bytecode, :rpfilter, :condition, :name ] end diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 717df6631..729c6f5b2 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -355,6 +355,7 @@ def munge_resource_map_from_resource(resource_map_original, compare) # This order can be determined by going through iptables source code or just tweaking and trying manually @resource_list = [ :table, :source, :destination, :iniface, :outiface, + # Match module options :physdev_in, :physdev_out, :physdev_is_bridged, :physdev_is_in, :physdev_is_out, :proto, :isfragment, :stat_mode, :stat_every, :stat_packet, :stat_probability, :src_range, :dst_range, :tcp_flags, :uid, :gid, :mac_source, :sport, :dport, :port, @@ -363,16 +364,22 @@ def munge_resource_map_from_resource(resource_map_original, compare) :ctorigsrcport, :ctorigdstport, :ctreplsrcport, :ctrepldstport, :ctstatus, :ctexpire, :ctdir, :icmp, :limit, :burst, :length, :recent, :rseconds, :reap, :rhitcount, :rttl, :rname, :mask, :rsource, :rdest, :ipset, :string, :string_hex, :string_algo, - :string_from, :string_to, :jump, :goto, :clusterip_new, :clusterip_hashmode, - :clusterip_clustermac, :clusterip_total_nodes, :clusterip_local_node, :clusterip_hash_init, :queue_num, :queue_bypass, - :nflog_group, :nflog_prefix, :nflog_range, :nflog_size, :nflog_threshold, :clamp_mss_to_pmtu, :gateway, + :string_from, :string_to, + # ONLY target extension options from here to END + # otherwise a target spec and its options can end up separated by a match module and ITS options + :jump, :goto, + :clusterip_new, :clusterip_hashmode, :clusterip_clustermac, :clusterip_total_nodes, :clusterip_local_node, :clusterip_hash_init, + :queue_num, :queue_bypass, :nflog_group, :nflog_prefix, :nflog_range, :nflog_size, :nflog_threshold, :clamp_mss_to_pmtu, :gateway, :set_mss, :set_dscp, :set_dscp_class, :todest, :tosource, :toports, :to, :checksum_fill, :random_fully, :random, :log_prefix, - :log_level, :log_uid, :log_tcp_sequence, :log_tcp_options, :log_ip_options, :reject, :set_mark, :match_mark, :mss, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, + :log_level, :log_uid, :log_tcp_sequence, :log_tcp_options, :log_ip_options, :reject, :set_mark, :zone, :helper, :notrack, :synproxy_sack_perm, :synproxy_timestamp, :synproxy_wscale, :synproxy_mss, :synproxy_ecn, + # END target extension options + # Resume matcher options + :match_mark, :mss, :connlimit_above, :connlimit_mask, :connmark, :time_start, :time_stop, :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone, :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, :bytecode, :ipvs, :zone, :helper, :cgroup, :rpfilter, :condition, :name, :notrack + :hashlimit_htable_max, :hashlimit_htable_expire, :hashlimit_htable_gcinterval, :bytecode, :ipvs, :cgroup, :rpfilter, :condition, :name ] def insert From f85424b9e41bdd31681c21a3c6cc5268f3447416 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 25 May 2023 21:41:39 -0700 Subject: [PATCH 13/19] Teach the parser context-awareness Not all arguments are globally unique across all iptables extensions, and we should not search the entire rule string for all arguments when each one is only ever valid within a known context. Define start and stop delimiters for existing known overlaps, and default to a full-string search if no start or end markers are defined. Also, stop being quite so terse; the Ruby authors do not charge by the letter for variable names, and readability is good. --- lib/puppet/provider/firewall/ip6tables.rb | 24 ++++++++++ lib/puppet/provider/firewall/iptables.rb | 56 +++++++++++++++++++---- 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/lib/puppet/provider/firewall/ip6tables.rb b/lib/puppet/provider/firewall/ip6tables.rb index 4df8ea15b..b4cac6145 100644 --- a/lib/puppet/provider/firewall/ip6tables.rb +++ b/lib/puppet/provider/firewall/ip6tables.rb @@ -345,4 +345,28 @@ def self.iptables_save(*args) :hashlimit_mode, :hashlimit_srcmask, :hashlimit_dstmask, :hashlimit_htable_size, :hashlimit_htable_max, :hashlimit_htable_expire, :hashlimit_htable_gcinterval, :bytecode, :rpfilter, :condition, :name ] + + # Not all arguments are globally unique across all iptables extensions. For matchers we should + # only find within a specific context, a start and end marker can be supplied here. Either a + # plain string or a regex will work; these are passed as an argument to String#index(), and limit + # the search scope. If the resource matches on or after the first matching character in + # context_start, and before the first matching character in context_end, the match succeeds. + @resource_parse_context = { + synproxy_mss: { + context_start: '-j SYNPROXY', + }, + mss: { + # Extra starting space because the matcher for :mss includes '-m tcpmss', + # and the search for it prefixes the matcher with a space + context_start: ' -m tcpmss', + context_end: %r{ -[mgj] }, + }, + string_to: { + context_start: '-m string', + context_end: %r{ -[mgj] }, + }, + to: { + context_start: '-j NETMAP', + }, + } end diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 729c6f5b2..b4903f6c7 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -382,6 +382,30 @@ def munge_resource_map_from_resource(resource_map_original, compare) :hashlimit_htable_max, :hashlimit_htable_expire, :hashlimit_htable_gcinterval, :bytecode, :ipvs, :cgroup, :rpfilter, :condition, :name ] + # Not all arguments are globally unique across all iptables extensions. For matchers we should + # only find within a specific context, a start and end marker can be supplied here. Either a + # plain string or a regex will work; these are passed as an argument to String#index(), and limit + # the search scope. If the resource matches on or after the first matching character in + # context_start, and before the first matching character in context_end, the match succeeds. + @resource_parse_context = { + synproxy_mss: { + context_start: '-j SYNPROXY', + }, + mss: { + # Extra starting space because the matcher for :mss includes '-m tcpmss', + # and the search for it prefixes the matcher with a space + context_start: ' -m tcpmss', + context_end: %r{ -[mgj] }, + }, + string_to: { + context_start: '-m string', + context_end: %r{ -[mgj] }, + }, + to: { + context_start: '-j NETMAP', + }, + } + def insert debug 'Inserting rule %s' % resource[:name] iptables insert_args @@ -591,17 +615,31 @@ def self.rule_to_hash(line, table, counter) ############ # Populate parser_list with used value, in the correct order ############ - map_index = {} - resource_map.each_pair do |map_k, map_v| - [map_v].flatten.each do |v| - ind = values.index(%r{\s#{v}\s}) - next unless ind - map_index[map_k] = ind + index_map = resource_map.each_with_object({}) do |(resource_name, matchers), index_mapping| + # Get the start and end markers. If none are defined, use start/end of the rule. + context_start = @resource_parse_context.dig(resource_name, :context_start) || %r{^} + context_end = @resource_parse_context.dig(resource_name, :context_end) || %r{$} + + # Get the offset at which to start searching + search_start = values.index(context_start) + # If a context marker was defined but wasn't in the string, then the resource can't be here, either + next unless search_start + + # Get the offset at which to stop searching + # Start the search for the end marker past the end of context_start, so end markers + # like '-m' won't match against their own start marker (e.g., '-m tcpmss') + context_start = values.match(context_start).to_s if context_start.is_a? Regexp + # If an end marker was defined but wasn't in the string, the context may be the last jump or match + # target in the rule; so if no end marker is found, just use something past the end of the string. + search_end = values.index(context_end, (search_start + context_start.length)) || values.length + + [matchers].flatten.each do |v| + index = values.index(%r{\s#{v}\s}, search_start) + next unless index && index < search_end + index_mapping[resource_name] = index end end - # Generate parser_list based on the index of the found option - parser_list = [] - map_index.sort_by { |_k, v| v }.each { |mapi| parser_list << mapi.first } + parser_list = index_map.sort_by { |_resource_name, index| index }.to_h.keys ############ # MAIN PARSE From 9a641f8da3a620e636722dd089786515f33fb335 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 25 May 2023 22:18:22 -0700 Subject: [PATCH 14/19] Remove trash from the middle of the main parse The regular parse mechanisms can handle table, chain, and comment(s) on their own already, if a single space character is added to the head of the rule. :table was already in @resource_map, :chain is added to the parse output by the unnecessary special mechanism anyway, and :name already grabs ALL comments successfully since the escaping fixes. 'line' is used instead of 'line.dup' because string addition returns a new string, so String#dup would be redundant. --- lib/puppet/provider/firewall/ip6tables.rb | 1 + lib/puppet/provider/firewall/iptables.rb | 20 ++------------------ 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/lib/puppet/provider/firewall/ip6tables.rb b/lib/puppet/provider/firewall/ip6tables.rb index b4cac6145..fce908903 100644 --- a/lib/puppet/provider/firewall/ip6tables.rb +++ b/lib/puppet/provider/firewall/ip6tables.rb @@ -93,6 +93,7 @@ def self.iptables_save(*args) @resource_map = { burst: '--limit-burst', + chain: '-A', checksum_fill: '--checksum-fill', clamp_mss_to_pmtu: '--clamp-mss-to-pmtu', condition: '--condition', diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index b4903f6c7..a41fb38c3 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -85,6 +85,7 @@ @resource_map = { burst: '--limit-burst', + chain: '-A', checksum_fill: '--checksum-fill', clamp_mss_to_pmtu: '--clamp-mss-to-pmtu', condition: '--condition', @@ -512,7 +513,7 @@ def self.instances def self.rule_to_hash(line, table, counter) hash = {} keys = [] - values = line.dup + values = ' ' + line #################### # PRE-PARSE CLUDGING @@ -656,23 +657,6 @@ def self.rule_to_hash(line, table, counter) end end - # Manually remove chain - if %r{(\s|^)-A\s}.match?(values) - values = values.sub(%r{(\s|^)-A\s}, '\1') - keys << :chain - end - - # Manually remove table (used in some tests) - if %r{^-t\s}.match?(values) - values = values.sub(%r{^-t\s}, '') - keys << :table - end - - # manually remove comments if they made it this far - if %r{-m comment --comment}.match?(values) - values = values.sub(%r{-m comment --comment "((?:\\"|[^"])*)"}, {}) - end - valrev = values.scan(%r{("([^"\\]|\\.)*"|\S+)}).transpose[0].reverse if keys.length != valrev.length From 1f8488ff9dcf534b120b785505f0ba7ecdcb5969 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Thu, 25 May 2023 22:47:19 -0700 Subject: [PATCH 15/19] Reverse the parse direction Parse the rule from the head to prevent the parser from finding aliases of the intended argument in the rule's heead. String#slice! searches from the head of a string, so if the parser is using it to iteratively search for strings ordered progressively further from the back of a rule, each iteration is a new opportunity to find an aliased substring in its head and completely moot the point of generating an ordered parse list in the first place. Additionally: - Untie the logical knots that were required for reverse-parsing - Don't use Enumerable#each to do absolutely everything by side effect when an iterator could just return the constructed value directly - Don't pre-define variables we don't need yet just so they can be reassigned - Freshen up pointlessly terse variable names that only exist in an ephemeral scope anyway, this is already hard enough to follow - Stop our value regex from capturing a second, nested group if we only need the outer to avoid any later need to transpose the result - Fix any remaining variables with misleading names (e.g., valrev is no longer "values, reversed") or errant comments that no longer accurately describe the operation of the code --- lib/puppet/provider/firewall/iptables.rb | 35 ++++++++++++------------ 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index a41fb38c3..0c8d0ac69 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -511,8 +511,6 @@ def self.instances end def self.rule_to_hash(line, table, counter) - hash = {} - keys = [] values = ' ' + line #################### @@ -646,35 +644,38 @@ def self.rule_to_hash(line, table, counter) # MAIN PARSE ############ - # Here we iterate across our values to generate an array of keys - parser_list.reverse_each do |k| - resource_map_key = resource_map[k] - [resource_map_key].flatten.each do |opt| - if values.slice!(%r{\s#{opt}}) - keys << k + # Iterate through the sorted list of resource matchers we found, and + # pull them out of the rule text so only their matching values remain + keys = parser_list.each_with_object([]) do |resource_key, found_keys| + [resource_map[resource_key]].flatten.each do |matcher| + if values.slice!(%r{\s#{matcher}}) + found_keys << resource_key break end end end - valrev = values.scan(%r{("([^"\\]|\\.)*"|\S+)}).transpose[0].reverse + # Slurp up the resource values + found_values = values.scan(%r{("(?:[^"\\]|\\.)*"|\S+)}).flatten - if keys.length != valrev.length - warning "Skipping unparsable iptables rule: keys (#{keys.length}) and values (#{valrev.length}) count mismatch on line: #{line}" + if keys.length != found_values.length + warning "Skipping unparsable iptables rule: keys (#{keys.length}) and values (#{found_values.length}) count mismatch on line: #{line}" return end # Here we generate the main hash by scanning arguments off the values # string, handling any quoted characters present in the value, and then # zipping the values with the array of keys. - keys.zip(valrev) do |f, v| - hash[f] = if %r{^".*"$}.match?(v) - v.sub(%r{^"(.*)"$}, '\1').gsub(%r{\\(\\|'|")}, '\1') - else - v.dup - end + found_values = found_values.map do |value| + if %r{^".*"$}.match?(value) + value.sub(%r{^"(.*)"$}, '\1').gsub(%r{\\(\\|'|")}, '\1') + else + value + end end + hash = keys.zip(found_values).to_h + ##################### # POST PARSE CLUDGING ##################### From ed26196ff4404a8d69c183d44530b9aadb887803 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Fri, 26 May 2023 00:43:31 -0700 Subject: [PATCH 16/19] Treat tcpmss match extension as a match extension The ip[6]tables providers understand handling extension modules; treat tcpmss as an extension module, rather than mangling its matcher --- lib/puppet/provider/firewall/ip6tables.rb | 7 ++++--- lib/puppet/provider/firewall/iptables.rb | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/puppet/provider/firewall/ip6tables.rb b/lib/puppet/provider/firewall/ip6tables.rb index fce908903..d054fe264 100644 --- a/lib/puppet/provider/firewall/ip6tables.rb +++ b/lib/puppet/provider/firewall/ip6tables.rb @@ -142,7 +142,7 @@ def self.iptables_save(*args) match_mark: '-m mark --mark', name: '-m comment --comment', mac_source: ['-m mac --mac-source', '--mac-source'], - mss: '-m tcpmss --mss', + mss: '--mss', nflog_group: '--nflog-group', nflog_prefix: '--nflog-prefix', nflog_range: '--nflog-range', @@ -276,6 +276,7 @@ def self.iptables_save(*args) iprange: [:src_range, :dst_range], owner: [:uid, :gid], condition: [:condition], + tcpmss: [:mss], conntrack: [:ctstate, :ctproto, :ctorigsrc, :ctorigdst, :ctreplsrc, :ctrepldst, :ctorigsrcport, :ctorigdstport, :ctreplsrcport, :ctrepldstport, :ctstatus, :ctexpire, :ctdir], time: [:time_start, :time_stop, :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone], @@ -357,8 +358,8 @@ def self.iptables_save(*args) context_start: '-j SYNPROXY', }, mss: { - # Extra starting space because the matcher for :mss includes '-m tcpmss', - # and the search for it prefixes the matcher with a space + # Extra starting space because '-m tcpmss' gets prepended to the matcher for :mss before parse, + # and the search for it while building the parser list prefixes the matcher with a space context_start: ' -m tcpmss', context_end: %r{ -[mgj] }, }, diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 0c8d0ac69..60fbc9176 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -130,7 +130,7 @@ mac_source: ['-m mac --mac-source', '--mac-source'], mask: '--mask', match_mark: '-m mark --mark', - mss: '-m tcpmss --mss', + mss: '--mss', name: '-m comment --comment', nflog_group: '--nflog-group', nflog_prefix: '--nflog-prefix', @@ -276,6 +276,7 @@ iprange: [:src_range, :dst_range], owner: [:uid, :gid], condition: [:condition], + tcpmss: [:mss], conntrack: [:ctstate, :ctproto, :ctorigsrc, :ctorigdst, :ctreplsrc, :ctrepldst, :ctorigsrcport, :ctorigdstport, :ctreplsrcport, :ctrepldstport, :ctstatus, :ctexpire, :ctdir], time: [:time_start, :time_stop, :month_days, :week_days, :date_start, :date_stop, :time_contiguous, :kernel_timezone], @@ -393,8 +394,8 @@ def munge_resource_map_from_resource(resource_map_original, compare) context_start: '-j SYNPROXY', }, mss: { - # Extra starting space because the matcher for :mss includes '-m tcpmss', - # and the search for it prefixes the matcher with a space + # Extra starting space because '-m tcpmss' gets prepended to the matcher for :mss before parse, + # and the search for it while building the parser list prefixes the matcher with a space context_start: ' -m tcpmss', context_end: %r{ -[mgj] }, }, From dd862c4dd7672bc8ade8b00ba6d2c6ba30d006b0 Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Fri, 26 May 2023 01:05:14 -0700 Subject: [PATCH 17/19] Make options for '-m rpfilter' work correctly Parse the trailing arguments to '-m rpfilter' out of existing rules. Prior behavior included the '--' prefix along with the options themselves when pulling them out of the rule. For ip6tables, the provider could not correctly generate an ip6tables commandline that included '-m rpfilter' at all - its inclusion in the known booleans array precluded its options being expanded or included at all. Additionally: - Using a comma rather than a space as a separator character in the pre-parse munging doesn't require any quotes, nor does it require any new post-parse munging when there is already an existing iterator to handle splitting of comma-separated multiple elements into arrays - '-m rpfilter' on its own is supposed to be valid, and in fact is used in exactly this style of invocation in the examples included in 'man iptables-extensions'; but support for '-m rpfilter' in this module is limited to uses that include one or more modifying arguments. When this is eventually fixed (which I do not have time to do right now), the updated pre-parse munge logic will work with no further alteration - Adjusting the regex used by String#scan to capture the arguments to treat its capture groups differently SIGNIFICANTLY simplifies the logic around substitutions for the pre-parse munge such that no additional branching is required, and the operation is still safe --- lib/puppet/provider/firewall/ip6tables.rb | 1 - lib/puppet/provider/firewall/iptables.rb | 16 ++++------------ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/lib/puppet/provider/firewall/ip6tables.rb b/lib/puppet/provider/firewall/ip6tables.rb index d054fe264..de67fe869 100644 --- a/lib/puppet/provider/firewall/ip6tables.rb +++ b/lib/puppet/provider/firewall/ip6tables.rb @@ -243,7 +243,6 @@ def self.iptables_save(*args) :rsource, :rdest, :reap, - :rpfilter, :rttl, :socket, :physdev_is_bridged, diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 60fbc9176..9b60dc472 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -581,15 +581,9 @@ def self.rule_to_hash(line, table, counter) (\s--next)?}x, '--pol "ipsec\1\2\3\4\5\6\7\8" ') - # rpfilter also takes multiple parameters; use quote trick again - rpfilter_opts = values.scan(%r{-m\srpfilter(\s(--loose)|\s(--validmark)|\s(--accept-local)|\s(--invert))+}) - if rpfilter_opts && rpfilter_opts.length == 1 && rpfilter_opts[0] - rpfilter_opts = rpfilter_opts[0][1..-1].reject { |x| x.nil? } - values = values.sub( - %r{-m\srpfilter(\s(--loose)|\s(--validmark)|\s(--accept-local)|\s(--invert))+}, - "-m rpfilter \"#{rpfilter_opts.join(' ')}\"", - ) - end + # rpfilter can take multiple parameters; if present, strip and comma-join them. + rpfilter_opts = values.scan(%r{-m\srpfilter(?:\s--(loose)|\s--(validmark)|\s--(accept-local)|\s--(invert))+}).flatten.compact + values.sub!(%r{-m\srpfilter(?:\s--(?:loose|validmark|accept-local|invert))+}, "-m rpfilter #{rpfilter_opts.join(',')}") # on some iptables versions, --connlimit-saddr switch is added after the rule is applied values = values.gsub(%r{--connlimit-saddr}, '') @@ -681,7 +675,7 @@ def self.rule_to_hash(line, table, counter) # POST PARSE CLUDGING ##################### - [:dport, :sport, :port, :state, :ctstate, :ctstatus].each do |prop| + [:dport, :sport, :port, :state, :ctstate, :ctstatus, :rpfilter].each do |prop| hash[prop] = hash[prop].split(',') unless hash[prop].nil? end @@ -689,8 +683,6 @@ def self.rule_to_hash(line, table, counter) hash[prop] = hash[prop].split(';') unless hash[prop].nil? end - hash[:rpfilter] = hash[:rpfilter].split(' ') unless hash[:rpfilter].nil? - ## clean up DSCP class to HEX mappings valid_dscp_classes = { '0x0a' => 'af11', From deb20255bc374164397cf9ffe553d875e526141f Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Fri, 26 May 2023 01:49:15 -0700 Subject: [PATCH 18/19] Make mode options for '-m recent' parse correctly Options setting the mode of operation for the 'recent' match extension no longer parse from existing rule definitions with their long-option double-dashes still included. Note that these options are meant to be bang-invertible, but this module does not support that use case (and currently will explode if it encounters any such rule). --- lib/puppet/provider/firewall/iptables.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/puppet/provider/firewall/iptables.rb b/lib/puppet/provider/firewall/iptables.rb index 9b60dc472..a410c096a 100644 --- a/lib/puppet/provider/firewall/iptables.rb +++ b/lib/puppet/provider/firewall/iptables.rb @@ -585,6 +585,10 @@ def self.rule_to_hash(line, table, counter) rpfilter_opts = values.scan(%r{-m\srpfilter(?:\s--(loose)|\s--(validmark)|\s--(accept-local)|\s--(invert))+}).flatten.compact values.sub!(%r{-m\srpfilter(?:\s--(?:loose|validmark|accept-local|invert))+}, "-m rpfilter #{rpfilter_opts.join(',')}") + # For recent matching, the 'recent' param takes the name of the long opt that should follow '-m recent', + # which otherwise gets parsed out with the double-dashes for the long opt still present + values.gsub!(%r{#{@resource_map[:recent]}\s--(set|rcheck|update|remove)}, "#{@resource_map[:recent]} \\1") + # on some iptables versions, --connlimit-saddr switch is added after the rule is applied values = values.gsub(%r{--connlimit-saddr}, '') From 0714213f63d29d8d1153a5e6bd8e32bb14d71b3d Mon Sep 17 00:00:00 2001 From: Steve Russell Date: Fri, 26 May 2023 02:21:28 -0700 Subject: [PATCH 19/19] Capture progress on Rubocop's complexity metrics --- .rubocop_todo.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 108e85171..07a94b232 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -112,7 +112,7 @@ Lint/RedundantSafeNavigation: # Offense count: 13 # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes. Metrics/AbcSize: - Max: 235 + Max: 226.8 # Offense count: 17 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns, inherit_mode. @@ -128,7 +128,7 @@ Metrics/BlockNesting: # Offense count: 8 # Configuration parameters: AllowedMethods, AllowedPatterns. Metrics/CyclomaticComplexity: - Max: 60 + Max: 57 # Offense count: 19 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. @@ -143,7 +143,7 @@ Metrics/ModuleLength: # Offense count: 6 # Configuration parameters: AllowedMethods, AllowedPatterns. Metrics/PerceivedComplexity: - Max: 65 + Max: 62 # Offense count: 1 # Configuration parameters: EnforcedStyleForLeadingUnderscores.