Repository: memorysafety/sudo-rs
Pull request #63 (2023-03-07 16:14:02)

Use configuration in env processing


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index e419cc99..6f0b1af0 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -482,6 +482,7 @@ dependencies = [
  "sudo-cli",
  "sudo-pam",
  "sudo-system",
+ "sudoers",
  "thiserror",
  "which",
 ]
diff --git a/lib/sudo-common/Cargo.toml b/lib/sudo-common/Cargo.toml
index 4772ef86..da22a675 100644
--- a/lib/sudo-common/Cargo.toml
+++ b/lib/sudo-common/Cargo.toml
@@ -12,6 +12,7 @@ build = "build.rs"
 sudo-cli.workspace = true
 sudo-pam.workspace = true
 sudo-system.workspace = true
+sudoers.workspace = true
 libc.workspace = true
 thiserror.workspace = true
 which.workspace = true
diff --git a/lib/sudo-common/src/context.rs b/lib/sudo-common/src/context.rs
index 2e079ca3..e2a79c6e 100644
--- a/lib/sudo-common/src/context.rs
+++ b/lib/sudo-common/src/context.rs
@@ -1,6 +1,7 @@
-use std::{path::PathBuf, str::FromStr};
+use std::{collections::HashSet, path::PathBuf, str::FromStr};
 use sudo_cli::SudoOptions;
 use sudo_system::{hostname, Group, User};
+use sudoers::Settings;
 
 use crate::{env::Environment, error::Error};
 
@@ -10,6 +11,16 @@ pub struct CommandAndArguments<'a> {
     pub arguments: Vec<&'a str>,
 }
 
+impl<'a> ToString for CommandAndArguments<'a> {
+    fn to_string(&self) -> String {
+        format!(
+            "{} {}",
+            self.command.to_string_lossy(),
+            self.arguments.join(" ")
+        )
+    }
+}
+
 impl<'a> TryFrom<&'a [String]> for CommandAndArguments<'a> {
     type Error = Error;
 
@@ -47,45 +58,108 @@ impl<'a, T: FromStr> NameOrId<'a, T> {
 
 #[derive(Debug)]
 pub struct Context<'a> {
-    pub preserve_env: bool,
+    // cli options
     pub preserve_env_list: Vec<String>,
     pub set_home: bool,
+    pub login: bool,
+    pub shell: bool,
+    pub chdir: Option<PathBuf>,
     pub command: CommandAndArguments<'a>,
-    pub hostname: String,
-    pub current_user: User,
     pub target_user: User,
     pub target_group: Group,
+    // configuration
+    pub env_delete: &'a HashSet<String>,
+    pub env_keep: &'a HashSet<String>,
+    pub env_check: &'a HashSet<String>,
+    pub always_set_home: bool,
+    pub use_pty: bool,
+    // system
+    pub hostname: String,
+    pub current_user: User,
+    // computed
     pub target_environment: Environment,
 }
 
