Fix parsing of user/group during copy operation by thaJeztah · Pull Request #34143 · moby/moby

thaJeztah · 2017-07-17T17:22:31Z

relates to

Change uid/gid on 'docker cp' and preserve it on 'docker cp -a' #34099
fixes docker cp looks up user on the host instead of in container #34142
fixes Adding GID to Dockerfile breaks Docker compose watch functionality #49213
fixes [BUG] Watch can't copy host file to container when user: "1000:1000" (UID:GID format) was used docker/compose#12467
relates to Fix copy API (docker cp, etc) uid/gid handling #28923

If a container was started with

a numeric uid
both a user and group (username:groupname)
uid and gid (uid:gid)

The copy action failed, because the "username:groupname"
was looked up using getent.

This patch;

splits user and group before looking up
if numeric uid or gid is used and lookup fails,
the uid / gid is used as-is

AkihiroSuda · 2017-07-18T07:33:22Z

/etc/passwd -> getent(1)?

ah, yes, perhaps I should change that; I started writing for this PR, but found

moby/daemon/daemon_unix.go

Lines 941 to 951 in c8a2596

// Parse the remapped root (user namespace) option, which can be one of:

// username - valid username from /etc/passwd

// username:groupname - valid username; valid groupname from /etc/group

// uid - 32-bit unsigned int valid Linux UID value

// uid:gid - uid value; 32-bit unsigned int Linux GID value

//

// If no groupname is specified, and a username is specified, an attempt

// will be made to lookup a gid for that username as a groupname

//

// If names are used, they are verified to exist in passwd/group

