Merge pull request #15 from kbarber/ticket/9362-action_property

(#9362) Create action property and perform transformation for accept, dro

README.markdown

@@ -35,13 +35,13 @@ Basic accept ICMP request example:
 
     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):
@@ -72,18 +72,18 @@ If you wish to ensure any reject rules are executed last, try using stages.
 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
@@ -120,6 +120,17 @@ common practice to prefix all rules with numbers to force ordering. For example:
 
 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'.
@@ -174,22 +185,24 @@ By default the setting is 'filter'.
 
 #### 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
 

examples/iptables/test.pp

@@ -1,11 +1,11 @@
 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",
@@ -24,84 +24,84 @@
 }
 
 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':

lib/puppet/provider/firewall.rb

@@ -38,6 +38,7 @@ def method_missing(meth, *args, &block)
     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

lib/puppet/provider/firewall/iptables.rb

@@ -134,8 +134,17 @@ def self.rule_to_hash(line, table, counter)
     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
 
@@ -180,17 +189,31 @@ def delete_args
 
   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
 

lib/puppet/type/firewall.rb

@@ -42,10 +42,9 @@
     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
@@ -108,11 +107,30 @@ def should_to_s(value)
   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
@@ -284,5 +302,9 @@ def should_to_s(value)
     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