Fix copy API (`docker cp`, etc) uid/gid handling #28923

erikh · 2016-11-29T07:38:34Z

- What I did

Adjusted the archiving UID/GID maps to not only consider root. Adjusted tests to appropriately measure permissions as well as the metadata it already carried to better enable testing with permissions crossing the border into/out of a container.

- How I did it

It's a pretty small patch. :D

- How to verify it

This aughta work:

docker create --name foo debian
touch bar && chown nobody:nogroup bar
docker cp bar foo:/bar

Before, these files would be root. After, they should reflect the UID of the files on the host.

- Description for the changelog

Fixed copy protocol and docker cp to appropriately transfer UID/GID to files copied into the container.

AkihiroSuda · 2016-11-29T07:45:49Z

For compatibility, probably it should be an option, e.g. docker cp --preserve-permission?
#21651 (comment)

cc @thaJeztah @estesp

AkihiroSuda · 2016-11-29T08:56:00Z

windowsRS1 failing with many "not supported by windows" errors

08:12:47 ----------------------------------------------------------------------
08:12:47 FAIL: docker_cli_cp_to_container_test.go:228: DockerSuite.TestCpToCaseA
08:12:47 
08:12:47 docker_cli_cp_to_container_test.go:236:
08:12:47     makeTestContentInDir(c, tmpDir)
08:12:47 docker_cli_cp_utils.go:107:
08:12:47     c.Assert(os.Chown(path, fd.uid, fd.gid), checker.IsNil)
08:12:47 ... value *os.PathError = &os.PathError{Op:"chown", Path:"d:\\CI\\CI-f2356d4\\test-cp-to-case-a949295019\\file1", Err:0x20000082} ("chown d:\\CI\\CI-f2356d4\\test-cp-to-case-a949295019\\file1: not supported by windows")
08:12:47 
08:12:51 
08:12:51 ----------------------------------------------------------------------

erikh · 2016-11-29T09:02:01Z

ah ok. what's the way to gate this test so it only runs on linux?

…

On Tue, Nov 29, 2016 at 12:56 AM, Akihiro Suda ***@***.***> wrote: windowsRS1 failing with many "not supported by windows" errors 08:12:47 ---------------------------------------------------------------------- 08:12:47 FAIL: docker_cli_cp_to_container_test.go:228: DockerSuite.TestCpToCaseA 08:12:47 08:12:47 docker_cli_cp_to_container_test.go:236: 08:12:47 makeTestContentInDir(c, tmpDir) 08:12:47 docker_cli_cp_utils.go:107: 08:12:47 c.Assert(os.Chown(path, fd.uid, fd.gid), checker.IsNil) 08:12:47 ... value *os.PathError = &os.PathError{Op:"chown", Path:"d:\\CI\\CI-f2356d4\\test-cp-to-case-a949295019\\file1", Err:0x20000082} ("chown d:\\CI\\CI-f2356d4\\test-cp-to-case-a949295019\\file1: not supported by windows") 08:12:47 08:12:51 08:12:51 ---------------------------------------------------------------------- — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#28923 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ63fYLdDR1_GMsPQJDgPu3j3K1CUKks5rC-jNgaJpZM4K-qDd> .

thaJeztah · 2016-11-29T09:07:49Z

@erikh if it requires a Linux daemon;

https://github.com/docker/docker/blob/master/integration-cli/docker_api_stats_test.go#L262

If it requires the daemon and client to be linux, probably https://github.com/docker/docker/blob/master/integration-cli/docker_api_stats_test.go#L166

However, there are also some test files that are only built/run on Linux/Unix (keep in mind that work is in progress on Solaris support); https://github.com/docker/docker/blob/master/integration-cli/docker_api_stats_unix_test.go#L1

erikh · 2016-11-29T16:18:40Z

Hmm. actually it looks like the chmod is failing in the test setup instead as well. I will revisit tonight.

…

