Pull listener/backend configuration out into separate arguments, sinc…

…e they're required; only manage service if they're set. Fix shell escaping and quoting in init scripts.
EvanKrall · Jun 18, 2015 · 37257d1 · 37257d1
1 parent 957b87f
commit 37257d1
Show file tree

Hide file tree

Showing 9 changed files with 189 additions and 97 deletions.
diff --git a/manifests/init.pp b/manifests/init.pp
@@ -7,6 +7,13 @@
 # [*version*]
 #   Specify version of vault binary to download.
 #
+# [*backend*]
+#   The backend configuration dictionary. If unset or undef, the vault service
+#   will not be run (vault will only be installed.)
+#
+# [*listener*]
+#   The listener configuration dictionary. Must be set if backend is set.
+#
 # [*config_hash*]
 #   Use this to populate the JSON config file for vault.
 #
@@ -21,14 +28,20 @@
 #   Only valid when the install_method == package. Defaults to `latest`.
 #
 # [*extra_options*]
-#   Extra arguments to be passed to the vault agent
+#   Extra arguments to be passed to the vault agent, as an array of strings.
+#   Will be shellquoted before being written to init scripts.
 #
 # [*init_style*]
-#   What style of init system your system uses.
+#   What style of init system your system uses. Valid options are 'upstart',
+#   'systemd', 'sysv', 'debian', 'sles', 'launchd'. If unset, this will be
+#   auto-detected.
 #
 # [*purge_config_dir*]
-#   Purge config files no longer generated by Puppet
+#   Purge config files no longer generated by Puppet.
+#
 class vault (
+  $backend           = undef,
+  $listener          = undef,
   $manage_user       = false,
   $user              = 'root',
   $manage_group      = false,
@@ -43,36 +56,48 @@
   $package_name      = $vault::params::package_name,
   $package_ensure    = $vault::params::package_ensure,
   $config_dir        = '/etc/vault',
-  $extra_options     = '',
+  $extra_options     = [],
   $config_hash       = {},
   $config_defaults   = {},
-  $service_enable    = false,
+  $service_enable    = undef,
   $service_ensure    = 'stopped',
   $manage_service    = false,
   $init_style        = $vault::params::init_style,
 ) inherits vault::params {
 
-  validate_bool($purge_config_dir)
-  validate_bool($manage_user)
-  validate_bool($manage_service)
-  validate_hash($config_hash)
-  validate_hash($config_defaults)
+  # with no arguments, we simply install vault.
+  anchor {'vault_first': } ->
+  class { 'vault::install': } ->
+  anchor {'vault_last': }
+
+  # but if $backend is set, assume we want to run the service.
+  if $backend {
+    validate_bool($purge_config_dir)
+    validate_bool($manage_user)
+    validate_bool($manage_service)
+    validate_hash($backend)
+    validate_hash($listener)
+    validate_hash($config_hash)
+    validate_hash($config_defaults)
 
-  $config_hash_real = merge($config_defaults, $config_hash)
-  validate_hash($config_hash_real)
+    $config_hash_real = merge(
+      $config_defaults,
+      { backend => $backend },
+      { listener => $listener },
+      $config_hash
+    )
+    validate_hash($config_hash_real)
 
-  if $config_hash_real['data_dir'] {
-    $data_dir = $config_hash_real['data_dir']
-  }
+    if $config_hash_real['data_dir'] {
+      $data_dir = $config_hash_real['data_dir']
+    }
 
-  anchor {'vault_first': }
-  ->
-  class { 'vault::install': } ->
-  class { 'vault::config':
-    config_hash => $config_hash_real,
-    purge       => $purge_config_dir,
-  } ~>
-  class { 'vault::service': }
-  ->
-  anchor {'vault_last': }
+    Class['vault::install'] ->
+    class { 'vault::config':
+      config_hash => $config_hash_real,
+      purge       => $purge_config_dir,
+    } ~>
+    class { 'vault::service': } ->
+    Anchor['vault_last']
+  }
 }
diff --git a/manifests/service.pp b/manifests/service.pp
@@ -1,7 +1,7 @@
 # == Class vault::service
 #
 # This class is meant to be called from vault
