From 2cf20dd6c09ddb8fdd781bb260b7b73baaa59ea8 Mon Sep 17 00:00:00 2001
From: Alexander Fisher <alex@linfratech.co.uk>
Date: Mon, 17 Jun 2019 12:22:38 +0100
Subject: [PATCH] Replace `source` and `dest` parameters

Split `source` parameter into `source_socket_file`, `source_host` and
`source_port` parameters.  Rename `dest` parameter to `target_`
parameters.

By splitting the parameters, it's much easier to do proper input
validation and build the socket parameters passed to the binary
correctly.  For instance, IP addresses are supposed to be surrounded by
square brackets but hostnames should not.

See https://github.com/Tarsnap/spiped/blob/5c13832aeecdad8a655dadcf5413cc504ad99e49/libcperciva/util/sock.c#L253

Replace erb with epp template.
---
 README.md                                 |  14 +--
 manifests/tunnel.pp                       |  53 ++++++++++-
 manifests/tunnel/client.pp                |  27 ++++--
 manifests/tunnel/server.pp                |  27 ++++--
 metadata.json                             |   8 +-
 spec/acceptance/spiped_spec.rb            |  12 ++-
 spec/defines/spiped_tunnel_client_spec.rb |  13 +--
 spec/defines/spiped_tunnel_server_spec.rb | 111 +++++++++++++++++++++-
 templates/service.epp                     |  21 ++++
 templates/service.erb                     |  14 ---
 10 files changed, 243 insertions(+), 57 deletions(-)
 create mode 100644 templates/service.epp
 delete mode 100644 templates/service.erb

diff --git a/README.md b/README.md
index 5a382e0..c5b3aa7 100644
--- a/README.md
+++ b/README.md
@@ -46,9 +46,10 @@ On `redis-host`, we would define a server tunnel:
 
 ```puppet
 spiped::tunnel::server { 'redis':
-  source => '0.0.0.0:1234',
-  dest   => '/var/run/redis.sock',
-  secret => 'hunter2',
+  source_host        => '0.0.0.0',
+  source_port        => 1234,
+  target_socket_file => '/var/run/redis.sock',
+  secret             => 'hunter2',
 }
 ```
 
@@ -56,9 +57,10 @@ On clients, we would define a client tunnel:
 
 ```puppet
 spiped::tunnel::client { 'redis':
-  source => '/var/run/redis.sock',
-  dest   => 'redis-host:1234',
-  secret => 'hunter2',
+  source_socket_file => '/var/run/redis.sock',
+  target_host        => 'redis-host'
+  target_port        => 1234',
+  secret             => 'hunter2',
 }
 ```
 
