builder: make ADD and COPY obey the USER directive

[cyphar]

Add some sanity to the Dockerfile builder by making ADD and COPY obey
the USER directive (so the owner of the files created using ADD and COPY
will match the user that RUN instructions run as).

This is a revived version of #9046 (which is an implementation of the proposal #7537). Hopefully this will end better this time.

Fixes #6119

Signed-off-by: Aleksa Sarai asarai@suse.de

/cc @calavera @runcom @vdemeester @tiborvass @thaJeztah

[skolodyazhnyy]

Should it be gidMaps?

[cyphar]

Ah, yes it should. Thanks.

[cyphar]

/ping @calavera @runcom @vdemeester @tiborvass @thaJeztah
/cc (the original list) @shykes @crosbymichael @unclejack @cpuguy83 @erikh @tiborvass

I would really prefer that this time I don't have to wait 3 months to get a response as to whether or not this feature is acceptable. I don't have the patience to keep this patchset alive for the next 3 months.

[cpuguy83]

@cyphar I'd love to have it, but it's a breaking change that could potentially even lead to security issues in certain situations (e.g. a user expecting root).

I know a few of us have seen the PR but haven't responded because we don't have a good answer.

[duglin]

Agreed - like I the idea but I'm not thrilled with breaking backwards compat - which is why I would prefer to do it via a "--user" flag so we don't break anyone.

[cyphar]

My issue is that the "solution" described at the tail end of #9046 never actually shipped. Sure, the parser did support arguments at some point (I'm not sure it still does), but the fact that we spent so much time coming up with more and more esoteric cases where "backwards compatibility was broken" rather than actually saying "what is the worst that can happen?" is quite troubling. We even had proposals to version the Dockerfile format so we could fix broken behaviour. Those didn't ship either. There have been countless proposals to implement something like this. They still haven't happened. It's been more than 2 years. Meanwhile, people are hit by this all the time, and things like RUN chown have caused issues with some graphdrivers as well. But hey, at least it's backwards compatible!

If you look at the commit which added this "functionality" (026aebd), there was no 3 month discussion about whether the user who owns the final file should be root or the user from the config or the fairy godmother. Why would there be, that would be ridiculous! It was based on "the docs say so, so we must implement it". But when people start pointing out that it is misleading and a janky UX, suddenly we have to start talking about how there might be someone on some moon who's written a Dockerfile that might break at some point.

but it's a breaking change that could potentially even lead to security issues in certain situations (e.g. a user expecting root).

I've got to admit, this is a legitimate concern. Sure, if you write a Dockerfile like this (where somesecretfile is 0400):

USER someone
COPY somesecretfile /secret

Then it will be read-only to the "someone". I would argue that's a badly written Dockerfile, but I guess that doesn't fly. And no, I don't have a good answer to it. Because the only good answer is "stop writing bad Dockerfiles". And maybe --owner will allow people to do what they want to do without doubling their image size or resorting to ADD (but it still won't fix the horrible default and USER will also never die).

But the thing is that I know that --owner won't ship. Because someone will come up with yet another backwards compatibility argument why we can't put the options after the COPY or RUN. Then someone will make an argument about why we can't put it before because --owner COPY ... used to be ignored in older versions of the daemon. I know that us creating a new builder from the ashes of the old Dockerfile also won't ship, because this builder has too much traction.

We add features with little regard for their consequences and then we hesitate endlessly about fixing them. I don't understand how we are meant to fix bad past decisions if we can never change anything. It's infuriating.

[erikh]

I left docker and Doug was kind of put on the spot to maintain the
builder + 400 other things.

There's more than the technical element here, so be mindful of that
please.

-Erik

On 18 Mar 2016, at 8:42, Aleksa Sarai wrote:

My issue is that the "solution" described at the tail end of #9046
never actually shipped. Sure, the parser did support arguments at some
point (I'm not sure it still does), but the fact that we spent so much
time coming up with more and more esoteric cases where "backwards
compatibility was broken" rather than actually saying "what is the
worst that can happen?" is quite troubling. We even had proposals to
version the Dockerfile format so we could fix broken behaviour.
Those didn't ship either. There have been countless proposals to
implement something like this. They still haven't happened. It's been
more than 2 years. Meanwhile, people are hit by this all the time,
and things like RUN chown have caused issues with some graphdrivers
as well. But hey, at least it's backwards compatible!

If you look at the commit which added this "functionality"
(026aebd), there was no 3 month
discussion about whether the user who owns the final file should be
root or the user from the config or the fairy godmother. Why would
there be, that would be ridiculous! It was based on "the docs say so,
so we must implement it". But when people start pointing out that it
is misleading and a janky UX, suddenly we have to start talking about
how there might be someone on some moon who's written a Dockerfile
that might break at some point.

but it's a breaking change that could potentially even lead to
security issues in certain situations (e.g. a user expecting root).

I've got to admit, this is a legitimate concern. Sure, if you write a
Dockerfile like this (where somesecretfile is 0400):

USER someone
COPY somesecretfile /secret

Then it will be read-only to the "someone". I would argue that's a
badly written Dockerfile, but I guess that doesn't fly. And no, I
don't have a good answer to it. Because the only good answer is "stop
writing bad Dockerfiles". And maybe --owner will allow people to do
what they want to do without doubling their image size or resorting to
ADD (but it still won't fix the horrible default and USER will
also never die).

But the thing is that I know that --owner won't ship. Because
someone will come up with yet another backwards compatibility argument
why we can't put the options after the COPY or RUN. Then someone
will make an argument about why we can't put it before because
--owner COPY ... used to be ignored in older versions of the daemon.
I know that us creating a new builder from the ashes of the old
Dockerfile also won't ship, because this builder has too much
traction.

We add features with little regard for their consequences and then we
hesitate endlessly about fixing them. I don't understand how we are
meant to fix bad past decisions if we can never change anything. It's
infuriating.

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#21088 (comment)

[calavera]

Agreed, we do not want this patch. Thanks.

[sdoxsee]

So is there a workaround? I was really looking forward to this patch :( I feel like my only option is to make everything work as "root" because otherwise I can't use my copied files with the user the image asks for.

[cyphar]

@sdoxsee You can put your files in a tar archive and use ADD. I don't like the use of ADD (it's too magical), which was the whole point of the proposal. Oh well.

[jswetzen]

For me it doesn't matter how the feature is added, a flag or using USER. It just needs to be there. I'm using your tar workaround now, that works well. Thanks!

[michaeljs1990]

If this is the final solution it seems like https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/ and the dockerfile syntax documentation should be updated to let users know that if you want to run as a non root user you will be in some cases doubling your image size.

I hope to see the --user flag introduced soon.

[helmingstay]

+1 to documentation updates at very least. I just spent a day chasing down a proper solution to this, and cyphar's "too much magic" manifesto is exactly how I feel at the end my (ultimately fruitless) pursuit. Years-old issues, unanswered SO questions, and rejected pull requests aren't the ideal primary documentation for well-known behavior.

[etotheipi]

Can someone explain the "tar workaround"? All I see is that ADD will automatically unpack your .tar file, but I don't see how that saves you the trouble of having to switch to root, chmod/chown, switch back, and accept double layer sizes. Am I missing something?

This desperately needs to be fixed. What should be a trivially simple workflow to default to a non-privileged user has turned into a nightmare of bloated Dockerfiles and bloated image sizes. This encourages some very poor practices.

[jswetzen]

The workaround is that you create the files on your local machine, chown/chmod everything you need (as root probably) and then make a tarball of that. When it's unpacked, the ownership and rights will be preserved as they were when you packed it. It's an ugly workaround, but it's the only way to do it in some cases.

[italomaia]

Being able to COPY files respecting USER seems quite reasonable. Sometimes one has to break things for the greater good.

[erikh]

this was resolved recently in a ticket I did, should be in the next release.

…

On Wed, Apr 19, 2017 at 11:00 AM Italo Maia ***@***.***> wrote: Being able to COPY files respecting USER seems quite reasonable. Sometimes one has to break things for the greater good. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21088 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6ydGZtYv_W-t8_GokD2PffZ4F_nSks5rxkuygaJpZM4Htt-j> .

[wprater]

@erkh did this make it into a release yet? still seeing the behavior in 17.03.1-ce

[italomaia]

@erikh I would also very much like to know that.

[mightyiam]

See #34263.

[jukefr]

@erikh very interesting information you're giving us here. many detail. much links. such status.

[erikh]

Yep.

…

On Wed, Aug 9, 2017 at 8:16 AM, Kevin Jullien ***@***.***> wrote: @erikh <https://github.com/erikh> very interesting information you're giving us here. many detail. much links. such status. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21088 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6xCHdcX1P8BwUDfyTwgdgGkAZZvjks5sWc1QgaJpZM4Htt-j> .

[erikh]

You guys can fix it. Thanks for playing!