feat: Add newgrp command (#94)#117
Conversation
|
@microsoft-github-policy-service agree |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
| pub fn uu_app() -> Command { | ||
| Command::new("newgrp") | ||
| .about("Log in to a new group") | ||
| .arg( | ||
| Arg::new("group") | ||
| .help("The group to log into") | ||
| .index(1) | ||
| .required(false), | ||
| ) | ||
| } |
There was a problem hiding this comment.
The -c/--command arg is missing.
| } | ||
|
|
||
| // Spawn the shell | ||
| let shell = std::env::var("SHELL").unwrap_or_else(|_| "cmd.exe".to_string()); |
There was a problem hiding this comment.
The original newgrp command does not respect a SHELL environment variable. Where did you find this behavior?
There was a problem hiding this comment.
The original
newgrpcommand does not respect a SHELL environment variable. Where did you find this behavior?
Cygwin/MSYS2
There was a problem hiding this comment.
I'm a bit conflicted on whether we should port that behavior. Other tools so far have generally followed the GNU coreutils behavior, so I would assume that newgrp would follow the util-linux behavior. But we don't have login(1), so...
| let mut token: HANDLE = std::ptr::null_mut(); | ||
| if OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY | TOKEN_ADJUST_DEFAULT, &mut token) == 0 { | ||
| return Err(format!("OpenProcessToken failed: {}", GetLastError())); | ||
| } |
There was a problem hiding this comment.
You don't want to change the token of newgrp but the token of the created process. I believe CreateProcessAsUserW would be the appropriate API for that.
There was a problem hiding this comment.
You don't want to change the token of
newgrpbut the token of the created process. I believeCreateProcessAsUserWwould be the appropriate API for that.
That should be fine, because the token is then inherited by the new process.
There was a problem hiding this comment.
You're right, but I prefer the suggested approach because it expresses the intent better. I think it's a good idea if we treat this like Microsoft reference code to at least a certain extent. And at least conceptually speaking, newgrp does not really modify its own group, right?
I.e.:
duplicate = DuplicateTokenEx(TokenPrimary)
SetTokenInformation(duplicate, TokenPrimaryGroup)
CreateProcessAsUserW(duplicate, shell + command)
| ) | ||
| } | ||
|
|
||
| pub fn uumain(args: impl uucore::Args) -> i32 { |
There was a problem hiding this comment.
Check how grep handles this. You're missing #[uucore::main(no_signals)] and should be returning an UResult<()>. Same for the function below. You can use an io::Error to GetLastError.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Security: The newgrp/sg tools should ONLY allow switching to a group which is listed in TOKEN_GROUPS (read https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-token_groups) and has the SE_GROUP_ENABLED flag set. This is what https://github.com/kofemann/ms-nfs41-client/blob/master/tests/winsg/winsg.c , SFU newgrp.exe, Cygwin newgrp.exe are doing. This prevents users from switching to a "random" group for which the admin or system didn't give them access to. |
|
IMO the tool should be aliased to sg.exe too: |
|
IMO also useful: |
4 participants
-This PR implements the newgrp command requested in #94.
-It uses LookupAccountNameW and SetTokenInformation to modify the primary group of the process token, then spawns a new shell (%SHELL% or cmd.exe), conforming to standard POSIX-like expectations on Windows.
-It is implemented as a local workspace crate (uu_newgrp) since GNU coreutils does not natively provide it.