Implement support for docker-credential-helpers #2651

rawler · 2017-05-17T11:03:35Z

Solves: #2334

The exact ENV for the docker-helper to run in could be discussed, but at the very least I think it needs the vault-token of the runner.

For our specific use-case, we have a self-contained python-script that from vaults pulls out a restricted AWS-key and ECR-token. A simple solution for running containers from ECR.

dadgar · 2017-05-17T16:37:55Z

client/driver/docker.go

@@ -1495,3 +1538,13 @@ func authOptionFrom(file, repo string) (*docker.AuthConfiguration, error) {

 	return apiAuthConfig, nil
 }
+


Hey it would be great if we could actually reuse the auth config file. That file supports setting credential helpers per repo and we parse it right above: https://github.com/hashicorp/nomad/pull/2651/files#diff-a6757d41ad7cd7f37f0f0243e3575307R1524.

I would suggest we use that to determine if we should use a credential helper.

credHelpers is indeed a nice solution. I will see if we can use it somehow

dadgar · 2017-05-17T16:39:00Z

client/driver/docker.go

+	cmd.Stdin = strings.NewReader(repoName)
+
+	// For helper to be able to read credentials from vault
+	cmd.Env = append(os.Environ(), fmt.Sprintf("VAULT_TOKEN=%s", clientConfig.VaultConfig.Token))


I would like to see an operator option to allow passing the vault token to credential helpers. I would probably treat it as a white list of allowed helpers. You generally do not want to give that out any more than you have to! Isn't security fun :)

I considered this when writing the code. I get a feeling this deserve something bigger than a small helper-detail in the docker driver though. There must be more places where external processes/plugins are called for as a part of the task-setup, needing access to different nomad-config, for example the vault-address used, the consul-connection details etc?

From a security-standpoint however; the helper will run in the same security-context as the nomad process. This means whomever control the helper can still read the token from the nomad-config?

If it simplifies the approval of this PR, we could skip the custom env altogether, postpone it for something more thought-through?

I see your point! Once we have generic plugin support, a overall whitelist for the things Nomad fork/execs to delegate work would probably make sense. I can see the Vault use case being nice for people writing custom credential-helpers. However I would like even the first cut to allow operators to control which ones have access, so given it expands the PR I would be okay if you want to skip that functionality for the first PR!

dadgar · 2017-05-17T16:39:24Z

Great work so far! This is going to be a great addition!

rawler · 2017-05-19T09:53:28Z

There. I kept the docker.auth.helpers variable (for simple use-cases it avoid an extra file entirely) and added handling of auth, credHelpers and credsStore (in priority order) from the config file.

And removed VAULT_TOKEN, let's take that in a separate patch later

rawler · 2017-05-24T08:33:45Z

Hmm. Not sure if I broke test (it does not seem so?) or if this is simply due to master being broken?

dadgar

This is super sweet work @rawler! Sorry for the delayed review! Had HashiDays NYC and playing catch up still!

dadgar · 2017-05-24T17:18:21Z

client/driver/docker.go


