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

Support Custom Secret Targets #32571

Merged
merged 4 commits into from May 10, 2017

Conversation

@ehazlett
Contributor

ehazlett commented Apr 12, 2017

This adds support for custom secret target paths lifting the restriction of limiting secrets to /var/run. Here is an example:

$> docker service create --name test --secret source=app,target=/etc/app -t alpine ash
# brace yourself for this one-liner...  yolo
$> docker exec -ti $(docker inspect --format '{{.Status.ContainerStatus.ContainerID}}' $(docker service ps --format '{{.ID}}' test)) mount | grep tmpfs | grep app
tmpfs on /etc/app type tmpfs (ro,relatime)
$> docker exec -ti $(docker inspect --format '{{.Status.ContainerStatus.ContainerID}}' $(docker service ps --format '{{.ID}}' test)) cat /etc/app
TESTING

Refs #32336 (comment)
Depends on docker/swarmkit#2118

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett
Contributor

ehazlett commented Apr 12, 2017

/cc @tianon @thaJeztah @diogomonica

Show outdated Hide outdated vendor.conf Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 14, 2017

Member

Design looks good to me 👍 Moving this to code review, but let me know if there's objections 😄

Member

thaJeztah commented Apr 14, 2017

Design looks good to me 👍 Moving this to code review, but let me know if there's objections 😄

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 14, 2017

Contributor

It still looks like this will break when two secrets have the same target base name.

Contributor

aaronlehmann commented Apr 14, 2017

It still looks like this will break when two secrets have the same target base name.

@thaJeztah thaJeztah added this to the 17.06 milestone Apr 25, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 27, 2017

Contributor

@ehazlett: I'd suggest this borrow the code from configs in #32336. That stores the config under the full target path, so there isn't a risk of collisions from targets that share the same basename. Also, it would be good for the config and secret implementations to stay consistent.

Let me know if it would be helpful for me to carry this PR.

Contributor

aaronlehmann commented Apr 27, 2017

@ehazlett: I'd suggest this borrow the code from configs in #32336. That stores the config under the full target path, so there isn't a risk of collisions from targets that share the same basename. Also, it would be good for the config and secret implementations to stay consistent.

Let me know if it would be helpful for me to carry this PR.

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett Apr 27, 2017

Contributor

Ya if you want to carry that's fine with me.

Contributor

ehazlett commented Apr 27, 2017

Ya if you want to carry that's fine with me.

@thaJeztah thaJeztah added this to backlog in maintainers-session Apr 27, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 28, 2017

Contributor

Force-pushed to rebase and change the "local" storage paths to use the full target path instead of just the basename (which could collide).

Tested manually:

root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker secret create simple -
simple
wvgodvdsfnu1r9jj06kqiijmy
root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker secret create secret2 -
secret2
xjdubg4uatd025z9bctmejwxo
root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker service create --name top --secret simple --secret source=secret2,target=/path/to/secret busybox top
xoiziqqmqq2rlskffqkiccr5x
Since --detach=false was not specified, tasks will be created in the background.
In a future release, --detach=false will become the default.
root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
bc7e8dbbcb7a        busybox:latest      "top"               4 seconds ago       Up 2 seconds                            top.1.i224djxoo17c7twruoaasigl9
root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker exec -ti bc7e8 sh
/ # ls /run/secrets
simple
/ # cat /run/secrets/simple
simple
/ # cat /path/to/secret
secret2
/ #

When I have a chance (either later today or early next week), I'll expand the automated tests to check that the files actually exist inside the container in the right places, with the right content, using both relative and absolute paths. I'll also improve the tests to cover invalid targets and referencing the same secret twice under different targets.

Contributor

aaronlehmann commented Apr 28, 2017

Force-pushed to rebase and change the "local" storage paths to use the full target path instead of just the basename (which could collide).

Tested manually:

root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker secret create simple -
simple
wvgodvdsfnu1r9jj06kqiijmy
root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker secret create secret2 -
secret2
xjdubg4uatd025z9bctmejwxo
root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker service create --name top --secret simple --secret source=secret2,target=/path/to/secret busybox top
xoiziqqmqq2rlskffqkiccr5x
Since --detach=false was not specified, tasks will be created in the background.
In a future release, --detach=false will become the default.
root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
bc7e8dbbcb7a        busybox:latest      "top"               4 seconds ago       Up 2 seconds                            top.1.i224djxoo17c7twruoaasigl9
root@4d4d089ce2bf:/go/src/github.com/docker/docker# docker exec -ti bc7e8 sh
/ # ls /run/secrets
simple
/ # cat /run/secrets/simple
simple
/ # cat /path/to/secret
secret2
/ #

