Skip to content
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

Templated secrets and configs #33702

Merged
merged 8 commits into from Feb 21, 2018

Conversation

@aaronlehmann
Copy link
Contributor

commented Jun 15, 2017

This adds support for template expansion in secrets and configs, as added up stream in docker/swarmkit#2133.

SecretSpec and ConfigSpec have a new optional Templating field. Currently, the only supported value is golang for go-style templates, but I imagine supporting other templating engines in the future, as Go-style templating is not especially familiar to Docker's target audience.

When templating is enabled, it's possible to expand information about the task and service (examples: {{.Task.ID}}, {{env "VAR"}}), and other secrets and configs available to the service (example: {{secret "sometarget"}}).

A few areas where this can be useful:

  • Fill in information in a configuration file based on the task, service, or environment.
  • Insert a secret such as a password into a configuration file, so the configuration file template can be managed separatly from the actual credential.
  • Compose a secret from multiple secrets.

cc @diogomonica

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:templated-secrets-and-configs branch 2 times, most recently from 0ee21b0 to 5259d1a Jun 15, 2017

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2017

Just so I understand, this is making it so you can template the actual secret/config data and effectively creates a dependencies between secrets/configs (assuming that the template is referencing a secret/config).

@aaronlehmann

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2017

Just so I understand, this is making it so you can template the actual secret/config data and effectively creates a dependencies between secrets/configs (assuming that the template is referencing a secret/config).

I suppose you can say that, but you only secrets and configs already attached to the service may be referenced. So it effectively creates dependencies between services/configs, but cannot add new secret/config dependencies to a service.

