review: fix all findings from comprehensive code review

Security fixes:
- newgrp: add initgroups() before execv to reset supplementary groups (H-3)
- pam: use Zeroizing<String> for password buffers on all exit paths (M-5)
- atomic: retry-once on stale tmp file from crashed run (M-3)
- atomic: UmaskGuard opts out of Send/Sync via PhantomData (L-2)
- lock: flush + fsync in write_pid_file before hard_link (L-7)

Deduplication (~300 lines removed):
- Centralize caller_is_root(), current_username(), SignalBlocker in
  shadow_core::hardening — remove 10+5+5 private copies across tools
- Add SignalBlocker to all 6 remaining mutating tools (M-4)
- Remove dead CantSort variant from grpck, fix pwck allow (M-8)
- Remove dead read_passwd from test_chfn (M-8)

Binary cleanup:
- shadow-rs multicall: use ExitCode instead of process::exit (M-6)
- completions: use ExitCode + Result pattern, remove unwrap/exit (H-3)

Documentation:
- crypt: thread-safety warning on public API (H-1)
- shadow: clarify unlock() behavior for * password (H-2)
- validate: document $ rejection deviation from GNU (L-4)
- selinux/audit: note shell-out implementation status (L-6)
- chpasswd/chage tests: TODO for --prefix integration (M-8)
- userdel: document inline skip_unless_root reason (L-9)

Testing:
- Add fuzz targets for group and gshadow parsers (M-7)
- skel: use create_dir for tighter error detection (L-8)
- crypt: compile-time modulo bias assertion (L-1)
This commit is contained in:
Pierre Warnier
2026-04-03 19:27:25 +02:00
parent 046ec1de2b
commit 92783375aa
32 changed files with 363 additions and 432 deletions
+8 -5
View File
@@ -72,11 +72,14 @@ fn test_conflicting_list_and_lastday() {
// ---------------------------------------------------------------------------
// Root-only tests — exercise real operations via SysRoot prefix
// ---------------------------------------------------------------------------
// Note: these tests use SysRoot via the --root flag. Since chage uses
// SysRoot::default() (no --prefix support like passwd), we test by
// constructing the shadow file and calling chage's internal functions.
// For full integration, we'd need to chroot, which requires root.
//
// TODO(#integration): These tests directly manipulate shadow-core data
// structures instead of calling chage::uumain(). Full end-to-end integration
// via uumain() is not yet feasible because chage only supports --root (which
// performs a real chroot(2) and requires root), not --prefix (path-prefix
// without chroot). Once chage gains a --prefix flag, replace these tests with
// uumain() calls using run(&["chage", "--prefix", ..., "-m", "10", "testuser"])
// with synthetic files.
#[test]
fn test_list_output() {
-6
View File
@@ -29,12 +29,6 @@ fn setup_prefix(passwd_content: &str) -> tempfile::TempDir {
dir
}
/// Read the passwd file content back from a prefix dir.
#[allow(dead_code)]
fn read_passwd(dir: &tempfile::TempDir) -> String {
std::fs::read_to_string(dir.path().join("etc/passwd")).expect("failed to read passwd file")
}
// ---------------------------------------------------------------------------
// Non-root tests
// ---------------------------------------------------------------------------
+8
View File
@@ -55,6 +55,14 @@ fn test_invalid_crypt_method_exits_error() {
// ---------------------------------------------------------------------------
// Root-only tests — exercise real operations via shadow-core
// ---------------------------------------------------------------------------
//
// TODO(#integration): These tests directly manipulate shadow-core data
// structures instead of calling chpasswd::uumain(). Full end-to-end
// integration via uumain() is not yet feasible because chpasswd only supports
// --root (which performs a real chroot(2) and requires root), not --prefix
// (path-prefix without chroot). Once chpasswd gains a --prefix flag, replace
// these tests with uumain() calls using run(&["chpasswd", "--prefix", ...,
// "-e", ...]) with synthetic files.
#[test]
fn test_batch_password_update() {