8000 Making it possible to pass Windows credential specs directly to the engine by wk8 · Pull Request #38777 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 16, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Making it possible to pass Windows credential specs directly to the e… 
…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
#38632 - but alas these tricks are not
available to the Kubelet.)

Signed-off-by: Jean Rouge <rougej+github@gmail.com>
  • Loading branch information
@wk8
wk8 committed Mar 16, 2019
commit 7fdac7eb0ff836633c0a08c430b9472a3bfd3e20
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
180 changes: 98 additions & 82 deletions daemon/oci_windows.go
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -253,68 +254,8 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *spec 8000 s.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?
Expand Down Expand Up @@ -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":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same question, actually :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just in a separate commit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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.
Expand Down Expand Up @@ -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)
}
if val, _, err = k.GetStringValue(name); err != nil {
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)
}
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
Expand All @@ -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
}
Loading
0