Merge pull request #843 from memorysafety/visudo-trunc

Fix visudo bugs; update the CI

src/defaults/settings_dsl.rs

@@ -1,5 +1,6 @@
 macro_rules! add_from {
     ($ctor:ident, $type:ty) => {
+        #[allow(non_local_definitions)]
         impl From<$type> for $crate::defaults::SudoDefault {
             fn from(value: $type) -> Self {
                 $crate::defaults::SudoDefault::$ctor(value.into())
@@ -8,6 +9,7 @@ macro_rules! add_from {
     };
 
     ($ctor:ident, $type:ty, negatable$(, $vetting_function:expr)?) => {
+        #[allow(non_local_definitions)]
         impl From<$type> for $crate::defaults::SudoDefault {
             fn from(value: $type) -> Self {
                 $crate::defaults::SudoDefault::$ctor(OptTuple {
@@ -17,6 +19,7 @@ macro_rules! add_from {
             }
         }
 
+        #[allow(non_local_definitions)]
         impl From<($type, $type)> for $crate::defaults::SudoDefault {
             fn from((value, neg): ($type, $type)) -> Self {
                 $crate::defaults::SudoDefault::$ctor(OptTuple {

src/sudo/cli/mod.rs

@@ -10,6 +10,8 @@ pub mod help;
 #[cfg(test)]
 mod tests;
 
+// remove dead_code when sudoedit has been implemented
+#[allow(dead_code)]
 pub enum SudoAction {
     Edit(SudoEditOptions),
     Help(SudoHelpOptions),
@@ -205,6 +207,7 @@ impl TryFrom<SudoOptions> for SudoValidateOptions {
 }
 
 // sudo -e [-ABkNnS] [-r role] [-t type] [-C num] [-D directory] [-g group] [-h host] [-p prompt] [-R directory] [-T timeout] [-u user] file ...
+#[allow(dead_code)]
 pub struct SudoEditOptions {
     // -k
     pub reset_timestamp: bool,

src/sudo/env/environment.rs

@@ -133,6 +133,9 @@ fn is_safe_tz(value: &[u8]) -> bool {
     };
 
     if check_value.starts_with(&[b'/']) {
+        // clippy 1.79 wants to us to optimise this check away; but we don't know what this will always
+        // be possible; and the compiler is clever enough to do that for us anyway if it can be.
+        #[allow(clippy::const_is_empty)]
         if !PATH_ZONEINFO.is_empty() {
             if !check_value.starts_with(PATH_ZONEINFO.as_bytes())
                 || check_value.get(PATH_ZONEINFO.len()) != Some(&b'/')

src/sudo/env/tests.rs

@@ -96,8 +96,6 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
     let current_group = Group {
         gid: 1000,
         name: "test".to_string(),
-        passwd: String::new(),
-        members: Vec::new(),
     };
 
     let root_user = User {
@@ -114,8 +112,6 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
     let root_group = Group {
         gid: 0,
         name: "root".to_string(),
-        passwd: String::new(),
-        members: Vec::new(),
     };
 
     Context {

src/sudo/mod.rs

@@ -16,7 +16,7 @@ use pipeline::{Pipeline, PolicyPlugin};
 use std::path::Path;
 
 mod cli;
-mod diagnostic;
+pub mod diagnostic;
 mod env;
 mod pam;
 mod pipeline;

src/sudoers/mod.rs

@@ -590,160 +590,159 @@ fn analyze(
         }
     }
 
-    impl Sudoers {
-        fn include(
-            &mut self,
-            parent: &Path,
-            path: &Path,
-            diagnostics: &mut Vec<Error>,
-            count: &mut u8,
-        ) {
-            if *count >= INCLUDE_LIMIT {
-                diagnostics.push(Error {
-                    source: Some(parent.to_owned()),
-                    location: None,
-                    message: format!("include file limit reached opening '{}'", path.display()),
-                })
-            // FIXME: this will cause an error in `visudo` if we open a non-privileged sudoers file
-            // that includes another non-privileged sudoer files.
-            } else {
-                match open_subsudoers(path) {
-                    Ok(subsudoer) => {
-                        *count += 1;
-                        self.process(path, subsudoer, diagnostics, count)
-                    }
-                    Err(e) => {
-                        let message = if e.kind() == io::ErrorKind::NotFound {
-                            // improve the error message in this case
-                            format!("cannot open sudoers file '{}'", path.display())
-                        } else {
-                            e.to_string()
-                        };
+    fn include(
+        cfg: &mut Sudoers,
+        parent: &Path,
+        path: &Path,
+        diagnostics: &mut Vec<Error>,
+        count: &mut u8,
+    ) {
+        if *count >= INCLUDE_LIMIT {
+            diagnostics.push(Error {
+                source: Some(parent.to_owned()),
+                location: None,
+                message: format!("include file limit reached opening '{}'", path.display()),
+            })
+        // FIXME: this will cause an error in `visudo` if we open a non-privileged sudoers file
+        // that includes another non-privileged sudoer files.
+        } else {
+            match open_subsudoers(path) {
+                Ok(subsudoer) => {
+                    *count += 1;
+                    process(cfg, path, subsudoer, diagnostics, count)
+                }
+                Err(e) => {
+                    let message = if e.kind() == io::ErrorKind::NotFound {
+                        // improve the error message in this case
+                        format!("cannot open sudoers file '{}'", path.display())
+                    } else {
+                        e.to_string()
+                    };
 
-                        diagnostics.push(Error {
-                            source: Some(parent.to_owned()),
-                            location: None,
-                            message,
-                        })
-                    }
+                    diagnostics.push(Error {
+                        source: Some(parent.to_owned()),
+                        location: None,
+                        message,
+                    })
                 }
             }
         }
+    }
 
-        fn process(
-            &mut self,
-            cur_path: &Path,
-            sudoers: impl IntoIterator<Item = basic_parser::Parsed<Sudo>>,
-            diagnostics: &mut Vec<Error>,
-            safety_count: &mut u8,
-        ) {
-            for item in sudoers {
-                match item {
-                    Ok(line) => match line {
-                        Sudo::LineComment => {}
-
-                        Sudo::Spec(permission) => self.rules.push(permission),
-
-                        Sudo::Decl(UserAlias(mut def)) => self.aliases.user.1.append(&mut def),
-                        Sudo::Decl(HostAlias(mut def)) => self.aliases.host.1.append(&mut def),
-                        Sudo::Decl(CmndAlias(mut def)) => self.aliases.cmnd.1.append(&mut def),
-                        Sudo::Decl(RunasAlias(mut def)) => self.aliases.runas.1.append(&mut def),
-
-                        Sudo::Decl(Defaults(params)) => {
-                            for (name, value) in params {
-                                self.set_default(name, value)
-                            }
+    fn process(
+        cfg: &mut Sudoers,
+        cur_path: &Path,
+        sudoers: impl IntoIterator<Item = basic_parser::Parsed<Sudo>>,
+        diagnostics: &mut Vec<Error>,
+        safety_count: &mut u8,
+    ) {
+        for item in sudoers {
+            match item {
+                Ok(line) => match line {
+                    Sudo::LineComment => {}
+
+                    Sudo::Spec(permission) => cfg.rules.push(permission),
+
+                    Sudo::Decl(UserAlias(mut def)) => cfg.aliases.user.1.append(&mut def),
+                    Sudo::Decl(HostAlias(mut def)) => cfg.aliases.host.1.append(&mut def),
+                    Sudo::Decl(CmndAlias(mut def)) => cfg.aliases.cmnd.1.append(&mut def),
+                    Sudo::Decl(RunasAlias(mut def)) => cfg.aliases.runas.1.append(&mut def),
+
+                    Sudo::Decl(Defaults(params)) => {
+                        for (name, value) in params {
+                            set_default(cfg, name, value)
                         }
+                    }
 
-                        Sudo::Include(path) => self.include(
-                            cur_path,
-                            &resolve_relative(cur_path, path),
-                            diagnostics,
-                            safety_count,
-                        ),
-
-                        Sudo::IncludeDir(path) => {
-                            if path.contains("%h") {
-                                diagnostics.push(Error{
+                    Sudo::Include(path) => include(
+                        cfg,
+                        cur_path,
+                        &resolve_relative(cur_path, path),
+                        diagnostics,
+                        safety_count,
+                    ),
+
+                    Sudo::IncludeDir(path) => {
+                        if path.contains("%h") {
+                            diagnostics.push(Error{
                                     source: Some(cur_path.to_owned()),
                                     location: None,
                                     message: format!("cannot open sudoers file {path}: percent escape %h in includedir is unsupported")});
-                                continue;
-                            }
-
-                            let path = resolve_relative(cur_path, path);
-                            let Ok(files) = std::fs::read_dir(&path) else {
-                                diagnostics.push(Error {
-                                    source: Some(cur_path.to_owned()),
-                                    location: None,
-                                    message: format!("cannot open sudoers file {}", path.display()),
-                                });
-                                continue;
-                            };
-                            let mut safe_files = files
-                                .filter_map(|direntry| {
-                                    let path = direntry.ok()?.path();
-                                    let text = path.file_name()?.to_str()?;
-                                    if text.ends_with('~') || text.contains('.') {
-                                        None
-                                    } else {
-                                        Some(path)
-                                    }
-                                })
-                                .collect::<Vec<_>>();
-                            safe_files.sort();
-                            for file in safe_files {
-                                self.include(cur_path, file.as_ref(), diagnostics, safety_count)
-                            }
+                            continue;
                         }
-                    },
 
-                    Err(basic_parser::Status::Fatal(pos, message)) => diagnostics.push(Error {
-                        source: Some(cur_path.to_owned()),
-                        location: Some(pos),
-                        message,
-                    }),
-                    Err(_) => panic!("internal parser error"),
-                }
+                        let path = resolve_relative(cur_path, path);
+                        let Ok(files) = std::fs::read_dir(&path) else {
+                            diagnostics.push(Error {
+                                source: Some(cur_path.to_owned()),
+                                location: None,
+                                message: format!("cannot open sudoers file {}", path.display()),
+                            });
+                            continue;
+                        };
+                        let mut safe_files = files
+                            .filter_map(|direntry| {
+                                let path = direntry.ok()?.path();
+                                let text = path.file_name()?.to_str()?;
+                                if text.ends_with('~') || text.contains('.') {
+                                    None
+                                } else {
+                                    Some(path)
+                                }
+                            })
+                            .collect::<Vec<_>>();
+                        safe_files.sort();
+                        for file in safe_files {
+                            include(cfg, cur_path, file.as_ref(), diagnostics, safety_count)
+                        }
+                    }
+                },
+
+                Err(basic_parser::Status::Fatal(pos, message)) => diagnostics.push(Error {
+                    source: Some(cur_path.to_owned()),
+                    location: Some(pos),
+                    message,
+                }),
+                Err(_) => panic!("internal parser error"),
             }
         }
+    }
 
-        fn set_default(&mut self, name: String, value: ConfigValue) {
-            match value {
-                Flag(value) => {
-                    if value {
-                        self.settings.flags.insert(name);
-                    } else {
-                        self.settings.flags.remove(&name);
-                    }
+    fn set_default(cfg: &mut Sudoers, name: String, value: ConfigValue) {
+        match value {
+            Flag(value) => {
+                if value {
+                    cfg.settings.flags.insert(name);
+                } else {
+                    cfg.settings.flags.remove(&name);
                 }
-                List(mode, values) => {
-                    let slot: &mut _ = self.settings.list.entry(name).or_default();
-                    match mode {
-                        Mode::Set => *slot = values.into_iter().collect(),
-                        Mode::Add => slot.extend(values),
-                        Mode::Del => {
-                            for key in values {
-                                slot.remove(&key);
-                            }
+            }
+            List(mode, values) => {
+                let slot: &mut _ = cfg.settings.list.entry(name).or_default();
+                match mode {
+                    Mode::Set => *slot = values.into_iter().collect(),
+                    Mode::Add => slot.extend(values),
+                    Mode::Del => {
+                        for key in values {
+                            slot.remove(&key);
                         }
                     }
                 }
-                Text(value) => {
-                    self.settings.str_value.insert(name, value);
-                }
-                Enum(value) => {
-                    self.settings.enum_value.insert(name, value);
-                }
-                Num(value) => {
-                    self.settings.int_value.insert(name, value);
-                }
+            }
+            Text(value) => {
+                cfg.settings.str_value.insert(name, value);
+            }
+            Enum(value) => {
+                cfg.settings.enum_value.insert(name, value);
+            }
+            Num(value) => {
+                cfg.settings.int_value.insert(name, value);
             }
         }
     }
 
     let mut diagnostics = vec![];
-    result.process(path, sudoers, &mut diagnostics, &mut 0);
+    process(&mut result, path, sudoers, &mut diagnostics, &mut 0);
 
     let alias = &mut result.aliases;
     alias.user.0 = sanitize_alias_table(&alias.user.1, &mut diagnostics);