Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BREAKING: Add selinux_fcontext and selinux_fcontext_equivalence types #177

Merged
merged 9 commits into from
Jan 31, 2017

Conversation

oranenj
Copy link
Contributor

@oranenj oranenj commented Jan 15, 2017

Based on my port type, here's a pull request for fcontexts, up for review and comments.

It seemed easier to separate the equivalences from the other kind of fcontext. It might make sense to separate the defines too (ie. selinux::fcontext and selinux::fcontext::equivalence) depending on how much of the API it is okay to break.

As of the creation of this pull request, this branch breaks support for systems with old semanage versions (ie. rhel6 and older) because the accepted syntax is different. It's probably possible to subclass the provider and override the affected methods.

@oranenj oranenj force-pushed the selinux_fcontext_type branch from 6b47f6b to 4cf32eb Compare January 15, 2017 17:41
Copy link
Contributor

@vinzent vinzent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I agree with you that fcontext and fcontext_equivalence are dedicated things.
  • I also would prefer to split out equivalence to a dedicated defined type and introduce a breaking change. We're not yet 1.0.0 and for 1.0.0 we should have something that will work for a longer time.
  • EL6 support is needed (and IMHO even EL5, but maybe makes no difference)

isrequired
end

newproperty(:context) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as with the selinux_port type. context is the combination of user:role:type:range https://selinuxproject.org/page/BasicConcepts

prior art in naming the selinux parameters see the puppet file type: https://docs.puppet.com/puppet/latest/type.html#file

Optional[String] $user = undef,
Optional[String] $filemode = 'a',
Boolean $equals = false,
Boolean $restorecond = true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are already changing things, restorcond with d referes to the daemon. the utility is only called restorecon. The parameter only runs restorcon and does nothing with restorcond.

defaultfor kernel: 'Linux'

commands semanage: 'semanage'
confine selinux: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment why selinux: true is used?

@oranenj
Copy link
Contributor Author

oranenj commented Jan 16, 2017

If we're really breaking APIs I might like to do away with the restorecon parameters in fcontext entirely and provide a separate helper define for restorecon execs. The API as it is is rather clunky.

@oranenj
Copy link
Contributor Author

oranenj commented Jan 16, 2017

will fix tests tomorrow

@vinzent
Copy link
Contributor

vinzent commented Jan 16, 2017 via email

@vinzent vinzent added this to the 2.0.0 milestone Jan 17, 2017
@oranenj oranenj force-pushed the selinux_fcontext_type branch 3 times, most recently from 1cf9d7c to d988150 Compare January 17, 2017 16:47
@oranenj
Copy link
Contributor Author

oranenj commented Jan 19, 2017

Latest commit attempts to restore support for old semanage. The proper way would probably be to have a custom fact query the package version, but explicit checking for RedHat <7 will probably do for most cases.

@oranenj oranenj force-pushed the selinux_fcontext_type branch from f2d70c8 to 996304c Compare January 21, 2017 18:10
@oranenj
Copy link
Contributor Author

oranenj commented Jan 21, 2017

I don't know how to stub facter properly. I have no idea if the cause for the errors is in the spec or in the provider :/

@oranenj oranenj force-pushed the selinux_fcontext_type branch from 286e6c3 to 09225ba Compare January 22, 2017 09:30
@oranenj
Copy link
Contributor Author

oranenj commented Jan 22, 2017

I gave up on trying to get the RHEL6 specs to work, so currently the spec is actually wrong for RHEL5 and 6. It looks like the test harness just ignores facts for some reason, or maybe they are evaluated before Facter is mocked. Either way, I don't know how to fix it and my patience has been used up.

The code itself works when I tested it, so as long as I write some acceptance tests it should be fine.

@oranenj
Copy link
Contributor Author

oranenj commented Jan 22, 2017

Currently working on extending the acceptance tests. I also simplified the fcontext and equivalence defines and removed the built-in restorecon support. I may add restorecon back at some point.

There's something weird going on with the file resource, as the files do not get created with the correct equivalences, and even though I set it up to run restorecon the contexts are still wrong until the second run...

@oranenj oranenj force-pushed the selinux_fcontext_type branch from 09225ba to 88e249c Compare January 22, 2017 13:30
@oranenj
Copy link
Contributor Author

oranenj commented Jan 22, 2017

Acceptance test manifest passed on CentOS6

@oranenj
Copy link
Contributor Author

oranenj commented Jan 22, 2017

CentOS7 was fine too

@oranenj oranenj force-pushed the selinux_fcontext_type branch from 88e249c to 510ab20 Compare January 22, 2017 14:15
@oranenj oranenj force-pushed the selinux_fcontext_type branch 3 times, most recently from 87bd168 to 27e2e7b Compare January 28, 2017 09:40
@oranenj
Copy link
Contributor Author

oranenj commented Jan 28, 2017

hm, now I wonder how to get the tests to pass considering the provider requires the selinux module which is not available. For the testing it is enough to mock the existence of the module, but the provider still does require 'selinux' which seems to not work.

EDIT: hm, looks like I don't need to explicitly require the selinux module for it to work. I guess Puppet does that for me.

@oranenj oranenj force-pushed the selinux_fcontext_type branch 2 times, most recently from 2188e14 to 2464ee5 Compare January 28, 2017 14:12
@vinzent
Copy link
Contributor

vinzent commented Jan 30, 2017

seems like the file_context.local is not present with a default install:

       	Error: /Stage[main]/Main/Resources[selinux_fcontext]: Failed to generate additional resources using 'generate': No such file or directory @ rb_sysopen - /etc/selinux/targeted/contexts/files/file_contexts.local
       	Error: Failed to apply catalog: No such file or directory @ rb_sysopen - /etc/selinux/targeted/contexts/files/file_contexts.local

seen with centos-7-x64 (7.3), centos-6-64 (centos project upstream vagrant boxes)

@vinzent
Copy link
Contributor

vinzent commented Jan 31, 2017

merged your change of yesterday, centos-6 now complains about file_contexts.subs:

       	Error: /Stage[main]/Main/Resources[selinux_fcontext_equivalence]: Failed to generate additional resources using 'generate': No such file or directory @ rb_sysopen - /etc/selinux/targeted/contexts/files/file_contexts.subs
       	Error: Failed to apply catalog: No such file or directory @ rb_sysopen - /etc/selinux/targeted/contexts/files/file_contexts.subs

@vinzent
Copy link
Contributor

vinzent commented Jan 31, 2017

with this patch it works for me:

diff --git a/lib/puppet/provider/selinux_fcontext_equivalence/semanage.rb b/lib/puppet/provider/selinux_fcontext_equivalence/semanage.rb
index 9232e99..cf6613a 100644
--- a/lib/puppet/provider/selinux_fcontext_equivalence/semanage.rb
+++ b/lib/puppet/provider/selinux_fcontext_equivalence/semanage.rb
@@ -22,9 +22,10 @@ Puppet::Type.type(:selinux_fcontext_equivalence).provide(:semanage) do
   end
 
   def self.instances
-    # Allow this to fail with an exception if it does not exist
     lines = File.read(Selinux.selinux_file_context_subs_path).split("\n")
     parse_fcontext_subs_lines(lines)
+  rescue Errno::ENOENT
+    return []
   end
 
   def self.prefetch(resources)

@oranenj oranenj force-pushed the selinux_fcontext_type branch from 156f921 to 13ed94c Compare January 31, 2017 16:36
@oranenj
Copy link
Contributor Author

oranenj commented Jan 31, 2017

I implemented a slightly different version of the same patch along with spec tests and rebased

@oranenj oranenj force-pushed the selinux_fcontext_type branch from 13ed94c to 89172d1 Compare January 31, 2017 17:43
Copy link
Contributor

@vinzent vinzent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

successfully ran acceptance tests on centos 6, centos 7, fedora 24 and fedora 25.

@vinzent vinzent merged commit adb8cd4 into voxpupuli:master Jan 31, 2017
@vinzent vinzent changed the title Add selinux_fcontext and selinux_fcontext_equivalence types BREAKING: Add selinux_fcontext and selinux_fcontext_equivalence types Mar 29, 2017
EmRowlands pushed a commit to EmRowlands/puppet-selinux that referenced this pull request Mar 29, 2023
Add selinux_fcontext and selinux_fcontext_equivalence types
cegeka-jenkins pushed a commit to cegeka/puppet-selinux that referenced this pull request Jan 10, 2025
Add selinux_fcontext and selinux_fcontext_equivalence types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants