From 0e247d121a810f5adb5b6ba58fbae4f191be3b3f Mon Sep 17 00:00:00 2001 From: Marti Raudsepp Date: Mon, 26 Jun 2023 02:34:21 +0300 Subject: [PATCH] Warn if target user home directory is not accessible (#139) Adds additional log output when target user's home directory seems incorrectly configured. This would make issues like #131 less puzzling. * Also added snapshot tests for log output using new `testing_logger` dependency. * Also now using an unprivileged user to build in Docker, as tests assume `/root/` directory is not accessible. --- Cargo.lock | 10 +++++ Cargo.toml | 1 + src/main.rs | 43 ++++++++++++++++++++- src/snapshots/check_user_homedir.txt | 7 ++++ src/tests.rs | 57 ++++++++++++++++++++++++++-- varia/Dockerfile.tests | 7 ++++ 6 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 src/snapshots/check_user_homedir.txt diff --git a/Cargo.lock b/Cargo.lock index f1f6eaa..b76eba4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -134,6 +134,7 @@ dependencies = [ "shell-words", "simple-error", "snapbox", + "testing_logger", ] [[package]] @@ -298,6 +299,15 @@ version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" +[[package]] +name = "testing_logger" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6d92b727cb45d33ae956f7f46b966b25f1bc712092aeef9dba5ac798fc89f720" +dependencies = [ + "log", +] + [[package]] name = "utf8parse" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index b1bee52..0f5c05b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,3 +30,4 @@ default = [] [dev-dependencies] clap_complete = "~4.3.1" snapbox = "0.4.11" +testing_logger = "0.1.1" diff --git a/src/main.rs b/src/main.rs index dfba3c4..a251e1b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -4,14 +4,16 @@ extern crate simple_error; use crate::cli::{parse_args, Method}; use crate::errors::{print_error, AnyErr, ErrorWithHint}; use crate::util::{exec_command, have_command, run_command, sd_booted}; -use log::{debug, info, warn}; +use log::{debug, info, log, warn, Level}; use nix::libc::uid_t; use nix::unistd::{Uid, User}; use posix_acl::{PosixACL, Qualifier, ACL_EXECUTE, ACL_READ, ACL_RWX}; use simple_error::SimpleError; use std::env::VarError; use std::fs::DirBuilder; +use std::io::ErrorKind::PermissionDenied; use std::os::unix::fs::DirBuilderExt; +use std::os::unix::fs::MetadataExt; use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; use std::process::exit; @@ -24,11 +26,13 @@ mod logging; mod tests; mod util; +#[derive(Clone)] struct EgoContext { runtime_dir: PathBuf, target_user: String, target_uid: uid_t, target_user_shell: PathBuf, + target_user_homedir: PathBuf, } fn main_inner() -> Result<(), AnyErr> { @@ -43,6 +47,8 @@ fn main_inner() -> Result<(), AnyErr> { ctx.target_user, ctx.target_uid ); + check_user_homedir(&ctx); + let ret = prepare_runtime_dir(&ctx); if let Err(msg) = ret { bail!("Error preparing runtime dir: {msg}"); @@ -144,6 +150,7 @@ fn create_context(username: String) -> Result { target_user: user.name, target_uid: user.uid.as_raw(), target_user_shell: user.shell, + target_user_homedir: user.dir, }) } @@ -154,6 +161,40 @@ fn add_file_acl(path: &Path, uid: u32, flags: u32) -> Result<(), AnyErr> { Ok(()) } +/// Report warning if user home directory does not exist or has wrong ownership +fn check_user_homedir(ctx: &EgoContext) { + let home = &ctx.target_user_homedir; + match fs::metadata(home) { + Ok(meta) => { + if meta.uid() != ctx.target_uid { + warn!( + "User {} home directory {} has incorrect ownership (expected UID {}, found {})", + ctx.target_user, + home.display(), + ctx.target_uid, + meta.uid() + ); + } + } + Err(err) => { + // Report PermissionDenied as `info` level, user home directory is probably in a parent + // directory we have no access to, avoid nagging. + let level = match err.kind() { + PermissionDenied => Level::Info, + _ => Level::Warn, + }; + + log!( + level, + "User {} home directory {} is not accessible: {}", + ctx.target_user, + home.display(), + err + ); + } + } +} + /// Add execute perm to runtime dir, e.g. `/run/user/1000` fn prepare_runtime_dir(ctx: &EgoContext) -> Result<(), AnyErr> { let path = &ctx.runtime_dir; diff --git a/src/snapshots/check_user_homedir.txt b/src/snapshots/check_user_homedir.txt new file mode 100644 index 0000000..e908002 --- /dev/null +++ b/src/snapshots/check_user_homedir.txt @@ -0,0 +1,7 @@ +INFO: TEST: Success (no output) +INFO: TEST: Home does not exist +WARN: User nope home directory /tmp/path-does-not-exist.example is not accessible: No such file or directory (os error 2) +INFO: TEST: Permission denied +INFO: User root home directory /root/path-is-not-accessible.example is not accessible: Permission denied (os error 13) +INFO: TEST: Wrong owner +WARN: User root home directory /root has incorrect ownership (expected UID 1234, found 0) diff --git a/src/tests.rs b/src/tests.rs index 10c539b..a65c7ed 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,19 +1,31 @@ use std::env; -use std::path::PathBuf; +use std::fmt::Write; +use std::path::{Path, PathBuf}; use clap_complete::shells::{Bash, Fish, Zsh}; use clap_complete::Generator; -use log::Level; +use log::{info, Level}; use crate::cli::{build_cli, parse_args, Method}; use crate::util::have_command; -use crate::{get_wayland_socket, EgoContext}; +use crate::{check_user_homedir, get_wayland_socket, EgoContext}; /// `vec![]` constructor that converts arguments to String macro_rules! string_vec { ($($x:expr),*) => (vec![$($x.to_string()),*] as Vec); } +/// Compare log output with snapshot file. Call `testing_logger::setup()` at beginning of test. +fn assert_log_snapshot(expected_path: impl AsRef) { + testing_logger::validate(|logs| { + let output = logs.iter().fold(String::new(), |mut a, b| { + write!(a, "{}: {}\n", b.level.as_str(), b.body).unwrap(); + a + }); + snapbox::assert_eq_path(&expected_path, output); + }) +} + fn render_completion(generator: impl Generator) -> Vec { let mut buf = Vec::::new(); let mut app = build_cli(); @@ -60,6 +72,7 @@ fn test_context() -> EgoContext { target_user: "ego".into(), target_uid: 155, target_user_shell: "/bin/bash".into(), + target_user_homedir: "/home/ego".into(), } } @@ -130,3 +143,41 @@ fn test_have_command() { assert!(have_command("sh")); assert!(!have_command("what-is-this-i-don't-even")); } + +#[test] +fn test_check_user_homedir() { + let ctx = EgoContext { + runtime_dir: Default::default(), + target_user: "root".to_string(), + target_uid: 0, + target_user_shell: Default::default(), + target_user_homedir: "/root".into(), + }; + + // Capture log output from called functions + testing_logger::setup(); + + info!("TEST: Success (no output)"); + check_user_homedir(&ctx); + + info!("TEST: Home does not exist"); + check_user_homedir(&EgoContext { + target_user: "nope".into(), + target_user_homedir: "/tmp/path-does-not-exist.example".into(), + ..ctx.clone() + }); + + info!("TEST: Permission denied"); + check_user_homedir(&EgoContext { + target_user_homedir: "/root/path-is-not-accessible.example".into(), + ..ctx.clone() + }); + + info!("TEST: Wrong owner"); + check_user_homedir(&EgoContext { + target_uid: 1234, + ..ctx.clone() + }); + + assert_log_snapshot("src/snapshots/check_user_homedir.txt"); +} diff --git a/varia/Dockerfile.tests b/varia/Dockerfile.tests index ac72190..e03ffa9 100644 --- a/varia/Dockerfile.tests +++ b/varia/Dockerfile.tests @@ -8,7 +8,14 @@ ENV RUSTFLAGS="-D warnings" RUN apt-get update && \ apt-get install -y libacl1-dev && \ rm -rf /var/lib/apt/lists/* + +# Build as unprivileged user +RUN useradd build --create-home +WORKDIR /home/build +USER build + RUN rustup component add rustfmt clippy + # Build Cargo dependencies for cache COPY Cargo.toml Cargo.lock ./ RUN mkdir src/ && \