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

valentindavid · 2024-04-02T12:50:55Z

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

bboozzoo · 2024-04-03T08:11:53Z

osutil/user/user_from_snap.go

+				}
+				return nil, err
+			}
+			line = append(line, chunk...)


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

bboozzoo · 2024-04-03T08:15:50Z

osutil/user/user_from_snap.go

+			}
+		}
+
+		if len(line) == 0 || line[0] == '#' {


bytes.TrimSpace() before?

I do not think this is allowed.

bboozzoo · 2024-04-03T08:17:49Z

osutil/user/user_from_snap.go

+		components := strings.SplitN(string(line), ":", 4)
+		if len(components) != 4 {
+			continue
+		}
+
+		if components[index] != expectedValue {
+			continue
+		}


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 · 2024-06-24T11:51:07Z

This branch depends on #13517 to be properly tested.

codecov-commenter · 2024-07-08T13:24:47Z

⚠️ Please install the 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.

bboozzoo · 2024-07-31T10:39:14Z

client/apps.go

@@ -27,11 +27,11 @@ import (
 	"errors"
 	"fmt"
 	"net/url"
-	"os/user"


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

I have not done this one...

bboozzoo · 2024-07-31T10:46:07Z

osutil/user/user.go

+package user
+
+import (
+	origUser "os/user"


"orig" for "original"

Nitpick, I would also prefer osuser for simplicity.

bboozzoo · 2024-07-31T10:58:33Z

osutil/user/getent.go

+	}, nil
+}
+
+func lookupUserFromGetent(index int, expectedValue string) (*User, error) {


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()))

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.

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.

bboozzoo · 2024-07-31T11:02:40Z

build-aux/snap/snapcraft.yaml

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


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?

For the cases where HOME environment variable is not defined? I wonder what weird cases exist without it.

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

zyga

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?

zyga · 2024-09-09T06:30:14Z

osutil/user/getent.go

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


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 }

zyga · 2024-09-09T06:31:50Z

osutil/user/getent.go

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


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.

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

This is what I have changed to.

zyga · 2024-09-09T06:36:04Z

osutil/user/user.go

+package user
+
+import (
+	origUser "os/user"


Nitpick, I would also prefer osuser for simplicity.

osutil/user/user.go

ernestl · 2024-09-19T11:27:39Z

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

zyga

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

zyga · 2024-09-19T11:21:04Z

.golangci.yml

+          - "!**/osutil/user/*.go"
+        deny:
+          - pkg: "os/user"
+            desc: "Please use osutil/user instead"


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

zyga · 2024-09-19T11:22:56Z

osutil/user/getent.go

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


I think this should be errors.As technically.

zyga · 2024-09-19T11:25:47Z

osutil/user/getent.go

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


You can drop the else and unindent the return

zyga · 2024-09-19T11:26:35Z

osutil/user/getent.go

+	value string
+}
+
+func (m *groupnameMatcher) index() int {


Nitpick, this can be passed by value

zyga · 2024-09-19T11:26:41Z

osutil/user/getent.go

+	return 0
+}
+
+func (m *groupnameMatcher) expectedValue() string {


zyga · 2024-09-19T11:27:49Z

osutil/user/getent.go

+	value string
+}
+
+func (m *usernameMatcher) index() int {


zyga · 2024-09-19T11:27:56Z

osutil/user/getent.go

+}
+
+func userMatchUsername(username string) userMatcher {
+	return &usernameMatcher{


zyga · 2024-09-19T11:28:05Z

osutil/user/getent.go

+	value int
+}
+
+func (m *uidMatcher) index() int {


zyga · 2024-09-19T11:28:21Z

osutil/user/getent.go

+}
+
+func userMatchUid(uid int) userMatcher {
+	return &uidMatcher{


zyga · 2024-09-19T11:29:21Z

osutil/user/user.go

+)
+
+type User = osuser.User
+type Group = osuser.Group


Nitpick: type ( ... ) perhaps?

bboozzoo

LGTM

bboozzoo · 2024-09-19T11:19:49Z

.golangci.yml

-  #   include-go-root: false
-  #   packages:
-  #     - github.com/davecgh/go-spew/spew
+  depguard:


thanks for this

bboozzoo · 2024-09-19T11:22:27Z

osutil/user/getent.go

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


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)

}

