lint: warn about potential UID/GID drift under /etc; respect tmpfiles chown (z/Z) (#1562) #1570
Conversation
…coverage (z/Z) (bootc-dev#1562) Signed-off-by: Rishi Jat <rishijat098@gmail.com>
rishi-jat
force-pushed
the
fix/1562-uid-drift-lint
branch
from
947bdf2 to
488a48b
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable new lint to detect potential UID/GID drift in /etc, which is a great addition for ensuring system integrity across upgrades. The implementation logic for parsing sysusers.d and tmpfiles.d files and checking file ownership is sound. I've provided a few suggestions for the new helper functions in crates/tmpfiles/src/lib.rs to improve code clarity and maintainability by removing dead code and using more idiomatic Rust. Overall, this is a solid contribution.
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
| // Depth-first walk under /etc | ||
| let mut stack: Vec<(Dir, std::path::PathBuf)> = vec![(etcd, std::path::PathBuf::from("/etc"))]; | ||
| while let Some((dir, abspath)) = stack.pop() { |
There was a problem hiding this comment.
This is probably OK as is but I've been trying to have recursive filesystem walks use https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.walk
| root.create_dir_all("etc/uncovered")?; | ||
|
|
||
| let config = &LintExecutionConfig { no_truncate: true }; | ||
| // Since we cannot alter uid/gid in this test easily, fake non-root by relying on the logic |
There was a problem hiding this comment.
Right...but we are going to need some "real" tests for this.
is one possible place - that test suite is defined to run as root and so it's supported to make tempdirs with arbitrary owned files.Another (which is perhaps overdue) is one where we have Dockerfile fixtures and we explicitly just build each one and run bootc container lint in there and check the result.
| // Only process .conf files | ||
| let name = entry.file_name(); | ||
| let path = Path::new(&name); | ||
| if path.extension() != Some(OsStr::new("conf")) { |
There was a problem hiding this comment.
I think it'd be a lot cleaner to have this run read_tmpfiles() and then just filter the result.
| if kind != 'z' && kind != 'Z' { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I think it'd be cleaner to just fold this into the match
| impl TmpfilesChowners { | ||
| /// Returns true if a chown entry would apply to the specified absolute path | ||
| pub fn covers(&self, p: &Path) -> bool { | ||
| self.exact.contains(p) || p.ancestors().any(|anc| self.recursive.contains(anc)) |
There was a problem hiding this comment.
I think we could have some unit tests for this pretty easily
Description
This adds a new container lint (etc-uid-drift) that warns when files or directories under /etc are owned by non-root users or groups that aren’t numerically pinned in systemd-sysusers and aren’t covered by systemd-tmpfiles chown rules. The goal is to help catch ownership that can “drift” across upgrades when /etc persists.
The lint parses usr/lib/sysusers.d to find users/groups with fixed numeric IDs, and scans usr/lib/tmpfiles.d for z/Z entries (chown at boot, either exact or recursive). It walks /etc (skipping symlinks, handling non‑UTF8 paths), and warns on entries where uid/gid is non-root, not pinned via sysusers, and not covered by a tmpfiles chown. For example, if a component uses a floating user but ships a tmpfiles rule like Z /etc/polkit-1/…, the lint will not warn.
This is a warning-only check (can be promoted to fatal with --fatal-warnings) and does not change runtime behavior.
Fixes #1562