Skip to content
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.

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

Add Windows secrets support #32208

Merged
merged 1 commit into from May 16, 2017

Conversation

@johnstep
Copy link
Member

commented Mar 29, 2017

Signed-off-by: John Stephens johnstep@docker.com

This change implements secrets support for Windows by writing to persistent files on the host, due to the lack of a RAM disk driver built in to Windows. A separate change will add RAM disk support later.

@jhowardmsft @PatrickLang @ehazlett @diogomonica @cyli

@ehazlett

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2017

+1 to get to a ramdisk (or similar) backend so we don't have to persist these on disk. as long as we ensure the permissions and access is limited this sgtm as a start.

design LGTM

@johnstep

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2017

@ehazlett Since secrets are stored in the Docker runtime root (%ProgramData%\Docker by default), the secrets directories inherit those permissions, only granting administrators and system access:

$ icacls %ProgramData%\Docker
C:\ProgramData\Docker BUILTIN\Administrators:(OI)(CI)(F)
                      NT AUTHORITY\SYSTEM:(OI)(CI)(F)

Successfully processed 1 files; Failed processing 0 files

However, for a custom runtime root (e.g. --graph A:\docker), security is based on that custom root directory. We could explicitly force stricter permissions on each secrets directory.

@diogomonica

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2017

Thanks for the PR @johnstep.

  • We need a commitment on the timeline for the RamFS before we merge this
  • We need the docs to reflect that secrets will not be as secure on windows as they are on Linux.
@ehazlett

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2017

+1 for documenting differences between Windows and Linux

@johnstep

We could explicitly force stricter permissions on each secrets directory.

Yes +1. I say we do this regardless of the Docker root to be sure nothing would accidentally leak and to make sure at minimum the secrets directory has the proper access restrictions.

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Mar 30, 2017

Can we disable docker commit when secret is mounted on the rootfs rather than on RAM disk?

@johnstep

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

@AkihiroSuda Windows does not support commit of a running container, and Docker deletes secrets when stopping a container. If the daemon terminates abnormally, it deletes the secrets on restarting.

@johnstep

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

@mstanleyjones We need to document that secrets for Windows will be persisted on the host, in the container directory. The secrets directory is deleted when the container is stopped, or when the daemon is restarted, in the case of abnormal termination. In the short term, customers can mitigate this concern by enabling BitLocker on the volume containing the Docker runtime root.

Separately, we are working on a RAM disk driver for consistency with Linux.

@johnstep johnstep force-pushed the johnstep:windows-secrets branch from 9b51ab0 to cb73b2c Mar 30, 2017

@johnstep

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

Updated to always secure secrets directories explicitly, and enabled integration tests.

}
if err := ioutil.WriteFile(fPath, secret.Spec.Data, s.File.Mode); err != nil {
return errors.Wrap(err, "error injecting secret")
}

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft Mar 30, 2017

Contributor

I'd really like to see much of this combined with the (almost) identical code in container_operations_unix.go

This comment has been minimized.

Copy link
@johnstep

johnstep May 5, 2017

Author Member

I looked at this, and definitely can move some of it out into a shared file, but would rather keep it as is for now, and factor it out in a future change.

func (container *Container) SecretMountPath() string {
return filepath.Join(container.Root, "secrets")
}

// IpcMounts returns the list of Ipc related mounts.
func (container *Container) IpcMounts() []Mount {
return nil
}

// SecretMount returns the mount for the secret path
func (container *Container) SecretMount() *Mount {

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft Mar 30, 2017

Contributor

This is identical to SecretMount() in container_unix.go. Can it be combined?

This comment has been minimized.

Copy link
@johnstep

johnstep May 5, 2017

Author Member

True, there are at least a couple of these simple functions that could be shared. It might make sense to move them to container.go but I think I'd like to keep them separate for now.

@ehazlett

This comment has been minimized.

Copy link
Contributor

commented May 10, 2017

@johnstep needs a rebase

@johnstep johnstep force-pushed the johnstep:windows-secrets branch from cb73b2c to 4bc8e91 May 10, 2017

@johnstep johnstep added this to the 17.06.0 milestone May 10, 2017

@johnstep johnstep requested review from aaronlehmann and ehazlett May 10, 2017

// CreateSecretSymlinks creates symlinks to the secret volume.
func (container *Container) CreateSecretSymlinks() error {
for _, r := range container.SecretReferences {
if r.File == nil {

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 10, 2017

Contributor

Wondering under what circumstances this condition would be hit?

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann May 10, 2017

Contributor

IIRC File is in a oneof in the swarmkit gRPC API. If we add other possible types of secret targets, to allow exposing them in other ways than as files, File will be nil.


return mounts
}

// UnmountSecrets unmounts the fs for secrets

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 10, 2017

Contributor

The comment here should probably be updated.

This comment has been minimized.

Copy link
@johnstep

johnstep May 11, 2017

Author Member

Updated.

@@ -25,11 +25,30 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) {
// In base spec
s.Hostname = c.FullHostname()

if err := daemon.setupSecretDir(c); err != nil {

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 10, 2017

Contributor

setupSecretDir is doing a mount/unmount, and then that's being done in the loop at line 38 immediately after. Mount/Unmount is a very expensive (relatively) operation compared to Linux (we had strong feedback from the perf team when I was doing an accidental mount/unmounts a while back and had to refactor), so I'd really like to see it limited to at most 1 mount/unmounts pair to support secrets.

This comment has been minimized.

Copy link
@johnstep

johnstep May 10, 2017

Author Member

Good catch; the mount in setupSecretDir is actually not required; I will remove it.

defer func() {
if setupErr != nil {
if err := os.RemoveAll(localMountPath); err != nil {
logrus.Errorf("error cleaning up secret mount: %s", err)

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 10, 2017

Contributor

It looks like the this would be logged if system.MkdirAllWIthACL failed.

This comment has been minimized.

Copy link
@johnstep

johnstep May 10, 2017

Author Member

I will skip the log message if the error is non-existence.

@johnstep johnstep force-pushed the johnstep:windows-secrets branch from 4bc8e91 to 007f8fb May 11, 2017

@johnstep johnstep requested review from cpuguy83 and thaJeztah May 11, 2017

@mlaventure
Copy link
Contributor

left a comment

LGTM

ping @jhowardmsft

return fmt.Errorf("secret store is not initialized")
}

// TODO (ehazlett): use type switch when more are supported

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks May 11, 2017

Contributor

Is it OK to have TODO comments in the code?

This comment has been minimized.

Copy link
@mlaventure

mlaventure May 11, 2017

Contributor

Consider this one as a hint ;-)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah May 11, 2017

Member

To clarify; yes, we use TODO's in the code; usually not for "unfinished" code, but more as reminders that (e.g.) something needs to be removed after release X, or after feature Y is implemented. The TODO (name) format also works in some editors/IDE's to show a todo list for one person specific 😄 https://confluence.jetbrains.com/display/PhpStorm/Working+with+todo+comments+and+the+todo+tool+window

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah May 11, 2017

Member

t.b.h., I don't think this TODO is really useful 😄 ...

This comment has been minimized.

Copy link
@johnstep

johnstep May 12, 2017

Author Member

Yes, perhaps not all that useful, and now in 4 separate places...

This comment has been minimized.

Copy link
@ehazlett

ehazlett May 12, 2017

Contributor

these should be fine to remove. there were some timeline and roadmap adjustments that delayed this a bit so I think it's fine to remove for now.

@johnstep johnstep force-pushed the johnstep:windows-secrets branch from 007f8fb to 4660660 May 12, 2017

@ehazlett
Copy link
Contributor

left a comment

code LGTM

// In s.Mounts
mounts, err := daemon.setupMounts(c)
if err != nil {
return nil, err
}

if !c.HasBeenStartedBefore && len(c.SecretReferences) > 0 {

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 16, 2017

Contributor

I know we've talked offline about this, but I would still really like a comment here to explain why not been started before

This comment has been minimized.

Copy link
@johnstep

johnstep May 16, 2017

Author Member

I am not sure this is the best place to explain when to mount containers on Windows, in general, but perhaps you can give me a rough idea of the comment you would like to see. Thanks for pointing me to HasBeenStartedBefore; this check fixes an issue where the code was trying to recreate the symlinks for a container that had already been started, which fails.

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 16, 2017

Contributor

Something like "Ensure that we do not attempt to mount the filesystem of a Hyper-V container that has previously been started to stop an attack vector."

// In s.Mounts
mounts, err := daemon.setupMounts(c)
if err != nil {
return nil, err
}

if !c.HasBeenStartedBefore && len(c.SecretReferences) > 0 {
if err := daemon.Mount(c); err != nil {

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 16, 2017

Contributor

Again, as per offline, if we can avoid this and not rely on reference counting, that would be far better and more explicit. Note that as also mentioned, this should be co-ordinated with the configs PR

This comment has been minimized.

Copy link
@johnstep

johnstep May 16, 2017

Author Member

Is there a guarantee that the container file system is mounted here in the non-Hyper-V case? It seems strange to me to start modifying the container file system without first ensuring it is mounted. Is that guarantee documented anywhere, or just implicit in the design?

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 16, 2017

Contributor

Yes, for Windows Server Containers, you are guaranteed that the container FS is mounted here. In start.go:

	if err := daemon.conditionalMountOnStart(container); err != nil {
		return err
	}

	if err := daemon.initializeNetworking(container); err != nil {
		return err
	}

	spec, err := daemon.createSpec(container)
	if err != nil {
		return err
	}

Where conditionalMountOnStart always mounts Windows Server Containers, but not Hyper-V containers:

	// We do not mount if a Hyper-V container
	if !daemon.runAsHyperVContainer(container.HostConfig) {
		return daemon.Mount(container)
	}
@jhowardmsft

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

It's much closer, but I'd still like a few changes. I'd also like to see some docs here explaining the nuances of what all this implies before giving a 'LGTM'

@johnstep

This comment has been minimized.

Copy link
Member Author

commented May 16, 2017

I will work on the documentation. This Windows differences will definitely be documented for 17.06.

The main differences between secrets on Linux and Windows are:

  1. Windows stores temporary secret files for containers directly in the data root instead of in a tmpfs file system, since Windows does not have a built-in RAM disk driver. The primary mitigation for this is to enable BitLocker on the volume containing the data root.

  2. Windows secret files with custom targets are not directly bind-mounted into the container, since Windows does not support non-directory file bind-mounts. Instead, secrets for a container are all mounted in C:\ProgramData\Docker\internal\secrets (within the container, and an implementation detail), and symlinks are created that point from the target, to the secret in the internal directory.

  3. On Windows, when creating a service, the options to specify UID, GID, and mode are not supported for secrets; secrets are currently only accessible by administrators and system in a container.

@jhowardmsft Are there any other important differences you can think of? Thanks!

@jhowardmsft

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

@johnstep ^^ differences sounds good.

John Stephens
Add Windows secrets support
Signed-off-by: John Stephens <johnstep@docker.com>

@johnstep johnstep force-pushed the johnstep:windows-secrets branch from 4660660 to bd4e8aa May 16, 2017

}
err := c.CreateSecretSymlinks()
if isHyperV {
daemon.Unmount(c)

This comment has been minimized.

Copy link
@jhowardmsft

jhowardmsft May 16, 2017

Contributor

Minor nit, talked offline. When the configs PR is ready after this, change to a defer in the previous check.

@jhowardmsft
Copy link
Contributor

left a comment

LGTM. Thanks for addressing the feedback. One minor nit which can be addressed in the configs PR, but let's get this merged.

@jhowardmsft jhowardmsft merged commit 55fd0d0 into moby:master May 16, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34188 has succeeded
Details
janky Jenkins build Docker-PRs 42788 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3172 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 14023 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2888 has succeeded
Details
@thaJeztah

This comment has been minimized.

Copy link
Member

commented May 16, 2017

Thanks for reviewing @jhowardmsft

And whoop, @johnstep, happy to see this merged!

@johnstep johnstep deleted the johnstep:windows-secrets branch May 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.