Name: "golang",
},
Data: []byte("SERVICE_NAME={{.Service.Name}}\n" +
"{{secret \"referencedsecrettarget\"}}\n" +

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Jun 22, 2017

Contributor

If configs can pull in secrets, doesn't this have security consequences since configs are not on tmpfs?

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Jun 22, 2017

Author Contributor

I brought that up here, but there didn't seem to be concerns about it: docker/swarmkit#2133 (comment)

I'd be fine with restricting the ability to insert secrets into configs if people think that's the right thing to do.

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Jun 26, 2017

Contributor

Yeah, I think it's a mistake to allow importing secrets into a config.
/cc @diogomonica

This comment has been minimized.

Copy link
@diogomonica

diogomonica Jun 26, 2017

Contributor

Aren't configs also tmpFS?

This comment has been minimized.

Copy link
@aaronlehmann

This comment has been minimized.

Copy link
@cyli

cyli Jun 26, 2017

Contributor

I vaguely remember the outcome of that discussion being that if configs are flagged having a dependency on a secret, it was going to be treated as a secret? It was left as an implementation detail for the future, though.

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Jun 26, 2017

Author Contributor

I started working on flagging configs that depend on a secret so they can be stored in tmpfs. docker/swarmkit#2286 is the swarmkit part of this. I will update this PR to make use of this.

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Jun 27, 2017

Author Contributor

Updated the PR with new commits to store configs in the tmpfs secrets directory when they have secrets inserted inside them. Marking WIP because this depends on docker/swarmkit#2286.

This comment has been minimized.

Copy link
@justincormack

justincormack Sep 21, 2017

Contributor

that seems really complicated, why don't we move all configs to tmpfs? They should be accounted for under the container's memory usage so I don't think thats an issue.

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Sep 22, 2017

Author Contributor

That would be fine with me. I don't have time to update the PR right now, but it's okay with me if someone wants to carry this.

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:templated-secrets-and-configs branch from 5259d1a to 6715c7e Jun 23, 2017

@aaronlehmann

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2017

Rebased

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:templated-secrets-and-configs branch from 6715c7e to 82340f8 Jun 27, 2017

@aaronlehmann aaronlehmann changed the title Templated secrets and configs [WIP] Templated secrets and configs Jun 27, 2017

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:templated-secrets-and-configs branch from 82340f8 to c0f58cf Jun 27, 2017

@aaronlehmann

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2017

docker-py test failures are unrelated (#33863)

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:templated-secrets-and-configs branch from c0f58cf to 6c946d4 Jul 11, 2017

@aaronlehmann aaronlehmann changed the title [WIP] Templated secrets and configs Templated secrets and configs Jul 19, 2017

@aaronlehmann aaronlehmann force-pushed the aaronlehmann:templated-secrets-and-configs branch from 6c946d4 to 3c61b4c Jul 19, 2017

@aaronlehmann

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2017

Swarmkit PR was merged. Rebased this one.

ping @diogomonica @cyli @cpuguy83

@cyli

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

Not sure if the lack of network-create event failures are related to the vendoring... but the config changes LGTM.

@aaronlehmann

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2017

I broke something in docker/swarmkit#2310. I messed up the lifecycle on the watch server - it's supposed to be available on non-leader nodes as well. I'll fix it.

@vdemeester
Copy link
Member

left a comment

With adding a way to ignore the templating if the API version is lower than the current one (don't remember the exact number), SGTM 👍

// TODO: Right now Windows doesn't really have a "secure" storage for secrets,
// however some configs may contain secrets. Once secure storage is worked out,
// configs and secret handling should be merged.
func (container *Container) ConfigMounts() []Mount {

This comment has been minimized.

Copy link
@vdemeester

vdemeester Feb 1, 2018

Member

Won't this fail to validate (golint 😛)

@cpuguy83 cpuguy83 force-pushed the aaronlehmann:templated-secrets-and-configs branch from 48615bf to 4d3d211 Feb 15, 2018

aaronlehmann and others added 8 commits Jun 15, 2017
executor: Use a TemplatedDependencyGetter to support template expansion
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
api: Add Templating parameter to SecretSpec and ConfigSpec
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
integration-cli: Add secret/config templating tests
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Store configs that contain secrets on tmpfs
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
daemon: Check return value of createSecretDir
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
Always mount configs with tmpfs
This makes configs and secrets behavior identical.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Merge configs/secrets in unix implementation
On unix, merge secrets/configs handling. This is important because
configs can contain secrets (via templating) and potentially a config
could just simply have secret information "by accident" from the user.
This just make sure that configs are as secure as secrets and de-dups a
lot of code.
Generally this makes everything simpler and configs more secure.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Error out on secret/config templates for older API
Makes sure if the user specifies an older API version that we don't pass
through templating options for versions that templating was not
supported.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

@cpuguy83 cpuguy83 force-pushed the aaronlehmann:templated-secrets-and-configs branch from 4d3d211 to a407761 Feb 16, 2018

@vdemeester
Copy link
Member

left a comment

LGTM 🦁

@thaJeztah
Copy link
Member

left a comment

LGTM

@@ -372,6 +372,10 @@ func (sr *swarmRouter) createSecret(ctx context.Context, w http.ResponseWriter,
if err := json.NewDecoder(r.Body).Decode(&secret); err != nil {
return err
}
version := httputils.VersionFromContext(ctx)
if secret.Templating != nil && versions.LessThan(version, "1.36") {
return errdefs.InvalidParameter(errors.Errorf("secret templating is not supported on the specified API version: %s", version))

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2018

Member

Hm initially thought we'd ignore the property (as in: "old API doesn't know about this option, so just ignores it"), but I guess it's ok

@@ -372,6 +372,10 @@ func (sr *swarmRouter) createSecret(ctx context.Context, w http.ResponseWriter,
if err := json.NewDecoder(r.Body).Decode(&secret); err != nil {
return err
}
version := httputils.VersionFromContext(ctx)
if secret.Templating != nil && versions.LessThan(version, "1.36") {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2018

Member

oh, dang! this needs to be 1.37 now (#33922 was merged) - also needs a mention in the API version history, can do that in the same follow-up

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 21, 2018

Member

I'm ok with fixing that in a follow-up

@codecov

This comment has been minimized.

Copy link

commented Feb 21, 2018

Codecov Report

Merging #33702 into master will decrease coverage by 0.17%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #33702      +/-   ##
==========================================
- Coverage   34.22%   34.05%   -0.18%     
==========================================
  Files         609      609              
  Lines       45267    46075     +808     
==========================================
+ Hits        15493    15690     +197     
- Misses      27821    28371     +550     
- Partials     1953     2014      +61
@thaJeztah

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

Ignoring codecode/patch;

Missing base report.
Unable to compare commits because the base of the compare did not upload a coverage report.

@thaJeztah thaJeztah merged commit 0076343 into moby:master Feb 21, 2018

7 of 8 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
codecov/project 34.05% (-0.18%) compared to 508d5a0
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 39426 has succeeded
Details
janky Jenkins build Docker-PRs 48174 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 8584 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 19698 has succeeded
Details
z Jenkins build Docker-PRs-s390x 8566 has succeeded
Details
@vdemeester vdemeester referenced this pull request May 31, 2018
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.