diff --git a/manifests/tunnel.pp b/manifests/tunnel.pp
index b90ed1e..ef9720a 100644
--- a/manifests/tunnel.pp
+++ b/manifests/tunnel.pp
@@ -1,12 +1,46 @@
 define spiped::tunnel(
-  $type,
-  $source,
-  $dest,
-  $secret,
+  Enum['client','server'] $type,
+  Variant[Sensitive[String[1]],String[1]] $secret,
+
+  Optional[Stdlib::Unixpath] $source_socket_file = undef,
+  Optional[Stdlib::Host]     $source_host = undef,
+  Optional[Stdlib::Port]     $source_port = undef,
+
+  Optional[Stdlib::Unixpath] $target_socket_file = undef,
+  Optional[Stdlib::Host]     $target_host = undef,
+  Optional[Stdlib::Port]     $target_port = undef,
 ) {
   assert_private()
   include spiped
 
+  if $source_socket_file {
+    if $source_host { fail('`source_host` must not be specified when using `source_socket_file`') }
+    if $source_port { fail('`source_port` must not be specified when using `source_socket_file`') }
+    $source_socket = $source_socket_file
+  } else {
+    unless $source_host and $source_port { fail('either `source_socket_file` or `source_host` and `source_port must be specified') }
+    $source_socket = $source_host ? {
+      # lint:ignore:unquoted_string_in_selector
+      Stdlib::IP::Address => "[${source_host}]:${source_port}",
+      Stdlib::Fqdn        => "${source_host}:${source_port}",
+      # lint:endignore
+    }
+  }
+
+  if $target_socket_file {
+    if $target_host { fail('`target_host` must not be specified when using `target_socket_file`') }
+    if $target_port { fail('`target_port` must not be specified when using `target_socket_file`') }
+    $target_socket = $target_socket_file
+  } else {
+    unless $target_host and $target_port { fail('either `target_socket_file` or `target_host` and `target_port must be specified') }
+    $target_socket = $target_host ? {
+      # lint:ignore:unquoted_string_in_selector
+      Stdlib::IP::Address => "[${target_host}]:${target_port}",
+      Stdlib::Fqdn        => "${target_host}:${target_port}",
+      # lint:endignore
+    }
+  }
+
   $keyfile = "/etc/spiped/${title}.key"
 
   file { $keyfile:
@@ -24,7 +58,16 @@
   }
 
   systemd::unit_file { "spiped-${title}.service":
-    content => template('spiped/service.erb'),
+    content => epp(
+      'spiped/service.epp',
+      {
+        'name'          => $title,
+        'type'          => $type,
+        'keyfile'       => $keyfile,
+        'source_socket' => $source_socket,
+        'target_socket' => $target_socket,
+      },
+    ),
   }
 
   service { "spiped-${title}":
diff --git a/manifests/tunnel/client.pp b/manifests/tunnel/client.pp
index 806f24b..ea46b63 100644
--- a/manifests/tunnel/client.pp
+++ b/manifests/tunnel/client.pp
@@ -1,12 +1,23 @@
 define spiped::tunnel::client(
-  $source,
-  $dest,
-  $secret,
-) {
+  Variant[Sensitive[String[1]],String[1]] $secret,
+
+  Optional[Stdlib::Unixpath] $source_socket_file = undef,
+  Optional[Stdlib::Host]     $source_host = undef,
+  Optional[Stdlib::Port]     $source_port = undef,
+
+  Optional[Stdlib::Unixpath] $target_socket_file = undef,
+  Optional[Stdlib::Host]     $target_host = undef,
+  Optional[Stdlib::Port]     $target_port = undef,
+)
+{
   spiped::tunnel { $title:
-    type   => 'client',
-    source => $source,
-    dest   => $dest,
-    secret => $secret;
+    type               => 'client',
+    source_socket_file => $source_socket_file,
+    source_host        => $source_host,
+    source_port        => $source_port,
+    target_socket_file => $target_socket_file,
+    target_host        => $target_host,
+    target_port        => $target_port,
+    secret             => $secret,
   }
 }
diff --git a/manifests/tunnel/server.pp b/manifests/tunnel/server.pp
index cd0528d..eff99fc 100644
--- a/manifests/tunnel/server.pp
+++ b/manifests/tunnel/server.pp
@@ -1,12 +1,23 @@
 define spiped::tunnel::server(
-  $source,
-  $dest,
-  $secret,
-) {
+  Variant[Sensitive[String[1]],String[1]] $secret,
+
+  Optional[Stdlib::Unixpath] $source_socket_file = undef,
+  Optional[Stdlib::Host]     $source_host = undef,
+  Optional[Stdlib::Port]     $source_port = undef,
+
+  Optional[Stdlib::Unixpath] $target_socket_file = undef,
+  Optional[Stdlib::Host]     $target_host = undef,
+  Optional[Stdlib::Port]     $target_port = undef,
+)
+{
   spiped::tunnel { $title:
-    type   => 'server',
-    source => $source,
-    dest   => $dest,
-    secret => $secret;
+    type               => 'server',
+    source_socket_file => $source_socket_file,
+    source_host        => $source_host,
+    source_port        => $source_port,
+    target_socket_file => $target_socket_file,
+    target_host        => $target_host,
+    target_port        => $target_port,
+    secret             => $secret,
   }
 }
diff --git a/metadata.json b/metadata.json
index a1e088a..8add161 100644
--- a/metadata.json
+++ b/metadata.json
@@ -8,7 +8,7 @@
     "project_page": "https://github.com/chriskuehl/puppet-spiped",
     "issues_url": "https://github.com/chriskuehl/puppet-spiped/issues",
     "dependencies": [
-        { "name": "puppetlabs/stdlib", "version_requirement": ">= 2.3.1 < 7.0.0" },
+        { "name": "puppetlabs/stdlib", "version_requirement": ">= 4.25.0 < 7.0.0" },
         { "name": "camptocamp/systemd", "version_requirement": ">= 1.1.0 < 3.0.0" }
     ],
     "operatingsystem_support": [
@@ -38,5 +38,11 @@
           "18.04"
         ]
       }
+    ],
+    "requirements": [
+      {
+        "name": "puppet",
+        "version_requirement": ">= 5.5.8 < 7.0.0"
+      }
     ]
 }