When I have a chance (either later today or early next week), I'll expand the automated tests to check that the files actually exist inside the container in the right places, with the right content, using both relative and absolute paths. I'll also improve the tests to cover invalid targets and referencing the same secret twice under different targets.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 29, 2017

Contributor

I've updated the vendoring, as the upstream swarmkit PR was merged.

I added test coverage for the following:

  • Verify the presence and contents of secrets with simple, relative, and absolute targets
  • Ensure that directory traversal is prevented
  • Check that a service can reference the same secret multiple times under different targets

I also added corresponding tests to the configs PR (#32336).

PTAL

Contributor

aaronlehmann commented Apr 29, 2017

I've updated the vendoring, as the upstream swarmkit PR was merged.

I added test coverage for the following:

  • Verify the presence and contents of secrets with simple, relative, and absolute targets
  • Ensure that directory traversal is prevented
  • Check that a service can reference the same secret multiple times under different targets

I also added corresponding tests to the configs PR (#32336).

PTAL

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Apr 29, 2017

Thanks @aaronlehmann!

ping @cyli @riyazdf ptal

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 29, 2017

Contributor

TestSwarmLockUnlockCluster appears to have hung on powerpc and z. I'd guess this is related to the swarmkit vendoring rather than the specific changes in this PR. We're working on getting an etcd/raft issue fixed upstream - this might be related.

Contributor

aaronlehmann commented Apr 29, 2017

TestSwarmLockUnlockCluster appears to have hung on powerpc and z. I'd guess this is related to the swarmkit vendoring rather than the specific changes in this PR. We're working on getting an etcd/raft issue fixed upstream - this might be related.

@diogomonica

Couple of questions:

  • Should we even support secret creation with targets that have potential OS separators?
  • Should we list all secrets in /var/run/secrets even if the target is outside of base secrets dir? Would have the advantage that a ls /var/run/secrets/ always returns all the secrets available.
  • Should we allow someone to add a target into /var/run/secrets/? In particular concerned that the validation of multiple targets being set somehow doesn't handle this new way of adding targets and the behavior will be unknown or the task unschedulable.
  • Are there any files/paths we want to reject outright from being overwritten? (like anything in /proc/?)

Thanks for carrying this PR @aaronlehmann

Show outdated Hide outdated container/container.go Outdated
Show outdated Hide outdated container/container_unix.go Outdated
Show outdated Hide outdated integration-cli/docker_cli_service_create_test.go Outdated
@@ -53,10 +52,6 @@ func (o *SecretOpt) Set(value string) error {
case "source", "src":
options.SecretName = value
case "target":
tDir, _ := filepath.Split(value)

This comment has been minimized.

@diogomonica

diogomonica Apr 29, 2017

Contributor

Why are we fine with a target being a directory? This still doesn't make sense right?

@diogomonica

diogomonica Apr 29, 2017

Contributor

Why are we fine with a target being a directory? This still doesn't make sense right?

This comment has been minimized.

@aaronlehmann

aaronlehmann Apr 30, 2017

Contributor

The client code has no way of knowing whether a path is a file or a directory.

This old code was essentially checking that there was no path separator. It is being removed because now path separators are allowed.

@aaronlehmann

aaronlehmann Apr 30, 2017

Contributor

The client code has no way of knowing whether a path is a file or a directory.

This old code was essentially checking that there was no path separator. It is being removed because now path separators are allowed.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 4, 2017

Contributor

Fixed nits

Contributor

aaronlehmann commented May 4, 2017

Fixed nits

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 4, 2017

Contributor

I've pushed an update that uses the secret ID instead of the secret target as the filename for storing the secret on the host's tmpfs mount.

Contributor

aaronlehmann commented May 4, 2017

I've pushed an update that uses the secret ID instead of the secret target as the filename for storing the secret on the host's tmpfs mount.

@cyli

This comment has been minimized.

Show comment
Hide comment
@cyli

cyli May 4, 2017

Contributor

Thanks @aaronlehmann! I could not think of any issues allowing a secret to escape /run/secrets inside a container, since we already allow absolute paths inside the container, whomever provides the target can get to any path anyway.

I don't think users should be providing traversals from /run/secrets rather than absolute paths, but other than them confusing themselves, I don't think it presents any vulnerability if we're just using IDs (which we control) on the host.

Thinking about it more, it may be nice if we could prevent path traversals at all on service create but since as you mentioned that could be tricky, I'm not sure it's worth the effort, but would appreciate others giving this line of reasoning a sanity check :)

LGTM on green.

Contributor

cyli commented May 4, 2017

Thanks @aaronlehmann! I could not think of any issues allowing a secret to escape /run/secrets inside a container, since we already allow absolute paths inside the container, whomever provides the target can get to any path anyway.

I don't think users should be providing traversals from /run/secrets rather than absolute paths, but other than them confusing themselves, I don't think it presents any vulnerability if we're just using IDs (which we control) on the host.

Thinking about it more, it may be nice if we could prevent path traversals at all on service create but since as you mentioned that could be tricky, I'm not sure it's worth the effort, but would appreciate others giving this line of reasoning a sanity check :)

LGTM on green.

@thaJeztah thaJeztah removed this from backlog in maintainers-session May 4, 2017

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica May 5, 2017

Contributor

@cyli @aaronlehmann since we don't see any security issues and this would just be a hygienic decision (no transversal), it would probably just be best effort.

I also agree that we shouldn't allow relative paths at all. Unless someone can come up with a good use case.

Contributor

diogomonica commented May 5, 2017

@cyli @aaronlehmann since we don't see any security issues and this would just be a hygienic decision (no transversal), it would probably just be best effort.

I also agree that we shouldn't allow relative paths at all. Unless someone can come up with a good use case.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 8, 2017

Contributor

Rebased

Contributor

aaronlehmann commented May 8, 2017

Rebased

aaronlehmann added a commit to aaronlehmann/cli that referenced this pull request May 9, 2017

[WIP] Support Custom Secret Targets
CLI counterpart to moby/moby#32571. Just
involves vendoring github.com/docker/docker/opts.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 9, 2017

Contributor

Corresponding CLI PR in docker/cli#50 (really just vendoring opts, which is a bit odd...)

Contributor

aaronlehmann commented May 9, 2017

Corresponding CLI PR in docker/cli#50 (really just vendoring opts, which is a bit odd...)

@johnstep

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 9, 2017

Contributor
Handler for DELETE /v1.30/containers/9e returned error: Unable to remove filesystem for 9e6c79a949b080473b9d4778f6eb9b3c9fb06a142260912457d57306a3e6d476: remove /var/lib/docker/containers/9e6c79a949b080473b9d4778f6eb9b3c9fb06a142260912457d57306a3e6d476/secrets: device or resource busy

This seems to happen with any secret target right now.

And I end up with leaked mounts:

tmpfs on /var/lib/docker/containers/67c604df32844744fea5d2613dc850d1c85227fc0a9406f4008938b4975358e4/secrets type tmpfs (rw,nosuid,nodev,noexec,relatime)
Contributor

cpuguy83 commented May 9, 2017

Handler for DELETE /v1.30/containers/9e returned error: Unable to remove filesystem for 9e6c79a949b080473b9d4778f6eb9b3c9fb06a142260912457d57306a3e6d476: remove /var/lib/docker/containers/9e6c79a949b080473b9d4778f6eb9b3c9fb06a142260912457d57306a3e6d476/secrets: device or resource busy

This seems to happen with any secret target right now.

And I end up with leaked mounts:

tmpfs on /var/lib/docker/containers/67c604df32844744fea5d2613dc850d1c85227fc0a9406f4008938b4975358e4/secrets type tmpfs (rw,nosuid,nodev,noexec,relatime)
Show outdated Hide outdated container/container_unix.go Outdated
Show outdated Hide outdated container/container_unix.go Outdated
Show outdated Hide outdated container/container_unix.go Outdated
Show outdated Hide outdated container/container_unix.go Outdated

ehazlett and others added some commits Apr 11, 2017

support custom paths for secrets
This adds support to specify custom container paths for secrets.

Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Use "local" secret paths based on the secretID
This prevents targets with the same basename from colliding.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Extend test coverage of secrets
Actually look inside the container to see if the secret data is present
and correct. Test absolute paths, relative paths, and just a basename.
Test the scenario where a service references the same secret under
different targets.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Use forked version of CLI for tests
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

I've reverted all changes to UnmountSecrets. I think these were incorrect to begin with. The individual secret mounts are passed on to runc and should be cleaned up by runc. All that's necessary is unmounting the tmpfs on the host.

PTAL

Contributor

aaronlehmann commented May 10, 2017

I've reverted all changes to UnmountSecrets. I think these were incorrect to begin with. The individual secret mounts are passed on to runc and should be cleaned up by runc. All that's necessary is unmounting the tmpfs on the host.

PTAL

DOCKERCLI_REPO=https://github.com/docker/cli
DOCKERCLI_COMMIT=c3648a9c9400d45524cc71b8fca4085b192c626f
DOCKERCLI_REPO=https://github.com/aaronlehmann/cli
DOCKERCLI_COMMIT=fd5a5910409fe5ed862b29de45a66f2bf8056894

This comment has been minimized.

@cpuguy83

cpuguy83 May 10, 2017

Contributor

Need to change these back before merge.

@cpuguy83

cpuguy83 May 10, 2017

Contributor

Need to change these back before merge.

This comment has been minimized.

@aaronlehmann

aaronlehmann May 10, 2017

Contributor

They can't be changed back before the merge. This would cause CI to fail - unless you think the CLI PR should be merged before this one (but that seems backwards).

These circular dependencies are very hard to deal with.

@aaronlehmann

aaronlehmann May 10, 2017

Contributor

They can't be changed back before the merge. This would cause CI to fail - unless you think the CLI PR should be merged before this one (but that seems backwards).

These circular dependencies are very hard to deal with.

This comment has been minimized.

@johnstep

johnstep May 10, 2017

Contributor

Don't we need to keep this until docker/cli#50 is merged?

@johnstep

johnstep May 10, 2017

Contributor

Don't we need to keep this until docker/cli#50 is merged?

This comment has been minimized.

@cpuguy83

cpuguy83 May 10, 2017

Contributor

We can move the tests to a separate PR.

Opts package should really not be in this repo.
We're only using a couple of functions from it for cmd/dockerd.

@cpuguy83

cpuguy83 May 10, 2017

Contributor

We can move the tests to a separate PR.

Opts package should really not be in this repo.
We're only using a couple of functions from it for cmd/dockerd.

This comment has been minimized.

@aaronlehmann

aaronlehmann May 10, 2017

Contributor

The plan I discussed with @tiborvass and @thaJeztah was to merge this PR with the DOCKERCLI_REPO change intact, then change this back to the main repo in a followup.

@aaronlehmann

aaronlehmann May 10, 2017

Contributor

The plan I discussed with @tiborvass and @thaJeztah was to merge this PR with the DOCKERCLI_REPO change intact, then change this back to the main repo in a followup.

This comment has been minimized.

@cpuguy83

cpuguy83 May 10, 2017

Contributor

Really even the opts change could be made on it's own since the existing code is not valid as is.

@cpuguy83

cpuguy83 May 10, 2017

Contributor

Really even the opts change could be made on it's own since the existing code is not valid as is.

This comment has been minimized.

@cpuguy83

cpuguy83 May 10, 2017

Contributor

@aaronlehmann That's fine.

@cpuguy83

cpuguy83 May 10, 2017

Contributor

@aaronlehmann That's fine.

@cpuguy83

LGTM

@cpuguy83 cpuguy83 merged commit 08fdce7 into moby:master May 10, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 33935 has succeeded
Details
janky Jenkins build Docker-PRs 42532 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 2901 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13773 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2638 has succeeded
Details
@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett May 10, 2017

Contributor

FWIW tested on my end again and LGTM

Contributor

ehazlett commented May 10, 2017

FWIW tested on my end again and LGTM

aaronlehmann added a commit to aaronlehmann/cli that referenced this pull request May 10, 2017

Support Custom Secret Targets
CLI counterpart to moby/moby#32571. Just
involves vendoring github.com/docker/docker/opts.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request May 19, 2017

Support Custom Secret Targets
CLI counterpart to moby/moby#32571. Just
involves vendoring github.com/docker/docker/opts.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Upstream-commit: 978aa7ede06b347629f9ac5b422cf1b723580e59
Component: cli

@ehazlett ehazlett deleted the ehazlett:secret-targets branch Jun 7, 2017

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