Skip to content

pkg/idtools: refactor to avoid string-splitting#41377

Merged
cpuguy83 merged 1 commit into
moby:masterfrom
thaJeztah:refactor_idtools
Aug 20, 2020
Merged

pkg/idtools: refactor to avoid string-splitting#41377
cpuguy83 merged 1 commit into
moby:masterfrom
thaJeztah:refactor_idtools

Conversation

@thaJeztah

@thaJeztah thaJeztah commented Aug 20, 2020

Copy link
Copy Markdown
Member

fixes #41372

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 (9a3ab03 / #12648).

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'

- What I did

- How I did it

- How to verify it

- Description for the changelog

Fix handling of looking up user- and group-names with spaces

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah

Copy link
Copy Markdown
Member Author

@AkihiroSuda @kolyshkin @estesp PTAL

Comment thread pkg/idtools/utils_unix.go Outdated
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 (9a3ab03).

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 <[email protected]>

@estesp estesp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 left a comment

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.

LGTM

// 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")

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.

😱

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.

LOL, yes, I guess it had to return "an error", but empty error string felt a bit too minimal 😹

@cpuguy83
cpuguy83 merged commit 3902057 into moby:master Aug 20, 2020
@thaJeztah
thaJeztah deleted the refactor_idtools branch August 20, 2020 19:50
@thaJeztah thaJeztah added this to the 20.03.0 milestone Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

userns-remap fails with username/groupname with space (getent command)

3 participants