Skip to content

argument to change uid/gid #927

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Sep 22, 2024
Merged

argument to change uid/gid #927

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7cd0f6b
argument to change uid/gid
cathugger Jul 11, 2022
72fbe25
attempt to make CodeQL warnings go away
cathugger Dec 8, 2023
87fa0a8
Update cmd/yggdrasil/chuser_unix.go
neilalexander Aug 3, 2024
cb29747
Update cmd/yggdrasil/chuser_unix.go
neilalexander Aug 3, 2024
a2f35a3
Fix imports
neilalexander Aug 3, 2024
fb587b1
Merge branch 'develop' into setuid
neilalexander Sep 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cmd/yggdrasil/chuser_other.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//go:build !aix && !darwin && !dragonfly && !freebsd && !linux && !netbsd && !openbsd && !solaris
// +build !aix,!darwin,!dragonfly,!freebsd,!linux,!netbsd,!openbsd,!solaris

package main

import "errors"

func chuser(user string) error {
return errors.New("setting uid/gid is not supported on this platform")
}
85 changes: 85 additions & 0 deletions cmd/yggdrasil/chuser_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
//go:build aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris
// +build aix darwin dragonfly freebsd linux netbsd openbsd solaris

package main

import (
"fmt"
osuser "os/user"
"strconv"
"strings"
"syscall"
)

func chuser(user string) error {
group := ""
if i := strings.IndexByte(user, ':'); i >= 0 {
user, group = user[:i], user[i+1:]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just strings.Split(user, ':')?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IndexByte is probably speediest as Split allocates slice and uses string for splitting.

This code runs once, it is not a hot path.

Cut would 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.


u := (*osuser.User)(nil)
g := (*osuser.Group)(nil)

if user != "" {
if _, err := strconv.ParseUint(user, 10, 32); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm we do LookupId after this anyway so it's a moot point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parsing before potentially IO hitting pw database lookup is probably what i thought back when i wrote this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

LookupId depends 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

u, err = osuser.LookupId(user)
if err != nil {
return fmt.Errorf("failed to lookup user by id %q: %v", user, err)
}
} else {
u, err = osuser.Lookup(user)
if err != nil {
return fmt.Errorf("failed to lookup user by name %q: %v", user, err)
}
}
}
if group != "" {
if _, err := strconv.ParseUint(group, 10, 32); err == nil {
g, err = osuser.LookupGroupId(group)
if err != nil {
return fmt.Errorf("failed to lookup group by id %q: %v", user, err)
}
} else {
g, err = osuser.LookupGroup(group)
if err != nil {
return fmt.Errorf("failed to lookup group by name %q: %v", user, err)
}
}
}

if g != nil {
gid, _ := strconv.ParseUint(g.Gid, 10, 32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questionable for this API to return the ID for a known-to-exist group as string...

var err error
if gid < math.MaxInt {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os/user returns ID strings that were "validated" with strconv.Atoi() which returns int, so this should always be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

87fa0a8 apparently was someone's else's change too but I can't argue one way or another abt it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to satisfy faulty Code scanning / CodeQL warnings out of all the things

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to satisfy faulty Code scanning / CodeQL warnings out of all the things

This looks related over at #1202:
Screenshot 2024-11-04 at 04-05-21 Set groups when dropping privileges to not leak supplementary group access by klemensn · Pull Request #1202 · yggdrasil-network_yggdrasil-go

Copy link
Contributor

@klemensn klemensn Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :group changes 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.

err = syscall.Setgid(int(gid))
} else {
err = errors.New("gid too big")
}

if err != nil {
return fmt.Errorf("failed to setgid %d: %v", gid, err)
}
} else if u != nil {
gid, _ := strconv.ParseUint(u.Gid, 10, 32)
err := syscall.Setgid(int(uint32(gid)))
if err != nil {
return fmt.Errorf("failed to setgid %d: %v", gid, err)
}
}

if u != nil {
uid, _ := strconv.ParseUint(u.Uid, 10, 32)
var err error
if uid < math.MaxInt {
err = syscall.Setuid(int(uid))
} else {
err = errors.New("uid too big")
}

if err != nil {
return fmt.Errorf("failed to setuid %d: %v", uid, err)
}
}

return nil
}
9 changes: 9 additions & 0 deletions cmd/yggdrasil/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func main() {
getsnet := flag.Bool("subnet", false, "use in combination with either -useconf or -useconffile, outputs your IPv6 subnet")
getpkey := flag.Bool("publickey", false, "use in combination with either -useconf or -useconffile, outputs your public key")
loglevel := flag.String("loglevel", "info", "loglevel to enable")
chuserto := flag.String("user", "", "user (and, optionally, group) to set UID/GID to")
flag.Parse()

done := make(chan struct{})
Expand Down Expand Up @@ -280,6 +281,14 @@ func main() {
<-done
})

// Change user if requested
if *chuserto != "" {
err = chuser(*chuserto)
if err != nil {
panic(err)
}
}

Copy link
Contributor

@klemensn klemensn Nov 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Block until we are told to shut down.
<-ctx.Done()

Expand Down