Docs?

Yeah, docs are obviously missing.
I'd like to get some feedback on the rest before spending time on docs though. To avoid wasting time.

/cc @ktintc

@alexlarsson Okay. Docs are never a waste of time IMHO. :P But understand.

I am personally a +0. I think making docker cp bi-directional would resolve this use case for me but I understand the desire.

Why does this functionality have to be specific to private files? Instead, if you just created a generic "--add" argument that had the same semantics as the Dockerfile ADD but just allows you to add files on run, that could be used for this also. You'd just do "--add my.key:/run/private/".

@jamtur01 docker cp wouldn't really work for most of the usecases here, as it inherently works on the copy-on-write parts of the docker container. I.e. the part that will be commited to an image. It never lets you copy from e.g. a volume or a tmpfs like /run. So, its generally no different from just having the file in the base image, i.e. all private data will be leaked with the image. Additionally, even if you could copy a file into a tmpfs like /run, that would only be for the instance of /run for the cp, and it will be gone the next time you start the container, which means this would not be useful in e.g. a Dockerfile where each line is a new container.

@ibuildthecloud I've intentionally tried to limit the capabilities for this because it doesn't really match with the desire to make docker build fully repeatable and host independent. This is by design of course, the goal is of course to only let you build if you have the right key, etc. However, we want to avoid this being misused for other things. Also, the way the code works (holding data in memory, encoding it, etc) makes it not really work well for e.g. large files.

You do not want all third party containers running on your host get access to those private files. So I think it is not a good idea to put the private files from /etc/docker/private.d into every container that is run on a system. There should be an explicit command line option (like --add-private-files) for that.

@djmaze Well, if you want container specific files you can just just --add-private-files, and ignore /etc/docker/private.d. On the other hand, an important usecase for us (redhat) is to have host-specific product entitlements in all containers so you can yum install rhel packages when running on a rhel host.

@alexlarsson From what I gather the private files is not persisted in the container *.json config files so you have to do docker start --private-files ... if the container is ever stopped. This is going to be problematic when you restart dockerd. Your containers will be restarted but with no data that was passed in from run --private-files.

How bad would it be to just persist the data in the json config files? Basically only root can see those files, if you're root on the box you can get to the data in the container /run/private anyhow.

@ibuildthecloud Well, it wouldn't be hard to persist it, in fact it would be easier code-wise than what the current code is doing. However, how bad would it be from a security/risk perspective? Thats really hard to say, as it depends on all sorts of details. I just erred on the side of caution. If we add it to the config file the main difference is a) its a bit easier to get at when you're root, and b) its possible to get the secrets from non-running containers, and the advantage is that you can start a container that dies without knowing the secret. Is it worth it? I don't really know.

@alexlarsson If you don't persist it then I think there needs to be something that would prevent restarting a container that was previously launched with run --private-files because the container is most likely not valid without the private files.

The only security issue I can think of if you persist it it is that the contents of the private files is accessible through the API which may be accessible by non-root. Maybe you just don't return the contents in a docker inspect call.

I agree with @ibuildthecloud that runconfig will cause containers to fail to restart cleanly. Also, I'd like to remind @alexlarsson that security doesn't come from "it might be more secure, I'll err on the side of caution". Security comes from actually being secure, and keeping a file path secret isn't security at all. runconfig in my opinion complicates the code base while providing no security benefit at all. In fact, it makes security worse:

The key is held in a file owned by root, presumably a file that is labeled with SELinux to be accessible only to docker. If you store the path to the key in the docker config(also labeled by SELinux as belonging to docker and docker alone) then the information about where the secret file is is confined to the same security context as the secret itself(the docker security context).

However, if the user has to type in the path to the file using --private-files every time they launch the container, then the "secret path" to the "secret file" will now be stored in .bash_history, which is outside the docker context. ;)

@timthelion Its not the path, its the actual data, which may have been uploaded from another host (and thus not exist anywhere else on the host but inside the tmpfs of the container).

That said, if you're root on the host you can still nsenter the container and extract the data from the tmpfs, so its not technically safer there. The main real difference is whether the secret data is fully gone when the container exits, or not.

It seems to me, that it would be more useful and flexible to:

A) make volumes work at build time.

and

B) use a wrapper script to create and destroy the tempfs

@timthelion volumes at buildtime was nak:ed by @shykes due to not wanting host-dependent builds. This pr is the direct result of that, as for these usecases we conceptually need to be somewhat host-dependent, but we try to do it in a minimal way.

Hmph, I don't see this re-branding of volumes to be any less host-dependent....

When you think about it from RedHat's point of view, it doesn't make much sense to keep the product key secret while allowing the product itself to be easily copied. So why not keep the key in the image ;)

It just occurred to me that a generally useful command would be WITHFILE.

WITHFILE works a lot like ADD but with a command on the same line. You want WITHFILE if you need to use some large resource at build time, but you don't want to to ADD the file to your final image due to space concerns.