On Tue, Nov 29, 2016 at 1:08 AM, Sebastiaan van Stijn < ***@***.***> wrote: @erikh <https://github.com/erikh> if it requires a Linux *daemon*; https://github.com/docker/docker/blob/master/integration-cli/docker_api_ stats_test.go#L262 If it requires the daemon and client to be linux, probably https://github.com/docker/docker/blob/master/integration-cli/docker_api_ stats_test.go#L166 However, there are also some test *files* that are only built/run on Linux/Unix (keep in mind that work is in progress on Solaris support); https://github.com/docker/docker/blob/master/integration-cli/docker_api_ stats_unix_test.go#L1 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28923 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ61g0_PN_oG_2UDvlWYDaN83SITuhks5rC-uUgaJpZM4K-qDd> .

erikh · 2016-12-05T08:53:57Z

I'm working on this now, sorry for the delay.

erikh · 2016-12-05T10:13:33Z

The cp tests are passing again on windows. PTAL?

thaJeztah · 2016-12-05T14:53:44Z

/cc @jlhawn @estesp

estesp · 2016-12-05T15:28:53Z

@thaJeztah do we have a way to force a userns CI run? I know on a recent PR it was mentioned that it wasn't working properly?

thaJeztah · 2016-12-05T16:37:11Z

hm, no doesn't seem to work indeed 😢 not sure

estesp · 2016-12-08T18:52:59Z

FWIW, I ran the full suite myself against a user namespace-enabled daemon and all tests passed, but most importantly:

PASS: docker_cli_cp_to_container_test.go:602: DockerSuite.TestCpToHostWithPermissions   24.939s

Taking 25sec to run the test seems a bit long given the steps don't seem intensive, but it is passing.

Personally I'm fine with design; not sure if this needs more design review or just code review and confirmation on functionality of preserving ownership (which probably was the way it was working before user namespaces were merged in 1.9 experimental)

tonistiigi · 2016-12-08T21:48:21Z

That is a documented behavior in https://github.com/docker/docker/blob/master/docs/reference/commandline/cp.md so I'm not sure we can just flip it.

Even if it makes more sense we should probably keep the cli behavior and change the API only.

cc @jlhawn

jlhawn · 2016-12-08T22:54:18Z

That is a documented behavior so I'm not sure we can just flip it.

Ownership is set to the user and primary group at the destination. For example, files copied to a container are created with UID:GID of the root user. Files copied to the local machine are created with the UID:GID of the user which invoked the docker cp command.

Even if it makes more sense we should probably keep the cli behavior and change the API only.

👍

I'm not sure what the project's official process and policy is if we were to change the default behavior, but I think it would be a good idea to do this (preserve permissions) since the stated goal of docker container cp ... is to behave like the utility cp -a which does preserve ownership. Here's the relevant section from the manual for the BSD cp utility:

     -p    Cause cp to preserve the following attributes of each source file
           in the copy: modification time, access time, file flags, file mode,
           user ID, and group ID, as allowed by permissions.  Access Control
           Lists (ACLs) and Extended Attributes (EAs), including resource
           forks, will also be preserved.

           If the user ID and group ID cannot be preserved, no error message
           is displayed and the exit value is not altered.

           If the source file has its set-user-ID bit on and the user ID can-
           not be preserved, the set-user-ID bit is not preserved in the
           copy's permissions.  If the source file has its set-group-ID bit on
           and the group ID cannot be preserved, the set-group-ID bit is not
           preserved in the copy's permissions.  If the source file has both
           its set-user-ID and set-group-ID bits on, and either the user ID or
           group ID cannot be preserved, neither the set-user-ID nor set-
           group-ID bits are preserved in the copy's permissions.

If we do not want to change the current behavior, we could provide a --preserve-ownership, -p flag to the CLI subcommand which defaults to false.

Also, last year, when we added support for extracting an archive into a container, I considered adding a --chown uid[:gid] option but it was too much extra work at the time. This option would be used to specify that you want to recursively set user and group ownership of the copied content to the given uid and gid. You might also argue that chown could be its own separate command ... then you get into debating whether we should have an entire remote filesystem API and we probably don't want to go there.

thaJeztah · 2016-12-08T23:31:59Z

Adding -p / --preserve-ownership doesn't sound too bad; at least it guarantees a non-breaking change

erikh · 2016-12-10T19:31:22Z

This is the less surprising behavior to me (documented or not, broken is broken). That said, I will do whatever you guys want. Just give me exact details please.

…

On Thu, Dec 8, 2016 at 3:32 PM, Sebastiaan van Stijn < ***@***.***> wrote: Adding -p / --preserve-ownership doesn't sound too bad; at least it guarantees a non-breaking change — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28923 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ64YNvH3jzhc9k7B51fSNAtjzd3v7ks5rGJOXgaJpZM4K-qDd> .

thaJeztah · 2016-12-12T16:14:56Z

This is the less surprising behavior to me (documented or not, broken is broken).

Not saying I disagree, but changing behavior like this has bitten us on multiple occasions, because people start to rely on it (even if it's broken). If we do decide to change the default, we should consider adding a flag / option to restore the current behavior.

erikh · 2016-12-12T16:40:15Z

Oh definitely re: flag to restore behavior. I completely agree. Once you folks decide on something, I will change the PR. Happy to do it either scenario, just expressing my opinion on the subject.

…

On Mon, Dec 12, 2016 at 8:15 AM, Sebastiaan van Stijn < ***@***.***> wrote: This is the less surprising behavior to me (documented or not, broken is broken). Not saying I disagree, but changing behavior like this has bitten us on multiple occasions, because people start to rely on it (even if it's broken). If we *do* decide to change the default, we should consider adding a flag / option to restore the current behavior. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28923 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6--NZjYpMbII1Pn1Bqt5KyULpJAaks5rHXMsgaJpZM4K-qDd> .

Kronos1776 · 2016-12-14T21:48:29Z

Consider the linux command: chmod --reference={reference_file} {target_file}
This is an established behavior in linux.
The same works on chown and chcon.
The default behavior when a file is created is to use the current user/group/rwx/tag.
This matches the current 'docker cp' behavior very well and with the addition of parameter(s) the behavior can be modified... possibly setting values based on a reference already inside the volume using a flag like --reference={container_file_reference}... and, do we even dare, use a flag like --chown user:group or --chmod 755 on the docker cp? These are very familiar to most of us and would require no additional learning curve.
The permissions issue may eventually grow to encompass all of the permission attributes. ;-)

thaJeztah · 2016-12-15T19:30:59Z

We were comparing behavior with a regular cp;

cp source target uses permissions of target if target exists, and uses permissions of the current user if the target file doesn't exist
cp -a source target uses permissions of source if target exists, and also uses permissions of the current user if the target file doesn't exist

Suggested;

use an --inherit-ownership flag (but open to suggestions)
if target does not exist, give the file ownership of the container user (instead of root)
if target file does exist, give the file ownership of the source file.
add another flag (suggestions as well) to force files maintaining ownership of the source files

/cc @cpuguy83

erikh · 2016-12-15T19:35:43Z

interesting. I can patch this in but it seems surprising (at least, I didn't realize it merged permissions) from a behavior standpoint. Is there any justification out there I can read? Mostly just curious. what do you want to do about permission bits (if anything)? preserve host? Not sure it's relevant, but might be a good time to discuss that while we're here.

…

On Thu, Dec 15, 2016 at 11:31 AM, Sebastiaan van Stijn < ***@***.***> wrote: We were comparing behavior with a regular cp; - cp source target uses permissions of target if target exists, and uses permissions of the current user if the target file doesn't exist - cp -a source target uses permissions of source if target exists, and *also* uses permissions of the current user if the target file doesn't exist Suggested; - use an --inherit-ownership flag (but open to suggestions) - if target does *not* exist, give the file ownership of the container user (instead of root) - if target file *does* exist, give the file ownership of the source file. - add another flag (suggestions as well) to force files maintaining ownership of the source files /cc @cpuguy83 <https://github.com/cpuguy83> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28923 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6ypvtPetRuBWEb1ZvROgYDVcolp7ks5rIZWogaJpZM4K-qDd> .

cpuguy83 · 2016-12-15T20:34:09Z

if target file does exist, give the file ownership of the source file.

Wouldn't the new file inherit ownership from the target file?

Is there any justification out there I can read? Mostly just curious.

Open source fire hose.
Probably something like "we want to add this!", "ok, sounds awesome!", "woohoo docker cp is now a thing!"

erikh · 2016-12-15T20:37:02Z

Maybe I'm misunderstanding something; are those `/bin/cp` (aka POSIX) rules or something else? I guess I should just look this up myself haha

…

On Thu, Dec 15, 2016 at 12:34 PM, Brian Goff ***@***.***> wrote: if target file does exist, give the file ownership of the source file. Wouldn't the new file inherit ownership from the target file? Is there any justification out there I can read? Mostly just curious. Open source fire hose. Probably something like "we want to add this!", "ok, sounds awesome!", "woohoo docker cp is now a thing!" — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28923 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ64z0qmBLo3oK2n1nXVqBx3Dh-I0-ks5rIaRugaJpZM4K-qDd> .

erikh · 2016-12-15T20:38:39Z

This is quite clear on what should be done, at least. I'll save the thread the rest. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html

…

On Thu, Dec 15, 2016 at 12:36 PM, Erik Hollensbe ***@***.***> wrote: Maybe I'm misunderstanding something; are those `/bin/cp` (aka POSIX) rules or something else? I guess I should just look this up myself haha On Thu, Dec 15, 2016 at 12:34 PM, Brian Goff ***@***.***> wrote: > if target file does exist, give the file ownership of the source file. > > Wouldn't the new file inherit ownership from the target file? > > Is there any justification out there I can read? Mostly just curious. > > Open source fire hose. > Probably something like "we want to add this!", "ok, sounds awesome!", > "woohoo docker cp is now a thing!" > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#28923 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AABJ64z0qmBLo3oK2n1nXVqBx3Dh-I0-ks5rIaRugaJpZM4K-qDd> > . >

erikh · 2017-04-09T19:44:38Z

no idea; if you have any suggestions or clues that'd be great.

anusha-ragunathan · 2017-04-11T21:54:58Z

@erikh : Use debian:jessie instead of debian in https://github.com/erikh/docker/blob/a52ae4b6491afe07c7a09786ca11c224112ec040/integration-cli/docker_cli_cp_to_container_unix_test.go#L27, so that CI uses the frozen images and not pulling from Hub, which is what was causing the extra time.

anusha-ragunathan · 2017-04-11T21:55:27Z

I get PASS: docker_cli_cp_to_container_unix_test.go:17: DockerSuite.TestCpToContainerWithPermissions 0.943s once this is done.

anusha-ragunathan · 2017-04-11T22:28:35Z

That's why the test fails on powerpc and s390 as well. Download frozen images on both arch shows debian:jessie is available.

ppc64le/debian:jessie@sha256:412845f51b6ab662afba71bc7a716e20fdb9b84f185d180d4c7504f8a75c4f91

s390x/debian:jessie@sha256:b74c863400909eff3c5e196cac9bfd1f6333ce47aae6a38398d87d5875da170a

erikh · 2017-04-11T23:57:22Z

ah ok! Will update tonight. Thanks.

…

On Tue, Apr 11, 2017 at 3:29 PM Anusha Ragunathan ***@***.***> wrote: That's probably why the test fails on powerpc and s390 as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28923 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6y73mw68u1gWjMEf9NPr0eM1B_Fnks5ru_7GgaJpZM4K-qDd> .

This changes the long-standing bug of copy operations not preserving the UID/GID information after the files arrive to the container. Signed-off-by: Erik Hollensbe <github@hollensbe.org>

erikh · 2017-04-12T10:34:43Z

Tests are running now, I'll validate later today.

anusha-ragunathan · 2017-04-12T15:01:06Z

LGTM

estesp

LGTM

estesp · 2017-04-12T15:22:16Z

@thaJeztah are there any follow-on doc changes needed for the new -a flag? I know it will automatically get into the help output of docker cp, but wasn't sure if there are other docs that need an update.

anusha-ragunathan · 2017-04-12T15:22:56Z

Changelog needs update and I've added the label so that it gets tracked.

Fix copy API (`docker cp`, etc) uid/gid handling

javabrett · 2017-12-05T19:24:15Z

I'm been doing some work on #34096 and wondered if (other than what is in the updated docs for docker cp -a) whether there is any more detail on the intended behaviour. Something like:

docker cp without -a: behaviour should be unchanged - files are copied to the container as root:root ownership.
- need to check what happens when running with --user
- need to check impact of --userns
docker cp -a should copy verbatim the uid:gid of the local file into the container. The remote uid:gid might not exist.
- any special handling for --user?
- any special handling for --userns?

GordonTheTurtle added the status/0-triage label Nov 29, 2016

AkihiroSuda added status/1-design-review and removed status/0-triage labels Nov 29, 2016

AkihiroSuda added the status/failing-ci Indicates that the PR in its current state fails the test suite label Nov 29, 2016

estesp removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 5, 2016

thaJeztah added the rebuild/userns label Dec 5, 2016

erikh mentioned this pull request Dec 10, 2016

copy should respect user and with_user statements box-builder/box#5

Open

GordonTheTurtle removed the rebuild/z label Apr 7, 2017

daemon/archive.go: Fix copy routines to preserve UID.

8a7ff5f

This changes the long-standing bug of copy operations not preserving the UID/GID information after the files arrive to the container. Signed-off-by: Erik Hollensbe <github@hollensbe.org>

estesp approved these changes Apr 12, 2017

View reviewed changes

anusha-ragunathan merged commit d40a17f into moby:master Apr 12, 2017

anusha-ragunathan added the impact/changelog label Apr 12, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28923 from erikh/fix-copy

384e8e3

Fix copy API (`docker cp`, etc) uid/gid handling

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28923 from erikh/fix-copy

5084e4d

Fix copy API (`docker cp`, etc) uid/gid handling

thaJeztah added this to the 17.06.0 milestone May 11, 2017

thaJeztah added docs/revisit impact/documentation labels May 11, 2017

thaJeztah added the impact/cli label Jun 6, 2017

boaz0 mentioned this pull request Jun 6, 2017

Update the cp command docs according to commit d40a17ffc2 #33548

Closed

markshust mentioned this pull request Jun 28, 2017

Copying /var/www/html from appdata volume fails markshust/docker-magento#50

Closed

This was referenced Jul 13, 2017

docker cp behavior changed #34096

Open

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

Open

javabrett mentioned this pull request Dec 25, 2017

Fix ownership-regression in docker cp to container #35874

Closed

thaJeztah mentioned this pull request Aug 3, 2018

Add bash completion for cp --archive docker/cli#1269

Merged

ZYecho mentioned this pull request May 29, 2019

fix: fix lack of copyUIDGID in swagger.yaml #39279

Merged

thaJeztah mentioned this pull request Jun 7, 2019

[19.03 backport] fix: fix lack of copyUIDGID in swagger.yaml docker-archive/engine#270

Merged

corhere mentioned this pull request Mar 8, 2022

Reduce usage of runc/libcontainer/user package #43351

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix copy API (`docker cp`, etc) uid/gid handling #28923

Fix copy API (`docker cp`, etc) uid/gid handling #28923

erikh commented Nov 29, 2016

AkihiroSuda commented Nov 29, 2016

AkihiroSuda commented Nov 29, 2016

erikh commented Nov 29, 2016 via email

thaJeztah commented Nov 29, 2016

erikh commented Nov 29, 2016 via email

erikh commented Dec 5, 2016

erikh commented Dec 5, 2016

thaJeztah commented Dec 5, 2016

estesp commented Dec 5, 2016

thaJeztah commented Dec 5, 2016

estesp commented Dec 8, 2016

tonistiigi commented Dec 8, 2016

jlhawn commented Dec 8, 2016

thaJeztah commented Dec 8, 2016

erikh commented Dec 10, 2016 via email

thaJeztah commented Dec 12, 2016

erikh commented Dec 12, 2016 via email

Kronos1776 commented Dec 14, 2016 •

edited

Loading

thaJeztah commented Dec 15, 2016

erikh commented Dec 15, 2016 via email

cpuguy83 commented Dec 15, 2016

erikh commented Dec 15, 2016 via email

erikh commented Dec 15, 2016 via email

erikh commented Apr 9, 2017

anusha-ragunathan commented Apr 11, 2017

anusha-ragunathan commented Apr 11, 2017

anusha-ragunathan commented Apr 11, 2017 •

edited

Loading

erikh commented Apr 11, 2017 via email

erikh commented Apr 12, 2017

anusha-ragunathan commented Apr 12, 2017

estesp left a comment

estesp commented Apr 12, 2017

anusha-ragunathan commented Apr 12, 2017

javabrett commented Dec 5, 2017 •

edited

Loading

Fix copy API (docker cp, etc) uid/gid handling #28923

Fix copy API (docker cp, etc) uid/gid handling #28923

Conversation

erikh commented Nov 29, 2016

AkihiroSuda commented Nov 29, 2016

AkihiroSuda commented Nov 29, 2016

erikh commented Nov 29, 2016 via email

thaJeztah commented Nov 29, 2016

erikh commented Nov 29, 2016 via email

erikh commented Dec 5, 2016

erikh commented Dec 5, 2016

thaJeztah commented Dec 5, 2016

estesp commented Dec 5, 2016

thaJeztah commented Dec 5, 2016

estesp commented Dec 8, 2016

tonistiigi commented Dec 8, 2016

jlhawn commented Dec 8, 2016

thaJeztah commented Dec 8, 2016

erikh commented Dec 10, 2016 via email

thaJeztah commented Dec 12, 2016

erikh commented Dec 12, 2016 via email

Kronos1776 commented Dec 14, 2016 • edited Loading

thaJeztah commented Dec 15, 2016

erikh commented Dec 15, 2016 via email

cpuguy83 commented Dec 15, 2016

erikh commented Dec 15, 2016 via email

erikh commented Dec 15, 2016 via email

erikh commented Apr 9, 2017

anusha-ragunathan commented Apr 11, 2017

anusha-ragunathan commented Apr 11, 2017

anusha-ragunathan commented Apr 11, 2017 • edited Loading

erikh commented Apr 11, 2017 via email

erikh commented Apr 12, 2017

anusha-ragunathan commented Apr 12, 2017

estesp left a comment

Choose a reason for hiding this comment

estesp commented Apr 12, 2017

anusha-ragunathan commented Apr 12, 2017

javabrett commented Dec 5, 2017 • edited Loading

Fix copy API (`docker cp`, etc) uid/gid handling #28923

Fix copy API (`docker cp`, etc) uid/gid handling #28923

Kronos1776 commented Dec 14, 2016 •

edited

Loading

anusha-ragunathan commented Apr 11, 2017 •

edited

Loading

javabrett commented Dec 5, 2017 •

edited

Loading