client: protect user lookups with global lock (#14742)

* client: protect user lookups with global lock

This PR updates Nomad client to always do user lookups while holding
a global process lock. This is to prevent concurrency unsafe implementations
of NSS, but still enabling NSS lookups of users (i.e. cannot not use osusergo).

* cl: add cl

.changelog/14742.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+client: re-enable nss-based user lookups
+```

GNUmakefile

@@ -21,7 +21,7 @@ ifndef BIN
 BIN := $(GOPATH)/bin
 endif
 
-GO_TAGS := osusergo $(GO_TAGS)
+GO_TAGS := $(GO_TAGS)
 
 ifeq ($(CI),true)
 GO_TAGS := codegen_generated $(GO_TAGS)

client/allocdir/fs_unix.go

@@ -10,6 +10,7 @@ import (
 	"strconv"
 	"syscall"
 
+	"github.com/hashicorp/nomad/helper/users"
 	"golang.org/x/sys/unix"
 )
 
@@ -18,7 +19,7 @@ var (
 	// directory shared across tasks in a task group.
 	SharedAllocContainerPath = filepath.Join("/", SharedAllocName)
 
-	// TaskLocalContainer is the path inside a container for mounted directory
+	// TaskLocalContainerPath is the path inside a container for mounted directory
 	// for local storage.
 	TaskLocalContainerPath = filepath.Join("/", TaskLocal)
 
@@ -39,17 +40,14 @@ func dropDirPermissions(path string, desired os.FileMode) error {
 		return nil
 	}
 
-	u, err := user.Lookup("nobody")
+	nobody := users.Nobody()
+
+	uid, err := getUid(&nobody)
 	if err != nil {
 		return err
 	}
 
-	uid, err := getUid(u)
+	gid, err := getGid(&nobody)
-	if err != nil {
-		return err
-	}
-
-	gid, err := getGid(u)
 	if err != nil {
 		return err
 	}

client/allocrunner/taskrunner/template/template_test.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"os/user"
 	"path/filepath"
 	"reflect"
 	"regexp"
@@ -28,6 +27,7 @@ import (
 	clienttestutil "github.com/hashicorp/nomad/client/testutil"
 	"github.com/hashicorp/nomad/helper/pointer"
 	"github.com/hashicorp/nomad/helper/testlog"
+	"github.com/hashicorp/nomad/helper/users"
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
@@ -2391,10 +2391,10 @@ func TestTaskTemplateManager_writeToFile(t *testing.T) {
 
 	ci.Parallel(t)
 
-	cu, err := user.Current()
+	cu, err := users.Current()
 	require.NoError(t, err)
 
-	cg, err := user.LookupGroupId(cu.Gid)
+	cg, err := users.LookupGroupId(cu.Gid)
 	require.NoError(t, err)
 
 	file := "my.tmpl"

command/agent/config.go

@@ -8,7 +8,6 @@ import (
 	"net"
 	"os"
 	"os/exec"
-	"os/user"
 	"path/filepath"
 	"runtime"
 	"sort"
@@ -23,6 +22,7 @@ import (
 	"github.com/hashicorp/nomad/client/fingerprint"
 	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/helper/pointer"
+	"github.com/hashicorp/nomad/helper/users"
 	"github.com/hashicorp/nomad/nomad"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/nomad/structs/config"
@@ -1082,7 +1082,7 @@ func newDevModeConfig(devMode, connectMode bool) (*devModeConfig, error) {
 			// come up and fail unexpectedly to run jobs
 			return nil, fmt.Errorf("-dev-connect is only supported on linux.")
 		}
-		u, err := user.Current()
+		u, err := users.Current()
 		if err != nil {
 			return nil, fmt.Errorf(
 				"-dev-connect uses network namespaces and is only supported for root: %v", err)

drivers/shared/executor/executor_universal_linux.go

@@ -3,7 +3,6 @@ package executor
 import (
 	"fmt"
 	"os/exec"
-	"os/user"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -13,6 +12,7 @@ import (
 	"github.com/hashicorp/nomad/client/lib/cgutil"
 	"github.com/hashicorp/nomad/client/lib/resources"
 	"github.com/hashicorp/nomad/client/taskenv"
+	"github.com/hashicorp/nomad/helper/users"
 	"github.com/hashicorp/nomad/plugins/drivers"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runc/libcontainer/specconv"
@@ -21,7 +21,7 @@ import (
 // setCmdUser takes a user id as a string and looks up the user, and sets the command
 // to execute as that user.
 func setCmdUser(cmd *exec.Cmd, userid string) error {
-	u, err := user.Lookup(userid)
+	u, err := users.Lookup(userid)
 	if err != nil {
 		return fmt.Errorf("failed to identify user %v: %v", userid, err)
 	}

helper/users/lookup.go

@@ -0,0 +1,53 @@
+package users
+
+import (
+	"fmt"
+	"os/user"
+	"sync"
+)
+
+// lock is used to serialize all user lookup at the process level, because
+// some NSS implementations are not concurrency safe
+var lock *sync.Mutex
+
+// nobody is a cached copy of the nobody user, which is going to be looked-up
+// frequently and is unlikely to be modified on the underlying system.
+var nobody user.User
+
+// Nobody returns User data for the "nobody" user on the system, bypassing the
+// locking / file read / NSS lookup.
+func Nobody() user.User {
+	// original is immutable via copy by value
+	return nobody
+}
+
+func init() {
+	lock = new(sync.Mutex)
+	u, err := Lookup("nobody")
+	if err != nil {
+		panic(fmt.Sprintf("unable to lookup the nobody user: %v", err))
+	}
+	nobody = *u
+}
+
+// Lookup username while holding a global process lock.
+func Lookup(username string) (*user.User, error) {
+	lock.Lock()
+	defer lock.Unlock()
+	return user.Lookup(username)
+}
+
+// LookupGroupId while holding a global process lock.
+func LookupGroupId(gid string) (*user.Group, error) {
+	lock.Lock()
+	defer lock.Unlock()
+	return user.LookupGroupId(gid)
+}
+
+// Current returns the current user, acquired while holding a global process
+// lock.
+func Current() (*user.User, error) {
+	lock.Lock()
+	defer lock.Unlock()
+	return user.Current()
+}

helper/users/lookup_test.go

@@ -0,0 +1,35 @@
+//go:build linux
+
+package users
+
+import (
+	"errors"
+	"os/user"
+	"testing"
+
+	"github.com/shoenig/test/must"
+)
+
+func TestLookup(t *testing.T) {
+	cases := []struct {
+		username string
+
+		expErr  error
+		expUser *user.User
+	}{
+		{username: "nobody", expUser: &user.User{Username: "nobody", Uid: "65534", Gid: "65534", Name: "nobody", HomeDir: "/nonexistent"}}, // ubuntu
+		{username: "root", expUser: &user.User{Username: "root", Uid: "0", Gid: "0", Name: "root", HomeDir: "/root"}},
+		{username: "doesnotexist", expErr: errors.New("user: unknown user doesnotexist")},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.username, func(t *testing.T) {
+			u, err := Lookup(tc.username)
+			if tc.expErr != nil {
+				must.EqError(t, tc.expErr, err.Error())
+			} else {
+				must.Eq(t, tc.expUser, u)
+			}
+		})
+	}
+}