argument to change uid/gid #927
Conversation
cmd/yggdrasil/main.go
Outdated
| @@ -361,6 +364,13 @@ func run(args yggArgs, ctx context.Context, done chan struct{}) { | |||
| logger.Errorln("An error occurred starting TUN/TAP:", err) | |||
| } | |||
| n.tuntap.SetupAdminHandlers(n.admin) | |||
| // Change user if requested | |||
| if args.chuser != "" { | |||
There was a problem hiding this comment.
It may make sense to change to nobody:nobody by default, so things are a little less unsafe for people who just sudo yggdrasil -autoconf or similar. Thoughts @neilalexander?
There was a problem hiding this comment.
@Arceliar I think it should be noted that nobody:nobody is not standard across Linux distros. For example, Debian and friends use nobody:nogroup instead. We should check which group is available first or maybe use something like Dynamic Users in systemd (use a random UID/GID that's currently not in use by the system).
There was a problem hiding this comment.
could just use nobody and maybe it would just resolve primary group on its own i think?
also would need to check UID being 0 before doing chuser by default (as this can be started as own user if right caps are given).
and opting out may be kinda uglier - manually specifying empty user string, though i can't really find many reasons of opting out, other than some possible exotic distributions/installations without proper user databases - which i guess i could imagine in some sterilized docker environments or routers but i don't really know how common, and probably checking existence of nobody user before deciding to default to chuser'ing to it would make sense - and we could just check database before setting default to nobody, likely writing function in chuser_unix.go for that (& stub returning empty string in chuser_other.go).
Requesting opinions for this mindfuzz of mine i guess
There was a problem hiding this comment.
I thought about this a bit more and probably programming in this kind of logic into daemon itself is too much of a magical behavior imo, and this should be provided in distributions' .service files instead.
There was a problem hiding this comment.
well there isn't any need for this to be in any distro, Debian has a pretty hardened default that runs as yggdrasil user from the start. I think all you need to get it to work is CAP_NET_ADMIN, I think this is how Debian configured their OpenVPN services too
There was a problem hiding this comment.
I see. This packaging is indeed fairly good. This has gotten a bit offtopic, but it would be cool if one could specify default socket / config path at compile time (eg using go build -ldflags="-X defaults.defaultAdminListen=unix:///var/run/yggdrasil/yggdrasil.sock") so that linux distros wouldn't need patches there to achieve saner packaging.
Either way this repo's systemd files suck at running non-root, and so consequentially do vendor (not official debian's) debs and archlinux package.
There was a problem hiding this comment.
On the whole, I'm very pro-having separate flags for UID and GID, because then it is clearer that we can do one without the other, and it is easier for packagers to do the right thing for their distribution/platform.
I'm not entirely sure whether we want to do that by default — we could certainly do some testing to see how it feels. The main thing I can think of that might break is setting up new multicast sockets when an interface disappears and reappears later but we've already dropped privileges, but maybe it'll be OK?
There was a problem hiding this comment.
Actually isn't that what #817 is doing?
There was a problem hiding this comment.
#817 can take only numeric UID/GIDs, which is unwieldy.
because then it is clearer that we can do one without the other
why would you want do one but not another? everywhere i looked around, one either wants to set uid and also gid to primary group of uid, or uid/gid separate, which is also possible. with this implementation, one can also set gid without uid (eg -user ":groupname" or -user ":gid"), though it may be a little bit unobvious, but why would you want to not drop root?
if you think that instead of using -user uid:gid syntax we should do -uid user -gid group, i can change to that too, looks like a minimal change to me.
The main thing I can think of that might break is setting up new multicast sockets when an interface disappears and reappears later but we've already dropped privileges, but maybe it'll be OK?
that's sorta fundamentally incompatible with privilege dropping tbh. on linux, we could probably do some magic with caps and reserve the right to deal with interfaces, but on other unixes we can't. restarting daemon when new interfaces to be managed pop up could probably make sense, but i don't quite feel that's what yggdrasil should do on its own either.
There was a problem hiding this comment.
unless multicast actually works without requiring root (i somehow confused it with managing interfaces, and using multicast aint that)
|
I looked around a bit, and git daemon implements this in a bit cleaner way, maybe, command line arguments wise: Though nsd for example uses different syntax, using |
neilalexander
force-pushed
the
develop
branch
from
5a0470e
to
8ce7c86
Compare
|
rebased. does this need any further work? |
|
I haven't forgotten about this — I'd like to get this merged in some shape or other, but let's wait until the v0.5 branch is a bit cleaner before rebasing there. |
|
If you're happy to rebase one last time on top of |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@neilalexander done. |
Co-authored-by: VNAT <[email protected]>
Co-authored-by: VNAT <[email protected]>
| group := "" | ||
| if i := strings.IndexByte(user, ':'); i >= 0 { | ||
| user, group = user[:i], user[i+1:] | ||
| } |
There was a problem hiding this comment.
Why not just strings.Split(user, ':')?
There was a problem hiding this comment.
IndexByte is probably speediest as Split allocates slice and uses string for splitting.
Cut would probably be more fitting here API niceness wise, but I'm not sure if really worth changing at this point.
There was a problem hiding this comment.
IndexByteis probably speediest asSplitallocates slice and uses string for splitting.
This code runs once, it is not a hot path.
Cutwould probably be more fitting here API niceness wise, but I'm not sure if really worth changing at this point.
Sure, either way looks cleaner than manual indexing.
| g := (*osuser.Group)(nil) | ||
|
|
||
| if user != "" { | ||
| if _, err := strconv.ParseUint(user, 10, 32); err == nil { |
There was a problem hiding this comment.
Why parse it yourself? You can try a name lookup, fallback to UID on error and return if that fails, too, just like getpwnam(3) behaves:
if u, err = osuser.Lookup(user); err != nil {
if u, err = osuser.LookupId(user); if err != nil {
return err
}
}
There was a problem hiding this comment.
this PR introduced as replacement for one that only used numeric IDs so I preserved ability to do chown/chgrp without passwords database file being present in system.
LookupId depends on /etc/passwd being present which is technically not a necessity.
There was a problem hiding this comment.
nvm we do LookupId after this anyway so it's a moot point.
There was a problem hiding this comment.
parsing before potentially IO hitting pw database lookup is probably what i thought back when i wrote this.
There was a problem hiding this comment.
this PR introduced as replacement for one that only used numeric IDs so I preserved ability to do chown/chgrp without passwords database file being present in system.
Do these systems exist? Are they relevant?
LookupIddepends on /etc/passwd being present which is technically not a necessity.
The next line uses that function, so this did not go as planned.
So you might as well clean it up, imho.
There was a problem hiding this comment.
as I've mentioned in following comments, yes, this needs pw database anyways, but i prefer minimizing potentially latency introducing tasks (like opening/closing passwd file) even if they're done at init time, so it's guarded by fast parseuint check. it's a matter of taste, eliminating that goes into category of nitpicking IMO.
| } | ||
|
|
||
| if g != nil { | ||
| gid, _ := strconv.ParseUint(g.Gid, 10, 32) |
There was a problem hiding this comment.
Questionable for this API to return the ID for a known-to-exist group as string...
| if g != nil { | ||
| gid, _ := strconv.ParseUint(g.Gid, 10, 32) | ||
| var err error | ||
| if gid < math.MaxInt { |
There was a problem hiding this comment.
os/user returns ID strings that were "validated" with strconv.Atoi() which returns int, so this should always be true.
There was a problem hiding this comment.
had to dig deep to confirm this but indeed.
also could probably be <=.
feel free to make a PR if you care for this as this looks like something code being saner internally not something that'll lead to bugs.
There was a problem hiding this comment.
feel free to make a PR if you care for this as this looks like something code being saner internally not something that'll lead to bugs.
Either that or a clean(er) chuser_bsd.go implementation, should I ever start using this option.
Point being: Complexity hides bugs, so code ought to be as simple as possible.
To my eye, multiple Lookup*() and Parse*() are not as trivial to reason about as it could be.
There was a problem hiding this comment.
without digging deep into the source of osuser.LookupId one can't even know whether these ints were checked so I'm not sure complexity argument is even applicable here, it's more complex to verify whether the code is correct if there is no check.
my gut instinct when API doesn't tell stuff like returned ranges of ints to just check myself, because checking more usually catches more bugs than not checking stuff like returns.
IMO if check is eliminated, then strconv.Atoi() check behavior should be documented in comment, as otherwise it won't end up more simple.
There was a problem hiding this comment.
also uint32 vs int is potentially hell on its own due to int being 64bit on 64bit systems and strconv.Atoi() ending up verifying different on 32bit vs 64bit systems but honestly systems just don't use ints as large as these for UIDs/GIDs anyways so i can't really care abt this.
There was a problem hiding this comment.
87fa0a8 apparently was someone's else's change too but I can't argue one way or another abt it.
There was a problem hiding this comment.
to satisfy faulty Code scanning / CodeQL warnings out of all the things
There was a problem hiding this comment.
to satisfy faulty Code scanning / CodeQL warnings out of all the things
This looks related over at #1202:
There was a problem hiding this comment.
feel free to make a PR if you care for this as this looks like something code being saner internally not something that'll lead to bugs.
-user ''silently ignored and the process continues without error or change in privilege.-user ':'as well.-user :groupchanges just the group and not the user, contrary to what the usage says:
-user string
user (and, optionally, group) to set UID/GID to
I did fix these now in #1203.
| panic(err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The main goroutine changing user/group to drop privileges is racing against others creating a tun(4) interface, raw socket for routing and UNIX socket as control interface, i.e. startup can fail now as yggdrasil may drop to an unprivileged user before having completed all privileged tasks.
# yggdrasil -autoconf -user nobody
2024/11/02 20:24:32 Build name: yggdrasil-go
2024/11/02 20:24:32 Build version: 0.5.9
2024/11/02 20:24:32 Your public key is 82ff9b15b30dc6e460f907cb3aecbe664c024c101efa773b3ecdccf1cb72d818
2024/11/02 20:24:32 Your IPv6 address is 200:fa00:c9d4:99e4:7237:3e0d:f069:8a26
2024/11/02 20:24:32 Your IPv6 subnet is 300:fa00:c9d4:99e4::/64
2024/11/02 20:24:32 Interface name: tun0
2024/11/02 20:24:32 Interface IPv6: 200:fa00:c9d4:99e4:7237:3e0d:f069:8a26/7
2024/11/02 20:24:32 Interface MTU: 16384
2024/11/02 20:24:32 Admin socket failed to listen: listen unix /var/run/yggdrasil.sock: bind: permission denied
Rerun this a few times; sometimes the control socket gets created as root, sometimes it fails to create it as nobody:
$ ls -ld /var/run/
drwxr-xr-x 7 root wheel 512 Nov 2 20:31 /var/run
Otherwise the main goroutine races dropping privileges races against those carrying out privileges tasks at startup. Reproducible at least on OpenBSD/amd64 7.6-current, where it (expectedly) fails to create a UNIX socket in `0755 root:wheel /var/run/` *after* calling setuid(2) to `nobody`: ``` # yggdrasil -autoconf -user nobody 2024/11/03 21:15:27 Build name: yggdrasil-go 2024/11/03 21:15:27 Build version: 0.5.9 ... 2024/11/03 21:15:27 Admin socket failed to listen: listen unix /var/run/yggdrasil.sock: bind: permission denied ``` Rerun, now the order is flipped: ``` # yggdrasil -autoconf -user nobody 2024/11/03 21:15:34 Build name: yggdrasil-go 2024/11/03 21:15:34 Build version: 0.5.9 [...] 2024/11/03 21:15:34 UNIX admin socket listening on /var/run/yggdrasil.sock [...] ``` The `AdminSocket`s `done` channel is insufficient to sync here, waiting for `n.admin.IsStarted()` does not guarantee socket creation, so export a simple boolean instead. This is a minimal fix to prevent startup failure; TUN interface and raw socket creation (to manage routes) might suffer the same problem, but I have not seem them fail yet. Fixes yggdrasil-network#927.
|
This code fails to unset supplementary groups, i.e. the process retains whatever access it had even after dropping to a different user and group: Thus the process still has permissions to open, e.g. a |
…ccess Changing the real and effective user/group IDs and the saved set-user/group-ID is not enough to get rid of intial access permissions. The list of groups must be cleared also, otherwise a process changing from, e.g. `root:root` to `nobody:nobody` retains rights to access `:wheel` files (assuming `root` is a member of the `wheel` group). For example: ``` # id uid=0(root) gid=0(wheel) groups=0(wheel), 2(kmem), 3(sys), 4(tty), 5(operator), 20(staff), 31(guest) # ./yggdrasil -autoconf -logto /dev/null -user nobody & [1] 4337 # ps -o command,user,group,supgrp -U nobody COMMAND USER GROUP SUPGRP ./yggdrasil -aut nobody nobody wheel,kmem,sys,tty,operator,staff,guest ``` Fix that so the process runs as mere ``` COMMAND USER GROUP SUPGRP ./yggdrasil -aut nobody nobody nobody ``` Fixes yggdrasil-network#927. # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # Дата: Mon Nov 4 03:39:23 2024 +0300 # # Текущая ветка: drop-supplementary-groups # Изменения, которые будут включены в коммит: # изменено: cmd/yggdrasil/chuser_unix.go #
…ccess Changing the real and effective user/group IDs and the saved set-user/group-ID is not enough to get rid of intial access permissions. The list of groups must be cleared also, otherwise a process changing from, e.g. `root:root` to `nobody:nobody` retains rights to access `:wheel` files (assuming `root` is a member of the `wheel` group). For example: ``` # id uid=0(root) gid=0(wheel) groups=0(wheel), 2(kmem), 3(sys), 4(tty), 5(operator), 20(staff), 31(guest) # ./yggdrasil -autoconf -logto /dev/null -user nobody & [1] 4337 # ps -o command,user,group,supgrp -U nobody COMMAND USER GROUP SUPGRP ./yggdrasil -aut nobody nobody wheel,kmem,sys,tty,operator,staff,guest ``` Fix that so the process runs as mere ``` COMMAND USER GROUP SUPGRP ./yggdrasil -aut nobody nobody nobody ``` Fixes yggdrasil-network#927.
- Use unambiguous variable names (w/o package name conflict). - Fail on invalid input such as the empty string or `:`. - Do not change group without user, i.e. fail on `:group`. - Parse input using mnemonic APIs. - Do not juggle between integer types. - Unset supplementary groups. - Use setres[ug]id(2) to match the idiom of OpenBSD base programs. Includes/Supersedes yggdrasil-network#1202. Fixes yggdrasil-network#927. I only tested on OpenBSD (so far), hence the split, but other systems should just work.
Creating UNIX sockets the listen() goroutine that races against the main one dropping to an unprivileged user may cause startup failure when privdrop happens before privileged filesystem access. Setup or fail in New() and only do listen(2) in listen() to avoid this. ``` # yggdrasil -autoconf -user nobody 2024/11/03 21:15:27 Build name: yggdrasil-go 2024/11/03 21:15:27 Build version: 0.5.9 ... 2024/11/03 21:15:27 Admin socket failed to listen: listen unix /var/run/yggdrasil.sock: bind: permission denied ``` Rerun, now the order is flipped: ``` # yggdrasil -autoconf -user nobody 2024/11/03 21:15:34 Build name: yggdrasil-go 2024/11/03 21:15:34 Build version: 0.5.9 [...] 2024/11/03 21:15:34 UNIX admin socket listening on /var/run/yggdrasil.sock [...] ``` Fixes #927.
…ccess (#1202) Changing the real and effective user/group IDs and the saved set-user/group-ID is not enough to get rid of intial access permissions. The list of groups must be cleared also, otherwise a process changing from, e.g. `root:root` to `nobody:nobody` retains rights to access `:wheel` files (assuming `root` is a member of the `wheel` group). For example: ``` # id uid=0(root) gid=0(wheel) groups=0(wheel), 2(kmem), 3(sys), 4(tty), 5(operator), 20(staff), 31(guest) # ./yggdrasil -autoconf -logto /dev/null -user nobody & [1] 4337 # ps -o command,user,group,supgrp -U nobody COMMAND USER GROUP SUPGRP ./yggdrasil -aut nobody nobody wheel,kmem,sys,tty,operator,staff,guest ``` Fix that so the process runs as mere ``` COMMAND USER GROUP SUPGRP ./yggdrasil -aut nobody nobody nobody ``` Fixes #927.
- Use unambiguous variable names (w/o package name conflict). - Fail on invalid input such as the empty string or `:`. - Do not change group without user, i.e. fail on `:group`. - Parse input using mnemonic APIs. - Do not juggle between integer types. - Unset supplementary groups. - Use setres[ug]id(2) to match the idiom of OpenBSD base programs. Includes/Supersedes yggdrasil-network#1202. Fixes yggdrasil-network#927. I only tested on OpenBSD (so far), hence the split, but other systems should just work.
- Use unambiguous variable names (w/o package name conflict). - Fail on invalid input such as the empty string or `:`. - Do not change group without user, i.e. fail on `:group`. - Parse input using mnemonic APIs. - Do not juggle between integer types. - Unset supplementary groups. - Use set[ug]id(2) to follow the idiom of OpenBSD base programs. (cannot use setres[ug]id(2) as macOS does not have them.) Includes/Supersedes #1202. Fixes #927. I only tested on OpenBSD (so far), but other systems should just work.
different from #817 in that it can resolve user names, automatically use user's primary gid & allows specifying gid in the same argument, with
:egusername:groupname.feel free to criticize & suggest different argument name & description because i didn't put much of thought to that.