Proposal: Implement --user flag for COPY and ADD

[kostickm]

Proposal: Implement --user and --group flag for COPY and ADD

This is a docs-first proposal for the implementation portion of the proposal approved in #9934.

Background

Numerous Issues have stressed the problem of allowing USER to be set, but then files added via COPY or ADD by default are set to a UID and GID of 0, often causing unexpected problems. A framework was implemented via PR #10775 to allow user options/flags to be added to Dockerfile commands. This proposal aims to add two flags for the COPY and ADD commands to allow the user to specify the ownership of the files/directories copied/added using those commands. Additional discussion and approval was handled via proposal #9934.

Primary goals

• Implement the use of the --user=<uid_or_username> and
  --group=<gid_or_groupname> flags for COPY and ADD, leveraging the
  framework implemented in PR Add support for Dockerfile CMD options #10775.
• These flags will allow the user to specify the ownership permissions for the
  files copied/added using the COPY and ADD commands.
• Maintain backwards compatibility (flags are optional, will continue the
  default behavior if not specified)
• Update documentation to reflect this new implementation.

Proposed change

Add configuration flags to COPY and ADD:

COPY --user=<uid_or_username> <src>... <dest>
ADD --user=<uid_or_username> <src> ... <dest>

COPY --group=<gid_or_groupname> <src>... <dest>
ADD --group=<gid_or_groupname> <src> ... <dest>

COPY --user=<uid_or_username> --group=<gid_or_groupname> <src>... <dest>
ADD --user=<uid_or_username> --group=<gid_or_groupname> <src> ... <dest>

All files and directories copied to the container will be owned by the
specified <uid_or_username> and/or <gid_or_groupname>. Use of the --user and
--group flags will only affect the current COPY or ADD command.

The configuration is optional. If the --user and/or --group flag are not
provided, files and directories are added in the default manner with UID and
GID of 0. (A check will be performed for the flag, if not present the
original/default manner is followed).

docker/libcontainer has existing code for providing the permission bits for a
given user/group. This existing function will be leveraged to determine the
correct permission bits to set on the newly copied file within the container.

Implementation

1. Create Dockerfile COPY/ADD flags (--user=<uid_or_username> and --group=<gid_or_groupname>)
   in builder/dispatchers.go to be used for both COPY and ADD commands
   (func dispatchCopy and func add)
2. Use CMD framework from PR Add support for Dockerfile CMD options #10775 to parse for the --user=<uid_or_username>
   and/or --group=<gid_or_groupname> in both COPY and ADD commands
3. Pass the specified user and/or group as additional parameters to
   runContextCommand(...), or nil if not present
4. In builder/internals.go update func runContextCommand to take the
   additional variables
5. In builder/internals.go update func addContext to take and check for the
   user and group variable
   • if nil continue in default manner (set Uid and Gid to 0)
   • else call func GetExecUserPath from libcontainer/user/user.go to return
     the ExecUser
   • if user does not exist throw error, and do not continue on to Step 6
6. If the ExecUser is returned, pass the Uid and Gid to copyAsDirectory(...),
   fixFilePermissions(...), etc., else if continuing in the default manner,
   pass 0 for Uid and Gid.

Known objections

• Must be backwards compatible (that is why the flag is optional)
• Should not be tied to the USER command

Process

• The description of this proposal is also included as part of the PR. To
  comment on the description, create inline comments, this makes it easier to
  discuss things.
• Please focus on the --user and --group flag implementation since this
  general idea has been discussed and approved via PR Proposal: Specify user/ownership for COPY. #9934.

Signed-off-by: Megan Kostick mkostick@us.ibm.com

[thaJeztah]

Actually, a uid (numeric) doesn't have to exist.
(not sure what happens when an unknown username is used)

[thaJeztah]

Thanks @kostickm!

I'm interested to hear from @jhowardmsft and @ahmetalpbalkan what their thoughts are on this for future implementation on Windows

/cc @duglin @cyphar @erikh

[lowenna]

I would prefer separate -user= and -group=. Of course, none of this will be supported by Windows initially :)

[thaJeztah]

@jhowardmsft thanks for looking, I understand it won't be in (yet) but trying to avoid having to change in a later stage if it gets implemented

[cyphar]

