[BUG] selinux.fcontext_policy_is_applied doesn't check for errors, leading to silent failures to apply SELinux labels

[iaingeorgeson]

Description _selinux.fcontext_policy_isapplied calls "restorecon -n -v" without checking for success. It returns lines of stdout which indicate actions restorecon would take, but when the command fails there are no lines of stdout, This means when you call it with a non-existent file, it will respond that the file is already correctly relabelled.

This function is called by the _selinux.fcontext_policyapplied state, which silently fails when given a non-existent name.

The documentation states that the function supports regex syntax. This is potentially confusing. It supports shell globs, but not the regex syntax understood by "semanage fcontext". Using the same regex in _selinux.fcontext_policypresent and _selinux.fcontext_policyapplied will silently fail.

Setup

letsencrypt-well-known:
  selinux.fcontext_policy_present:
    - name: "/srv/letsencrypt(/.*)?"
    - sel_type: "httpd_sys_content_t"
letsencrypt-apply:
  selinux.fcontext_policy_applied:
    - name: "/srv/letsencrypt(/.*)?"
    - require:
      - letsencrypt-well-known

Steps to Reproduce the behavior Apply above.

Expected behavior /srv/letsencrypt/ is correctly relabelled.

Instead, the fcontext spec is correctly added, but restorecon is not run so the new label is not applied.

Versions Report Salt Version: Salt: 3003.1

Dependency Versions: cffi: Not Installed cherrypy: unknown dateutil: 2.7.3 docker-py: Not Installed gitdb: 2.0.5 gitpython: 2.1.11 Jinja2: 2.10 libgit2: Not Installed M2Crypto: 0.31.0 Mako: Not Installed msgpack: 0.5.6 msgpack-pure: Not Installed mysql-python: Not Installed pycparser: Not Installed pycrypto: 2.6.1 pycryptodome: 3.6.1 pygit2: Not Installed Python: 3.7.3 (default, Jan 22 2021, 20:04:44) python-gnupg: Not Installed PyYAML: 3.13 PyZMQ: 17.1.2 smmap: 2.0.5 timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.3.1

System Versions: dist: debian 10 buster locale: UTF-8 machine: x86_64 release: 4.19.0-17-cloud-amd64 system: Linux version: Debian GNU/Linux 10 buster

Additional context Add any other context about the problem here.

[leeclemens]

Hi @iaingeorgeson Thanks for reporting this. I'd like to look into it, but wanted to make sure I understand the issue. From your Expected Behavior, you expect it to be relabeled. However the base issue seems to be that an error is not reported when the directory/file being relabeled does not exist. Is that correct? It won't be able to relabel something that does not exist, but I can see the point that it should report that error. Would it be accurate to say your Expected Behavior is an error is reported when relabeling a file that does not exist?

I have a number of states using "(/.*)?" and have not seen an issue. However, I am not quoting it. If you are seeing an issue with the - name syntax, can you try not quoting them and see if that behaves differently?

[iaingeorgeson]

Thanks for investigating. I think there are two issues, which I didn't state terribly clearly.

The first is that the documentation suggests that the correct behaviour is that the file will be relabelled when this syntax is used. However after testing, it appears that restorecon cannot interpret that syntax, and so the documentation should be clarified.

Because of this, the call to restorecon fails, and the second issue is that this error isn't reported, so it appears that the call was successful.

I verified the behaviour with the "(/.)?" syntax by running "restorecon -n -v '/srv/letsencrypt(/.)?'" directly on the minion, which is what the underlying code does.

https://github.com/saltstack/salt/blob/master/salt/modules/selinux.py#L648

Thanks,

Iain.

[leeclemens]

Thanks, I see what you are saying now. I'm wondering if some of my states were 'working' because something else relabeled them.

I imagine this would work as expected? (not discounting the need for some fixes/updates.)

letsencrypt-apply:
  selinux.fcontext_policy_applied:
    - name: "/srv/letsencrypt"
    - recursive: True
    - require:
      - letsencrypt-well-known

[leeclemens]

For reference: module.fcontext_policy_is_applied is called multiple times: Directly from state.fcontext_apply_policy, which calls module.fcontext_apply_policy which then calls it again.

I removed the duplicate call to module.fcontext_policy_is_applied from module.fcontext_apply_policy since it seems redundant and the output is never checked until running the second restorecon command regardless of its return.

I'm thinking the call to module.fcontext_policy_is_applied could be removed from state.fcontext_policy_applied as well, I'm not sure what the performance benefit is from checking if the policy is applied vs attempting to apply it and processing the result (if any).

[leeclemens]

I ran restorecon with ~20 million small (10 bytes) files:

passive check (no changes), apply changes (none required):

restorecon -nR .

real    6m57.840s
user    5m26.534s
sys 1m28.156s

restorecon -FR .

real    7m1.123s
user    5m30.990s
sys 1m27.021s

apply changes (all required), passive check (no changes):

restorecon -nR .

real    4m17.861s
user    2m54.621s
sys 1m21.471s

restorecon -FR .

real    8m0.741s
user    2m49.402s
sys 3m35.137s

restorecon -nR .

real    4m0.329s
user    2m36.024s
sys 1m22.580s