Various pam changes (#982)

src/common/error.rs

@@ -80,7 +80,7 @@ impl fmt::Display for Error {
             }
             Error::Configuration(e) => write!(f, "invalid configuration: {e}"),
             Error::Options(e) => write!(f, "{e}"),
-            Error::Pam(e) => write!(f, "PAM error: {e}"),
+            Error::Pam(e) => write!(f, "{e}"),
             Error::Io(location, e) => {
                 if let Some(path) = location {
                     write!(f, "cannot execute '{}': {e}", path.display())

src/pam/error.rs

@@ -171,11 +171,8 @@ impl PamErrorType {
 pub enum PamError {
     UnexpectedNulByte(NulError),
     Utf8Error(Utf8Error),
-    InvalidState,
-    Pam(PamErrorType, String),
+    Pam(PamErrorType),
     IoError(std::io::Error),
-    SessionAlreadyOpen,
-    SessionNotOpen,
     EnvListFailure,
     InteractionRequired,
 }
@@ -203,18 +200,20 @@ impl fmt::Display for PamError {
         match self {
             PamError::UnexpectedNulByte(_) => write!(f, "Unexpected nul byte in input"),
             PamError::Utf8Error(_) => write!(f, "Could not read input data as UTF-8 string"),
-            PamError::InvalidState => {
+            PamError::Pam(PamErrorType::AuthError) => {
+                write!(f, "Account validation failure, is your account locked?")
+            }
+            PamError::Pam(PamErrorType::NewAuthTokenRequired) => {
                 write!(
                     f,
-                    "Could not initiate pam because the state is not complete"
+                    "Account or password is expired, reset your password and try again"
                 )
             }
-            PamError::Pam(tp, msg) => write!(f, "PAM returned an error ({tp:?}): {msg}"),
-            PamError::IoError(e) => write!(f, "IO error: {e}"),
-            PamError::SessionAlreadyOpen => {
-                write!(f, "Cannot open session while one is already open")
+            PamError::Pam(PamErrorType::AuthTokenExpired) => {
+                write!(f, "Password expired, contact your system administrator")
             }
-            PamError::SessionNotOpen => write!(f, "Cannot close session while none is open"),
+            PamError::Pam(tp) => write!(f, "PAM error: {}", tp.get_err_msg()),
+            PamError::IoError(e) => write!(f, "IO error: {e}"),
             PamError::EnvListFailure => {
                 write!(
                     f,
@@ -227,12 +226,10 @@ impl fmt::Display for PamError {
 }
 
 impl PamError {
-    /// Create a new PamError based on the error number from pam and a handle to a pam session
-    /// The handle to the pam session is allowed to be null
+    /// Create a new PamError based on the error number from pam.
     pub(super) fn from_pam(errno: libc::c_int) -> PamError {
         let tp = PamErrorType::from_int(errno);
-        let msg = tp.get_err_msg();
-        PamError::Pam(tp, msg)
+        PamError::Pam(tp)
     }
 }
 

src/pam/mod.rs

@@ -1,5 +1,6 @@
 use std::{
     ffi::{CStr, CString, OsStr, OsString},
+    io,
     os::raw::c_char,
     os::unix::prelude::OsStrExt,
     ptr::NonNull,
@@ -34,103 +35,81 @@ pub struct PamContext<C: Converser> {
     session_started: bool,
 }
 
-pub struct PamContextBuilder<C> {
-    converser: Option<C>,
-    service_name: Option<String>,
-    target_user: Option<String>,
-}
-
-impl<C: Converser> PamContextBuilder<C> {
-    /// Build the PamContext based on the current configuration.
-    ///
-    /// This function will error when the required settings have not yet been
-    /// set, or when initialization of the PAM session somehow failed.
-    pub fn build(self) -> PamResult<PamContext<C>> {
-        if let (Some(converser), Some(service_name)) = (self.converser, self.service_name) {
-            let c_service_name = CString::new(service_name)?;
-            let c_user = self.target_user.map(CString::new).transpose()?;
-            let c_user_ptr = match c_user {
-                Some(ref c) => c.as_ptr(),
-                None => std::ptr::null(),
-            };
-
-            // this will be de-allocated explicitly in this type's drop method
-            let data_ptr = Box::into_raw(Box::new(ConverserData {
-                converser,
-                panicked: false,
-            }));
-
-            let mut pamh = std::ptr::null_mut();
-            // SAFETY: we are passing the required fields to `pam_start`; in particular, the value
-            // of `pamh` set above is not used, but will be overwritten by `pam_start`.
-            let res = unsafe {
-                pam_start(
-                    c_service_name.as_ptr(),
-                    c_user_ptr,
-                    &pam_conv {
-                        conv: Some(converse::converse::<C>),
-                        appdata_ptr: data_ptr as *mut libc::c_void,
-                    },
-                    &mut pamh,
-                )
-            };
-
-            pam_err(res)?;
-
-            if pamh.is_null() {
-                Err(PamError::InvalidState)
-            } else {
-                Ok(PamContext {
-                    data_ptr,
-                    pamh,
-                    silent: false,
-                    allow_null_auth_token: true,
-                    last_pam_status: None,
-                    session_started: false,
-                })
-            }
-        } else {
-            Err(PamError::InvalidState)
-        }
-    }
-
-    /// Set a converser implementation that will be used for the PAM conversation.
-    pub fn converser(mut self, converser: C) -> PamContextBuilder<C> {
-        self.converser = Some(converser);
-        self
-    }
-
-    /// Set the service name for the PAM session.
-    ///
-    /// Note that the service name should be based on a static string and not
-    /// based on the name of the binary.
-    pub fn service_name<T: Into<String>>(mut self, name: T) -> PamContextBuilder<C> {
-        self.service_name = Some(name.into());
-        self
-    }
-
-    /// Set a target user that should be inserted into the pam context.
+impl PamContext<CLIConverser> {
+    /// Build the PamContext with the CLI conversation function.
     ///
     /// The target user is optional and may also be set after the context was
     /// constructed or not set at all in which case PAM will ask for a
     /// username.
-    pub fn target_user<T: Into<String>>(mut self, user: T) -> PamContextBuilder<C> {
-        self.target_user = Some(user.into());
-        self
-    }
-}
+    ///
+    /// This function will error when initialization of the PAM session somehow failed.
+    pub fn new_cli(
+        converser_name: &str,
+        service_name: &str,
+        use_stdin: bool,
+        no_interact: bool,
+        password_feedback: bool,
+        target_user: Option<&str>,
+    ) -> PamResult<PamContext<CLIConverser>> {
+        let converser = CLIConverser {
+            name: converser_name.to_owned(),
+            use_stdin,
+            no_interact,
+            password_feedback,
+        };
 
-impl<C> Default for PamContextBuilder<C> {
-    fn default() -> Self {
-        Self {
-            converser: None,
-            service_name: None,
-            target_user: None,
-        }
+        Self::new(converser, service_name, target_user)
     }
 }
 
 impl<C: Converser> PamContext<C> {
+    fn new(
+        converser: C,
+        service_name: &str,
+        target_user: Option<&str>,
+    ) -> PamResult<PamContext<C>> {
+        let c_service_name = CString::new(service_name)?;
+        let c_user = target_user.map(CString::new).transpose()?;
+        let c_user_ptr = match c_user {
+            Some(ref c) => c.as_ptr(),
+            None => std::ptr::null(),
+        };
+
+        // this will be de-allocated explicitly in this type's drop method
+        let data_ptr = Box::into_raw(Box::new(ConverserData {
+            converser,
+            panicked: false,
+        }));
+
+        let mut pamh = std::ptr::null_mut();
+        // SAFETY: we are passing the required fields to `pam_start`; in particular, the value
+        // of `pamh` set above is not used, but will be overwritten by `pam_start`.
+        let res = unsafe {
+            pam_start(
+                c_service_name.as_ptr(),
+                c_user_ptr,
+                &pam_conv {
+                    conv: Some(converse::converse::<C>),
+                    appdata_ptr: data_ptr as *mut libc::c_void,
+                },
+                &mut pamh,
+            )
+        };
+
+        pam_err(res)?;
+
+        assert!(!pamh.is_null());
+
+        Ok(PamContext {
+            data_ptr,
+            pamh,
+            silent: false,
+            allow_null_auth_token: true,
+            last_pam_status: None,
+            session_started: false,
+        })
+    }
+
     /// Set whether output of pam calls should be silent or not, by default
     /// PAM calls are not silent.
     pub fn mark_silent(&mut self, silent: bool) {
@@ -192,7 +171,7 @@ impl<C: Converser> PamContext<C> {
         let check_val = self.validate_account();
         match check_val {
             Ok(()) => Ok(()),
-            Err(PamError::Pam(PamErrorType::NewAuthTokenRequired, _)) => {
+            Err(PamError::Pam(PamErrorType::NewAuthTokenRequired)) => {
                 self.change_auth_token(true)?;
                 Ok(())
             }
@@ -223,7 +202,10 @@ impl<C: Converser> PamContext<C> {
         // safety check to make sure that we were not passed a null pointer by PAM,
         // or that in fact PAM did not write to `data` at all.
         if data.is_null() {
-            return Err(PamError::InvalidState);
+            return Err(PamError::IoError(io::Error::new(
+                io::ErrorKind::InvalidData,
+                "PAM didn't return username",
+            )));
         }
 
         // SAFETY: the contract for `pam_get_item` ensures that if `data` was touched by
@@ -292,25 +274,22 @@ impl<C: Converser> PamContext<C> {
 
     /// Start a user session for the authenticated user.
     pub fn open_session(&mut self) -> PamResult<()> {
-        if !self.session_started {
-            // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
-            pam_err(unsafe { pam_open_session(self.pamh, self.silent_flag()) })?;
-            self.session_started = true;
-            Ok(())
-        } else {
-            Err(PamError::SessionAlreadyOpen)
-        }
+        assert!(!self.session_started);
+
+        // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
+        pam_err(unsafe { pam_open_session(self.pamh, self.silent_flag()) })?;
+        self.session_started = true;
+        Ok(())
     }
 
     /// End the user session.
-    pub fn close_session(&mut self) -> PamResult<()> {
+    pub fn close_session(&mut self) {
+        // closing the pam session is best effort, if any error occurs we cannot
+        // do anything with it
         if self.session_started {
             // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
-            pam_err(unsafe { pam_close_session(self.pamh, self.silent_flag()) })?;
+            let _ = pam_err(unsafe { pam_close_session(self.pamh, self.silent_flag()) });
             self.session_started = false;
-            Ok(())
-        } else {
-            Err(PamError::SessionNotOpen)
         }
     }
 
@@ -372,29 +351,12 @@ impl<C: Converser> PamContext<C> {
     }
 }
 
-impl PamContext<CLIConverser> {
-    /// Create a builder that uses the CLI conversation function.
-    pub fn builder_cli(
-        name: &str,
-        use_stdin: bool,
-        no_interact: bool,
-        password_feedback: bool,
-    ) -> PamContextBuilder<CLIConverser> {
-        PamContextBuilder::default().converser(CLIConverser {
-            name: name.to_owned(),
-            use_stdin,
-            no_interact,
-            password_feedback,
-        })
-    }
-}
-
 impl<C: Converser> Drop for PamContext<C> {
     fn drop(&mut self) {
         // data_ptr's pointee is de-allocated in this scope
         // SAFETY: self.data_ptr was created by Box::into_raw
         let _data = unsafe { Box::from_raw(self.data_ptr) };
-        let _ = self.close_session();
+        self.close_session();
 
         // It looks like PAM_DATA_SILENT is important to set for our sudo context, but
         // it is unclear what it really does and does not do, other than the vague

src/su/mod.rs

@@ -33,10 +33,7 @@ fn authenticate(
         "su"
     };
     let use_stdin = true;
-    let mut pam = PamContext::builder_cli("su", use_stdin, false, false)
-        .target_user(user)
-        .service_name(context)
-        .build()?;
+    let mut pam = PamContext::new_cli("su", context, use_stdin, false, false, Some(user))?;
     pam.set_requesting_user(requesting_user)?;
 
     // attempt to set the TTY this session is communicating on
@@ -59,12 +56,12 @@ fn authenticate(
             Ok(_) => break,
 
             // maxtries was reached, pam does not allow any more tries
-            Err(PamError::Pam(PamErrorType::MaxTries, _)) => {
+            Err(PamError::Pam(PamErrorType::MaxTries)) => {
                 return Err(Error::MaxAuthAttempts(current_try));
             }
 
             // there was an authentication error, we can retry
-            Err(PamError::Pam(PamErrorType::AuthError, _)) => {
+            Err(PamError::Pam(PamErrorType::AuthError)) => {
                 max_tries -= 1;
                 if max_tries == 0 {
                     return Err(Error::MaxAuthAttempts(current_try));
@@ -112,9 +109,7 @@ fn run(options: SuRunOptions) -> Result<(), Error> {
         restore_signal_handlers,
     } = crate::exec::run_command(&context, environment)?;
 
-    // closing the pam session is best effort, if any error occurs we cannot
-    // do anything with it
-    let _ = pam.close_session();
+    pam.close_session();
 
     // Run any clean-up code before this line.
     restore_signal_handlers();