-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Add Secret store #6075
Add Secret store #6075
Conversation
@@ -72,6 +73,7 @@ func (cli *DockerCli) CmdHelp(args ...string) error { | |||
{"run", "Run a command in a new container"}, | |||
{"save", "Save an image to a tar archive"}, | |||
{"search", "Search for an image in the docker index"}, | |||
{"secret", "Maintain secrets database"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Maintain a secrets database" or "Maintain a database of secrets"
Update fixes the doc misspelling |
+1 for secrets which are added to a container's fs during run-time. This would be invaluable for ssh keys and other secrets. This is a feature which would make a project I'm working on much simpler. |
Updated to latest master |
return err | ||
} | ||
for _, s := range data { | ||
container.command.CreateFiles[filepath.Join("/run/secrets", s.Name)] = s.Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/var/run is typically a symlink to /run in "modern" distros. See e.g. https://wiki.debian.org/ReleaseGoals/RunDirectory
I don't think /var/tmp makes sense, its semantically less like a temporary file, more like a runtime transient file.
@alexlarsson would you mind answering this: https://groups.google.com/forum/#!topic/docker-user/aWStVPCwq6w ? |
Out of interest, why |
@cyphar It grants access to a predefined secret rather than define a secret, so it felt right. I'm open to changing that of course, if we want it to be shorter then |
--grant-secret is better, less ambiguos. |
+1 on |
@alexlarsson why can these files not be bind mounted? |
} else { | ||
hostBased := "" | ||
if out.GetBool("HostBased") { | ||
hostBased = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use [OK]
instead of *
like in docker search ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that signifies "trusted" to which "ok" seems to make sense. Here it is a bit more generic boolean. Maybe [x]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe [HOST]
or just [H]
?
@crosbymichael We want to copy the files for real into the container so that later changes to the secret store does not affect running containers. For instance, it should be possible to remove a secret from the store once a container is started with it. Furthermore, we might have secrets that are not necessarily stored as regular files in the filesystem in the future (in an earlier patch the secrets were never on the host fs at all for instance). That said, we could perhaps use bind mounts at a higher level. I.e. mount the tmpfs on the host, copy files to it, then bind mount it into the container and unmount the host version. I'll have a look at that. |
@crosbymichael Here is the alternative implementation using a tmpfs from the host bind-mounted in: If you prefer that approach we can drop a bunch of the libcontainer commits when this is squashed in. |
If you implent CmdFoo with an extra boolean first arg, and CmdFooList and CmdFooAdd as usual the code will make this just work. Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
The secrets store keeps track of a set of named secrets, such as API keys or ssh keys that you can later access from containers. There are two types of secrets, the host based ones, which are stored in /etc/docker/secrets, and are automatically applied to all containers, and user secrets which you can add via "docker secret add <name> <file>" and which can be granted permission to by name. You can also list secrets with "docker secret list" and remove them with "docker secret rm". Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
If you run something like docker run --grant-secret foo image Then the "foo" secret will be copied into a /run/secrets tmpfs in the container. Additionally, all host-based secrets will be copied into all containers running on a specific host. Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
This is useful if you want to use secrets during docker build which will not be recorded in the final image. Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
I rebased on master with the alternative approach from above. |
|
||
Lists the names and details of all the secrets that are installed in | ||
the docker daemon secret store. Access to these secrets can be granted | ||
to containers at user disgression. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume you mean discretion
:)
@SvenDowideit how about the commit I've added here https://github.com/vbatts/docker/compare/alexlarsson-secrets |
@vbatts @alexlarsson better, but still i don't think its enough. I would suggest adding a mention to the now to the harder part.... Does this mean these questions should be answered for the reader in the cli documentation - otherwise they're left not knowing - and obviously, if its documented, there should be tests for that. |
|
@SvenDowideit @timthelion k. I'll work on this. |
As a note, I've identified a couple of items regarding this feature that I will either fix or get clarity on.
Also, i'm not sure how to handle the uniqueness of file names, and having them appear in the container in a consistent way. Perhaps to have a markup, similar to --volume, so there could be --grant-secret=:content.pem to have the uniquely named file from the host/store, be available in the container as "content.pem". |
@vbatts I think that the existence of |
are we moving to #6697, and thus should close this to avoid losing info due to split conversation? |
@SvenDowideit I'm not opposed to moving the conversation. Most of the meat of the design is on this issue. @timthelion I can't disagree with you. I've only gotten involved on this request recently. Perhaps having a syntax like |
The changes here in |
@SvenDowideit Yes, please close this in favor of #6697, this is getting confusing :( |
Closing in favor of #6697 (which is carrying this PR) |
This is a continuation of #5836, and the discussions about it we had on last weeks irc meeting. It implements a secret store that you can access via:
You can then grant access to a specific secret by using
docker run --grant-secret a-name image
.The granted secrets are stored (by name) in the hostconfig, and will be re-granted on restarting the container (assuming the secret is still in the store). Additionally any files stored in /etc/docker/secrets are considered "host based" secrets and will be granted to all containers on the host.
On container start the secrets are copied into a tmpfs in /run/secrets, so they will not ever be part of any image based on the container.
There are no docs yet, I'll be working on that next.