pkg/idtools: refactor to avoid string-splitting

The package used a lot of string-formatting, followed by string-splitting.
This looked to originate from attempts to use templating to allow future
extensibility (9a3ab0358ecd657e3754677ff52250fd6cca4422).

Looking at the history of the package, only a single update was made to
these templates, 5 years go, which makes it unlikely that more templating
will be needed.

This patch simplifies the handling of arguments to use `[]string` instead
of a single `string` (and splitting to a `[]string`). This both simplifies
the code somewhat, and prevents user/group-names containing spaces to be
splitted (causing, e.g. `getent` to fail).

Note that user/group-names containing spaces are invalid (or at least
discouraged), there are situations where such names may be used, so we
should avoid breaking on such names.

Before this change, a user/group name with a space in its name would fail;

    dockerd --userns-remap="user:domain users"
    INFO[2020-08-19T10:26:59.288868661+02:00] Starting up
    Error during groupname lookup for "domain users": getent unable to find entry "domain" in group database

With this change:

    # Add some possibly problematic usernames for testing
    # need to do this manually, as `adduser` / `useradd` won't accept these names
    echo 'user name:x:1002:1002::/home/one:/bin/false' >> /etc/passwd; \
    echo 'user name:x:1002:' >> /etc/group; \
    echo 'user name:1266401166:65536' >> /etc/subuid; \
    echo 'user name:1266401153:65536' >> /etc/subgid; \
    echo 'user$HOME:x:1003:1003::/home/one:/bin/false' >> /etc/passwd; \
    echo 'user$HOME:x:1003:' >> /etc/group; \
    echo 'user$HOME:1266401166:65536' >> /etc/subuid; \
    echo 'user$HOME:1266401153:65536' >> /etc/subgid; \
    echo 'user'"'"'name:x:1004:1004::/home/one:/bin/false' >> /etc/passwd; \
    echo 'user'"'"'name:x:1004:' >> /etc/group; \
    echo 'user'"'"'name:1266401166:65536' >> /etc/subuid; \
    echo 'user'"'"'name:1266401153:65536' >> /etc/subgid; \
    echo 'user"name:x:1005:1005::/home/one:/bin/false' >> /etc/passwd; \
    echo 'user"name:x:1005:' >> /etc/group; \
    echo 'user"name:1266401166:65536' >> /etc/subuid; \
    echo 'user"name:1266401153:65536' >> /etc/subgid;

    # Start the daemon using those users
    dockerd --userns-remap="user name:user name"
    dockerd --userns-remap='user$HOME:user$HOME'
    dockerd --userns-remap="user'name":"user'name"
    dockerd --userns-remap='user"name':'user"name'

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/idtools/idtools_unix.go

@@ -9,7 +9,6 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
-	"strings"
 	"sync"
 	"syscall"
 
@@ -107,14 +106,14 @@ func accessible(isOwner, isGroup bool, perms os.FileMode) bool {
 
 // LookupUser uses traditional local system files lookup (from libcontainer/user) on a username,
 // followed by a call to `getent` for supporting host configured non-files passwd and group dbs
-func LookupUser(username string) (user.User, error) {
+func LookupUser(name string) (user.User, error) {
 	// first try a local system files lookup using existing capabilities
-	usr, err := user.LookupUser(username)
+	usr, err := user.LookupUser(name)
 	if err == nil {
 		return usr, nil
 	}
 	// local files lookup failed; attempt to call `getent` to query configured passwd dbs
-	usr, err = getentUser(fmt.Sprintf("%s %s", "passwd", username))
+	usr, err = getentUser(name)
 	if err != nil {
 		return user.User{}, err
 	}
@@ -130,11 +129,11 @@ func LookupUID(uid int) (user.User, error) {
 		return usr, nil
 	}
 	// local files lookup failed; attempt to call `getent` to query configured passwd dbs
-	return getentUser(fmt.Sprintf("%s %d", "passwd", uid))
+	return getentUser(strconv.Itoa(uid))
 }
 
-func getentUser(args string) (user.User, error) {
-	reader, err := callGetent(args)
+func getentUser(name string) (user.User, error) {
+	reader, err := callGetent("passwd", name)
 	if err != nil {
 		return user.User{}, err
 	}
@@ -143,21 +142,21 @@ func getentUser(args string) (user.User, error) {
 		return user.User{}, err
 	}
 	if len(users) == 0 {
-		return user.User{}, fmt.Errorf("getent failed to find passwd entry for %q", strings.Split(args, " ")[1])
+		return user.User{}, fmt.Errorf("getent failed to find passwd entry for %q", name)
 	}
 	return users[0], nil
 }
 
 // LookupGroup uses traditional local system files lookup (from libcontainer/user) on a group name,
 // followed by a call to `getent` for supporting host configured non-files passwd and group dbs
-func LookupGroup(groupname string) (user.Group, error) {
+func LookupGroup(name string) (user.Group, error) {
 	// first try a local system files lookup using existing capabilities
-	group, err := user.LookupGroup(groupname)
+	group, err := user.LookupGroup(name)
 	if err == nil {
 		return group, nil
 	}
 	// local files lookup failed; attempt to call `getent` to query configured group dbs
-	return getentGroup(fmt.Sprintf("%s %s", "group", groupname))
+	return getentGroup(name)
 }
 
 // LookupGID uses traditional local system files lookup (from libcontainer/user) on a group ID,
@@ -169,11 +168,11 @@ func LookupGID(gid int) (user.Group, error) {
 		return group, nil
 	}
 	// local files lookup failed; attempt to call `getent` to query configured group dbs
-	return getentGroup(fmt.Sprintf("%s %d", "group", gid))
+	return getentGroup(strconv.Itoa(gid))
 }
 
-func getentGroup(args string) (user.Group, error) {
-	reader, err := callGetent(args)
+func getentGroup(name string) (user.Group, error) {
+	reader, err := callGetent("group", name)
 	if err != nil {
 		return user.Group{}, err
 	}
@@ -182,18 +181,18 @@ func getentGroup(args string) (user.Group, error) {
 		return user.Group{}, err
 	}
 	if len(groups) == 0 {
-		return user.Group{}, fmt.Errorf("getent failed to find groups entry for %q", strings.Split(args, " ")[1])
+		return user.Group{}, fmt.Errorf("getent failed to find groups entry for %q", name)
 	}
 	return groups[0], nil
 }
 
-func callGetent(args string) (io.Reader, error) {
+func callGetent(database, key string) (io.Reader, error) {
 	entOnce.Do(func() { getentCmd, _ = resolveBinary("getent") })
 	// if no `getent` command on host, can't do anything else
 	if getentCmd == "" {
-		return nil, fmt.Errorf("")
+		return nil, fmt.Errorf("unable to find getent command")
 	}
-	out, err := execCmd(getentCmd, args)
+	out, err := execCmd(getentCmd, database, key)
 	if err != nil {
 		exitCode, errC := system.GetExitCode(err)
 		if errC != nil {
@@ -203,8 +202,7 @@ func callGetent(args string) (io.Reader, error) {
 		case 1:
 			return nil, fmt.Errorf("getent reported invalid parameters/database unknown")
 		case 2:
-			terms := strings.Split(args, " ")
-			return nil, fmt.Errorf("getent unable to find entry %q in %s database", terms[1], terms[0])
+			return nil, fmt.Errorf("getent unable to find entry %q in %s database", key, database)
 		case 3:
 			return nil, fmt.Errorf("getent database doesn't support enumeration")
 		default:
@@ -235,19 +233,19 @@ func lazyChown(p string, uid, gid int, stat *system.StatT) error {
 // NewIdentityMapping takes a requested username and
 // using the data from /etc/sub{uid,gid} ranges, creates the
 // proper uid and gid remapping ranges for that user/group pair
-func NewIdentityMapping(username string) (*IdentityMapping, error) {
-	usr, err := LookupUser(username)
+func NewIdentityMapping(name string) (*IdentityMapping, error) {
+	usr, err := LookupUser(name)
 	if err != nil {
-		return nil, fmt.Errorf("Could not get user for username %s: %v", username, err)
+		return nil, fmt.Errorf("Could not get user for username %s: %v", name, err)
 	}
 
 	uid := strconv.Itoa(usr.Uid)
 
-	subuidRangesWithUserName, err := parseSubuid(username)
+	subuidRangesWithUserName, err := parseSubuid(name)
 	if err != nil {
 		return nil, err
 	}
-	subgidRangesWithUserName, err := parseSubgid(username)
+	subgidRangesWithUserName, err := parseSubgid(name)
 	if err != nil {
 		return nil, err
 	}
@@ -265,10 +263,10 @@ func NewIdentityMapping(username string) (*IdentityMapping, error) {
 	subgidRanges := append(subgidRangesWithUserName, subgidRangesWithUID...)
 
 	if len(subuidRanges) == 0 {
-		return nil, errors.Errorf("no subuid ranges found for user %q", username)
+		return nil, errors.Errorf("no subuid ranges found for user %q", name)
 	}
 	if len(subgidRanges) == 0 {
-		return nil, errors.Errorf("no subgid ranges found for user %q", username)
+		return nil, errors.Errorf("no subgid ranges found for user %q", name)
 	}
 
 	return &IdentityMapping{

pkg/idtools/usergroupadd_linux.go

@@ -17,18 +17,13 @@ import (
 var (
 	once        sync.Once
 	userCommand string
-
-	cmdTemplates = map[string]string{
-		"adduser": "--system --shell /bin/false --no-create-home --disabled-login --disabled-password --group %s",
-		"useradd": "-r -s /bin/false %s",
-		"usermod": "-%s %d-%d %s",
-	}
-
 	idOutRegexp = regexp.MustCompile(`uid=([0-9]+).*gid=([0-9]+)`)
+)
+
+const (
 	// default length for a UID/GID subordinate range
 	defaultRangeLen   = 65536
 	defaultRangeStart = 100000
-	userMod           = "usermod"
 )
 
 // AddNamespaceRangesUser takes a username and uses the standard system
@@ -67,7 +62,7 @@ func AddNamespaceRangesUser(name string) (int, int, error) {
 	return uid, gid, nil
 }
 
-func addUser(userName string) error {
+func addUser(name string) error {
 	once.Do(func() {
 		// set up which commands are used for adding users/groups dependent on distro
 		if _, err := resolveBinary("adduser"); err == nil {
@@ -76,13 +71,18 @@ func addUser(userName string) error {
 			userCommand = "useradd"
 		}
 	})
-	if userCommand == "" {
-		return fmt.Errorf("Cannot add user; no useradd/adduser binary found")
+	var args []string
+	switch userCommand {
+	case "adduser":
+		args = []string{"--system", "--shell", "/bin/false", "--no-create-home", "--disabled-login", "--disabled-password", "--group", name}
+	case "useradd":
+		args = []string{"-r", "-s", "/bin/false", name}
+	default:
+		return fmt.Errorf("cannot add user; no useradd/adduser binary found")
 	}
-	args := fmt.Sprintf(cmdTemplates[userCommand], userName)
-	out, err := execCmd(userCommand, args)
-	if err != nil {
-		return fmt.Errorf("Failed to add user with error: %v; output: %q", err, string(out))
+
+	if out, err := execCmd(userCommand, args...); err != nil {
+		return fmt.Errorf("failed to add user with error: %v; output: %q", err, string(out))
 	}
 	return nil
 }
@@ -101,7 +101,7 @@ func createSubordinateRanges(name string) error {
 		if err != nil {
 			return fmt.Errorf("Can't find available subuid range: %v", err)
 		}
-		out, err := execCmd(userMod, fmt.Sprintf(cmdTemplates[userMod], "v", startID, startID+defaultRangeLen-1, name))
+		out, err := execCmd("usermod", "-v", fmt.Sprintf("%d-%d", startID, startID+defaultRangeLen-1), name)
 		if err != nil {
 			return fmt.Errorf("Unable to add subuid range to user: %q; output: %s, err: %v", name, out, err)
 		}
@@ -117,7 +117,7 @@ func createSubordinateRanges(name string) error {
 		if err != nil {
 			return fmt.Errorf("Can't find available subgid range: %v", err)
 		}
-		out, err := execCmd(userMod, fmt.Sprintf(cmdTemplates[userMod], "w", startID, startID+defaultRangeLen-1, name))
+		out, err := execCmd("usermod", "-w", fmt.Sprintf("%d-%d", startID, startID+defaultRangeLen-1), name)
 		if err != nil {
 			return fmt.Errorf("Unable to add subgid range to user: %q; output: %s, err: %v", name, out, err)
 		}

pkg/idtools/utils_unix.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"os/exec"
 	"path/filepath"
-	"strings"
 )
 
 func resolveBinary(binname string) (string, error) {
@@ -26,7 +25,7 @@ func resolveBinary(binname string) (string, error) {
 	return "", fmt.Errorf("Binary %q does not resolve to a binary of that name in $PATH (%q)", binname, resolvedPath)
 }
 
-func execCmd(cmd, args string) ([]byte, error) {
-	execCmd := exec.Command(cmd, strings.Split(args, " ")...)
+func execCmd(cmd string, arg ...string) ([]byte, error) {
+	execCmd := exec.Command(cmd, arg...)
 	return execCmd.CombinedOutput()
 }