func parseRemappedRoot(usergrp string) (string, string, error) {

, so copied parts from that

I'm not sure what approach should be taken though if the user needs to come from within the container (where getent may not be available, such as in a busybox container) - that's why I kept it as "WIP"

updated 👍

aaronlehmann · 2017-08-01T03:34:12Z

@AkihiroSuda, does this look good now?

AkihiroSuda · 2017-08-01T04:23:33Z

getent - > typically getent? (Sorry for going back and forth)

AkihiroSuda · 2017-08-01T04:23:50Z

Is this still WIP?

thaJeztah · 2017-08-01T08:22:18Z

let me remove "WIP", code itself should be generally ok (not sure how to test this, without the test becoming dependent on the host)

I think I made this WIP because it only resolves the parsing issue, and not the fact that this looks up the user/group from the host (#34142).

getent - > typically getent? (Sorry for going back and forth)

Oh, can you be more specific; happy to update but wasn't sure what you meant / where you wanted to have that updated 😄

thaJeztah · 2017-08-01T08:23:33Z

oh, need to remove this one as that's part of #34099, so don't merge as-is

runcom · 2017-09-18T16:36:11Z

any update on this?

olljanat · 2018-12-21T18:43:42Z

@thaJeztah this one needs to be rebased

thaJeztah · 2018-12-24T16:23:12Z

Rebased @tonistiigi @AkihiroSuda PTAL

codecov · 2019-03-08T19:06:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@54dddad). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #34143   +/-   ##
=========================================
  Coverage          ?   36.52%           
=========================================
  Files             ?      608           
  Lines             ?    45060           
  Branches          ?        0           
=========================================
  Hits              ?    16459           
  Misses            ?    26321           
  Partials          ?     2280

thaJeztah · 2025-01-05T11:15:15Z

+
+	usr, err := lookupUser(ctr, userNameOrID)
+	if err != nil {
+		return 0, 0, errors.Wrapf(err, "failed to look up user %q in container: %w", userNameOrID)


Ah, forgot to remove the %w when I swapped to errors.Wrapf;

daemon/archive_tarcopyoptions_unix.go:64:16: github.com/docker/docker/vendor/github.com/pkg/errors.Wrapf format %w reads arg #2, but call has 1 arg daemon/archive_tarcopyoptions_unix.go:73:16: github.com/docker/docker/vendor/github.com/pkg/errors.Wrapf format %w reads arg #2, but call has 1 arg

thaJeztah · 2025-01-15T16:06:34Z

+	if uid, err := strconv.ParseUint(nameOrID, 10, 32); err == nil && uid <= math.MaxInt32 {
+		// uid provided
+		return int(uid), ""
+	}


Wondering if this is too strict, and if we should just use something like

moby/vendor/github.com/moby/sys/user/lookup_unix.go

Lines 137 to 139 in 079b6e6

filter := func(entry SubID) bool {

return entry.Name == u.Name || entry.Name == strconv.Itoa(u.Uid)

}

thaJeztah · 2025-01-15T16:07:56Z

+	if userName == "" && hasGroup && groupName == "" {
+		// UID/GID passed; nothing to look up.
+		return userID, groupID, nil
+	}
+
+	usr, err := lookupUser(ctr, userNameOrID)
+	if err != nil {
+		return 0, 0, errors.Wrapf(err, "failed to look up user %q in container", userNameOrID)
+	}


That said.. with the logic in this PR, we'd be able to skip lookup if a numeric UID/GID is passed; that means we're also able to work with a container that doesn't have /etc/passwd / etc/group.

If a container was started with - a numeric uid - both a user and group (username:groupname) - uid and gid (uid:gid) The copy action failed, because the "username:groupname" was looked up using getent. This patch; - splits `user` and `group` before looking up - if numeric `uid` or `gid` is used and lookup fails, the `uid` / `gid` is used as-is The code also looked up the user and group on the host instead of in the container when using getent; this patch fixes the lookup to only use the container's /etc/passwd and /etc/group instead. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Integration tests were added

thaJeztah · 2025-01-20T17:10:46Z

Let me bring this one in; thx!

GordonTheTurtle added the status/0-triage label Jul 17, 2017

thaJeztah mentioned this pull request Jul 17, 2017

Change uid/gid on 'docker cp' and preserve it on 'docker cp -a' #34099

Open

AkihiroSuda reviewed Jul 18, 2017

View reviewed changes

thaJeztah force-pushed the fix-copy-permissions branch from bd967ec to 1996def Compare July 18, 2017 09:43

GordonTheTurtle assigned aaronlehmann Aug 1, 2017

thaJeztah added status/2-code-review and removed status/0-triage labels Aug 1, 2017

thaJeztah changed the title ~~[WIP] Fix parsing of user/group during copy operation~~ Fix parsing of user/group during copy operation Aug 1, 2017

thaJeztah commented Aug 1, 2017

View reviewed changes

thaJeztah force-pushed the fix-copy-permissions branch from 1996def to e4a5d45 Compare August 1, 2017 08:26

thaJeztah force-pushed the fix-copy-permissions branch from e4a5d45 to cbc2bbc Compare June 12, 2018 01:05

derek Bot added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 22, 2018

thaJeztah force-pushed the fix-copy-permissions branch from cbc2bbc to 1aa8d22 Compare December 24, 2018 16:21

thaJeztah added rebuild/z and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels Dec 24, 2018

GordonTheTurtle removed the rebuild/z label Dec 24, 2018

tonistiigi approved these changes Jan 2, 2019

View reviewed changes

derek Bot added the rebuild/* label Jan 2, 2019

GordonTheTurtle removed the rebuild/* label Jan 2, 2019

derek Bot added the rebuild/powerpc label Jan 2, 2019

GordonTheTurtle removed the rebuild/powerpc label Jan 2, 2019

thaJeztah force-pushed the fix-copy-permissions branch from 1aa8d22 to dabb29a Compare March 8, 2019 19:06

github-advanced-security AI found potential problems Jan 3, 2025

View reviewed changes

Comment thread daemon/archive_tarcopyoptions_unix.go Fixed

Comment thread daemon/archive_tarcopyoptions_unix.go Fixed

Comment thread daemon/archive_tarcopyoptions_unix.go Fixed

thaJeztah force-pushed the fix-copy-permissions branch 2 times, most recently from 2c69e04 to bab4e16 Compare January 4, 2025 01:08

github-advanced-security AI found potential problems Jan 4, 2025

View reviewed changes

Comment thread daemon/archive_tarcopyoptions_unix.go Fixed

thaJeztah force-pushed the fix-copy-permissions branch from bab4e16 to 5bd409e Compare January 4, 2025 01:15

thaJeztah commented Jan 5, 2025

View reviewed changes

thaJeztah force-pushed the fix-copy-permissions branch from 5bd409e to 29b709c Compare January 6, 2025 08:29

thaJeztah commented Jan 15, 2025

View reviewed changes

thompson-shaun added this to the 29.0.0 milestone Jan 16, 2025

vvoland approved these changes Jan 16, 2025

View reviewed changes

Comment thread daemon/archive_tarcopyoptions_unix.go Outdated

thaJeztah force-pushed the fix-copy-permissions branch from 29b709c to 63675e9 Compare January 16, 2025 20:36

This comment was marked as resolved.

Sign in to view

thaJeztah modified the milestones: 29.0.0, 28.0.0 Jan 17, 2025

thaJeztah force-pushed the fix-copy-permissions branch from 63675e9 to 046c40e Compare January 17, 2025 18:26

thaJeztah assigned thaJeztah and unassigned aaronlehmann Jan 17, 2025

thaJeztah force-pushed the fix-copy-permissions branch 4 times, most recently from 1eff567 to ee025b2 Compare January 18, 2025 00:34

thaJeztah force-pushed the fix-copy-permissions branch from ee025b2 to f658ea3 Compare January 20, 2025 11:45

vvoland approved these changes Jan 20, 2025

View reviewed changes

thaJeztah merged commit 500ece5 into moby:master Jan 20, 2025

thaJeztah deleted the fix-copy-permissions branch January 20, 2025 17:10

thaJeztah mentioned this pull request Oct 20, 2025

[BUG] crashes while parsing when using docker compose secrets and USER in Dockerfile docker/compose#13296

Closed

	// Parse the remapped root (user namespace) option, which can be one of:
	// username - valid username from /etc/passwd
	// username:groupname - valid username; valid groupname from /etc/group
	// uid - 32-bit unsigned int valid Linux UID value
	// uid:gid - uid value; 32-bit unsigned int Linux GID value
	//
	// If no groupname is specified, and a username is specified, an attempt
	// will be made to lookup a gid for that username as a groupname
	//
	// If names are used, they are verified to exist in passwd/group
	func parseRemappedRoot(usergrp string) (string, string, error) {

	filter := func(entry SubID) bool {
	return entry.Name == u.Name \|\| entry.Name == strconv.Itoa(u.Uid)
	}

Conversation

thaJeztah commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Aug 1, 2017

Uh oh!

AkihiroSuda commented Aug 1, 2017

Uh oh!

AkihiroSuda commented Aug 1, 2017

Uh oh!

thaJeztah commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Sep 18, 2017

Uh oh!

olljanat commented Dec 21, 2018

Uh oh!

thaJeztah commented Dec 24, 2018

Uh oh!

codecov Bot commented Mar 8, 2019

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

thaJeztah commented Jan 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

thaJeztah commented Jul 17, 2017 •

edited

Loading