Skip to content
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

Change windows default permissions to 755 not 711, read access for all p... #11395

Merged
merged 1 commit into from Mar 23, 2015

Conversation

@mitchcapper
Copy link
Contributor

@mitchcapper mitchcapper commented Mar 15, 2015

...oses little security risk and prevents breaking existing Dockerfiles

See issue #11246 for more details. #11246 goes to more than just windows, but windows was recently rolled in so what would be good to rollback sooner rather than later.

@GordonTheTurtle
Copy link

@GordonTheTurtle GordonTheTurtle commented Mar 15, 2015

Can you please sign your commits following these rules:

https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work

The easiest way to do this is to amend the last commit:

$ git clone -b "master" git@github.com:mitchcapper/docker.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

This will update the existing PR, so you do not need to open a new one.

@duglin
Copy link
Contributor

@duglin duglin commented Mar 20, 2015

Marking this with 1.6 because I think we should decide on this (one way or the other) prior to 1.6 going out.

ping @ahmetalpbalkan

@mitchcapper
Copy link
Contributor Author

@mitchcapper mitchcapper commented Mar 20, 2015

Maybe the best solution is to set it to the softer 755 mask, and add a note to the documentation that ADD/COPY commands on windows will set permissions to 755 by default (due to no windows permission equiv) so that no one is surprised by this action. That insures existing Dockerfiles don't break (along with better similarities to building on linux) and lets users know that private data will need to be chmodded afterwards if they are running a container as root and dropping to another user (and don't want the dropped user to be able to access it). If you decide later on you can always make it more restrictive without much impact.

@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 20, 2015

@mitchcapper #11397 is already adding a warning about current 0711 mask

@duglin maybe we can pull some Docker security engineers to the discussion at #11246? This has been initially discussed and folks were ok with removing r/w bits for others/group. This was not the main change we made in #11246 ; it was to add +x.

I'm definitely +1 for this proposal however normal Windows users mostly won't know chmod strings, what they mean and how to change them. If we can agree that we will be fine by just adding warning message (#11397) then I'm +1.

@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 20, 2015

@mitchcapper windows tests are failing because you need to change the expected default file permission for windows in integration-cli package to -rwxr-xr-x now. :) please amend your existing commit and git push -f to your branch.

@mitchcapper
Copy link
Contributor Author

@mitchcapper mitchcapper commented Mar 20, 2015

We are talking about a Windows user is building a linux container with modifications, and having that container start as root but drop to another user for security purposes (which is the use case where the more restrictive bits are useful). For this type of user I think them knowing chmod is not unreasonable (especially as if they had put that file onto the linux box by another method they would need to chmod it).

@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 20, 2015

@mitchcapper I haven't run any containers as root nor heard my colleagues doing the same thing, but then I'm probably not a good docker user. :) You might be correct in the "people run containers with root" statement.

If you update this PR with the test fix, I think I can be +1, but I'm not a maintainer so you need to convince other folks as well. :)

@tianon
Copy link
Member

@tianon tianon commented Mar 20, 2015

I'm +1 on -rwxr-xr-x via Windows, FWIW. 👍

@@ -29,7 +29,7 @@ func CanonicalTarNameForPath(p string) (string, error) {
// 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.
Copy link
Contributor

@ahmetb ahmetb Mar 20, 2015

comment out of date with this PR, remove.

@mitchcapper mitchcapper force-pushed the master branch 2 times, most recently from e1e61cc to 6446ae6 Mar 20, 2015
@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 20, 2015

@mitchcapper please squash your commits into 1 and git push -f to your branch... I see 3 commits currently on the PR.

Also #11397 is merged so the warning shown assumes that this PR is merged.

@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 20, 2015

hmm tests are still failing with the change. Apparently the files added are getting other mod than -rwxr-xr-x. That's weird. Can you build some stuff and see what permission they're getting with this change?

@@ -28,8 +28,7 @@ func CanonicalTarNameForPath(p string) (string, error) {
// chmodTarEntry is used to adjust the file permissions used in tar header based
// 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
perm &= 0755
// Add the x bit: make everything +x from windows
perm |= 0100
Copy link
Contributor

@ahmetb ahmetb Mar 21, 2015

you need to make this 0111 since we'll add +x to others/grp as well now. that's why tests are failing

@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 21, 2015

also fix the TestChmodTarEntry test in pkg\archive\archive_windows_test.go (you don't notice it's failing because package unit tests are currently not part of the windows CI suite). @mitchcapper

…l poses little security risk and prevents breaking existing Dockerfiles

Signed-off-by: Mitch Capper <mitch.capper@gmail.com>
@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Mar 21, 2015

LGTM. Note to maintainers: please !rebuild once the CI issues are resolved. This should be passing now.

@duglin
Copy link
Contributor

@duglin duglin commented Mar 23, 2015

LGTM

1 similar comment
@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Mar 23, 2015

LGTM

jessfraz pushed a commit that referenced this issue Mar 23, 2015
Change windows default permissions to 755 not 711, read access for all p...
@jessfraz jessfraz merged commit 56acb1a into moby:master Mar 23, 2015
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants