diff --git a/Makefile b/Makefile index 67f6c3dab1..bc8e71d070 100644 --- a/Makefile +++ b/Makefile @@ -184,7 +184,7 @@ validate: build ## validate DCO, Seccomp profile generation, gofmt,\n./pkg/ isol $(DOCKER_RUN_DOCKER) hack/validate/all win: build ## cross build the binary for windows - $(DOCKER_RUN_DOCKER) hack/make.sh win + $(DOCKER_RUN_DOCKER) DOCKER_CROSSPLATFORMS=windows/amd64 hack/make.sh cross .PHONY: swagger-gen swagger-gen: diff --git a/daemon/oci_windows.go b/daemon/oci_windows.go index 215d5b6d3a..a1ceea9956 100644 --- a/daemon/oci_windows.go +++ b/daemon/oci_windows.go @@ -9,6 +9,7 @@ import ( containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/container" + "github.com/docker/docker/errdefs" "github.com/docker/docker/oci" "github.com/docker/docker/oci/caps" "github.com/docker/docker/pkg/sysinfo" @@ -253,68 +254,8 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S setResourcesInSpec(c, s, isHyperV) // Read and add credentials from the security options if a credential spec has been provided. - if c.HostConfig.SecurityOpt != nil { - cs := "" - for _, sOpt := range c.HostConfig.SecurityOpt { - sOpt = strings.ToLower(sOpt) - if !strings.Contains(sOpt, "=") { - return fmt.Errorf("invalid security option: no equals sign in supplied value %s", sOpt) - } - var splitsOpt []string - splitsOpt = strings.SplitN(sOpt, "=", 2) - if len(splitsOpt) != 2 { - return fmt.Errorf("invalid security option: %s", sOpt) - } - if splitsOpt[0] != "credentialspec" { - return fmt.Errorf("security option not supported: %s", splitsOpt[0]) - } - - var ( - match bool - csValue string - err error - ) - if match, csValue = getCredentialSpec("file://", splitsOpt[1]); match { - if csValue == "" { - return fmt.Errorf("no value supplied for file:// credential spec security option") - } - if cs, err = readCredentialSpecFile(c.ID, daemon.root, filepath.Clean(csValue)); err != nil { - return err - } - } else if match, csValue = getCredentialSpec("registry://", splitsOpt[1]); match { - if csValue == "" { - return fmt.Errorf("no value supplied for registry:// credential spec security option") - } - if cs, err = readCredentialSpecRegistry(c.ID, csValue); err != nil { - return err - } - } else if match, csValue = getCredentialSpec("config://", splitsOpt[1]); match { - // if the container does not have a DependencyStore, then it - // isn't swarmkit managed. In order to avoid creating any - // impression that `config://` is a valid API, return the same - // error as if you'd passed any other random word. - if c.DependencyStore == nil { - return fmt.Errorf("invalid credential spec security option - value must be prefixed file:// or registry:// followed by a value") - } - - // after this point, we can return regular swarmkit-relevant - // errors, because we'll know this container is managed. - if csValue == "" { - return fmt.Errorf("no value supplied for config:// credential spec security option") - } - - csConfig, err := c.DependencyStore.Configs().Get(csValue) - if err != nil { - return errors.Wrap(err, "error getting value from config store") - } - // stuff the resulting secret data into a string to use as the - // CredentialSpec - cs = string(csConfig.Spec.Data) - } else { - return fmt.Errorf("invalid credential spec security option - value must be prefixed file:// or registry:// followed by a value") - } - } - s.Windows.CredentialSpec = cs + if err := daemon.setWindowsCredentialSpec(c, s); err != nil { + return err } // Do we have any assigned devices? @@ -344,6 +285,78 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S return nil } +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")) + +// setWindowsCredentialSpec sets the spec's `Windows.CredentialSpec` +// field if relevant +func (daemon *Daemon) setWindowsCredentialSpec(c *container.Container, s *specs.Spec) error { + if c.HostConfig == nil || c.HostConfig.SecurityOpt == nil { + return nil + } + + // TODO (jrouge/wk8): if provided with several security options, we silently ignore + // all but the last one (provided they're all valid, otherwise we do return an error); + // this doesn't seem like a great idea? + credentialSpec := "" + + for _, secOpt := range c.HostConfig.SecurityOpt { + optSplits := strings.SplitN(secOpt, "=", 2) + if len(optSplits) != 2 { + return errdefs.InvalidParameter(fmt.Errorf("invalid security option: no equals sign in supplied value %s", secOpt)) + } + if !strings.EqualFold(optSplits[0], "credentialspec") { + return errdefs.InvalidParameter(fmt.Errorf("security option not supported: %s", optSplits[0])) + } + + credSpecSplits := strings.SplitN(optSplits[1], "://", 2) + if len(credSpecSplits) != 2 || credSpecSplits[1] == "" { + return errInvalidCredentialSpecSecOpt + } + value := credSpecSplits[1] + + var err error + switch strings.ToLower(credSpecSplits[0]) { + case "file": + if credentialSpec, err = readCredentialSpecFile(c.ID, daemon.root, filepath.Clean(value)); err != nil { + return errdefs.InvalidParameter(err) + } + case "registry": + if credentialSpec, err = readCredentialSpecRegistry(c.ID, value); err != nil { + return errdefs.InvalidParameter(err) + } + case "config": + // if the container does not have a DependencyStore, then it + // isn't swarmkit managed. In order to avoid creating any + // impression that `config://` is a valid API, return the same + // error as if you'd passed any other random word. + if c.DependencyStore == nil { + return errInvalidCredentialSpecSecOpt + } + + csConfig, err := c.DependencyStore.Configs().Get(value) + if err != nil { + return errdefs.System(errors.Wrap(err, "error getting value from config store")) + } + // stuff the resulting secret data into a string to use as the + // CredentialSpec + credentialSpec = string(csConfig.Spec.Data) + case "raw": + credentialSpec = value + default: + return errInvalidCredentialSpecSecOpt + } + } + + if credentialSpec != "" { + if s.Windows == nil { + s.Windows = &specs.Windows{} + } + s.Windows.CredentialSpec = credentialSpec + } + + return nil +} + // Sets the Linux-specific fields of the OCI spec // TODO: @jhowardmsft LCOW Support. We need to do a lot more pulling in what can // be pulled in from oci_linux.go. @@ -427,34 +440,37 @@ func (daemon *Daemon) mergeUlimits(c *containertypes.HostConfig) { return } -// getCredentialSpec is a helper function to get the value of a credential spec supplied -// on the CLI, stripping the prefix -func getCredentialSpec(prefix, value string) (bool, string) { - if strings.HasPrefix(value, prefix) { - return true, strings.TrimPrefix(value, prefix) - } - return false, "" +// registryKey is an interface wrapper around `registry.Key`, +// listing only the methods we care about here. +// It's mainly useful to easily allow mocking the registry in tests. +type registryKey interface { + GetStringValue(name string) (val string, valtype uint32, err error) + Close() error +} + +var registryOpenKeyFunc = func(baseKey registry.Key, path string, access uint32) (registryKey, error) { + return registry.OpenKey(baseKey, path, access) } // readCredentialSpecRegistry is a helper function to read a credential spec from // the registry. If not found, we return an empty string and warn in the log. // This allows for staging on machines which do not have the necessary components. func readCredentialSpecRegistry(id, name string) (string, error) { - var ( - k registry.Key - err error - val string - ) - if k, err = registry.OpenKey(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.QUERY_VALUE); err != nil { - return "", fmt.Errorf("failed handling spec %q for container %s - %s could not be opened", name, id, credentialSpecRegistryLocation) + key, err := registryOpenKeyFunc(registry.LOCAL_MACHINE, credentialSpecRegistryLocation, registry.QUERY_VALUE) + if err != nil { + return "", errors.Wrapf(err, "failed handling spec %q for container %s - registry key %s could not be opened", name, id, credentialSpecRegistryLocation) } - if val, _, err = k.GetStringValue(name); err != nil { + defer key.Close() + + value, _, err := key.GetStringValue(name) + if err != nil { if err == registry.ErrNotExist { - return "", fmt.Errorf("credential spec %q for container %s as it was not found", name, id) + return "", fmt.Errorf("registry credential spec %q for container %s was not found", name, id) } - return "", fmt.Errorf("error %v reading credential spec %q from registry for container %s", err, name, id) + return "", errors.Wrapf(err, "error reading credential spec %q from registry for container %s", name, id) } - return val, nil + + return value, nil } // readCredentialSpecFile is a helper function to read a credential spec from @@ -471,7 +487,7 @@ func readCredentialSpecFile(id, root, location string) (string, error) { } bcontents, err := ioutil.ReadFile(full) if err != nil { - return "", fmt.Errorf("credential spec '%s' for container %s as the file could not be read: %q", full, id, err) + return "", errors.Wrapf(err, "credential spec for container %s could not be read from file %q", id, full) } return string(bcontents[:]), nil } diff --git a/daemon/oci_windows_test.go b/daemon/oci_windows_test.go new file mode 100644 index 0000000000..370187afcb --- /dev/null +++ b/daemon/oci_windows_test.go @@ -0,0 +1,314 @@ +package daemon + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + "strings" + "testing" + + "gotest.tools/fs" + + containertypes "github.com/docker/docker/api/types/container" + "github.com/docker/docker/container" + swarmagent "github.com/docker/swarmkit/agent" + swarmapi "github.com/docker/swarmkit/api" + "github.com/opencontainers/runtime-spec/specs-go" + "golang.org/x/sys/windows/registry" + "gotest.tools/assert" +) + +func TestSetWindowsCredentialSpecInSpec(t *testing.T) { + // we need a temp directory to act as the daemon's root + tmpDaemonRoot := fs.NewDir(t, t.Name()).Path() + defer func() { + assert.NilError(t, os.RemoveAll(tmpDaemonRoot)) + }() + + daemon := &Daemon{ + root: tmpDaemonRoot, + } + + t.Run("it does nothing if there are no security options", func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(&container.Container{}, spec) + assert.NilError(t, err) + assert.Check(t, spec.Windows == nil) + + err = daemon.setWindowsCredentialSpec(&container.Container{HostConfig: &containertypes.HostConfig{}}, spec) + assert.NilError(t, err) + assert.Check(t, spec.Windows == nil) + + err = daemon.setWindowsCredentialSpec(&container.Container{HostConfig: &containertypes.HostConfig{SecurityOpt: []string{}}}, spec) + assert.NilError(t, err) + assert.Check(t, spec.Windows == nil) + }) + + dummyContainerID := "dummy-container-ID" + containerFactory := func(secOpt string) *container.Container { + if !strings.Contains(secOpt, "=") { + secOpt = "credentialspec=" + secOpt + } + return &container.Container{ + ID: dummyContainerID, + HostConfig: &containertypes.HostConfig{ + SecurityOpt: []string{secOpt}, + }, + } + } + + credSpecsDir := filepath.Join(tmpDaemonRoot, credentialSpecFileLocation) + dummyCredFileContents := `{"We don't need no": "education"}` + + t.Run("happy path with a 'file://' option", func(t *testing.T) { + spec := &specs.Spec{} + + // let's render a dummy cred file + err := os.Mkdir(credSpecsDir, os.ModePerm) + assert.NilError(t, err) + dummyCredFileName := "dummy-cred-spec.json" + dummyCredFilePath := filepath.Join(credSpecsDir, dummyCredFileName) + err = ioutil.WriteFile(dummyCredFilePath, []byte(dummyCredFileContents), 0644) + defer func() { + assert.NilError(t, os.Remove(dummyCredFilePath)) + }() + assert.NilError(t, err) + + err = daemon.setWindowsCredentialSpec(containerFactory("file://"+dummyCredFileName), spec) + assert.NilError(t, err) + + if assert.Check(t, spec.Windows != nil) { + assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec) + } + }) + + t.Run("it's not allowed to use a 'file://' option with an absolute path", func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory(`file://C:\path\to\my\credspec.json`), spec) + assert.ErrorContains(t, err, "invalid credential spec - file:// path cannot be absolute") + + assert.Check(t, spec.Windows == nil) + }) + + t.Run("it's not allowed to use a 'file://' option breaking out of the cred specs' directory", func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory(`file://..\credspec.json`), spec) + assert.ErrorContains(t, err, fmt.Sprintf("invalid credential spec - file:// path must be under %s", credSpecsDir)) + + assert.Check(t, spec.Windows == nil) + }) + + t.Run("when using a 'file://' option pointing to a file that doesn't exist, it fails gracefully", func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory("file://i-dont-exist.json"), spec) + assert.ErrorContains(t, err, fmt.Sprintf("credential spec for container %s could not be read from file", dummyContainerID)) + assert.ErrorContains(t, err, "The system cannot find") + + assert.Check(t, spec.Windows == nil) + }) + + t.Run("happy path with a 'registry://' option", func(t *testing.T) { + valueName := "my-cred-spec" + key := &dummyRegistryKey{ + getStringValueFunc: func(name string) (val string, valtype uint32, err error) { + assert.Equal(t, valueName, name) + return dummyCredFileContents, 0, nil + }, + } + defer setRegistryOpenKeyFunc(t, key)() + + spec := &specs.Spec{} + assert.NilError(t, daemon.setWindowsCredentialSpec(containerFactory("registry://"+valueName), spec)) + + if assert.Check(t, spec.Windows != nil) { + assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec) + } + assert.Check(t, key.closed) + }) + + t.Run("when using a 'registry://' option and opening the registry key fails, it fails gracefully", func(t *testing.T) { + dummyError := fmt.Errorf("dummy error") + defer setRegistryOpenKeyFunc(t, &dummyRegistryKey{}, dummyError)() + + spec := &specs.Spec{} + err := daemon.setWindowsCredentialSpec(containerFactory("registry://my-cred-spec"), spec) + assert.ErrorContains(t, err, fmt.Sprintf("registry key %s could not be opened: %v", credentialSpecRegistryLocation, dummyError)) + + assert.Check(t, spec.Windows == nil) + }) + + t.Run("when using a 'registry://' option pointing to a value that doesn't exist, it fails gracefully", func(t *testing.T) { + valueName := "my-cred-spec" + key := &dummyRegistryKey{ + getStringValueFunc: func(name string) (val string, valtype uint32, err error) { + assert.Equal(t, valueName, name) + return "", 0, registry.ErrNotExist + }, + } + defer setRegistryOpenKeyFunc(t, key)() + + spec := &specs.Spec{} + err := daemon.setWindowsCredentialSpec(containerFactory("registry://"+valueName), spec) + assert.ErrorContains(t, err, fmt.Sprintf("registry credential spec %q for container %s was not found", valueName, dummyContainerID)) + + assert.Check(t, key.closed) + }) + + t.Run("when using a 'registry://' option and reading the registry value fails, it fails gracefully", func(t *testing.T) { + dummyError := fmt.Errorf("dummy error") + valueName := "my-cred-spec" + key := &dummyRegistryKey{ + getStringValueFunc: func(name string) (val string, valtype uint32, err error) { + assert.Equal(t, valueName, name) + return "", 0, dummyError + }, + } + defer setRegistryOpenKeyFunc(t, key)() + + spec := &specs.Spec{} + err := daemon.setWindowsCredentialSpec(containerFactory("registry://"+valueName), spec) + assert.ErrorContains(t, err, fmt.Sprintf("error reading credential spec %q from registry for container %s: %v", valueName, dummyContainerID, dummyError)) + + assert.Check(t, key.closed) + }) + + t.Run("happy path with a 'config://' option", func(t *testing.T) { + configID := "my-cred-spec" + + dependencyManager := swarmagent.NewDependencyManager() + dependencyManager.Configs().Add(swarmapi.Config{ + ID: configID, + Spec: swarmapi.ConfigSpec{ + Data: []byte(dummyCredFileContents), + }, + }) + + task := &swarmapi.Task{ + Spec: swarmapi.TaskSpec{ + Runtime: &swarmapi.TaskSpec_Container{ + Container: &swarmapi.ContainerSpec{ + Configs: []*swarmapi.ConfigReference{ + { + ConfigID: configID, + }, + }, + }, + }, + }, + } + + cntr := containerFactory("config://" + configID) + cntr.DependencyStore = swarmagent.Restrict(dependencyManager, task) + + spec := &specs.Spec{} + err := daemon.setWindowsCredentialSpec(cntr, spec) + assert.NilError(t, err) + + if assert.Check(t, spec.Windows != nil) { + assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec) + } + }) + + 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) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory("config://whatever"), spec) + assert.Equal(t, errInvalidCredentialSpecSecOpt, err) + + assert.Check(t, spec.Windows == nil) + }) + + t.Run("happy path with a 'raw://' option", func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory("raw://"+dummyCredFileContents), spec) + assert.NilError(t, err) + + if assert.Check(t, spec.Windows != nil) { + assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec) + } + }) + + t.Run("it's not case sensitive in the option names", func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory("CreDENtiaLSPeC=rAw://"+dummyCredFileContents), spec) + assert.NilError(t, err) + + if assert.Check(t, spec.Windows != nil) { + assert.Equal(t, dummyCredFileContents, spec.Windows.CredentialSpec) + } + }) + + t.Run("it rejects unknown options", func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory("credentialspe=config://whatever"), spec) + assert.ErrorContains(t, err, "security option not supported: credentialspe") + + assert.Check(t, spec.Windows == nil) + }) + + t.Run("it rejects unsupported credentialspec options", func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory("idontexist://whatever"), spec) + assert.Equal(t, errInvalidCredentialSpecSecOpt, err) + + assert.Check(t, spec.Windows == nil) + }) + + for _, option := range []string{"file", "registry", "config", "raw"} { + t.Run(fmt.Sprintf("it rejects empty values for %s", option), func(t *testing.T) { + spec := &specs.Spec{} + + err := daemon.setWindowsCredentialSpec(containerFactory(option+"://"), spec) + assert.Equal(t, errInvalidCredentialSpecSecOpt, err) + + assert.Check(t, spec.Windows == nil) + }) + } +} + +/* Helpers below */ + +type dummyRegistryKey struct { + getStringValueFunc func(name string) (val string, valtype uint32, err error) + closed bool +} + +func (k *dummyRegistryKey) GetStringValue(name string) (val string, valtype uint32, err error) { + return k.getStringValueFunc(name) +} + +func (k *dummyRegistryKey) Close() error { + k.closed = true + return nil +} + +// setRegistryOpenKeyFunc replaces the registryOpenKeyFunc package variable, and returns a function +// to be called to revert the change when done with testing. +func setRegistryOpenKeyFunc(t *testing.T, key *dummyRegistryKey, err ...error) func() { + previousRegistryOpenKeyFunc := registryOpenKeyFunc + + registryOpenKeyFunc = func(baseKey registry.Key, path string, access uint32) (registryKey, error) { + // this should always be called with exactly the same arguments + assert.Equal(t, registry.LOCAL_MACHINE, baseKey) + assert.Equal(t, credentialSpecRegistryLocation, path) + assert.Equal(t, uint32(registry.QUERY_VALUE), access) + + if len(err) > 0 { + return nil, err[0] + } + return key, nil + } + + return func() { + registryOpenKeyFunc = previousRegistryOpenKeyFunc + } +} diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 81232a5d6a..2a02edc7f3 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -4145,29 +4145,37 @@ func (s *DockerSuite) TestRunStoppedLoggingDriverNoLeak(c *check.C) { // requires additional infrastructure (AD for example) on CI servers. func (s *DockerSuite) TestRunCredentialSpecFailures(c *check.C) { testRequires(c, DaemonIsWindows) + attempts := []struct{ value, expectedError string }{ - {"rubbish", "invalid credential spec security option - value must be prefixed file:// or registry://"}, - {"rubbish://", "invalid credential spec security option - value must be prefixed file:// or registry://"}, - {"file://", "no value supplied for file:// credential spec security option"}, - {"registry://", "no value supplied for registry:// credential spec security option"}, + {"rubbish", "invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"}, + {"rubbish://", "invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"}, + {"file://", "invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"}, + {"registry://", "invalid credential spec security option - value must be prefixed by 'file://', 'registry://', or 'raw://' followed by a non-empty value"}, {`file://c:\blah.txt`, "path cannot be absolute"}, {`file://doesnotexist.txt`, "The system cannot find the file specified"}, } for _, attempt := range attempts { _, _, err := dockerCmdWithError("run", "--security-opt=credentialspec="+attempt.value, "busybox", "true") c.Assert(err, checker.NotNil, check.Commentf("%s expected non-nil err", attempt.value)) - c.Assert(err.Error(), checker.Contains, attempt.expectedError, check.Commentf("%s expected %s got %s", attempt.value, attempt.expectedError, err)) + c.Check(err.Error(), checker.Contains, attempt.expectedError, check.Commentf("%s expected %s got %s", attempt.value, attempt.expectedError, err)) } } // Windows specific test to validate credential specs with a well-formed spec. -// Note it won't actually do anything in CI configuration with the spec, but -// it should not fail to run a container. func (s *DockerSuite) TestRunCredentialSpecWellFormed(c *check.C) { testRequires(c, DaemonIsWindows, testEnv.IsLocalDaemon) - validCS := readFile(`fixtures\credentialspecs\valid.json`, c) - writeFile(filepath.Join(testEnv.DaemonInfo.DockerRootDir, `credentialspecs\valid.json`), validCS, c) - dockerCmd(c, "run", `--security-opt=credentialspec=file://valid.json`, "busybox", "true") + + validCredSpecs := readFile(`fixtures\credentialspecs\valid.json`, c) + writeFile(filepath.Join(testEnv.DaemonInfo.DockerRootDir, `credentialspecs\valid.json`), validCredSpecs, c) + + for _, value := range []string{"file://valid.json", "raw://" + validCredSpecs} { + // `nltest /PARENTDOMAIN` simply reads the local config, and does not require having an AD + // controller handy + out, _ := dockerCmd(c, "run", "--rm", "--security-opt=credentialspec="+value, minimalBaseImage(), "nltest", "/PARENTDOMAIN") + + c.Check(out, checker.Contains, "hyperv.local.") + c.Check(out, checker.Contains, "The command completed successfully") + } } func (s *DockerSuite) TestRunDuplicateMount(c *check.C) {