New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/archive: adjust chmod bits on windows #11148

Merged
merged 1 commit into from Mar 6, 2015

Conversation

Projects
None yet
9 participants
@ahmetb
Contributor

ahmetb commented Mar 4, 2015

  • Followup to proposal at #11047

This change modifies the chmod bits of build context archives built on
windows to preserve the execute bit and remove the r/w bits from
grp/others.

I just created a post-processing step to modify tar headers retrieved from
tar.FileInfoHeader() method. This fixes tests:

  • TestBuildAddSingleFileToRoot
  • TestBuildAddWholeDirToRoot
  • TestBuildCopySingleFileToRoot
  • TestBuildCopyWholeDirToRoot

Also adjusted integration-cli tests to verify permissions based on the platform
the tests are running. Fixes #11047.

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @duglin @tiborvass @icecrime @jfrazelle @tianon

@tianon

This comment has been minimized.

Member

tianon commented Mar 4, 2015

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

@@ -5,4 +5,6 @@ package main
const (
// identifies if test suite is running on a unix platform
isUnixCli = false
expectedFileChmod = "-rwx------"

This comment has been minimized.

@tiborvass

tiborvass Mar 4, 2015

Collaborator

@ahmetalpbalkan could you please reference #11047 in a comment ?

This comment has been minimized.

@ahmetb

ahmetb Mar 4, 2015

Contributor

fixed @tiborvass

pkg/archive: adjust chmod bits on windows
This change modifies the chmod bits of build context archives built on
windows to preserve the execute bit and remove the r/w bits from
grp/others.

Also adjusted integ-cli tests to verify permissions based on the platform
the tests are running.

Fixes #11047.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@duglin

This comment has been minimized.

Contributor

duglin commented Mar 4, 2015

LGTM

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Mar 4, 2015

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.

// on the platform the archival is done.
func chmodTarEntry(perm os.FileMode) os.FileMode {
// Clear r/w on grp/others: no precise equivalen of group/others on NTFS.
perm &= 0711

This comment has been minimized.

@tiborvass

tiborvass Mar 5, 2015

Collaborator

@ahmetalpbalkan is it 11 just in case the x bit is set for some magical reasons? :)

This comment has been minimized.

@ahmetb

ahmetb Mar 5, 2015

Contributor

@tiborvass I didn't want to clear the +x on grp/others. (e.g. in case of directories, I wanted to preserve the x bit)

This comment has been minimized.

@ahmetb

ahmetb Mar 5, 2015

Contributor

@tiborvass so direct answer to your question is, directory entries are also passed to this method and we already have x bit on those from os.Stat, we don't want to clear those bits.

This comment has been minimized.

@ahmetb

ahmetb Mar 5, 2015

Contributor

ping @tiborvass for review

This comment has been minimized.

@jhowardmsft

jhowardmsft Mar 5, 2015

Contributor

in archive_windows.go, should you be using os.PathSeparator rather than hard coding?

This comment has been minimized.

@ahmetb

ahmetb Mar 5, 2015

Contributor

@jhowardmsft i'm not sure which one you are referring to can you please comment on that line? Edit: corrected Siri dictation

@@ -23,6 +24,17 @@ func CanonicalTarNameForPath(p string) (string, error) {
return strings.Replace(p, "\\", "/", -1), nil

This comment has been minimized.

@jhowardmsft

jhowardmsft Mar 5, 2015

Contributor

This line should be using os.PathSeparator rather than ""

This comment has been minimized.

@ahmetb

ahmetb Mar 5, 2015

Contributor

that's probably out of scope of this PR. but you're right. I'll submit a PR separately.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Mar 6, 2015

@tianon re-LGTM maybe?

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Mar 6, 2015

LGTM

@tiborvass

This comment has been minimized.

Collaborator

tiborvass commented Mar 6, 2015

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

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Mar 6, 2015

@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 added a commit that referenced this pull request Mar 6, 2015

Merge pull request #11148 from ahmetalpbalkan/win-cli/chmod-x-fix
pkg/archive: adjust chmod bits on windows

@tiborvass tiborvass merged commit a6ddb8d into moby:master Mar 6, 2015

1 of 2 checks passed

windows Jenkins build Windows-PRs 97 has failed
Details
janky Jenkins build Docker-PRs 2497 has succeeded
Details
@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Mar 6, 2015

@tiborvass ❤️ 🍸 🎶

@ahmetb ahmetb deleted the ahmetb:win-cli/chmod-x-fix branch Mar 6, 2015

@mitchcapper

This comment has been minimized.

Contributor

mitchcapper commented Mar 8, 2015

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.

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Mar 8, 2015

@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.

@mitchcapper

This comment has been minimized.

Contributor

mitchcapper commented Mar 8, 2015

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment