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

Secret Management #27794

Merged
merged 37 commits into from Nov 9, 2016

Conversation

@ehazlett
Contributor

ehazlett commented Oct 26, 2016

This adds support for the new Secret API in Swarmkit (docker/swarmkit#1567). This adds commands (create, inspect, ls, rm) for managing secrets in a Swarm cluster. This is currently for Swarm mode only as the backing store is Swarm and as such is only for Linux. This is the foundation for future secret support in Docker with potential improvements such as Windows support, different backing stores, etc.

This takes the ideas and collective recommendations from #13490 to set the groundwork. It exposes a file system based interface for secret access. The engine creates a tmpfs and mounts it in container (/run/secrets). A file is created for each secret that is assigned to the service. For example, if a secret named "ssh-dev" with a target of "id_rsa" is assigned to the service, a file with the secret contents will be created at /run/secrets/id_rsa.

TODO:

  • Service update support to add / rm secrets from a service
  • User facing docs
  • More tests
  • Depends on docker/swarmkit#1697

Examples (these will be turned into user docs):

Create a Secret:

$> cat test_key | docker secret create ssh-dev
qluh2o4xbvtu8rhvz0hueyeou

Create a Service with a Secret

$> docker service create --name nginx --secret ssh-dev nginx
t9kwy2v04l7zl3hl9dlvt5vq0

We can then check the created container for the secret:

$> docker exec -ti 06 mount | grep tmpfs
...
tmpfs on /run/secrets type tmpfs (ro,relatime)

$> docker exec -ti 06 cat /run/secrets/id_rsa
-----BEGIN RSA PRIVATE KEY-----
MIIEpAIBAAKCAQEA2Xz+yHs5pDgeCyv7ul10MkSYcAu79sRBh/VNGXBPMW1QbBcJ
...

Create with full options

$> docker service create --name nginx --secret source=ssh-dev,target=id_rsa,uid=1000,gid=1000,mode=0400 nginx
t9kwy2v04l7zl3hl9dlvt5vq0
Show outdated Hide outdated daemon/cluster/convert/container.go
Show outdated Hide outdated daemon/cluster/convert/container.go
Show outdated Hide outdated daemon/cluster/convert/secret.go
Show outdated Hide outdated daemon/cluster/convert/secret.go
Show outdated Hide outdated daemon/cluster/convert/secret.go
Show outdated Hide outdated daemon/cluster/executor/container/attachment.go
Show outdated Hide outdated daemon/container_operations_unix.go
Show outdated Hide outdated daemon/container_operations_unix.go
Show outdated Hide outdated daemon/container_operations_unix.go
Show outdated Hide outdated daemon/secrets.go
@cpuguy83

Having issues making this work:

$ echo foo=bar | docker secret create test
$ docker service create --name test --secret test:/foo busybox top
$ docker exec <cid> ls /foo

/foo does not exist.

I have concerns on supporting the format <name>:<target> which puts us in the same sticky situation we are in with -v on run.

Show outdated Hide outdated api/server/router/swarm/cluster_routes.go
Show outdated Hide outdated api/server/router/swarm/cluster_routes.go
Show outdated Hide outdated cli/command/service/parse.go
Show outdated Hide outdated daemon/container_operations_unix.go
Show outdated Hide outdated container/container_unix.go
Show outdated Hide outdated container/container_unix.go
ExecCommands *exec.Store `json:"-"`
HostConfig *containertypes.HostConfig `json:"-"` // do not serialize the host config in the json, otherwise we'll make the container unportable
ExecCommands *exec.Store `json:"-"`
Secrets []*containertypes.ContainerSecret `json:"-"` // do not serialize

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Don't like that we're shoving secrets in here purely for indexing, but I guess we are doing it for other things as well right now.

@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Don't like that we're shoving secrets in here purely for indexing, but I guess we are doing it for other things as well right now.

ctx, cancel := c.getRequestContext()
defer cancel()
r, err := c.node.client.GetSecret(ctx, &swarmapi.GetSecretRequest{SecretID: id})

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Doesn't work when a name is specified.

@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Doesn't work when a name is specified.

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Nor prefixes

@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Nor prefixes

This comment has been minimized.

@ehazlett

ehazlett Oct 26, 2016

Contributor

correct. swarmkit only supports getting by id atm

@ehazlett

ehazlett Oct 26, 2016

Contributor

correct. swarmkit only supports getting by id atm

This comment has been minimized.

@aaronlehmann

aaronlehmann Oct 27, 2016

Contributor

swarmkit can filter by name or name prefix using the ListSecrets RPC. This is the same as other object types like service and node - Get is always by ID.

@aaronlehmann

aaronlehmann Oct 27, 2016

Contributor

swarmkit can filter by name or name prefix using the ListSecrets RPC. This is the same as other object types like service and node - Get is always by ID.

This comment has been minimized.

@diogomonica

diogomonica Nov 3, 2016

Contributor

So we could implement a GetSecretByName that effectively uses ListSecrets with a name filter under it.

@diogomonica

diogomonica Nov 3, 2016

Contributor

So we could implement a GetSecretByName that effectively uses ListSecrets with a name filter under it.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Also sorry, I added some code review in there though it's still in design review.
I think overall the design is mostly good except for the format.

The other item would be only supporting stdin... but that's probably not a big deal... who knows for Windows though.

Contributor

cpuguy83 commented Oct 26, 2016

Also sorry, I added some code review in there though it's still in design review.
I think overall the design is mostly good except for the format.

The other item would be only supporting stdin... but that's probably not a big deal... who knows for Windows though.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Tests are also failing because of the linter.

Contributor

cpuguy83 commented Oct 26, 2016

Tests are also failing because of the linter.

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett Oct 26, 2016

Contributor

lint issues should be fixed.

@cpuguy83 the design was to have the SECRET_NAME:TARGET_NAME but I'm not tied to it. The intended use (sorry for the lack of docs atm) was to not have a path for the target so we can have a simple directory instead of nesting. The latest update checks and returns an error if a path is specified.

Contributor

ehazlett commented Oct 26, 2016

lint issues should be fixed.

@cpuguy83 the design was to have the SECRET_NAME:TARGET_NAME but I'm not tied to it. The intended use (sorry for the lack of docs atm) was to not have a path for the target so we can have a simple directory instead of nesting. The latest update checks and returns an error if a path is specified.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 26, 2016

Contributor

Aha! It goes in /run/secrets of course... This makes sense. I think I'm ok with the format in that case. It could still end up tricky though.

Contributor

cpuguy83 commented Oct 26, 2016

Aha! It goes in /run/secrets of course... This makes sense. I think I'm ok with the format in that case. It could still end up tricky though.

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett Oct 26, 2016

Contributor

@cpuguy83 I was possibly thinking something like the more granular mount syntax in the future but it might be too much for now. idk

Contributor

ehazlett commented Oct 26, 2016

@cpuguy83 I was possibly thinking something like the more granular mount syntax in the future but it might be too much for now. idk

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Oct 26, 2016

Contributor

@ehazlett looks like vendor needs to be updated.

Contributor

LK4D4 commented Oct 26, 2016

@ehazlett looks like vendor needs to be updated.

@vdemeester vdemeester added this to the 1.13.0 milestone Oct 26, 2016

@vdemeester

vdemeester requested changes Oct 26, 2016 edited

Few questions :

What should happen if I remove a secret that is mounted ? Right now it's still available in the container, that might be by design, just asking 👼.

On service create, there is a problem if I specify multiple secrets (using --secret) ; I get the same content for all the secrets.

$ docker service create sec1 <<EOF
A
EOF
$ docker secret create sec2 <<EOF
B
EOF
$ docker service create --name test --secret sec1 --secret sec2 busybox top
# Task container is 2b66…
$ docker exec -ti 2b66 sh
$ ls /run/secrets
sec1 sec2
$ cat /run/secrets/sec1
B
# should have be A
$ cat /run/secrets/sec2
B

On service update, it doesn't seems it updates the secret or re-create the task. Not sure what is the preferred way to go (I would think re-create the container), but as of now, it doesn't have any effect. — just saw the checkbox, nevermind 👼

$ docker service ps test 
NAME                              IMAGE    NODE          DESIRED STATE  CURRENT STATE           ERROR
test.1.imutqdjh4k2cm24e90mpkt9vh  busybox  4c780a8f3912  Running        Running 37 seconds ago
$ docker secret create sec3 <<EOF
C
EOF
$ docker service update --secret sec3 test
test
$ docker service ps test
NAME                              IMAGE    NODE          DESIRED STATE  CURRENT STATE          ERROR
test.1.imutqdjh4k2cm24e90mpkt9vh  busybox  4c780a8f3912  Running        Running 7 minutes ago
$ docker exec -ti 2b6 sh
$ ls /run/secrets
sec1 sec2

For docker secret subcommands (like rm or inspect), should we support prefixes (like on all container subcommands) and names ? right now it just supports the full ID.

Other than that, it looks good 👼 🐸

Show outdated Hide outdated api/server/router/swarm/cluster.go
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Oct 27, 2016

Contributor

On service create, there is a problem if I specify multiple secrets (using --secret) ; I get the same content for all the secrets.

That's probably because of this: https://github.com/docker/docker/pull/27794/files#r85236096

Contributor

aaronlehmann commented Oct 27, 2016

On service create, there is a problem if I specify multiple secrets (using --secret) ; I get the same content for all the secrets.

That's probably because of this: https://github.com/docker/docker/pull/27794/files#r85236096

// ContainerSecret represents a secret in a container. This gets realized
// in the container tmpfs
type ContainerSecret struct {

This comment has been minimized.

@stevvooe

stevvooe Nov 14, 2016

Contributor

Again, this repeats secretreference. Why aren't we using that type for what it is intended?

@stevvooe

stevvooe Nov 14, 2016

Contributor

Again, this repeats secretreference. Why aren't we using that type for what it is intended?

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Nov 14, 2016

Contributor

I'm still confused on the role of SecretRequestOption. Seems like this should just be SecretReference. Also, I am not sure if ContainerSecret is the right model. Don't we have a secret store to hide secret data?

Contributor

stevvooe commented Nov 14, 2016

I'm still confused on the role of SecretRequestOption. Seems like this should just be SecretReference. Also, I am not sure if ContainerSecret is the right model. Don't we have a secret store to hide secret data?

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett Nov 14, 2016

Contributor

We need a way to define the secret on the container. Since we can't call the secret store direct from the engine side I chose to implement it here and have the cluster define it as discussed during design.

ContainerSecret does repeat most of the SecretReference however it also needs data. I chose to keep them separate instead of mix the types between swarm and the container types. I could embed if that's better but I felt it made a clear separation. This is the same for SecretRequestOption. This is similar to things like ContainerAttachConfig and ContainerAttachOptions where there are two fields different.

Contributor

ehazlett commented Nov 14, 2016

We need a way to define the secret on the container. Since we can't call the secret store direct from the engine side I chose to implement it here and have the cluster define it as discussed during design.

ContainerSecret does repeat most of the SecretReference however it also needs data. I chose to keep them separate instead of mix the types between swarm and the container types. I could embed if that's better but I felt it made a clear separation. This is the same for SecretRequestOption. This is similar to things like ContainerAttachConfig and ContainerAttachOptions where there are two fields different.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Nov 14, 2016

Contributor

@ehazlett ContainerSecret mixes the concepts of reference and secret.

In general, these types should be isomorphic to swarm types. We can share types with swarm types, just fine. We do this for mounts.

Contributor

stevvooe commented Nov 14, 2016

@ehazlett ContainerSecret mixes the concepts of reference and secret.

In general, these types should be isomorphic to swarm types. We can share types with swarm types, just fine. We do this for mounts.

@xeor

This comment has been minimized.

Show comment
Hide comment
@xeor

xeor Nov 16, 2016

Is the design of the secret management allowing for future features like one-time-read options? Or is the design not allowing that kind of extensions of dynamically adding/removing secrets in a secure way?

xeor commented Nov 16, 2016

Is the design of the secret management allowing for future features like one-time-read options? Or is the design not allowing that kind of extensions of dynamically adding/removing secrets in a secure way?

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Nov 16, 2016

Contributor

Is the design of the secret management allowing for future features like one-time-read options? Or is the design not allowing that kind of extensions of dynamically adding/removing secrets in a secure way?

The design of the secrets functionality in this PR does not preclude such future features. Many of the existing features were designed with these use cases in mind for the future.

Contributor

stevvooe commented Nov 16, 2016

Is the design of the secret management allowing for future features like one-time-read options? Or is the design not allowing that kind of extensions of dynamically adding/removing secrets in a secure way?

The design of the secrets functionality in this PR does not preclude such future features. Many of the existing features were designed with these use cases in mind for the future.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 16, 2016

Member

opened #28516 for tracking the remaining review comments

Member

thaJeztah commented Nov 16, 2016

opened #28516 for tracking the remaining review comments

@icecrime icecrime referenced this pull request Nov 17, 2016

Closed

[UX] Guidelines for short/long syntax flags #28527

2 of 3 tasks complete

tianon added a commit to infosiftr/stackbrew that referenced this pull request Nov 17, 2016

Update docker-library images
- `cassandra`: 3.0.10
- `docker`: 1.13.0-rc1 (currently failing the `override-cmd` test; see moby/moby#28329), `docker daemon` -> `dockerd` (docker-library/docker#31)
- `drupal`: 8.2.3
- `java`: alpine 7.121.2.6.8-r0, 8.111.14-r0
- `openjdk`: alpine 7.121.2.6.8-r0, 8.111.14-r0
- `python`: 3.6.0b3 (docker-library/python#157), pip 9.0.1 (docker-library/python#154)
- `ruby`: 2.3.2, 2.2.6
- `tomcat`: 6.0.48, 7.0.73, 8.0.39, 8.5.8, 9.0.0.M13
- `wordpress`: add `WORDPRESS_*_FILE` support for passing parameters via files especially for [Docker secrets](moby/moby#27794) (docker-library/wordpress#186)
@RRAlex

This comment has been minimized.

Show comment
Hide comment
@RRAlex

RRAlex Nov 18, 2016

@jefflill You might also want to make sure your second tmpfs is mounted securely, with rw,noexec,nosuid,nodev,size=1m
Hopefully all of those can also be passed to a service (I didn't verify).

Short term, I also wish the /run/secrets/ could have an option to simply o-rwx the directory (since it's read-only and we can't chmod it).
That could be simpler & easier to implement, until we get secrets that are read once, etc.
A root user could set things up before dropping its privileges (to a user), for example...

That said, read once sounds a lot better... :)

RRAlex commented Nov 18, 2016

@jefflill You might also want to make sure your second tmpfs is mounted securely, with rw,noexec,nosuid,nodev,size=1m
Hopefully all of those can also be passed to a service (I didn't verify).

Short term, I also wish the /run/secrets/ could have an option to simply o-rwx the directory (since it's read-only and we can't chmod it).
That could be simpler & easier to implement, until we get secrets that are read once, etc.
A root user could set things up before dropping its privileges (to a user), for example...

That said, read once sounds a lot better... :)

@jefflill

This comment has been minimized.

Show comment
Hide comment
@jefflill

jefflill Nov 18, 2016

Actually, I've verified that opts/mount: add tmpfs-specific options
#28150 works in recent 1.13 builds.

This is super clean now.

--Jeff

On Fri, Nov 18, 2016 at 7:30 AM, Alexandre Guédon notifications@github.com
wrote:

@jefflill https://github.com/jefflill You might also want to make sure
your second tmpfs is mounted securely, with rw,noexec,nosuid,nodev,size=1m
Hopefully all of those can also be passed to a service (I didn't verify).

Short term, I also wish the /run/secrets/ could have an option to simply
o-rwx the directory (since it's read-only and we can't chmod it).
That could be simpler & easier to implement, until we get secrets that are
read once, etc.
A root user could set things up before dropping its privileges (to a
user), for example...

That said, read once sounds a lot better... :)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27794 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AErlj8NdrY9xZ3ZpKoOPpVk3chpi6wtmks5q_cSXgaJpZM4Khe0a
.

jefflill commented Nov 18, 2016

Actually, I've verified that opts/mount: add tmpfs-specific options
#28150 works in recent 1.13 builds.

This is super clean now.

--Jeff

On Fri, Nov 18, 2016 at 7:30 AM, Alexandre Guédon notifications@github.com
wrote:

@jefflill https://github.com/jefflill You might also want to make sure
your second tmpfs is mounted securely, with rw,noexec,nosuid,nodev,size=1m
Hopefully all of those can also be passed to a service (I didn't verify).

Short term, I also wish the /run/secrets/ could have an option to simply
o-rwx the directory (since it's read-only and we can't chmod it).
That could be simpler & easier to implement, until we get secrets that are
read once, etc.
A root user could set things up before dropping its privileges (to a
user), for example...

That said, read once sounds a lot better... :)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27794 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AErlj8NdrY9xZ3ZpKoOPpVk3chpi6wtmks5q_cSXgaJpZM4Khe0a
.

@shouze

This comment has been minimized.

Show comment
Hide comment
@shouze

shouze Dec 14, 2016

Contributor

@ehazlett wow it's pretty cool ! Was working on a vault integration of our own when I've seen that.

As the feature comes from swarmkit, could we imagine to have it in non swarm docker use (docker for mac, docker engine)?

And what about docker-compose support? There's already a discussion opened about secrets docker/compose#1534

To wum up, I would be very pleased to be able to use secrets on my local machine/ci/... the same way than on the big cluster ;)

Contributor

shouze commented Dec 14, 2016

@ehazlett wow it's pretty cool ! Was working on a vault integration of our own when I've seen that.

As the feature comes from swarmkit, could we imagine to have it in non swarm docker use (docker for mac, docker engine)?

And what about docker-compose support? There's already a discussion opened about secrets docker/compose#1534

To wum up, I would be very pleased to be able to use secrets on my local machine/ci/... the same way than on the big cluster ;)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 14, 2016

Member

@shouze this all works on docker for mac, but currently only for "services" (so if you have swarm mode enabled (docker swarm init). Future enhancements to allow it to be used for docker run, and docker build may be coming in future releases.

Member

thaJeztah commented Dec 14, 2016

@shouze this all works on docker for mac, but currently only for "services" (so if you have swarm mode enabled (docker swarm init). Future enhancements to allow it to be used for docker run, and docker build may be coming in future releases.

@shouze

This comment has been minimized.

Show comment
Hide comment
@shouze

shouze Dec 14, 2016

Contributor

@thaJeztah ho nice! I've just tested it, very very nice ;) I guess that when enhancements in docker run/build will come it will also be integrated with docker compose?

Contributor

shouze commented Dec 14, 2016

@thaJeztah ho nice! I've just tested it, very very nice ;) I guess that when enhancements in docker run/build will come it will also be integrated with docker compose?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 14, 2016

Member

@shouze docker-compose is a separate project, but I think that would be a reasonable assumption 😄 FWIW; @ehazlett started on a PR for build-time secrets; #28079, but it needed more time so couldn't make it into 1.13; for 1.14 it will probably need a rewrite, so it was closed in the meantime.

Member

thaJeztah commented Dec 14, 2016

@shouze docker-compose is a separate project, but I think that would be a reasonable assumption 😄 FWIW; @ehazlett started on a PR for build-time secrets; #28079, but it needed more time so couldn't make it into 1.13; for 1.14 it will probably need a rewrite, so it was closed in the meantime.

@nasskach

This comment has been minimized.

Show comment
Hide comment
@nasskach

nasskach Dec 21, 2016

Hi,

Sorry for the question but does the secrets are stored in an encrypted way ?

nasskach commented Dec 21, 2016

Hi,

Sorry for the question but does the secrets are stored in an encrypted way ?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 21, 2016

Member

@nasskach yes; see the (work in progress) documentation; https://github.com/docker/docker.github.io/pull/568/files

Member

thaJeztah commented Dec 21, 2016

@nasskach yes; see the (work in progress) documentation; https://github.com/docker/docker.github.io/pull/568/files

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