Merge pull request #117 from cybercom-finland/cleanups2

Cleanups and dangling issues

lib/facter/selinux_custom_policy.rb

@@ -7,6 +7,13 @@
 Facter.add(:selinux_custom_policy) do
   confine kernel: 'Linux', osfamily: 'RedHat', operatingsystemmajrelease: '7', selinux: ['true', true]
   setcode do
-    Facter::Util::Resolution.exec("sestatus | grep 'Loaded policy name' | awk '{ print \$4 }'")
+    policy = nil
+    output = Facter::Util::Resolution.exec('sestatus 2>/dev/null')
+    if output
+      output.each_line do |line|
+        break if line =~ %r{^Loaded policy name:\s*(?<policy>.*)$}
+      end
+    end
+    policy
   end
 end

manifests/config.pp

@@ -61,8 +61,8 @@
 
     exec { "change-selinux-status-to-${mode}":
       command => "setenforce ${sestatus}",
-      unless  => "getenforce | grep -qi \"${mode}\\|disabled\"",
-      path    => '/bin:/usr/bin:/usr/sbin',
+      unless  => "getenforce | grep -Eqi '${mode}|disabled'",
+      path    => '/bin:/sbin:/usr/bin:/usr/sbin',
     }
   }
 

manifests/fcontext.pp

@@ -70,7 +70,7 @@
   $destination         = undef,
   $context             = undef,
   $filetype            = false,
-  $filemode            = undef,
+  $filemode            = 'a',
   $equals              = false,
   $restorecond         = true,
   $restorecond_path    = undef,
@@ -96,43 +96,41 @@
   validate_absolute_path($restorecond_path_private)
 
   $restorecond_resurse_private = $restorecond_recurse ? {
-    true  => '-R ',
-    false => ''
+    true  => ['-R'],
+    false => [],
   }
 
   if $equals and $filetype {
     fail('Resource cannot contain both "equals" and "filetype" options')
   }
 