-# It ensure the service is running
+# It ensures the service is running
 #
 class vault::service {
   $init_selector = $vault::init_style ? {

diff --git a/spec/classes/init_spec.rb b/spec/classes/init_spec.rb
@@ -1,17 +1,80 @@
 require 'spec_helper'
 
 describe 'vault' do
-	RSpec.configure do |c|
-	  c.default_facts = {
-	    :architecture    => 'x86_64',
-	    :operatingsystem => 'Ubuntu',
-	    :lsbdistrelease  => '10.04',
-	    :kernel          => 'Linux',
-	  }
-	end
-
-	context 'Should compile with no arguments' do
-		let(:pre_condition) { 'Exec { path => [ "/bin/", "/sbin/" , "/usr/bin/", "/usr/sbin/" ] }' }
-		it { should compile }
-	end
+  RSpec.configure do |c|
+    c.default_facts = {
+      :architecture    => 'x86_64',
+      :operatingsystem => 'Ubuntu',
+      :lsbdistrelease  => '10.04',
+      :kernel          => 'Linux',
+    }
+  end
+
+  let(:pre_condition) { 'Exec { path => [ "/bin/", "/sbin/" , "/usr/bin/", "/usr/sbin/" ] }' }
+  context 'Should compile with no arguments' do
+    it { should compile }
+  end
+
+  context 'properly escaped shellwords debian' do
+    let(:params) {{
+      :extra_options => ['foo', 'bar', 'baz baz baz'],
+      :init_style    => 'debian',
+      :backend       => {},
+      :listener      => {},
+    }}
+    it { should contain_file('/etc/init.d/vault').with_content(%r{^DAEMON_ARGS=\( server -config /etc/vault foo bar baz\\ baz\\ baz \)})}
+  end
+
+  context 'properly escaped shellwords sles' do
+    let(:params) {{
+      :extra_options => ['foo', 'bar', 'baz baz baz'],
+      :init_style    => 'sles',
+      :backend       => {},
+      :listener      => {},
+    }}
+    it { should contain_file('/etc/init.d/vault').with_content(%r{server -config "\$CONFIG_DIR" foo bar baz\\ baz\\ baz})}
+  end
+
+  context 'properly escaped shellwords systemd' do
+    let(:params) {{
+      :extra_options => ['foo', 'bar', 'baz baz baz'],
+      :init_style    => 'systemd',
+      :backend       => {},
+      :listener      => {},
+    }}
+    it { should contain_file('/lib/systemd/system/vault.service').with_content(%r{-config /etc/vault foo bar baz\\ baz\\ baz})}
+  end
+
+  context 'properly escaped shellwords launchd' do
+    let(:params) {{
+      :extra_options => ['foo', 'bar', 'baz <baz> baz'],
+      :init_style    => 'launchd',
+      :backend       => {},
+      :listener      => {},
+    }}
+    it {
+      should contain_file('/Library/LaunchDaemons/io.vaultproject.daemon.plist') \
+        .with_content(%r{<string>foo</string>\n\s*<string>bar</string>\n\s*<string>baz &lt;baz&gt; baz</string>})
+    }
+  end
+
+  context 'properly escaped shellwords sysv' do
+    let(:params) {{
+      :extra_options => ['foo', 'bar', 'baz baz baz'],
+      :init_style    => 'sysv',
+      :backend       => {},
+      :listener      => {},
+    }}
+    it { should contain_file('/etc/init.d/vault').with_content(%r{server -config "\$CONFIG" foo bar baz\\ baz\\ baz})}
+  end
+
+  context 'properly escaped shellwords upstart' do
+    let(:params) {{
+      :extra_options => ['foo', 'bar', 'baz baz baz'],
+      :init_style    => 'upstart',
+      :backend       => {},
+      :listener      => {},
+    }}
+    it { should contain_file('/etc/init/vault.conf').with_content(%r{server -config "\$CONFIG" foo bar baz\\ baz\\ baz})}
+  end
 end
diff --git a/templates/vault.debian.erb b/templates/vault.debian.erb
@@ -1,4 +1,4 @@
-#! /bin/sh
+#!/bin/bash
 ### BEGIN INIT INFO
 # Provides:          vault
 # Required-Start:    $local_fs $remote_fs
@@ -15,17 +15,18 @@
 PATH=/usr/sbin:/usr/bin:/sbin:/bin:<%= scope.lookupvar('vault::bin_dir') %>
 DESC="Vault secrets management framework"
 NAME=vault
-DAEMON=<%= scope.lookupvar('vault::bin_dir') %>/$NAME
-PIDFILE=/var/run/$NAME/$NAME.pid
-DAEMON_ARGS="server -config <%= scope.lookupvar('vault::config_dir') %> <%= scope.lookupvar('vault::extra_options') %>"
-USER=<%= scope.lookupvar('vault::user') %>
-SCRIPTNAME=/etc/init.d/$NAME
+<% require 'shellwords' %>
+DAEMON=<%= scope.lookupvar('vault::bin_dir').shellescape %>/"$NAME"
+PIDFILE="/var/run/$NAME/$NAME.pid"
+DAEMON_ARGS=( server -config <%= scope.lookupvar('vault::config_dir').shellescape %> <%= scope.lookupvar('vault::extra_options').shelljoin %> )
+USER=<%= scope.lookupvar('vault::user').shellescape %>
+SCRIPTNAME="/etc/init.d/$NAME"
 
 # Exit if the package is not installed
 [ -x "$DAEMON" ] || exit 0
 
 # Read configuration variable file if it is present
-[ -r /etc/default/$NAME ] && . /etc/default/$NAME
+[ -r "/etc/default/$NAME" ] && . "/etc/default/$NAME"
 
 # Load the VERBOSE setting and other rcS variables
 [ -f /etc/default/rcS ] && . /etc/default/rcS
@@ -39,7 +40,7 @@ SCRIPTNAME=/etc/init.d/$NAME
 #
 mkrundir() {
         [ ! -d /var/run/vault ] && mkdir -p /var/run/vault
-        chown $USER /var/run/vault
+        chown "$USER" /var/run/vault
 }
 
 #
@@ -53,15 +54,15 @@ do_start()
     #   2 if daemon could not be started
     echo "Starting vault and backgrounding"
     mkrundir
-    start-stop-daemon --start --quiet --pidfile $PIDFILE --exec $DAEMON --chuid $USER --background --make-pidfile --test > /dev/null \
+    start-stop-daemon --start --quiet --pidfile "$PIDFILE" --exec "$DAEMON" --chuid "$USER" --background --make-pidfile --test > /dev/null \
         || return 1
-    start-stop-daemon --start --quiet --pidfile $PIDFILE --exec $DAEMON --chuid $USER --background --make-pidfile -- \
-        $DAEMON_ARGS \
+    start-stop-daemon --start --quiet --pidfile "$PIDFILE" --exec "$DAEMON" --chuid "$USER" --background --make-pidfile -- \
+        "${DAEMON_ARGS[@]}" \
         || return 2
 
     echo -n "Waiting for vault daemon to be listening..."
     for i in `seq 1 30`; do
-        if ! start-stop-daemon --quiet --stop --test --pidfile $PIDFILE --exec $DAEMON --user $USER; then
+        if ! start-stop-daemon --quiet --stop --test --pidfile "$PIDFILE" --exec "$DAEMON" --user "$USER"; then
             echo " FAIL: vault process died"
             return 2
         fi
@@ -86,7 +87,7 @@ do_stop()
     #   1 if daemon was already stopped
     #   2 if daemon could not be stopped
     #   other if a failure occurred
-    start-stop-daemon --stop --quiet --retry=TERM/30/KILL/5 --pidfile $PIDFILE --name $NAME
+    start-stop-daemon --stop --quiet --retry=TERM/30/KILL/5 --pidfile "$PIDFILE" --name "$NAME"
     RETVAL="$?"
     [ "$RETVAL" = 2 ] && return 2
     # Wait for children to finish too if this is a daemon that forks
@@ -95,10 +96,10 @@ do_stop()
     # that waits for the process to drop all resources that could be
     # needed by services started subsequently.  A last resort is to
     # sleep for some time.
-    start-stop-daemon --stop --quiet --oknodo --retry=0/30/KILL/5 --exec $DAEMON
+    start-stop-daemon --stop --quiet --oknodo --retry=0/30/KILL/5 --exec "$DAEMON"
     [ "$?" = 2 ] && return 2
     # Many daemons don't delete their pidfiles when they exit.
-    rm -f $PIDFILE
+    rm -f "$PIDFILE"
     return "$RETVAL"
 }
 
@@ -111,7 +112,7 @@ do_reload() {
     # restarting (for example, when it is sent a SIGHUP),
     # then implement that here.
     #
-    start-stop-daemon --stop --signal 1 --quiet --pidfile $PIDFILE --name $NAME
+    start-stop-daemon --stop --signal 1 --quiet --pidfile "$PIDFILE" --name "$NAME"
     return 0
 }
 
@@ -164,7 +165,7 @@ case "$1" in
     esac
     ;;
   status)
