Skip to content

Implement -context#540

Open
fu050409 wants to merge 7 commits into
uutils:mainfrom
fu050409:feat/implement-context
Open

Implement -context#540
fu050409 wants to merge 7 commits into
uutils:mainfrom
fu050409:feat/implement-context

Conversation

@fu050409
Copy link
Copy Markdown

@fu050409 fu050409 commented Apr 26, 2025

Closes #375

To avoid using C bindings, I wrote the detection logic for SELinux in Rust by referring to libselinux, and used xattr crate to capture the SELinux extension for matching during the matching stage. xattr only supported on unix platforms so it was marked as [target.'cfg(target_os = "linux")'.dependencies].

I'm still looking for a suitable way to write the test.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 26, 2025

Codecov Report

❌ Patch coverage is 1.13636% with 87 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.62%. Comparing base (575bf0a) to head (d983538).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/find/matchers/context.rs 0.00% 82 Missing ⚠️
src/find/matchers/mod.rs 16.66% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
- Coverage   91.85%   90.62%   -1.23%     
==========================================
  Files          31       32       +1     
  Lines        6420     6508      +88     
  Branches      338      354      +16     
==========================================
+ Hits         5897     5898       +1     
- Misses        397      484      +87     
  Partials      126      126              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fu050409 fu050409 force-pushed the feat/implement-context branch from 908a6ac to 8006d1d Compare April 26, 2025 12:24
@sylvestre
Copy link
Copy Markdown
Contributor

sylvestre commented Apr 26, 2025

looks like you missed my comment here: #375 (comment) :(

@sylvestre
Copy link
Copy Markdown
Contributor

https://github.com/uutils/coreutils/blob/main/src/uucore/src/lib/features/selinux.rs i have been working on this and to use it on findutils

also, there is that crate https://codeberg.org/koutheir/selinux
Sorry you wasted your time :(

@fu050409
Copy link
Copy Markdown
Author

looks like you missed my comment here: #375 (comment) :(

@sylvestre I feel so sorry that GitHub doesn't seem to have pushed this comment to me correctly🥺, I didn't realize you had done some work on it and I didn't deliberately ignore your efforts.

@fu050409
Copy link
Copy Markdown
Author

fu050409 commented Apr 26, 2025

also, there is that crate https://codeberg.org/koutheir/selinux Sorry you wasted your time :(

I noticed the crate as well, but I personally don't think we should try to use more c bindings in findutils, it might be more appropriate to use more pure Rust code. What do you think?

@sylvestre
Copy link
Copy Markdown
Contributor

@fu050409 i am not sure TBH. I like that crates are providing some abstractions.
In the meantime, if we could remove the dependency on a library, that would be ideal

@koutheir what is your take on this if you have one ? :)

@koutheir
Copy link
Copy Markdown

koutheir commented May 9, 2025

I noticed the crate as well, but I personally don't think we should try to use more c bindings in findutils, it might be more appropriate to use more pure Rust code. What do you think?

Short answer: either (1) use the selinux crate, or (2) move this implementation-dependent logic into its own crate.

Looking at the code, I see multiple hard-coded paths, constants, and logic, all relative to the current implementation details of SELinux; details that could change anytime in subtle ways that could break this Rust implementation. It seems like bad design to me to have that non-trivial amount of logic inside this program. In my opinion, that logic belongs inside a dedicated crate that abstracts it away from this program. Now of course, I prefer that logic to be written in Rust too, but for the moment, such a crate doesn't exist, as far as I know. The closest thing to it is a Rust crate (selinux/selinux-sys) that calls into the official C implementation (libselinux) in order to perform all that logic. On one hand, this implies that using that crate will pull two more Rust crates and one more C shared library as dependencies, but on the other hand, we are not introducing a dependency that is not already in the original C implementation of this program.

Full disclosure: I'm the maintainer of the selinux and selinux-sys crates.

@sylvestre sylvestre force-pushed the feat/implement-context branch from 1527504 to d4e084f Compare May 11, 2025 21:30
@sylvestre
Copy link
Copy Markdown
Contributor

a bunch of changes for this PR:

  1. String::from_utf8(attr) keeps the trailing NUL - security.selinux is NUL-terminated, so patterns like *:s0 won't match. Strip it: attr.strip_suffix(b"\0").unwrap_or(&attr).
  2. Gating on enforce is wrong - permissive mode (enforce==0) still has valid contexts. -context should work whenever SELinux is enabled, not enforcing.
  3. new() returning Err aborts the whole command on non-SELinux/permissive hosts (e.g. -context x -o -name y). GNU is best-effort; don't hard-fail at parse time.
  4. buf.parse::()? is fragile if /enforce has a trailing newline - use buf.trim().parse().
  5. xattr::get follows symlinks unconditionally - ignores -H/-L/-P / WalkEntry follow mode. At least add a comment.
  6. writeln!(...).unwrap() violates the no-unwrap/no-panic rule - use let _ = writeln!(…);.
  7. Use nix's typed constant nix::sys::statfs::SELINUX_MAGIC instead of FsType(nix::libc::SELINUX_MAGIC) — avoids a fs_type_t cast mismatch across targets.
  8. Tests missing (blocker) - add a parser test (-context w/ missing arg errors) and a runtime-gated integration test.

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.

Implement -context

3 participants