Skip to content

Fix userns-remap option when username & UID match#42004

Merged
thaJeztah merged 1 commit into
moby:masterfrom
Rid:42003-fix-userns-uid-username-match
Feb 11, 2021
Merged

Fix userns-remap option when username & UID match#42004
thaJeztah merged 1 commit into
moby:masterfrom
Rid:42003-fix-userns-uid-username-match

Conversation

@Rid

@Rid Rid commented Feb 9, 2021

Copy link
Copy Markdown
Contributor

Signed-off-by: Grant Millar [email protected]

- What I did
Fixed #42003 by only matching UID or username.

- How I did it
Check subuidRangesWithUID first then subuidRangesWithUserName.

- How to verify it

  1. echo "10426:x:10426:10426::/nonexistent:/bin/false" >> /etc/passwd
  2. echo "10426:x:10426:" >> /etc/group
  3. echo "10426:10426000:10000" >> /etc/subuid
  4. echo "10426:10426000:10000" >> /etc/subgid
  5. dockerd --userns-remap=10426 &
  6. docker run hello-world

- Description for the changelog

Fix userns-remap option when username & UID match

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

Comment thread pkg/idtools/idtools_unix.go Outdated
Comment on lines 271 to 275

@thaJeztah thaJeztah Feb 9, 2021

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.

Actually wondering why we're looking up both; perhaps instead we should return early (try lookup sub-uids by UID -> if error, or no results -> lookup by Name -> repeat for sub-gids).

That said; ISTR there was some discussion about wether "user-name" or "user-id" takes precedence (user-name should probably not be able to take precedence (i.e., a user named 0 should not be able to "trump" root), but linux disagreed on that (some discussion on #21436, opencontainers/runc#695, and opencontainers/runc@0a5293f (opencontainers/runc#708))

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.

Did a quick try at what it could look like (feedback welcome) earlier;

// 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(name string) (*IdentityMapping, error) {
	usr, err := LookupUser(name)
	if err != nil {
		return nil, fmt.Errorf("Could not get user for username %s: %v", name, err)
	}

	subuidRanges, err := lookupSubUIDRanges(usr)
	if err != nil {
		return nil, err
	}
	subgidRanges, err := lookupSubGIDRanges(usr)
	if err != nil {
		return nil, err
	}

	return &IdentityMapping{
		uids: subuidRanges,
		gids: subgidRanges,
	}, nil
}

func lookupSubUIDRanges(usr user.User) ([]IDMap, error) {
	r, err := parseSubuid(strconv.Itoa(usr.Uid))
	if err != nil {
		return nil, err
	}
	if len(r) == 0 {
		r, err = parseSubuid(usr.Name)
		if err != nil {
			return nil, err
		}
	}
	if len(r) == 0 {
		return nil, errors.Errorf("no subuid ranges found for user %q", usr.Name)
	}
	return createIDMap(r), nil
}

func lookupSubGIDRanges(usr user.User) ([]IDMap, error) {
	r, err := parseSubgid(strconv.Itoa(usr.Uid))
	if err != nil {
		return nil, err
	}
	if len(r) == 0 {
		r, err = parseSubgid(usr.Name)
		if err != nil {
			return nil, err
		}
	}
	if len(r) == 0 {
		return nil, errors.Errorf("no subgid ranges found for user %q", usr.Name)
	}
	return createIDMap(r), nil
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense for UIDs to take precedence, as 99% of the time a numerical value is going to be a UID rather than a username.

I was thinking originally to implement separate functions, I wasn't sure if it was too much boilerplate but semantically it looks better to me.

Do you want me to update the commit with your implementation?

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.

Yes, there's definitely some boilerplating; I initially kept it inline, and, well, it was still boilerplating, so I extracted it to separate functions.

Definitely feel free to copy my code (check if I didn't make a mistake, as it was a quick write up 😅)

@Rid

Rid commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

@thaJeztah Updated, PTAL.

@Rid

Rid commented Feb 11, 2021

Copy link
Copy Markdown
Contributor Author

Ping @thaJeztah, are we able to merge this and get it into the next release? I hate to push but we really need this to upgrade our production environment. 😇

@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

@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.

@tiborvass mentioned we should have a test for this.

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.

Using userns-remap option when username & UID are the same causes containers to not run since 20.10.0

4 participants