-	dockerAuthConfig := registry.ResolveAuthConfig(cfile.AuthConfigs, repoInfo.Index)
+		if response["Username"] != "" || response["Secret"] != "" {


Should this be an and?

Not my intention, no? If either of the two relevant fields is filled-in, we consider it an authentication.

For example, blank password is a completely possible scenario and should not be ignored. Likewise, it's conceivable for a private repo to auth using a blank username and I.E. api-key in Secret.

dadgar · 2017-05-24T17:19:35Z

website/source/docs/drivers/docker.html.md

+  information for a private registry, from either (in order) `auths`,
+  `credHelpers` or `credsStore`.
+
+* `docker.auth.helper` <a id="auth_helper"></a>- Allows an operator to specify


I would prefer if we remove this. Sometimes optionality actually just leads to confusion! I think it is better to have one well documented, canonical way to do this.

I tend to agree. The rationale I see for it here, is that for simple scenarios (like in my organization) it's very likely to only have a single helper required. For those cases, a docker-config really isn't needed. Another possible such scenario is to have all secrets stored in Vault. Hashicorp might even want to have an official docker-helper for that.

With this option, moving nomad docker-auth from clear-text docker-config, to protected in vault, is simply a matter of

one config-line in the nomad agent-config

a VAULT_TOKEN in env

a script installed to $PATH

Without this option, an entire extra config-file is required. (Likely needlessly, since the nomad agent host should preferably not be shared for other purposes, I guess?)

dadgar · 2017-05-24T17:20:01Z

website/source/docs/drivers/docker.html.md

-[docker.auth.config](#auth_file) value on the client.
+you will need to specify credentials in your job via:
+
+ * the `auth` option


the auth option in the driver's config.

"task-config" more precisely, right?

dadgar · 2017-05-24T17:20:52Z

website/source/docs/drivers/docker.html.md

+
+ * the `auth` option
+
+ * by storing credentials or `credHelpers` in a file and setting the


Would you be willing to inline an example of what that file would look like with cred helpers set and plain base64 credentials set for a separate repo?

Sure thing. One example says more than 1000 words.

dadgar · 2017-05-24T17:21:08Z

client/driver/docker.go

 	}

 	d.emitEvent("Downloading image %s:%s", repo, tag)
 	coordinator, callerID := d.getDockerCoordinator(client)
 	return coordinator.PullImage(driverConfig.ImageName, authOptions, callerID)
 }

+type authBackend func(string) (*docker.AuthConfiguration, error)


Can we get comments on all the functions/types

rawler · 2017-05-29T10:48:04Z

So, I think all the questions are now addressed, except possibly the existence of "docker.auth.helper".

If you still want it dropped, it's quickly done

dadgar · 2017-05-30T18:16:38Z

@rawler Thanks for the great work and thoughtful reply! Lets keep the separate helper option.

Would you mind fixing this and then we can merge: https://travis-ci.org/hashicorp/nomad/builds/237122683#L830

dadgar · 2017-05-30T18:25:31Z

website/source/docs/drivers/docker.html.md

+  "credHelpers": {
+      "<XYZ>.dkr.ecr.<region>.amazonaws.com": "vault-ecr-login"
+  },
+  "credsStore": "vault"


Could you use a publicly available credential store (maybe "secretservice")

Sure thing, but ideally, maybe we should even (after this PR) write a small example-script to pull credentials from vault? I suspect more people would find that useful, since secretservice is more oriented to desktop, I believe.

dadgar · 2017-05-30T18:25:55Z

website/source/docs/drivers/docker.html.md