For the builder-side changes, you can take inspiration from my original patchset to this end: #9046.

[kostickm]

@thaJeztah @jhowardmsft @cyphar Thank you all for your feedback, suggestions, and tips so far! I think the --group= flag would be good to include as well. I'll update the current commit to include that change in the documentation, and see how it looks.

[kostickm]

Updated to include the --group= flag portion. Not sure I like how the docs are laid out at the moment, so may need some reformatting there.

[kostickm]

ping @crosbymichael @jfrazelle @tiborvass

[icecrime]

Design LGTM. Do you mind if we close this PR while you work on the implementation? However, please note that we will soon be starting an effort to pull out the builder client-side, and that we will prioritize it over this PR (which may in turn have to be rebased and submitted against another repo). Does it work for you? Thanks!

[thaJeztah]

Apologies upfront for going off-topic here. I'm a bit -1 on closing proposals that are basically "accepted"; they're easily forgotten, harder to find, and might give the wrong impression (is this still coming?).

Can we come up with some clear status labels for these (e.g. "on hold"), possibly adding that to the title as well, and a message in the top description explaining why?

[pier-oliviert]

I'm sure I'm going against the grain here, but since Dockerfiles directive are sequentials, I think the default should be to use the current user instead of roots.

My view and again, I might be wrong, is that it should honor the last USER instruction by default. Providing a --root would then copy everything with UID=0.

Of course, this bring backward incompatibility to anything that was copied and assumed to be UID 0. However, the flag is obvious and setting it doesn't have any other side-effect than retrieving the previous behavior.

I also think that in the long run, it would be more beneficial to specify root compared to always settting the user (Imagine a complex Dockerfile with more than 1 user...)

[kostickm]

I like @thaJeztah 's idea of having some sort of "on hold" flag or title change. If it is best to wait for the build client-side code to be pulled out first before proceeding, I'd like to keep this open, but in a "on hold" state just so folks know it's still in the works. @icecrime what do you think?

[cyphar]

@pothibo Yes, that is the most logical method. However, it breaks backwards compatibility very badly. I tried to get some patches merged to that effect several months ago and they were rejected for that reason. #9046.

[simonvanderveldt]

I might be a bit late to the party, but I missed the discussion in #7537 and #9934 and would like to voice my concerns.

Every new release of Docker we end up with more and more instructions/commands or even worse flags (especially --flag-xyz ones) which are really doing the usability of Docker a major disservice.
Things should be easy and the principle of least astonishment should be the guiding principle here.
On an average day in #docker there are already a huge amount of questions from users about the simplest of things. Think about those users and how they should ever grasp what all those options and flags that sound the same exactly do.

This topic, the way the USER instruction in combination with the ADD or COPY instruction works in a Dockerfile, is a prime example of this. It's clearly in violation of the principle of least astonishment, as many people have noticed and as the multiple issues filed about it show.
ADD and COPY themselves are great examples of this as well.

The main argument I've seen against properly fixing these issues is backwards compatibility. A somewhat strange term to use for software of which every person who seriously uses it knows damn well that it's all very new and that it will (and should!) change in the future.

We're barely 2 years past the initial public release and we're already limiting ourself to our past mistakes and working around those in all kinds of silly ways.

I'd like to ask all of you to please reconsider this. There is an experimental build now which can be used to identify all breaking changes when used in a users current setup and there are ways to feedback to users that certain functionality will change in the future.
If it's really out of the question the very least we should do is determine a roadmap to 2.0 where these issues definitely should be addressed.

Anyway, that's just my 2c, not trying to be an ass, just want Docker to be the best it can be.

[thaJeztah]

Thanks for raising your concerns, @simonvanderveldt the fact you're writing this up shows your not an ass 🐴. Making backwards incompatible changes to the Dockerfile behavior is a big concern though, because this could (for example) break automated builds on Docker Hub, and there is no versioning in the Dockerfile format (#4907) to prevent this.

Although I disagree that adding per-instruction flags are bad (they can offer more fine grained control), I agree that in this particular case, it's a trade-off (and I think all maintainers agree).

You might want to watch #14298, which describes moving the builder out of the daemon and making it client side, which would allow for much more flexibility. There's now also a roadmap (https://github.com/docker/docker/blob/master/ROADMAP.md), which may still be incomplete, but hopefully makes future plans a bit more clear.

