Repository: memorysafety/sudo-rs
Pull request #75 (2023-03-13 13:10:00)

More abstract sudoers interface
Avoid exposing "ast::Tag" to the outside world, and communicate "settings" from the 'check' function (when all information about what sudo is about to attempt is present) instead of the 'compile' function. 

You can go commit-by-commit.

Note: commit 01b0348 is handled in separate PR #74, so merge that first.

Merging this closes issue #67 

squell: Note: if you don't like functional-style code, you will not like commit 89ef664 ;)

Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 3b19a06b..b1ce822d 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -32,9 +32,9 @@ checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a"
 
 [[package]]
 name = "block-buffer"
-version = "0.10.3"
+version = "0.10.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "69cce20737498f97b993470a6e536b8523f0af7892a4f928cceb1ac5e52ebe7e"
+checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71"
 dependencies = [
  "generic-array",
 ]
@@ -244,9 +244,9 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55"
 
 [[package]]
 name = "libc"
-version = "0.2.139"
+version = "0.2.140"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "201de327520df007757c1f0adce6e827fe8562fbc28bfd9c15571c66ca1f5f79"
+checksum = "99227334921fae1a979cf0bfdfcc6b3e5ce376ef57e16fb6fb3ea2ed6095f80c"
 
 [[package]]
 name = "libloading"
diff --git a/lib/sudo-common/src/context.rs b/lib/sudo-common/src/context.rs
index e09df7db..9ff2c882 100644
--- a/lib/sudo-common/src/context.rs
+++ b/lib/sudo-common/src/context.rs
@@ -46,6 +46,7 @@ impl<'a, T: FromStr> NameOrId<'a, T> {
     }
 }
 
+/// A Context is based off of global information and is 'non-judgmental'
 #[derive(Debug)]
 pub struct Context<'a> {
     // cli options
@@ -57,15 +58,17 @@ pub struct Context<'a> {
     pub command: CommandAndArguments<'a>,
     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,
+}
+
+/// A ContextWithEnv is specific to the operation and obtaining it indicates approval
+#[derive(Debug)]
+pub struct ContextWithEnv<'a> {
+    pub context: Context<'a>,
+    // command-specific
+    pub settings: &'a Settings,
     // computed
     pub target_environment: Environment,
 }
@@ -141,10 +144,7 @@ fn resolve_target_group(
 }
 
 impl<'a> Context<'a> {
-    pub fn build_from_options(
-        sudo_options: &'a SudoOptions,
-        settings: &'a Settings,
-    ) -> Result<Context<'a>, Error> {
+    pub fn build_from_options(sudo_options: &'a SudoOptions) -> Result<Context<'a>, Error> {
         let command = CommandAndArguments::try_from(sudo_options.external_args.as_slice())?;
         let hostname = hostname();
         let current_user = resolve_current_user()?;
@@ -157,24 +157,25 @@ impl<'a> Context<'a> {
             current_user,
             target_user,
             target_group,
-            target_environment: Default::default(),
             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(),
         })
     }
 
-    pub fn with_filtered_env(mut self, current_env: Environment) -> Context<'a> {
-        self.target_environment = crate::env::get_target_environment(current_env, &self);
-
-        self
+    pub fn with_filtered_env(
+        self,
+        current_env: Environment,
+        settings: &'a Settings,
+    ) -> ContextWithEnv<'a> {
+        let env = crate::env::get_target_environment(current_env, &self, settings);
+        ContextWithEnv {
+            context: self,
+            settings,
+            target_environment: env,
+        }
     }
 }
 
@@ -194,17 +195,20 @@ mod tests {
         current_env.insert("FOO".to_string(), "BAR".to_string());
 
         let settings = Settings::default();
-        let context = Context::build_from_options(&options, &settings)
+        let context = Context::build_from_options(&options)
             .unwrap()
-            .with_filtered_env(current_env);
+            .with_filtered_env(current_env, &settings);
 
-        assert_eq!(context.command.command.to_str().unwrap(), "/usr/bin/echo");
-        assert_eq!(context.command.arguments, ["hello"]);
-        assert_eq!(context.hostname, sudo_system::hostname());
-        assert_eq!(context.target_user.uid, 0);
+        assert_eq!(
+            context.context.command.command.to_str().unwrap(),
+            "/usr/bin/echo"
+        );
+        assert_eq!(context.context.command.arguments, ["hello"]);
+        assert_eq!(context.context.hostname, sudo_system::hostname());
+        assert_eq!(context.context.target_user.uid, 0);
         assert_eq!(
             context.target_environment["SUDO_USER"],
-            context.current_user.name
+            context.context.current_user.name
         );
     }
 }