-  if $filetype and $filemode !~ /(a|f|d|c|b|s|l|p)/ {
-    fail('file mode must be one of: a,f,d,c,b,s,l,p - see "man semanage-fcontext"')
-  }
-
   if $equals {
     $resource_name = "add_${destination}_${pathname}"
-    $command       = "semanage fcontext -a -e \"${destination}\" \"${pathname}\""
-    $unless        = "semanage fcontext -l | grep -E \"^${pathname} = ${destination}$\""
-  } elsif $filetype {
-    $resource_name = "add_${context}_${pathname}_type_${filemode}"
-    $command       = "semanage fcontext -a -f ${filemode} -t ${context} \"${pathname}\""
-    $unless        = "semanage fcontext -l | grep \"^${pathname}[[:space:]].*:${context}:\""
+    $command       = shellquote('semanage', 'fcontext','-a', '-e', $destination, $pathname)
+    $unless        = sprintf('semanage fcontext -l | grep -Fx %s', shellquote("${pathname} = ${destination}"))
   } else {
-    $resource_name = "add_${context}_${pathname}"
-    $command       = "semanage fcontext -a -t ${context} \"${pathname}\""
-    $unless        = "semanage fcontext -l | grep \"^${pathname}[[:space:]].*:${context}:\""
+    if $filemode !~ /^(?:a|f|d|c|b|s|l|p)$/ {
+      fail('"filemode" must be one of: a,f,d,c,b,s,l,p - see "man semanage-fcontext"')
+    }
+    $resource_name = "add_${context}_${pathname}_type_${filemode}"
+    $command       = shellquote('semanage', 'fcontext','-a', '-f', $filemode, '-t', $context, $pathname)
+    $unless        = sprintf('semanage fcontext -E | grep -Fx %s', shellquote("fcontext -a -f ${filemode} -t ${context} '${pathname}'"))
+  }
+
+  Exec {
+    path => '/bin:/sbin:/usr/bin:/usr/sbin',
   }
 
   exec { $resource_name:
     command => $command,
     unless  => $unless,
-    path    => '/bin:/sbin:/usr/bin:/usr/sbin',
     require => Class['selinux::package'],
   }
 
   if $restorecond {
     exec { "restorecond ${resource_name}":
-      path        => '/bin:/sbin:/usr/bin:/usr/sbin',
-      command     => "restorecon ${restorecond_resurse_private}${restorecond_path_private}",
+      command     => shellquote('restorecon', $restorecond_resurse_private, $restorecond_path_private),
+      onlyif      => shellquote('test', '-e', $restorecond_path_private),
       refreshonly => true,
       subscribe   => Exec[$resource_name],
     }

manifests/module.pp

@@ -71,10 +71,10 @@
   ~>
   exec { "${sx_mod_dir}/${prefix}${name}.pp":
   # Only allow refresh in the event that the initial .te file is updated.
-    path        => '/sbin:/usr/sbin:/bin:/usr/bin',
+    command     => shellquote('make', '-f', $makefile, "${prefix}${name}.pp"),
+    path        => '/bin:/sbin:/usr/bin:/usr/sbin',
     refreshonly => true,
     cwd         => $sx_mod_dir,
-    command     => "make -f ${makefile} ${prefix}${name}.pp",
   }
   ->
   selmodule { $name:

manifests/permissive.pp

@@ -33,8 +33,8 @@
   include ::selinux
 
   exec { "add_${context}":
-    command => "semanage permissive -a ${context}",
-    unless  => "semanage permissive -l|grep ${context}",
+    command => shellquote('semanage', 'permissive', '-a', $context),
+    unless  => sprintf('semanage permissive -l | grep -Fx %s', shellquote($context)),
     path    => '/bin:/sbin:/usr/bin:/usr/sbin',
     require => Class['selinux::package'],
   }

manifests/port.pp

@@ -45,16 +45,19 @@
 
   if $protocol {
     validate_re($protocol, ['^tcp6?$', '^udp6?$'])
-    $protocol_switch = "-p ${protocol} "
+    $protocol_switch = ['-p', $protocol]
+    $protocol_check = "${protocol} "
     $port_exec_command = "add_${context}_${port}_${protocol}"
   } else {
-    $protocol_switch = undef
+    $protocol_switch = []
+    $protocol_check = '' # lint:ignore:empty_string_assignment variable is used to create regexp and undef is not possible
     $port_exec_command = "add_${context}_${port}"
   }
 
   exec { $port_exec_command:
-    command => "semanage port -a -t ${context} ${protocol_switch}${port}",
-    unless  => "semanage port -l|grep \"^${context}.*${protocol}.*${port}\"|grep -w ${port}",
+    command => shellquote('semanage', 'port', '-a', '-t', $context, $protocol_switch, "${port}"), # lint:ignore:only_variable_string port can be number and we need to force it to be string for shellquote
+    # This works because there seems to be more than one space after protocol and before first port
+    unless  => sprintf('semanage port -l | grep -E %s', shellquote("^${context}  *${protocol_check}.* ${port}(\$|,)")),
     path    => '/bin:/sbin:/usr/bin:/usr/sbin',
     require => Class['selinux::package'],
   }

spec/defines/selinux_fcontext_spec.rb

@@ -9,50 +9,142 @@
   end
 
   context 'equal requires destination' do
-    let(:params) { { pathname: '/tmp/file1', equals: true } }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        equals: true
+      }
+    end
     it { expect { is_expected.to compile }.to raise_error(%r{is not an absolute path}) }
   end
 
+  context 'invalid filemode with filetype false' do
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        filetype: false,
+        filemode: 'X',
+        context: 'user_home_dir_t'
+      }
+    end
+    it { expect { is_expected.to compile }.to raise_error(%r{"filemode" must be one of: a,f,d,c,b,s,l,p - see "man semanage-fcontext"}) }
+  end
+
   context 'invalid filetype' do
-    let(:params) { { pathname: '/tmp/file1', filetype: true, filemode: 'X', context: 'user_home_dir_t' } }
-    it { expect { is_expected.to compile }.to raise_error(%r{file mode must be one of: a,f,d,c,b,s,l,p - see "man semanage-fcontext"}) }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        filetype: true,
+        filemode: 'X',
+        context: 'user_home_dir_t'
+      }
+    end
+    it { expect { is_expected.to compile }.to raise_error(%r{"filemode" must be one of: a,f,d,c,b,s,l,p - see "man semanage-fcontext"}) }
+  end
+
+  context 'invalid multiple filetype' do
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        filetype: true,
+        filemode: 'afdcbslp',
+        context: 'user_home_dir_t'
+      }
+    end
+    it { expect { is_expected.to compile }.to raise_error(%r{"filemode" must be one of: a,f,d,c,b,s,l,p - see "man semanage-fcontext"}) }
   end
 
   context 'equals and filetype' do
-    let(:params) { { pathname: '/tmp/file1', equals: true, filetype: true, filemode: 'a', context: 'user_home_dir_t', destination: '/tmp/file2' } }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        equals: true,
+        filetype: true,
+        filemode: 'a',
+        context: 'user_home_dir_t',
+        destination: '/tmp/file2'
+      }
+    end
     it { expect { is_expected.to compile }.to raise_error(%r{cannot contain both "equals" and "filetype" options}) }
   end
 
   context 'substituting fcontext' do
-    let(:params) { { pathname: '/tmp/file1', equals: true, destination: '/tmp/file2' } }
-    it { should contain_exec('add_/tmp/file2_/tmp/file1').with(command: 'semanage fcontext -a -e "/tmp/file2" "/tmp/file1"') }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        equals: true,
+        destination: '/tmp/file2'
+      }
+    end
+    it { should contain_exec('add_/tmp/file2_/tmp/file1').with(command: 'semanage fcontext -a -e /tmp/file2 /tmp/file1') }
     it { should contain_exec('restorecond add_/tmp/file2_/tmp/file1').with(command: 'restorecon /tmp/file1') }
   end
 
   context 'set filemode and context' do
-    let(:params) { { pathname: '/tmp/file1', filetype: true, filemode: 'a', context: 'user_home_dir_t' } }
-    it { should contain_exec('add_user_home_dir_t_/tmp/file1_type_a').with(command: 'semanage fcontext -a -f a -t user_home_dir_t "/tmp/file1"') }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        filetype: true,
+        filemode: 'a',
+        context: 'user_home_dir_t'
+      }
+    end
+    it { should contain_exec('add_user_home_dir_t_/tmp/file1_type_a').with(command: 'semanage fcontext -a -f a -t user_home_dir_t /tmp/file1') }
     it { should contain_exec('restorecond add_user_home_dir_t_/tmp/file1_type_a').with(command: 'restorecon /tmp/file1') }
   end
 
   context 'set context' do
-    let(:params) { { pathname: '/tmp/file1', context: 'user_home_dir_t' } }
-    it { should contain_exec('add_user_home_dir_t_/tmp/file1').with(command: 'semanage fcontext -a -t user_home_dir_t "/tmp/file1"') }
-    it { should contain_exec('restorecond add_user_home_dir_t_/tmp/file1').with(command: 'restorecon /tmp/file1') }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        context: 'user_home_dir_t'
+      }
+    end
+    it { should contain_exec('add_user_home_dir_t_/tmp/file1_type_a').with(command: 'semanage fcontext -a -f a -t user_home_dir_t /tmp/file1') }
+    it { should contain_exec('restorecond add_user_home_dir_t_/tmp/file1_type_a').with(command: 'restorecon /tmp/file1') }
   end
 
   context 'with restorecon disabled' do
-    let(:params) { { pathname: '/tmp/file1', context: 'user_home_dir_t', restorecond: false } }
-    it { should_not contain_exec('restorecond add_user_home_dir_t_/tmp/file1').with(command: 'restorecon /tmp/file1') }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        context: 'user_home_dir_t',
+        restorecond: false
+      }
+    end
+    it { should_not contain_exec('restorecond add_user_home_dir_t_/tmp/file1_type_a').with(command: %r{restorecon}) }
   end
   context 'with restorecon specific path' do
-    let(:params) { { pathname: '/tmp/file1', context: 'user_home_dir_t', restorecond_path: '/tmp/file1/different' } }
-    it { should contain_exec('add_user_home_dir_t_/tmp/file1').with(command: 'semanage fcontext -a -t user_home_dir_t "/tmp/file1"') }
-    it { should contain_exec('restorecond add_user_home_dir_t_/tmp/file1').with(command: 'restorecon /tmp/file1/different') }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        context: 'user_home_dir_t',
+        restorecond_path: '/tmp/file1/different'
+      }
+    end
+    it { should contain_exec('add_user_home_dir_t_/tmp/file1_type_a').with(command: 'semanage fcontext -a -f a -t user_home_dir_t /tmp/file1') }
+    it { should contain_exec('restorecond add_user_home_dir_t_/tmp/file1_type_a').with(command: 'restorecon /tmp/file1/different') }
   end
   context 'with restorecon recurse specific path' do