bboozzoo · 2024-09-19T11:32:57Z

osutil/user/user_from_snap.go

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


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

I would try something more specific, eg snapdusergo ?

I have renamed it.

…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 · 2024-09-30T09:55:16Z

Failures:
nested-ubuntu-24.04 |

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

pedronis

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?

pedronis · 2024-09-30T09:41:47Z

osutil/user/user_from_snap.go

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

pedronis · 2024-09-30T09:47:51Z

osutil/user/getent.go

+// 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.


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

pedronis · 2024-09-30T09:50:28Z

osutil/user/user.go

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


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

pedronis · 2024-09-30T09:52:48Z

osutil/user/getent.go

+func isKey(index int, expectedValue string) bool {
+	numeric := isNumeric(expectedValue)
+	return (index == 0 && !numeric) || (index == 2 && numeric)
+}


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

pedronis · 2024-09-30T09:53:45Z

osutil/user/getent.go

+// 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) {


it seems this could an interface instead:

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

pedronis · 2024-09-30T09:55:30Z

osutil/user/getent.go

+	}
+
+	if components == nil {
+		return nil, nil


test don't seem to hit here

ernestl · 2024-09-30T10:16:10Z

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

valentindavid mentioned this pull request Apr 2, 2024

snapcraft.yaml: build on 22.04 #13370

Merged

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

valentindavid force-pushed the valentindavid/user-no-nss branch from e52d6a4 to 7b51d5b Compare April 2, 2024 14:25

bboozzoo reviewed Apr 3, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/user-no-nss branch from 7b51d5b to 3bf27fb Compare May 31, 2024 14:41

valentindavid force-pushed the valentindavid/user-no-nss branch from 3bf27fb to 5713649 Compare June 24, 2024 10:35

valentindavid force-pushed the valentindavid/user-no-nss branch from 4a74f31 to 2ef7a9f Compare July 8, 2024 08:10

valentindavid added the Run nested The PR also runs tests inluded in nested suite label Jul 8, 2024

valentindavid closed this Jul 8, 2024

valentindavid reopened this Jul 8, 2024

valentindavid force-pushed the valentindavid/user-no-nss branch 3 times, most recently from 44bf5f1 to ecc0756 Compare July 10, 2024 11:53

valentindavid marked this pull request as ready for review July 11, 2024 13:40

valentindavid requested review from bboozzoo and zyga July 12, 2024 08:18

bboozzoo reviewed Jul 31, 2024

View reviewed changes

zyga requested changes Sep 9, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/user-no-nss branch 8 times, most recently from d4c8037 to 1bab881 Compare September 19, 2024 10:39

valentindavid requested a review from zyga September 19, 2024 11:11

valentindavid requested a review from bboozzoo September 19, 2024 11:11

ernestl added this to the 2.66 milestone Sep 19, 2024

zyga approved these changes Sep 19, 2024

View reviewed changes

bboozzoo approved these changes Sep 19, 2024

View reviewed changes

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

valentindavid added 2 commits September 19, 2024 14:32

.golangci.yml: verify for forbidden use of os/user

22f3403

valentindavid force-pushed the valentindavid/user-no-nss branch from 7bd1ffb to 22f3403 Compare September 19, 2024 12:32

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

pedronis reviewed Sep 30, 2024

View reviewed changes

valentindavid mentioned this pull request Sep 30, 2024

github: test snapdusergo tag #14550

Merged

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

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

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

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

Conversation

valentindavid commented Apr 2, 2024 • edited Loading

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

valentindavid commented Jun 24, 2024

codecov-commenter commented Jul 8, 2024

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a 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

ernestl commented Sep 19, 2024

zyga left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a 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

ernestl commented Sep 30, 2024

pedronis left a 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

ernestl commented Sep 30, 2024

valentindavid commented Apr 2, 2024 •

edited

Loading