sefcontext: add support for path substitutions #5830

bluikko · 2023-01-13T14:05:28Z

SUMMARY

Fixes #1193.

Add support for path substitution mappings in the sefcontext module.

I would like to get feedback on this architecture that is quite a bit different from another (dead) PR #5189:

Don't use new functions just for adding/deleting path substitutions. Modify the existing functions.
How deletion is done has minor change - current playbooks continue to work as before:
- Don't require the setype argument with state=absent - it is not really required for deletion and it was not even used in the old code.
- If neither setype or substitute arg passed: delete both path substitutions and regular context mappings for target if either was found when deletion requested.
- If substitute was passed with state=absent then delete only that equivalence if found (don't delete regular context mappings); if not found then return unchanged, if found but target substitutes some other path then return unchanged.
- If setype was passed with state=absent then delete only existing context mappings (don't delete path substitutions), don't care about the setype matching the current mapping to delete (this is the old behavior which seems a bit loose).

Fixes #4564.

Change the incorrect setype in documentation example to a working label.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

sefcontext

ADDITIONAL INFORMATION

Feedback please! It is the largest code change I have done yet so I really need feedback.

Should adding/deleting path substitutions be moved to their own functions instead? Does it matter?
Should deletion behavior be more strict?
- Consider example sefcontext setype=tmp_t target=/opt/tmp state=absent: what if /opt/tmp is currently not mapped to tmp_t, say it's mapped to opt_t - in this case should the module just return changed=false? Or should it delete the mapping anyways? The old behavior is the latter and I've kept that unchanged. Personally I feel that it is wrong and would like to hear feedback on this.
The arg name substitute? I couldn't come up with a great name for it. The man page for semanage-fcontext talks about "path substitution" and the switch -e for this is called "equals". Better args names welcome: current comments seem to support substitute but there is also request to add equal as an alias (I would hesitate, perhaps unnecessarily, to add an alias to a new feature).

Also adds "attributes" block to documentation & some minor documentation improvements.

ansibullbot · 2023-01-13T14:08:23Z

cc @dagwieers
click here for bot help

bluikko · 2023-01-14T08:49:38Z

I apologize for not marking this PR as WIP in the beginning.

It would be ready now. Feedback would be appreciated!

plugins/modules/sefcontext.py

changelogs/fragments/5830-sefcontext-path-subs.yml

plugins/modules/sefcontext.py

russoz

hi @bluikko thanks for the contribution!

I got a couple of comments on some points of the PR. Thanks!

plugins/modules/sefcontext.py

tests/integration/targets/sefcontext/tasks/sefcontext.yml

bluikko · 2023-01-16T10:00:37Z

hi @bluikko thanks for the contribution!

I got a couple of comments on some points of the PR. Thanks!

Very much appreciated! But please excuse me, I had to ask for clarification.
In general: good stuff, will incorporate your feedback, absolutely.

bluikko · 2023-01-19T05:36:17Z

CI fails until #5837 merged.

Change arg name from equal to substitute. Print target = subsitute in diff mode same way as semanage does. Also put back platform attribute, try to improve clumsy language in the substitute arg docs.

Test 5 indicates that deletion is supposed to not check that the arg setype passed when deleting matches the setype of the mapping to delete. Delete any mapping that matches target, regardless of setype arg value.

Accidentally replaced seobject function names so fix them back

Change from httpd_git_rw_content_t which does not exist to httpd_sys_rw_content_t Fixes ansible-collections#4564

Additional fragment Co-authored-by: Felix Fontein <felix@fontein.de>

Bumping minor to 6.4.0 since it didn't make 6.3.0.

Try to improve discoverability of the new feature and make it easier to understand without deep SELinux understanding.

plugins/modules/sefcontext.py

russoz · 2023-02-25T02:43:52Z

Well, I had reviewed this PR a while ago, and the recent improvements continue to look good to me. As of the parameter name, I agree that if equivalence or equal are established in the context of SELinux, than by all means add aliases, or reverse back to equal. For someone not familiar to that it seems odd to read r.equal == something.

That being said, it LGTM.

Improve discoverability of the new feature by adding an alias to the new module argument. The argument name "equal" will be easy to find for users who are not familiar with SELinux and who just try to match to the CLI tool `semanage`.

bluikko · 2023-02-25T09:34:12Z

than by all means add aliases

I added alias equal for the new argument since that matches with the semanage CLI tool. I do like the substitute argument name since that matches so well with what the argument actually does, and the help text.

Previous commit missed actually adding the alias (added to docs only).

felixfontein

Looks good as far as I can tell. (I haven't done a thorough review though and I'm not using selinux myself.)

patchback · 2023-02-26T13:05:00Z

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/c8a2ac3a475ab490ad00f1db1a6197b108c66413/pr-5830

Backported as #6098

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* sefcontext: add path substitution support (#1193) First commit for feedback, missing docs and tests. * sefcontext: add documentation * Add changelog fragment * Documentation formatting * Delete extra newline * pep8 fixes Fix indentation * Add version_added to arg docs * Add examples * Don't delete non-matching path substitutions * Add integration tests * Delete only substitutions if such arg passed Don't delete existing regular file context mappings if deletion of a path substitution was requested with the presence of the `equal` arg - delete only path substitutions in such case. Path substitutions and regular mappings may overlap. * Can only add args in minor releases :( * Cleanup before tests * Fix deletion using substitution Was comparing wrong var. * Fix test checking wrong var * Improve args documentation and examples List the default values for selevel, seuser. Add example for deleting path substitutions only. * Add attributes documentation block Not sure if should add become/delegate/async, shouldn't those work just like that without any specific code added for them? * and fix indentation on attribute block * Consistent indentation for attributes Confusing, most plugins indent with 4 spaces. But some use 2 like the rest of the code, so use 2. * Add missing ref for attribute block * Use correct c.g version in doc block Co-authored-by: Felix Fontein <felix@fontein.de> * Add full stop to changelog fragment Co-authored-by: Felix Fontein <felix@fontein.de> * Streamline documentation Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> * Support limiting deletion to setype Deleting file context mappings may be limited by passing setype or equal, if neither arg is passed then delete either setype/equal mappings that match. * Change arg name, diff mode output fix Change arg name from equal to substitute. Print target = subsitute in diff mode same way as semanage does. Also put back platform attribute, try to improve clumsy language in the substitute arg docs. * Delete even if arg setype not match existing Test 5 indicates that deletion is supposed to not check that the arg setype passed when deleting matches the setype of the mapping to delete. Delete any mapping that matches target, regardless of setype arg value. * Update arg name in tests * Too eager replacing Accidentally replaced seobject function names so fix them back * 4564: Fix invalid setype in doc example Change from httpd_git_rw_content_t which does not exist to httpd_sys_rw_content_t Fixes #4564 * Fix documentation attributes Additional fragment Co-authored-by: Felix Fontein <felix@fontein.de> * Update version_added in docs Bumping minor to 6.4.0 since it didn't make 6.3.0. * Add more description to the new arg docs Try to improve discoverability of the new feature and make it easier to understand without deep SELinux understanding. * Update platform to Linux in documentation * Add equal as alias for the new argument Improve discoverability of the new feature by adding an alias to the new module argument. The argument name "equal" will be easy to find for users who are not familiar with SELinux and who just try to match to the CLI tool `semanage`. * And add alias argument properly Previous commit missed actually adding the alias (added to docs only). --------- Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> (cherry picked from commit c8a2ac3)

felixfontein · 2023-02-26T13:05:50Z

@bluikko thanks a lot for improving this module!
@nmaludy @russoz thanks for reviewing and commenting on this!

…th substitutions (#6098) sefcontext: add support for path substitutions (#5830) * sefcontext: add path substitution support (#1193) First commit for feedback, missing docs and tests. * sefcontext: add documentation * Add changelog fragment * Documentation formatting * Delete extra newline * pep8 fixes Fix indentation * Add version_added to arg docs * Add examples * Don't delete non-matching path substitutions * Add integration tests * Delete only substitutions if such arg passed Don't delete existing regular file context mappings if deletion of a path substitution was requested with the presence of the `equal` arg - delete only path substitutions in such case. Path substitutions and regular mappings may overlap. * Can only add args in minor releases :( * Cleanup before tests * Fix deletion using substitution Was comparing wrong var. * Fix test checking wrong var * Improve args documentation and examples List the default values for selevel, seuser. Add example for deleting path substitutions only. * Add attributes documentation block Not sure if should add become/delegate/async, shouldn't those work just like that without any specific code added for them? * and fix indentation on attribute block * Consistent indentation for attributes Confusing, most plugins indent with 4 spaces. But some use 2 like the rest of the code, so use 2. * Add missing ref for attribute block * Use correct c.g version in doc block Co-authored-by: Felix Fontein <felix@fontein.de> * Add full stop to changelog fragment Co-authored-by: Felix Fontein <felix@fontein.de> * Streamline documentation Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> * Support limiting deletion to setype Deleting file context mappings may be limited by passing setype or equal, if neither arg is passed then delete either setype/equal mappings that match. * Change arg name, diff mode output fix Change arg name from equal to substitute. Print target = subsitute in diff mode same way as semanage does. Also put back platform attribute, try to improve clumsy language in the substitute arg docs. * Delete even if arg setype not match existing Test 5 indicates that deletion is supposed to not check that the arg setype passed when deleting matches the setype of the mapping to delete. Delete any mapping that matches target, regardless of setype arg value. * Update arg name in tests * Too eager replacing Accidentally replaced seobject function names so fix them back * 4564: Fix invalid setype in doc example Change from httpd_git_rw_content_t which does not exist to httpd_sys_rw_content_t Fixes #4564 * Fix documentation attributes Additional fragment Co-authored-by: Felix Fontein <felix@fontein.de> * Update version_added in docs Bumping minor to 6.4.0 since it didn't make 6.3.0. * Add more description to the new arg docs Try to improve discoverability of the new feature and make it easier to understand without deep SELinux understanding. * Update platform to Linux in documentation * Add equal as alias for the new argument Improve discoverability of the new feature by adding an alias to the new module argument. The argument name "equal" will be easy to find for users who are not familiar with SELinux and who just try to match to the CLI tool `semanage`. * And add alias argument properly Previous commit missed actually adding the alias (added to docs only). --------- Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> (cherry picked from commit c8a2ac3) Co-authored-by: bluikko <14869000+bluikko@users.noreply.github.com>

ansibullbot added feature This issue/PR relates to a feature request module module plugins plugin (any type) system labels Jan 13, 2023

This comment was marked as outdated.

Sign in to view

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 13, 2023

This comment was marked as outdated.

Sign in to view

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-6 labels Jan 13, 2023

This comment was marked as outdated.

Sign in to view

ansibullbot added integration tests/integration tests tests and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 14, 2023

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed ci_verified Push fixes to PR branch to re-run CI labels Jan 14, 2023

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI and removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 14, 2023

felixfontein reviewed Jan 14, 2023

View reviewed changes

plugins/modules/sefcontext.py Outdated Show resolved Hide resolved

felixfontein reviewed Jan 14, 2023

View reviewed changes

changelogs/fragments/5830-sefcontext-path-subs.yml Outdated Show resolved Hide resolved

plugins/modules/sefcontext.py Outdated Show resolved Hide resolved

russoz reviewed Jan 16, 2023

View reviewed changes

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 19, 2023

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jan 20, 2023

bluikko and others added 8 commits February 21, 2023 12:00

Change arg name, diff mode output fix

f6f299c

Change arg name from equal to substitute. Print target = subsitute in diff mode same way as semanage does. Also put back platform attribute, try to improve clumsy language in the substitute arg docs.

Delete even if arg setype not match existing

bfde46c

Test 5 indicates that deletion is supposed to not check that the arg setype passed when deleting matches the setype of the mapping to delete. Delete any mapping that matches target, regardless of setype arg value.

Update arg name in tests

a7f6ccb

Too eager replacing

80e604a

Accidentally replaced seobject function names so fix them back

4564: Fix invalid setype in doc example

c217a95

Change from httpd_git_rw_content_t which does not exist to httpd_sys_rw_content_t Fixes ansible-collections#4564

Fix documentation attributes

03c9901

Additional fragment Co-authored-by: Felix Fontein <felix@fontein.de>

Update version_added in docs

b4d2aec

Bumping minor to 6.4.0 since it didn't make 6.3.0.

Add more description to the new arg docs

b73deee

Try to improve discoverability of the new feature and make it easier to understand without deep SELinux understanding.

bluikko force-pushed the bluikko-1193-sefcontext branch from e6b138b to b73deee Compare February 21, 2023 05:04

ansibullbot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Feb 21, 2023

felixfontein reviewed Feb 22, 2023

View reviewed changes

plugins/modules/sefcontext.py Outdated Show resolved Hide resolved

Update platform to Linux in documentation

5c5a222

Add equal as alias for the new argument

2da462d

Improve discoverability of the new feature by adding an alias to the new module argument. The argument name "equal" will be easy to find for users who are not familiar with SELinux and who just try to match to the CLI tool `semanage`.

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 25, 2023

And add alias argument properly

6114aba

Previous commit missed actually adding the alias (added to docs only).

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 25, 2023

felixfontein approved these changes Feb 25, 2023

View reviewed changes

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 26, 2023

felixfontein merged commit c8a2ac3 into ansible-collections:main Feb 26, 2023

patchback bot mentioned this pull request Feb 26, 2023

[PR #5830/c8a2ac3a backport][stable-6] sefcontext: add support for path substitutions #6098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sefcontext: add support for path substitutions #5830

sefcontext: add support for path substitutions #5830

bluikko commented Jan 13, 2023 •

edited

Loading

ansibullbot commented Jan 13, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

bluikko commented Jan 14, 2023

This comment was marked as outdated.

This comment was marked as outdated.

russoz left a comment

bluikko commented Jan 16, 2023

bluikko commented Jan 19, 2023

russoz commented Feb 25, 2023

bluikko commented Feb 25, 2023

This comment was marked as outdated.

felixfontein left a comment

patchback bot commented Feb 26, 2023 •

edited

Loading

felixfontein commented Feb 26, 2023

sefcontext: add support for path substitutions #5830

sefcontext: add support for path substitutions #5830

Conversation

bluikko commented Jan 13, 2023 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Jan 13, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

bluikko commented Jan 14, 2023

This comment was marked as outdated.

This comment was marked as outdated.

russoz left a comment

Choose a reason for hiding this comment

bluikko commented Jan 16, 2023

bluikko commented Jan 19, 2023

russoz commented Feb 25, 2023

bluikko commented Feb 25, 2023

This comment was marked as outdated.

felixfontein left a comment

Choose a reason for hiding this comment

patchback bot commented Feb 26, 2023 • edited Loading

Backport to stable-6: 💚 backport PR created

felixfontein commented Feb 26, 2023

bluikko commented Jan 13, 2023 •

edited

Loading

patchback bot commented Feb 26, 2023 •

edited

Loading