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

Proposal: Handling of permission bits for image context created via Windows #11047

Closed
ahmetb opened this issue Feb 26, 2015 · 7 comments · Fixed by #11148
Closed

Proposal: Handling of permission bits for image context created via Windows #11047

ahmetb opened this issue Feb 26, 2015 · 7 comments · Fixed by #11148
Labels
kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny platform/windows

Comments

@ahmetb
Copy link
Contributor

ahmetb commented Feb 26, 2015

Problem

Converting Windows file attributes + ACLs to 9-bit UNIX permissions when
docker images are built from windows CLI. Concept of 'execute' doesn't
match 1:1 on Windows vs POSIX.

Scenario

  1. User downloads Docker client for Windows (docker.exe)
  2. User develops an "app" to run on Linux containers.
  3. User runs the command docker build .
  4. Context tar is created on client-side and transferred to daemon (running on Linux) for building.

Problem steps:

  1. Let's say the "app" is actually something user cloned from a remote git repo.
  2. Repo had a setup.sh script (chmod +xed)
  3. There's no x bit on windows, so during git clone we lost that bit
  4. Today all files packaged to context from windows get -rw-rw-rw- permissions.
  5. This can also happen if user unzips a file or downloads from ftp etc.
  6. If user tries to RUN ./setup.sh or ENTRYPOINT ./setup.sh it will fail
    due to a vague "permission denied" error because script was not chmod +xed.
  7. Result: a dockerfile that builds correctly fine from OS X/Linux cli has failed on windows.
  8. User will take a lot of time to figure +x bit was missing.
  9. User will go add RUN chmod +x setup.sh as a dockerfile instruction if they're
    building from Windows.

Root cause 1: No 'x' (execute) bit on Windows

  • Windows does not have an execute permission as the 'x' bit in Unix
  • By default, almost all files are executable on windows. (different than unix)
  • You can execute a binary if the extension is exe/bat/cmd/com/ps1 etc.
  • Windows executable permission is more about dynamically linked libraries (dlls) etc.
  • It's not widely used.
  • Does not correspond to executability concept in Unix.
  • Every time user grabs a +xed file from ftp/git/unzip/samba, we lose that
    unix bit on windows, because it's not the same thing.

Quoting from SAMBA book:

“Note that there is no bit to specify that a file is executable. DOS and Windows NT filesystems identify executable files by giving them the extensions .EXE, .COM, .CMD, or .BAT. [...] Samba can preserve these bits by reusing the executable permission bits of the file on the Unix side - if it is instructed to do so. Mapping these bits, however, has an unfortunate side-effect: if a Windows user stores a file in a Samba share, and you view it on Unix with the ls -al command, some of the executable bits won't mean what you'd expect them to.”

Root cause 2: Go Windows Problems

Golang's os.Stat implementation is very poor:

  • It cannot figure out executability. (because it's not a file attribute on
    Windows but an ACL setting –found out through a separate syscall, not implemented in golang)
  • It returns same permissions for all user/group/others sections:
    • Because there's no group/everybody concept on Windows as in chmod bits.
    • That's why it returns -rw-rw-rw- for all files on Windows and that's the permission what files copied to a docker build context gets by default.
    • This is dangerous (imagine a private key being set to -rw-rw-rw- permissions by default, that's the state we're in now).

We are hoping to address problems in golang in the long term. (fingers crossed)

Solution

  1. We are going to clear all 'group/others' bits: -???------
    • Since we're clearing r from group/others, this might break multi-user docker images.
  2. We are going to copy r/w bits we got from golang os.Stat().
  3. We are going to add +x to all files packaged from windows. 💥
    • This way we can at least have +x on all files and can support executing ./binary or ./script.sh from shell.
    • This may break some apps that decline +xed files as input. I haven't really heard of anything like that,
      must be a really rare case. Even so user can have RUN chmod -x to remove that bit manually
    • Security issue: We are making everything executable. Could be. ❗ Please discuss.
  4. We are going to add a notice for windows CLI: WARNING: Permission bits on files added to image might not be correctly set. 🆕

Proposed Fix

We modify the perm bits of the tar header created via tar.FileInfoHeader
with a simple platform-specific helper method. (see code)

We are also going to change the expected permission string
on the integration-cli test cases when executed on windows.

Result

We will end up with a docker image in which all files copied from
Windows filesystem have (1) grp/others bits cleared (2) +x added
regardless of it should have +x because it does no harm.

Behavior on Linux/Mac CLIs are unchanged.


