-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add -uid and -uid flags to drop permissions #804
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know this makes Yggdrasil no longer portable, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see the circleci failure under the "other" OS (I assume that's windows?). I'm a bit surprised there's an OS that doesn't have UIDs, but just a little bit. Anyhow, I guess the fix for that would be to move the args struct and arg parser into a arg_windows.go and a arg_posix.go, and only have the gid/uid options in the args_posix side. Similarly define a drop_perm_unix.go that does the above, and a drop_perm_windows.go that's just a no-op. I can definitely do that, but before I do, is there interest in privilege dropping in yggdrasil? I'm happy to do the grunt work, but if it's something that's not seen as necessary anyhow, then I can just drop this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More importantly,
syscall.Setgid
(or setting UIDs) probably won't work in Go. Or to be exact, it will set the ID for whatever random thread the goroutine happened to be running on at the time, but the rest of the threads will be unaffected.The language recently added https://golang.org/pkg/syscall/#AllThreadsSyscall which is meant to address issues like this, but it has some caveats associated with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the
AllThreadsSyscall
function is only available on linux, so this feature wouldn't be cross-platform.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POSIX setuid is required to set the uid for the entire process (ref and a stackoverflow). It definitely appears to be working under FreeBSD and Linux, but I haven't tried it under OSX. I'd expect it to work the same there, as OSX's POSIX layer appears to still be based on FreeBSD.
Or am I misunderstanding the problem? It looks like Linux is actually the weird one, because it does (in the kernel) implement per-thread uids, and sane programs depend on glibc or musl to provide the POSIX semantics that people tend to expect (more). If Go's "libc" really is just sending the raw syscalls to the kernel, then I can see how the AllThreadsSyscall would be needed to be run explicitly on LInux systems. Unfortunately I'm not actually a Go programmer, and I have no idea what AllThreadsSyscall's function signature means. So, I guess I'll drop this, unless somebody really wants to see it completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into it more, it appears that
Setgid
andSetuid
were rewritten to useAllThreadsSyscall
on linux (as long ascgo
isn't being used), so this should be doing the right thing.Some refactoring/cleanup is needed to avoid breaking things on platforms where
syscall.Setuid
/syscall.Setgid
are undefined (if any... windows maybe?). That shouldn't be too difficult, I can probably take a look at in the next few days.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If somebody else could figure out how to do the conditional compilation thing, that would be cool. I tried to split out main.go's argv into its own main_posix.go and main_other.go with
// +build
directives for per-OS compilation, and I just couldn't get the thing to compile. I don't know Go though, and its package system seems to be pretty unique, so I'm sure I was just doing something really basic wrong.