Skip to content

utils: SELinux file context #101

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

Merged
merged 1 commit into from
May 12, 2025
Merged

Conversation

ikerexxe
Copy link
Contributor

Implement SELinux file context class and LinuxFileSystem method.

@@ -230,6 +231,23 @@ def exists(self, path: str) -> bool:

return False

def selinux_context(self, path: str) -> SELinuxContext | None:
Copy link
Collaborator

@jakub-vavra-cz jakub-vavra-cz Apr 24, 2025

Choose a reason for hiding this comment

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

If this can be None, we will always need to run two asserts otherwise static analysis will be sad:
assert x in not None, "Could not detect selinux context"
assert x.type == "foo_t", "Context does not match"

I would rather see the object of the type SELinuxContext that has "bogus values" that You can just directly compare instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def selinux_context(self, path: str) -> SELinuxContext | None:
def selinux_context(self, path: str) -> SELinuxContext:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning both SELinuxContext or None is generally preferred in Python for its clarity and flexibility in handling the potential absence of a value. It could happen that the command didn't run successfully and we don't get any SELinux context, thus checking for it is the only way of getting the proper verbosity output in the logs. I could also return an exception, but that wouldn't be as clear as None.

Comment on lines 69 to 101
else:
return None
else:
return None
Copy link
Collaborator

@jakub-vavra-cz jakub-vavra-cz Apr 24, 2025

Choose a reason for hiding this comment

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

Suggested change
else:
return None
else:
return None
return cls("Not found","Not found","Not found","Not found","Not found")

Comment on lines 246 to 248
if result.rc != 0:
return None

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if result.rc != 0:
return None

Copy link
Collaborator

@jakub-vavra-cz jakub-vavra-cz left a comment

Choose a reason for hiding this comment

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

See inline.

@ikerexxe ikerexxe force-pushed the selinux-context branch 2 times, most recently from 244f636 to 5c5da56 Compare April 25, 2025 14:42
@alexey-tikhonov
Copy link
Collaborator

How is this going to be used?

@ikerexxe ikerexxe force-pushed the selinux-context branch from 5c5da56 to beedbc3 Compare May 2, 2025 10:39
@ikerexxe
Copy link
Contributor Author

ikerexxe commented May 2, 2025

This will be used by shadow project to check the correct labeling of SELinux files. More specifically the issue that triggered this development is shadow-maint/shadow#940.

I thought this might be useful for other projects that use pytest-mh and its file processing utilities, and that's why I decided to implement it in pytest-mh instead of in the shadow test framework.

@ikerexxe ikerexxe force-pushed the selinux-context branch from beedbc3 to b236b05 Compare May 2, 2025 10:49
Copy link
Contributor

@pbrezina pbrezina left a comment

Choose a reason for hiding this comment

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

How about creating standalone SELinux utility and implementing the method there instead of mixing this in fs.py?

@ikerexxe
Copy link
Contributor Author

ikerexxe commented May 5, 2025

I'm fine with that, but apart from checking SELinux file context labels I wouldn't like to implement anything else for now. Would you be okay with such a simple class?

@pbrezina
Copy link
Contributor

pbrezina commented May 6, 2025

I'm fine with that, but apart from checking SELinux file context labels I wouldn't like to implement anything else for now. Would you be okay with such a simple class?

Yes, at least we will have a placeholder for more selinux code in the future.

@ikerexxe ikerexxe force-pushed the selinux-context branch from 5c13ad8 to 88cfdc6 Compare May 7, 2025 09:42
@ikerexxe
Copy link
Contributor Author

ikerexxe commented May 7, 2025

Yes, at least we will have a placeholder for more selinux code in the future.

It's very simple but it's done.

Copy link
Collaborator

@jakub-vavra-cz jakub-vavra-cz left a comment

Choose a reason for hiding this comment

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

See inline.
I do not want exception when comparing the SELinuxContext to None.

Implement SELinux and SELinux file context classes to check the SELinux
context of a file.

Signed-off-by: Iker Pedrosa <[email protected]>
@ikerexxe ikerexxe merged commit 10584e2 into next-actions:master May 12, 2025
4 checks passed
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.

4 participants