Merge pull request #114 from foxxx0/fix-portrange-regression

implement proper sport/dport types, validate port ranges, fix some minor regressions

8 files changed, 348 insertions, 9 deletions

diff --git a/manifests/rule.pp b/manifests/rule.pp
index 458bef6..f239402 100644
--- a/manifests/rule.pp
+++ b/manifests/rule.pp
@@ -59,8 +59,8 @@ define ferm::rule (
   String $comment = $name,
   Optional[Ferm::Actions] $action = undef,
   Optional[Ferm::Policies] $policy = undef,
-  Optional[Variant[Stdlib::Port,Array[Stdlib::Port]]] $dport = undef,
-  Optional[Variant[Stdlib::Port,Array[Stdlib::Port]]] $sport = undef,
+  Optional[Ferm::Port] $dport = undef,
+  Optional[Ferm::Port] $sport = undef,
   Optional[Variant[Array, String[1]]] $saddr = undef,
   Optional[Variant[Array, String[1]]] $daddr = undef,
   Optional[String[1]] $proto_options = undef,
@@ -95,26 +95,60 @@ define ferm::rule (
     String => "proto ${proto}",
   }
 
-  # ferm supports implicit multiport using the "dports" shortcut
+
   if $dport =~ Array {
     $dports = join($dport, ' ')
-    $dport_real = "dports (${dports})"
+    $dport_real = "mod multiport destination-ports (${dports})"
   } elsif $dport =~ Integer {
     $dport_real = "dport ${dport}"
-  } else {
+  } elsif String($dport) =~ /^\d*:\d+$/ {
+    $portrange = split($dport, /:/)
+    $lower = $portrange[0] ? {
+      ''      => 0,
+      default => Integer($portrange[0]),
+    }
+    $upper = Integer($portrange[1])
+    assert_type(Tuple[Stdlib::Port, Stdlib::Port], [$lower, $upper]) |$expected, $actual| {
+      fail("The data type should be \'${expected}\', not \'${actual}\'. The data is [${lower}, ${upper}])}.")
+        ''
+    }
+    if $lower > $upper {
+      fail("Lower port number of the port range is larger than upper. ${lower}:${upper}")
+    }
+    $dport_real = "dport ${lower}:${upper}"
+  } elsif String($dport) == '' {
     $dport_real = ''
+  } else {
+    fail("invalid destination-port: ${dport}")
   }
 
-  # ferm supports implicit multiport using the "sports" shortcut
   if $sport =~ Array {
     $sports = join($sport, ' ')
-    $sport_real = "sports (${sports})"
+    $sport_real = "mod multiport source-ports (${sports})"
   } elsif $sport =~ Integer {
     $sport_real = "sport ${sport}"
-  } else {
+  } elsif String($sport) =~ /^\d*:\d+$/ {
+    $portrange = split($sport, /:/)
+    $lower = $portrange[0] ? {
+      ''      => 0,
+      default => Integer($portrange[0]),
+    }
+    $upper = Integer($portrange[1])
+    assert_type(Tuple[Stdlib::Port, Stdlib::Port], [$lower, $upper]) |$expected, $actual| {
+      fail("The data type should be \'${expected}\', not \'${actual}\'. The data is [${lower}, ${upper}])}.")
+        ''
+    }
+    if $lower > $upper {
+      fail("Lower port number of the port range is larger than upper. ${lower}:${upper}")
+    }
+    $sport_real = "sport ${lower}:${upper}"
+  } elsif String($sport) == '' {
     $sport_real = ''
+  } else {
+    fail("invalid source-port: ${sport}")
   }
 
+
   if $saddr =~ Array {
     assert_type(Array[Stdlib::IP::Address], flatten($saddr)) |$expected, $actual| {
       fail( "The data type should be \'${expected}\', not \'${actual}\'. The data is ${flatten($saddr)}." )
diff --git a/spec/defines/rule_spec.rb b/spec/defines/rule_spec.rb
index f1887b6..f2601c6 100644
--- a/spec/defines/rule_spec.rb
+++ b/spec/defines/rule_spec.rb
@@ -127,12 +127,91 @@ describe 'ferm::rule', type: :define do
         end
 
         it { is_expected.to compile.with_all_deps }
-        it { is_expected.to contain_concat__fragment('INPUT-filter-consul').with_content("mod comment comment 'filter-consul' proto (tcp udp) dports (8301 8302) saddr @ipfilter((127.0.0.1)) ACCEPT;\n") }
+        it { is_expected.to contain_concat__fragment('INPUT-filter-consul').with_content("mod comment comment 'filter-consul' proto (tcp udp) mod multiport destination-ports (8301 8302) saddr @ipfilter((127.0.0.1)) ACCEPT;\n") }
         it { is_expected.to contain_concat__fragment('filter-INPUT-config-include') }
         it { is_expected.to contain_concat__fragment('filter-FORWARD-config-include') }
         it { is_expected.to contain_concat__fragment('filter-OUTPUT-config-include') }
       end
 
+      context 'with a valid destination-port range' do
+        let(:title) { 'filter-portrange' }
+        let :params do
+          {
+            chain: 'INPUT',
+            action: 'ACCEPT',
+            proto: 'tcp',
+            dport: '20000:25000',
+            saddr: '127.0.0.1'
+          }
+        end
+
+        it { is_expected.to compile.with_all_deps }
+        it { is_expected.to contain_concat__fragment('INPUT-filter-portrange').with_content("mod comment comment 'filter-portrange' proto tcp dport 20000:25000 saddr @ipfilter((127.0.0.1)) ACCEPT;\n") }
+        it { is_expected.to contain_concat__fragment('filter-INPUT-config-include') }
+        it { is_expected.to contain_concat__fragment('filter-FORWARD-config-include') }
+        it { is_expected.to contain_concat__fragment('filter-OUTPUT-config-include') }
+      end
+
+      context 'with a malformed source-port range' do
+        let(:title) { 'filter-malformed-portrange' }
+        let :params do
+          {
+            chain: 'INPUT',
+            action: 'ACCEPT',
+            proto: 'tcp',
+            sport: '25000:20000',
+            saddr: '127.0.0.1'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{Lower port number of the port range is larger than upper. 25000:20000}) }
+      end
+
+      context 'with an invalid destination-port range' do
+        let(:title) { 'filter-invalid-portrange' }
+        let :params do
+          {
+            chain: 'INPUT',
+            action: 'ACCEPT',
+            proto: 'tcp',
+            dport: '50000:65538',
+            saddr: '127.0.0.1'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{The data type should be 'Tuple\[Stdlib::Port, Stdlib::Port\]', not 'Tuple\[Integer\[50000, 50000\], Integer\[65538, 65538\]\]'. The data is \[50000, 65538\]}) }
+      end
+
+      context 'with an invalid destination-port string' do
+        let(:title) { 'filter-invalid-portnumber' }
+        let :params do
+          {
+            chain: 'INPUT',
+            action: 'ACCEPT',
+            proto: 'tcp',
+            dport: '65538',
+            saddr: '127.0.0.1'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{parameter 'dport' expects a Ferm::Port .* value, got String}) }
+      end
+
+      context 'with an invalid source-port number' do
+        let(:title) { 'filter-invalid-portnumber' }
+        let :params do
+          {
+            chain: 'INPUT',
+            action: 'ACCEPT',
+            proto: 'tcp',
+            sport: 65_538,
+            saddr: '127.0.0.1'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{parameter 'sport' expects a Ferm::Port .* value, got Integer}) }
+      end
+
       context 'with jumping to custom chains' do
         # create custom chain
         let(:pre_condition) do
