Skip to content

shadow-core: generic record I/O, locked-file transaction, shared exit errors, feature and dead-code cleanup #249

Description

@pierre-warnier

Simplifications in shadow-core and the tool crates that remove code while strengthening invariants. Each is independent; the first two are prerequisites for several bug issues.

  1. One generic reader/writer for the five record files. read_*_file/write_* are identical modulo type, and the 33 fields.next().ok_or_else(...) chains have one shape. read_entries<T: FromStr> / write_entries<T: Display> plus a Fields<'a> helper; this is also where raw-line preservation (compat lines, comments) and per-line error context are implemented once.
  2. LockedFile<T> transaction: open acquires the lock (and .pwd.lock), reads under it; entries_mut(); commit() writes atomically and releases; Drop releases without writing. Every tool repeats the lock→read→mutate→write→unlock dance ~13 times, and pwck -s gets the order wrong (sorts entries read before the lock). Invariant gained: no read outside the lock, no write after release.
  3. One exit-code error type (cli::ExitError { code, msg } or equivalent). The per-tool enum XxxError(String) + Display + UError blocks are the same ~40 lines twelve times; exit_codes constants stay the single source of truth.
  4. Cargo features: drop the dependency-free parser features (shadow, group, gshadow, login-defs, subid) — they make cargo test -p shadow-core skip most of the crate and add a cfg(all(...)) on uid_alloc; keep pam, crypt, landlock (they change linkage) and make zeroize/subtle optional under them. In the root manifest, pam = ["passwd?/pam", "chfn?/pam", "chsh?/pam"] (today the feature force-enables the three applets) and delete the unused feat_* aliases.
  5. Dead code: process::PwEntry + lookup_username + hardening::lookup_username_by_uid (a field-by-field copy of PasswdEntry); SavedSigSet/block_critical_signals/restore_signals/SignalBlocker are four names for one guard (add SIGQUIT); error::Io, Permission, ShadowResult have no producers or users; pam.rs's unsafe impl Send, display_message(_, _is_error), unused item/flag surface.
  6. uid_alloc: one IdRange built from a key pair and one next_free; add the NSS check (getpwuid) so a directory user's UID is never handed out locally.
  7. login_defs::get_i64 parses decimal only; login.defs(5) allows octal (leading 0) and hex (0x): UID_MIN 01000 is 512 to GNU and 1000 here; UMASK 022/HOME_MODE 0750 will be misread the day they are honoured.
  8. harden_process() returns an environment nobody uses; the doc says the environment is sanitized and it is not. Either sanitize for real (clearenv + the safe set, in the unsafe-permitted process module) or make the function () and keep sanitized_env() private to a safe_command(path) helper shared by nscd/audit. audit can write to /dev/log with a UnixDatagram instead of forking logger.
  9. Doc statements that are false and should go with the code they describe: sysroot "--root does an actual chroot" (only some tools), pam PAM_DISALLOW_NULL_AUTHTOK description, hardening names rw_paths/ro_paths that do not exist, atomic "unique across threads" for a single-threaded crate.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions