LGTM although I'm uncertain on what the best path might be for group/other bits

LGTM

https://jenkins.dockerproject.com/job/Windows-PRs/96/console
--- PASS: TestBuildAddSingleFileToRoot (4.11s)
--- PASS: TestBuildAddWholeDirToRoot (4.09s)
--- PASS: TestBuildCopySingleFileToRoot (3.25s)
--- PASS: TestBuildCopyWholeDirToRoot (4.37s)

Tests fixed on windows.

@tianon re-LGTM maybe?

LGTM

I wonder whether we need docs to explain the "issue".

@tiborvass hmm we don't have any windows docs at all at this point. I will create an issue now and you can assign that to me.

@tiborvass ❤️ 🍸 🎶

This is a bit overkill for windows as you have the default windows permissions now at "-rwx------" rather than the linux permissions of "-rw-r--r--". Having new files added to containers not be group/world readable is a big downside and probably not expected. I agree group/world writeable is not a good idea but removing all read permissions is probably too far. I would assume a 0755 would be a more appropriate mask override. In addition windows users can build tar archives with proper permissions (ie --owner 0 --mode 755) so it may be better rather than masking all permissions in windows to only look for the bad permission of 666 (default for tarred items for windows) and changing that to 755 or 644.

I don't think this commit is also the cause of the ADD/COPY behavior change but I may be wrong. As now with Add doing similar doing an ADD then running as a restricted user will now fail if a chown is not done so:
FROM busybox
RUN adduser -D bob
ADD test.conf /tmp/
USER bob
CMD cat /tmp/test.conf

will fail with permission denied. (Also while the commit seems to primarily deal with archives I noticed the change to default file permissions above.

@mitchcapper main reason we cleared the permissions from others/group is because people might assume they set the correct permissions on NTFS and those ACLs cannot be translated to unix formats. Assumption here was people are not very commonly using USER ... in their dockerfiles. I don't really think it's extreme, as IMO it's better than stuff like private keys being '-rw-r--r--' without letting the user know. We should also add some warning probably here to let people know they're running on windows and they will not achieve the same high fidelity ACLs against linux hosts (in fact I forgot to add that).

Feel free to come up with another proposal, I'd happily review but this is basically result of what we discussed in #11047.

I 100% agree with you on the fact people are not commonly using USER, the problem is this is recommended practice from a security perspective. Running as root (even containered root priv dropped rooot) is probably a bad idea. Docker is fairly hard to run as non-user, which is a whole separate discussion, but I figured this behavior is only making it all the harder to run as a non-root user. I am not sure I agree with the private key aspect.

1. As most people run the container as root anyway unless they are priv dropping once inside the container (which would still not be as good as running the container as a user) its not really keeping the private key more secure.

2. Sensitive information should generally not be inserted into containers (I believe that is also best practice) and why there is a whole other discussion on how to keep it out of containers.

3. Private keys are probably a very small fraction of what is added to the container, with other items far more common, and while convenience over security is not great there has to be some line

4. You shouldn't ever make permissions less strict down the road. Right now this is just in HEAD but once it goes to a release you don't want to ever say well, maybe we shouldn't be removing all global/group permissions and change it (as then you have made peoples builds less secure). It would be far better to apply a less aggressive masking leaving read access (or preferably even read/execute) for group/user alone. It would break fewer peoples current builds, and I don't think there is a whole lot of expectation of security in windows that when you call ADD or COPY on a windows files it will by default be root only readable.

I missed the boat on the other proposal and realize the input is far after the fact, but figured I would comment my 2cents. I am all for security, and do a decent bit of work to run containers with least privs possible, but am not sure this will work to improve it (even at the cost of convenience). This masking I think will truly weaken good user behavior (as more dockerfiles just wont work as users other than root out of the box), in exchange for protecting bad user behavior (adding sensitive information into containers and expecting privacy by default).