From fd33346e0e73706d6ade65ea91d17ac05083a437 Mon Sep 17 00:00:00 2001 From: Ray Ruvinskiy Date: Thu, 17 Nov 2016 10:49:41 -0500 Subject: [PATCH] Fix handling of `weight` attribute in simple_iptables_rule resource Typically, the order of the rules in an iptables chain is determined by the order of simple_iptables_rules resources in a recipe. However, if a non-standard `weight` attribute value was used in *any* simple_iptables_rule, the relative order of the rules that had the same weight became arbitrary. This is because the code repeatedly calls `sort!` on the array of rules after adding every rule, using the weights as the sort key, which potentially causes rules with the same weight to be sorted in a different order with respect to each other on each invocation of the `sort!` method, since they're viewed as equivalent. Store rules in a hash where keys are weights and values are arrays of rules having that weight. As new rules are added, they're appended to the relevant list. When the final iptables rule file is generated, we output all the rules in decreasing weight order. Also take this opportunity to store rule in default-precedence attributes rather than "normal" (persistent) attributes. As we reset all the attributes at the beginning of the Chef run, there is no point to persisting the attributes. This also resolves deprecation warnings for node.set in newer versions of Chef. Resolves #81 --- attributes/default.rb | 4 +- providers/policy.rb | 2 +- providers/rule.rb | 26 ++++--- recipes/default.rb | 16 ++--- spec/spec_helper.rb | 6 ++ .../test_simple_iptables/metadata.rb | 13 ++++ .../test_simple_iptables/recipes/weight.rb | 39 +++++++++++ spec/weight_spec.rb | 69 +++++++++++++++++++ templates/default/ip6tables-rules.erb | 24 ++++--- templates/default/iptables-rules.erb | 24 ++++--- test/fixtures/smoke/recipes/ipv6_default.rb | 2 +- test/fixtures/smoke/recipes/ipv6_tables.rb | 6 +- test/fixtures/smoke/recipes/tables.rb | 2 +- 13 files changed, 186 insertions(+), 47 deletions(-) create mode 100644 spec/spec_helper.rb create mode 100644 spec/support/cookbooks/test_simple_iptables/metadata.rb create mode 100644 spec/support/cookbooks/test_simple_iptables/recipes/weight.rb create mode 100644 spec/weight_spec.rb diff --git a/attributes/default.rb b/attributes/default.rb index 72c92b9..3f470a6 100644 --- a/attributes/default.rb +++ b/attributes/default.rb @@ -1,7 +1,7 @@ -default["simple_iptables"]["ipv4"]["rules"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []} +default["simple_iptables"]["ipv4"]["rules"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}} default["simple_iptables"]["ipv4"]["chains"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []} default["simple_iptables"]["ipv4"]["policy"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}} -default["simple_iptables"]["ipv6"]["rules"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []} +default["simple_iptables"]["ipv6"]["rules"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}} default["simple_iptables"]["ipv6"]["chains"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []} default["simple_iptables"]["ipv6"]["policy"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}} diff --git a/providers/policy.rb b/providers/policy.rb index 7d2b4bc..c8e82cc 100644 --- a/providers/policy.rb +++ b/providers/policy.rb @@ -11,6 +11,6 @@ def handle_policy(new_resource, ip_version) Chef::Log.debug("[#{ip_version}] setting policy for #{new_resource.chain} to #{new_resource.policy}") - node.set["simple_iptables"][ip_version]["policy"][new_resource.table][new_resource.chain] = new_resource.policy + node.default["simple_iptables"][ip_version]["policy"][new_resource.table][new_resource.chain] = new_resource.policy return true end diff --git a/providers/rule.rb b/providers/rule.rb index ec2622e..9789d67 100644 --- a/providers/rule.rb +++ b/providers/rule.rb @@ -25,36 +25,40 @@ def handle_rule(new_resource, ip_version) else rules = [''] end - if not node["simple_iptables"][ip_version]["chains"][new_resource.table].include?(new_resource.chain) - node.set["simple_iptables"][ip_version]["chains"][new_resource.table] = node["simple_iptables"][ip_version]["chains"][new_resource.table].dup << new_resource.chain unless ["PREROUTING", "INPUT", "FORWARD", "OUTPUT", "POSTROUTING"].include?(new_resource.chain) + + unless node["simple_iptables"][ip_version]["rules"][new_resource.table].include?(new_resource.weight) + node.default["simple_iptables"][ip_version]["rules"][new_resource.table][new_resource.weight] = [] + end + unless node["simple_iptables"][ip_version]["chains"][new_resource.table].include?(new_resource.chain) + unless ["PREROUTING", "INPUT", "FORWARD", "OUTPUT", "POSTROUTING"].include?(new_resource.chain) + node.default["simple_iptables"][ip_version]["chains"][new_resource.table] << new_resource.chain + end unless new_resource.chain == new_resource.direction || new_resource.direction == :none - node.set["simple_iptables"][ip_version]["rules"][new_resource.table] << {:rule => "-A #{new_resource.direction} #{new_resource.chain_condition} --jump #{new_resource.chain}", :weight => new_resource.weight} + node.default["simple_iptables"][ip_version]["rules"][new_resource.table][new_resource.weight] << "-A #{new_resource.direction} #{new_resource.chain_condition} --jump #{new_resource.chain}" end end # Then apply the rules to the node updated = false rules.each do |rule| - new_rule_string = rule_string(new_resource, rule, false) - new_rule = {:rule => new_rule_string, :weight => new_resource.weight} - table_rules = node.set["simple_iptables"][ip_version]["rules"][new_resource.table] + new_rule = rule_string(new_resource, rule, false) + table_rules = node.default["simple_iptables"][ip_version]["rules"][new_resource.table][new_resource.weight] unless table_rules.include?(new_rule) table_rules << new_rule - table_rules.sort! {|a,b| a[:weight] <=> b[:weight]} updated = true - Chef::Log.debug("[#{ip_version}] added rule '#{new_rule_string}'") + Chef::Log.debug("[#{ip_version}] added rule '#{new_rule}'") else - Chef::Log.debug("[#{ip_version}] ignoring duplicate simple_iptables_rule '#{new_rule_string}'") + Chef::Log.debug("[#{ip_version}] ignoring duplicate simple_iptables_rule '#{new_rule}'") end end - return updated + updated end def rule_string(new_resource, rule, include_table) jump = new_resource.jump ? "--jump #{new_resource.jump} " : "" table = include_table ? "--table #{new_resource.table} " : "" - comment = %Q{ -m comment --comment "#{new_resource.comment || new_resource.name}" } + comment = %Q{ -m comment --comment "#{new_resource.comment || new_resource.name}"} rule = "#{table}-A #{new_resource.chain} #{jump}#{rule}#{comment}" rule end diff --git a/recipes/default.rb b/recipes/default.rb index d588be8..87091dc 100644 --- a/recipes/default.rb +++ b/recipes/default.rb @@ -33,16 +33,9 @@ # better way to do this, please let me know! ruby_block "run-iptables-resources-early" do block do - # Before executing the simple_iptables_* resources, reset the - # node attributes to their defaults. This gives "action :delete" - # semantics for free by removing a resource from a recipe. - node.set["simple_iptables"]["ipv4"]["chains"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []} - node.set["simple_iptables"]["ipv4"]["rules"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []} - node.set["simple_iptables"]["ipv4"]["policy"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}} - - node.set["simple_iptables"]["ipv6"]["chains"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []} - node.set["simple_iptables"]["ipv6"]["rules"] = {"filter" => [], "nat" => [], "mangle" => [], "raw" => []} - node.set["simple_iptables"]["ipv6"]["policy"] = {"filter" => {}, "nat" => {}, "mangle" => {}, "raw" => {}} + # Clear old normal-level attributes set by previous versions of the cookbook + node.rm_normal("simple_iptables", "ipv4") + node.rm_normal("simple_iptables", "ipv6") # Then run all the simple_iptables_* resources run_context.resource_collection.each do |resource| if resource.kind_of?(Chef::Resource::SimpleIptablesRule) @@ -54,7 +47,7 @@ end end - Chef::Log.debug("After run-iptables-resources-early data is: #{node['simple_iptables'].inspect}") + Chef::Log.debug("After run-iptables-resources-early data is: #{node['simple_iptables']}") end end @@ -137,4 +130,3 @@ end end end - diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb new file mode 100644 index 0000000..dd9b0b3 --- /dev/null +++ b/spec/spec_helper.rb @@ -0,0 +1,6 @@ +require "chefspec" + +RSpec.configure do |config| + config.cookbook_path = ["..", + "spec/support/cookbooks"] +end diff --git a/spec/support/cookbooks/test_simple_iptables/metadata.rb b/spec/support/cookbooks/test_simple_iptables/metadata.rb new file mode 100644 index 0000000..ff41fe7 --- /dev/null +++ b/spec/support/cookbooks/test_simple_iptables/metadata.rb @@ -0,0 +1,13 @@ +maintainer "Arctic Wolf Networks" +maintainer_email "dev@arcticwolf.com" +license "BSD" +description "Support cookbook for ChefSpec tests for simple_iptables" +version "1.0.0" +name "test_simple_iptables" + +supports "debian", ">= 6.0" +supports "centos", ">= 5.8" +supports "redhat", ">= 5.8" +supports "ubuntu", ">= 10.04" + +depends "simple_iptables" diff --git a/spec/support/cookbooks/test_simple_iptables/recipes/weight.rb b/spec/support/cookbooks/test_simple_iptables/recipes/weight.rb new file mode 100644 index 0000000..e816cb0 --- /dev/null +++ b/spec/support/cookbooks/test_simple_iptables/recipes/weight.rb @@ -0,0 +1,39 @@ +include_recipe "simple_iptables" + +simple_iptables_rule "rule1" do + direction "INPUT" + rule "rule1 content" + jump "ACCEPT" +end + +simple_iptables_rule "rule2" do + direction "INPUT" + rule "rule2 content" + jump "ACCEPT" +end + +simple_iptables_rule "rule3" do + direction "INPUT" + rule "rule3 content" + jump "REJECT" + weight 95 +end + +simple_iptables_rule "rule4" do + direction "INPUT" + rule ["rule4.1 content", "rule4.2 content"] + jump "ACCEPT" +end + +simple_iptables_rule "rule5" do + direction "INPUT" + rule "rule5 content" + jump "ACCEPT" +end + +simple_iptables_rule "rule6" do + direction "INPUT" + rule "rule6 content" + jump "REJECT" + weight 95 +end diff --git a/spec/weight_spec.rb b/spec/weight_spec.rb new file mode 100644 index 0000000..6666929 --- /dev/null +++ b/spec/weight_spec.rb @@ -0,0 +1,69 @@ +require "spec_helper" + +describe "test_simple_iptables::weight" do + let(:chef_run) do + ChefSpec::SoloRunner.new(platform: "ubuntu", version: "14.04", + step_into: ["ruby_block", "simple_iptables_rule"]) + end + + it "generates rules in resource appearance order for every weight" do + expected_rules = +%{# This file generated by Chef. Changes will be overwritten. +*nat +:PREROUTING ACCEPT [0:0] +:INPUT ACCEPT [0:0] +:OUTPUT ACCEPT [0:0] +:POSTROUTING ACCEPT [0:0] +COMMIT +# Completed +# This file generated by Chef. Changes will be overwritten. +*mangle +:PREROUTING ACCEPT [0:0] +:INPUT ACCEPT [0:0] +:FORWARD ACCEPT [0:0] +:OUTPUT ACCEPT [0:0] +:POSTROUTING ACCEPT [0:0] +COMMIT +# Completed +# This file generated by Chef. Changes will be overwritten. +*filter +:INPUT ACCEPT [0:0] +:FORWARD ACCEPT [0:0] +:OUTPUT ACCEPT [0:0] +:rule1 - [0:0] +:rule2 - [0:0] +:rule3 - [0:0] +:rule4 - [0:0] +:rule5 - [0:0] +:rule6 - [0:0] +-A INPUT --jump rule1 +-A rule1 --jump ACCEPT rule1 content -m comment --comment "rule1" +-A INPUT --jump rule2 +-A rule2 --jump ACCEPT rule2 content -m comment --comment "rule2" +-A INPUT --jump rule4 +-A rule4 --jump ACCEPT rule4.1 content -m comment --comment "rule4" +-A rule4 --jump ACCEPT rule4.2 content -m comment --comment "rule4" +-A INPUT --jump rule5 +-A rule5 --jump ACCEPT rule5 content -m comment --comment "rule5" +-A INPUT --jump rule3 +-A rule3 --jump REJECT rule3 content -m comment --comment "rule3" +-A INPUT --jump rule6 +-A rule6 --jump REJECT rule6 content -m comment --comment "rule6" +COMMIT +# Completed +# This file generated by Chef. Changes will be overwritten. +*raw +:PREROUTING ACCEPT [0:0] +:OUTPUT ACCEPT [0:0] +COMMIT +# Completed +} + chef_run.converge(described_recipe) + t = chef_run.template("/etc/iptables-rules") + actual_content = ChefSpec::Renderer.new(chef_run, t).content + File.open("/tmp/foo", "w") { |file| file.write(actual_content) } + File.open("/tmp/foo1", "w") { |file| file.write(expected_rules) } + expect(chef_run).to render_file("/etc/iptables-rules") + .with_content(expected_rules) + end +end diff --git a/templates/default/ip6tables-rules.erb b/templates/default/ip6tables-rules.erb index 7cfbbe3..f874d38 100644 --- a/templates/default/ip6tables-rules.erb +++ b/templates/default/ip6tables-rules.erb @@ -9,8 +9,10 @@ <% node["simple_iptables"]["ipv6"]["chains"]["nat"].each do |chain| -%> :<%= chain %> - [0:0] <% end -%> -<% node["simple_iptables"]["ipv6"]["rules"]["nat"].each do |rule| -%> -<%= rule[:rule] %> +<% node["simple_iptables"]["ipv6"]["rules"]["nat"].sort.each do |weight, rules| -%> +<% rules.each do |rule| -%> +<%= rule %> +<% end -%> <% end -%> COMMIT # Completed @@ -26,8 +28,10 @@ COMMIT <% node["simple_iptables"]["ipv6"]["chains"]["mangle"].each do |chain| -%> :<%= chain %> - [0:0] <% end -%> -<% node["simple_iptables"]["ipv6"]["rules"]["mangle"].each do |rule| -%> -<%= rule[:rule] %> +<% node["simple_iptables"]["ipv6"]["rules"]["mangle"].sort.each do |weight, rules| -%> +<% rules.each do |rule| -%> +<%= rule %> +<% end -%> <% end -%> COMMIT # Completed @@ -41,8 +45,10 @@ COMMIT <% node["simple_iptables"]["ipv6"]["chains"]["filter"].each do |chain| -%> :<%= chain %> - [0:0] <% end -%> -<% node["simple_iptables"]["ipv6"]["rules"]["filter"].each do |rule| -%> -<%= rule[:rule] %> +<% node["simple_iptables"]["ipv6"]["rules"]["filter"].sort.each do |weight, rules| -%> +<% rules.each do |rule| -%> +<%= rule %> +<% end -%> <% end -%> COMMIT # Completed @@ -55,8 +61,10 @@ COMMIT <% node["simple_iptables"]["ipv6"]["chains"]["raw"].each do |chain| -%> :<%= chain %> - [0:0] <% end -%> -<% node["simple_iptables"]["ipv6"]["rules"]["raw"].each do |rule| -%> -<%= rule[:rule] %> +<% node["simple_iptables"]["ipv6"]["rules"]["raw"].sort.each do |weight, rules| -%> +<% rules.each do |rule| -%> +<%= rule %> +<% end -%> <% end -%> COMMIT # Completed diff --git a/templates/default/iptables-rules.erb b/templates/default/iptables-rules.erb index 750a97e..a9c205b 100644 --- a/templates/default/iptables-rules.erb +++ b/templates/default/iptables-rules.erb @@ -8,8 +8,10 @@ <% node["simple_iptables"]["ipv4"]["chains"]["nat"].each do |chain| -%> :<%= chain %> - [0:0] <% end -%> -<% node["simple_iptables"]["ipv4"]["rules"]["nat"].each do |rule| -%> -<%= rule[:rule] %> +<% node["simple_iptables"]["ipv4"]["rules"]["nat"].sort.each do |weight, rules| -%> +<% rules.each do |rule| -%> +<%= rule %> +<% end -%> <% end -%> COMMIT # Completed @@ -25,8 +27,10 @@ COMMIT <% node["simple_iptables"]["ipv4"]["chains"]["mangle"].each do |chain| -%> :<%= chain %> - [0:0] <% end -%> -<% node["simple_iptables"]["ipv4"]["rules"]["mangle"].each do |rule| -%> -<%= rule[:rule] %> +<% node["simple_iptables"]["ipv4"]["rules"]["mangle"].sort.each do |weight, rules| -%> +<% rules.each do |rule| -%> +<%= rule %> +<% end -%> <% end -%> COMMIT # Completed @@ -40,8 +44,10 @@ COMMIT <% node["simple_iptables"]["ipv4"]["chains"]["filter"].each do |chain| -%> :<%= chain %> - [0:0] <% end -%> -<% node["simple_iptables"]["ipv4"]["rules"]["filter"].each do |rule| -%> -<%= rule[:rule] %> +<% node["simple_iptables"]["ipv4"]["rules"]["filter"].sort.each do |weight, rules| -%> +<% rules.each do |rule| -%> +<%= rule %> +<% end -%> <% end -%> COMMIT # Completed @@ -54,8 +60,10 @@ COMMIT <% node["simple_iptables"]["ipv4"]["chains"]["raw"].each do |chain| -%> :<%= chain %> - [0:0] <% end -%> -<% node["simple_iptables"]["ipv4"]["rules"]["raw"].each do |rule| -%> -<%= rule[:rule] %> +<% node["simple_iptables"]["ipv4"]["rules"]["raw"].sort.each do |weight, rules| -%> +<% rules.each do |rule| -%> +<%= rule %> +<% end -%> <% end -%> COMMIT # Completed diff --git a/test/fixtures/smoke/recipes/ipv6_default.rb b/test/fixtures/smoke/recipes/ipv6_default.rb index bdbdbcb..7fbfef5 100644 --- a/test/fixtures/smoke/recipes/ipv6_default.rb +++ b/test/fixtures/smoke/recipes/ipv6_default.rb @@ -1,4 +1,4 @@ -node.set["simple_iptables"]["ip_versions"] = ["ipv4", "ipv6"] +node.default["simple_iptables"]["ip_versions"] = ["ipv4", "ipv6"] include_recipe "simple_iptables::default" diff --git a/test/fixtures/smoke/recipes/ipv6_tables.rb b/test/fixtures/smoke/recipes/ipv6_tables.rb index 2538204..a7c96e4 100644 --- a/test/fixtures/smoke/recipes/ipv6_tables.rb +++ b/test/fixtures/smoke/recipes/ipv6_tables.rb @@ -1,6 +1,6 @@ -node.set["simple_iptables"]["ip_versions"] = ["ipv4", "ipv6"] -node.set["simple_iptables"]["ipv4"]["tables"] = %w(filter mangle) -node.set["simple_iptables"]["ipv6"]["tables"] = %w(filter mangle) +node.default["simple_iptables"]["ip_versions"] = ["ipv4", "ipv6"] +node.default["simple_iptables"]["ipv4"]["tables"] = %w(filter mangle) +node.default["simple_iptables"]["ipv6"]["tables"] = %w(filter mangle) include_recipe "simple_iptables::default" diff --git a/test/fixtures/smoke/recipes/tables.rb b/test/fixtures/smoke/recipes/tables.rb index 5b7e078..56899cc 100644 --- a/test/fixtures/smoke/recipes/tables.rb +++ b/test/fixtures/smoke/recipes/tables.rb @@ -1,4 +1,4 @@ -node.set["simple_iptables"]["ipv4"]["tables"] = %w(filter mangle) +node.default["simple_iptables"]["ipv4"]["tables"] = %w(filter mangle) include_recipe "simple_iptables::default"