Repository: memorysafety/sudo-rs
Pull request #34 (2023-02-07 12:35:25)

Support for user/group info as #ID and name
Closing #16 

Just the parsing bit isn't that much work; code has been worked in such a way that the parser/checking routines is not doing any syscalls itself to resolve ID's to names and vice versa.

squell: (This PR should be squashed because some intermediate commits break the build of the whole workspace.)

Patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index 4b44c060..a944924b 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -81,6 +81,8 @@ jobs:
 
   format:
     runs-on: ubuntu-latest
+    env:
+      RUSTDOCFLAGS: '-D warnings'
     steps:
       - uses: actions/checkout@v2
 
@@ -106,6 +108,8 @@ jobs:
   clippy:
     needs: format
     runs-on: ubuntu-latest
+    env:
+      RUSTDOCFLAGS: '-D warnings'
     steps:
       - uses: actions/checkout@v2
 
@@ -132,6 +136,7 @@ jobs:
         uses: actions-rs/clippy-check@v1
         with:
           token: ${{ secrets.GITHUB_TOKEN }}
+          args: --no-deps
 
   docs:
     needs: clippy
diff --git a/lib/sudo-cli/src/lib.rs b/lib/sudo-cli/src/lib.rs
index 2a36401a..1de0c54d 100644
--- a/lib/sudo-cli/src/lib.rs
+++ b/lib/sudo-cli/src/lib.rs
@@ -266,7 +266,7 @@ impl TryFrom<Cli> for SudoOptions {
         let host = if command.host.is_some() {
             return Err(Error::raw(
                 clap::error::ErrorKind::ArgumentConflict,
-                "Cannot use `-h=<HOST>` and `--help=<HOST>` at the same time",
+                "Cannot use `-h=<HOST>` and `--host=<HOST>` at the same time",
             ));
         } else {
             command.host_or_help
@@ -321,36 +321,111 @@ impl SudoOptions {
         I: IntoIterator<Item = T>,
         T: Into<String> + Clone,
     {
-        let mut args = iter.into_iter();
+        // We need all this extra logic because `clap` cannot handle environment variable
+        // declarations. This means that we must filter any args with the shape `NAME=VALUE` before
+        // passing the arguments to `clap`.
+        //
+        // However, this is not straightforward because such args could be part of the command that
+        // `sudo` is supposed to execute. For example, in `sudo FOO=1 -b cmd BAR=2`, `FOO=1` should
+        // be filtered but `BAR=2` should not because it is part of the `cmd BAR=2` command.
 
+        // Keep the original arguments into `vec_args` in case we need them later.
+        let mut vec_args = iter.into_iter().map(Into::into).collect::<Vec<String>>();
+        let mut args = vec_args.iter();
+
+        // Store which args are environment variable declarations.
+        let mut vec_env_vars = vec![false; vec_args.len()];
+        let mut env_vars = vec_env_vars.iter_mut();
+
+        // Store the arguments that were environment variable declarations.
         let mut env_var_list = Vec::new();
+        // Store the arguments that were not environment variable declarations.
         let mut remaining_args = Vec::new();
+        // Whether the args had a `"--"` to separate the external args from the regular args.
+        let mut had_separator = false;
 
-        while let Some(arg) = args.next() {
-            let arg = arg.into();
+        while let (Some(arg), Some(is_env_var)) = (args.next(), env_vars.next()) {
             // If we found `--` we know that the remaining arguments are not env variable
             // definitions.
             if arg == "--" {
+                had_separator = true;
+                // Push the separator.
                 remaining_args.push(arg);
-                remaining_args.extend(args.map(Into::into));
+                // None of the remaining arguments were environment variable declarations.
+                remaining_args.extend(args);
                 break;
             }
 
-            if let Some((name, value)) = arg.split_once('=').and_then(|(name, value)| {
-                name.chars()
-                    .all(|c| c.is_alphanumeric() || c == '_')
-                    .then_some((name, value))
-            }) {
+            if let Some((name, value)) = try_to_env_var(arg) {
+                // If this arg is an environment variable we store it as so.
+                *is_env_var = true;
                 env_var_list.push((name.to_owned(), value.to_owned()));
             } else {
+                // If this arg is not environment variable we store it as so.
                 remaining_args.push(arg);
             }
         }
 
+        // Now that the remaining args are not environment variable declarations we can let `clap`
+        // do its magic.
         let mut opts: SudoOptions = Cli::try_parse_from(remaining_args)?.try_into()?;
+        // Populate the environment variable declarations.
         opts.env_var_list = env_var_list;
 
-        Ok(opts)
+        // If there was a separator or if there is no command to run, there is nothing else to do.
+        if had_separator || opts.external_args.is_empty() {
+            return Ok(opts);
+        }
+
+        // Otherwise, it could be that some of the environment variable declarations were actually
+        // part of the command to be executed by `sudo`. For example, in `sudo env FOO=1 ls`,
+        // `FO0=1` is not a sudo argument but part of the command to be executed.
+
+        // We will traverse the external args and the original args in reverse amd try to figure
+        // out which args were not actually environment variable declarations but external args
+        // instead.
+        let mut external_args = opts.external_args.iter().rev();
+        // Then we will add a separator manually so we can reprocess everything by calling this
+        // function recursively. To do this, we need to figure out where this separator actually
+        // goes.
+        let mut index = None;
+
+        for (i, last_arg) in vec_args.iter().enumerate().rev() {
+            // If the arg was an environment variable declaration, then we must put the separator
+            // before this arg.
+            if vec_env_vars[i] {
+                continue;
+            }
+            // If the last external arg is the same as this arg, we know that the separator must go
+            // before this arg.
+            if let Some(last_external) = external_args.next() {
+                if last_arg == last_external {
+                    continue;
+                }
+            }
+            // Otherwise, this arg is already a non-external arg.
+            index = Some(i + 1);
+            break;
+        }
+
+        if let Some(mut index) = index {
+            // There is one last catch. In this command `sudo FOO=1 env BAR=2 ls`, `FOO=1` is not
+            // part of the external command. So we need to ignore the first environment variable
+            // declarations before putting the separator.
+            while vec_env_vars[index] {
+                index += 1;
+            }
+            // Then we insert the separator and parse the arguments again. This will not recurse
+            // indefinitely because we don't do any of this extra logic if there was a separator.
+            vec_args.insert(index, "--".to_owned());
+            Self::try_parse_from(vec_args)
+        } else {
+            // If `index` were `None`, it would mean that all the args were environment variable
+            // declarations or part of the external args. But this cannot happen because the first
+            // argument should be the name of the binary being run and that is neither an
+            // environment variable declaration nor a external argument
+            unreachable!()
+        }
     }
 
     pub fn parse() -> Self {
@@ -364,6 +439,18 @@ impl SudoOptions {
     }
 }
 
+fn try_to_env_var(arg: &str) -> Option<(String, String)> {
+    if let Some((name, value)) = arg.split_once('=').and_then(|(name, value)| {
+        name.chars()
+            .all(|c| c.is_alphanumeric() || c == '_')
+            .then_some((name, value))
+    }) {
+        Some((name.to_owned(), value.to_owned()))
+    } else {
+        None
+    }
+}
+
 #[cfg(test)]
 mod tests {
     #[test]
diff --git a/lib/sudo-system/src/lib.rs b/lib/sudo-system/src/lib.rs
index f98d23e7..869322f1 100644
--- a/lib/sudo-system/src/lib.rs
+++ b/lib/sudo-system/src/lib.rs
@@ -66,7 +66,7 @@ pub fn hostname() -> String {
     }
 }
 
-#[derive(Debug)]
+#[derive(Debug, PartialEq)]
 pub struct User {
     pub uid: libc::uid_t,
     pub gid: libc::gid_t,
diff --git a/lib/sudoers/Cargo.toml b/lib/sudoers/Cargo.toml
index 59393aa6..de39acbb 100644
--- a/lib/sudoers/Cargo.toml
+++ b/lib/sudoers/Cargo.toml
@@ -5,5 +5,9 @@ edition = "2021"
 license = "Apache-2.0 OR MIT"
 
 [dependencies]
-derive_more = ">=0.99.17"
-glob = ">= 0.3.1"
+derive_more = "0.99.17"
+libc = "0.2.139"
+glob = "0.3.1"
+sudo-system = {path="../sudo-system"}
+
+
diff --git a/lib/sudoers/src/ast.rs b/lib/sudoers/src/ast.rs
index 921a1cf1..f494bd5a 100644
--- a/lib/sudoers/src/ast.rs
+++ b/lib/sudoers/src/ast.rs
@@ -14,11 +14,22 @@ pub enum Qualified<T> {
 pub type Spec<T> = Qualified<Meta<T>>;
 pub type SpecList<T> = Vec<Spec<T>>;
 
+/// A userspecifier is either a username, or a (non-unix) group name, or netgroup
+#[derive(Debug)]
+#[cfg_attr(test, derive(Clone, PartialEq, Eq))]
+pub enum UserSpecifier {
+    User(Identifier),
+    Group(Identifier),
+    NonunixGroup(Identifier),
+}
+
+pub use crate::tokens::Identifier;
+
 /// The RunAs specification consists of a (possibly empty) list of userspecifiers, followed by a (possibly empty) list of groups.
 #[derive(Debug, Default)]
 pub struct RunAs {
     pub users: SpecList<UserSpecifier>,
-    pub groups: SpecList<Username>,
+    pub groups: SpecList<Identifier>,
 }
 
 /// Commands in /etc/sudoers can have attributes attached to them, such as NOPASSWD, NOEXEC, ...
@@ -53,6 +64,7 @@ pub enum Directive {
 }
 
 /// The Sudoers file can contain permissions and directives
+// TODO: Defaults
 pub enum Sudo {
     Spec(PermissionSpec),
     Decl(Directive),
@@ -91,6 +103,41 @@ impl<T: Many> Many for Qualified<T> {
     const LIMIT: usize = T::LIMIT;
 }
 
+/// Since UserSpecifier is not a token, implement the parser for Meta<UserSpecifier>
+impl Parse for Meta<UserSpecifier> {
+    fn parse(stream: &mut Peekable<impl Iterator<Item = char>>) -> Parsed<Self> {
+        use UserSpecifier::*;
+        if let Some(meta) = maybe(try_nonterminal::<Meta<Username>>(stream))? {
+            make(match meta {
+                Meta::All => Meta::All,
+                Meta::Alias(alias) => Meta::Alias(alias),
+                Meta::Only(Username(name)) => Meta::Only(User(Identifier::Name(name))),
+            })
+        } else {
+            let userspec = if maybe(accept_if(|c| c == '%', stream))?.is_some() {
+                let ctor = if maybe(accept_if(|c| c == ':', stream))?.is_some() {
+                    UserSpecifier::NonunixGroup
+                } else {
+                    UserSpecifier::Group
+                };
+                // in this case we must fail 'hard', since input has been consumed
+                ctor(expect_nonterminal(stream)?)
+            } else if maybe(accept_if(|c| c == '+', stream))?.is_some() {
+                // TODO Netgroups; in this case we need to "return early" since
+                // netgroups don't share the syntactic structure of the other alternatives
+                unrecoverable!("netgroups are not supported yet");
+            } else {
+                // in this case we must fail 'softly', since no input has been consumed yet
+                UserSpecifier::User(try_nonterminal(stream)?)
+            };
+
+            make(Meta::Only(userspec))
+        }
+    }
+}
+
+impl Many for UserSpecifier {}
+
 /// grammar:
 /// ```text
 /// runas = "(", userlist, (":", grouplist?)?, ")"
@@ -245,12 +292,15 @@ fn get_directive(
     use crate::ast::Meta::*;
     use crate::ast::Qualified::*;
     use crate::ast::UserSpecifier::*;
-    let Allow(Only(User(keyword))) = perhaps_keyword else { return reject() };
+    let Allow(Only(User(Identifier::Name(keyword)))) = perhaps_keyword else { return reject() };
 
-    fn parse_alias<T: Token + Many>(
+    fn parse_alias<T>(
         ctor: fn(Def<T>) -> Directive,
         stream: &mut Peekable<impl Iterator<Item = char>>,
-    ) -> Parsed<Directive> {
+    ) -> Parsed<Directive>
+    where
+        Meta<T>: Parse + Many,
+    {
         let Upper(name) = expect_nonterminal(stream)?;
         expect_syntax('=', stream)?;
 
diff --git a/lib/sudoers/src/lib.rs b/lib/sudoers/src/lib.rs
index dcc1cdee..ed0bc7bc 100644
--- a/lib/sudoers/src/lib.rs
+++ b/lib/sudoers/src/lib.rs
@@ -16,16 +16,12 @@ pub use ast::Sudo;
 pub use ast::Tag;
 pub use basic_parser::parse_string;
 
-/// TODO: this interface should be replaced by something that interacts with the operating system
-/// Right now, we emulate that a user is always only in its own group.
+mod sysuser;
+pub use sysuser::*;
 
-fn in_group(user: &str, group: &str) -> bool {
-    user == group
-}
-
-pub struct UserInfo<'a> {
-    pub user: &'a str,
-    pub group: &'a str,
+pub struct Request<'a, User: Identifiable, Group: UnixGroup> {
+    pub user: &'a User,
+    pub group: &'a Group,
 }
 
 // TODO: combine this with Vec<PermissionSpec> into a single data structure?
@@ -52,11 +48,11 @@ 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<'a>(
+pub fn check_permission<'a, User: Identifiable + PartialEq<User>, Group: UnixGroup>(
     sudoers: impl IntoIterator<Item = &'a PermissionSpec>,
     alias_table: &AliasTable,
-    am_user: &str,
-    request: &UserInfo,
+    am_user: &User,
+    request: Request<User, Group>,
     on_host: &str,
     cmdline: &str,
 ) -> Option<Vec<Tag>> {
@@ -82,9 +78,9 @@ pub fn check_permission<'a>(
                             *find_item(users, &match_user(request.user), &runas_user_aliases)?
                         }
                         if !in_group(request.user, request.group) {
-                            *find_item(groups, &match_token(request.group), &runas_group_aliases)?
+                            *find_item(groups, &match_group(request.group), &runas_group_aliases)?
                         }
-                    } else if request.user != "root" || !in_group("root", request.group) {
+                    } else if !(request.user.is_root() && in_group(request.user, request.group)) {
                         None?;
                     }
 
@@ -127,20 +123,43 @@ where
     result
 }
 
-fn match_user(username: &str) -> (impl Fn(&UserSpecifier) -> bool + '_) {
+fn match_user(user: &impl Identifiable) -> impl Fn(&UserSpecifier) -> bool + '_ {
     move |spec| match spec {
-        UserSpecifier::User(name) => name.0 == username,
-        UserSpecifier::Group(groupname) => in_group(username, groupname.0.as_str()),
+        UserSpecifier::User(id) => match_identifier(user, id),
+        UserSpecifier::Group(Identifier::Name(name)) => user.in_group_by_name(name),
+        UserSpecifier::Group(Identifier::ID(num)) => user.in_group_by_gid(*num),
+        _ => todo!(), // nonunix-groups, netgroups, etc.
+    }
+}
+
+//TODO: in real life, just checking the gid should suffice; for testability, we check the name first; THIS MUST BE REMOVED
+fn in_group(user: &impl Identifiable, group: &impl UnixGroup) -> bool {
+    if cfg!(test) {
+        group
+            .try_as_name()
+            .as_ref()
+            .map_or(user.in_group_by_gid(group.as_gid()), |name| {
+                user.in_group_by_name(name)
+            })
+    } else {
+        user.in_group_by_gid(group.as_gid())
+    }
+}
+
+fn match_group(group: &impl UnixGroup) -> impl Fn(&Identifier) -> bool + '_ {
+    move |id| match id {
+        Identifier::ID(num) => group.as_gid() == *num,
+        Identifier::Name(name) => group.try_as_name().map_or(false, |s| s == name),
     }
 }
 
-fn match_group_alias(groupname: &str) -> (impl Fn(&UserSpecifier) -> bool + '_) {
+fn match_group_alias(group: &impl UnixGroup) -> impl Fn(&UserSpecifier) -> bool + '_ {
     move |spec| match spec {
-        UserSpecifier::User(name) => name.0 == groupname,
-        /* the parser rejects this, but can happen due to Runas_Alias,
+        UserSpecifier::User(ident) => match_group(group)(ident),
+        /* the parser does not allow this, but can happen due to Runas_Alias,
          * see https://github.com/memorysafety/sudo-rs/issues/13 */
-        UserSpecifier::Group(_) => {
-            eprintln!("warning: ignoring %group syntax for use sudo -g");
+        _ => {
+            eprintln!("warning: ignoring %group syntax in runas_alias for checking sudo -g");
             false
         }
     }
@@ -175,6 +194,15 @@ where
     set
 }
 
+/// Code to map an ast::Identifier to the Identifiable trait
+
+fn match_identifier(user: &impl Identifiable, ident: &ast::Identifier) -> bool {
+    match ident {
+        Identifier::Name(name) => user.has_name(name),
+        Identifier::ID(num) => user.has_uid(*num),
+    }
+}
+
 /// Process a sudoers-parsing file into a workable AST
 
 pub fn analyze(sudoers: impl IntoIterator<Item = Sudo>) -> (Vec<PermissionSpec>, AliasTable) {
@@ -272,6 +300,15 @@ mod test {
         }
     }
 
+    impl UnixGroup for (u16, &str) {
+        fn try_as_name(&self) -> Option<&str> {
+            Some(self.1)
+        }
+        fn as_gid(&self) -> libc::gid_t {
+            self.0 as libc::gid_t
+        }
+    }
+
     #[test]
     #[should_panic]
     fn invalid_spec() {
@@ -296,86 +333,86 @@ mod test {
 
     #[test]
     fn permission_test() {
-        let root = UserInfo {
-            user: "root",
-            group: "root",
+        let root = || Request::<&str, _> {
+            user: &"root",
+            group: &(0, "root"),
         };
 
         macro_rules! FAIL {
             ([$($sudo:expr),*], $user:expr => $req:expr, $server:expr; $command:expr) => {
                 let (input,alias) = analyze(sudoer![$($sudo),*]);
-                assert_eq!(check_permission(&input, &alias, $user, $req, $server, $command), None);
+                assert_eq!(check_permission(&input, &alias, &$user, $req, $server, $command), None);
             }
         }
 
         macro_rules! pass {
             ([$($sudo:expr),*], $user:expr => $req:expr, $server:expr; $command:expr $(=> [$($list:expr),*])?) => {
                 let (input,alias) = analyze(sudoer![$($sudo),*]);
-                let result = check_permission(&input, &alias, $user, $req, $server, $command);
+                let result = check_permission(&input, &alias, &$user, $req, $server, $command);
                 $(assert_eq!(result, Some(vec![$($list),*]));)?
                 assert!(!result.is_none());
             }
         }
         use crate::ast::Tag::*;
 
-        FAIL!(["user ALL=(ALL:ALL) ALL"], "nobody"    => &root, "server"; "/bin/hello");
-        pass!(["user ALL=(ALL:ALL) ALL"], "user"      => &root, "server"; "/bin/hello");
-        pass!(["user ALL=(ALL:ALL) /bin/foo"], "user" => &root, "server"; "/bin/foo");
-        FAIL!(["user ALL=(ALL:ALL) /bin/foo"], "user" => &root, "server"; "/bin/hello");
-        pass!(["user ALL=(ALL:ALL) /bin/foo, NOPASSWD: /bin/bar"], "user" => &root, "server"; "/bin/foo");
-        pass!(["user ALL=(ALL:ALL) /bin/foo, NOPASSWD: /bin/bar"], "user" => &root, "server"; "/bin/bar" => [NoPasswd]);
+        FAIL!(["user ALL=(ALL:ALL) ALL"], "nobody"    => root(), "server"; "/bin/hello");
+        pass!(["user ALL=(ALL:ALL) ALL"], "user"      => root(), "server"; "/bin/hello");
+        pass!(["user ALL=(ALL:ALL) /bin/foo"], "user" => root(), "server"; "/bin/foo");
+        FAIL!(["user ALL=(ALL:ALL) /bin/foo"], "user" => root(), "server"; "/bin/hello");
+        pass!(["user ALL=(ALL:ALL) /bin/foo, NOPASSWD: /bin/bar"], "user" => root(), "server"; "/bin/foo");
+        pass!(["user ALL=(ALL:ALL) /bin/foo, NOPASSWD: /bin/bar"], "user" => root(), "server"; "/bin/bar" => [NoPasswd]);
 
-        pass!(["user server=(ALL:ALL) ALL"], "user" => &root, "server"; "/bin/hello");
-        FAIL!(["user laptop=(ALL:ALL) ALL"], "user" => &root, "server"; "/bin/hello");
+        pass!(["user server=(ALL:ALL) ALL"], "user" => root(), "server"; "/bin/hello");
+        FAIL!(["user laptop=(ALL:ALL) ALL"], "user" => root(), "server"; "/bin/hello");
 
-        pass!(["user ALL=!/bin/hello", "user ALL=/bin/hello"], "user" => &root, "server"; "/bin/hello");
-        FAIL!(["user ALL=/bin/hello", "user ALL=!/bin/hello"], "user" => &root, "server"; "/bin/hello");
+        pass!(["user ALL=!/bin/hello", "user ALL=/bin/hello"], "user" => root(), "server"; "/bin/hello");
+        FAIL!(["user ALL=/bin/hello", "user ALL=!/bin/hello"], "user" => root(), "server"; "/bin/hello");
 
         for alias in [
             "User_Alias GROUP=user1, user2",
             "User_Alias GROUP=ALL,!user3",
         ] {
-            pass!([alias,"GROUP ALL=/bin/hello"], "user1" => &root, "server"; "/bin/hello");
-            pass!([alias,"GROUP ALL=/bin/hello"], "user2" => &root, "server"; "/bin/hello");
-            FAIL!([alias,"GROUP ALL=/bin/hello"], "user3" => &root, "server"; "/bin/hello");
+            pass!([alias,"GROUP ALL=/bin/hello"], "user1" => root(), "server"; "/bin/hello");
+            pass!([alias,"GROUP ALL=/bin/hello"], "user2" => root(), "server"; "/bin/hello");
+            FAIL!([alias,"GROUP ALL=/bin/hello"], "user3" => root(), "server"; "/bin/hello");
         }
-        pass!(["user ALL=/bin/hello arg"], "user" => &root, "server"; "/bin/hello arg");
-        pass!(["user ALL=/bin/hello  arg"], "user" => &root, "server"; "/bin/hello arg");
-        pass!(["user ALL=/bin/hello arg"], "user" => &root, "server"; "/bin/hello  arg");
-        FAIL!(["user ALL=/bin/hello arg"], "user" => &root, "server"; "/bin/hello boo");
-        pass!(["user ALL=/bin/hello a*g"], "user" => &root, "server"; "/bin/hello  aaaarg");
-        FAIL!(["user ALL=/bin/hello a*g"], "user" => &root, "server"; "/bin/hello boo");
-        pass!(["user ALL=/bin/hello"], "user" => &root, "server"; "/bin/hello boo");
-        FAIL!(["user ALL=/bin/hello \"\""], "user" => &root, "server"; "/bin/hello boo");
-        pass!(["user ALL=/bin/hello \"\""], "user" => &root, "server"; "/bin/hello");
-        pass!(["user ALL=/bin/hel*"], "user" => &root, "server"; "/bin/hello");
-        pass!(["user ALL=/bin/hel*"], "user" => &root, "server"; "/bin/help");
-        pass!(["user ALL=/bin/hel*"], "user" => &root, "server"; "/bin/help me");
-        pass!(["user ALL=/bin/hel* *"], "user" => &root, "server"; "/bin/help");
-        FAIL!(["user ALL=/bin/hel* me"], "user" => &root, "server"; "/bin/help");
-        pass!(["user ALL=/bin/hel* me"], "user" => &root, "server"; "/bin/help me");
-        FAIL!(["user ALL=/bin/hel* me"], "user" => &root, "server"; "/bin/help me please");
-
-        pass!(["User_Alias FULLTIME=ALL,!marc","FULLTIME ALL=ALL"], "user" => &root, "server"; "/bin/bash");
-        FAIL!(["User_Alias FULLTIME=ALL,!marc","FULLTIME ALL=ALL"], "marc" => &root, "server"; "/bin/bash");
-        FAIL!(["User_Alias FULLTIME=ALL,!marc","ALL,!FULLTIME ALL=ALL"], "user" => &root, "server"; "/bin/bash");
-        pass!(["User_Alias FULLTIME=ALL,!marc","ALL,!FULLTIME ALL=ALL"], "marc" => &root, "server"; "/bin/bash");
-        pass!(["Host_Alias MACHINE=laptop,server","user MACHINE=ALL"], "user" => &root, "server"; "/bin/bash");
-        pass!(["Host_Alias MACHINE=laptop,server","user MACHINE=ALL"], "user" => &root, "laptop"; "/bin/bash");
-        FAIL!(["Host_Alias MACHINE=laptop,server","user MACHINE=ALL"], "user" => &root, "desktop"; "/bin/bash");
-        pass!(["Cmnd_Alias WHAT=/bin/dd, /bin/rm","user ALL=WHAT"], "user" => &root, "server"; "/bin/rm");
-        pass!(["Cmd_Alias WHAT=/bin/dd,/bin/rm","user ALL=WHAT"], "user" => &root, "laptop"; "/bin/dd");
-        FAIL!(["Cmnd_Alias WHAT=/bin/dd,/bin/rm","user ALL=WHAT"], "user" => &root, "desktop"; "/bin/bash");
-
-        pass!(["User_Alias A=B","User_Alias B=user","A ALL=ALL"], "user" => &root, "vm"; "/bin/ls");
-        pass!(["Host_Alias A=B","Host_Alias B=vm","ALL A=ALL"], "user" => &root, "vm"; "/bin/ls");
-        pass!(["Cmnd_Alias A=B","Cmnd_Alias B=/bin/ls","ALL ALL=A"], "user" => &root, "vm"; "/bin/ls");
-
-        FAIL!(["Runas_Alias TIME=%wheel,sudo","user ALL=() ALL"], "user" => &UserInfo{ user: "sudo", group: "sudo" }, "vm"; "/bin/ls");
-        pass!(["Runas_Alias TIME=%wheel,sudo","user ALL=(TIME) ALL"], "user" => &UserInfo{ user: "sudo", group: "sudo" }, "vm"; "/bin/ls");
-        FAIL!(["Runas_Alias TIME=%wheel,sudo","user ALL=(:TIME) ALL"], "user" => &UserInfo{ user: "sudo", group: "sudo" }, "vm"; "/bin/ls");
-        pass!(["Runas_Alias TIME=%wheel,sudo","user ALL=(:TIME) ALL"], "user" => &UserInfo{ user: "user", group: "sudo" }, "vm"; "/bin/ls");
-        pass!(["Runas_Alias TIME=%wheel,sudo","user ALL=(TIME) ALL"], "user" => &UserInfo{ user: "wheel", group: "wheel" }, "vm"; "/bin/ls");
+        pass!(["user ALL=/bin/hello arg"], "user" => root(), "server"; "/bin/hello arg");
+        pass!(["user ALL=/bin/hello  arg"], "user" => root(), "server"; "/bin/hello arg");
+        pass!(["user ALL=/bin/hello arg"], "user" => root(), "server"; "/bin/hello  arg");
+        FAIL!(["user ALL=/bin/hello arg"], "user" => root(), "server"; "/bin/hello boo");
+        pass!(["user ALL=/bin/hello a*g"], "user" => root(), "server"; "/bin/hello  aaaarg");
+        FAIL!(["user ALL=/bin/hello a*g"], "user" => root(), "server"; "/bin/hello boo");
+        pass!(["user ALL=/bin/hello"], "user" => root(), "server"; "/bin/hello boo");
+        FAIL!(["user ALL=/bin/hello \"\""], "user" => root(), "server"; "/bin/hello boo");
+        pass!(["user ALL=/bin/hello \"\""], "user" => root(), "server"; "/bin/hello");
+        pass!(["user ALL=/bin/hel*"], "user" => root(), "server"; "/bin/hello");
+        pass!(["user ALL=/bin/hel*"], "user" => root(), "server"; "/bin/help");
+        pass!(["user ALL=/bin/hel*"], "user" => root(), "server"; "/bin/help me");
+        pass!(["user ALL=/bin/hel* *"], "user" => root(), "server"; "/bin/help");
+        FAIL!(["user ALL=/bin/hel* me"], "user" => root(), "server"; "/bin/help");
+        pass!(["user ALL=/bin/hel* me"], "user" => root(), "server"; "/bin/help me");
+        FAIL!(["user ALL=/bin/hel* me"], "user" => root(), "server"; "/bin/help me please");
+
+        pass!(["User_Alias FULLTIME=ALL,!marc","FULLTIME ALL=ALL"], "user" => root(), "server"; "/bin/bash");
+        FAIL!(["User_Alias FULLTIME=ALL,!marc","FULLTIME ALL=ALL"], "marc" => root(), "server"; "/bin/bash");
+        FAIL!(["User_Alias FULLTIME=ALL,!marc","ALL,!FULLTIME ALL=ALL"], "user" => root(), "server"; "/bin/bash");
+        pass!(["User_Alias FULLTIME=ALL,!marc","ALL,!FULLTIME ALL=ALL"], "marc" => root(), "server"; "/bin/bash");
+        pass!(["Host_Alias MACHINE=laptop,server","user MACHINE=ALL"], "user" => root(), "server"; "/bin/bash");
+        pass!(["Host_Alias MACHINE=laptop,server","user MACHINE=ALL"], "user" => root(), "laptop"; "/bin/bash");
+        FAIL!(["Host_Alias MACHINE=laptop,server","user MACHINE=ALL"], "user" => root(), "desktop"; "/bin/bash");
+        pass!(["Cmnd_Alias WHAT=/bin/dd, /bin/rm","user ALL=WHAT"], "user" => root(), "server"; "/bin/rm");
+        pass!(["Cmd_Alias WHAT=/bin/dd,/bin/rm","user ALL=WHAT"], "user" => root(), "laptop"; "/bin/dd");
+        FAIL!(["Cmnd_Alias WHAT=/bin/dd,/bin/rm","user ALL=WHAT"], "user" => root(), "desktop"; "/bin/bash");
+
+        pass!(["User_Alias A=B","User_Alias B=user","A ALL=ALL"], "user" => root(), "vm"; "/bin/ls");
+        pass!(["Host_Alias A=B","Host_Alias B=vm","ALL A=ALL"], "user" => root(), "vm"; "/bin/ls");
+        pass!(["Cmnd_Alias A=B","Cmnd_Alias B=/bin/ls","ALL ALL=A"], "user" => root(), "vm"; "/bin/ls");
+
+        FAIL!(["Runas_Alias TIME=%wheel,sudo","user ALL=() ALL"], "user" => Request{ user: &"sudo", group: &(42,"sudo") }, "vm"; "/bin/ls");
+        pass!(["Runas_Alias TIME=%wheel,sudo","user ALL=(TIME) ALL"], "user" => Request{ user: &"sudo", group: &(42,"sudo") }, "vm"; "/bin/ls");
+        FAIL!(["Runas_Alias TIME=%wheel,sudo","user ALL=(:TIME) ALL"], "user" => Request{ user: &"sudo", group: &(42,"sudo") }, "vm"; "/bin/ls");
+        pass!(["Runas_Alias TIME=%wheel,sudo","user ALL=(:TIME) ALL"], "user" => Request{ user: &"user", group: &(42,"sudo") }, "vm"; "/bin/ls");
+        pass!(["Runas_Alias TIME=%wheel,sudo","user ALL=(TIME) ALL"], "user" => Request{ user: &"wheel", group: &(37,"wheel") }, "vm"; "/bin/ls");
     }
 
     #[test]
diff --git a/lib/sudoers/src/sysuser.rs b/lib/sudoers/src/sysuser.rs
new file mode 100644
index 00000000..2a08f1ed
--- /dev/null
+++ b/lib/sudoers/src/sysuser.rs
@@ -0,0 +1,116 @@
+/// This trait/module is here to not make this crate independent (at the present time) in the idiosyncracies of user representation details
+/// (which we may decide over time), as well as to make explicit what functionality a user-representation must have; this
+/// interface is not set in stone and "easy" to change.
+pub trait Identifiable {
+    fn has_name(&self, _name: &str) -> bool {
+        false
+    }
+    fn has_uid(&self, _uid: libc::uid_t) -> bool {
+        false
+    }
+
+    fn is_root(&self) -> bool {
+        false
+    }
+    fn in_group_by_name(&self, _name: &str) -> bool {
+        false
+    }
+    fn in_group_by_gid(&self, _name: libc::gid_t) -> bool {
+        false
+    }
+}
+
+pub trait UnixGroup {
+    fn as_gid(&self) -> libc::gid_t;
+    fn try_as_name(&self) -> Option<&str>;
+}
+
+/// This is the "canonical" info that we need
+#[derive(Debug)]
+pub struct GroupID(pub libc::gid_t, pub Option<String>);
+#[derive(Debug)]
+pub struct UserRecord(pub libc::gid_t, pub Option<String>, pub Vec<GroupID>);
+
+impl PartialEq<UserRecord> for UserRecord {
+    fn eq(&self, other: &Self) -> bool {
+        self.0 == other.0 && self.1 == other.1
+    }
+}
+
+impl Eq for UserRecord {}
+
+impl UnixGroup for GroupID {
+    fn as_gid(&self) -> libc::gid_t {
+        self.0
+    }
+
+    fn try_as_name(&self) -> Option<&str> {
+        self.1.as_deref()
+    }
+}
+
+impl Identifiable for &str {
+    fn has_name(&self, name: &str) -> bool {
+        *self == name
+    }
+
+    fn in_group_by_name(&self, name: &str) -> bool {
+        self.has_name(name)
+    }
+
+    fn is_root(&self) -> bool {
+        self.has_name("root")
+    }
+}
+
+impl Identifiable for GroupID {
+    fn has_uid(&self, uid: libc::gid_t) -> bool {
+        self.0 == uid
+    }
+    fn has_name(&self, name: &str) -> bool {
+        self.1.as_ref().map_or(false, |s| s == name)
+    }
+}
+
+impl Identifiable for UserRecord {
+    fn is_root(&self) -> bool {
+        self.has_name("root") && self.has_uid(0)
+    }
+
+    fn in_group_by_name(&self, name: &str) -> bool {
+        self.2.iter().any(|g| g.has_name(name))
+    }
+
+    fn in_group_by_gid(&self, id: libc::gid_t) -> bool {
+        self.2.iter().any(|g| g.has_uid(id))
+    }
+}
+
+impl Identifiable for sudo_system::User {
+    fn has_name(&self, name: &str) -> bool {
+        self.name == name
+    }
+    fn has_uid(&self, uid: libc::gid_t) -> bool {
+        self.uid == uid
+    }
+
+    fn is_root(&self) -> bool {
+        self.has_uid(0)
+    }
+    fn in_group_by_name(&self, _name: &str) -> bool {
+        false
+    }
+    fn in_group_by_gid(&self, _name: libc::gid_t) -> bool {
+        false
+    }
+}
+
+impl UnixGroup for sudo_system::Group {
+    fn as_gid(&self) -> libc::gid_t {
+        self.gid
+    }
+
+    fn try_as_name(&self) -> Option<&str> {
+        Some(&self.name)
+    }
+}
diff --git a/lib/sudoers/src/tokens.rs b/lib/sudoers/src/tokens.rs
index 15bc522d..ff13eeb0 100644
--- a/lib/sudoers/src/tokens.rs
+++ b/lib/sudoers/src/tokens.rs
@@ -1,6 +1,6 @@
 //! Various tokens
 
-use crate::basic_parser::{Many, Parsed, Status, Token};
+use crate::basic_parser::{unrecoverable, Many, Parsed, Status, Token};
 use derive_more::Deref;
 
 #[derive(Debug, Deref)]
@@ -60,18 +60,21 @@ impl Many for Hostname {}
 /// A userspecifier is either a username, or a group name (TODO: user ID and group ID)
 #[derive(Debug)]
 #[cfg_attr(test, derive(Clone, PartialEq, Eq))]
-pub enum UserSpecifier {
-    User(Username),
-    Group(Username),
+pub enum Identifier {
+    Name(String),
+    ID(libc::gid_t),
 }
 
-impl Token for UserSpecifier {
+impl Token for Identifier {
     fn construct(text: String) -> Parsed<Self> {
         let mut chars = text.chars();
-        Ok(if let Some('%') = chars.next() {
-            UserSpecifier::Group(Username(chars.as_str().to_string()))
+        Ok(if let Some('#') = chars.next() {
+            match chars.as_str().parse() {
+                Ok(guid) => Identifier::ID(guid),
+                Err(err) => unrecoverable!("invalid user id: {err}"),
+            }
         } else {
-            UserSpecifier::User(Username(text))
+            Identifier::Name(text)
         })
     }
 
@@ -79,11 +82,11 @@ impl Token for UserSpecifier {
         Username::accept(c)
     }
     fn accept_1st(c: char) -> bool {
-        Self::accept(c) || c == '%'
+        Self::accept(c) || c == '#'
     }
 }
 
-impl Many for UserSpecifier {}
+impl Many for Identifier {}
 
 /// This enum allows items to use the ALL wildcard or be specified with aliases, or directly.
 /// (Maybe this is better defined not as a Token but simply directly as an implementation of [crate::basic_parser::Parse])
diff --git a/sudo/src/main.rs b/sudo/src/main.rs
index b0a0c23c..e1daf82c 100644
--- a/sudo/src/main.rs
+++ b/sudo/src/main.rs
@@ -75,10 +75,10 @@ fn check_sudoers(context: &Context, sudo_options: &SudoOptions) -> Result<Option
     Ok(sudoers::check_permission(
         &input,
         &aliases,
-        &context.current_user.name,
-        &sudoers::UserInfo {
-            user: &context.target_user.name,
-            group: &context.target_group.name,
+        &context.current_user,
+        sudoers::Request {
+            user: &context.target_user,
+            group: &context.target_group,
         },
         &context.hostname,
         &sudo_options.external_args.join(" "),


Test patch:
diff --git a/lib/sudo-cli/tests/lib.rs b/lib/sudo-cli/tests/lib.rs
index e3d7e5e8..77f96ee6 100644
--- a/lib/sudo-cli/tests/lib.rs
+++ b/lib/sudo-cli/tests/lib.rs
@@ -146,3 +146,40 @@ fn trailing_args_hyphens_known_flag() {
 fn remove_and_reset_timestamp_exclusion() {
     SudoOptions::try_parse_from(["sudo", "--reset-timestamp", "--reboot-timestamp"]).unwrap();
 }
+
+/// Check that the first environment variable declaration before any command is not treated as part
+/// of the command.
+#[test]
+fn first_trailing_env_var_is_not_an_external_arg() {
+    let cmd = SudoOptions::try_parse_from(["sudo", "FOO=1", "command", "BAR=2"]).unwrap();
+    assert_eq!(cmd.env_var_list, vec![("FOO".to_owned(), "1".to_owned()),]);
+    assert_eq!(cmd.external_args, vec!["command", "BAR=2"]);
+}
+
+#[test]
+fn trailing_env_vars_are_external_args() {
+    let cmd = SudoOptions::try_parse_from([
+        "sudo", "FOO=1", "-b", "BAR=2", "command", "BAZ=3", "arg", "FOOBAR=4", "command", "arg",
+        "BARBAZ=5",
+    ])
+    .unwrap();
+    assert!(cmd.background);
+    assert_eq!(
+        cmd.env_var_list,
+        vec![
+            ("FOO".to_owned(), "1".to_owned()),
+            ("BAR".to_owned(), "2".to_owned())
+        ]
+    );
+    assert_eq!(
+        cmd.external_args,
+        vec!["command", "BAZ=3", "arg", "FOOBAR=4", "command", "arg", "BARBAZ=5"]
+    );
+}
+
+#[test]
+fn single_env_var_declaration() {
+    let cmd = SudoOptions::try_parse_from(["sudo", "FOO=1", "command"]).unwrap();
+    assert_eq!(cmd.env_var_list, vec![("FOO".to_owned(), "1".to_owned())]);
+    assert_eq!(cmd.external_args, vec!["command"]);
+}
diff --git a/test-binaries/checkpermit.rs b/test-binaries/checkpermit.rs
index eb4fe25e..3faa243a 100644
--- a/test-binaries/checkpermit.rs
+++ b/test-binaries/checkpermit.rs
@@ -12,17 +12,17 @@ fn sudoers_parse(lines: impl Iterator<Item = String>) -> impl Iterator<Item = su
 fn chatty_check_permission(
     sudoers: impl Iterator<Item = String>,
     am_user: &str,
-    request: &sudoers::UserInfo,
+    request: sudoers::Request<&str, sudoers::GroupID>,
     on_host: &str,
     chosen_poison: &str,
 ) {
     println!(
-        "Is '{}' allowed on '{}' to run: '{}' (as {}:{})?",
+        "Is '{}' allowed on '{}' to run: '{}' (as {}:{:?})?",
         am_user, on_host, chosen_poison, request.user, request.group
     );
     let (input, aliases) = sudoers::analyze(sudoers_parse(sudoers));
     let result =
-        sudoers::check_permission(&input, &aliases, am_user, request, on_host, chosen_poison);
+        sudoers::check_permission(&input, &aliases, &am_user, request, on_host, chosen_poison);
     println!("OUTCOME: {result:?}");
 }
 
@@ -31,6 +31,7 @@ use std::fs::File;
 use std::io::{self, BufRead};
 
 fn main() {
+    use sudoers::GroupID;
     let args: Vec<String> = env::args().collect();
     if let Ok(file) = File::open("./sudoers") {
         let cfg = io::BufReader::new(file).lines().map(|x| x.unwrap());
@@ -39,9 +40,12 @@ fn main() {
             chatty_check_permission(
                 cfg,
                 &args[1],
-                &sudoers::UserInfo {
-                    user: args.get(4).unwrap_or(&"root".to_string()),
-                    group: args.get(5).unwrap_or(&"root".to_string())
+                sudoers::Request::<&str, GroupID> {
+                    user: &args.get(4).unwrap_or(&"root".to_owned()).as_str(),
+                    group: &args
+                        .get(5)
+                        .map(|x| GroupID(2347, Some(x.clone())))
+                        .unwrap_or_else(|| (GroupID(0, Some("root".to_owned()))))
                 },
                 &args[2],
                 &args[3],