Repository: memorysafety/sudo-rs
Pull request #151 (2023-03-27 21:58:36)

Policy infrastructure
Small code changes, but a pretty influential step: nothing from the sudoers file leaks out except through the `Policy` trait (there is still a dependency on the sudoers crate since it defines the `Policy` trait, but that is all).

Note that this does not allow for any kind of 'pre-permission check' and 'post-permission check' configuration option retrieval. This is a clear way to do things, but it will come into conflict when we start adding more features from original sudo. There are two possible ways this can go:

1) Anything that needs to be checked "before" asking the permission checking routine has to be isolated in the sudoers crate as well (generally, the only reason to check them "before" is because they might influence the information passed to the checker; but in that case the checker can also pass that information 'to itself')

2) A second trait 'UncheckedPolicy' or some such will eventually be created and will become a supertrait of 'Policy'; the sudoers object implements the former, the judgement object implements the latter.

But for now, this works.



Patch:
diff --git a/lib/sudo-common/Cargo.toml b/lib/sudo-common/Cargo.toml
index 094056fb..9d60824f 100644
--- a/lib/sudo-common/Cargo.toml
+++ b/lib/sudo-common/Cargo.toml
@@ -11,7 +11,6 @@ publish.workspace = true
 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 3bb481fe..bf3fac3a 100644
--- a/lib/sudo-common/src/context.rs
+++ b/lib/sudo-common/src/context.rs
@@ -1,11 +1,6 @@
-use std::{
-    collections::{HashMap, HashSet},
-    path::PathBuf,
-    str::FromStr,
-};
+use std::{collections::HashMap, path::PathBuf, str::FromStr};
 use sudo_cli::SudoOptions;
 use sudo_system::{hostname, Group, User};
-use sudoers::Settings;
 
 use crate::error::Error;
 
@@ -26,7 +21,7 @@ impl<'a> TryFrom<&'a [String]> for CommandAndArguments<'a> {
         let command = iter.next().ok_or(Error::InvalidCommand)?.to_string();
 
         // resolve the binary if the path is not absolute