+    "internal.repo": { "auth": "`echo -n '<username>:<password>' | base64 -w0`" }
+  },
+  "credHelpers": {
+      "<XYZ>.dkr.ecr.<region>.amazonaws.com": "vault-ecr-login"


Could you use: https://github.com/awslabs/amazon-ecr-credential-helper

Solves: hashicorp#2334

rawler · 2017-05-31T10:47:30Z

As an example, here is an (untested, I don't really have a good setup for it) script for pulling secrets from vault: https://pastebin.com/vyUHGgbX

dadgar · 2017-05-31T22:45:08Z

Thanks @rawler

aep · 2017-06-26T16:01:21Z

@rawler could you share your ecr creds script? https://github.com/awslabs/amazon-ecr-credential-helper is not compatible with docker.auth.helper, because it (correctly?) returns a non zero exist code for non-ecr repos. the official docker client seems to just continue without creds

rawler · 2017-06-26T21:29:32Z

@aep oh, sorry I did not notice this behavior when building the patch. Do you happen to know if the behavior is documented somewhere? Is there a precedent set from git-credential-helpers?

Anyways, sure I can share the script we're using for ECR. Not though that this is probably really interesting for us, who is using AWS and ECR in combination with Vault, whereas Vault is responsible for generating short-lived AWS-credentials, to convert into ECR-credentials

However, with some simple hacking it should be doable to bypass the Vault-parts, and get ECR-creds from simple AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment. If the ECRClient is initialized with ECRClient(region, None, None), that should be it.

https://gist.github.com/rawler/03ab8641f280d16505d0d334da8bc312

this amends the behaviour introduced with hashicorp#2651 and allows pulling public images when docker.auth.helper is set

aep · 2017-06-27T09:19:02Z

@rawler i'm not sure that particular behaviour is documented. seems a bit subtle.
thanks for sharing your script. my python isn't that great, but it's a good starting point.
We're planning to do ECR auth over vault sts.

let me know if you're ok with #2744

btw i found https://godoc.org/github.com/docker/docker-credential-helpers/client

github-actions · 2023-03-31T02:13:38Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

dadgar requested changes May 17, 2017

View reviewed changes

rawler force-pushed the feature/2334 branch from c19dea5 to 61463df Compare May 19, 2017 09:43

rawler force-pushed the feature/2334 branch 3 times, most recently from 333d09a to 93127f4 Compare May 23, 2017 09:03

dadgar reviewed May 24, 2017

View reviewed changes

rawler force-pushed the feature/2334 branch from 93127f4 to 5c75e7b Compare May 29, 2017 10:46

dadgar reviewed May 30, 2017

View reviewed changes

rawler force-pushed the feature/2334 branch from 5c75e7b to f6af537 Compare May 31, 2017 10:19

Implement support for docker-credential-helpers

20fd19d

Solves: hashicorp#2334

rawler force-pushed the feature/2334 branch from f6af537 to 20fd19d Compare May 31, 2017 10:45

Merge branch 'master' into feature/2334

e9f6769

dadgar merged commit 84aba1d into hashicorp:master May 31, 2017

dadgar mentioned this pull request May 31, 2017

[feature] Docker Credential Helper #2334

Closed

rawler deleted the feature/2334 branch June 2, 2017 07:53

aep added a commit to aep/nomad that referenced this pull request Jun 27, 2017

Do not fail when no docker registry auth is available

1699761

this amends the behaviour introduced with hashicorp#2651 and allows pulling public images when docker.auth.helper is set

aep mentioned this pull request Jun 27, 2017

Do not fail when no docker registry auth is available #2744

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for docker-credential-helpers #2651

Implement support for docker-credential-helpers #2651

rawler commented May 17, 2017

dadgar May 17, 2017

rawler May 17, 2017

dadgar May 17, 2017

rawler May 17, 2017

dadgar May 18, 2017

dadgar commented May 17, 2017

rawler commented May 19, 2017

rawler commented May 24, 2017

dadgar left a comment

dadgar May 24, 2017

rawler May 29, 2017

dadgar May 24, 2017

rawler May 29, 2017

dadgar May 24, 2017

rawler May 29, 2017

dadgar May 24, 2017

rawler May 29, 2017

dadgar May 24, 2017

rawler commented May 29, 2017

dadgar commented May 30, 2017 •

edited

Loading

dadgar May 30, 2017

rawler May 31, 2017

dadgar May 30, 2017

rawler commented May 31, 2017

dadgar commented May 31, 2017

aep commented Jun 26, 2017 •

edited

Loading

rawler commented Jun 26, 2017

aep commented Jun 27, 2017 •

edited

Loading

github-actions bot commented Mar 31, 2023

		@@ -1495,3 +1538,13 @@ func authOptionFrom(file, repo string) (*docker.AuthConfiguration, error) {

		return apiAuthConfig, nil
		}


		dockerAuthConfig := registry.ResolveAuthConfig(cfile.AuthConfigs, repoInfo.Index)
		if response["Username"] != "" \|\| response["Secret"] != "" {


		* the `auth` option

		* by storing credentials or `credHelpers` in a file and setting the

Implement support for docker-credential-helpers #2651

Implement support for docker-credential-helpers #2651

Conversation

rawler commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadgar commented May 17, 2017

rawler commented May 19, 2017

rawler commented May 24, 2017

dadgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawler commented May 29, 2017

dadgar commented May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawler commented May 31, 2017

dadgar commented May 31, 2017

aep commented Jun 26, 2017 • edited Loading

rawler commented Jun 26, 2017

aep commented Jun 27, 2017 • edited Loading

github-actions bot commented Mar 31, 2023

dadgar commented May 30, 2017 •

edited

Loading

aep commented Jun 26, 2017 •

edited

Loading

aep commented Jun 27, 2017 •

edited

Loading