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

Configuration files for services #32336

Merged
merged 6 commits into from May 11, 2017

Conversation

@aaronlehmann
Contributor

aaronlehmann commented Apr 4, 2017

This adds a docker config subcommand and related APIs that allows "config" objects to be attached to services. Config files can be mounted inside services' containers, avoiding the need to bake configuration into images.

Configuration files are similar to secrets, and in fact the CLI and API show few differences between the two. The principal differences so far are:

  • Secrets are always redacted at the API level, so the payload cannot be obtained through an API call after they are created.
  • Secrets are restricted to the /run/secrets directory inside the container, as a design choice. Config files can be mounted anywhere.

We are expecting to introduce some additional features for configs in the future that may cause the feature to diverge further from secrets. Although secrets and configs are very similar right now, we decided that creating a parallel API and subcommand was better than overloading secrets for use by things that do not have the same confidentiality requirements.

I suspect one thing that will come up in the review process is duplication of code between secrets and configs. This is definitely an issue, and I would have liked to reduce the level of duplication. However, with separate APIs and CLI subcommands, a certain amount of code duplication is unavoidable. Since the APIs use different types, and Go doesn't support generics, it's difficult to reuse code between the two implementations. Also, I didn't want to prematurely refactor things, since it's very possible that configs and secrets will diverge more soon (perhaps even during the review process). But if there are any suggestions for easy wins on reducing duplicated code, they are certainly welcome.

This feature is wanted for 17.05.

Marked WIP because this depends on docker/swarmkit#2036 (which should be merged shortly).

ping @dhiltgen @dnephin @aluzzardi @diogomonica

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 5, 2017

Member

I would rather see secrets implemented as a --secret flag on config to enable the "secret behaviour". Having two objects that are nearly identical is really unfortunate. I'm not sure what we do about this now that secrets already exist. Maybe docker secrets becomes a hidden alias for docker config which sets that flag?

Secrets are always redacted at the API level, so the payload cannot be obtained through an API call after they are created.

Unless I run this ?

docker service create --secret foo --name printsecrets alpine:3.5 cat /run/secrets/foo
docker service logs printsecrets

Secrets are restricted to the /run/secrets directory inside the container, as a design choice. Config files can be mounted anywhere.

Do you know where I can learn more about this design choice?

Member

dnephin commented Apr 5, 2017

I would rather see secrets implemented as a --secret flag on config to enable the "secret behaviour". Having two objects that are nearly identical is really unfortunate. I'm not sure what we do about this now that secrets already exist. Maybe docker secrets becomes a hidden alias for docker config which sets that flag?

Secrets are always redacted at the API level, so the payload cannot be obtained through an API call after they are created.

Unless I run this ?

docker service create --secret foo --name printsecrets alpine:3.5 cat /run/secrets/foo
docker service logs printsecrets

Secrets are restricted to the /run/secrets directory inside the container, as a design choice. Config files can be mounted anywhere.

Do you know where I can learn more about this design choice?

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Apr 5, 2017

Contributor

@dnephin there are a lot of things packed in there. Ultimately we want to make sure people deal with sensitive data in a different way than they deal with configs.

  • Having two distinct cmd-line arguments and two distinct APIs allows us to separate the roles of managing secrets from managing configs. It's easy to imagine a team being in charge of secrets and another team just being responsible for their own configs.
  • Making secrets restrictive where they can be found creates consistency of where they live across containers, and allows security teams to reason better about permissions/access to sensitive material inside of the container.
  • Configs will diverge significantly from secrets. It doesn't make a lot of sense to template a secret, but it makes sense to template a config, for example. Also it allows us to have different experiences to deal with sensitive data (secret data is never returned to the user after set), than we have with configs (you can always inspect the material inside.

I agree that a lot of these reasons are nuanced, but we feel like separating them is the right way to move forward.

Contributor

diogomonica commented Apr 5, 2017

@dnephin there are a lot of things packed in there. Ultimately we want to make sure people deal with sensitive data in a different way than they deal with configs.

  • Having two distinct cmd-line arguments and two distinct APIs allows us to separate the roles of managing secrets from managing configs. It's easy to imagine a team being in charge of secrets and another team just being responsible for their own configs.
  • Making secrets restrictive where they can be found creates consistency of where they live across containers, and allows security teams to reason better about permissions/access to sensitive material inside of the container.
  • Configs will diverge significantly from secrets. It doesn't make a lot of sense to template a secret, but it makes sense to template a config, for example. Also it allows us to have different experiences to deal with sensitive data (secret data is never returned to the user after set), than we have with configs (you can always inspect the material inside.

I agree that a lot of these reasons are nuanced, but we feel like separating them is the right way to move forward.

@aaronlehmann aaronlehmann changed the title from [WIP] Configuration files for services to Configuration files for services Apr 5, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 5, 2017

Contributor

docker/swarmkit#2036 was merged upstream. Updated to incorporate proper swarmkit vendor. No longer marked WIP.

Contributor

aaronlehmann commented Apr 5, 2017

docker/swarmkit#2036 was merged upstream. Updated to incorporate proper swarmkit vendor. No longer marked WIP.

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

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 6, 2017

Member

secret data is never returned to the user after set

Sure it is, we've just made it slightly inconvenient.

docker service create --secret foo --name printsecrets alpine:3.5 cat /run/secrets/foo
docker service logs printsecrets
Member

dnephin commented Apr 6, 2017

secret data is never returned to the user after set

Sure it is, we've just made it slightly inconvenient.

docker service create --secret foo --name printsecrets alpine:3.5 cat /run/secrets/foo
docker service logs printsecrets

@thaJeztah thaJeztah moved this from backlog to Revisit in maintainers-session Apr 6, 2017

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Apr 6, 2017

Contributor

@dnephin I think you would agree those are separate concerns. Not allowing accidental secret retrieval by using the API (specially on the list secrets endpoint, which would return all secrets in plaintext), is not the same thing as a user intentionally deploying services with legitimate secret access to exfiltrate them.

Contributor

diogomonica commented Apr 6, 2017

@dnephin I think you would agree those are separate concerns. Not allowing accidental secret retrieval by using the API (specially on the list secrets endpoint, which would return all secrets in plaintext), is not the same thing as a user intentionally deploying services with legitimate secret access to exfiltrate them.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 6, 2017

Contributor

I've rebased and marked configs as an experimental feature. The API endpoints, CLI subcommand, flags for service create / service update, and integration tests only run in experimental mode. PTAL

Contributor

aaronlehmann commented Apr 6, 2017

I've rebased and marked configs as an experimental feature. The API endpoints, CLI subcommand, flags for service create / service update, and integration tests only run in experimental mode. PTAL

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 7, 2017

Member

@dnephin I think you would agree those are separate concerns.

Sorry, separate from what? I was responding to this statement "secret data is never returned to the user after set", which I believe I've demonstrated to be incorrect. We do allow secret data to be returned, we've just made it (afaik unnecessarily) inconvenient.

Not allowing accidental secret retrieval by using the API (specially on the list secrets endpoint, which would return all secrets in plaintext)

Why would the list endpoint returns secret text? Most list endpoints do not return full data. They only return "summary data". I don't think the "list secret" endpoint needs to change. It would be equally appropriate for a "list configs" endpoint as it is now.

... is not the same thing as a user intentionally deploying services with legitimate secret access to exfiltrate them.

I agree, but how accidental is docker secret inspect foo ? That seems like it's just as intentional.

This was just a response to "Also it allows us to have different experiences to deal with sensitive data", which is what I'm challenging.

I think for many users the distinction between a config and a secret is not clear. Many popular web frameworks expect credentials to be read from their config file. If users have passwords in their config files they should be using secrets, but they may also want to template that same config file. So I think it's quite possible that users will want to template secrets as well.

Having two distinct cmd-line arguments and two distinct APIs allows us to separate the roles of managing secrets from managing configs.

Even if this was a common case, it does not require two separate API endpoints or cmd-line arguments. Users are free to implement whatever process makes sense to them.

Configs will diverge significantly from secrets

So far I don't see an argument for this:

  • hiding them from the user - easily bypassed with a service create/logs, so not really significant (unless I'm misunderstanding this point)
  • templating - just as easily applied to "configs" that contain sensitive data , which would be considered "secrets"
Member

dnephin commented Apr 7, 2017

@dnephin I think you would agree those are separate concerns.

Sorry, separate from what? I was responding to this statement "secret data is never returned to the user after set", which I believe I've demonstrated to be incorrect. We do allow secret data to be returned, we've just made it (afaik unnecessarily) inconvenient.

Not allowing accidental secret retrieval by using the API (specially on the list secrets endpoint, which would return all secrets in plaintext)

Why would the list endpoint returns secret text? Most list endpoints do not return full data. They only return "summary data". I don't think the "list secret" endpoint needs to change. It would be equally appropriate for a "list configs" endpoint as it is now.

... is not the same thing as a user intentionally deploying services with legitimate secret access to exfiltrate them.

I agree, but how accidental is docker secret inspect foo ? That seems like it's just as intentional.

This was just a response to "Also it allows us to have different experiences to deal with sensitive data", which is what I'm challenging.

I think for many users the distinction between a config and a secret is not clear. Many popular web frameworks expect credentials to be read from their config file. If users have passwords in their config files they should be using secrets, but they may also want to template that same config file. So I think it's quite possible that users will want to template secrets as well.

Having two distinct cmd-line arguments and two distinct APIs allows us to separate the roles of managing secrets from managing configs.

Even if this was a common case, it does not require two separate API endpoints or cmd-line arguments. Users are free to implement whatever process makes sense to them.

Configs will diverge significantly from secrets

So far I don't see an argument for this:

  • hiding them from the user - easily bypassed with a service create/logs, so not really significant (unless I'm misunderstanding this point)
  • templating - just as easily applied to "configs" that contain sensitive data , which would be considered "secrets"
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 7, 2017

Member

Another issue that came up during the maintainers PR review session (that I guess wasn't mentioned here yet) is that docker config doesn't really do what you would expect. I would have expected that command to configure docker or dockerd, not just services.

Couldn't we just use secrets as they are now. Add a proper secret inspect and if/when the implementation actually requires some significant difference between these types we can add --sensitive=false (or some similar flag) to change the behaviour? Maybe that's already the case with the path restrictions?

Member

dnephin commented Apr 7, 2017

Another issue that came up during the maintainers PR review session (that I guess wasn't mentioned here yet) is that docker config doesn't really do what you would expect. I would have expected that command to configure docker or dockerd, not just services.

Couldn't we just use secrets as they are now. Add a proper secret inspect and if/when the implementation actually requires some significant difference between these types we can add --sensitive=false (or some similar flag) to change the behaviour? Maybe that's already the case with the path restrictions?

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Apr 7, 2017

Contributor

Couldn't we just use secrets as they are now. Add a proper secret inspect and if/when the implementation actually requires some significant difference between these types we can add --sensitive=false (or some similar flag) to change the behaviour? Maybe that's already the case with the path restrictions?

  • Secrets and Configs are going to diverge more and more
  • Configs will be supporting templates and things like that
  • Secrets will support types
  • Having config secrets doesn't make sense. We could have had secret configs, but it's way too late now.
Contributor

aluzzardi commented Apr 7, 2017

Couldn't we just use secrets as they are now. Add a proper secret inspect and if/when the implementation actually requires some significant difference between these types we can add --sensitive=false (or some similar flag) to change the behaviour? Maybe that's already the case with the path restrictions?

  • Secrets and Configs are going to diverge more and more
  • Configs will be supporting templates and things like that
  • Secrets will support types
  • Having config secrets doesn't make sense. We could have had secret configs, but it's way too late now.
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

Added config CLI unit tests inspired by #32289.

Contributor

aaronlehmann commented Apr 7, 2017

Added config CLI unit tests inspired by #32289.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 7, 2017

Contributor

Having these as experimental I think makes sense. It gives us time to let people test the feature before 17.06-stable and potentially pull it if it's not quite right.
Other than object/subcommand naming, I think it's good.

One suggestion made by @thaJeztah I think was docker stash (though that's a verb I would expect to be docker sash <name> -< <data>).
Not sure what else to call it.

Contributor

cpuguy83 commented Apr 7, 2017

Having these as experimental I think makes sense. It gives us time to let people test the feature before 17.06-stable and potentially pull it if it's not quite right.
Other than object/subcommand naming, I think it's good.

One suggestion made by @thaJeztah I think was docker stash (though that's a verb I would expect to be docker sash <name> -< <data>).
Not sure what else to call it.

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Apr 7, 2017

Contributor

Sorry, separate from what? I was responding to this statement "secret data is never returned to the user after set", which I believe I've demonstrated to be incorrect. We do allow secret data to be returned, we've just made it (afaik unnecessarily) inconvenient.

No, you demonstrated that you can deploy an app that can exfiltrate secrets that have been handed to it, you didn't demonstrate the API allows you to leak secrets. That is what I mean by separate concerns.

If you really want to protect your swarm from maliciously deployed containers, you'll have to deploy an AuthZ plugin that only runs signed code and segment the concerns.

I agree, but how accidental is docker secret inspect foo ? That seems like it's just as intentional.

Minimizing the ways secrets can get leaked from your swarm is a worth goal.

I think for many users the distinction between a config and a secret is not clear. Many popular web frameworks expect credentials to be read from their config file. If users have passwords in their config files they should be using secrets, but they may also want to template that same config file. So I think it's quite possible that users will want to template secrets as well.

This is why we would like to template configs, so you can include secrets in them without having to handle configs themselves as sensitive.

This was just a response to "Also it allows us to have different experiences to deal with sensitive data", which is what I'm challenging.

You're absolutely entitled to have your opinion; ours (secrets team) has been that production setups always have distinct treatment/confidentiality/apis/control around what they deem to be secret data, and therefore, we should keep them separate.

Contributor

diogomonica commented Apr 7, 2017

Sorry, separate from what? I was responding to this statement "secret data is never returned to the user after set", which I believe I've demonstrated to be incorrect. We do allow secret data to be returned, we've just made it (afaik unnecessarily) inconvenient.

No, you demonstrated that you can deploy an app that can exfiltrate secrets that have been handed to it, you didn't demonstrate the API allows you to leak secrets. That is what I mean by separate concerns.

If you really want to protect your swarm from maliciously deployed containers, you'll have to deploy an AuthZ plugin that only runs signed code and segment the concerns.

I agree, but how accidental is docker secret inspect foo ? That seems like it's just as intentional.

Minimizing the ways secrets can get leaked from your swarm is a worth goal.

I think for many users the distinction between a config and a secret is not clear. Many popular web frameworks expect credentials to be read from their config file. If users have passwords in their config files they should be using secrets, but they may also want to template that same config file. So I think it's quite possible that users will want to template secrets as well.

This is why we would like to template configs, so you can include secrets in them without having to handle configs themselves as sensitive.

This was just a response to "Also it allows us to have different experiences to deal with sensitive data", which is what I'm challenging.

You're absolutely entitled to have your opinion; ours (secrets team) has been that production setups always have distinct treatment/confidentiality/apis/control around what they deem to be secret data, and therefore, we should keep them separate.

@vieux vieux changed the title from Configuration files for services to [experimental] Configuration files for services Apr 7, 2017

@vieux vieux added this to the 17.05.0 milestone Apr 7, 2017

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 7, 2017

Member

This was discussed in maintainers weekly. There was opposition around the similarity with secrets, both from the ux and implementation, and for the subcommand name config. I don't remember if there was any actionable items other than it has been marked with "revisit". cc @thaJeztah

I think we should try to find a better name or put it behind docker service if the configs will always be tied to services. After that this SGTM as experimental feature.

Member

tonistiigi commented Apr 7, 2017

This was discussed in maintainers weekly. There was opposition around the similarity with secrets, both from the ux and implementation, and for the subcommand name config. I don't remember if there was any actionable items other than it has been marked with "revisit". cc @thaJeztah

I think we should try to find a better name or put it behind docker service if the configs will always be tied to services. After that this SGTM as experimental feature.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Apr 10, 2017

Member

Desgin SGTM for me with what @tonistiigi said (even if it might not be under services in the future, it could be like inspect). Having it in experimental will help making sure we are doing the right thing.

Member

vdemeester commented Apr 10, 2017

Desgin SGTM for me with what @tonistiigi said (even if it might not be under services in the future, it could be like inspect). Having it in experimental will help making sure we are doing the right thing.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 10, 2017

Member

Oh, yes, there was some confusion about docker config as a "top level" command, given that it could be confused with "configuring docker" / "configuring the docker daemon". Alternative names suggested were "configuration" (but admittedly only a minor enhancement), and I blurbed "docker configfile" or "docker stash"

Member

thaJeztah commented Apr 10, 2017

Oh, yes, there was some confusion about docker config as a "top level" command, given that it could be confused with "configuring docker" / "configuring the docker daemon". Alternative names suggested were "configuration" (but admittedly only a minor enhancement), and I blurbed "docker configfile" or "docker stash"

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 10, 2017

Member

Basically; I think having configurations as a separate option (both CLI/API) makes sense, given that;

  • it allows supporting custom back-ends for both secrets and configuration in future (which could have different requirements for both types)
  • it allows features to diverge between secrets and configurations (I know there was talk of "templating")

However, there are various situations where it's not easy to separate "configuration" from "secrets" (configuration files containing secrets); if we allow arbitrary paths in the container for configs but not for secrets, it's very likely that people will use configs for everything (including their secrets).

We briefly discussed this in the maintainers meeting as well, and we couldn't come with a technical reason to not allow arbitrary paths in the container for secrets (correct me if I missed one), so I suggest to keep feature parity between secrets and configuration, an add the same option to secrets as well (target=/some/path/in/container).

Member

thaJeztah commented Apr 10, 2017

Basically; I think having configurations as a separate option (both CLI/API) makes sense, given that;

  • it allows supporting custom back-ends for both secrets and configuration in future (which could have different requirements for both types)
  • it allows features to diverge between secrets and configurations (I know there was talk of "templating")

However, there are various situations where it's not easy to separate "configuration" from "secrets" (configuration files containing secrets); if we allow arbitrary paths in the container for configs but not for secrets, it's very likely that people will use configs for everything (including their secrets).

We briefly discussed this in the maintainers meeting as well, and we couldn't come with a technical reason to not allow arbitrary paths in the container for secrets (correct me if I missed one), so I suggest to keep feature parity between secrets and configuration, an add the same option to secrets as well (target=/some/path/in/container).

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Apr 10, 2017

Contributor

@thaJeztah sounds good. I propose that we automatically setup symlinks from /run/secrets/secret-name to target=/some/path/in/container/secret-name whenever a user passes a target outside of /run/secrets.

Contributor

diogomonica commented Apr 10, 2017

@thaJeztah sounds good. I propose that we automatically setup symlinks from /run/secrets/secret-name to target=/some/path/in/container/secret-name whenever a user passes a target outside of /run/secrets.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 10, 2017

Member

Sounds good to me! I think that will make many people happy 😄

Member

thaJeztah commented Apr 10, 2017

Sounds good to me! I think that will make many people happy 😄

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 10, 2017

Member

@diogomonica Instead of symlinks it should probably create binds to have feature parity with config and to avoid anyone overriding a secret by changing the symlink.

Member

tonistiigi commented Apr 10, 2017

@diogomonica Instead of symlinks it should probably create binds to have feature parity with config and to avoid anyone overriding a secret by changing the symlink.

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Apr 10, 2017

Contributor

@tonistiigi absolutely. Makes sense to me.

Contributor

diogomonica commented Apr 10, 2017

@tonistiigi absolutely. Makes sense to me.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 8, 2017

Contributor

CLI parts split out to docker/cli#45

Contributor

aaronlehmann commented May 8, 2017

CLI parts split out to docker/cli#45

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure May 9, 2017

Contributor

ping @cpuguy83 @ehazlett have all your concerns be addressed? Can this be moved to doc review?

Contributor

mlaventure commented May 9, 2017

ping @cpuguy83 @ehazlett have all your concerns be addressed? Can this be moved to doc review?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 9, 2017

Member

There was one thing that came up last week; the default location for configuration files is /name-of-config, which can possibly lead to conflicts with existing files in the container; options discussed (briefly) were to either make target= a required parameter, or to use a default (e.g.) /run/configurations directory in which the configuration files are put if no target is specified.

I think a default path was discussed before, so leaving as a comment in case we want to consider the option.

Member

thaJeztah commented May 9, 2017

There was one thing that came up last week; the default location for configuration files is /name-of-config, which can possibly lead to conflicts with existing files in the container; options discussed (briefly) were to either make target= a required parameter, or to use a default (e.g.) /run/configurations directory in which the configuration files are put if no target is specified.

I think a default path was discussed before, so leaving as a comment in case we want to consider the option.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 9, 2017

Contributor

Looks like paths are not quite sanitized (though in the PR for custom secret targets they are) such that creating a target like ../test will yield errors in runc since it's trying to mount to a path outside the rootfs dir.
So at least it's good that runc is preventing this, but would be nice to filter this out like we are with custom secret targets (where any amount of ../ just points to / in the container).

Contributor

cpuguy83 commented May 9, 2017

Looks like paths are not quite sanitized (though in the PR for custom secret targets they are) such that creating a target like ../test will yield errors in runc since it's trying to mount to a path outside the rootfs dir.
So at least it's good that runc is preventing this, but would be nice to filter this out like we are with custom secret targets (where any amount of ../ just points to / in the container).

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 9, 2017

Contributor

I can't tell if there's a way to update an existing config in a service, either by updating the config itself or by changing an existing target to point to a new config.

Contributor

cpuguy83 commented May 9, 2017

I can't tell if there's a way to update an existing config in a service, either by updating the config itself or by changing an existing target to point to a new config.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 9, 2017

Contributor

Looks like paths are not quite sanitized (though in the PR for custom secret targets they are) such that creating a target like ../test will yield errors in runc since it's trying to mount to a path outside the rootfs dir.
So at least it's good that runc is preventing this, but would be nice to filter this out like we are with custom secret targets (where any amount of ../ just points to / in the container).

I don't believe the custom secrets target does any kind of filtering like this. I think what you're seeing is that non-absolute paths are rooted in /run/secrets, so /run/secrets/../foo ends up inside the container. Presumably more than two ../ prefixes will cause runc to error out.

Where would you suggest doing this filtering, and how? Should we call filepath.Clean and error out if any target starts with ../ before handing off the path to runc?

I can't tell if there's a way to update an existing config in a service, either by updating the config itself or by changing an existing target to point to a new config.

Configs are immutable, but you should be able to combine --config-rm and --config-add to replace the config that's mounted at a particular target.

Contributor

aaronlehmann commented May 9, 2017

Looks like paths are not quite sanitized (though in the PR for custom secret targets they are) such that creating a target like ../test will yield errors in runc since it's trying to mount to a path outside the rootfs dir.
So at least it's good that runc is preventing this, but would be nice to filter this out like we are with custom secret targets (where any amount of ../ just points to / in the container).

I don't believe the custom secrets target does any kind of filtering like this. I think what you're seeing is that non-absolute paths are rooted in /run/secrets, so /run/secrets/../foo ends up inside the container. Presumably more than two ../ prefixes will cause runc to error out.

Where would you suggest doing this filtering, and how? Should we call filepath.Clean and error out if any target starts with ../ before handing off the path to runc?

I can't tell if there's a way to update an existing config in a service, either by updating the config itself or by changing an existing target to point to a new config.

Configs are immutable, but you should be able to combine --config-rm and --config-add to replace the config that's mounted at a particular target.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 May 10, 2017

Contributor

@aaronlehmann I'm not finding where it could be happening, but can confirm infinite ../../../../../foo is always scoped to /foo in the container for secrets with no runc error.

Indeed filepath.Join(root, filepath.Clean(target)) seems like it would yield a correctly scoped path.

Contributor

cpuguy83 commented May 10, 2017

@aaronlehmann I'm not finding where it could be happening, but can confirm infinite ../../../../../foo is always scoped to /foo in the container for secrets with no runc error.

Indeed filepath.Join(root, filepath.Clean(target)) seems like it would yield a correctly scoped path.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

Maybe /run/secrets/../../../foo is considered a valid mount target, but ../foo is not?

I wonder if prepending / to non-absolute paths would fix it (is this what you're suggesting?).

Contributor

aaronlehmann commented May 10, 2017

Maybe /run/secrets/../../../foo is considered a valid mount target, but ../foo is not?

I wonder if prepending / to non-absolute paths would fix it (is this what you're suggesting?).

@cpuguy83

LGTM

@ehazlett

This comment has been minimized.

Show comment
Hide comment
@ehazlett

ehazlett May 10, 2017

Contributor

LGTM

Contributor

ehazlett commented May 10, 2017

LGTM

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo May 10, 2017

Contributor

@aaronlehmann needs a rebase :(

Contributor

mavenugo commented May 10, 2017

@aaronlehmann needs a rebase :(

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

Rebased - PTAL

Contributor

aaronlehmann commented May 10, 2017

Rebased - PTAL

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass May 11, 2017

Collaborator

The two failures TestConfigHTTPHeader and TestExecPausedContainer are unrelated failures being fixed in #29030

Collaborator

tiborvass commented May 11, 2017

The two failures TestConfigHTTPHeader and TestExecPausedContainer are unrelated failures being fixed in #29030

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo May 11, 2017

Contributor

sorry @aaronlehmann. We need another rebase of this PR.

Contributor

mavenugo commented May 11, 2017

sorry @aaronlehmann. We need another rebase of this PR.

aaronlehmann added some commits Mar 15, 2017

Add config APIs
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Add configs support to client
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Update CLI docs and add opts/config.go
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Add config support to executor backend
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Add integration test coverage for configs
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Vendor CLI fork for integration tests
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 11, 2017

Contributor

Rebased

Contributor

aaronlehmann commented May 11, 2017

Rebased

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux May 11, 2017

Collaborator

LGTM

Collaborator

vieux commented May 11, 2017

LGTM

@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo May 11, 2017

Contributor

Janky failure (TestSwarmClusterRotateUnlockKey) is unrelated and this PR has gone through multiple rebases and I think it is time to put it out of its misery and merge the PR :)

Contributor

mavenugo commented May 11, 2017

Janky failure (TestSwarmClusterRotateUnlockKey) is unrelated and this PR has gone through multiple rebases and I think it is time to put it out of its misery and merge the PR :)

@vieux vieux merged commit 69c35da into moby:master May 11, 2017

5 of 6 checks passed

janky Jenkins build Docker-PRs 42609 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34009 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 2985 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13842 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2711 has succeeded
Details
@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux May 11, 2017

Collaborator

@aaronlehmann 🎉 🎉

Collaborator

vieux commented May 11, 2017

@aaronlehmann 🎉 🎉

albers added a commit to albers/docker-cli that referenced this pull request Jul 3, 2017

Add bash completion for `docker config` command family
This adds bash completion for
- docker#45
- moby/moby#32336

Signed-off-by: Harald Albers <github@albersweb.de>

albers added a commit to albers/docker-cli that referenced this pull request Jul 7, 2017

Add bash completion for `docker config` command family
This adds bash completion for
- docker#45
- moby/moby#32336

Signed-off-by: Harald Albers <github@albersweb.de>

thaJeztah added a commit to thaJeztah/docker-ce that referenced this pull request Jul 25, 2017

Add bash completion for `docker config` command family
This adds bash completion for
- docker/cli#45
- moby/moby#32336

Signed-off-by: Harald Albers <github@albersweb.de>
(cherry picked from commit c40952b3058db3c6e44bca8c518d888b83878ce5)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

alshabib added a commit to alshabib/cli that referenced this pull request Aug 1, 2017

Add bash completion for `docker config` command family
This adds bash completion for
- docker#45
- moby/moby#32336

Signed-off-by: Harald Albers <github@albersweb.de>

riyazdf added a commit to riyazdf/cli that referenced this pull request Aug 2, 2017

Add bash completion for `docker config` command family
This adds bash completion for
- docker#45
- moby/moby#32336

Signed-off-by: Harald Albers <github@albersweb.de>

vieux pushed a commit to vieux/docker-ce that referenced this pull request Aug 10, 2017

Add bash completion for `docker config` command family
This adds bash completion for
- docker/cli#45
- moby/moby#32336

Signed-off-by: Harald Albers <github@albersweb.de>
Upstream-commit: c40952b3058db3c6e44bca8c518d888b83878ce5
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment