Skip to content

Conversation

@eciii
Copy link
Contributor

@eciii eciii commented Oct 20, 2025

The default values of the SELinux-related properties of a file resource are calculated at the beginning of the catalog application phase, before the catalog resources are actually synchronized. However, by the time the file resource is synchronized, these default values might be outdated due to new policy rules being added in previous parts of the catalog.

This behavior can be easily reproduced in Rocky Linux 9 and probably other RHEL-like distros (maybe even Fedora):

$ git checkout main
[...]

$ bundle exec puppet apply -e "package { 'grafana': ensure => present } file { '/var/lib/grafana/test': ensure => file }"
Notice: Compiled catalog for packer-base-rocky9.afdata.local in environment production in 0.37 seconds
Notice: /Stage[main]/Main/Package[grafana]/ensure: created
Notice: /Stage[main]/Main/File[/var/lib/grafana/test]/ensure: created
Notice: Applied catalog in 27.52 seconds

$ ls -lhAZ /var/lib/grafana/
total 0
-rw-r-----. 1 grafana grafana system_u:object_r:grafana_var_lib_t:s0  0 Oct 16 16:55 grafana.db
drwxr-xr-x. 2 grafana grafana system_u:object_r:grafana_var_lib_t:s0 40 Oct 20 14:47 plugins
-rw-r--r--. 1 root    root    system_u:object_r:var_lib_t:s0          0 Oct 20 14:47 test

The test file is created with the wrong SELinux context because:

  • The default value of the seltype property is calculated before the grafana package is installed.
  • The grafana package is then installed, pulling in the grafana-selinux package, which in turn creates the grafana_var_lib_t type.
  • The test file is created using an outdated SELinux type.

This PR fixes the described behavior:

$ rm -f /var/lib/grafana/test && dnf remove -y grafana
[...]

$ git checkout fix
[...]

$ bundle exec puppet apply -e "package { 'grafana': ensure => present } file { '/var/lib/grafana/test': ensure => file }"
Notice: Compiled catalog for packer-base-rocky9.afdata.local in environment production in 0.38 seconds
Notice: /Stage[main]/Main/Package[grafana]/ensure: created
Notice: /Stage[main]/Main/File[/var/lib/grafana/test]/ensure: created
Notice: Applied catalog in 28.41 seconds

$ ls -lhAZ /var/lib/grafana/
total 0
-rw-r-----. 1 grafana grafana system_u:object_r:grafana_var_lib_t:s0  0 Oct 16 16:55 grafana.db
drwxr-xr-x. 2 grafana grafana system_u:object_r:grafana_var_lib_t:s0 40 Oct 20 14:49 plugins
-rw-r--r--. 1 root    root    system_u:object_r:grafana_var_lib_t:s0  0 Oct 20 14:49 test

This PR also adjusts the corresponding unit tests accordingly. As far as I cat tell, all the relevant tests pass:

$ SPEC=spec/unit/type/file/selinux_spec.rb bundle exec rake spec
rspec spec/unit/type/file/selinux_spec.rb
Run options: exclude {benchmark: true}
....................................

Finished in 0.37257 seconds (files took 0.93635 seconds to load)
36 examples, 0 failures

More details about the actual changes can be found in the commit description.

@bastelfreak
Copy link
Contributor

@eciii thanks for the PR!

@alexjfisher is that something you can take a look? I think you debugged various selinux issues with the file resource in the past?


@event = :file_changed
defaultto { retrieve_default_context(:seluser) }
defaultto { Puppet::Util::Platform.windows? || @resource[:selinux_ignore_defaults] == :true ? nil : :lookup }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like repeating this line. And it's too complex to be a one-liner.

I don't think it's correct either. Previously on Windows, this would be nil and now it's true.

@bastelfreak
Copy link
Contributor

@eciii can you rebase against our latest main branch please?

@eciii
Copy link
Contributor Author

eciii commented Dec 12, 2025

@bastelfreak I'm trying to address the failures reported by the test suite and found some issues in this PR that I'm trying to improve. Unfortunately I have stumbled with several problems when trying to run the full test suite locally. I'll try to get this PR in good shape again soon, rebase it to the latest main branch and address the comments made by @binford2k . In the meantime I opened a separate PR to address one of the problems regarding the test suite.

@eciii eciii force-pushed the fix-selinux-properties-of-file-resource branch from 7a669d4 to 04b8c3a Compare January 8, 2026 12:48
@eciii
Copy link
Contributor Author

eciii commented Jan 8, 2026

@bastelfreak @binford2k I was able to get this commit into good shape, at least in my opinion. I made some small changes with respect with the previous attempt. In particular the skip_property? method is introduced to simplify the one-liner that calculates the default value (as Ben suggested).

I also ran the full test suite locally on Ubuntu 24.04 LTS and on CentOS Stream 10. This allowed me to catch one bug in the actual proposed solution (catch nil returns of retrieve_default_context in the insync? method) but also in the tests themselves. In particular:

  • There was a problem with a test that checks if the selabel_handle is closed during transaction teardown. I adjusted this test so that it works but I think the code can be simplified in such a way that this test is made irrelevant. I'll try to put this in a separate PR later.
  • Two other tests were failing in CentOS Stream 10 because the test suite runs with SELinux actually enabled there. The easiest solution was to explicitly disable SELinux for those particular tests. These tests don't fail in GitHub because no RHEL-like distro is used there.

I'll fix the rubocop issue so that the GitHub tests are successful.

eciii added 2 commits January 8, 2026 15:03
The default values of the SELinux-related properties of a file resource are
calculated at the beginning of the catalog application phase, before the
catalog resources are actually synchronized. However, by the time the file
resource is synchronized, these default values might be outdated due to new
policy rules being added in previous parts of the catalog (for example, a
package was installed that included new policy rules that affect the file
currently being created by the file resource.). On these cases the file created
by the file resource ends up with the wrong SELinux context.

This commit delays the lookup of the default values until the very
moment the file resource is being synchronized. More precisely, the
following process is implemented:

- A `:lookup` symbol is introduced as the default value. It means that
  the actual value is to be looked up later during synchronization.
  However, maintaining the previous behaviour, a default value of `nil` is used
  if either the platform doesn't support SELinux (e.g Windows, or Ubuntu by
  default) or the user explicitly stated to ignore the default values (using
  the `selinux_ignore_defaults` parameter).
- During resource synchronization, when the `insync?` method is
  executed, the `:lookup` symbol is replaced with the actual looked up
  value and then the sync status is calculated as usual.

The unit tests have been adjusted accordingly the best I could but a somewhat
larger improvement/refactoring of these tests is still needed (and hopefully
will be made later) to get them into good shape.
After the introduction of the `:lookup` symbol as the default value of the
SELinux-related properties of the file resource, some tests now fail because
they expect the file resource to not have properties with such behaviour. This
commit disables SELinux for such tests. Note that the failures occur only when
the tests are executed on systems with SELinux enabled (e.g CentOS).
@eciii eciii force-pushed the fix-selinux-properties-of-file-resource branch from 04b8c3a to a2b7afa Compare January 8, 2026 14:04
@bastelfreak
Copy link
Contributor

The code looks okay to me, but I'm really not an selinux expert

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants