Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion user/go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/moby/sys/user

go 1.18
go 1.24

require golang.org/x/sys v0.1.0

Expand Down
80 changes: 75 additions & 5 deletions user/idtools.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,21 @@ package user
import (
"fmt"
"os"
"path/filepath"
)

// FS is the filesystem contract used by the Mkdir*AndChownFS helpers.
type FS interface {
Stat(name string) (os.FileInfo, error)
Mkdir(name string, perm os.FileMode) error
MkdirAll(name string, perm os.FileMode) error
Chmod(name string, mode os.FileMode) error
Chown(name string, uid, gid int) error
}

var (
_ FS = &os.Root{}
_ FS = &hostFS{}
)

// MkdirOpt is a type for options to pass to Mkdir calls
Expand All @@ -23,12 +38,24 @@ func WithOnlyNew(o *mkdirOptions) {
// function will still change ownership and permissions. If WithOnlyNew is passed as an
// option, then only the newly created directories will have ownership and permissions changed.
func MkdirAllAndChown(path string, mode os.FileMode, uid, gid int, opts ...MkdirOpt) error {
Comment on lines 39 to 40
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if we want users of this module to migrate to the new functions, we can add a //go:fix inline, then go fix will automatically migrate them 🎉

Suggested change
// option, then only the newly created directories will have ownership and permissions changed.
func MkdirAllAndChown(path string, mode os.FileMode, uid, gid int, opts ...MkdirOpt) error {
// option, then only the newly created directories will have ownership and permissions changed.
//
//go:fix inline
func MkdirAllAndChown(path string, mode os.FileMode, uid, gid int, opts ...MkdirOpt) error {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In this case (if you want everyone to migrate) it also makes sense to mark old ones as deprecated.

OTOH if you want the new functions signatures to be simpler as you're suggesting above, the go:fix won't be possible (or easy).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it also makes sense to mark old ones as deprecated.

Yup, deprecating is an option for sure; didn't want to make that a strict requiremnt for this PR (good as a follow-up, also for visibility).

OTOH if you want the new functions signatures to be simpler as you're suggesting above,

I'd be fine with keeping the signature the same, but at least wanted to mention the option, because now we have the chance to do so.

Specifically in these cases where Golang tries to abstract away platform differences, and on Windows making (e.g.) Chown and Chmod (for permissions) a no-op, silently discarding options that the user may have expected to secure things.

Arguments that are required should probably still be positional though (which is the fun thing here if those are required on Linux, but won't take effect on Windows I guess); happy for input on that though!

the go:fix won't be possible (or easy).

Yeah, //go:fix is not a MUST, but can help. It can do a pretty decent job; erm, but of course when I was about to write an example, I found yet another corner-case 😂 - adding it to my list of reports; golang/go#78432

var options mkdirOptions
for _, opt := range opts {
opt(&options)
return MkdirAllAndChownFS(nil, path, mode, uid, gid, opts...)
}

// MkdirAllAndChownFS creates a directory (including any along the path) on the
// provided filesystem and then modifies ownership to the requested uid/gid. If
// fsys is nil, the host filesystem is used.
func MkdirAllAndChownFS(fsys FS, path string, mode os.FileMode, uid, gid int, opts ...MkdirOpt) error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking at the signature; I see the variadic options at the end; did we add those later (but kept the positional options because they already were there?)

If that's the case, and we want to make more of these "optional", then now is the opportunity to change the signature for the new functions we added.

(also looking at windows <--> linux w.r.t. "uid", "gid" etc)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Given that Chown is in the function name, I don't think uid/gid should be optional.

if fsys == nil {
absPath, err := filepath.Abs(path)
if err != nil {
return err
}
fsys = &hostFS{}
path = absPath
}

return mkdirAs(path, mode, uid, gid, true, options.onlyNew)
options := mkdirOpts(opts)
return mkdirAs(fsys, path, mode, uid, gid, true, options.onlyNew)
}

// MkdirAndChown creates a directory and then modifies ownership to the requested uid/gid.
Expand All @@ -38,11 +65,32 @@ func MkdirAllAndChown(path string, mode os.FileMode, uid, gid int, opts ...Mkdir
// Note that unlike os.Mkdir(), this function does not return IsExist error
// in case path already exists.
func MkdirAndChown(path string, mode os.FileMode, uid, gid int, opts ...MkdirOpt) error {
Comment on lines 66 to 67
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same for this one;

Suggested change
// in case path already exists.
func MkdirAndChown(path string, mode os.FileMode, uid, gid int, opts ...MkdirOpt) error {
// in case path already exists.
//
//go:fix inline
func MkdirAndChown(path string, mode os.FileMode, uid, gid int, opts ...MkdirOpt) error {

return MkdirAndChownFS(nil, path, mode, uid, gid, opts...)
}

// MkdirAndChownFS creates a directory on the provided filesystem and then
// modifies ownership to the requested uid/gid. If fsys is nil, the host
// filesystem is used.
func MkdirAndChownFS(fsys FS, path string, mode os.FileMode, uid, gid int, opts ...MkdirOpt) error {
if fsys == nil {
absPath, err := filepath.Abs(path)
if err != nil {
return err
}
fsys = &hostFS{}
path = absPath
}

options := mkdirOpts(opts)
return mkdirAs(fsys, path, mode, uid, gid, false, options.onlyNew)
}

func mkdirOpts(opts []MkdirOpt) mkdirOptions {
var options mkdirOptions
for _, opt := range opts {
opt(&options)
}
return mkdirAs(path, mode, uid, gid, false, options.onlyNew)
return options
}

// getRootUIDGID retrieves the remapped root uid/gid pair from the set of maps.
Expand Down Expand Up @@ -139,3 +187,25 @@ func (i IdentityMapping) ToContainer(uid, gid int) (int, int, error) {
func (i IdentityMapping) Empty() bool {
return len(i.UIDMaps) == 0 && len(i.GIDMaps) == 0
}

type hostFS struct{}

func (*hostFS) Stat(name string) (os.FileInfo, error) {
return os.Stat(name)
}

func (*hostFS) Mkdir(name string, perm os.FileMode) error {
return os.Mkdir(name, perm)
}

func (*hostFS) MkdirAll(name string, perm os.FileMode) error {
return os.MkdirAll(name, perm)
}

func (*hostFS) Chmod(name string, mode os.FileMode) error {
return os.Chmod(name, mode)
}

func (*hostFS) Chown(name string, uid, gid int) error {
return os.Chown(name, uid, gid)
}
30 changes: 13 additions & 17 deletions user/idtools_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,15 @@ package user

import (
"fmt"
"io/fs"
"os"
"path/filepath"
"strconv"
"syscall"
)

func mkdirAs(path string, mode os.FileMode, uid, gid int, mkAll, onlyNew bool) error {
path, err := filepath.Abs(path)
if err != nil {
return err
}

stat, err := os.Stat(path)
func mkdirAs(fsys FS, path string, mode os.FileMode, uid, gid int, mkAll, onlyNew bool) error {
stat, err := fsys.Stat(path)
if err == nil {
if !stat.IsDir() {
return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR}
Expand All @@ -26,7 +22,7 @@ func mkdirAs(path string, mode os.FileMode, uid, gid int, mkAll, onlyNew bool) e
}

// short-circuit -- we were called with an existing directory and chown was requested
return setPermissions(path, mode, uid, gid, stat)
return setPermissions(fsys, path, mode, uid, gid, stat)
}

// make an array containing the original path asked for, plus (for mkAll == true)
Expand All @@ -44,23 +40,23 @@ func mkdirAs(path string, mode os.FileMode, uid, gid int, mkAll, onlyNew bool) e
dirPath := path
for {
dirPath = filepath.Dir(dirPath)
if dirPath == "/" {
if dirPath == string(filepath.Separator) || dirPath == "." {
break
}
if _, err = os.Stat(dirPath); os.IsNotExist(err) {
if _, err = fsys.Stat(dirPath); os.IsNotExist(err) {
paths = append(paths, dirPath)
}
}
if err = os.MkdirAll(path, mode); err != nil {
if err = fsys.MkdirAll(path, mode); err != nil {
return err
}
} else if err = os.Mkdir(path, mode); err != nil {
} else if err = fsys.Mkdir(path, mode); err != nil {
return err
}
// even if it existed, we will chown the requested path + any subpaths that
// didn't exist when we called MkdirAll
for _, pathComponent := range paths {
if err = setPermissions(pathComponent, mode, uid, gid, nil); err != nil {
if err = setPermissions(fsys, pathComponent, mode, uid, gid, nil); err != nil {
return err
}
}
Expand All @@ -71,24 +67,24 @@ func mkdirAs(path string, mode os.FileMode, uid, gid int, mkAll, onlyNew bool) e
// Normally a Chown is a no-op if uid/gid match, but in some cases this can still cause an error, e.g. if the
// dir is on an NFS share, so don't call chown unless we absolutely must.
// Likewise for setting permissions.
func setPermissions(p string, mode os.FileMode, uid, gid int, stat os.FileInfo) error {
func setPermissions(fsys FS, p string, mode os.FileMode, uid, gid int, stat fs.FileInfo) error {
if stat == nil {
var err error
stat, err = os.Stat(p)
stat, err = fsys.Stat(p)
if err != nil {
return err
}
}
if stat.Mode().Perm() != mode.Perm() {
if err := os.Chmod(p, mode.Perm()); err != nil {
if err := fsys.Chmod(p, mode.Perm()); err != nil {
return err
}
}
ssi := stat.Sys().(*syscall.Stat_t)
if ssi.Uid == uint32(uid) && ssi.Gid == uint32(gid) {
return nil
}
return os.Chown(p, uid, gid)
return fsys.Chown(p, uid, gid)
}

// LoadIdentityMapping takes a requested username and
Expand Down
54 changes: 50 additions & 4 deletions user/idtools_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package user
import (
"errors"
"fmt"
"maps"
"os"
"path/filepath"
"testing"
Expand Down Expand Up @@ -292,9 +293,7 @@ func readTree(base, root string) (map[string]node, error) {
if err != nil {
return nil, err
}
for path, nodeinfo := range subtree {
tree[path] = nodeinfo
}
maps.Copy(tree, subtree)
}
}
return tree, nil
Expand Down Expand Up @@ -385,12 +384,59 @@ func TestMkdirIsNotDir(t *testing.T) {
t.Fatalf("Couldn't create temp dir: %v", err)
}

err = mkdirAs(file.Name(), 0o755, 0, 0, false, false)
fsys := FS(&hostFS{})

err = mkdirAs(fsys, file.Name(), 0o755, 0, 0, false, false)
if expected := "mkdir " + file.Name() + ": not a directory"; err.Error() != expected {
t.Fatalf("expected error: %v, got: %v", expected, err)
}
}

func TestMkdirAllAndChownFSNilUsesHostFS(t *testing.T) {
requiresRoot(t)

baseDir := t.TempDir()
path := filepath.Join(baseDir, "usr", "share")

if err := MkdirAllAndChownFS(nil, path, 0o755, 99, 99); err != nil {
t.Fatal(err)
}

s := &unix.Stat_t{}
if err := unix.Stat(path, s); err != nil {
t.Fatal(err)
}
if s.Uid != 99 || s.Gid != 99 {
t.Fatalf("expected ownership 99:99, got %d:%d", s.Uid, s.Gid)
}
}

func TestMkdirAllAndChownFSWithRoot(t *testing.T) {
requiresRoot(t)

baseDir := t.TempDir()
root, err := os.OpenRoot(baseDir)
if err != nil {
t.Fatal(err)
}
defer root.Close()

if err := MkdirAllAndChownFS(root, "usr/share", 0o755, 123, 124); err != nil {
t.Fatal(err)
}

verifyTree, err := readTree(baseDir, "")
if err != nil {
t.Fatal(err)
}

expected := map[string]node{
"usr": {123, 124},
"usr/share": {123, 124},
}
compareTrees(t, expected, verifyTree)
}

func requiresRoot(t *testing.T) {
if os.Getuid() != 0 {
t.Skip("skipping test that requires root")
Expand Down
4 changes: 2 additions & 2 deletions user/idtools_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ import (
// permissions aren't set through this path, the identity isn't utilized.
// Ownership is handled elsewhere, but in the future could be support here
// too.
func mkdirAs(path string, _ os.FileMode, _, _ int, _, _ bool) error {
return os.MkdirAll(path, 0)
func mkdirAs(fsys FS, path string, _ os.FileMode, _, _ int, _, _ bool) error {
return fsys.MkdirAll(path, 0)
}
8 changes: 2 additions & 6 deletions user/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"io"
"os"
"slices"
"strconv"
"strings"
)
Expand Down Expand Up @@ -373,12 +374,7 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
// If the group argument isn't explicit, we'll just search for it.
if groupArg == "" {
// Check if user is a member of this group.
for _, u := range g.List {
if u == matchedUserName {
return true
}
}
return false
return slices.Contains(g.List, matchedUserName)
}

if gidErr == nil {
Expand Down
Loading