Skip to content

Commit

Permalink
client: protect user lookups with global lock (#14742)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
shoenig committed Sep 29, 2022
1 parent 0eb7119 commit e4e5bc5
Show file tree
Hide file tree
Showing 8 changed files with 104 additions and 15 deletions.
3 changes: 3 additions & 0 deletions .changelog/14742.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
client: re-enable nss-based user lookups
```
2 changes: 1 addition & 1 deletion GNUmakefile
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 5 additions & 7 deletions client/allocdir/fs_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"strconv"
"syscall"

"github.com/hashicorp/nomad/helper/users"
"golang.org/x/sys/unix"
)

Expand All @@ -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)

Expand All @@ -39,17 +40,14 @@ func dropDirPermissions(path string, desired os.FileMode) error {
return nil
}

u, err := user.Lookup("nobody")
if err != nil {
return err
}
nobody := users.Nobody()

uid, err := getUid(u)
uid, err := getUid(&nobody)
if err != nil {
return err
}

gid, err := getGid(u)
gid, err := getGid(&nobody)
if err != nil {
return err
}
Expand Down
6 changes: 3 additions & 3 deletions client/allocrunner/taskrunner/template/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"
"io"
"os"
"os/user"
"path/filepath"
"reflect"
"regexp"
Expand All @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
4 changes: 2 additions & 2 deletions command/agent/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"net"
"os"
"os/exec"
"os/user"
"path/filepath"
"runtime"
"sort"
Expand All @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions drivers/shared/executor/executor_universal_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package executor
import (
"fmt"
"os/exec"
"os/user"
"path/filepath"
"strconv"
"strings"
Expand All @@ -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"
Expand All @@ -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)
}
Expand Down
53 changes: 53 additions & 0 deletions helper/users/lookup.go
Original file line number Diff line number Diff line change
@@ -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()
}
35 changes: 35 additions & 0 deletions helper/users/lookup_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
}

0 comments on commit e4e5bc5

Please sign in to comment.