+pub trait Configuration {
+    fn env_delete(&self) -> &HashSet<String>;
+    fn env_keep(&self) -> &HashSet<String>;
+    fn env_check(&self) -> &HashSet<String>;
+    fn always_set_home(&self) -> bool;
+    fn use_pty(&self) -> bool;
+}
+
+impl Configuration for Settings {
+    fn env_delete(&self) -> &HashSet<String> {
+        self.list
+            .get("env_delete")
+            .expect("env_delete missing from settings")
+    }
+
+    fn env_keep(&self) -> &HashSet<String> {
+        self.list
+            .get("env_keep")
+            .expect("env_keep missing from settings")
+    }
+
+    fn env_check(&self) -> &HashSet<String> {
+        self.list
+            .get("env_check")
+            .expect("env_check missing from settings")
+    }
+
+    fn always_set_home(&self) -> bool {
+        self.flags.contains("always_set_home")
+    }
+
+    fn use_pty(&self) -> bool {
+        self.flags.contains("use_pty")
+    }
+}
+
+fn resolve_current_user() -> Result<User, Error> {
+    User::real()?.ok_or(Error::UserNotFound("current user".to_string()))
+}
+
+fn resolve_target_user(target_name_or_id: &Option<String>) -> Result<User, Error> {
+    let target_name_or_id = target_name_or_id.as_deref().unwrap_or("root");
+
+    match NameOrId::parse(target_name_or_id) {
+        Some(NameOrId::Name(name)) => User::from_name(name)?,
+        Some(NameOrId::Id(uid)) => User::from_uid(uid)?,
+        _ => None,
+    }
+    .ok_or_else(|| Error::UserNotFound(target_name_or_id.to_string()))
+}
+
+fn resolve_target_group(
+    target_name_or_id: &Option<String>,
+    target_user: &User,
+) -> Result<Group, Error> {
+    match target_name_or_id.as_deref() {
+        Some(name_or_id) => match NameOrId::parse(name_or_id) {
+            Some(NameOrId::Name(name)) => Group::from_name(name)?,
+            Some(NameOrId::Id(gid)) => Group::from_gid(gid)?,
+            _ => None,
+        },
+        None => Group::from_gid(target_user.gid)?,
+    }
+    .ok_or(Error::GroupNotFound(
+        target_name_or_id
+            .clone()
+            .unwrap_or_else(|| target_user.gid.to_string()),
+    ))
+}
+
 impl<'a> Context<'a> {
-    pub fn build_from_options(sudo_options: &SudoOptions) -> Result<Context, Error> {
+    pub fn build_from_options(
+        sudo_options: &'a SudoOptions,
+        settings: &'a Settings,
+    ) -> Result<Context<'a>, Error> {
         let command = CommandAndArguments::try_from(sudo_options.external_args.as_slice())?;
         let hostname = hostname();
-        let current_user = User::real()?.ok_or(Error::UserNotFound("current user".to_string()))?;
-
-        let target_name_or_id = sudo_options.user.as_deref().unwrap_or("root");
-        let target_user = match NameOrId::parse(target_name_or_id) {
-            Some(NameOrId::Name(name)) => User::from_name(name)?,
-            Some(NameOrId::Id(uid)) => User::from_uid(uid)?,
-            _ => None,
-        }
-        .ok_or_else(|| Error::UserNotFound(target_name_or_id.to_string()))?;
-
-        let target_group = match sudo_options.group.as_deref() {
-            Some(name_or_id) => match NameOrId::parse(name_or_id) {
-                Some(NameOrId::Name(name)) => Group::from_name(name)?,
-                Some(NameOrId::Id(gid)) => Group::from_gid(gid)?,
-                _ => None,
-            },
-            None => Group::from_gid(target_user.gid)?,
-        }
-        .ok_or(Error::GroupNotFound(
-            sudo_options
-                .group
-                .clone()
-                .unwrap_or_else(|| target_user.gid.to_string()),
-        ))?;
+        let current_user = resolve_current_user()?;
+        let target_user = resolve_target_user(&sudo_options.user)?;
+        let target_group = resolve_target_group(&sudo_options.group, &target_user)?;
 
         Ok(Context {
             hostname,
@@ -94,9 +168,16 @@ impl<'a> Context<'a> {
             target_user,
             target_group,
             target_environment: Default::default(),
-            preserve_env: sudo_options.preserve_env,
             set_home: sudo_options.set_home,
             preserve_env_list: sudo_options.preserve_env_list.clone(),
+            login: sudo_options.login,
+            shell: sudo_options.shell,
+            chdir: sudo_options.directory.clone(),
+            env_delete: settings.env_delete(),
+            env_keep: settings.env_keep(),
+            env_check: settings.env_check(),
+            always_set_home: settings.always_set_home(),
+            use_pty: settings.use_pty(),
         })
     }
 
@@ -113,6 +194,7 @@ mod tests {
 
     use super::Context;
     use sudo_cli::SudoOptions;
+    use sudoers::Settings;
 
     #[test]
     fn test_build_context() {
@@ -121,7 +203,8 @@ mod tests {
         let mut current_env = HashMap::new();
         current_env.insert("FOO".to_string(), "BAR".to_string());
 
-        let context = Context::build_from_options(&options)
+        let settings = Settings::default();
+        let context = Context::build_from_options(&options, &settings)
             .unwrap()
             .with_filtered_env(current_env);
 
diff --git a/lib/sudo-common/src/env.rs b/lib/sudo-common/src/env.rs
index d542266e..76bf0978 100644
--- a/lib/sudo-common/src/env.rs
+++ b/lib/sudo-common/src/env.rs
@@ -2,7 +2,7 @@ use crate::{
     context::{CommandAndArguments, Context},
     wildcard_match::wildcard_match,
 };
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use sudo_system::PATH_MAX;
 
 pub type Environment = HashMap<String, String>;
@@ -10,32 +10,6 @@ pub type Environment = HashMap<String, String>;
 const PATH_MAILDIR: &str = env!("PATH_MAILDIR");
 const PATH_ZONEINFO: &str = env!("PATH_ZONEINFO");
 
-/// Remove if these environment variables if the value contains '/' or '%'
-const CHECK_ENV_TABLE: &[&str] = &[
-    "COLORTERM",
-    "LANG",
-    "LANGUAGE",
-    "LC_*",
-    "LINGUAS",
-    "TERM",
-    "TZ",
-];
-
-/// Keep these environment variables by default
-const KEEP_ENV_TABLE: &[&str] = &[
-    "COLORS",
-    "DISPLAY",
-    "HOSTNAME",
-    "KRB5CCNAME",
-    "LS_COLORS",
-    "PATH",
-    "PS1",
-    "PS2",
-    "XAUTHORITY",
-    "XAUTHORIZATION",
-    "XDG_CURRENT_DESKTOP",
-];
-
 /// Convert a list of `Into<String>` key value pars to an Environment
 pub fn environment_from_list<K: Into<String>, V: Into<String>>(list: Vec<(K, V)>) -> Environment {
     list.into_iter()
@@ -66,25 +40,35 @@ fn format_command(command_and_arguments: &CommandAndArguments) -> String {
 
 /// Construct sudo-specific environment variables
 fn get_extra_env(context: &Context) -> Environment {
-    environment_from_list(vec![
+    let mut extra = vec![
         ("SUDO_COMMAND", format_command(&context.command)),
         ("SUDO_UID", context.current_user.uid.to_string()),
         ("SUDO_GID", context.current_user.gid.to_string()),
         ("SUDO_USER", context.current_user.name.clone()),
-        // TODO: preserve exsisting when sudo -s
-        ("SHELL", context.target_user.shell.clone()),
-        // TODO: Set to the login name of the target user when the -i option is specified,
-        // when the set_logname option is enabled in sudoers, or when the env_reset option
-        // is enabled in sudoers (unless LOGNAME is present in the env_keep list).
-        ("LOGNAME", context.target_user.name.clone()),
-        ("USER", context.target_user.name.clone()),
-        // TODO: check home dir config + options
-        ("HOME", context.target_user.home.clone()),
         (
             "MAIL",
             format!("{PATH_MAILDIR}/{}", context.target_user.name),
         ),
-    ])
+    ];
+
+    // TODO: preserve exsisting when sudo -s
+    if !context.shell {
+        extra.push(("SHELL", context.target_user.shell.clone()));
+    }
+
+    // TODO: Set to the login name of the target user when the -i option is specified,
+    // when the set_logname option is enabled in sudoers, or when the env_reset option
+    // is enabled in sudoers (unless LOGNAME is present in the env_keep list).
+    if context.login {
+        extra.push(("LOGNAME", context.target_user.name.clone()));
+        extra.push(("USER", context.target_user.name.clone()));
+    }
+
+    if context.always_set_home || context.set_home {
+        extra.push(("HOME", context.target_user.home.clone()));
+    }
+
+    environment_from_list(extra)
 }
 
 /// Check a string only contains printable (non-space) characters
@@ -120,14 +104,14 @@ fn is_safe_tz(value: &str) -> bool {
 }
 
 /// Check whether the needle exists in a haystack, in which the haystack is a list of patterns, possibly containing wildcards
-fn in_table(needle: &str, haystack: &[&str]) -> bool {
+fn in_table(needle: &str, haystack: &HashSet<String>) -> bool {
     haystack
         .iter()
         .any(|pattern| wildcard_match(needle, pattern))
 }
 
 /// Determine whether a specific environment variable should be kept
-fn should_keep(key: &str, value: &str, check_env: &[&str], keep_env: &[&str]) -> bool {
+fn should_keep(key: &str, value: &str, context: &Context) -> bool {
     if value.starts_with("()") {
         return false;
     }
@@ -136,11 +120,11 @@ fn should_keep(key: &str, value: &str, check_env: &[&str], keep_env: &[&str]) ->
         return false;
     }
 
-    if in_table(key, check_env) && !value.contains(|c| c == '%' || c == '/') {
+    if in_table(key, context.env_check) && !value.contains(|c| c == '%' || c == '/') {
         return true;
     }
 
-    in_table(key, keep_env)
+    in_table(key, context.env_keep)
 }
 
 /// Construct the final environment from the current one and a sudo context
@@ -160,7 +144,7 @@ pub fn get_target_environment(current_env: Environment, context: &Context) -> En
     let mut result = Environment::new();
 
     for (key, value) in current_env.into_iter() {
-        if should_keep(&key, &value, CHECK_ENV_TABLE, KEEP_ENV_TABLE) {
+        if should_keep(&key, &value, context) {
             result.insert(key, value);
         }
     }
diff --git a/sudo/src/main.rs b/sudo/src/main.rs
index 54db2602..3ce70087 100644
--- a/sudo/src/main.rs
+++ b/sudo/src/main.rs
@@ -2,10 +2,9 @@
 
 use sudo_cli::SudoOptions;
 use sudo_common::{context::Context, env::Environment, error::Error, pam::authenticate};
-use sudoers::Tag;
+use sudoers::{Sudoers, Tag};
 
-/// parse suoers file and check permission to run the provided command given the context
-fn check_sudoers(context: &Context, sudo_options: &SudoOptions) -> Result<Option<Vec<Tag>>, Error> {
+fn parse_sudoers() -> Result<Sudoers, Error> {
     // TODO: move to global configuration
     let sudoers_path = "/etc/sudoers.test";
 
@@ -16,15 +15,20 @@ fn check_sudoers(context: &Context, sudo_options: &SudoOptions) -> Result<Option
         eprintln!("Parse error: {error}");
     }
 
+    Ok(sudoers)
+}
+
+/// parse suoers file and check permission to run the provided command given the context
+fn check_sudoers(sudoers: &Sudoers, context: &Context) -> Result<Option<Vec<Tag>>, Error> {
     Ok(sudoers::check_permission(
-        &sudoers,
+        sudoers,
         &context.current_user,
         sudoers::Request {
             user: &context.target_user,
             group: &context.target_group,
         },
         &context.hostname,
-        &sudo_options.external_args.join(" "),
+        context.command.to_string().as_str(),
     ))
 }
 
@@ -32,12 +36,16 @@ fn main() -> Result<(), Error> {
     // parse cli options
     let sudo_options = SudoOptions::parse();
 
+    // parse sudoers file
+    let sudoers = parse_sudoers()?;
+
     // build context and environment
     let current_env = std::env::vars().collect::<Environment>();
-    let context = Context::build_from_options(&sudo_options)?.with_filtered_env(current_env);
+    let context = Context::build_from_options(&sudo_options, &sudoers.settings)?
+        .with_filtered_env(current_env);
 
     // check sudoers file for permission
-    match check_sudoers(&context, &sudo_options)? {
+    match check_sudoers(&sudoers, &context)? {
         Some(tags) => {
             if !tags.contains(&Tag::NoPasswd) {
                 // authenticate user using pam


Test patch:
diff --git a/lib/sudo-common/tests/env_tests.rs b/lib/sudo-common/tests/env_tests.rs
index d9ebf8fd..6b7c5d49 100644
--- a/lib/sudo-common/tests/env_tests.rs
+++ b/lib/sudo-common/tests/env_tests.rs
@@ -1,11 +1,11 @@
 use std::collections::HashSet;
-
 use sudo_cli::SudoOptions;
 use sudo_common::{
-    context::{CommandAndArguments, Context},
+    context::{CommandAndArguments, Configuration, Context},
     env::{get_target_environment, Environment},
 };
 use sudo_system::{Group, User};
+use sudoers::Settings;
 
 const TESTS: &str = "
 > env
@@ -22,12 +22,10 @@ const TESTS: &str = "
     TERM=xterm
     _=/usr/bin/sudo
 > sudo env
-    HOME=/root
     HOSTNAME=test-ubuntu
     LANG=en_US.UTF-8
     LANGUAGE=en_US.UTF-8
     LC_ALL=en_US.UTF-8
-    LOGNAME=root
     LS_COLORS=cd=40;33;01:*.jpg=01;35:*.mp3=00;36:
     MAIL=/var/mail/root
     PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
@@ -37,14 +35,11 @@ const TESTS: &str = "
     SUDO_UID=1000
     SUDO_USER=test
     TERM=xterm
-    USER=root
 > sudo -u test env
-    HOME=/home/test
     HOSTNAME=test-ubuntu
     LANG=en_US.UTF-8
     LANGUAGE=en_US.UTF-8
     LC_ALL=en_US.UTF-8
-    LOGNAME=test
     LS_COLORS=cd=40;33;01:*.jpg=01;35:*.mp3=00;36:
     MAIL=/var/mail/test
     PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
@@ -54,7 +49,6 @@ const TESTS: &str = "
     SUDO_UID=1000
     SUDO_USER=test
     TERM=xterm
-    USER=test
 ";
 
 fn parse_env_commands(input: &str) -> Vec<(&str, Environment)> {
@@ -76,7 +70,7 @@ fn parse_env_commands(input: &str) -> Vec<(&str, Environment)> {
         .collect()
 }
 
-fn create_test_context(sudo_options: &SudoOptions) -> Context {
+fn create_test_context<'a>(sudo_options: &'a SudoOptions, settings: &'a Settings) -> Context<'a> {
     let command = CommandAndArguments::try_from(sudo_options.external_args.as_slice()).unwrap();
 
     let current_user = User {
@@ -130,9 +124,16 @@ fn create_test_context(sudo_options: &SudoOptions) -> Context {
             root_group
         },
         target_environment: Default::default(),
-        preserve_env: sudo_options.preserve_env,
         set_home: sudo_options.set_home,
         preserve_env_list: sudo_options.preserve_env_list.clone(),
+        login: sudo_options.login,
+        shell: sudo_options.shell,
+        chdir: sudo_options.directory.clone(),
+        env_delete: settings.env_delete(),
+        env_keep: settings.env_keep(),
+        env_check: settings.env_check(),
+        always_set_home: settings.always_set_home(),
+        use_pty: settings.use_pty(),
     }
 }
 
@@ -147,7 +148,8 @@ fn test_environment_variable_filtering() {
 
     for (cmd, expected_env) in parts {
         let options = SudoOptions::try_parse_from(cmd.split_whitespace()).unwrap();
-        let context = create_test_context(&options);
+        let settings = Settings::default();
+        let context = create_test_context(&options, &settings);
         let resulting_env = get_target_environment(initial_env.clone(), &context);
 
         let resulting_env = environment_to_set(resulting_env);