Skip to content
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

many: do not use nss when looking up for users/groups from snapd snap #13776

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

valentindavid
Copy link
Contributor

@valentindavid valentindavid commented Apr 2, 2024

Depends on #13370

When snapd runs as a snap, it has its own runtime. This may not have NSS plugins needed for the host. For example to get users from AD/LDAP/Kerberos, or systemd-homed, or custom user databses. In general we can use tag osusergo to make go not to use the local configuration (i.e. /etc/nsswitch.conf), however, even if it is fine for most databases, we really need users and groups to be resolved with the host configuration.

To be able to load correctly plugins, we expect the host system to provide getent. And we query passwd and group databases through this command.

In the future we should connect the systemd-userdb if it is running and use getent only as fallback.

@github-actions github-actions bot added Needs Documentation -auto- Label automatically added which indicates the change needs documentation Run Nested -auto- Label automatically added in case nested tests need to be executed labels Apr 2, 2024
}
return nil, err
}
line = append(line, chunk...)
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're doing what bufio.Scanner would do, but less efficiently, we should use scanner here (and in lookupExtraUser)

}
}

if len(line) == 0 || line[0] == '#' {
Copy link
Contributor

Choose a reason for hiding this comment

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

bytes.TrimSpace() before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think this is allowed.

Comment on lines 68 to 71
components := strings.SplitN(string(line), ":", 4)
if len(components) != 4 {
continue
}

if components[index] != expectedValue {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this could probably be in a callback, eg func(line []byte) error, so that you can share the code reading the file between group/user since processing is identical for the both, eg.

var extrausersGroup = "/var/lib/extrausers/group"

func lookupExtraGroup(group string) (gr *Group, err error) {
	gb := []byte(group)
	err = processPasswdFile(extrausersGroup, func(line []byte) error {
		components := strings.SplitN(line, []byte(":"), 4)
		if bytes.Equal(components[0], gb) {
			gr = &Group{
				Name: string(components[0]),
				Gid: string(components[2]),
			}
			// we're done
			return io.EOF
		}
		return nil
	})
	if err != nil {
		return nil, err
	}
	return gr, nil
}

@valentindavid
Copy link
Contributor Author

This branch depends on #13517 to be properly tested.

@valentindavid valentindavid added the Run nested The PR also runs tests inluded in nested suite label Jul 8, 2024
@valentindavid valentindavid reopened this Jul 8, 2024
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.74%. Comparing base (ea13a33) to head (b9df3a3).
Report is 37 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13776      +/-   ##
==========================================
+ Coverage   78.73%   78.74%   +0.01%     
==========================================
  Files        1055     1061       +6     
  Lines      138275   139197     +922     
==========================================
+ Hits       108866   109609     +743     
- Misses      22588    22736     +148     
- Partials     6821     6852      +31     
Flag Coverage Δ
unittests 78.74% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@valentindavid valentindavid force-pushed the valentindavid/user-no-nss branch 3 times, most recently from 44bf5f1 to ecc0756 Compare July 10, 2024 11:53
@valentindavid valentindavid marked this pull request as ready for review July 11, 2024 13:40
@@ -27,11 +27,11 @@ import (
"errors"
"fmt"
"net/url"
"os/user"
Copy link
Contributor

Choose a reason for hiding this comment

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

btw. perhaps we could prevent from os/user being imported by our code, there's a depguard linter we could configure in golangci-lint: https://golangci-lint.run/usage/linters/#depguard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not done this one...

package user

import (
origUser "os/user"
Copy link
Contributor

Choose a reason for hiding this comment

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

osuser ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"orig" for "original"

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, I would also prefer osuser for simplicity.

}, nil
}

func lookupUserFromGetent(index int, expectedValue string) (*User, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

hm I'm slightly uneasy with the caller needing to know the index here, what if we had:

type matcher func() (index int, value string)

func lookupUserFromGetent(match matcher) (*User, error) {
	index, value := match()
	...
}

// and relevant matchers
func userMatch(name string) matcher { 
	return func() (int, string) { return 0, name }
}

func uidMatch(uid int) (int string) matcher {
	return func() (int, string) { return 2, strconv.Itoa(uid) }
}

func groupMatch (name string) matcher {
	return func() (int, string) { return 0, name }
}

and the the actual call:

u, err := lookupUserFromGetent(userMatch("foo"))
// or 
u, err := lookupUserFromGetent(uidMatch(os.Geteuid()))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure that sounds fine. I did not want to over engineer it because we are not really going to even add more stuff in there. But this does not seem to be a complex design. I will try 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.

I will push a fix. I have made 2 different matcher types. Even though they do the same, in theory they do not make sense when used to the wrong function.

@@ -305,6 +302,7 @@ parts:
esac
;;
esac
TAGS+=(snap osusergo)
Copy link
Contributor

Choose a reason for hiding this comment

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

github.com/godbus/dbus seems to call user.Current() a couple of times in some fallback paths:

vendor/github.com/godbus/dbus/conn_other.go
80:     if currentUser, err := user.Current(); err != nil {

vendor/github.com/godbus/dbus/homedir_dynamic.go
10:     u, err := user.Current()

which could be incorrect with osusergo. Perhaps we should still pack libnss-extrausers to handle this edge case but make sure our code is using the osutil wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the cases where HOME environment variable is not defined? I wonder what weird cases exist without 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.

Those cases should work. One is for the home dir, and if HOME defined it should work. Which should be 99.99% cases.

The other case is to get the uid, which should work correct in osusergo. It is just os.Getuid

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Marking as requested changes since the numeric / digit code looks wrong.

Separately, I really wish Go had a way to do this globally without the developer being really careful not to import the other package. I wonder what else might be using os/user internally in Go? Do we know?

cmd.Stderr = &errBuf

if err := cmd.Run(); err != nil {
return nil, fmt.Errorf("getent returned an error: %q", errBuf.Bytes())
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should dispaly the error code here?

Alternatively if we don't capture stderr we could just return fmt.Errorf("... : %w", err) here:

type ExitError struct {
	*os.ProcessState

	// Stderr holds a subset of the standard error output from the
	// Cmd.Output method if standard error was not otherwise being
	// collected.
	//
	// If the error output is long, Stderr may contain only a prefix
	// and suffix of the output, with the middle replaced with
	// text about the number of omitted bytes.
	//
	// Stderr is provided for debugging, for inclusion in error messages.
	// Users with other needs should redirect Cmd.Stderr as needed.
	Stderr []byte
}


func isNumeric(value string) bool {
for _, c := range value {
return unicode.IsDigit(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to use this. This will return true for roman numerals, fractions and other weird stuff: https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/unicode/digit.go

Separately this is a misnomer. Unicode has deifnitions for digits and number that are different. There's also unicode.IsNumber which we are not calling here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a big issue if we simply check that c >= '0' && c <= '9'?

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 is what I have changed to.

package user

import (
origUser "os/user"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, I would also prefer osuser for simplicity.

osutil/user/user.go Show resolved Hide resolved
@valentindavid valentindavid force-pushed the valentindavid/user-no-nss branch 8 times, most recently from d4c8037 to 1bab881 Compare September 19, 2024 10:39
@ernestl ernestl added this to the 2.66 milestone Sep 19, 2024
@ernestl
Copy link
Collaborator

ernestl commented Sep 19, 2024

We might need this in coming release. Added milestone 2.66 to track it.
For detail discuss with @valentindavid.

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

LGTM with small nitpicks on pass-by-value and use of errors.As

.golangci.yml Outdated
- "!**/osutil/user/*.go"
deny:
- pkg: "os/user"
desc: "Please use osutil/user instead"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link to a spec number or this PR perhaps?


outBuf, err := cmd.Output()
if err != nil {
exitError, isExitError := err.(*exec.ExitError)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be errors.As technically.

exitError, isExitError := err.(*exec.ExitError)
if isExitError {
return nil, fmt.Errorf("getent returned an error: %q", exitError.Stderr)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop the else and unindent the return

value string
}

func (m *groupnameMatcher) index() int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, this can be passed by value

return 0
}

func (m *groupnameMatcher) expectedValue() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

value string
}

func (m *usernameMatcher) index() int {
Copy link
Contributor

Choose a reason for hiding this comment

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

By value

}

func userMatchUsername(username string) userMatcher {
return &usernameMatcher{
Copy link
Contributor

Choose a reason for hiding this comment

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

By value

value int
}

func (m *uidMatcher) index() int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

}

func userMatchUid(uid int) userMatcher {
return &uidMatcher{
Copy link
Contributor

Choose a reason for hiding this comment

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

My value

)

type User = osuser.User
type Group = osuser.Group
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: type ( ... ) perhaps?

Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM

# include-go-root: false
# packages:
# - github.com/davecgh/go-spew/spew
depguard:
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for this

Comment on lines 37 to 41
exitError, isExitError := err.(*exec.ExitError)
if isExitError {
return nil, fmt.Errorf("getent returned an error: %q", exitError.Stderr)
} else {
return nil, fmt.Errorf("getent could not be executed: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
exitError, isExitError := err.(*exec.ExitError)
if isExitError {
return nil, fmt.Errorf("getent returned an error: %q", exitError.Stderr)
} else {
return nil, fmt.Errorf("getent could not be executed: %w", err)
}
var exitError *exec.ExitError
if errors.As(err, &exitError) {
return nil, fmt.Errorf("getent returned an error: %q", exitError.Stderr)
} else {
return nil, fmt.Errorf("getent could not be executed: %w", err)
}

@@ -0,0 +1,74 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
//go:build snap
Copy link
Contributor

Choose a reason for hiding this comment

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

hm not sure about the build tag, maybe @pedronis has an opinion about it

I would try something more specific, eg snapdusergo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have renamed it.

@valentindavid valentindavid force-pushed the valentindavid/user-no-nss branch 2 times, most recently from 45a215d to 7bd1ffb Compare September 19, 2024 11:43
…apd snap

When snapd runs as a snap, it has its own runtime. This may not have
NSS plugins needed for the host. For example to get users from
AD/LDAP/Kerberos, or systemd-homed, or custom user databses.  In
general we can use tag `osusergo` to make go not to use the local
configuration (i.e. `/etc/nsswitch.conf`), however, even if it is fine
for most databases, we really need users and groups to be resolved
with the host configuration.

To be able to load correctly plugins, we expect the host system to
provide `getent`. And we query `passwd` and `group` databases through
this command.

In the future we should connect the systemd-userdb if it is
running and use `getent` only as fallback.
@ernestl
Copy link
Collaborator

ernestl commented Sep 30, 2024

Failures:
nested-ubuntu-24.04 |

  • google-nested:ubuntu-24.04-64:tests/nested/manual/hybrid-remodel | known, already addressed, unrelated

@ernestl ernestl merged commit 15296f9 into canonical:master Sep 30, 2024
53 of 54 checks passed
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

it's a bit unclear what happens with unit tests here given the new tags? also do we need a spread test that shows the changes to be relevant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

why from_snap in the name of this one? also this code doesn't seem to be tested?

// The component at `index` will need to match `expectedValue`.
// If `isKey`, then `expectedValue` will also be passed as parameter
// to getent along `database`. `numComponents` should be 4 for groups
// and 7 for users.
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be a bit cleaner to use a map database -> #fields and return an internal error if database is not in the map

@@ -0,0 +1,60 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
//go:build !snapdusergo
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need a new unit tests variant in GH, or changes to the current ones? cc @bboozzoo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +89 to +92
func isKey(index int, expectedValue string) bool {
numeric := isNumeric(expectedValue)
return (index == 0 && !numeric) || (index == 2 && numeric)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

couldn't isKey also be part of the interfaces below?

// If `isKey`, then `expectedValue` will also be passed as parameter
// to getent along `database`. `numComponents` should be 4 for groups
// and 7 for users.
func lookupFromGetent(database string, index int, expectedValue string, isKey bool, numComponents int) ([]string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems this could an interface instead:

type entMatcher interface {
  index() int
  expectedValue() string
  isKey() bool
}

}

if components == nil {
return nil, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

test don't seem to hit here

@ernestl
Copy link
Collaborator

ernestl commented Sep 30, 2024

@valentindavid will do followup PRs for improvements related to latest comments (after merged).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Documentation -auto- Label automatically added which indicates the change needs documentation Run Nested -auto- Label automatically added in case nested tests need to be executed Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants