Make the Context immutable (#1372)

squell · web-flow · commit 71a2eb7331ee · 2025-12-08T13:02:59.000+01:00
diff --git a/src/common/context.rs b/src/common/context.rs
@@ -1,15 +1,16 @@
-use std::{env, io};
+use std::env;
 
-use crate::common::{HARDENED_ENUM_VALUE_0, HARDENED_ENUM_VALUE_1, HARDENED_ENUM_VALUE_2};
-use crate::exec::{RunOptions, Umask};
+use crate::common::{Error, HARDENED_ENUM_VALUE_0, HARDENED_ENUM_VALUE_1, HARDENED_ENUM_VALUE_2};
+use crate::exec::RunOptions;
 use crate::sudo::{SudoEditOptions, SudoListOptions, SudoRunOptions, SudoValidateOptions};
 use crate::sudoers::Sudoers;
+use crate::sudoers::{DirChange, Restrictions};
 use crate::system::{audit::sudo_call, Group, Hostname, Process, User};
 
 use super::{
     command::CommandAndArguments,
     resolve::{resolve_shell, resolve_target_user_and_group, CurrentUser},
-    Error, SudoPath,
+    SudoPath,
 };
 
 #[derive(Debug)]
@@ -29,11 +30,6 @@ pub struct Context {
     pub hostname: Hostname,
     pub current_user: CurrentUser,
     pub process: Process,
-    // policy
-    pub use_pty: bool,
-    pub noexec: bool,
-    pub umask: Umask,
-    pub noninteractive_auth: bool,
     // sudoedit
     pub files_to_edit: Vec<Option<SudoPath>>,
 }
@@ -101,10 +97,6 @@ impl Context {
             prompt,
             non_interactive: sudo_options.non_interactive,
             process: Process::new(),
-            use_pty: true,
-            noexec: false,
-            umask: Umask::Preserve,
-            noninteractive_auth: false,
             files_to_edit: vec![],
         })
     }
@@ -172,10 +164,6 @@ impl Context {
             prompt: sudo_options.prompt,
             non_interactive: sudo_options.non_interactive,
             process: Process::new(),
-            use_pty: true,
-            noexec: false,
-            umask: Umask::Preserve,
-            noninteractive_auth: false,
             files_to_edit,
         })
     }
@@ -199,10 +187,6 @@ impl Context {
             prompt: sudo_options.prompt,
             non_interactive: sudo_options.non_interactive,
             process: Process::new(),
-            use_pty: true,
-            noexec: false,
-            umask: Umask::Preserve,
-            noninteractive_auth: false,
             files_to_edit: vec![],
         })
     }
@@ -249,31 +233,50 @@ impl Context {
             prompt: sudo_options.prompt,
             non_interactive: sudo_options.non_interactive,
             process: Process::new(),
-            use_pty: true,
-            noexec: false,
-            umask: Umask::Preserve,
-            noninteractive_auth: false,
             files_to_edit: vec![],
         })
     }
 
-    pub(crate) fn try_as_run_options(&self) -> io::Result<RunOptions<'_>> {
+    pub(crate) fn try_as_run_options(
+        &self,
+        controls: &Restrictions,
+    ) -> Result<RunOptions<'_>, Error> {
+        // see if the chdir flag is permitted
+        let mut chdir = match controls.chdir {
+            DirChange::Any => self.chdir.clone(),
+            DirChange::Strict(optdir) => {
+                if let Some(chdir) = &self.chdir {
+                    return Err(Error::ChDirNotAllowed {
+                        chdir: chdir.clone(),
+                        command: self.command.command.clone(),
+                    });
+                } else {
+                    optdir.cloned()
+                }
+            }
+        };
+
+        // expand tildes in the path with the users home directory
+        if let Some(dir) = chdir.take() {
+            chdir = Some(dir.expand_tilde_in_path(&self.target_user.name)?);
+        }
+
         Ok(RunOptions {
             command: if self.command.resolved {
                 &self.command.command
             } else {
-                return Err(io::ErrorKind::NotFound.into());
+                return Err(Error::CommandNotFound(self.command.command.clone()));
             },
             arguments: &self.command.arguments,
             arg0: self.command.arg0.as_deref(),
-            chdir: self.chdir.as_deref(),
+            chdir: chdir.as_deref().map(ToOwned::to_owned),
             is_login: self.launch == LaunchType::Login,
             user: &self.target_user,
             group: &self.target_group,
-            umask: self.umask,
+            umask: controls.umask,
 
-            use_pty: self.use_pty,
-            noexec: self.noexec,
+            use_pty: controls.use_pty,
+            noexec: controls.noexec,
         })
     }
 }
diff --git a/src/exec/mod.rs b/src/exec/mod.rs
@@ -12,7 +12,7 @@ use std::{
     io,
     os::unix::ffi::OsStrExt,
     os::unix::process::CommandExt,
-    path::Path,
+    path::{Path, PathBuf},
     process::Command,
     time::Duration,
 };
@@ -65,7 +65,7 @@ pub struct RunOptions<'a> {
     pub command: &'a Path,
     pub arguments: &'a [String],
     pub arg0: Option<&'a Path>,
-    pub chdir: Option<&'a Path>,
+    pub chdir: Option<PathBuf>,
     pub is_login: bool,
     pub user: &'a User,
     pub group: &'a Group,
@@ -120,6 +120,7 @@ pub fn run_command(
     // Decide if the pwd should be changed. `--chdir` takes precedence over `-i`.
     let path = options
         .chdir
+        .as_ref()
         .map(|chdir| chdir.to_owned())
         .or_else(|| options.is_login.then(|| options.user.home.clone().into()))
         .clone();
diff --git a/src/sudo/env/environment.rs b/src/sudo/env/environment.rs
@@ -281,7 +281,6 @@ mod tests {
                         #[cfg(feature = "apparmor")]
                         apparmor_profile: None,
                         noexec: false,
-                        noninteractive_auth: false,
                     }
                 ),
                 expected,
diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs
@@ -1,6 +1,5 @@
 use crate::common::resolve::CurrentUser;
 use crate::common::{CommandAndArguments, Context};
-use crate::exec::Umask;
 use crate::sudo::{
     cli::{SudoAction, SudoRunOptions},
     env::environment::{get_target_environment, Environment},
@@ -132,11 +131,7 @@ fn create_test_context(sudo_options: SudoRunOptions) -> Context {
         non_interactive: sudo_options.non_interactive,
         process: Process::new(),
         use_session_records: false,
-        use_pty: true,
-        noexec: false,
         bell: false,
-        umask: Umask::Preserve,
-        noninteractive_auth: false,
         files_to_edit: vec![],
     }
 }
@@ -175,7 +170,6 @@ fn test_environment_variable_filtering() {
                 chdir: crate::sudoers::DirChange::Strict(None),
                 trust_environment: false,
                 umask: crate::exec::Umask::Preserve,
-                noninteractive_auth: false,
                 #[cfg(feature = "apparmor")]
                 apparmor_profile: None,
                 noexec: false,
diff --git a/src/sudo/pipeline.rs b/src/sudo/pipeline.rs
@@ -11,9 +11,7 @@ use crate::log::{auth_info, auth_warn};
 use crate::pam::PamContext;
 use crate::sudo::env::environment;
 use crate::sudo::pam::{attempt_authenticate, init_pam, pre_exec, InitPamArgs};
-use crate::sudoers::{
-    AuthenticatingUser, Authentication, Authorization, DirChange, Judgement, Restrictions, Sudoers,
-};
+use crate::sudoers::{AuthenticatingUser, Authentication, Authorization, Judgement, Sudoers};
 use crate::system::term::current_tty_name;
 use crate::system::timestamp::{RecordScope, SessionRecordFile, TouchResult};
 use crate::system::{escape_os_str_lossy, Process};
@@ -61,15 +59,14 @@ pub fn run(mut cmd_opts: SudoRunOptions) -> Result<(), Error> {
 
     let user_requested_env_vars = std::mem::take(&mut cmd_opts.env_var_list);
 
-    let mut context = Context::from_run_opts(cmd_opts, &mut policy)?;
+    let context = Context::from_run_opts(cmd_opts, &mut policy)?;
 
     let policy = judge(policy, &context)?;
 
     let Authorization::Allowed(auth, controls) = policy.authorization() else {
         return Err(Error::Authorization(context.current_user.name.to_string()));
     };
 
-    apply_policy_to_context(&mut context, &controls)?;
     let mut pam_context = auth_and_update_record_file(&context, auth)?;
 
     // build environment
@@ -96,25 +93,19 @@ pub fn run(mut cmd_opts: SudoRunOptions) -> Result<(), Error> {
 
     // prepare switch of apparmor profile
     #[cfg(feature = "apparmor")]
-    if let Some(profile) = controls.apparmor_profile {
-        crate::apparmor::set_profile_for_next_exec(&profile)
-            .map_err(|err| Error::AppArmor(profile, err))?;
+    if let Some(profile) = &controls.apparmor_profile {
+        crate::apparmor::set_profile_for_next_exec(profile)
+            .map_err(|err| Error::AppArmor(profile.clone(), err))?;
     }
 
+    let options = context.try_as_run_options(&controls)?;
+
+    // Log after try_as_run_options to avoid logging if the command is not resolved
+    log_command_execution(&context);
+
     // run command and return corresponding exit code
-    let command_exit_reason = if context.command.resolved {
-        log_command_execution(&context);
-
-        crate::exec::run_command(
-            context
-                .try_as_run_options()
-                .map_err(|io_error| Error::Io(Some(context.command.command.clone()), io_error))?,
-            target_env,
-        )
-        .map_err(|io_error| Error::Io(Some(context.command.command), io_error))
-    } else {
-        Err(Error::CommandNotFound(context.command.command))
-    };
+    let command_exit_reason = crate::exec::run_command(options, target_env)
+        .map_err(|io_error| Error::Io(Some(context.command.command), io_error));
 
     pam_context.close_session();
 
@@ -154,6 +145,7 @@ fn auth_and_update_record_file(
         password_timeout,
         ref credential,
         pwfeedback,
+        noninteractive_auth,
     }: Authentication,
 ) -> Result<PamContext, Error> {
     let auth_user = match credential {
@@ -190,7 +182,7 @@ fn auth_and_update_record_file(
         hostname: &context.hostname,
     })?;
     if auth_status.must_authenticate {
-        if context.non_interactive && !context.noninteractive_auth {
+        if context.non_interactive && !noninteractive_auth {
             return Err(Error::InteractionRequired);
         }
 
@@ -213,40 +205,6 @@ fn auth_and_update_record_file(
     Ok(pam_context)
 }
 
-fn apply_policy_to_context(
-    context: &mut Context,
-    controls: &Restrictions,
-) -> Result<(), crate::common::Error> {
-    // see if the chdir flag is permitted
-    match controls.chdir {
-        DirChange::Any => {}
-        DirChange::Strict(optdir) => {
-            if let Some(chdir) = &context.chdir {
-                return Err(Error::ChDirNotAllowed {
-                    chdir: chdir.clone(),
-                    command: context.command.command.clone(),
-                });
-            } else {
-                context.chdir = optdir.cloned();
-            }
-        }
-    }
-
-    // expand tildes in the path with the users home directory
-    if let Some(dir) = context.chdir.take() {
-        context.chdir = Some(dir.expand_tilde_in_path(&context.target_user.name)?)
-    }
-
-    // in case the user could set these from the commandline, something more fancy
-    // could be needed, but here we copy these -- perhaps we should split up the Context type
-    context.use_pty = controls.use_pty;
-    context.noexec = controls.noexec;
-    context.umask = controls.umask;
-    context.noninteractive_auth = controls.noninteractive_auth;
-
-    Ok(())
-}
-
 /// This should determine what the authentication status for the given record
 /// match limit and origin/target user from the context is.
 fn determine_auth_status(
diff --git a/src/sudo/pipeline/edit.rs b/src/sudo/pipeline/edit.rs
@@ -10,15 +10,14 @@ use crate::system::audit;
 pub fn run_edit(edit_opts: SudoEditOptions) -> Result<(), Error> {
     let policy = super::read_sudoers()?;
 
-    let mut context = Context::from_edit_opts(edit_opts)?;
+    let context = Context::from_edit_opts(edit_opts)?;
 
     let policy = super::judge(policy, &context)?;
 
-    let Authorization::Allowed(auth, controls) = policy.authorization() else {
+    let Authorization::Allowed(auth, _controls) = policy.authorization() else {
         return Err(Error::Authorization(context.current_user.name.to_string()));
     };
 
-    super::apply_policy_to_context(&mut context, &controls)?;
     let mut pam_context = super::auth_and_update_record_file(&context, auth)?;
 
     let pid = context.process.pid;
diff --git a/src/sudoers/policy.rs b/src/sudoers/policy.rs
@@ -32,6 +32,7 @@ pub struct Authentication {
     pub prior_validity: Duration,
     pub pwfeedback: bool,
     pub password_timeout: Option<Duration>,
+    pub noninteractive_auth: bool,
 }
 
 impl super::Settings {
@@ -45,6 +46,7 @@ impl super::Settings {
                 0 => None,
                 timeout => Some(Duration::from_secs(timeout)),
             },
+            noninteractive_auth: self.noninteractive_auth(),
             credential: if self.rootpw() {
                 AuthenticatingUser::Root
             } else if self.targetpw() {
@@ -67,7 +69,6 @@ pub struct Restrictions<'a> {
     pub chdir: DirChange<'a>,
     pub path: Option<&'a str>,
     pub umask: Umask,
-    pub noninteractive_auth: bool,
     #[cfg(feature = "apparmor")]
     pub apparmor_profile: Option<String>,
 }
@@ -132,7 +133,6 @@ impl Judgement {
                             Umask::Extend(mask)
                         }
                     },
-                    noninteractive_auth: self.settings.noninteractive_auth(),
                     #[cfg(feature = "apparmor")]
                     apparmor_profile: tag
                         .apparmor_profile
@@ -195,6 +195,7 @@ mod test {
                 prior_validity: Duration::from_secs(15 * 60),
                 credential: AuthenticatingUser::InvokingUser,
                 pwfeedback: false,
+                noninteractive_auth: false,
                 password_timeout: Some(Duration::from_secs(300)),
             },
         );
@@ -212,6 +213,7 @@ mod test {
                 prior_validity: Duration::from_secs(15 * 60),
                 credential: AuthenticatingUser::InvokingUser,
                 pwfeedback: false,
+                noninteractive_auth: false,
                 password_timeout: Some(Duration::from_secs(300)),
             },
         );

Original file line number	Diff line number	Diff line change
`@@ -281,7 +281,6 @@ mod tests {`
`281`	`281`	`#[cfg(feature = "apparmor")]`
`282`	`282`	`apparmor_profile: None,`
`283`	`283`	`noexec: false,`
`284`		`- noninteractive_auth: false,`
`285`	`284`	`}`
`286`	`285`	`),`
`287`	`286`	`expected,`