Repository: memorysafety/sudo-rs
Pull request #816 (2024-01-11 17:57:50)

miscellaneous refactors
rebased version of #796 


Patch:
diff --git a/src/common/context.rs b/src/common/context.rs
index b7b5be45..c9412063 100644
--- a/src/common/context.rs
+++ b/src/common/context.rs
@@ -1,11 +1,11 @@
 use crate::common::{HARDENED_ENUM_VALUE_0, HARDENED_ENUM_VALUE_1, HARDENED_ENUM_VALUE_2};
-use crate::system::{hostname, Group, Process, User};
+use crate::system::{Group, Hostname, Process, User};
 
-use super::path::SudoPath;
+use super::resolve::CurrentUser;
 use super::{
     command::CommandAndArguments,
-    resolve::{resolve_current_user, resolve_launch_and_shell, resolve_target_user_and_group},
-    Error, SudoString,
+    resolve::{resolve_launch_and_shell, resolve_target_user_and_group},
+    Error, SudoPath, SudoString,
 };
 
 #[derive(Clone, Copy)]
@@ -41,8 +41,8 @@ pub struct Context {
     pub non_interactive: bool,
     pub use_session_records: bool,
     // system
-    pub hostname: String,
-    pub current_user: User,
+    pub hostname: Hostname,
+    pub current_user: CurrentUser,
     pub process: Process,
     // policy
     pub use_pty: bool,
@@ -61,8 +61,8 @@ impl Context {
         sudo_options: OptionsForContext,
         path: String,
     ) -> Result<Context, Error> {
-        let hostname = hostname();
-        let current_user = resolve_current_user()?;
+        let hostname = Hostname::resolve();
+        let current_user = CurrentUser::resolve()?;
         let (target_user, target_group) =
             resolve_target_user_and_group(&sudo_options.user, &sudo_options.group, &current_user)?;
         let (launch, shell) = resolve_launch_and_shell(&sudo_options, &current_user, &target_user);
@@ -95,7 +95,7 @@ impl Context {
 
 #[cfg(test)]
 mod tests {
-    use crate::{sudo::SudoAction, system::hostname};
+    use crate::{sudo::SudoAction, system::Hostname};
     use std::collections::HashMap;
 
     use super::Context;
@@ -115,7 +115,7 @@ mod tests {
 
         assert_eq!(context.command.command.to_str().unwrap(), "/usr/bin/echo");
         assert_eq!(context.command.arguments, ["hello"]);
-        assert_eq!(context.hostname, hostname());
+        assert_eq!(context.hostname, Hostname::resolve());
         assert_eq!(context.target_user.uid, 0);
     }
 }
diff --git a/src/common/error.rs b/src/common/error.rs
index 97829e6b..f30d19a1 100644
--- a/src/common/error.rs
+++ b/src/common/error.rs
@@ -1,4 +1,4 @@
-use crate::pam::PamError;
+use crate::{pam::PamError, system::Hostname};
 use std::{borrow::Cow, fmt, path::PathBuf};
 
 use super::{SudoPath, SudoString};
@@ -9,7 +9,7 @@ pub enum Error {
     NotAllowed {
         username: SudoString,
         command: Cow<'static, str>,
-        hostname: String,
+        hostname: Hostname,
         other_user: Option<SudoString>,
     },
     SelfCheck,
diff --git a/src/common/resolve.rs b/src/common/resolve.rs
index ac2810e7..118e3b5c 100644
--- a/src/common/resolve.rs
+++ b/src/common/resolve.rs
@@ -1,7 +1,9 @@
 use crate::system::{Group, User};
-use std::ffi::CStr;
+use core::fmt;
 use std::{
-    env, fs, io,
+    env,
+    ffi::CStr,
+    fs, io, ops,
     os::unix::prelude::MetadataExt,
     path::{Path, PathBuf},
     str::FromStr,
@@ -31,8 +33,42 @@ impl<'a, T: FromStr> NameOrId<'a, T> {
     }
 }
 
-pub fn resolve_current_user() -> Result<User, Error> {
-    User::real()?.ok_or(Error::UserNotFound("current user".to_string()))
+#[derive(Clone)]
+pub struct CurrentUser {
+    inner: User,
+}
+
+impl From<CurrentUser> for User {
+    fn from(value: CurrentUser) -> Self {
+        value.inner
+    }
+}
+
+impl fmt::Debug for CurrentUser {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_tuple("CurrentUser").field(&self.inner).finish()
+    }
+}
+
+impl ops::Deref for CurrentUser {
+    type Target = User;
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}
+
+impl CurrentUser {
+    #[cfg(test)]
+    pub fn fake(user: User) -> Self {
+        Self { inner: user }
+    }
+
+    pub fn resolve() -> Result<Self, Error> {
+        Ok(Self {
+            inner: User::real()?.ok_or(Error::UserNotFound("current user".to_string()))?,
+        })
+    }
 }
 
 type Shell = Option<PathBuf>;
@@ -58,7 +94,7 @@ pub(super) fn resolve_launch_and_shell(
 pub(crate) fn resolve_target_user_and_group(
     target_user_name_or_id: &Option<SudoString>,
     target_group_name_or_id: &Option<SudoString>,
-    current_user: &User,
+    current_user: &CurrentUser,
 ) -> Result<(User, Group), Error> {
     // resolve user name or #<id> to a user
     let mut target_user =
@@ -71,7 +107,7 @@ pub(crate) fn resolve_target_user_and_group(
     match (&target_user_name_or_id, &target_group_name_or_id) {
         // when -g is specified, but -u is not specified default -u to the current user
         (None, Some(_)) => {
-            target_user = Some(current_user.clone());
+            target_user = Some(current_user.clone().into());
         }
         // when -u is specified but -g is not specified, default -g to the primary group of the specified user
         (Some(_), None) => {
@@ -186,10 +222,9 @@ pub(crate) fn resolve_path(command: &Path, path: &str) -> Option<PathBuf> {
 mod tests {
     use std::path::PathBuf;
 
-    use super::{
-        is_valid_executable, resolve_current_user, resolve_path, resolve_target_user_and_group,
-        NameOrId,
-    };
+    use crate::common::resolve::CurrentUser;
+
+    use super::{is_valid_executable, resolve_path, resolve_target_user_and_group, NameOrId};
 
     #[test]
     fn test_resolve_path() {
@@ -234,7 +269,7 @@ mod tests {
 
     #[test]
     fn test_resolve_target_user_and_group() {
-        let current_user = resolve_current_user().unwrap();
+        let current_user = CurrentUser::resolve().unwrap();
 
         // fallback to root
         let (user, group) = resolve_target_user_and_group(&None, &None, &current_user).unwrap();
diff --git a/src/exec/use_pty/parent.rs b/src/exec/use_pty/parent.rs
index 31b70aae..3b10bb27 100644
--- a/src/exec/use_pty/parent.rs
+++ b/src/exec/use_pty/parent.rs
@@ -266,7 +266,7 @@ fn get_pty() -> io::Result<Pty> {
 
     let pty = Pty::open().map_err(|err| {
         dev_error!("cannot allocate pty: {err}");
-        io::Error::new(io::ErrorKind::NotFound, "unable to open pty")
+        err
     })?;
 
     let euid = User::effective_uid();
diff --git a/src/su/context.rs b/src/su/context.rs
index 5ee00140..f2083fc6 100644
--- a/src/su/context.rs
+++ b/src/su/context.rs
@@ -5,11 +5,8 @@ use std::{
     path::{Path, PathBuf},
 };
 
-use crate::common::{error::Error, Environment};
-use crate::common::{
-    resolve::{is_valid_executable, resolve_current_user},
-    SudoPath,
-};
+use crate::common::{error::Error, resolve::CurrentUser, Environment};
+use crate::common::{resolve::is_valid_executable, SudoPath};
 use crate::exec::RunOptions;
 use crate::log::user_warn;
 use crate::system::{Group, Process, User};
@@ -30,7 +27,7 @@ pub(crate) struct SuContext {
     options: SuRunOptions,
     pub(crate) environment: Environment,
     user: User,
-    requesting_user: User,
+    requesting_user: CurrentUser,
     group: Group,
     pub(crate) process: Process,
 }
@@ -75,7 +72,7 @@ impl SuContext {
             }
         }
 
-        let requesting_user = resolve_current_user()?;
+        let requesting_user = CurrentUser::resolve()?;
 
         // resolve target user
         let mut user = User::from_name(options.user.as_cstr())?
diff --git a/src/sudo/mod.rs b/src/sudo/mod.rs
index e1e686ae..61888734 100644
--- a/src/sudo/mod.rs
+++ b/src/sudo/mod.rs
@@ -1,6 +1,7 @@
 #![forbid(unsafe_code)]
 
-use crate::common::{resolve::resolve_current_user, Context, Error};
+use crate::common::resolve::CurrentUser;
+use crate::common::{Context, Error};
 use crate::log::dev_info;
 use crate::system::timestamp::RecordScope;
 use crate::system::User;
@@ -59,7 +60,7 @@ impl PolicyPlugin for SudoersPolicy {
         context: &Context,
     ) -> Result<Self::Policy, Error> {
         Ok(pre.check(
-            &context.current_user,
+            &*context.current_user,
             &context.hostname,
             crate::sudoers::Request {
                 user: &context.target_user,
@@ -95,17 +96,17 @@ fn sudo_process() -> Result<(), Error> {
                 std::process::exit(0);
             }
             SudoAction::RemoveTimestamp(_) => {
-                let user = resolve_current_user()?;
+                let user = CurrentUser::resolve()?;
                 let mut record_file =
-                    SessionRecordFile::open_for_user(user.uid, Duration::seconds(0))?;
+                    SessionRecordFile::open_for_user(&user, Duration::seconds(0))?;
                 record_file.reset()?;
                 Ok(())
             }
             SudoAction::ResetTimestamp(_) => {
                 if let Some(scope) = RecordScope::for_process(&Process::new()) {
-                    let user = resolve_current_user()?;
+                    let user = CurrentUser::resolve()?;
                     let mut record_file =
-                        SessionRecordFile::open_for_user(user.uid, Duration::seconds(0))?;
+                        SessionRecordFile::open_for_user(&user, Duration::seconds(0))?;
                     record_file.disable(scope, None)?;
                 }
                 Ok(())
diff --git a/src/sudo/pipeline.rs b/src/sudo/pipeline.rs
index 0fddd2d5..949970f4 100644
--- a/src/sudo/pipeline.rs
+++ b/src/sudo/pipeline.rs
@@ -3,6 +3,7 @@ use std::process::exit;
 
 use super::cli::{SudoRunOptions, SudoValidateOptions};
 use crate::common::context::OptionsForContext;
+use crate::common::resolve::CurrentUser;
 use crate::common::{Context, Environment, Error};
 use crate::exec::{ExecOutput, ExitReason};
 use crate::log::{auth_info, auth_warn};
@@ -145,7 +146,7 @@ impl<Policy: PolicyPlugin, Auth: AuthPlugin> Pipeline<Policy, Auth> {
             context.use_session_records,
             scope,
             context.current_user.uid,
-            context.current_user.uid,
+            &context.current_user,
             prior_validity,
         );
         self.authenticator.init(context)?;
@@ -216,7 +217,7 @@ fn determine_auth_status(
     use_session_records: bool,
     record_for: Option<RecordScope>,
     auth_uid: UserId,
-    current_user: UserId,
+    current_user: &CurrentUser,
     prior_validity: Duration,
 ) -> AuthStatus {
     if !must_policy_authenticate {
diff --git a/src/sudo/pipeline/list.rs b/src/sudo/pipeline/list.rs
index f1216d85..da1f1d9a 100644
--- a/src/sudo/pipeline/list.rs
+++ b/src/sudo/pipeline/list.rs
@@ -79,7 +79,7 @@ impl Pipeline<SudoersPolicy, PamAuthenticator<CLIConverser>> {
             target_group: &context.target_group,
         };
         let judgement =
-            sudoers.check_list_permission(&context.current_user, &context.hostname, list_request);
+            sudoers.check_list_permission(&*context.current_user, &context.hostname, list_request);
         match judgement.authorization() {
             Authorization::Allowed(auth) => {
                 self.auth_and_update_record_file(context, auth)?;
diff --git a/src/sudoers/mod.rs b/src/sudoers/mod.rs
index 45ff5243..cb57509a 100644
--- a/src/sudoers/mod.rs
+++ b/src/sudoers/mod.rs
@@ -15,8 +15,8 @@ use std::{io, mem};
 
 use crate::common::resolve::resolve_path;
 use crate::log::auth_warn;
-use crate::system::can_execute;
 use crate::system::interface::{UnixGroup, UnixUser};
+use crate::system::{self, can_execute};
 use ast::*;
 use tokens::*;
 
@@ -77,7 +77,7 @@ impl Sudoers {
     pub fn check<User: UnixUser + PartialEq<User>, Group: UnixGroup>(
         &self,
         am_user: &User,
-        on_host: &str,
+        on_host: &system::Hostname,
         request: Request<User, Group>,
     ) -> Judgement {
         // exception: if user is root or does not switch users, NOPASSWD is implied
@@ -100,7 +100,7 @@ impl Sudoers {
     pub fn check_list_permission<User: UnixUser + PartialEq<User>, Group: UnixGroup>(
         &self,
         invoking_user: &User,
-        hostname: &str,
+        hostname: &system::Hostname,
         request: ListRequest<User, Group>,
     ) -> Judgement {
         // exception: if user is root or does not switch users, NOPASSWD is implied
@@ -142,7 +142,7 @@ impl Sudoers {
     fn matching_user_specs<'a: 'b + 'c, 'b: 'c, 'c, User: UnixUser + PartialEq<User>>(
         &'a self,
         invoking_user: &'b User,
-        hostname: &'c str,
+        hostname: &'c system::Hostname,
     ) -> impl Iterator<Item = impl Iterator<Item = (Option<&'a RunAs>, (Tag, &'a Spec<Command>))> + 'b>
            + 'c {
         let Self { rules, aliases, .. } = self;
@@ -166,7 +166,7 @@ impl Sudoers {
     pub fn matching_entries<'a, User: UnixUser + PartialEq<User>>(
         &'a self,
         invoking_user: &User,
-        hostname: &str,
+        hostname: &system::Hostname,
     ) -> Vec<Entry<'a>> {
         // NOTE this method MUST NOT perform any filtering that `Self::check` does not do to
         // ensure `sudo $command` and `sudo --list` use the same permission checking logic
@@ -276,7 +276,7 @@ fn open_subsudoers(path: &Path) -> io::Result<Vec<basic_parser::Parsed<Sudo>>> {
 #[derive(Default)]
 pub(super) struct AliasTable {
     user: VecOrd<Def<UserSpecifier>>,
-    host: VecOrd<Def<Hostname>>,
+    host: VecOrd<Def<tokens::Hostname>>,
     cmnd: VecOrd<Def<Command>>,
     runas: VecOrd<Def<UserSpecifier>>,
 }
@@ -299,7 +299,7 @@ fn elems<T>(vec: &VecOrd<T>) -> impl Iterator<Item = &T> {
 fn check_permission<User: UnixUser + PartialEq<User>, Group: UnixGroup>(
     sudoers: &Sudoers,
     am_user: &User,
-    on_host: &str,
+    on_host: &system::Hostname,
     request: Request<User, Group>,
 ) -> Option<Tag> {
     let cmdline = (request.command, request.arguments);
diff --git a/src/system/mod.rs b/src/system/mod.rs
index 07cc0190..a929bfed 100644
--- a/src/system/mod.rs
+++ b/src/system/mod.rs
@@ -1,9 +1,11 @@
+use core::fmt;
 // TODO: remove unused attribute when system is cleaned up
 use std::{
     collections::BTreeSet,
     ffi::{c_uint, CStr, CString},
     io,
     mem::MaybeUninit,
+    ops,
     os::{
         fd::AsRawFd,
         unix::{self, prelude::OsStrExt},
@@ -147,25 +149,63 @@ pub fn setsid() -> io::Result<ProcessId> {
     cerr(unsafe { libc::setsid() })
 }
 
-pub fn hostname() -> String {
-    // see `man 2 gethostname`
-    const MAX_HOST_NAME_SIZE_ACCORDING_TO_SUSV2: libc::c_long = 255;
+#[derive(Clone)]
+#[cfg_attr(test, derive(PartialEq))]
+pub struct Hostname {
+    inner: String,
+}
+
+impl fmt::Debug for Hostname {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_tuple("Hostname").field(&self.inner).finish()
+    }
+}
+
+impl fmt::Display for Hostname {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(&self.inner)
+    }
+}
+
+impl ops::Deref for Hostname {
+    type Target = str;
 
-    // POSIX.1 systems limit hostnames to `HOST_NAME_MAX` bytes
-    // not including null-byte in the count
-    let max_hostname_size =
-        sysconf(libc::_SC_HOST_NAME_MAX).unwrap_or(MAX_HOST_NAME_SIZE_ACCORDING_TO_SUSV2) as usize;
+    fn deref(&self) -> &str {
+        &self.inner
+    }
+}
 
-    let buffer_size = max_hostname_size + 1 /* null byte delimiter */ ;
-    let mut buf = vec![0; buffer_size];
+impl Hostname {
+    #[cfg(test)]
+    pub fn fake(hostname: &str) -> Self {
+        Self {
+            inner: hostname.to_string(),
+        }
+    }
 
-    match cerr(unsafe { libc::gethostname(buf.as_mut_ptr(), buffer_size) }) {
-        Ok(_) => unsafe { string_from_ptr(buf.as_ptr()) },
+    pub fn resolve() -> Self {
+        // see `man 2 gethostname`
+        const MAX_HOST_NAME_SIZE_ACCORDING_TO_SUSV2: libc::c_long = 255;
 
-        // ENAMETOOLONG is returned when hostname is greater than `buffer_size`
-        Err(_) => {
-            // but we have chosen a `buffer_size` larger than `max_hostname_size` so no truncation error is possible
-            panic!("Unexpected error while retrieving hostname, this should not happen");
+        // POSIX.1 systems limit hostnames to `HOST_NAME_MAX` bytes
+        // not including null-byte in the count
+        let max_hostname_size = sysconf(libc::_SC_HOST_NAME_MAX)
+            .unwrap_or(MAX_HOST_NAME_SIZE_ACCORDING_TO_SUSV2)
+            as usize;
+
+        let buffer_size = max_hostname_size + 1 /* null byte delimiter */ ;
+        let mut buf = vec![0; buffer_size];
+
+        match cerr(unsafe { libc::gethostname(buf.as_mut_ptr(), buffer_size) }) {
+            Ok(_) => Self {
+                inner: unsafe { string_from_ptr(buf.as_ptr()) },
+            },
+
+            // ENAMETOOLONG is returned when hostname is greater than `buffer_size`
+            Err(_) => {
+                // but we have chosen a `buffer_size` larger than `max_hostname_size` so no truncation error is possible
+                panic!("Unexpected error while retrieving hostname, this should not happen");
+            }
         }
     }
 }
diff --git a/src/system/timestamp.rs b/src/system/timestamp.rs
index c8b9c426..c5d5542d 100644
--- a/src/system/timestamp.rs
+++ b/src/system/timestamp.rs
@@ -4,7 +4,10 @@ use std::{
     path::PathBuf,
 };
 
-use crate::log::{auth_info, auth_warn};
+use crate::{
+    common::resolve::CurrentUser,
+    log::{auth_info, auth_warn},
+};
 
 use super::{
     audit::secure_open_cookie_file,
@@ -44,10 +47,11 @@ pub struct SessionRecordFile {
 impl SessionRecordFile {
     const BASE_PATH: &'static str = "/var/run/sudo-rs/ts";
 
-    pub fn open_for_user(user: UserId, timeout: Duration) -> io::Result<Self> {
+    pub fn open_for_user(user: &CurrentUser, timeout: Duration) -> io::Result<Self> {
+        let uid = user.uid;
         let mut path = PathBuf::from(Self::BASE_PATH);
-        path.push(user.to_string());
-        SessionRecordFile::new(user, secure_open_cookie_file(&path)?, timeout)
+        path.push(uid.to_string());
+        SessionRecordFile::new(uid, secure_open_cookie_file(&path)?, timeout)
     }
 
     const FILE_VERSION: u16 = 1;


Test patch:
diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs
index 4824fe5e..a0028be0 100644
--- a/src/sudo/env/tests.rs
+++ b/src/sudo/env/tests.rs
@@ -1,9 +1,10 @@
+use crate::common::resolve::CurrentUser;
 use crate::common::{CommandAndArguments, Context, Environment};
 use crate::sudo::{
     cli::{SudoAction, SudoRunOptions},
     env::environment::get_target_environment,
 };
-use crate::system::{Group, Process, User};
+use crate::system::{Group, Hostname, Process, User};
 use std::collections::{HashMap, HashSet};
 
 const TESTS: &str = "
@@ -80,7 +81,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
     let command =
         CommandAndArguments::build_from_args(None, sudo_options.positional_args.clone(), &path);
 
-    let current_user = User {
+    let current_user = CurrentUser::fake(User {
         uid: 1000,
         gid: 1000,
 
@@ -90,7 +91,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
         shell: "/bin/sh".into(),
         passwd: String::new(),
         groups: vec![],
-    };
+    });
 
     let current_group = Group {
         gid: 1000,
@@ -118,11 +119,11 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
     };
 
     Context {
-        hostname: "test-ubuntu".to_string(),
+        hostname: Hostname::fake("test-ubuntu"),
         command,
         current_user: current_user.clone(),
         target_user: if sudo_options.user.as_deref() == Some("test") {
-            current_user
+            current_user.into()
         } else {
             root_user
         },
diff --git a/src/sudoers/test/mod.rs b/src/sudoers/test/mod.rs
index 21482cf8..79606007 100644
--- a/src/sudoers/test/mod.rs
+++ b/src/sudoers/test/mod.rs
@@ -94,7 +94,7 @@ fn permission_test() {
             let (Sudoers { rules,aliases,settings }, _) = analyze(Path::new("/etc/fakesudoers"), sudoer![$($sudo),*]);
             let cmdvec = $command.split_whitespace().map(String::from).collect::<Vec<_>>();
             let req = Request { user: $req.0, group: $req.1, command: &realpath(cmdvec[0].as_ref()), arguments: &cmdvec[1..].to_vec() };
-            assert_eq!(Sudoers { rules, aliases, settings }.check(&Named($user), $server, req).flags, None);
+            assert_eq!(Sudoers { rules, aliases, settings }.check(&Named($user), &system::Hostname::fake($server), req).flags, None);
         }
     }
 
@@ -103,7 +103,7 @@ fn permission_test() {
             let (Sudoers { rules,aliases,settings }, _) = analyze(Path::new("/etc/fakesudoers"), sudoer![$($sudo),*]);
             let cmdvec = $command.split_whitespace().map(String::from).collect::<Vec<_>>();
             let req = Request { user: $req.0, group: $req.1, command: &realpath(cmdvec[0].as_ref()), arguments: &cmdvec[1..].to_vec() };
-            let result = Sudoers { rules, aliases, settings }.check(&Named($user), $server, req).flags;
+            let result = Sudoers { rules, aliases, settings }.check(&Named($user), &system::Hostname::fake($server), req).flags;
             assert!(!result.is_none());
             $(
                 let result = result.unwrap();