BREAKING: Refactor selinux::module #189

oranenj · 2017-01-29T15:09:58Z

I thought I'd actually implement part of what's being discussed in #178

This doesn't yet support building the things without selinux-devel,
but it does work, and handles failures gracefully.

It doesn't look like module building needs its own provider, but behaviour on RHEL5/6 should be tested.

oranenj · 2017-01-29T15:11:57Z

tests will be failing and there are lots of hardcoded paths for now. Still wondering what the best way to select the build dir would be. A custom fact as discussed in #146 ?

oranenj · 2017-01-29T16:49:44Z

I implemented simple module building using just checkmodule with a helper script. Also added the custom fact. works for me, but there's still some work to do.

I noticed that the module installs the wrong packages by default. On some distros, it only installs policycoreutils, and on others it only installs selinux-policy-devel

If we really want to support modules without selinux-policy-devel etc, that needs to be refactored too.

oranenj · 2017-02-04T18:46:12Z

I cleaned this up a bit and fixed the tests.

Acceptance goes through with CentOS6, CentOS7 and Fedora 25

oranenj · 2017-02-04T19:23:03Z

RHEL7 default package was selinux-policy-devel too. Changed it to policycoreutils-python which is where semanage resides

This commit also introduces an alternative 'simple' builder to refpolicy, and consequently the 'selinux-policy-devel' package is not always needed

oranenj · 2017-02-04T19:28:54Z

rebased once more to clean things up. I'm pretty happy with this as it is. Any comments?

vinzent

only had a very quick look. do tests tomorrow. :)

overall looks great!

vinzent · 2017-02-04T20:00:23Z

manifests/config.pp

+  }
+
+  # put helper in place:
+  file {"${module_build_root}/modules/selinux_build_module.sh":


I think this file needs bin_tseltype or puppet agent will maybe throw SELinux AVC's when it executes?

maybe we should review seltypes of the directory.

I didn't get any SELinux errors running the module, and I run my desktop in enforcing mode, so it should be fine. The acceptance tests also seemed fine.

even if the puppet agent process runs as puppetagent_t type it does not produce an AVC when it executes the selinux_build_module.sh script which has type usr_t. so this is fine. I just wonder why it does not.

by default the puppet agent process even does run in unconfinged_service_t (or initrc_t) because the paths in the refpolicy were not adapted to the /opt/puppetlabs change.

vinzent · 2017-02-04T20:01:35Z

manifests/init.pp

+# refpolicy module builder
+#   Default value: OS dependent (see params.pp)
+# @param default_builder which builder to use by default with selinux::module
+#   Default value: refpolicy


default is simple isn't it?

This is a leftover when I had refpolicy as the default earlier, but I think simple is better because it has fewer dependencies.

vinzent · 2017-02-05T12:02:29Z

manifests/config.pp

+  }
+
+  # put helper in place:
+  file {"${module_build_root}/modules/selinux_build_module.sh":


even if the puppet agent process runs as puppetagent_t type it does not produce an AVC when it executes the selinux_build_module.sh script which has type usr_t. so this is fine. I just wonder why it does not.

by default the puppet agent process even does run in unconfinged_service_t (or initrc_t) because the paths in the refpolicy were not adapted to the /opt/puppetlabs change.

vinzent · 2017-02-05T12:03:39Z

manifests/init.pp

+  $package_name           = $::selinux::params::package_name,
+  $refpolicy_package_name = $::selinux::params::refpolicy_package_name,
+  $module_build_root      = $::selinux::params::module_build_root,
+  $default_builder        = 'simple',


it's a good choice that the default is the one with the least required packages.

vinzent · 2017-02-05T12:10:50Z

manifests/module.pp

  $syncversion  = undef,
 ) {

+  if $builder == 'refpolicy' {
+    require ::selinux::refpolicy_package


can we maybe make this selinux::package::policy_devel? the term refpolicy related to the package isn't quite right because refpolicy is what Tresys provides. The RHEL/Fedora fork has large differences to the refpolicy.

But maybe at all not important and just a personal opinion of mine. :)

vinzent · 2017-02-05T12:13:53Z

manifests/module.pp

+      creates => "${module_file}.pp",
+      notify  => Exec["install-module-${title}"],
+    }
+    # we need to install the module manually because selmodule is kind of dumb. It ends up


what kind of dumb? can you elaborate more? Should we file a bug on the puppetlabs jira?

It's because of the syncversion bug, I think. It can only check that the module is loaded, and if you recompile a module that's loaded, it won't actually detect the change. Installing it manually is a workaround for that.

Also, the lack of an AVC might be because the files are in the libdir which contains executables anyway (ie. facts and such). The policy seems to allow puppetagent_t to do quite a lot, so might be that it just has permission to execute whatever is in the libdir

vinzent · 2017-02-05T12:16:46Z

spec/acceptance/class_spec.rb

-      # with puppet4 I would use a HERE DOC to make this pretty,
-      # but with puppet3 it's not possible.
+      # just something simple I found via Google:
+      file {'/tmp/selinux_simple_policy.te':


thanks for rewriting with HERE DOC. 👍

vinzent · 2017-02-05T12:22:18Z

manifests/module.pp

+  validate_absolute_path($::selinux::config::module_build_dir)
+
+  $module_dir = "${::selinux::config::module_build_dir}/${title}"
+  $module_file = "${module_dir}/${title}"


If one manages multiple refpolicy style policies, lets call them A and B, with *.if files and A depends on an interface provided by B's *.if I don't know if they require to be in the same directory so that the makefile finds the interfaces.

this path creates a directory for every module. Is this a requirement? There should IMHO never be a same named policy source file.

With newer (>= CentOS 7.3.) the refpolicy style will fail if filename and resulting policy name are not the same.

vinzent · 2017-02-05T12:49:08Z

spec/acceptance/class_spec.rb

+      # just something simple I found via Google:
+      file {'/tmp/selinux_simple_policy.te':
+        ensure  => 'file',
+        content => @("EOF")


adding 2-space indentation to the here doc would make it more readable.

vinzent added this to the 1.0.0 milestone Feb 2, 2017

vinzent added the needs-work not ready to merge just yet label Feb 2, 2017

oranenj force-pushed the module_refactor branch from a01db1f to 147b778 Compare February 4, 2017 18:45

oranenj changed the title ~~Refactor selinux::module (WIP)~~ Refactor selinux::module Feb 4, 2017

oranenj added 6 commits February 4, 2017 21:27

Refactor module building to be more robust

11833ca

This commit also introduces an alternative 'simple' builder to refpolicy, and consequently the 'selinux-policy-devel' package is not always needed

Fix package name for Fedora 24/25

f94da68

Fix RHEL7 package name

c6257f2

Add spec tests for the module build functionality

1512735

Fix the acceptance tests

555297b

Bring README.md up-to-date with a note about upgrading from 0.8

8da2298

oranenj force-pushed the module_refactor branch from 2fc7908 to 8da2298 Compare February 4, 2017 19:28

vinzent reviewed Feb 4, 2017

View reviewed changes

oranenj added 2 commits February 4, 2017 23:09

Actually make use of the '$refpolicy_makefile' variable

1f91bc7

Fix documentation for default_builder

8c9ef4e

oranenj force-pushed the module_refactor branch from 91f5749 to 8c9ef4e Compare February 4, 2017 21:10

Use types in init.pp instead of validation functions

d10fa4d

vinzent added backwards-incompatible enhancement New feature or request needs-feedback Further information is requested and removed needs-work not ready to merge just yet labels Feb 5, 2017

vinzent reviewed Feb 5, 2017

View reviewed changes

Indent HEREDOC for readability

1df7b55

vinzent mentioned this pull request Feb 8, 2017

BREAKING: Redesign selinux::module defined type #195

Merged

vinzent closed this in #195 Feb 17, 2017

vinzent changed the title ~~Refactor selinux::module~~ BREAKING: Refactor selinux::module Mar 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING: Refactor selinux::module #189

BREAKING: Refactor selinux::module #189

oranenj commented Jan 29, 2017

oranenj commented Jan 29, 2017

oranenj commented Jan 29, 2017

oranenj commented Feb 4, 2017

oranenj commented Feb 4, 2017

oranenj commented Feb 4, 2017

vinzent left a comment

vinzent Feb 4, 2017

vinzent Feb 4, 2017

oranenj Feb 4, 2017

vinzent Feb 5, 2017

vinzent Feb 4, 2017

oranenj Feb 4, 2017

vinzent Feb 5, 2017

vinzent Feb 5, 2017

vinzent Feb 5, 2017

vinzent Feb 5, 2017

oranenj Feb 5, 2017

oranenj Feb 5, 2017

vinzent Feb 5, 2017

vinzent Feb 5, 2017

vinzent Feb 5, 2017

BREAKING: Refactor selinux::module #189

BREAKING: Refactor selinux::module #189

Conversation

oranenj commented Jan 29, 2017

oranenj commented Jan 29, 2017

oranenj commented Jan 29, 2017

oranenj commented Feb 4, 2017

oranenj commented Feb 4, 2017

oranenj commented Feb 4, 2017

vinzent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment