(SIMP-4568) Stunnel changing ownership of root (#53)

- Removed init.d script on systemd systems
- Added checks for chroot evaluation to '/'
- Set value for pidfile name if it was undefined to
  prevent problems if for some reason the old init.d
  script is on the system.
- Updated tests and added check for directory
- Remove unnecessary idempotency hack

SIMP-4568 #comment close

CHANGELOG

@@ -1,3 +1,10 @@
+* Tue Mar 27 2018 Trevor Vaughan <[email protected]> - 6.3.0
+- Ensure init.d script is absent if systemd system because puppet
+  was finding it and running it and setting permissions on root to
+  stunnel:stunnel.
+- Ensure pid file name is not left undefined to prevent unplanned results.
+- Added check to make sure chroot has not evaluated to '/'.
+
 * Tue Mar 06 2018 Liz Nemsick <[email protected]> - 6.3.0
 - Fixed bug in which the stunnel systemd pre-exec script failed to
   execute completely, because one command did not have a fully

manifests/config.pp

@@ -171,17 +171,32 @@
   if $pid =~ Undef {
     if $on_systemd {
       $_foreground = true
-      $_pid        = $pid
     } else {
       $_foreground = undef
-      $_pid        = '/var/run/stunnel/stunnel.pid'
     }
+    $_pid        = '/var/run/stunnel/stunnel.pid'
     $_legacy_pid = '/var/run/stunnel/stunnel.pid'
+
   } else {
     $_pid        = $pid
     $_legacy_pid = $pid
   }
 
+  if $_pid {
+    $_stunnel_piddir = File[dirname($_pid)]
+    ensure_resource('file', dirname($_pid),
+      {
+        'ensure'  => 'directory',
+        'owner'   => $setuid,
+        'group'   => $setgid,
+        'mode'    => '0644',
+        'seluser' => 'system_u',
+        'selrole' => 'object_r',
+        'seltype' => 'stunnel_var_run_t',
+      }
+    )
+  }
+
   concat { '/etc/stunnel/stunnel.conf':
     owner          => 'root',
     group          => 'root',
@@ -196,7 +211,12 @@
     content => template('stunnel/connection_conf.erb')
   }
 
-  if $_chroot {
+  if $_chroot !~ Undef {
+    # $chroot should never be undef here, or just '/'.
+    if $_chroot in ['/',''] {
+      fail("stunnel: \$chroot should not be root ('/')")
+    }
+
     # The _chroot directory
     file { $_chroot:
       ensure => 'directory',

manifests/service.pp

@@ -5,17 +5,22 @@
 class stunnel::service {
 
   if 'systemd' in $facts['init_systems'] {
+    file { '/etc/rc.d/init.d/stunnel': ensure => 'absent' }
+
     service { 'stunnel':
       ensure  => running,
       enable  => true,
-      require => File['/etc/systemd/system/stunnel.service']
+      require => [
+        File['/etc/systemd/system/stunnel.service'],
+        File['/etc/rc.d/init.d/stunnel']
+      ]
     }
   } else {
     # The script takes care of chkconfig
     service { 'stunnel':
       ensure  => running,
       enable  => true,
-      require => File['/etc/rc.d/init.d/stunnel'],
+      require => File['/etc/rc.d/init.d/stunnel']
     }
   }
 }

spec/acceptance/suites/default/00_instances_spec.rb

@@ -43,8 +43,6 @@
       it 'should apply with no errors' do
         set_hieradata_on(host,hieradata)
         apply_manifest_on(host,manifest)
-        apply_manifest_on(host,manifest, catch_failures: true)
-        apply_manifest_on(host,manifest, catch_failures: true)
       end
 
       it 'should be idempotent' do

spec/acceptance/suites/default/01_connection_spec.rb

@@ -60,14 +60,13 @@
           on(host, 'chown -R root:root /etc/pki/simp-testing/pki')
           on(host, 'chmod -R go+r /etc/pki/simp-testing/pki')
           on(host, 'chcon -R --type cert_t /etc/pki/simp-testing/pki')
-          on(host, '/etc/rc.d/init.d/stunnel_legacy start')
+          on(host, 'SYSTEMCTL_SKIP_REDIRECT=yes /etc/rc.d/init.d/stunnel_legacy start')
           pid = on(host, 'cat /var/run/stunnel/stunnel.pid').stdout.strip
           on(host, "ps -f --pid #{pid}")
 
           apply_manifest_on(host,base_manifest, catch_failures: true)
           apply_manifest_on(host,base_manifest, catch_changes: true)
           on(host, "ps -f --pid #{pid}", :acceptable_exit_codes => [1])
-          on(host, 'ls /var/run/stunnel/stunnel.pid', :acceptable_exit_codes => [2])
         end
       end
     end

spec/classes/config_spec.rb

@@ -51,7 +51,7 @@
                 debug = err
                 syslog = no
                 foreground = yes
-                pid =
+                pid = /var/run/stunnel/stunnel.pid
                 engine = auto
                 fips = no
               EOM
@@ -86,8 +86,9 @@
             it { is_expected.to create_file('/etc/systemd/system/stunnel.service')
                                   .that_notifies('Exec[stunnel daemon reload]')
                                   .with_content(service_file) }
+            it { is_expected.to contain_file('/etc/rc.d/init.d/stunnel').with_ensure('absent')}
             it { is_expected.to contain_service('stunnel')
-                                  .that_requires('File[/etc/systemd/system/stunnel.service]') }
+                                  .that_requires(['File[/etc/systemd/system/stunnel.service]','File[/etc/rc.d/init.d/stunnel]']) }
             it { is_expected.to contain_exec('stunnel daemon reload') }
           else
             let(:service_file) { File.read('spec/expected/connection/chroot-init.txt') }
@@ -96,6 +97,16 @@
           end
         end
 
+        context 'with parameters chroot set to /' do
+          let(:params) {{
+            chroot:     '/',
+          }}
+          #          Evaluation Error: Error while evaluating a Function Call, stunnel: $chroot should not be root ('/') at /var/jmg/SIMP-4568/pupmod-simp-stunnel/spec/fixtures/modules/stunnel/manifests/config.pp:202:7 on node ws151.tasty.bacon
+          it "is expected to fail" do
+            expect { catalogue }.to raise_error Puppet::Error, /chroot should not be root/
+          end
+        end
+
         context 'with selinux = true (non-chrooted)' do
           let(:facts) {
             os_facts.merge(
@@ -114,7 +125,7 @@
                 debug = err
                 syslog = no
                 foreground = yes
-                pid =
+                pid = /var/run/stunnel/stunnel.pid
                 engine = auto
                 fips = no
               EOM