I really appreciate your input on this. That's the only thing left prevents us from getting green on Windows client CI.

cc: @ewindisch @crosbymichael @cpuguy83 @tianon @tiborvass @jfrazelle @icecrime @johngossman @sachin-jayant-joshi @jhowardmsft

@jessfraz jessfraz added Proposal kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny labels Feb 26, 2015
@tiborvass
Copy link
Contributor

IANTM, but this seems to be the best compromise from what I can tell. So +1 from me.

@thaJeztah
Copy link
Member

First of all, thanks for looking into this. In general I can see this as a reasonable compromise (but of course hope something better can be invented in future). I'll add a couple of ideas / concerns;

  • Users building on Windows "from scratch" (not the image) probably won't be surprised to have to perform the extra steps; this is similar to uploading a file via FTP and having to set the correct permissions. I see this as the "least problematic" case; they will have to add additional chmod/chown to their Dockerfile, but that's fine.
  • Indeed; people checking out someone's repo and build it on Windows is more problematic; the permissions are lost and the Dockerfile doesn't account for this, possibly breaking it. For people wanting to build a GitHub repo unmodified, we could advise them to docker build github.com/some/repo, so that the files never "touch" the Windows filesystem.
  • If Git keeps track of permissions in its meta-data, would it be possible to set these permissions when tar-ing the files before upload to the daemon?
  • Setting permissions rwx------ for all files added via COPY or ADD, will make it (near) impossible for users running on AUFS to add group/world permissions afterwards because of unexpected file permission error in container #783. This can be 'solved' if Proposal: Specify user/ownership for COPY. #9934

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 27, 2015

@thaJeztah that is also another option. I just wanted to make sure if it is okay to drop the x bit and leave chmod/chown to the users as an exercise.

If we can just abandon the requirement of preserving the 'x' bit when deployed from windows, it's fine with me either. I guess developers working on windows machines deploying things to Linux servers already don't use much of +x bit. 😃 (correct me if you think this is a wrong assumption). As you said, they won't be sruprised.

You are also right about remote git builds. If the user cares about git repo, they can use that. We don't need to find out if we're on a git repo then check index for file metadata.

@tianon
Copy link
Member

tianon commented Feb 28, 2015 via email

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 28, 2015

@tianon I wasn't aware of the AUFS situation tbh. That's what I initially suggested but security problems are my biggest concern. We might assume people know what they're doing by adding a warning message and just make everything +x.

We mostly assume user knows what they COPY/ADD to container, so the only +x'ed stuff are going to be those files. And if it happens so that somebody gains shell access to inside container, they can "chmod +x" or replace system binaries anyway. I'm mostly convinced to follow "everything +x" at this point.

@duglin
Copy link
Contributor

duglin commented Mar 4, 2015

FWIW, I agree with +x'ing everything. Not ideal but really the only choice we have. Making people explicitly do a "chmod +x" themselves will almost guarantee people avoid the Windows CLI, which would be bad.

@ahmetb
Copy link
Contributor Author

ahmetb commented Mar 4, 2015

@duglin thanks for your input on the proposal. Check out #11148 for the code.

seschwar added a commit to seschwar/docker-py that referenced this issue Feb 10, 2016
Be consistent with `docker build`, which marks the files in the context tarball as executable. See also moby/moby#11047.

Fixes docker#937.
seschwar added a commit to seschwar/docker-py that referenced this issue Feb 14, 2016
The build context is tarred up on the client and then sent to the Docker
daemon.  However Windows permissions don't match the Unix ones.

Therefore we have to mark all files as executable when creating a build
context on Windows, like `docker build` already does:
moby/moby#11047.

Signed-off-by: Sebastian Schwarz <seschwar@gmail.com>
christianbundy pushed a commit to christianbundy/docker-py that referenced this issue Sep 27, 2016
The build context is tarred up on the client and then sent to the Docker
daemon.  However Windows permissions don't match the Unix ones.

Therefore we have to mark all files as executable when creating a build
context on Windows, like `docker build` already does:
moby/moby#11047.

Signed-off-by: Sebastian Schwarz <seschwar@gmail.com>
shin- pushed a commit to docker/docker-py that referenced this issue Dec 8, 2016
The build context is tarred up on the client and then sent to the Docker
daemon.  However Windows permissions don't match the Unix ones.

Therefore we have to mark all files as executable when creating a build
context on Windows, like `docker build` already does:
moby/moby#11047.

Signed-off-by: Sebastian Schwarz <seschwar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny platform/windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants