Making it possible to pass Windows credential specs directly to the engine #38777

wk8 · 2019-02-22T06:07:51Z

Instead of having to go through files or registry values as is currently the
case.

While adding GMSA support to Kubernetes (kubernetes/kubernetes#73726)
I stumbled upon the fact that Docker currently only allows passing Windows
credential specs through files or registry values, forcing the Kubelet
to perform a rather awkward dance of writing-then-deleting to either the
disk or the registry to be able to create a Windows container with cred
specs.

This patch solves this problem by making it possible to directly pass
whole base64-encoded cred specs to the engine's API. I took the opportunity
to slightly refactor the method responsible for Windows cred spec as it
seemed hard to read to me.

Added some unit tests on Windows credential specs handling, as there were
previously none.

Added/amended the relevant integration tests.

I have also tested it manually: given a Windows container using a cred spec
that you would normally start with e.g.

docker run --rm --security-opt "credentialspec=file://win.json" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain
# output:
# my.ad.domain.com. (1)
# The command completed successfully

can now equivalently be started with

$rawCredSpec = & cat 'C:\ProgramData\docker\credentialspecs\win.json'
$escaped = $rawCredSpec.Replace('"', '\"')
docker run --rm --security-opt "credentialspec=raw://$escaped" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain
# same output!

I'll do another PR on Swarmkit after this is merged to allow services to use
the same option.

(It's worth noting that @dperny faced the same problem adding GMSA support
to Swarmkit, to which he came up with an interesting solution - see
#38632 - but alas these tricks are not
available to the Kubelet.)

codecov · 2019-02-22T09:19:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@87d5936). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38777   +/-   ##
=========================================
  Coverage          ?   36.44%           
=========================================
  Files             ?      613           
  Lines             ?    45836           
  Branches          ?        0           
=========================================
  Hits              ?    16705           
  Misses            ?    26847           
  Partials          ?     2284

thaJeztah · 2019-02-22T13:07:20Z

daemon/oci_windows.go

+			if credentialSpec, err = readCredentialSpecRegistry(c.ID, value); err != nil {
+				return err
+			}
+		case "config":


wondering (haven't fully thought this through); should we remove the config option, and make swarmkit convert it to base64 (read the credential-spec from the config, and pass it as base64)?

@dperny @cpuguy83

I had the same question, actually :)

Yes, please ❤️

Happy to make a PR for that on Swarmkit after this is merged is @dperny likes the idea too :)

Does it require a swarmkit change? I think the node agent can just read the config and stick it into the the credential spec?

Why not just in a separate commit?

Seems like the whole reason to add this is to remove config.

Well, the original reason for this is to remove the whole awkward dance I had to do in the kubelet to write/clean up registry keys to add GMSA support there (see the PR message).

And I'll be happy to do that in a follow-up PR; it can't hurt to keep PRs to a reasonable size?

Fine, I would prefer to have the change come in through a single PR but in separate commits but 🤷‍♂️

It arguably is 2 different things; I'll make a follow-up PR as soon as this one is merged. Thanks!

daemon/oci_windows.go

thaJeztah · 2019-02-22T13:09:23Z

daemon/oci_windows.go

 		}
-		return "", fmt.Errorf("error %v reading credential spec %q from registry for container %s", err, name, id)
+		return "", fmt.Errorf("error reading credential spec %q from registry for container %s: %v", name, id, err)


Same here

Suggested change

return "", fmt.Errorf("error reading credential spec %q from registry for container %s: %v", name, id, err)

return "", errors.Wrapf(err, "error reading credential spec %q from registry for container %s", name, id)

daemon/oci_windows.go

dperny · 2019-02-22T21:09:13Z

daemon/oci_windows.go

@@ -344,6 +285,82 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S
 	return nil
 }

+var errInvalidCredentialSpecSecOpt = fmt.Errorf("invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'base64://' followed by a non-empty value")


I have a PR in the works (#38770) to normalize all of the error values to use the errdefs definitions. Can you modify this and any other new errors to use those error types, so that I don't have to repeat the work when rebasing that PR?

Sure thing, doing that today or tomorrow.

thaJeztah · 2019-02-26T15:56:11Z

Looks like there's failures on Windows;

01:50:37 FAIL: docker_cli_run_test.go:4146: DockerSuite.TestRunCredentialSpecFailures
01:50:37 
01:50:37 docker_cli_run_test.go:4159:
01:50:37     c.Assert(err.Error(), checker.Contains, attempt.expectedError, check.Commentf("%s expected %s got %s", attempt.value, attempt.expectedError, err))
01:50:37 ... obtained string = "" +
01:50:37 ...     "\n" +
01:50:37 ...     "Command:  d:\\CI\\CI-34c625c5ff\\binary\\docker.exe run --security-opt=credentialspec=rubbish busybox true\n" +
01:50:37 ...     "ExitCode: 125\n" +
01:50:37 ...     "Error:    exit status 125\n" +
01:50:37 ...     "Stdout:   \n" +
01:50:37 ...     "Stderr:   d:\\CI\\CI-34c625c5ff\\binary\\docker.exe: error during connect: Post http://127.0.0.1:2357/v1.30/containers/7e999460d094c24e4ebea2e71c3f8e33cdfd700ccfe3a476766cb6886ceff284/start: EOF.\n" +
01:50:37 ...     "\n" +
01:50:37 ...     "\n" +
01:50:37 ...     "Failures:\n" +
01:50:37 ...     "ExitCode was 125 expected 0\n" +
01:50:37 ...     "Expected no error"
01:50:37 ... substring string = "invalid credential spec security option - value must be prefixed file:// or registry://"
01:50:37 ... rubbish expected invalid credential spec security option - value must be prefixed file:// or registry:// got 
01:50:37 Command:  d:\CI\CI-34c625c5ff\binary\docker.exe run --security-opt=credentialspec=rubbish busybox true
01:50:37 ExitCode: 125
01:50:37 Error:    exit status 125
01:50:37 Stdout:   
01:50:37 Stderr:   d:\CI\CI-34c625c5ff\binary\docker.exe: error during connect: Post http://127.0.0.1:2357/v1.30/containers/7e999460d094c24e4ebea2e71c3f8e33cdfd700ccfe3a476766cb6886ceff284/start: EOF.
01:50:37 
01:50:37 
01:50:37 Failures:
01:50:37 ExitCode was 125 expected 0
01:50:37 Expected no error

wk8 · 2019-02-26T16:12:37Z

@thaJeztah : thanks, having a look at the failure shortly.

GordonTheTurtle · 2019-03-06T16:48:50Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "wk8/raw_cred_specs" git@github.com:wk8/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354465880
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

wk8 · 2019-03-07T01:46:17Z

@thaJeztah : changed to accept raw strings, and fixed the failing test (and added more). PTAL.

cpuguy83

LGTM

thaJeztah

LGTM; left a suggestion for using gotest.tools for creating the directory and test-file, but let me know if you like that or not 😅

thaJeztah · 2019-03-15T00:40:33Z

daemon/oci_windows_test.go

+
+func TestSetWindowsCredentialSpecInSpec(t *testing.T) {
+	// we need a temp directory to act as the daemon's root
+	tmpDaemonRoot, err := ioutil.TempDir("", "moby-windows-cred-spec-test-")


Note that you could use gotest.tools/fs for this, which would take care of all the asserts;

Suggested change

tmpDaemonRoot, err := ioutil.TempDir("", "moby-windows-cred-spec-test-")

tmpDaemonRoot := fs.NewDir(t, t.Name())

defer tmpDaemonRoot.Remove()

And (I see you have to create a subdirectory and dummy file in this directory below); could do that in one go as well;

Suggested change

tmpDaemonRoot, err := ioutil.TempDir("", "moby-windows-cred-spec-test-")

tmpDaemonRoot := fs.NewDir(t, t.Name(),

fw.WithDir(credentialSpecFileLocation, fs.WithFile("dummy-cred-spec.json", `{"We don't need no": "education"}`))

)

defer tmpDaemonRoot.Remove()

A more complete example;

moby/integration/container/mounts_linux_test.go

Lines 224 to 226 in e859282

tmpDir2 := fs.NewDir(t, "tmpdir2", fs.WithMode(0755),

fs.WithFile("file", "should not be visible when NonRecursive", fs.WithMode(0644)))

defer tmpDir2.Remove()

Can't do it for the second part, as I do need a specific dir name in there - and moving it all up to the very beginning of the test, far from where it's actually used, makes it a bit confusing IMO.

wk8 · 2019-03-15T21:32:57Z

@thaJeztah : thanks, made the changes you requested.

@dperny

…ngine Instead of having to go through files or registry values as is currently the case. While adding GMSA support to Kubernetes (kubernetes/kubernetes#73726) I stumbled upon the fact that Docker currently only allows passing Windows credential specs through files or registry values, forcing the Kubelet to perform a rather awkward dance of writing-then-deleting to either the disk or the registry to be able to create a Windows container with cred specs. This patch solves this problem by making it possible to directly pass whole base64-encoded cred specs to the engine's API. I took the opportunity to slightly refactor the method responsible for Windows cred spec as it seemed hard to read to me. Added some unit tests on Windows credential specs handling, as there were previously none. Added/amended the relevant integration tests. I have also tested it manually: given a Windows container using a cred spec that you would normally start with e.g. ```powershell docker run --rm --security-opt "credentialspec=file://win.json" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # output: # my.ad.domain.com. (1) # The command completed successfully ``` can now equivalently be started with ```powershell $rawCredSpec = & cat 'C:\ProgramData\docker\credentialspecs\win.json' $escaped = $rawCredSpec.Replace('"', '\"') docker run --rm --security-opt "credentialspec=raw://$escaped" mcr.microsoft.com/windows/servercore:ltsc2019 nltest /parentdomain # same output! ``` I'll do another PR on Swarmkit after this is merged to allow services to use the same option. (It's worth noting that @dperny faced the same problem adding GMSA support to Swarmkit, to which he came up with an interesting solution - see moby#38632 - but alas these tricks are not available to the Kubelet.) Signed-off-by: Jean Rouge <rougej+github@gmail.com>

thaJeztah

LGTM, thanks!

wk8 · 2019-03-16T17:08:42Z

Thanks! :)

Drewster727 · 2019-08-11T18:22:59Z

@thaJeztah @wk8 Is there any documentation on this?
I have a swarm setup with windows nodes and would like to pass credential specs directly. However, I do not want to add the spec file to each node, which is why I liked @wk8's original thought of passing it in a raw/base64 form with the docker service create or docker run -- which seems to work currently.

I'd like to see how this can be added to a docker-compose.yml file for when I deploy my stack.
Looking at

moby/daemon/oci_windows.go

Line 355 in 90af4ba

case "raw":

It looks like I can do...

configs:
  ems_credential_spec:
    raw: "some_escaped_json_here"

Is this something we can do now on 19.03?

Thanks,
Drew

GordonTheTurtle added the status/0-triage label Feb 22, 2019

wk8 force-pushed the wk8/raw_cred_specs branch from a9f748b to d12a15a Compare February 22, 2019 09:19

wk8 changed the title ~~[WIP] Making it possible to pass Windows credential specs directly to the engine~~ Making it possible to pass Windows credential specs directly to the engine Feb 22, 2019

wk8 force-pushed the wk8/raw_cred_specs branch 2 times, most recently from 02019ca to e78a1ed Compare February 22, 2019 09:49

thaJeztah requested changes Feb 22, 2019

View reviewed changes

wk8 force-pushed the wk8/raw_cred_specs branch 6 times, most recently from 1e5dd1d to 34c625c Compare February 22, 2019 19:32

wk8 mentioned this pull request Feb 22, 2019

Kubelet changes for Windows GMSA support kubernetes/kubernetes#73726

Merged

dperny reviewed Feb 22, 2019

View reviewed changes

thaJeztah added the rebuild/* label Feb 23, 2019

GordonTheTurtle removed the rebuild/* label Feb 23, 2019

This was referenced Mar 2, 2019

CLI: Add support for using Configs as CredentialSpecs in services docker/cli#1656

Closed

Update docker/docker 8aca18d, containerd v1.2.4 docker/cli#1700

Merged

wk8 force-pushed the wk8/raw_cred_specs branch from 34c625c to d01164e Compare March 6, 2019 00:59

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 6, 2019

wk8 force-pushed the wk8/raw_cred_specs branch 5 times, most recently from b16eb8d to 8857710 Compare March 7, 2019 01:45

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 7, 2019

thaJeztah assigned cpuguy83 and thaJeztah Mar 7, 2019

thaJeztah added status/2-code-review and removed status/0-triage labels Mar 7, 2019

cpuguy83 approved these changes Mar 14, 2019

View reviewed changes

thaJeztah approved these changes Mar 15, 2019

View reviewed changes

thaJeztah added platform/windows impact/changelog labels Mar 15, 2019

wk8 force-pushed the wk8/raw_cred_specs branch from 8857710 to f535c89 Compare March 15, 2019 21:32

wk8 force-pushed the wk8/raw_cred_specs branch 6 times, most recently from ee2356a to d61c3a5 Compare March 16, 2019 01:27

wk8 force-pushed the wk8/raw_cred_specs branch from d61c3a5 to 7fdac7e Compare March 16, 2019 02:20

thaJeztah approved these changes Mar 16, 2019

View reviewed changes

thaJeztah merged commit 2925eb7 into moby:master Mar 16, 2019

wk8 mentioned this pull request Jul 19, 2019

Make container removal fail if platform-specific containers fail kubernetes/kubernetes#80320

Merged

This was referenced Aug 12, 2019

Raw credential spec in compose yml docker/compose#6842

Closed

Raw credential spec in docker compose / docker stack #39730

Open

add raw credential_spec capability docker/compose#6844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making it possible to pass Windows credential specs directly to the engine #38777

Making it possible to pass Windows credential specs directly to the engine #38777

wk8 commented Feb 22, 2019 •

edited

Loading

codecov bot commented Feb 22, 2019 •

edited

Loading

thaJeztah Feb 22, 2019

wk8 Feb 22, 2019

cpuguy83 Feb 22, 2019

wk8 Feb 22, 2019

cpuguy83 Feb 22, 2019

cpuguy83 Mar 8, 2019

cpuguy83 Mar 8, 2019

wk8 Mar 8, 2019

cpuguy83 Mar 14, 2019

wk8 Mar 14, 2019

thaJeztah Feb 22, 2019

wk8 Feb 22, 2019

dperny Feb 22, 2019

wk8 Feb 26, 2019

wk8 Mar 6, 2019

thaJeztah commented Feb 26, 2019

wk8 commented Feb 26, 2019

GordonTheTurtle commented Mar 6, 2019

wk8 commented Mar 7, 2019

cpuguy83 left a comment

thaJeztah left a comment

thaJeztah Mar 15, 2019

wk8 Mar 15, 2019

wk8 commented Mar 15, 2019 •

edited

Loading

thaJeztah left a comment

wk8 commented Mar 16, 2019

Drewster727 commented Aug 11, 2019 •

edited

Loading

	return "", fmt.Errorf("error reading credential spec %q from registry for container %s: %v", name, id, err)
	return "", errors.Wrapf(err, "error reading credential spec %q from registry for container %s", name, id)

	tmpDaemonRoot, err := ioutil.TempDir("", "moby-windows-cred-spec-test-")
	tmpDaemonRoot := fs.NewDir(t, t.Name())
	defer tmpDaemonRoot.Remove()

-	tmpDaemonRoot, err := ioutil.TempDir("", "moby-windows-cred-spec-test-")
+	tmpDaemonRoot := fs.NewDir(t, t.Name(),
+		fw.WithDir(credentialSpecFileLocation, fs.WithFile("dummy-cred-spec.json", `{"We don't need no": "education"}`))
+	)
+	defer tmpDaemonRoot.Remove()

	tmpDir2 := fs.NewDir(t, "tmpdir2", fs.WithMode(0755),
	fs.WithFile("file", "should not be visible when NonRecursive", fs.WithMode(0644)))
	defer tmpDir2.Remove()

Making it possible to pass Windows credential specs directly to the engine #38777

Making it possible to pass Windows credential specs directly to the engine #38777

Conversation

wk8 commented Feb 22, 2019 • edited Loading

codecov bot commented Feb 22, 2019 • edited Loading

Codecov Report

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

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

thaJeztah commented Feb 26, 2019

wk8 commented Feb 26, 2019

GordonTheTurtle commented Mar 6, 2019

wk8 commented Mar 7, 2019

cpuguy83 left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wk8 commented Mar 15, 2019 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

wk8 commented Mar 16, 2019

Drewster727 commented Aug 11, 2019 • edited Loading

wk8 commented Feb 22, 2019 •

edited

Loading

codecov bot commented Feb 22, 2019 •

edited

Loading

wk8 commented Mar 15, 2019 •

edited

Loading

Drewster727 commented Aug 11, 2019 •

edited

Loading