For example, if your build requires linux-headers to complete, but you don't want to add an extra 20 megabytes to your final image you can do:

WITHFILE linux-headers.tar.gz /build/linux-headers.tar.gz cd /build/ ; tar -xzf linux-headers.tar.gz; make ; make clean; rm -rf linux-headers ; rm linux-headers.tar.gz

You've just build your program with the linux headers, but these headers are not stored in the final image, thus bloat is reduced.

WITHFILE may also be useful for installing software that requires a product key to install.

For example to install proprietary redhat software into your image without adding your rhel key to the final image you can do:

WITHFILE rhel.key /etc/yum/rhel.key yum install proprietary-rhel-software ; rm /etc/yum/rhel.key

@timthelion The ; rm /etc/yum/rhel.key in your last line should not be necessary, because that should be automatically handled as part of the WITHFILE directive. (I see this as a temporary ADD just for one line, right?) +1 for WITHFILE.

@djmaze you're right that the ; rm /etc/yum/rhel.key doesn't need to be necessary, and I guess there's no point in not deleting the file automatically.

@djmaze Yes, it is just a temporary ADD...

@djmaze actually there is a good use case in which the file should not be deleted. Compare the disk usage of the following two snippets(given that WITHFILE does NOT delete the file after the command is run:

ADD docker /usr/bin/docker
RUN chmod +x /usr/bin/docker

and

WITHFILE docker /usr/bin/docker chmod +x /usr/bin/docker

;)

Though admittedly if WITHFILE did always delete the file after the command was run one could also do:

WITHFILE docker /docker mv /docker /usr/bin/docker ; chmod +x /usr/bin/docker

@timthelion Finally I get what you imagined. You want WITHFILE to automatically remove the file after the last line in the Dockerfile has been eval'd, right? I do not think this fits the syntax you are proposing. That should rather be called ADD_TEMPORARY (or similar) and stand on its own line in the Dockerfile, just like ADD.

No, WITHFILE will should add the file and run the command BEFORE saving a layer. If we removed the file after the last line in the docker file then the file would still be there, just marked as removed in the COW(Copy on write) file system.

@djmaze If the docker binary weighs 20 megabytes and we chmod +x it, then it will get copy-on-writed and it will weigh 40 megabytes. But if we were able to do the chmod +x at the same time as adding it, then it would be written to disk only once...

@djmaze in the case of the private key files, we never want them to be written into a layer at all, so that they stay private. Which is why WITHFILE is useful for that case as well.

WITHFILE would apply only to build, while the current --private-file works for both run and build.

Persisting the private files in the docker config would be nice because it would allow restarts, especially in the context of maintenance because the operator restarting docker and its container will most like not be able to restore the missing private files.

Having said that, I see it would also be nice that in the current approach if you were to use the API to start a container theoretically the private files will never be persisted to disk. For the extra paranoid that could be a nice feature.

What if we could do both, for example --private-file api.key:/secret/key would be purely ephemeral and not persisted to disk while --private-file api.key:!/secret/key (with an exclamation mark) would be persisted to the docker container config. The actual syntax isn't important, just the idea.

Side note, I ran this PR before and I swear the arguments were actually reversed so that its --private-file destination:source not --private-file source:destination.

@ibuildthecloud This is a huge amount of added complexity for a very narrow use case. And completely unnecessary, because the same functionality can be implemented more simply.

Image 6 months from now when everyone will be coming across the RunConfig struct and thinking "hey, I should put the configuration options needed to run the container in the RunConfig struct, that seems logical."

I am not the maintainer, but if I was, I'd never let this through.

@timthelion I think everyone has understood your point of view.

@unclejack I don't think that makes my critique less helpful. I'm not just ranting I'm making specific critiques and I'm trying to find solutions as well. Perhaps renaming RunConfig to TempRunConfig would help.

The Run in RunConfig means "per-Run", as the HostConfig means "per-Host" config and "Config" is global Config. Its possible that a better name can be found, in particular since the package is named runconfig this becomes a bit confusing. RuntimeConfig?

So what's the conclusion then? Are we saying that if you use docker run --private-file and then you have to restart docker -d, you're just out of luck, your containers are dead?

@alexlarsson Are you sure you don't want to flip the syntax to be --private-file /secret/key:api.key. If you look at other args like -p or --link, the format is [host value]:[container value].

@ibuildthecloud Really, this feature is meant for short term tasks like software installation. If you need the file to be in the container permanently and you need it to survive docker daemon restarts than this isn't the feature for you.

According to https://gist.github.com/alexlarsson/c8e3277d2678c1061319 this is not necessarily just short term build operations. For example, I may need an x509 client cert to communicate to some HTTPS service.

If we are limiting the scope of this to short term build operations, then I'd remove --private-file from run and only have it on build. But I don't think this was @alexlarsson original intention.

Long discussion at todays meeting lead to a slightly different design. I will re-do this with the new approach.

I made a new PR for the new approach to get a fresh start on the discussions:
#6075

closing this in favour of #6075