This commit introduces the new 'action' parameter which is designed to designate
the action to take when a match succeeds. This is a cross-platform parameter and
for the values 'accept','drop','reject' it will take the place of the existing
jump parameter.
The jump parameter is deemed as an iptables specific parameter so by splitting
out this parameter for common actions it allows us to extend the firewall
resource to include other providers much more easily in the future. By having
such a common parameter we will be able to compare resources between boxes that
may have different firewall implementations.
The new behaviour is to force the usage for action parameter, and using
'accept', 'drop' or 'reject' for jump will now no longer work.
Also - the default of 'accept' for jump has been removed which means you MUST
specify an action if you want your rule to do something. Without an action the
rule will match, but do nothing (so only useful for keeping counters generally).
To aid in the testing of this new property I've added new ways to test converting
iptables rules to hashes and hashes to general_args. This should simplify the
testing of new bugs as well.
firewall { "000 accept all icmp requests":
proto => "icmp",
- jump => "ACCEPT",
+ action => "accept",
}
Deny all:
- firewall { "999 deny all other requests":
- jump => "DENY",
+ firewall { "999 drop all other requests":
+ action => "drop",
}
Source NAT example (perfect for a virtualization host):
The following example shows the creation of a class which is where your
last rules should run, this however should belong in a puppet module.
- class my_fw::deny {
- iptables { "999 deny all":
- jump => "DENY"
+ class my_fw::drop {
+ iptables { "999 drop all":
+ action => "drop"
}
}
stage { pre: before => Stage[main] }
stage { post: require => Stage[main] }
- class { "my_fw::deny": stage => "post" }
+ class { "my_fw::drop": stage => "post" }
-By placing the 'my_fw::deny' class in the post stage it will always be inserted
+By placing the 'my_fw::drop' class in the post stage it will always be inserted
last thereby avoiding locking you out before the accept rules are inserted.
### Supported firewalls
This will occur very early.
+#### action
+
+This is the action to perform on a match. Can be one of:
+
+* accept - the packet is accepted
+* reject - the packet is rejected with a suitable ICMP response
+* drop - the packet is dropped
+
+If you specify no value it will simply match the rule but perform no
+action unless you provide a provider specific parameter (such as 'jump').
+
#### proto
Protocol to filter. By default this is 'tcp'.
#### jump
-Action to perform when filter is matched. Can be one of:
+Action to perform when filter is matched for iptables. Can be one of:
-* ACCEPT
-* DROP
* QUEUE
* RETURN
-* REJECT
* DNAT
* SNAT
* LOG
* MASQUERADE
* REDIRECT
-Or this can be a user defined chain.
+But any valid chain name is allowed.
+
+For the values ACCEPT, DROP and REJECT you must use the generic
+'action' parameter. This is to enfore the use of generic parameters where
+possible for maximum cross-platform modelling.
-The default value is 'ACCEPT'.
+If you set both 'accept' and 'jump' parameters, you will get an error as
+only one of the options should be set.
### Interface Matching Properties
firewall { '000 allow foo':
dport => [7061, 7062],
- jump => "ACCEPT",
+ action => accept,
proto => "tcp",
}
firewall { '001 allow boo':
- jump => "ACCEPT",
+ action => accept,
iniface => "eth0",
sport => "123",
dport => "123",
}
firewall { '999 bar':
+ action => accept,
dport => "1233",
proto => "tcp",
- jump => "DROP",
}
firewall { '002 foo':
+ action => drop,
dport => "1233",
proto => "tcp",
- jump => "DROP",
}
firewall { '010 icmp':
+ action => accept,
proto => "icmp",
icmp => "echo-reply",
- jump => "ACCEPT",
}
firewall { '010 INPUT allow loopback':
+ action => accept,
iniface => 'lo',
chain => 'INPUT',
- jump => 'ACCEPT'
}
firewall { '005 INPUT disregard DHCP':
+ action => drop,
dport => ['bootpc', 'bootps'],
- jump => 'DROP',
proto => 'udp'
}
firewall { '006 INPUT disregard netbios':
+ action => drop,
proto => 'udp',
dport => ['netbios-ns', 'netbios-dgm', 'netbios-ssn'],
- jump => 'DROP'
}
firewall { '006 Disregard CIFS':
+ action => drop,
dport => 'microsoft-ds',
- jump => 'DROP',
proto => 'tcp'
}
firewall { '050 INPUT drop invalid':
+ action => drop,
state => 'INVALID',
- jump => 'DROP'
}
firewall { '051 INPUT allow related and established':
+ action => accept,
state => ['RELATED', 'ESTABLISHED'],
- jump => 'ACCEPT'
}
firewall { '053 INPUT allow ICMP':
+ action => accept,
icmp => '8',
proto => 'icmp',
- jump => 'ACCEPT'
}
firewall { '055 INPUT allow DNS':
+ action => accept,
proto => 'udp',
- jump => 'ACCEPT',
sport => 'domain'
}
firewall { '999 FORWARD drop':
+ action => drop,
chain => 'FORWARD',
- jump => 'DROP'
}
firewall { '001 OUTPUT allow loopback':
+ action => accept,
chain => 'OUTPUT',
outiface => 'lo',
- jump => 'ACCEPT'
}
firewall { '100 OUTPUT drop invalid':
+ action => drop,
chain => 'OUTPUT',
state => 'INVALID',
- jump => 'DROP'
}
resources { 'firewall':
dynamic_methods = self.class.instance_variable_get('@resource_map').keys
dynamic_methods << :chain
dynamic_methods << :table
+ dynamic_methods << :action
if dynamic_methods.include?(meth.to_sym) then
if @property_hash[meth.to_sym] then
hash[:ensure] = :present
# Munge some vars here ...
- # proto should equal 'all' if undefined
+
+ # Proto should equal 'all' if undefined
hash[:proto] = "all" if !hash.include?(:proto)
+
+ # If the jump parameter is set to one of: ACCEPT, REJECT or DROP then
+ # we should set the action parameter instead.
+ if ['ACCEPT','REJECT','DROP'].include?(hash[:jump]) then
+ hash[:action] = hash[:jump].downcase
+ hash.delete(:jump)
+ end
+
hash
end
def general_args
debug "Current resource: %s" % resource.class
+
args = []
- self.class.instance_variable_get('@resource_list').each do |res|
- if(resource.value(res))
- args << self.class.instance_variable_get('@resource_map')[res].split(' ')
- if resource[res].is_a?(Array)
- args << resource[res].join(',')
- else
- args << resource[res]
- end
+ resource_list = self.class.instance_variable_get('@resource_list')
+ resource_map = self.class.instance_variable_get('@resource_map')
+
+ resource_list.each do |res|
+ resource_value = nil
+ if (resource[res]) then
+ resource_value = resource[res]
+ elsif res == :jump and resource[:action] then
+ # In this case, we are substituting jump for action
+ resource_value = resource[:action].to_s.upcase
+ else
+ next
+ end
+
+ args << resource_map[res].split(' ')
+
+ if resource_value.is_a?(Array)
+ args << resource_value.join(',')
+ else
+ args << resource_value
end
end
+
args
end
newvalues(/^\d+[a-zA-Z0-9\s\-_]+$/)
end
- newparam(:action) do
+ newproperty(:action) do
desc "Action to perform on this rule."
newvalues(:accept, :reject, :drop)
- defaultto :accept
end
# Generic matching properties
end
newproperty(:jump, :required_features => :iptables) do
- desc "The value for the iptables --jump parameter. Normal values are:
- 'ACCEPT', 'DROP', 'QUEUE', 'RETURN', REJECT', 'DNAT', 'SNAT', 'LOG',
- 'MASQUERADE', 'REDIRECT'. But any valid chain name is allowed."
- newvalues(/^[a-zA-Z0-9\-_]+$/)
- defaultto "ACCEPT"
+ desc <<EOS
+The value for the iptables --jump parameter. Normal values are:
+
+* QUEUE
+* RETURN
+* DNAT
+* SNAT
+* LOG
+* MASQUERADE
+* REDIRECT.
+
+But any valid chain name is allowed.
+
+For the values ACCEPT, DROP and REJECT you must use the generic
+'action' parameter. This is to enfore the use of generic parameters where
+possible for maximum cross-platform modelling.
+
+If you set both 'accept' and 'jump' parameters, the jump parameter will take
+precedence.
+EOS
+ validate do |value|
+ raise ArgumentError, "Jump destination must consist of alphanumeric characters, an underscore or a hyphen." unless value =~ /^[a-zA-Z0-9\-_]+$/
+ raise ArgumentError, "Jump destination should not be one of ACCEPT, REJECT or DENY. Use the action property instead." if ["accept","reject","drop"].include?(value.downcase)
+ end
end
# Interface specific matching properties
if value(:burst) && ! value(:limit)
self.fail "burst makes no sense without limit"
end
+
+ if value(:action) && value(:jump)
+ self.fail "Only one of the parameters 'action' and 'jump' can be set"
+ end
end
end
--- /dev/null
+# These hashes allow us to iterate across a series of test data
+# creating rspec examples for each parameter to ensure the input :line
+# extrapolates to the desired value for the parameter in question. And
+# vice-versa
+
+# This hash is for testing a line conversion to a hash of parameters
+# which will be used to create a resource.
+ARGS_TO_HASH = {
+ 'long_rule_1' => {
+ :line => '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061,7062 -m comment --comment "000 allow foo" -j ACCEPT',
+ :table => 'filter',
+ :compare_all => true,
+ :params => {
+ :action => "accept",
+ :chain => "INPUT",
+ :destination => "1.1.1.1",
+ :dport => ["7061","7062"],
+ :ensure => :present,
+ :line => '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061,7062 -m comment --comment "000 allow foo" -j ACCEPT',
+ :name => "000 allow foo",
+ :proto => "tcp",
+ :provider => "iptables",
+ :source => "1.1.1.1",
+ :sport => ["7061","7062"],
+ :table => "filter",
+ },
+ },
+ 'action_drop_1' => {
+ :line => '-A INPUT -m comment --comment "000 allow foo" -j DROP',
+ :table => 'filter',
+ :params => {
+ :jump => nil,
+ :action => "drop",
+ },
+ },
+ 'action_reject_1' => {
+ :line => '-A INPUT -m comment --comment "000 allow foo" -j REJECT',
+ :table => 'filter',
+ :params => {
+ :jump => nil,
+ :action => "reject",
+ },
+ },
+ 'action_nil_1' => {
+ :line => '-A INPUT -m comment --comment "000 allow foo"',
+ :table => 'filter',
+ :params => {
+ :jump => nil,
+ :action => nil,
+ },
+ },
+ 'jump_custom_chain_1' => {
+ :line => '-A INPUT -m comment --comment "000 allow foo" -j custom_chain',
+ :table => 'filter',
+ :params => {
+ :jump => "custom_chain",
+ :action => nil,
+ },
+ },
+}
+
+# This hash is for testing converting a hash to an argument line.
+HASH_TO_ARGS = {
+ 'long_rule_1' => {
+ :params => {
+ :action => "accept",
+ :chain => "INPUT",
+ :destination => "1.1.1.1",
+ :dport => ["7061","7062"],
+ :ensure => :present,
+ :name => "000 allow foo",
+ :proto => "tcp",
+ :source => "1.1.1.1",
+ :sport => ["7061","7062"],
+ :table => "filter",
+ },
+ :args => ["-t", :filter, "-s", "1.1.1.1", "-d", "1.1.1.1", "-p", :tcp, "-m", "multiport", "--sports", "7061,7062", "-m", "multiport", "--dports", "7061,7062", "-m", "comment", "--comment", "000 allow foo", "-j", "ACCEPT"],
+ },
+ 'long_rule_2' => {
+ :params => {
+ :chain => "INPUT",
+ :destination => "2.10.13.3/24",
+ :dport => ["7061"],
+ :ensure => :present,
+ :jump => "my_custom_chain",
+ :name => "700 allow bar",
+ :proto => "udp",
+ :source => "1.1.1.1",
+ :sport => ["7061","7062"],
+ :table => "filter",
+ },
+ :args => ["-t", :filter, "-s", "1.1.1.1", "-d", "2.10.13.3/24", "-p", :udp, "-m", "multiport", "--sports", "7061,7062", "-m", "multiport", "--dports", "7061", "-m", "comment", "--comment", "700 allow bar", "-j", "my_custom_chain"],
+ },
+ 'no_action' => {
+ :params => {
+ :name => "100 no action",
+ :table => "filter",
+ },
+ :args => ["-t", :filter, "-p", :tcp, "-m", "comment", "--comment",
+ "100 no action"],
+ }
+}
+#!/usr/bin/env rspec
+
require 'spec_helper'
+require 'puppet/provider/confine/exists'
describe 'iptables provider detection' do
- before :each do
- require 'puppet/provider/confine/exists'
- @exists = Puppet::Provider::Confine::Exists
+ let(:exists) {
+ Puppet::Provider::Confine::Exists
+ }
+ before :each do
# Reset the default provider
Puppet::Type.type(:firewall).defaultprovider = nil
end
it "should default to iptables provider if /sbin/iptables[-save] exists" do
# Stub lookup for /sbin/iptables & /sbin/iptables-save
- @exists.any_instance.stubs(:which).with("/sbin/iptables").
+ exists.any_instance.stubs(:which).with("/sbin/iptables").
returns "/sbin/iptables"
- @exists.any_instance.stubs(:which).with("/sbin/iptables-save").
+ exists.any_instance.stubs(:which).with("/sbin/iptables-save").
returns "/sbin/iptables-save"
# Every other command should return false so we don't pick up any
# other providers
- @exists.any_instance.stubs(:which).with() { |value|
+ exists.any_instance.stubs(:which).with() { |value|
! ["/sbin/iptables","/sbin/iptables-save"].include?(value)
}.returns false
it "should raise a default provider error when there are no commands" do
# Stub all commands lookups so they return nothing
- @exists.any_instance.stubs(:which).returns false
+ exists.any_instance.stubs(:which).returns false
# Instantiate a resource instance and make sure it raises an exception
lambda { resource = Puppet::Type.type(:firewall).new({
end
describe 'iptables provider' do
- before :each do
- @provider = Puppet::Type.type(:firewall).provider(:iptables)
- Puppet::Type::Firewall.stubs(:defaultprovider).returns @provider
- @provider.stubs(:command).with(:iptables_save).returns "/sbin/iptables-save"
- @resource = Puppet::Type.type(:firewall).new({
+ let(:provider) { Puppet::Type.type(:firewall).provider(:iptables) }
+ let(:resource) {
+ Puppet::Type.type(:firewall).new({
:name => '000 test foo',
- :chain => 'INPUT',
- :jump => 'ACCEPT'
+ :action => 'accept',
})
+ }
+
+ before :each do
+ Puppet::Type::Firewall.stubs(:defaultprovider).returns provider
+ provider.stubs(:command).with(:iptables_save).returns "/sbin/iptables-save"
end
it 'should be able to get a list of existing rules' do
# Pretend to return nil from iptables
- @provider.expects(:execute).with(['/sbin/iptables-save']).returns("")
+ provider.expects(:execute).with(['/sbin/iptables-save']).returns("")
- @provider.instances.each do |rule|
- rule.should be_instance_of(@provider)
- rule.properties[:provider].to_s.should == @provider.name.to_s
+ provider.instances.each do |rule|
+ rule.should be_instance_of(provider)
+ rule.properties[:provider].to_s.should == provider.name.to_s
end
end
- describe 'when converting rules to resources' do
- before :each do
- @resource = @provider.rule_to_hash('-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061,7062 -m comment --comment "000 allow foo" -j ACCEPT', 'filter', 0)
- end
+ # Load in ruby hash for test fixtures.
+ load 'spec/fixtures/iptables/conversion_hash.rb'
- [:name, :table, :chain, :proto, :jump, :source, :destination].each do |param|
- it "#{param} should be a string" do
- @resource[param].class.should == String
+ describe 'when converting rules to resources' do
+ ARGS_TO_HASH.each do |test_name,data|
+ describe "for test data '#{test_name}'" do
+ let(:resource) { provider.rule_to_hash(data[:line], data[:table], 0) }
+
+ # If this option is enabled, make sure the parameters exactly match
+ if data[:compare_all] then
+ it "the parameter hash keys should be the same as returned by rules_to_hash" do
+ resource.keys.sort.should == data[:params].keys.sort
+ end
+ end
+
+ # Iterate across each parameter, creating an example for comparison
+ data[:params].each do |param_name, param_value|
+ it "the parameter '#{param_name.to_s}' should match #{param_value.inspect}" do
+ resource[param_name].should == data[:params][param_name]
+ end
+ end
end
end
+ end
+
+ describe 'when working out general_args' do
+ HASH_TO_ARGS.each do |test_name,data|
+ describe "for test data '#{test_name}'" do
+ let(:resource) { Puppet::Type.type(:firewall).new(data[:params]) }
+ let(:provider) { Puppet::Type.type(:firewall).provider(:iptables) }
+ let(:instance) { provider.new(resource) }
- [:dport, :sport].each do |param|
- it "#{param} should be an array" do
- @resource[param].class.should == Array
+ it 'general_args should be valid' do
+ instance.general_args.flatten.should == data[:args]
+ end
end
end
end
describe 'when converting rules without comments to resources' do
- before :each do
- @rule = '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT'
- @resource = @provider.rule_to_hash(@rule, 'filter', 0)
- @instance = @provider.new(@resource)
- end
+ let(:sample_rule) {
+ '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT'
+ }
+ let(:resource) { provider.rule_to_hash(sample_rule, 'filter', 0) }
+ let(:instance) { provider.new(resource) }
it 'rule name contains a MD5 sum of the line' do
- @resource[:name].should == "9999 #{Digest::MD5.hexdigest(@resource[:line])}"
+ resource[:name].should == "9999 #{Digest::MD5.hexdigest(resource[:line])}"
end
end
describe 'when creating resources' do
+ let(:instance) { provider.new(resource) }
+
before :each do
- @instance = @provider.new(@resource)
- @provider.expects(:execute).with(['/sbin/iptables-save']).returns("")
+ provider.expects(:execute).with(['/sbin/iptables-save']).returns("")
end
it 'insert_args should be an array' do
- @instance.insert_args.class.should == Array
+ instance.insert_args.class.should == Array
end
end
describe 'when modifying resources' do
+ let(:instance) { provider.new(resource) }
+
before :each do
- @instance = @provider.new(@resource)
- @provider.expects(:execute).with(['/sbin/iptables-save']).returns ""
+ provider.expects(:execute).with(['/sbin/iptables-save']).returns ""
end
it 'update_args should be an array' do
- @instance.update_args.class.should == Array
+ instance.update_args.class.should == Array
end
end
describe 'when deleting resources' do
- before :each do
- @rule = '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT'
- @resource = @provider.rule_to_hash(@rule, 'filter', 0)
- @instance = @provider.new(@resource)
- end
+ let(:sample_rule) {
+ '-A INPUT -s 1.1.1.1 -d 1.1.1.1 -p tcp -m multiport --dports 7061,7062 -m multiport --sports 7061, 7062 -j ACCEPT'
+ }
+ let(:resource) { provider.rule_to_hash(sample_rule, 'filter', 0) }
+ let(:instance) { provider.new(resource) }
it 'resource[:line] looks like the original rule' do
- @resource[:line] == @rule
+ resource[:line] == sample_rule
end
it 'delete_args is an array' do
- @instance.delete_args.class.should == Array
+ instance.delete_args.class.should == Array
end
it 'delete_args is the same as the rule string when joined' do
- @instance.delete_args.join(' ').should == @rule.gsub(/\-A/, '-D')
+ instance.delete_args.join(' ').should == sample_rule.gsub(/\-A/, '-D')
end
end
end
+#!/usr/bin/env rspec
+
require 'spec_helper'
firewall = Puppet::Type.type(:firewall)
@provider.stubs(:name).returns(:iptables)
Puppet::Type::Firewall.stubs(:defaultprovider).returns @provider
- @resource = @class.new({
- :name => '000 test foo',
- :chain => 'INPUT',
- :jump => 'ACCEPT'
- })
+ @resource = @class.new({:name => '000 test foo'})
end
- # Example on how to test and have the full validation kick in
- #lambda { @class.new(:name => "001-test", :chain => chain) }.should_not raise_error
-
it 'should have :name be its namevar' do
@class.key_attributes.should == [:name]
end
end
end
+ describe ':action' do
+ it "should have no default" do
+ res = @class.new(:name => "000 test")
+ res.parameters[:action].should == nil
+ end
+
+ [:accept, :drop, :reject].each do |action|
+ it "should accept value #{action}" do
+ @resource[:action] = action
+ @resource[:action].should == action
+ end
+ end
+
+ it 'should fail when value is not recognized' do
+ lambda { @resource[:action] = 'not valid' }.should raise_error(Puppet::Error)
+ end
+ end
+
describe ':chain' do
[:INPUT, :FORWARD, :OUTPUT, :PREROUTING, :POSTROUTING].each do |chain|
it "should accept chain value #{chain}" do
end
describe ':jump' do
- [:ACCEPT, :DROP, :QUEUE, :RETURN, :REJECT, :DNAT, :SNAT, :LOG, :MASQUERADE, :REDIRECT].each do |jump|
+ it "should have no default" do
+ res = @class.new(:name => "000 test")
+ res.parameters[:jump].should == nil
+ end
+
+ ['QUEUE', 'RETURN', 'DNAT', 'SNAT', 'LOG', 'MASQUERADE', 'REDIRECT'].each do |jump|
it "should accept jump value #{jump}" do
@resource[:jump] = jump
@resource[:jump].should == jump
end
end
+ ['ACCEPT', 'DROP', 'REJECT'].each do |jump|
+ it "should now fail when value #{jump}" do
+ lambda { @resource[:jump] = jump }.should raise_error(Puppet::Error)
+ end
+ end
+
it "should fail when jump value is not recognized" do
- lambda { @resource[:proto] = 'jump' }.should raise_error(Puppet::Error)
+ lambda { @resource[:jump] = '%^&*' }.should raise_error(Puppet::Error)
end
end
lambda { @resource[:burst] = 'foo' }.should raise_error(Puppet::Error)
end
end
+
+ describe ':action and :jump' do
+ it 'should allow only 1 to be set at a time' do
+ expect {
+ @class.new(
+ :name => "001-test",
+ :action => "accept",
+ :jump => "custom_chain"
+ )
+ }.should raise_error(Puppet::Error, /^Only one of the parameters 'action' and 'jump' can be set$/)
+ end
+ end
end
Code Review / puppet-modules / puppetlabs-firewall.git / commitdiff