-        let command = if command.starts_with("/") {
+        let command = if command.starts_with('/') {
             PathBuf::from(command)
         } else {
             // TODO: we resolve in the context of the current user using the 'which' crate - we want to reconsider this in the future
@@ -78,25 +73,6 @@ pub struct Context<'a> {
     pub pid: i32,
 }
 
-pub trait Configuration {
-    fn env_keep(&self) -> &HashSet<String>;
-    fn env_check(&self) -> &HashSet<String>;
-}
-
-impl Configuration for 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 resolve_current_user() -> Result<User, Error> {
     User::real()?.ok_or(Error::UserNotFound("current user".to_string()))
 }
diff --git a/lib/sudo-env/Cargo.toml b/lib/sudo-env/Cargo.toml
index 82122cb8..d29e26c7 100644
--- a/lib/sudo-env/Cargo.toml
+++ b/lib/sudo-env/Cargo.toml
@@ -11,7 +11,7 @@ build = "build.rs"
 [dependencies]
 sudo-system.workspace = true
 sudo-common.workspace = true
+sudoers.workspace = true
 
 [dev-dependencies]
 sudo-cli.workspace = true
-sudoers.workspace = true
\ No newline at end of file
diff --git a/lib/sudo-env/src/environment.rs b/lib/sudo-env/src/environment.rs
index 53679390..652d0f8b 100644
--- a/lib/sudo-env/src/environment.rs
+++ b/lib/sudo-env/src/environment.rs
@@ -1,7 +1,8 @@
 use std::collections::HashSet;
 
-use sudo_common::context::{CommandAndArguments, Configuration, Context, Environment};
+use sudo_common::context::{CommandAndArguments, Context, Environment};
 use sudo_system::PATH_MAX;
+use sudoers::Policy;
 
 use crate::wildcard_match::wildcard_match;
 
@@ -105,7 +106,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, cfg: &impl Configuration) -> bool {
+fn should_keep(key: &str, value: &str, cfg: &impl Policy) -> bool {
     if value.starts_with("()") {
         return false;
     }
@@ -137,7 +138,7 @@ fn should_keep(key: &str, value: &str, cfg: &impl Configuration) -> bool {
 pub fn get_target_environment(
     current_env: Environment,
     context: &Context,
-    settings: &impl Configuration,
+    settings: &impl Policy,
 ) -> Environment {
     let mut environment = current_env
         .into_iter()
@@ -152,7 +153,7 @@ pub fn get_target_environment(
 #[cfg(test)]
 mod tests {
     use std::collections::HashSet;
-    use sudo_common::context::Configuration;
+    use sudoers::Policy;
 
     use crate::environment::{is_safe_tz, should_keep, PATH_ZONEINFO};
 
@@ -161,7 +162,7 @@ mod tests {
         check: HashSet<String>,
     }
 
-    impl Configuration for TestConfiguration {
+    impl Policy for TestConfiguration {
         fn env_keep(&self) -> &HashSet<String> {
             &self.keep
         }
diff --git a/lib/sudoers/src/lib.rs b/lib/sudoers/src/lib.rs
index cf92454c..7f8eac08 100644
--- a/lib/sudoers/src/lib.rs
+++ b/lib/sudoers/src/lib.rs
@@ -37,33 +37,14 @@ pub struct Request<'a, User: UnixUser, Group: UnixGroup> {
     pub arguments: &'a str,
 }
 
-/// 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 {
+#[derive(Debug, Default)]
+pub struct Judgement {
     flags: Option<Tag>,
-    pub settings: Settings, // TODO: hide behind interface
-}
-
-pub enum Authorization {
-    Required,
-    Passed,
-    Forbidden,
+    settings: Settings,
 }
 
-impl Policy {
-    pub fn authorization(&self) -> Authorization {
-        if let Some(tag) = &self.flags {
-            if !tag.passwd {
-                Authorization::Passed
-            } else {
-                Authorization::Required
-            }
-        } else {
-            Authorization::Forbidden
-        }
-    }
-}
+mod policy;
+pub use policy::{Authorization, Policy};
 
 /// This function takes a file argument for a sudoers file and processes it.
 impl Sudoers {
@@ -77,7 +58,7 @@ impl Sudoers {
         am_user: &User,
         on_host: &str,
         request: Request<User, Group>,
-    ) -> Policy {
+    ) -> Judgement {
         // exception: if user is root or does not switch users, NOPASSWD is implied
         let skip_passwd =
             am_user.is_root() || (request.user == am_user && in_group(am_user, request.group));
@@ -89,7 +70,7 @@ impl Sudoers {
             }
         }
 
-        Policy {
+        Judgement {
             flags,
             settings: self.settings.clone(), // this is wasteful, but in the future this will not be a simple clone and it avoids a lifetime
         }
diff --git a/lib/sudoers/src/policy.rs b/lib/sudoers/src/policy.rs
new file mode 100644
index 00000000..edcfc236
--- /dev/null
+++ b/lib/sudoers/src/policy.rs
@@ -0,0 +1,49 @@
+use super::Judgement;
+/// Data types and traits that represent what the "terms and conditions" are after a succesful
+/// permission check.
+///
+/// The trait definitions can be part of some global crate in the future, if we support more
+/// than just the sudoers file.
+use std::collections::HashSet;
+
+pub trait Policy {
+    fn env_keep(&self) -> &HashSet<String>;
+    fn env_check(&self) -> &HashSet<String>;
+    fn authorization(&self) -> Authorization {
+        Authorization::Forbidden
+    }
+}
+
+pub enum Authorization {
+    Required,
+    Passed,
+    Forbidden,
+}
+
+impl Policy for Judgement {
+    fn authorization(&self) -> Authorization {
+        if let Some(tag) = &self.flags {
+            if !tag.passwd {
+                Authorization::Passed
+            } else {
+                Authorization::Required
+            }
+        } else {
+            Authorization::Forbidden
+        }
+    }
+
+    fn env_keep(&self) -> &HashSet<String> {
+        self.settings
+            .list
+            .get("env_keep")
+            .expect("env_keep missing from settings")
+    }
+
+    fn env_check(&self) -> &HashSet<String> {
+        self.settings
+            .list
+            .get("env_check")
+            .expect("env_check missing from settings")
+    }
+}
diff --git a/sudo/src/main.rs b/sudo/src/main.rs
index 927f95bb..fbab88b7 100644
--- a/sudo/src/main.rs
+++ b/sudo/src/main.rs
@@ -7,7 +7,7 @@ use sudo_common::{
     error::Error,
 };
 use sudo_env::environment;
-use sudoers::{Authorization, Sudoers};
+use sudoers::{Authorization, Policy, Sudoers};
 
 mod diagnostic;
 mod pam;
@@ -27,7 +27,7 @@ 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) -> sudoers::Policy {
+fn check_sudoers(sudoers: &Sudoers, context: &Context) -> sudoers::Judgement {
     sudoers.check(
         &context.current_user,
         &context.hostname,
@@ -82,7 +82,7 @@ fn main() -> Result<(), Error> {
         }
     };
 
-    let target_env = environment::get_target_environment(current_env, &context, &policy.settings);
+    let target_env = environment::get_target_environment(current_env, &context, &policy);
 
     // run command and return corresponding exit code
     match sudo_exec::run_command(context, target_env) {


Test patch:
diff --git a/lib/sudo-env/tests/env_tests.rs b/lib/sudo-env/tests/env_tests.rs
index a0b57d51..6ee31888 100644
--- a/lib/sudo-env/tests/env_tests.rs
+++ b/lib/sudo-env/tests/env_tests.rs
@@ -3,7 +3,6 @@ use sudo_cli::SudoOptions;
 use sudo_common::context::{CommandAndArguments, Context, Environment};
 use sudo_env::environment::get_target_environment;
 use sudo_system::{Group, User};
-use sudoers::Settings;
 
 const TESTS: &str = "
 > env
@@ -147,7 +146,7 @@ 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 settings = sudoers::Judgement::default();
         let context = create_test_context(&options);
         let resulting_env = get_target_environment(initial_env.clone(), &context, &settings);