diff --git a/spec/acceptance/spiped_spec.rb b/spec/acceptance/spiped_spec.rb
index abd7f0a..575d156 100644
--- a/spec/acceptance/spiped_spec.rb
+++ b/spec/acceptance/spiped_spec.rb
@@ -7,8 +7,10 @@
     it 'setups spiped server idempotently' do
       pp = %(
       spiped::tunnel::server { 'redis':
-        source => '[0.0.0.0]:16379',
-        dest   => '127.0.0.1:6379',
+        source_host => '0.0.0.0',
+        source_port => 16379,
+        target_host => '127.0.0.1',
+        target_port => 6379,
         secret => 'hunter2',
       }
       )
@@ -21,8 +23,10 @@
     it 'setups spiped client idempotently' do
       pp = %(
       spiped::tunnel::client { 'redis':
-        source => '[127.0.0.1]:1234',
-        dest   => "#{server_ip}:16379",
+        source_host => '127.0.0.1',
+        source_port => 1234,
+        target_host => #{server_ip},
+        target_port => 16379
         secret => 'hunter2',
       }
       )
diff --git a/spec/defines/spiped_tunnel_client_spec.rb b/spec/defines/spiped_tunnel_client_spec.rb
index b032234..e3f886b 100644
--- a/spec/defines/spiped_tunnel_client_spec.rb
+++ b/spec/defines/spiped_tunnel_client_spec.rb
@@ -4,8 +4,9 @@
   let(:title) { 'redis' }
   let(:params) do
     {
-      source: '/var/run/redis.sock',
-      dest:   'redis-host:1234',
+      source_socket_file: '/var/run/redis.sock',
+      target_host: 'redis-host',
+      target_port: 1234,
       secret: 'hunter2'
     }
   end
@@ -17,9 +18,9 @@
       it { is_expected.to compile }
       it {
         is_expected.to contain_spiped__tunnel('redis').with(
-          type: 'client',
-          source: '/var/run/redis.sock',
-          dest:   'redis-host:1234',
+          source_socket_file: '/var/run/redis.sock',
+          target_host: 'redis-host',
+          target_port: 1234,
           secret: 'hunter2'
         )
       }
@@ -46,7 +47,7 @@
           expect(content).to include('Description=spiped tunnel (redis)')
         end
         it 'ExecStart' do
-          expect(content).to include('ExecStart=/usr/bin/spiped -e -D -g -F -k \'/etc/spiped/redis.key\' -p /dev/null -s \'/var/run/redis.sock\' -t \'redis-host:1234\'')
+          expect(content).to include('ExecStart=/usr/bin/spiped -e -D -g -F -k \'/etc/spiped/redis.key\' -s \'/var/run/redis.sock\' -t \'redis-host:1234\'')
         end
       end
 
diff --git a/spec/defines/spiped_tunnel_server_spec.rb b/spec/defines/spiped_tunnel_server_spec.rb
index d39f9e8..635c39e 100644
--- a/spec/defines/spiped_tunnel_server_spec.rb
+++ b/spec/defines/spiped_tunnel_server_spec.rb
@@ -4,8 +4,9 @@
   let(:title) { 'redis' }
   let(:params) do
     {
-      source: '0.0.0.0:1234',
-      dest:   '/var/run/redis.sock',
+      source_host: '0.0.0.0',
+      source_port: 1234,
+      target_socket_file: '/var/run/redis.sock',
       secret: 'hunter2'
     }
   end
@@ -18,8 +19,9 @@
       it {
         is_expected.to contain_spiped__tunnel('redis').with(
           type: 'server',
-          source: '0.0.0.0:1234',
-          dest:   '/var/run/redis.sock',
+          source_host: '0.0.0.0',
+          source_port: 1234,
+          target_socket_file: '/var/run/redis.sock',
           secret: 'hunter2'
         )
       }
@@ -46,7 +48,7 @@
           expect(content).to include('Description=spiped tunnel (redis)')
         end
         it 'ExecStart' do
-          expect(content).to include('ExecStart=/usr/bin/spiped -d -g -F -k \'/etc/spiped/redis.key\' -p /dev/null -s \'0.0.0.0:1234\' -t \'/var/run/redis.sock\'')
+          expect(content).to include('ExecStart=/usr/bin/spiped -d -g -F -k \'/etc/spiped/redis.key\' -s \'[0.0.0.0]:1234\' -t \'/var/run/redis.sock\'')
         end
       end
 
@@ -58,4 +60,103 @@
       }
     end
   end
+
+  describe 'conflicting parameters' do
+    describe 'source parameters' do
+      context 'source_socket_file and source_host' do
+        let(:params) do
+          {
+            source_socket_file: '/path/to/socket',
+            source_host: '0.0.0.0',
+            target_socket_file: '/var/run/redis.sock',
+            secret: 'hunter2'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{`source_host` must not be specified when using `source_socket_file`}) }
+      end
+      context 'source_socket_file and source_port' do
+        let(:params) do
+          {
+            source_socket_file: '/path/to/socket',
+            source_port: 1234,
+            target_socket_file: '/var/run/redis.sock',
+            secret: 'hunter2'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{`source_port` must not be specified when using `source_socket_file`}) }
+      end
+      context 'missing source_host' do
+        let(:params) do
+          {
+            source_port: 1234,
+            target_socket_file: '/var/run/redis.sock',
+            secret: 'hunter2'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{either `source_socket_file` or `source_host` and `source_port must be specified}) }
+      end
+      context 'missing source_port' do
+        let(:params) do
+          {
+            source_host: '0.0.0.0',
+            target_socket_file: '/var/run/redis.sock',
+            secret: 'hunter2'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{either `source_socket_file` or `source_host` and `source_port must be specified}) }
+      end
+    end
+    describe 'target parameters' do
+      context 'target_socket_file and target_host' do
+        let(:params) do
+          {
+            source_socket_file: '/path/to/socket',
+            target_socket_file: '/var/run/redis.sock',
+            target_host: 'redis-host',
+            secret: 'hunter2'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{`target_host` must not be specified when using `target_socket_file`}) }
+      end
+      context 'target_socket_file and target_port' do
+        let(:params) do
+          {
+            source_socket_file: '/path/to/socket',
+            target_socket_file: '/var/run/redis.sock',
+            target_port: 1234,
+            secret: 'hunter2'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{`target_port` must not be specified when using `target_socket_file`}) }
+      end
+      context 'missing target_host' do
+        let(:params) do
+          {
+            source_socket_file: '/path/to/socket',
+            target_port: 1234,
+            secret: 'hunter2'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{either `target_socket_file` or `target_host` and `target_port must be specified}) }
+      end
+      context 'missing target_port' do
+        let(:params) do
+          {
+            source_socket_file: '/path/to/socket',
+            target_host: 'redis-host',
+            secret: 'hunter2'
+          }
+        end
+
+        it { is_expected.to compile.and_raise_error(%r{either `target_socket_file` or `target_host` and `target_port must be specified}) }
+      end
+    end
+  end
 end
diff --git a/templates/service.epp b/templates/service.epp
new file mode 100644
index 0000000..25867d9
--- /dev/null
+++ b/templates/service.epp
@@ -0,0 +1,21 @@
+<%- | String                  $name,
+      Enum['client','server'] $type,
+      Stdlib::Unixpath        $keyfile,
+      String                  $source_socket,
+      String                  $target_socket,
+| -%>
+# THIS FILE IS MANAGED BY PUPPET
+[Unit]
+Description=spiped tunnel (<%= $name %>)
+
+[Service]
+User=root
+<% if $type == 'client' { -%>
+ExecStart=/usr/bin/spiped -e -D -g -F -k '<%= $keyfile %>' -s '<%= $source_socket %>' -t '<%= $target_socket %>'
+<% } else { -%>
+ExecStart=/usr/bin/spiped -d -g -F -k '<%= $keyfile %>' -s '<%= $source_socket %>' -t '<%= $target_socket %>'
+<% } -%>
+Restart=always
+
+[Install]
+WantedBy=multi-user.target
diff --git a/templates/service.erb b/templates/service.erb
deleted file mode 100644
index 58af29c..0000000
--- a/templates/service.erb
+++ /dev/null
@@ -1,14 +0,0 @@
-[Unit]
-Description=spiped tunnel (<%= @name %>)
-
-[Service]
-User=root
-<% if @type == 'client' %>
-ExecStart=/usr/bin/spiped -e -D -g -F -k '<%= @keyfile %>' -p /dev/null -s '<%= @source %>' -t '<%= @dest %>'
-<% else %>
-ExecStart=/usr/bin/spiped -d -g -F -k '<%= @keyfile %>' -p /dev/null -s '<%= @source %>' -t '<%= @dest %>'
-<% end %>
-Restart=always
-
-[Install]
-WantedBy=multi-user.target