diff --git a/lib/sudo-common/src/env.rs b/lib/sudo-common/src/env.rs
index 76bf0978..118cf7d5 100644
--- a/lib/sudo-common/src/env.rs
+++ b/lib/sudo-common/src/env.rs
@@ -1,5 +1,5 @@
 use crate::{
-    context::{CommandAndArguments, Context},
+    context::{CommandAndArguments, Configuration, Context},
     wildcard_match::wildcard_match,
 };
 use std::collections::{HashMap, HashSet};
@@ -10,13 +10,6 @@ pub type Environment = HashMap<String, String>;
 const PATH_MAILDIR: &str = env!("PATH_MAILDIR");
 const PATH_ZONEINFO: &str = env!("PATH_ZONEINFO");
 
-/// 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()
-        .map(|(k, v)| (k.into(), v.into()))
-        .collect::<Environment>()
-}
-
 /// Formats the command and arguments passed for the SUDO_COMMAND
 /// environment variable. Limit the length of arguments to 4096 bytes to prevent
 /// execve failure for very long argument vectors
@@ -39,7 +32,10 @@ fn format_command(command_and_arguments: &CommandAndArguments) -> String {
 }
 
 /// Construct sudo-specific environment variables
-fn get_extra_env(context: &Context) -> Environment {
+fn get_extra_env(
+    context: &Context,
+    settings: &impl Configuration,
+) -> impl Iterator<Item = (String, String)> {
     let mut extra = vec![
         ("SUDO_COMMAND", format_command(&context.command)),
         ("SUDO_UID", context.current_user.uid.to_string()),
@@ -51,7 +47,7 @@ fn get_extra_env(context: &Context) -> Environment {
         ),
     ];
 
-    // TODO: preserve exsisting when sudo -s
+    // TODO: preserve existing when sudo -s
     if !context.shell {
         extra.push(("SHELL", context.target_user.shell.clone()));
     }
@@ -64,11 +60,13 @@ fn get_extra_env(context: &Context) -> Environment {
         extra.push(("USER", context.target_user.name.clone()));
     }
 
-    if context.always_set_home || context.set_home {
+    if settings.always_set_home() || context.set_home {
         extra.push(("HOME", context.target_user.home.clone()));
     }
 
-    environment_from_list(extra)
+    extra
+        .into_iter()
+        .map(|(name, value)| (name.to_string(), value))
 }
 
 /// Check a string only contains printable (non-space) characters
@@ -111,7 +109,7 @@ fn in_table(needle: &str, haystack: &HashSet<String>) -> bool {
 }
 
 /// Determine whether a specific environment variable should be kept
-fn should_keep(key: &str, value: &str, context: &Context) -> bool {
+fn should_keep(key: &str, value: &str, cfg: &impl Configuration) -> bool {
     if value.starts_with("()") {
         return false;
     }
@@ -120,11 +118,11 @@ fn should_keep(key: &str, value: &str, context: &Context) -> bool {
         return false;
     }
 
-    if in_table(key, context.env_check) && !value.contains(|c| c == '%' || c == '/') {
+    if in_table(key, cfg.env_check()) && !value.contains(|c| c == '%' || c == '/') {
         return true;
     }
 
-    in_table(key, context.env_keep)
+    in_table(key, cfg.env_keep())
 }
 
 /// Construct the final environment from the current one and a sudo context
@@ -140,18 +138,16 @@ fn should_keep(key: &str, value: &str, context: &Context) -> bool {
 /// TODO: If the PATH and TERM variables are not preserved from the user's environment, they will be set to default value
 ///
 /// Environment variables with a value beginning with ‘()’ are removed
-pub fn get_target_environment(current_env: Environment, context: &Context) -> Environment {
-    let mut result = Environment::new();
-
-    for (key, value) in current_env.into_iter() {
-        if should_keep(&key, &value, context) {
-            result.insert(key, value);
-        }
-    }
-
-    result.extend(get_extra_env(context));
-
-    result
+pub fn get_target_environment(
+    current_env: Environment,
+    context: &Context,
+    settings: &impl Configuration,
+) -> Environment {
+    current_env
+        .into_iter()
+        .filter(|(key, value)| should_keep(key, value, settings))
+        .chain(get_extra_env(context, settings))
+        .collect()
 }
 
 #[cfg(test)]
diff --git a/lib/sudo-common/src/exec.rs b/lib/sudo-common/src/exec.rs
index d694c2b1..6e5aeee5 100644
--- a/lib/sudo-common/src/exec.rs
+++ b/lib/sudo-common/src/exec.rs
@@ -3,14 +3,14 @@ use std::{
     process::{Command, ExitStatus},
 };
 
-use crate::{context::Context, error::Error};
+use crate::{context::ContextWithEnv, error::Error};
 
-pub fn exec(context: Context) -> Result<ExitStatus, Error> {
-    Command::new(context.command.command)
-        .args(context.command.arguments)
-        .uid(context.target_user.uid)
-        .gid(context.target_user.gid)
-        .envs(context.target_environment)
+pub fn exec(setup: ContextWithEnv) -> Result<ExitStatus, Error> {
+    Command::new(setup.context.command.command)
+        .args(setup.context.command.arguments)
+        .uid(setup.context.target_user.uid)
+        .gid(setup.context.target_user.gid)
+        .envs(setup.target_environment)
         .status()
         .map_err(|_| Error::Exec)
 }
diff --git a/lib/sudo-defaults/src/strenum.rs b/lib/sudo-defaults/src/strenum.rs
index 1d151845..e9d39761 100644
--- a/lib/sudo-defaults/src/strenum.rs
+++ b/lib/sudo-defaults/src/strenum.rs
@@ -2,7 +2,7 @@
 /// compile-time guarantees as an actual Enum. On the other hand this allows for a bit more
 /// flexibility.
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct StrEnum<'a> {
     pub(super) value: &'a str,
     pub possible_values: &'a [&'a str],
diff --git a/lib/sudoers/src/lib.rs b/lib/sudoers/src/lib.rs
index a8fb97ce..b07d27e2 100644
--- a/lib/sudoers/src/lib.rs
+++ b/lib/sudoers/src/lib.rs
@@ -19,7 +19,6 @@ use tokens::*;
 const INCLUDE_LIMIT: u8 = 128;
 
 /// Export some necessary symbols from modules
-pub use ast::Tag;
 pub use ast::TextEnum;
 pub struct Error(pub Option<basic_parser::Position>, pub String);
 
@@ -27,9 +26,10 @@ pub struct Error(pub Option<basic_parser::Position>, pub String);
 pub struct Sudoers {
     rules: Vec<PermissionSpec>,
     aliases: AliasTable,
-    pub settings: Settings,
+    settings: Settings,
 }
 
+/// A structure that represents what the user wants to do
 pub struct Request<'a, User: UnixUser, Group: UnixGroup> {
     pub user: &'a User,
     pub group: &'a Group,
@@ -37,11 +37,52 @@ pub struct Request<'a, User: UnixUser, Group: UnixGroup> {
     pub arguments: &'a str,
 }
 
-/// This function takes a file argument for a sudoers file and processes it.
+/// Data types that represent what the "terms and conditions" are
+/// (this is currently a stub and should later be turned into a trait and moved to a sudo-policies crate)
+#[derive(Debug)]
+pub struct Policy {
+    flags: Option<Vec<Tag>>,
+    pub settings: Settings, // TODO: hide behind interface
+}
+
+pub enum Authorization {
+    Required,
+    Passed,
+    Forbidden,
+}
+
+impl Policy {
+    pub fn authorization(&self) -> Authorization {
+        if let Some(vec) = &self.flags {
+            if vec.contains(&Tag::NoPasswd) {
+                Authorization::Passed
+            } else {
+                Authorization::Required
+            }
+        } else {
+            Authorization::Forbidden
+        }
+    }
+}
 
-pub fn compile(path: impl AsRef<Path>) -> Result<(Sudoers, Vec<Error>), std::io::Error> {
-    let sudoers = read_sudoers(path.as_ref())?;
-    Ok(analyze(sudoers))
+/// This function takes a file argument for a sudoers file and processes it.
+impl Sudoers {
+    pub fn new(path: impl AsRef<Path>) -> Result<(Sudoers, Vec<Error>), std::io::Error> {
+        let sudoers = read_sudoers(path.as_ref())?;
+        Ok(analyze(sudoers))
+    }
+
+    pub fn check<User: UnixUser + PartialEq<User>, Group: UnixGroup>(
+        &self,
+        am_user: &User,
+        on_host: &str,
+        request: Request<User, Group>,
+    ) -> Policy {
+        Policy {
+            flags: check_permission(self, am_user, on_host, request),
+            settings: self.settings.clone(), // this is wasteful, but in the future this will not be a simple clone and it avoids a lifetime
+        }
+    }
 }
 
 fn read_sudoers(path: &Path) -> Result<Vec<basic_parser::Parsed<Sudo>>, std::io::Error> {
@@ -81,12 +122,8 @@ fn elems<T>(vec: &VecOrd<T>) -> impl Iterator<Item = &T> {
 
 // This code is structure to allow easily reading the 'happy path'; i.e. as soon as something
 // doesn't match, we escape using the '?' mechanism.
-pub fn check_permission<User: UnixUser + PartialEq<User>, Group: UnixGroup>(
-    Sudoers {
-        rules,
-        aliases,
-        settings: _,
-    }: &Sudoers,
+fn check_permission<User: UnixUser + PartialEq<User>, Group: UnixGroup>(
+    Sudoers { rules, aliases, .. }: &Sudoers,
     am_user: &User,
     on_host: &str,
     request: Request<User, Group>,
@@ -241,8 +278,7 @@ fn match_identifier(user: &impl UnixUser, ident: &ast::Identifier) -> bool {
     }
 }
 
-//TODO: don't derive Default, but implement it (based on what the actual defaults are)
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct Settings {
     pub flags: HashSet<String>,
     pub str_value: HashMap<String, String>,
diff --git a/sudo/src/main.rs b/sudo/src/main.rs
index 47437ba4..667d8afb 100644
--- a/sudo/src/main.rs
+++ b/sudo/src/main.rs
@@ -2,13 +2,13 @@
 
 use sudo_cli::SudoOptions;
 use sudo_common::{context::Context, env::Environment, error::Error, pam::authenticate};
-use sudoers::{Sudoers, Tag};
+use sudoers::{Authorization, Sudoers};
 
 fn parse_sudoers() -> Result<Sudoers, Error> {
     // TODO: move to global configuration
     let sudoers_path = "/etc/sudoers.test";
 
-    let (sudoers, syntax_errors) = sudoers::compile(sudoers_path)
+    let (sudoers, syntax_errors) = Sudoers::new(sudoers_path)
         .map_err(|e| Error::Configuration(format!("no sudoers file {e}")))?;
 
     for sudoers::Error(_pos, error) in syntax_errors {
@@ -19,9 +19,8 @@ fn parse_sudoers() -> Result<Sudoers, Error> {
 }
 
 /// 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,
+fn check_sudoers(sudoers: &Sudoers, context: &Context) -> sudoers::Policy {
+    sudoers.check(
         &context.current_user,
         &context.hostname,
         sudoers::Request {
@@ -30,7 +29,7 @@ fn check_sudoers(sudoers: &Sudoers, context: &Context) -> Result<Option<Vec<Tag>
             command: &context.command.command,
             arguments: &context.command.arguments.join(" "),
         },
-    ))
+    )
 }
 
 fn main() -> Result<(), Error> {
@@ -42,22 +41,23 @@ fn main() -> Result<(), Error> {
 
     // build context and environment
     let current_env = std::env::vars().collect::<Environment>();
-    let context = Context::build_from_options(&sudo_options, &sudoers.settings)?
-        .with_filtered_env(current_env);
+    let context = Context::build_from_options(&sudo_options)?;
 
     // check sudoers file for permission
-    match check_sudoers(&sudoers, &context)? {
-        Some(tags) => {
-            if !tags.contains(&Tag::NoPasswd) {
-                // authenticate user using pam
-                authenticate(&context.current_user.name)?;
-            }
+    let policy = check_sudoers(&sudoers, &context);
+    match policy.authorization() {
+        Authorization::Required => {
+            // authenticate user using pam
+            authenticate(&context.current_user.name)?;
         }
-        None => {
+        Authorization::Passed => {}
+        Authorization::Forbidden => {
             return Err(Error::auth("no permission"));
         }
     };
 
+    let context = context.with_filtered_env(current_env, &policy.settings);
+
     // run command and return corresponding exit code
     match sudo_common::exec::exec(context) {
         Ok(status) => {
@@ -68,7 +68,7 @@ fn main() -> Result<(), Error> {
             }
         }
         Err(e) => {
-            eprintln!("{e:?}");
+            eprintln!("{e}");
             std::process::exit(1);
         }
     }


Test patch:
diff --git a/lib/sudo-common/tests/env_tests.rs b/lib/sudo-common/tests/env_tests.rs
index 6b7c5d49..6d0b852f 100644
--- a/lib/sudo-common/tests/env_tests.rs
+++ b/lib/sudo-common/tests/env_tests.rs
@@ -1,7 +1,7 @@
 use std::collections::HashSet;
 use sudo_cli::SudoOptions;
 use sudo_common::{
-    context::{CommandAndArguments, Configuration, Context},
+    context::{CommandAndArguments, Context},
     env::{get_target_environment, Environment},
 };
 use sudo_system::{Group, User};
@@ -70,7 +70,7 @@ fn parse_env_commands(input: &str) -> Vec<(&str, Environment)> {
         .collect()
 }
 
-fn create_test_context<'a>(sudo_options: &'a SudoOptions, settings: &'a Settings) -> Context<'a> {
+fn create_test_context<'a>(sudo_options: &'a SudoOptions) -> Context<'a> {
     let command = CommandAndArguments::try_from(sudo_options.external_args.as_slice()).unwrap();
 
     let current_user = User {
@@ -123,17 +123,11 @@ fn create_test_context<'a>(sudo_options: &'a SudoOptions, settings: &'a Settings
         } else {
             root_group
         },
-        target_environment: Default::default(),
         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(),
     }
 }
 
@@ -149,8 +143,8 @@ fn test_environment_variable_filtering() {
     for (cmd, expected_env) in parts {
         let options = SudoOptions::try_parse_from(cmd.split_whitespace()).unwrap();
         let settings = Settings::default();
-        let context = create_test_context(&options, &settings);
-        let resulting_env = get_target_environment(initial_env.clone(), &context);
+        let context = create_test_context(&options);
+        let resulting_env = get_target_environment(initial_env.clone(), &context, &settings);
 
         let resulting_env = environment_to_set(resulting_env);
         let expected_env = environment_to_set(expected_env);
diff --git a/test-binaries/checkpermit.rs b/test-binaries/checkpermit.rs
index ad66b182..5f350787 100644
--- a/test-binaries/checkpermit.rs
+++ b/test-binaries/checkpermit.rs
@@ -1,5 +1,6 @@
 use std::env;
 use sudo_system::interface::{UnixGroup, UnixUser};
+use sudoers::Sudoers;
 
 fn chatty_check_permission(
     sudoers: sudoers::Sudoers,
@@ -16,8 +17,8 @@ fn chatty_check_permission(
         let mut items = chosen_poison.split_whitespace();
         (items.next().unwrap(), items.collect::<Vec<_>>().join(" "))
     };
-    let result = sudoers::check_permission(
-        &sudoers,
+
+    let result = sudoers.check(
         &am_user,
         on_host,
         sudoers::Request {
@@ -125,14 +126,13 @@ fn fancy_error(x: usize, y: usize, path: &str) {
 
 fn main() {
     let args: Vec<String> = env::args().collect();
-    if let Ok((cfg, warn)) = sudoers::compile("./sudoers") {
+    if let Ok((cfg, warn)) = Sudoers::new("./sudoers") {
         for sudoers::Error(pos, msg) in warn {
             if let Some((x, y)) = pos {
                 fancy_error(x, y, "./sudoers");
             }
             eprintln!("{msg}");
         }
-        println!("SETTINGS: {:?}", cfg.settings);
         println!(
             "{:?}",
             chatty_check_permission(