fix: fall back to getent for users that are not findable in /etc/passwd#183
fix: fall back to getent for users that are not findable in /etc/passwd#183tonyandrewmeyer wants to merge 6 commits into
Conversation
|
FYI, you can test this with the |
dimaqq
left a comment
dimaqq
left a comment
There was a problem hiding this comment.
Fly-by: the change looks good to me.
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Landing this as-is seems reasonable to me.
I do wonder if we should swallow all errors from trying to run getent the way we are, and also if we should have a test exercising how we fail if getent isn't available.
I don't think these should block landing this feature since Ubuntu should always have getent and 'user not found' is probably our only expected error.
| func lookupUserGetent(username string) (*user.User, error) { | ||
| out, err := exec.Command("getent", "passwd", username).Output() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("user: unknown user %s", username) |
There was a problem hiding this comment.
Is swallowing any error from getent this way the right approach? I assume there could be others other than 'unknown user'. Should we instead wrap err here, or inspect it to decide how to fail?
| if _, err := exec.LookPath("getent"); err != nil { | ||
| t.Skip("getent not available on this system") | ||
| } |
There was a problem hiding this comment.
Should we have a test that shows how we fail if getent isn't available? Maybe if we have a mockable getentBinary variable in util.go?
I guess right now that would surface as "user: unknown user %s".
3 participants
The go
User.Lookupmethod on Unix-like systems without ego just looks in the /etc/password file (whereas cgo does a getpwnam check via libc). On some systems, like the Canonical laptops, the user that is trying to run Concierge may not be in/etc/passwdand be loaded from a directory service instead.This PR adds a fallback system where we will run the getent binary. That comes from the
libc-binpackage in Ubuntu, so is very likely to be available (and the worst case is that it's not and we'll fail just as we did before).Fixes #182