[LK4D4]

@thaJeztah no, not all maintainers agree :)

[thaJeztah]

@LK4D4 apologies, I'll edit 👍

[cyphar]

And some non-maintainers don't agree too. :)

[jessfraz]

closing due to https://github.com/docker/docker/blob/master/ROADMAP.md#22-dockerfile-syntax

[thom4parisot]

@jfrazelle is there any plan anyway to address user switch and permissions? So far, without changing the syntax, and the following:

USER app

COPY /everything /app

CMD ["/app/bin/blah/", "--assets-out", "/app/public"]

I know about two scenarios if the CMD writes into /app/public:

1. adding a RUN chown -R app /app/public
2. having COPY to respect the USER statement

Do you think of any other alternatives? Or near future possibilities? The chmod works fine but for some reason, I have the feeling I am scalping a kitten while doing that – because of this extra layer/workaround.

[jessfraz]

yes as the linnk says when the builder is seperated out the maintainers of
it can then do whatever they want so addinng this functionality can be one
of the things

On Wed, Jul 8, 2015 at 3:16 AM, Thomas Parisot notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle is there any plan anyway to
address user switch and permissions? So far, without changing the syntax,
and the following:

USER app

COPY /everything /app

CMD ["/app/bin/blah/", "--assets-out", "/app/public"]

I know about two scenarios if the CMD writes into /app/public:

1. adding a RUN chown -R app /app/public
2. having COPY to respect the USER statement

Do you think of any other alternatives? Or near future possibilities? The
chmod works fine but for some reason, I have the feeling I am scalping a
kitten while doing that – because of this extra layer/workaround.

—
Reply to this email directly or view it on GitHub
#13600 (comment).

[thom4parisot]

Cool, great news @jfrazelle :-)

[adambiggs]

Any updates on this?

[SeaOfTea]

This is such a pain. Actively trying to avoid using chown in its own RUN statement as it will double the virtual size of the image depending on the files contained within.

[ushuz]

New command HEALTHCHECK and SHELL were added and Dockerfile syntax is no longer a frozen feature now, what about this one? Can we reopen it?

0725045

[N-McA]

This is still painful.

[thaJeztah]

We discussed this in the maintainers meeting, and we're ok with reopening this PR, to start the discussion again

[thaJeztah]

ping @kostickm are you still available to work on this?

[kostickm]

Not currently. @duglin is working on finding someone to take it over.

[thaJeztah]

Thanks @kostickm!

[duglin]

@thaJeztah I think we might be able to find someone to work on it, but what's the position of the maintainers during the call today w.r.t. what the UX should look like?

You know how I feel :-)
I'd like to see:
COPY [--user=...] [--group=...] src target
with the option of eventually supporting:
COPY [--user=...] [--group=...] [--decompress] src target to cover the case of a compress archive that people want automatically expanded so that we can deprecate ADD and remove the magic of it.

[thaJeztah]

I brought this topic up again and asked if we wanted to change the default (i.e. COPY after USER would use USER), but that was still considered too risky, so a flag was acceptable.

I'd keep the --decompress for a separate PR, to not (possibly) block this one. Having

COPY [--user=...] [--group=...] src target sounds good to me, possibly we could think of something that allows "inheriting" from USER (not sure about naming / syntax, so open to suggestions)

[duglin]

Yea I didn't want to include --decompress in the same PR, but wanted to mention it as justification for why I think using options is the better approach than adding "yet another cmd that's almost the same as another one (or two)".

As for COPY/ADD inheriting USER if it was right before it, that's just scary to me - and more magic than we should consider.

[ardnaxelarak]

I can pick this up.

[tianon]

Do you think it's worthwhile to make sure this new --user has the same syntax/functionality of the existing --user (on docker run/docker create)? What's the use case for using --group without also setting the user?

[duglin]

@tianon yea, I think we should be consistent with docker run on this - unless someone can explain why we need to allow for --group w/o --user.

[thaJeztah]

There was some discussion recently (i.e. why use a user:group "micro format" for this) but I agree that being consistent with docker run here makes sense

[thaJeztah]

Closing this PR as it's now being worked on in #27303