Browse code

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

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

While adding GMSA support to Kubernetes (https://github.com/kubernetes/kubernetes/pull/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
https://github.com/moby/moby/pull/38632 - but alas these tricks are not
available to the Kubelet.)

Signed-off-by: Jean Rouge <rougej+github@gmail.com>

Jean Rouge authored on 2019/03/07 10:44:36
Showing 4 changed files
... ...
@@ -184,7 +184,7 @@ validate: build ## validate DCO, Seccomp profile generation, gofmt,\n./pkg/ isol
184 184
 	$(DOCKER_RUN_DOCKER) hack/validate/all
185 185
 
186 186
 win: build ## cross build the binary for windows
187
-	$(DOCKER_RUN_DOCKER) hack/make.sh win
187
+	$(DOCKER_RUN_DOCKER) DOCKER_CROSSPLATFORMS=windows/amd64 hack/make.sh cross
188 188
 
189 189
 .PHONY: swagger-gen
190 190
 swagger-gen:
... ...
@@ -9,6 +9,7 @@ import (
9 9
 
10 10
 	containertypes "github.com/docker/docker/api/types/container"
11 11
 	"github.com/docker/docker/container"
12
+	"github.com/docker/docker/errdefs"
12 13
 	"github.com/docker/docker/oci"
13 14
 	"github.com/docker/docker/oci/caps"
14 15
 	"github.com/docker/docker/pkg/sysinfo"
... ...
@@ -253,68 +254,8 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S
253 253
 	setResourcesInSpec(c, s, isHyperV)
254 254
 
255 255
 	// Read and add credentials from the security options if a credential spec has been provided.
256
-	if c.HostConfig.SecurityOpt != nil {
257
-		cs := ""
258
-		for _, sOpt := range c.HostConfig.SecurityOpt {
259
-			sOpt = strings.ToLower(sOpt)
260
-			if !strings.Contains(sOpt, "=") {
261
-				return fmt.Errorf("invalid security option: no equals sign in supplied value %s", sOpt)
262
-			}
263
-			var splitsOpt []string
264
-			splitsOpt = strings.SplitN(sOpt, "=", 2)
265
-			if len(splitsOpt) != 2 {
266
-				return fmt.Errorf("invalid security option: %s", sOpt)
267
-			}
268
-			if splitsOpt[0] != "credentialspec" {
269
-				return fmt.Errorf("security option not supported: %s", splitsOpt[0])
270
-			}
271
-
272
-			var (
273
-				match   bool
274
-				csValue string
275
-				err     error
276
-			)
277
-			if match, csValue = getCredentialSpec("file://", splitsOpt[1]); match {
278
-				if csValue == "" {
279
-					return fmt.Errorf("no value supplied for file:// credential spec security option")
280
-				}
281
-				if cs, err = readCredentialSpecFile(c.ID, daemon.root, filepath.Clean(csValue)); err != nil {
282
-					return err
283
-				}
284
-			} else if match, csValue = getCredentialSpec("registry://", splitsOpt[1]); match {
285
-				if csValue == "" {
286
-					return fmt.Errorf("no value supplied for registry:// credential spec security option")
287
-				}
288
-				if cs, err = readCredentialSpecRegistry(c.ID, csValue); err != nil {
289
-					return err
290
-				}
291
-			} else if match, csValue = getCredentialSpec("config://", splitsOpt[1]); match {
292
-				// if the container does not have a DependencyStore, then it
293
-				// isn't swarmkit managed. In order to avoid creating any
294
-				// impression that `config://` is a valid API, return the same
295
-				// error as if you'd passed any other random word.
296
-				if c.DependencyStore == nil {
297
-					return fmt.Errorf("invalid credential spec security option - value must be prefixed file:// or registry:// followed by a value")
298
-				}
299
-
300
-				// after this point, we can return regular swarmkit-relevant
301
-				// errors, because we'll know this container is managed.
302
-				if csValue == "" {
303
-					return fmt.Errorf("no value supplied for config:// credential spec security option")
304
-				}
305
-
306
-				csConfig, err := c.DependencyStore.Configs().Get(csValue)
307
-				if err != nil {
308
-					return errors.Wrap(err, "error getting value from config store")
309
-				}
310
-				// stuff the resulting secret data into a string to use as the
311
-				// CredentialSpec
312
-				cs = string(csConfig.Spec.Data)
313
-			} else {
314
-				return fmt.Errorf("invalid credential spec security option - value must be prefixed file:// or registry:// followed by a value")
315
-			}
316
-		}
317
-		s.Windows.CredentialSpec = cs
256
+	if err := daemon.setWindowsCredentialSpec(c, s); err != nil {
257
+		return err
318 258
 	}
319 259
 
320 260
 	// Do we have any assigned devices?
... ...
@@ -344,6 +285,78 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S
344 344
 	return nil
345 345
 }
346 346
 
347
+var errInvalidCredentialSpecSecOpt = errdefs.InvalidParameter(fmt.Errorf("invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"))
348
+
349
+// setWindowsCredentialSpec sets the spec's `Windows.CredentialSpec`
350
+// field if relevant
351
+func (daemon *Daemon) setWindowsCredentialSpec(c *container.Container, s *specs.Spec) error {
352
+	if c.HostConfig == nil || c.HostConfig.SecurityOpt == nil {
353
+		return nil
354
+	}
355
+
356
+	// TODO (jrouge/wk8): if provided with several security options, we silently ignore
357
+	// all but the last one (provided they're all valid, otherwise we do return an error);
358
+	// this doesn't seem like a great idea?
359
+	credentialSpec := ""
360
+
361
+	for _, secOpt := range c.HostConfig.SecurityOpt {
362
+		optSplits := strings.SplitN(secOpt, "=", 2)
363
+		if len(optSplits) != 2 {
364
+			return errdefs.InvalidParameter(fmt.Errorf("invalid security option: no equals sign in supplied value %s", secOpt))
365
+		}
366
+		if !strings.EqualFold(optSplits[0], "credentialspec") {
367
+			return errdefs.InvalidParameter(fmt.Errorf("security option not supported: %s", optSplits[0]))
368
+		}
369
+
370
+		credSpecSplits := strings.SplitN(optSplits[1], "://", 2)
371
+		if len(credSpecSplits) != 2 || credSpecSplits[1] == "" {
372
+			return errInvalidCredentialSpecSecOpt
373
+		}
374
+		value := credSpecSplits[1]
375
+
376
+		var err error
377
+		switch strings.ToLower(credSpecSplits[0]) {
378
+		case "file":
379
+			if credentialSpec, err = readCredentialSpecFile(c.ID, daemon.root, filepath.Clean(value)); err != nil {
380
+				return errdefs.InvalidParameter(err)
381
+			}
382
+		case "registry":
383
+			if credentialSpec, err = readCredentialSpecRegistry(c.ID, value); err != nil {
384
+				return errdefs.InvalidParameter(err)
385
+			}
386
+		case "config":
387
+			// if the container does not have a DependencyStore, then it
388
+			// isn't swarmkit managed. In order to avoid creating any
389
+			// impression that `config://` is a valid API, return the same
390
+			// error as if you'd passed any other random word.
391
+			if c.DependencyStore == nil {
392
+				return errInvalidCredentialSpecSecOpt
393
+			}
394
+
395
+			csConfig, err := c.DependencyStore.Configs().Get(value)
396
+			if err != nil {
397
+				return errdefs.System(errors.Wrap(err, "error getting value from config store"))
398
+			}
399
+			// stuff the resulting secret data into a string to use as the
400
+			// CredentialSpec
401
+			credentialSpec = string(csConfig.Spec.Data)
402
+		case "raw":
403
+			credentialSpec = value
404
+		default:
405
+			return errInvalidCredentialSpecSecOpt
406
+		}
407
+	}
408
+
409
+	if credentialSpec != "" {
410
+		if s.Windows == nil {
411
+			s.Windows = &specs.Windows{}
412
+		}
413
+		s.Windows.CredentialSpec = credentialSpec
414
+	}
415
+
416
+	return nil
417
+}
418
+
347 419
 // Sets the Linux-specific fields of the OCI spec
348 420
 // TODO: @jhowardmsft LCOW Support. We need to do a lot more pulling in what can
349 421
 // be pulled in from oci_linux.go.
... ...
@@ -427,34 +440,37 @@ func (daemon *Daemon) mergeUlimits(c *containertypes.HostConfig) {
427 427
 	return
428 428
 }
429 429
 
430
-// getCredentialSpec is a helper function to get the value of a credential spec supplied
431
-// on the CLI, stripping the prefix
432
-func getCredentialSpec(prefix, value string) (bool, string) {
433
-	if strings.HasPrefix(value, prefix) {
434
-		return true, strings.TrimPrefix(value, prefix)
435
-	}
436
-	return false, ""
430
+// registryKey is an interface wrapper around `registry.Key`,
431
+// listing only the methods we care about here.
432
+// It's mainly useful to easily allow mocking the registry in tests.
433
+type registryKey interface {
434
+	GetStringValue(name string) (val string, valtype uint32, err error)
435
+	Close() error
436
+}
437
+
438
+var registryOpenKeyFunc = func(baseKey registry.Key, path string, access uint32) (registryKey, error) {
439
+	return registry.OpenKey(baseKey, path, access)
437 440
 }
438 441
 
439 442
 // readCredentialSpecRegistry is a helper function to read a credential spec from
440 443
 // the registry. If not found, we return an empty string and warn in the log.
441 444
 // This allows for staging on machines which do not have the necessary components.
442 445
 func readCredentialSpecRegistry(id, name string) (string, error) {
443
-	var (
444
-		k   registry.Key
445
-		err error
446
-		val string
447
-	)
448
-	if k, err = registry.OpenKey(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.QUERY_VALUE); err != nil {
449
-		return "", fmt.Errorf("failed handling spec %q for container %s - %s could not be opened", name, id, credentialSpecRegistryLocation)
450
-	}
451
-	if val, _, err = k.GetStringValue(name); err != nil {
446
+	key, err := registryOpenKeyFunc(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.QUERY_VALUE)
447
+	if err != nil {
448
+		return "", errors.Wrapf(err, "failed handling spec %q for container %s - registry key %s could not be opened", name, id, credentialSpecRegistryLocation)
449
+	}
450
+	defer key.Close()
451
+
452
+	value, _, err := key.GetStringValue(name)
453
+	if err != nil {
452 454
 		if err == registry.ErrNotExist {
453
-			return "", fmt.Errorf("credential spec %q for container %s as it was not found", name, id)
455
+			return "", fmt.Errorf("registry credential spec %q for container %s was not found", name, id)
454 456
 		}
455
-		return "", fmt.Errorf("error %v reading credential spec %q from registry for container %s", err, name, id)
457
+		return "", errors.Wrapf(err, "error reading credential spec %q from registry for container %s", name, id)
456 458
 	}
457
-	return val, nil
459
+
460
+	return value, nil
458 461
 }
459 462
 
460 463
 // readCredentialSpecFile is a helper function to read a credential spec from
... ...
@@ -471,7 +487,7 @@ func readCredentialSpecFile(id, root, location string) (string, error) {
471 471
 	}
472 472
 	bcontents, err := ioutil.ReadFile(full)
473 473
 	if err != nil {
474
-		return "", fmt.Errorf("credential spec '%s' for container %s as the file could not be read: %q", full, id, err)
474
+		return "", errors.Wrapf(err, "credential spec for container %s could not be read from file %q", id, full)
475 475
 	}
476 476
 	return string(bcontents[:]), nil
477 477
 }
478 478
new file mode 100644
... ...
@@ -0,0 +1,314 @@
0
+package daemon
1
+
2
+import (
3
+	"fmt"
4
+	"io/ioutil"
5
+	"os"
6
+	"path/filepath"
7
+	"strings"
8
+	"testing"
9
+
10
+	"gotest.tools/fs"
11
+
12
+	containertypes "github.com/docker/docker/api/types/container"
13
+	"github.com/docker/docker/container"
14
+	swarmagent "github.com/docker/swarmkit/agent"
15
+	swarmapi "github.com/docker/swarmkit/api"
16
+	"github.com/opencontainers/runtime-spec/specs-go"
17
+	"golang.org/x/sys/windows/registry"
18
+	"gotest.tools/assert"
19
+)
20
+
21
+func TestSetWindowsCredentialSpecInSpec(t *testing.T) {
22
+	// we need a temp directory to act as the daemon's root
23
+	tmpDaemonRoot := fs.NewDir(t, t.Name()).Path()
24
+	defer func() {
25
+		assert.NilError(t, os.RemoveAll(tmpDaemonRoot))
26
+	}()
27
+
28
+	daemon := &Daemon{
29
+		root: tmpDaemonRoot,
30
+	}
31
+
32
+	t.Run("it does nothing if there are no security options", func(t *testing.T) {
33
+		spec := &specs.Spec{}
34
+
35
+		err := daemon.setWindowsCredentialSpec(&container.Container{}, spec)
36
+		assert.NilError(t, err)
37
+		assert.Check(t, spec.Windows == nil)
38
+
39
+		err = daemon.setWindowsCredentialSpec(&container.Container{HostConfig: &containertypes.HostConfig{}}, spec)
40
+		assert.NilError(t, err)
41
+		assert.Check(t, spec.Windows == nil)
42
+
43
+		err = daemon.setWindowsCredentialSpec(&container.Container{HostConfig: &containertypes.HostConfig{SecurityOpt: []string{}}}, spec)
44
+		assert.NilError(t, err)
45
+		assert.Check(t, spec.Windows == nil)
46
+	})
47
+
48
+	dummyContainerID := "dummy-container-ID"
49
+	containerFactory := func(secOpt string) *container.Container {
50
+		if !strings.Contains(secOpt, "=") {
51
+			secOpt = "credentialspec=" + secOpt
52
+		}
53
+		return &container.Container{
54
+			ID: dummyContainerID,
55
+			HostConfig: &containertypes.HostConfig{
56
+				SecurityOpt: []string{secOpt},
57
+			},
58
+		}
59
+	}
60
+
61
+	credSpecsDir := filepath.Join(tmpDaemonRoot, credentialSpecFileLocation)
62
+	dummyCredFileContents := `{"We don't need no": "education"}`
63
+
64
+	t.Run("happy path with a 'file://' option", func(t *testing.T) {
65
+		spec := &specs.Spec{}
66
+
67
+		// let's render a dummy cred file
68
+		err := os.Mkdir(credSpecsDir, os.ModePerm)
69
+		assert.NilError(t, err)
70
+		dummyCredFileName := "dummy-cred-spec.json"
71
+		dummyCredFilePath := filepath.Join(credSpecsDir, dummyCredFileName)
72
+		err = ioutil.WriteFile(dummyCredFilePath, []byte(dummyCredFileContents), 0644)
73
+		defer func() {
74
+			assert.NilError(t, os.Remove(dummyCredFilePath))
75
+		}()
76
+		assert.NilError(t, err)
77
+
78
+		err = daemon.setWindowsCredentialSpec(containerFactory("file://"+dummyCredFileName), spec)
79
+		assert.NilError(t, err)
80
+
81
+		if assert.Check(t, spec.Windows != nil) {
82
+			assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec)
83
+		}
84
+	})
85
+
86
+	t.Run("it's not allowed to use a 'file://' option with an absolute path", func(t *testing.T) {
87
+		spec := &specs.Spec{}
88
+
89
+		err := daemon.setWindowsCredentialSpec(containerFactory(`file://C:\path\to\my\credspec.json`), spec)
90
+		assert.ErrorContains(t, err, "invalid credential spec - file:// path cannot be absolute")
91
+
92
+		assert.Check(t, spec.Windows == nil)
93
+	})
94
+
95
+	t.Run("it's not allowed to use a 'file://' option breaking out of the cred specs' directory", func(t *testing.T) {
96
+		spec := &specs.Spec{}
97
+
98
+		err := daemon.setWindowsCredentialSpec(containerFactory(`file://..\credspec.json`), spec)
99
+		assert.ErrorContains(t, err, fmt.Sprintf("invalid credential spec - file:// path must be under %s", credSpecsDir))
100
+
101
+		assert.Check(t, spec.Windows == nil)
102
+	})
103
+
104
+	t.Run("when using a 'file://' option pointing to a file that doesn't exist, it fails gracefully", func(t *testing.T) {
105
+		spec := &specs.Spec{}
106
+
107
+		err := daemon.setWindowsCredentialSpec(containerFactory("file://i-dont-exist.json"), spec)
108
+		assert.ErrorContains(t, err, fmt.Sprintf("credential spec for container %s could not be read from file", dummyContainerID))
109
+		assert.ErrorContains(t, err, "The system cannot find")
110
+
111
+		assert.Check(t, spec.Windows == nil)
112
+	})
113
+
114
+	t.Run("happy path with a 'registry://' option", func(t *testing.T) {
115
+		valueName := "my-cred-spec"
116
+		key := &dummyRegistryKey{
117
+			getStringValueFunc: func(name string) (val string, valtype uint32, err error) {
118
+				assert.Equal(t, valueName, name)
119
+				return dummyCredFileContents, 0, nil
120
+			},
121
+		}
122
+		defer setRegistryOpenKeyFunc(t, key)()
123
+
124
+		spec := &specs.Spec{}
125
+		assert.NilError(t, daemon.setWindowsCredentialSpec(containerFactory("registry://"+valueName), spec))
126
+
127
+		if assert.Check(t, spec.Windows != nil) {
128
+			assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec)
129
+		}
130
+		assert.Check(t, key.closed)
131
+	})
132
+
133
+	t.Run("when using a 'registry://' option and opening the registry key fails, it fails gracefully", func(t *testing.T) {
134
+		dummyError := fmt.Errorf("dummy error")
135
+		defer setRegistryOpenKeyFunc(t, &dummyRegistryKey{}, dummyError)()
136
+
137
+		spec := &specs.Spec{}
138
+		err := daemon.setWindowsCredentialSpec(containerFactory("registry://my-cred-spec"), spec)
139
+		assert.ErrorContains(t, err, fmt.Sprintf("registry key %s could not be opened: %v", credentialSpecRegistryLocation, dummyError))
140
+
141
+		assert.Check(t, spec.Windows == nil)
142
+	})
143
+
144
+	t.Run("when using a 'registry://' option pointing to a value that doesn't exist, it fails gracefully", func(t *testing.T) {
145
+		valueName := "my-cred-spec"
146
+		key := &dummyRegistryKey{
147
+			getStringValueFunc: func(name string) (val string, valtype uint32, err error) {
148
+				assert.Equal(t, valueName, name)
149
+				return "", 0, registry.ErrNotExist
150
+			},
151
+		}
152
+		defer setRegistryOpenKeyFunc(t, key)()
153
+
154
+		spec := &specs.Spec{}
155
+		err := daemon.setWindowsCredentialSpec(containerFactory("registry://"+valueName), spec)
156
+		assert.ErrorContains(t, err, fmt.Sprintf("registry credential spec %q for container %s was not found", valueName, dummyContainerID))
157
+
158
+		assert.Check(t, key.closed)
159
+	})
160
+
161
+	t.Run("when using a 'registry://' option and reading the registry value fails, it fails gracefully", func(t *testing.T) {
162
+		dummyError := fmt.Errorf("dummy error")
163
+		valueName := "my-cred-spec"
164
+		key := &dummyRegistryKey{
165
+			getStringValueFunc: func(name string) (val string, valtype uint32, err error) {
166
+				assert.Equal(t, valueName, name)
167
+				return "", 0, dummyError
168
+			},
169
+		}
170
+		defer setRegistryOpenKeyFunc(t, key)()
171
+
172
+		spec := &specs.Spec{}
173
+		err := daemon.setWindowsCredentialSpec(containerFactory("registry://"+valueName), spec)
174
+		assert.ErrorContains(t, err, fmt.Sprintf("error reading credential spec %q from registry for container %s: %v", valueName, dummyContainerID, dummyError))
175
+
176
+		assert.Check(t, key.closed)
177
+	})
178
+
179
+	t.Run("happy path with a 'config://' option", func(t *testing.T) {
180
+		configID := "my-cred-spec"
181
+
182
+		dependencyManager := swarmagent.NewDependencyManager()
183
+		dependencyManager.Configs().Add(swarmapi.Config{
184
+			ID: configID,
185
+			Spec: swarmapi.ConfigSpec{
186
+				Data: []byte(dummyCredFileContents),
187
+			},
188
+		})
189
+
190
+		task := &swarmapi.Task{
191
+			Spec: swarmapi.TaskSpec{
192
+				Runtime: &swarmapi.TaskSpec_Container{
193
+					Container: &swarmapi.ContainerSpec{
194
+						Configs: []*swarmapi.ConfigReference{
195
+							{
196
+								ConfigID: configID,
197
+							},
198
+						},
199
+					},
200
+				},
201
+			},
202
+		}
203
+
204
+		cntr := containerFactory("config://" + configID)
205
+		cntr.DependencyStore = swarmagent.Restrict(dependencyManager, task)
206
+
207
+		spec := &specs.Spec{}
208
+		err := daemon.setWindowsCredentialSpec(cntr, spec)
209
+		assert.NilError(t, err)
210
+
211
+		if assert.Check(t, spec.Windows != nil) {
212
+			assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec)
213
+		}
214
+	})
215
+
216
+	t.Run("using a 'config://' option on a container not managed by swarmkit is not allowed, and results in a generic error message to hide that purely internal API", func(t *testing.T) {
217
+		spec := &specs.Spec{}
218
+
219
+		err := daemon.setWindowsCredentialSpec(containerFactory("config://whatever"), spec)
220
+		assert.Equal(t, errInvalidCredentialSpecSecOpt, err)
221
+
222
+		assert.Check(t, spec.Windows == nil)
223
+	})
224
+
225
+	t.Run("happy path with a 'raw://' option", func(t *testing.T) {
226
+		spec := &specs.Spec{}
227
+
228
+		err := daemon.setWindowsCredentialSpec(containerFactory("raw://"+dummyCredFileContents), spec)
229
+		assert.NilError(t, err)
230
+
231
+		if assert.Check(t, spec.Windows != nil) {
232
+			assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec)
233
+		}
234
+	})
235
+
236
+	t.Run("it's not case sensitive in the option names", func(t *testing.T) {
237
+		spec := &specs.Spec{}
238
+
239
+		err := daemon.setWindowsCredentialSpec(containerFactory("CreDENtiaLSPeC=rAw://"+dummyCredFileContents), spec)
240
+		assert.NilError(t, err)
241
+
242
+		if assert.Check(t, spec.Windows != nil) {
243
+			assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec)
244
+		}
245
+	})
246
+
247
+	t.Run("it rejects unknown options", func(t *testing.T) {
248
+		spec := &specs.Spec{}
249
+
250
+		err := daemon.setWindowsCredentialSpec(containerFactory("credentialspe=config://whatever"), spec)
251
+		assert.ErrorContains(t, err, "security option not supported: credentialspe")
252
+
253
+		assert.Check(t, spec.Windows == nil)
254
+	})
255
+
256
+	t.Run("it rejects unsupported credentialspec options", func(t *testing.T) {
257
+		spec := &specs.Spec{}
258
+
259
+		err := daemon.setWindowsCredentialSpec(containerFactory("idontexist://whatever"), spec)
260
+		assert.Equal(t, errInvalidCredentialSpecSecOpt, err)
261
+
262
+		assert.Check(t, spec.Windows == nil)
263
+	})
264
+
265
+	for _, option := range []string{"file", "registry", "config", "raw"} {
266
+		t.Run(fmt.Sprintf("it rejects empty values for %s", option), func(t *testing.T) {
267
+			spec := &specs.Spec{}
268
+
269
+			err := daemon.setWindowsCredentialSpec(containerFactory(option+"://"), spec)
270
+			assert.Equal(t, errInvalidCredentialSpecSecOpt, err)
271
+
272
+			assert.Check(t, spec.Windows == nil)
273
+		})
274
+	}
275
+}
276
+
277
+/* Helpers below */
278
+
279
+type dummyRegistryKey struct {
280
+	getStringValueFunc func(name string) (val string, valtype uint32, err error)
281
+	closed             bool
282
+}
283
+
284
+func (k *dummyRegistryKey) GetStringValue(name string) (val string, valtype uint32, err error) {
285
+	return k.getStringValueFunc(name)
286
+}
287
+
288
+func (k *dummyRegistryKey) Close() error {
289
+	k.closed = true
290
+	return nil
291
+}
292
+
293
+// setRegistryOpenKeyFunc replaces the registryOpenKeyFunc package variable, and returns a function
294
+// to be called to revert the change when done with testing.
295
+func setRegistryOpenKeyFunc(t *testing.T, key *dummyRegistryKey, err ...error) func() {
296
+	previousRegistryOpenKeyFunc := registryOpenKeyFunc
297
+
298
+	registryOpenKeyFunc = func(baseKey registry.Key, path string, access uint32) (registryKey, error) {
299
+		// this should always be called with exactly the same arguments
300
+		assert.Equal(t, registry.LOCAL_MACHINE, baseKey)
301
+		assert.Equal(t, credentialSpecRegistryLocation, path)
302
+		assert.Equal(t, uint32(registry.QUERY_VALUE), access)
303
+
304
+		if len(err) > 0 {
305
+			return nil, err[0]
306
+		}
307
+		return key, nil
308
+	}
309
+
310
+	return func() {
311
+		registryOpenKeyFunc = previousRegistryOpenKeyFunc
312
+	}
313
+}
... ...
@@ -4145,29 +4145,37 @@ func (s *DockerSuite) TestRunStoppedLoggingDriverNoLeak(c *check.C) {
4145 4145
 // requires additional infrastructure (AD for example) on CI servers.
4146 4146
 func (s *DockerSuite) TestRunCredentialSpecFailures(c *check.C) {
4147 4147
 	testRequires(c, DaemonIsWindows)
4148
+
4148 4149
 	attempts := []struct{ value, expectedError string }{
4149
-		{"rubbish", "invalid credential spec security option - value must be prefixed file:// or registry://"},
4150
-		{"rubbish://", "invalid credential spec security option - value must be prefixed file:// or registry://"},
4151
-		{"file://", "no value supplied for file:// credential spec security option"},
4152
-		{"registry://", "no value supplied for registry:// credential spec security option"},
4150
+		{"rubbish", "invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"},
4151
+		{"rubbish://", "invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"},
4152
+		{"file://", "invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"},
4153
+		{"registry://", "invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"},
4153 4154
 		{`file://c:\blah.txt`, "path cannot be absolute"},
4154 4155
 		{`file://doesnotexist.txt`, "The system cannot find the file specified"},
4155 4156
 	}
4156 4157
 	for _, attempt := range attempts {
4157 4158
 		_, _, err := dockerCmdWithError("run", "--security-opt=credentialspec="+attempt.value, "busybox", "true")
4158 4159
 		c.Assert(err, checker.NotNil, check.Commentf("%s expected non-nil err", attempt.value))
4159
-		c.Assert(err.Error(), checker.Contains, attempt.expectedError, check.Commentf("%s expected %s got %s", attempt.value, attempt.expectedError, err))
4160
+		c.Check(err.Error(), checker.Contains, attempt.expectedError, check.Commentf("%s expected %s got %s", attempt.value, attempt.expectedError, err))
4160 4161
 	}
4161 4162
 }
4162 4163
 
4163 4164
 // Windows specific test to validate credential specs with a well-formed spec.
4164
-// Note it won't actually do anything in CI configuration with the spec, but
4165
-// it should not fail to run a container.
4166 4165
 func (s *DockerSuite) TestRunCredentialSpecWellFormed(c *check.C) {
4167 4166
 	testRequires(c, DaemonIsWindows, testEnv.IsLocalDaemon)
4168
-	validCS := readFile(`fixtures\credentialspecs\valid.json`, c)
4169
-	writeFile(filepath.Join(testEnv.DaemonInfo.DockerRootDir, `credentialspecs\valid.json`), validCS, c)
4170
-	dockerCmd(c, "run", `--security-opt=credentialspec=file://valid.json`, "busybox", "true")
4167
+
4168
+	validCredSpecs := readFile(`fixtures\credentialspecs\valid.json`, c)
4169
+	writeFile(filepath.Join(testEnv.DaemonInfo.DockerRootDir, `credentialspecs\valid.json`), validCredSpecs, c)
4170
+
4171
+	for _, value := range []string{"file://valid.json", "raw://" + validCredSpecs} {
4172
+		// `nltest /PARENTDOMAIN` simply reads the local config, and does not require having an AD
4173
+		// controller handy
4174
+		out, _ := dockerCmd(c, "run", "--rm", "--security-opt=credentialspec="+value, minimalBaseImage(), "nltest", "/PARENTDOMAIN")
4175
+
4176
+		c.Check(out, checker.Contains, "hyperv.local.")
4177
+		c.Check(out, checker.Contains, "The command completed successfully")
4178
+	}
4171 4179
 }
4172 4180
 
4173 4181
 func (s *DockerSuite) TestRunDuplicateMount(c *check.C) {