diff --git a/spec/type_aliases/actions_spec.rb b/spec/type_aliases/actions_spec.rb
new file mode 100644
index 0000000..9c42e12
--- /dev/null
+++ b/spec/type_aliases/actions_spec.rb
@@ -0,0 +1,46 @@
+# rubocop:disable Style/WordArray, Style/TrailingCommaInLiteral
+require 'spec_helper'
+
+describe 'Ferm::Actions' do
+  describe 'valid values' do
+    [
+      'RETURN',
+      'ACCEPT',
+      'DROP',
+      'REJECT',
+      'NOTRACK',
+      'LOG',
+      'MARK',
+      'DNAT',
+      'SNAT',
+      'MASQUERADE',
+      'REDIRECT',
+      'MYFANCYCUSTOMCHAINNAMEISALSOVALID',
+    ].each do |value|
+      describe value.inspect do
+        it { is_expected.to allow_value(value) }
+      end
+    end
+  end
+
+  describe 'invalid values' do
+    context 'with garbage inputs' do
+      [
+        # :symbol, # this should not match but seems liks String[1] allows it?
+        # nil,     # this should not match but seems liks String[1] allows it?
+        '',
+        true,
+        false,
+        ['meep', 'meep'],
+        65_538,
+        [95_000, 67_000],
+        {},
+        { 'foo' => 'bar' },
+      ].each do |value|
+        describe value.inspect do
+          it { is_expected.not_to allow_value(value) }
+        end
+      end
+    end
+  end
+end
diff --git a/spec/type_aliases/policies_spec.rb b/spec/type_aliases/policies_spec.rb
new file mode 100644
index 0000000..bc45423
--- /dev/null
+++ b/spec/type_aliases/policies_spec.rb
@@ -0,0 +1,39 @@
+# rubocop:disable Style/WordArray, Style/TrailingCommaInLiteral
+require 'spec_helper'
+
+describe 'Ferm::Policies' do
+  describe 'valid values' do
+    [
+      'ACCEPT',
+      'DROP',
+    ].each do |value|
+      describe value.inspect do
+        it { is_expected.to allow_value(value) }
+      end
+    end
+  end
+
+  describe 'invalid values' do
+    context 'with garbage inputs' do
+      [
+        'RETURN',
+        'REJECT',
+        'foobar',
+        :symbol,
+        nil,
+        '',
+        true,
+        false,
+        ['meep', 'meep'],
+        65_538,
+        [95_000, 67_000],
+        {},
+        { 'foo' => 'bar' },
+      ].each do |value|
+        describe value.inspect do
+          it { is_expected.not_to allow_value(value) }
+        end
+      end
+    end
+  end
+end
diff --git a/spec/type_aliases/port_spec.rb b/spec/type_aliases/port_spec.rb
new file mode 100644
index 0000000..e2b0d43
--- /dev/null
+++ b/spec/type_aliases/port_spec.rb
@@ -0,0 +1,43 @@
+# rubocop:disable Style/WordArray, Style/TrailingCommaInLiteral
+require 'spec_helper'
+
+describe 'Ferm::Port' do
+  describe 'valid values' do
+    [
+      17,
+      65_535,
+      '25:30',
+      ':22',
+      [80, 443, 8080, 8443],
+    ].each do |value|
+      describe value.inspect do
+        it { is_expected.to allow_value(value) }
+      end
+    end
+  end
+
+  describe 'invalid values' do
+    context 'with garbage inputs' do
+      [
+        'asdf',
+        true,
+        false,
+        :symbol,
+        ['meep', 'meep'],
+        65_538,
+        [95_000, 67_000],
+        '12345',
+        '20:22:23',
+        '1024:',
+        'ネット',
+        nil,
+        {},
+        { 'foo' => 'bar' },
+      ].each do |value|
+        describe value.inspect do
+          it { is_expected.not_to allow_value(value) }
+        end
+      end
+    end
+  end
+end
diff --git a/spec/type_aliases/protocols_spec.rb b/spec/type_aliases/protocols_spec.rb
new file mode 100644
index 0000000..a067b69
--- /dev/null
+++ b/spec/type_aliases/protocols_spec.rb
@@ -0,0 +1,46 @@
+# rubocop:disable Style/WordArray, Style/TrailingCommaInLiteral
+require 'spec_helper'
+
+describe 'Ferm::Protocols' do
+  describe 'valid values' do
+    [
+      'icmp',
+      'tcp',
+      'udp',
+      'udplite',
+      'icmpv6',
+      'esp',
+      'ah',
+      'sctp',
+      'mh',
+      'all',
+      ['icmp', 'tcp', 'udp'],
+    ].each do |value|
+      describe value.inspect do
+        it { is_expected.to allow_value(value) }
+      end
+    end
+  end
+
+  describe 'invalid values' do
+    context 'with garbage inputs' do
+      [
+        :symbol,
+        nil,
+        'foobar',
+        '',
+        true,
+        false,
+        ['meep', 'meep'],
+        65_538,
+        [95_000, 67_000],
+        {},
+        { 'foo' => 'bar' },
+      ].each do |value|
+        describe value.inspect do
+          it { is_expected.not_to allow_value(value) }
+        end
+      end
+    end
+  end
+end
diff --git a/spec/type_aliases/tables_spec.rb b/spec/type_aliases/tables_spec.rb
new file mode 100644
index 0000000..eb02877
--- /dev/null
+++ b/spec/type_aliases/tables_spec.rb
@@ -0,0 +1,39 @@
+# rubocop:disable Style/WordArray, Style/TrailingCommaInLiteral
+require 'spec_helper'
+
+describe 'Ferm::Tables' do
+  describe 'valid values' do
+    [
+      'raw',
+      'mangle',
+      'nat',
+      'filter',
+    ].each do |value|
+      describe value.inspect do
+        it { is_expected.to allow_value(value) }
+      end
+    end
+  end
+
+  describe 'invalid values' do
+    context 'with garbage inputs' do
+      [
+        :symbol,
+        nil,
+        'foobar',
+        '',
+        true,
+        false,
+        ['meep', 'meep'],
+        65_538,
+        [95_000, 67_000],
+        {},
+        { 'foo' => 'bar' },
+      ].each do |value|
+        describe value.inspect do
+          it { is_expected.not_to allow_value(value) }
+        end
+      end
+    end
+  end
+end
diff --git a/types/port.pp b/types/port.pp
new file mode 100644
index 0000000..dc2b7e1
--- /dev/null
+++ b/types/port.pp
@@ -0,0 +1,13 @@
+# @summary ferm port-spec
+#
+# allowed variants:
+# -----------------
+# + single Integer port
+# + Array of Integers (creates a multiport matcher)
+# + ferm range port-spec (pair of colon-separated integer, assumes 0 if first is omitted)
+
+type Ferm::Port = Variant[
+  Stdlib::Port,
+  Array[Stdlib::Port],
+  Pattern['^\d*:\d+$'],
+]