Add Secret store #6075

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
@alexlarsson
Contributor

alexlarsson commented May 28, 2014

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:

  • docker secret list
  • docker secret add a-name a-file
  • docker secret rm a-name

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.

api/client/commands.go
@@ -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"},

This comment has been minimized.

@timthelion

timthelion May 28, 2014

Contributor

"Maintain a secrets database" or "Maintain a database of secrets"

@timthelion

timthelion May 28, 2014

Contributor

"Maintain a secrets database" or "Maintain a database of secrets"

daemon/execdriver/driver.go
@@ -134,6 +134,7 @@ type Command struct {
Config map[string][]string `json:"config"` // generic values that specific drivers can consume
Resources *Resources `json:"resources"`
Mounts []Mount `json:"mounts"`
+ Files map[string][]byte `json:"files"`

This comment has been minimized.

@timthelion

timthelion May 28, 2014

Contributor

Again this should have a more descriptive name. What makes these files any different than all the other files that docker uses/adds to a container.

@timthelion

timthelion May 28, 2014

Contributor

Again this should have a more descriptive name. What makes these files any different than all the other files that docker uses/adds to a container.

This comment has been minimized.

@alexlarsson

alexlarsson May 28, 2014

Contributor

They are no special, its just a list of filenames and file content that will be copied into the container.
Do you have a better idea for the name?

@alexlarsson

alexlarsson May 28, 2014

Contributor

They are no special, its just a list of filenames and file content that will be copied into the container.
Do you have a better idea for the name?

This comment has been minimized.

@alexlarsson

alexlarsson May 28, 2014

Contributor

ExtraFiles?

@alexlarsson

alexlarsson May 28, 2014

Contributor

ExtraFiles?

This comment has been minimized.

@timthelion

timthelion May 28, 2014

Contributor

Well, I personally would name it ExtraFilesToAddToContainerAfterCreation but some might say I'm a bit of an extremist when it comes to naming ;)

@timthelion

timthelion May 28, 2014

Contributor

Well, I personally would name it ExtraFilesToAddToContainerAfterCreation but some might say I'm a bit of an extremist when it comes to naming ;)

This comment has been minimized.

@alexlarsson

alexlarsson May 28, 2014

Contributor

Turns out ExtraFiles already exist in the parent struct...

@alexlarsson

alexlarsson May 28, 2014

Contributor

Turns out ExtraFiles already exist in the parent struct...

runconfig/parse.go
@@ -82,6 +83,7 @@ func parseRun(cmd *flag.FlagSet, args []string, sysInfo *sysinfo.SysInfo) (*Conf
cmd.Var(&flDns, []string{"#dns", "-dns"}, "Set custom dns servers")
cmd.Var(&flDnsSearch, []string{"-dns-search"}, "Set custom dns search domains")
cmd.Var(&flVolumesFrom, []string{"#volumes-from", "-volumes-from"}, "Mount volumes from the specified container(s)")
+ cmd.Var(&flGrantSecrets, []string{"#grant-secrets", "-grant-secret"}, "Grant container access to named secret")

This comment has been minimized.

@timthelion

timthelion May 28, 2014

Contributor

I don't think you need the deprecation hash. Just:

    cmd.Var(&flGrantSecrets, []string{"-grant-secret"}, "Grant container access to named secret")

should suffice no?

@timthelion

timthelion May 28, 2014

Contributor

I don't think you need the deprecation hash. Just:

    cmd.Var(&flGrantSecrets, []string{"-grant-secret"}, "Grant container access to named secret")

should suffice no?

@@ -858,6 +867,11 @@ func NewDaemonFromDirectory(config *daemonconfig.Config, eng *engine.Engine) (*D
return nil, err
}
+ secrets, err := NewSecrets(config.Root, "/etc/docker/secrets")

This comment has been minimized.

@timthelion

timthelion May 28, 2014

Contributor

IIRC, @shykes wanted this in /var/lib/docker so that the serialization format would not look like a "public" stable API.

@timthelion

timthelion May 28, 2014

Contributor

IIRC, @shykes wanted this in /var/lib/docker so that the serialization format would not look like a "public" stable API.

This comment has been minimized.

@alexlarsson

alexlarsson May 28, 2014

Contributor

config,Root is in /var/lib/docker

@alexlarsson

alexlarsson May 28, 2014

Contributor

config,Root is in /var/lib/docker

This comment has been minimized.

@timthelion

timthelion May 28, 2014

Contributor

What is in /etc/docker/secrets then? I don't understand.

@timthelion

timthelion May 28, 2014

Contributor

What is in /etc/docker/secrets then? I don't understand.

This comment has been minimized.

@alexlarsson

alexlarsson May 28, 2014

Contributor

That is the host-based secrets. These are specific to the docker host rather than uploaded by the user. This is for things like inheriting host product entitlements.

@alexlarsson

alexlarsson May 28, 2014

Contributor

That is the host-based secrets. These are specific to the docker host rather than uploaded by the user. This is for things like inheriting host product entitlements.

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson May 28, 2014

Contributor

Update fixes the doc misspelling

Contributor

alexlarsson commented May 28, 2014

Update fixes the doc misspelling

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar May 29, 2014

Contributor

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

Contributor

cyphar commented May 29, 2014

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

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 2, 2014

Contributor

Updated to latest master

Contributor

alexlarsson commented Jun 2, 2014

Updated to latest master

daemon/container.go
+ return err
+ }
+ for _, s := range data {
+ container.command.CreateFiles[filepath.Join("/run/secrets", s.Name)] = s.Data

This comment has been minimized.

@proppy

proppy Jun 2, 2014

Contributor

did you consider /var/run or /var/tmp instead?

@proppy

proppy Jun 2, 2014

Contributor

did you consider /var/run or /var/tmp instead?

This comment has been minimized.

@alexlarsson

alexlarsson Jun 2, 2014

Contributor

/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

alexlarsson Jun 2, 2014

Contributor

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

@timthelion

This comment has been minimized.

Show comment
Hide comment
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 2, 2014

Contributor

Out of interest, why --grant-secret and not just --secret or --secrets?

Contributor

cyphar commented Jun 2, 2014

Out of interest, why --grant-secret and not just --secret or --secrets?

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 2, 2014

Contributor

@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 --secret would be better.

Contributor

alexlarsson commented Jun 2, 2014

@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 --secret would be better.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion Jun 2, 2014

Contributor

--grant-secret is better, less ambiguos.

Contributor

timthelion commented Jun 2, 2014

--grant-secret is better, less ambiguos.

@shykes

This comment has been minimized.

Show comment
Hide comment
@shykes

shykes Jun 4, 2014

Collaborator

+1 on --grant-secret

Collaborator

shykes commented Jun 4, 2014

+1 on --grant-secret

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jun 4, 2014

Contributor

@alexlarsson why can these files not be bind mounted?

Contributor

crosbymichael commented Jun 4, 2014

@alexlarsson why can these files not be bind mounted?

+ } else {
+ hostBased := ""
+ if out.GetBool("HostBased") {
+ hostBased = "*"

This comment has been minimized.

@vieux

vieux Jun 4, 2014

Collaborator

can we use [OK] instead of * like in docker search ?

/cc @crosbymichael @shykes

@vieux

vieux Jun 4, 2014

Collaborator

can we use [OK] instead of * like in docker search ?

/cc @crosbymichael @shykes

This comment has been minimized.

@alexlarsson

alexlarsson Jun 4, 2014

Contributor

Well, that signifies "trusted" to which "ok" seems to make sense. Here it is a bit more generic boolean. Maybe [x] ?

@alexlarsson

alexlarsson Jun 4, 2014

Contributor

Well, that signifies "trusted" to which "ok" seems to make sense. Here it is a bit more generic boolean. Maybe [x] ?

This comment has been minimized.

@cyphar

cyphar Jun 4, 2014

Contributor

Maybe [HOST] or just [H]?

@cyphar

cyphar Jun 4, 2014

Contributor

Maybe [HOST] or just [H]?

graph/graph.go
@@ -257,6 +257,7 @@ func SetupInitLayer(initLayer string) error {
"/dev/pts": "dir",
"/dev/shm": "dir",
"/proc": "dir",
+ "/run": "dir",

This comment has been minimized.

@crosbymichael

crosbymichael Jun 4, 2014

Contributor

Souldn't this be /run/secrets

@crosbymichael

crosbymichael Jun 4, 2014

Contributor

Souldn't this be /run/secrets

This comment has been minimized.

@alexlarsson

alexlarsson Jun 4, 2014

Contributor

Yeah

@alexlarsson

alexlarsson Jun 4, 2014

Contributor

Yeah

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 4, 2014

Contributor

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

Contributor

alexlarsson commented Jun 4, 2014

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

@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 4, 2014

Contributor

@crosbymichael Here is the alternative implementation using a tmpfs from the host bind-mounted in:
alexlarsson@d92b083

If you prefer that approach we can drop a bunch of the libcontainer commits when this is squashed in.

Contributor

alexlarsson commented Jun 4, 2014

@crosbymichael Here is the alternative implementation using a tmpfs from the host bind-mounted in:
alexlarsson@d92b083

If you prefer that approach we can drop a bunch of the libcontainer commits when this is squashed in.

alexlarsson added some commits May 23, 2014

cli: Allow multi-level command like docker foo list, and docker foo add
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)
Add secret store
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)
Add --grant-secret hostconfig option
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)
Add --grant-secret support to docker build
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)
API docs: add the new secret based requests
Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
docs: Add cli docs for the secret store
Docker-DCO-1.1-Signed-off-by: Alexander Larsson <alexl@redhat.com> (github: alexlarsson)
@alexlarsson

This comment has been minimized.

Show comment
Hide comment
@alexlarsson

alexlarsson Jun 16, 2014

Contributor

I rebased on master with the alternative approach from above.
Note, this depends on this PR for libcontainer to avoid a race condition: docker/libcontainer#22

Contributor

alexlarsson commented Jun 16, 2014

I rebased on master with the alternative approach from above.
Note, this depends on this PR for libcontainer to avoid a race condition: docker/libcontainer#22

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

This comment has been minimized.

@SvenDowideit

SvenDowideit Jun 16, 2014

Contributor

I presume you mean discretion :)

@SvenDowideit

SvenDowideit Jun 16, 2014

Contributor

I presume you mean discretion :)

@@ -1115,6 +1117,44 @@ It is used to create a backup that can then be used with
$ sudo docker save -o fedora-all.tar fedora
$ sudo docker save -o fedora-latest.tar fedora:latest

This comment has been minimized.

@SvenDowideit

SvenDowideit Jun 16, 2014

Contributor

There seems to be nothing telling the user how and when to use a granted secret. After reading for a while, I find out that a secret is a file, but I have no idea where in the build it turns up.

Is there a way for a Dockerfile to indicate a need for a secret? Can I use it to add a http_proxy setting at build time? What are the limitations? Can I copy the contents of the secret into the resulting image, thus risking exposing the secret to the world, or will it map to a location in the fs that i need it.

@SvenDowideit

SvenDowideit Jun 16, 2014

Contributor

There seems to be nothing telling the user how and when to use a granted secret. After reading for a while, I find out that a secret is a file, but I have no idea where in the build it turns up.

Is there a way for a Dockerfile to indicate a need for a secret? Can I use it to add a http_proxy setting at build time? What are the limitations? Can I copy the contents of the secret into the resulting image, thus risking exposing the secret to the world, or will it map to a location in the fs that i need it.

@vbatts

This comment has been minimized.

Show comment
Hide comment
Contributor

vbatts commented Jun 24, 2014

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Jun 24, 2014

Contributor

@vbatts @alexlarsson better, but still i don't think its enough. I would suggest adding a mention to the --grant-secrets cli summary; something like the selected secret file will be added in /run/secrets, and the docker build and docker run cli.md and man pages really do need to talk about what a secret is (it seems you've hinted that a secret is actually a single file, and that it will go into /run/secret, but you've not actually explicitly said so.

now to the harder part....

Does this mean /run/secret is now special, and any base image with that dir will lose access, or are you just inserting a file? Also... can I have more than one secret?

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.

Contributor

SvenDowideit commented Jun 24, 2014

@vbatts @alexlarsson better, but still i don't think its enough. I would suggest adding a mention to the --grant-secrets cli summary; something like the selected secret file will be added in /run/secrets, and the docker build and docker run cli.md and man pages really do need to talk about what a secret is (it seems you've hinted that a secret is actually a single file, and that it will go into /run/secret, but you've not actually explicitly said so.

now to the harder part....

Does this mean /run/secret is now special, and any base image with that dir will lose access, or are you just inserting a file? Also... can I have more than one secret?

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.

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion Jun 24, 2014

Contributor

/etc/docker/secrets is also completely undocumented.

Contributor

timthelion commented Jun 24, 2014

/etc/docker/secrets is also completely undocumented.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Jun 25, 2014

Contributor

@SvenDowideit @timthelion k. I'll work on this.

Contributor

vbatts commented Jun 25, 2014

@SvenDowideit @timthelion k. I'll work on this.

@vbatts

This comment has been minimized.

Show comment
Hide comment
Contributor

vbatts commented Jun 26, 2014

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Jun 26, 2014

Contributor

As a note, I've identified a couple of items regarding this feature that I will either fix or get clarity on.

  • docker secret add can not handle adding directories, but the /etc/docker/secrets handles directories fine
  • duplicate file names can not be added through docker secret add, but can exist if a duplicate is added to /etc/docker/secrets/ after the first is added through the cli
  • despite /run/secrets being create as 0700, once mounted tmpfs it's sticky world readable 1777 inside the container

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

Contributor

vbatts commented Jun 26, 2014

As a note, I've identified a couple of items regarding this feature that I will either fix or get clarity on.

  • docker secret add can not handle adding directories, but the /etc/docker/secrets handles directories fine
  • duplicate file names can not be added through docker secret add, but can exist if a duplicate is added to /etc/docker/secrets/ after the first is added through the cli
  • despite /run/secrets being create as 0700, once mounted tmpfs it's sticky world readable 1777 inside the container

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

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion Jun 26, 2014

Contributor

@vbatts I think that the existence of /etc/docker/secrets should really be reconsidered. When this was discussed at the a contributers IRC meeting which lead to this PR the idea of having /etc/docker/secrets was not really agreed upon. The whole point of this PR was supposed to be "to have something simple that can make it into 1.0". That is obviously a mute point now. It would be much better to have a docker secret set-group global command or something like that.

Contributor

timthelion commented Jun 26, 2014

@vbatts I think that the existence of /etc/docker/secrets should really be reconsidered. When this was discussed at the a contributers IRC meeting which lead to this PR the idea of having /etc/docker/secrets was not really agreed upon. The whole point of this PR was supposed to be "to have something simple that can make it into 1.0". That is obviously a mute point now. It would be much better to have a docker secret set-group global command or something like that.

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Jun 30, 2014

Contributor

are we moving to #6697, and thus should close this to avoid losing info due to split conversation?

Contributor

SvenDowideit commented Jun 30, 2014

are we moving to #6697, and thus should close this to avoid losing info due to split conversation?

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Jun 30, 2014

Contributor

@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 docker secret add --global foo.cert would be simpler and more understandable. Also, gating everything at add helps with the duplicate/conflicted name bug in the current implementation. And docker secret rm ... would still work the same. The issue becomes that "host secrets" implies an ACL that docker does not currently have full support of. Even adding a flag like --globabl so that the secret file is in all new containers, means that any user that is in the 'docker' group can add/rm with global secrets. Where as, as a sysadmin of the host I could ensure that this file is in all new containers, and a user in the 'docker' group could not remove that secret. This is a use case that is not easily done just-in-docker.

Contributor

vbatts commented Jun 30, 2014

@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 docker secret add --global foo.cert would be simpler and more understandable. Also, gating everything at add helps with the duplicate/conflicted name bug in the current implementation. And docker secret rm ... would still work the same. The issue becomes that "host secrets" implies an ACL that docker does not currently have full support of. Even adding a flag like --globabl so that the secret file is in all new containers, means that any user that is in the 'docker' group can add/rm with global secrets. Where as, as a sysadmin of the host I could ensure that this file is in all new containers, and a user in the 'docker' group could not remove that secret. This is a use case that is not easily done just-in-docker.

@pandrew pandrew referenced this pull request in boot2docker/boot2docker Jul 22, 2014

Open

How to keep the created user directory permanent ? #438

@erikh

This comment has been minimized.

Show comment
Hide comment
@erikh

erikh Aug 1, 2014

Contributor

The changes here in server/buildfile.go conflict with recent changes to this functionality. Can you rebase?

Contributor

erikh commented Aug 1, 2014

The changes here in server/buildfile.go conflict with recent changes to this functionality. Can you rebase?

@timthelion

This comment has been minimized.

Show comment
Hide comment
@timthelion

timthelion Aug 1, 2014

Contributor

@SvenDowideit Yes, please close this in favor of #6697, this is getting confusing :(

Contributor

timthelion commented Aug 1, 2014

@SvenDowideit Yes, please close this in favor of #6697, this is getting confusing :(

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Aug 1, 2014

Collaborator

Closing in favor of #6697 (which is carrying this PR)

Collaborator

tiborvass commented Aug 1, 2014

Closing in favor of #6697 (which is carrying this PR)

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