-      status_of_proc -p $PIDFILE $DAEMON $NAME && exit 0 || exit $?
+      status_of_proc -p "$PIDFILE" "$DAEMON" "$NAME" && exit 0 || exit $?
       ;;
   *)
     #echo "Usage: $SCRIPTNAME {start|stop|restart|reload|force-reload}" >&2

diff --git a/templates/vault.launchd.erb b/templates/vault.launchd.erb
@@ -18,9 +18,8 @@
             <string>server</string>
             <string>-config</string>
             <string><%= scope.lookupvar('vault::config_dir') %></string>
-<% require 'shellwords' %>
-<% for extra_option in Shellwords.split(scope.lookupvar('vault::extra_options')) %>
-            <string><%= extra_option %></string>
+<% for extra_option in scope.lookupvar('vault::extra_options') -%>
+            <string><%= extra_option.encode(:xml => :text) %></string>
 <% end %>
         </array>
 </dict>

diff --git a/templates/vault.sles.erb b/templates/vault.sles.erb
@@ -20,23 +20,24 @@
 
 rc_reset
 
-VAULT_BIN=<%= scope.lookupvar('vault::bin_dir') %>/vault
-CONFIG_DIR=<%= scope.lookupvar('vault::config_dir') %>
+<% require 'shellwords' %>
+VAULT_BIN=<%= scope.lookupvar('vault::bin_dir').shellescape %>/vault
+CONFIG_DIR=<%= scope.lookupvar('vault::config_dir').shellescape %>
 LOG_FILE=/var/log/vault
 
 
 # read settings like GOMAXPROCS from "/etc/sysconfig/vault"
 [ -e /etc/sysconfig/vault ] && . /etc/sysconfig/vault
 
-export GOMAXPROCS=${GOMAXPROCS:-2}
+export GOMAXPROCS="${GOMAXPROCS:-2}"
 
 
 case "$1" in
     start)
         echo -n "Starting vault "
         ## Start daemon with startproc(8). If this fails
         ## the return value is set appropriately by startproc.
-        startproc $VAULT_BIN server -config "$CONFIG_DIR" <%= scope.lookupvar('vault::extra_options') %> >> "$LOG_FILE"
+        startproc "$VAULT_BIN" server -config "$CONFIG_DIR" <%= scope.lookupvar('vault::extra_options').shelljoin %> >> "$LOG_FILE"
 
         # Remember status and be verbose
         rc_status -v
@@ -46,24 +47,24 @@ case "$1" in
         ## Stop daemon with killproc(8) and if this fails
         ## killproc sets the return value according to LSB.
 
-        killproc -TERM $VAULT_BIN
+        killproc -TERM "$VAULT_BIN"
 
         # Remember status and be verbose
         rc_status -v
         ;;
     restart)
         ## Stop the service and regardless of whether it was
         ## running or not, start it again.
-        $0 stop
-        $0 start
+        "$0" stop
+        "$0" start
 
         # Remember status and be quiet
         rc_status
         ;;
     reload)
         # If it supports signaling:
         echo -n "Reload service vault "
-        killproc -HUP $VAULT_BIN
+        killproc -HUP "$VAULT_BIN"
         #touch /var/run/vault.pid
         rc_status -v
 
@@ -85,7 +86,7 @@ case "$1" in
         # 5--199 reserved (5--99 LSB, 100--149 distro, 150--199 appl.)
 
         # NOTE: checkproc returns LSB compliant status values.
-        checkproc $VAULT_BIN
+        checkproc "$VAULT_BIN"
         # NOTE: rc_status knows that we called this init script with
         # "status" option and adapts its messages accordingly.
         rc_status -v