argument to change uid/gid #927

cathugger · 2022-07-11T16:45:32Z

different from #817 in that it can resolve user names, automatically use user's primary gid & allows specifying gid in the same argument, with : eg username:groupname.
feel free to criticize & suggest different argument name & description because i didn't put much of thought to that.

Arceliar · 2022-08-28T18:57:03Z

cmd/yggdrasil/main.go

@@ -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 != "" {


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?

@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).

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

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.

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

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.

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?

Actually isn't that what #817 is doing?

#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.

unless multicast actually works without requiring root (i somehow confused it with managing interfaces, and using multicast aint that)

cathugger · 2022-08-30T14:37:52Z

I looked around a bit, and git daemon implements this in a bit cleaner way, maybe, command line arguments wise:

   --user=<user>, --group=<group>
       Change daemon's uid and gid before entering the service loop. When
       only --user is given without --group, the primary group ID for the
       user is used. The values of the option are given to getpwnam(3) and
       getgrnam(3) and numeric IDs are not supported.

       Giving these options is an error when used with --inetd; use the
       facility of inet daemon to achieve the same before spawning git
       daemon if needed.

       Like many programs that switch user id, the daemon does not reset
       environment variables such as $HOME when it runs git programs, e.g.
       upload-pack and receive-pack. When using this option, you may also
       want to set and export HOME to point at the home directory of
       <user> before starting the daemon, and make sure any Git
       configuration files in that directory are readable by <user>.

Though nsd for example uses different syntax, using -u and taking in username, id, or id.gid, which is more similar to what I've implemented here.
I think that current implementation is kinda fine, but maybe documentation could be improved, but I can't really think of anything good atm.

cathugger · 2023-04-01T18:50:05Z

rebased.

does this need any further work?
should I change argument format / documentation? current stuff is kinda unclear a bit imo but does anyone else find that to be an issue?
splitting to two arguments could make argument documentation easier too probably.
i think multicast stuff should work fine with this unless multicast port is picked to be <=1024.

neilalexander · 2023-04-06T22:10:30Z

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.

neilalexander · 2023-10-21T17:17:23Z

If you're happy to rebase one last time on top of develop, let's get this merged for v0.5.

cmd/yggdrasil/chuser_unix.go

cathugger · 2023-12-08T18:19:37Z

@neilalexander done.
was away from this account while recovering from btrfs corruption.

cmd/yggdrasil/chuser_unix.go

+
+	if g != nil {
+		gid, _ := strconv.ParseUint(g.Gid, 10, 32)
+		err := syscall.Setgid(int(uint32(gid)))


cmd/yggdrasil/chuser_unix.go

+		}
+	} else if u != nil {
+		gid, _ := strconv.ParseUint(u.Gid, 10, 32)
+		err := syscall.Setgid(int(uint32(gid)))


cmd/yggdrasil/chuser_unix.go

+
+	if u != nil {
+		uid, _ := strconv.ParseUint(u.Uid, 10, 32)
+		err := syscall.Setuid(int(uint32(uid)))


cathugger force-pushed the setuid branch from 2d3238a to ddca637 Compare July 12, 2022 15:54

Arceliar reviewed Aug 28, 2022

View reviewed changes

neilalexander mentioned this pull request Sep 24, 2022

Add -uid and -uid flags to drop permissions #804

Closed

neilalexander force-pushed the develop branch from 5a0470e to 8ce7c86 Compare October 15, 2022 16:46

cathugger force-pushed the setuid branch from ddca637 to 424cf72 Compare April 1, 2023 18:31

neilalexander added the enhancement Enhances/improves functionality or UX label Apr 6, 2023

This was referenced Oct 21, 2023

[revised] Add -uid and -uid flags to drop permissions #817

Closed

[Security] do not run as root. #816

Closed

Make possible to run without root; socket creation. #802

Closed

argument to change uid/gid

f54103e

cathugger force-pushed the setuid branch from 424cf72 to f54103e Compare December 8, 2023 18:15

github-advanced-security bot found potential problems Dec 8, 2023

View reviewed changes

cmd/yggdrasil/chuser_unix.go Fixed Show fixed Hide fixed

cmd/yggdrasil/chuser_unix.go Fixed Show fixed Hide fixed

cmd/yggdrasil/chuser_unix.go Fixed Show fixed Hide fixed

attempt to make CodeQL warnings go away

fc5c13f

github-advanced-security bot found potential problems Dec 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

argument to change uid/gid #927

argument to change uid/gid #927

cathugger commented Jul 11, 2022

Arceliar Aug 28, 2022

rany2 Aug 28, 2022 •

edited

cathugger Aug 29, 2022

cathugger Aug 30, 2022

rany2 Aug 30, 2022

cathugger Aug 30, 2022

neilalexander Sep 24, 2022

neilalexander Sep 24, 2022

cathugger Sep 28, 2022

cathugger Sep 28, 2022

cathugger commented Aug 30, 2022

cathugger commented Apr 1, 2023

neilalexander commented Apr 6, 2023

neilalexander commented Oct 21, 2023

cathugger commented Dec 8, 2023

argument to change uid/gid #927

Are you sure you want to change the base?

argument to change uid/gid #927

Conversation

cathugger commented Jul 11, 2022

Choose a reason for hiding this comment

rany2 Aug 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathugger commented Aug 30, 2022

cathugger commented Apr 1, 2023

neilalexander commented Apr 6, 2023

neilalexander commented Oct 21, 2023

cathugger commented Dec 8, 2023

rany2 Aug 28, 2022 •

edited