-    let(:params) { { pathname: '/tmp/file1', context: 'user_home_dir_t', restorecond_path: '/tmp/file1/different', restorecond_recurse: true } }
-    it { should contain_exec('add_user_home_dir_t_/tmp/file1').with(command: 'semanage fcontext -a -t user_home_dir_t "/tmp/file1"') }
-    it { should contain_exec('restorecond add_user_home_dir_t_/tmp/file1').with(command: 'restorecon -R /tmp/file1/different') }
+    let(:params) do
+      {
+        pathname: '/tmp/file1',
+        context: 'user_home_dir_t',
+        restorecond_path: '/tmp/file1/different',
+        restorecond_recurse: true
+      }
+    end
+    it { should contain_exec('add_user_home_dir_t_/tmp/file1_type_a').with(command: 'semanage fcontext -a -f a -t user_home_dir_t /tmp/file1') }
+    it { should contain_exec('restorecond add_user_home_dir_t_/tmp/file1_type_a').with(command: 'restorecon -R /tmp/file1/different') }
+  end
+  context 'with restorecon path with quotation' do
+    let(:params) do
+      {
+        pathname: '/tmp/"$HOME"/"$PATH"/[^ \'\\\#\`]+(?:.*)',
+        context: 'user_home_dir_t'
+      }
+    end
+    it { should contain_exec('add_user_home_dir_t_/tmp/"$HOME"/"$PATH"/[^ \'\\\#\`]+(?:.*)_type_a').with(command: 'semanage fcontext -a -f a -t user_home_dir_t "/tmp/\\"\\$HOME\\"/\\"\\$PATH\\"/[^ \'\\\\\\\\#\\\\\`]+(?:.*)"') }
+    it { should contain_exec('restorecond add_user_home_dir_t_/tmp/"$HOME"/"$PATH"/[^ \'\\\#\`]+(?:.*)_type_a').with(command: 'restorecon "/tmp/\\"\\$HOME\\"/\\"\\$PATH\\"/[^ \'\\\\\\\\#\\\\\`]+(?:.*)"') }
   end
 end

spec/defines/selinux_module_spec.rb

@@ -7,7 +7,7 @@
   context 'present case' do
     let(:params) do
       {
-        source: 'test_value'
+        source: 'puppet:///modules/mymodule/selinux/mymodule.te'
       }
     end
 
@@ -23,8 +23,7 @@
   context 'absent case' do
     let(:params) do
       {
-        ensure: 'absent',
-        source: 'test_value'
+        ensure: 'absent'
       }
     end
 

spec/defines/selinux_port_spec.rb

@@ -6,18 +6,45 @@
 
   %w(tcp udp tcp6 udp6).each do |protocol|
     context "valid protocol #{protocol}" do
-      let(:params) { { context: 'http_port_t', port: 8080, protocol: protocol } }
+      let(:params) do
+        {
+          context: 'http_port_t',
+          port: 8080,
+          protocol: protocol
+        }
+      end
       it { should contain_exec("add_http_port_t_8080_#{protocol}").with(command: "semanage port -a -t http_port_t -p #{protocol} 8080") }
     end
+    context "protocol #{protocol} and port as range" do
+      let(:params) do
+        {
+          context: 'http_port_t',
+          port: '8080-8089',
+          protocol: protocol
+        }
+      end
+      it { should contain_exec("add_http_port_t_8080-8089_#{protocol}").with(command: "semanage port -a -t http_port_t -p #{protocol} 8080-8089") }
+    end
   end
 
   context 'invalid protocol' do
-    let(:params) { { context: 'http_port_t', port: 8080, protocol: 'bad' } }
+    let(:params) do
+      {
+        context: 'http_port_t',
+        port: 8080,
+        protocol: 'bad'
+      }
+    end
     it { expect { is_expected.to compile }.to raise_error(%r{error during compilation}) }
   end
 
   context 'no protocol' do
-    let(:params) { { context: 'http_port_t', port: 8080 } }
+    let(:params) do
+      {
+        context: 'http_port_t',
+        port: 8080
+      }
+    end
     it { should contain_exec('add_http_port_t_8080').with(command: 'semanage port -a -t http_port_t 8080') }
   end
 end

spec/defines/selinux_restorecond_fragment_spec.rb

@@ -6,12 +6,21 @@
   include_context 'RedHat 7'
 
   context 'source' do
-    let(:params) { { source: 'puppet:///data/cond.txt' } }
+    let(:params) do
+      {
+        source: 'puppet:///data/cond.txt'
+      }
+    end
     it { should contain_concat__fragment('restorecond_conf_cond').with(source: 'puppet:///data/cond.txt', order: 10) }
   end
 
   context 'content and order' do
-    let(:params) { { content: '/etc/myapp', order: 20 } }
+    let(:params) do
+      {
+        content: '/etc/myapp',
+        order: 20
+      }
+    end
     it { should contain_concat__fragment('restorecond_conf_cond').with(content: '/etc/myapp', order: 20) }
   end
 end