From f68c84b9a0bbdc45deb0f5b51ac711d391fc9c87 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 17 Jan 2018 10:49:58 -0500 Subject: [PATCH] Merge configs/secrets in unix implementation On unix, merge secrets/configs handling. This is important because configs can contain secrets (via templating) and potentially a config could just simply have secret information "by accident" from the user. This just make sure that configs are as secure as secrets and de-dups a lot of code. Generally this makes everything simpler and configs more secure. Signed-off-by: Brian Goff Upstream-commit: c02171802b788fb2d4d48bebcee2a57c8eabeeaa Component: engine --- components/engine/container/container.go | 34 +--- components/engine/container/container_unix.go | 38 ++--- .../engine/container/container_windows.go | 25 +-- components/engine/daemon/configs.go | 7 +- .../daemon/container_operations_unix.go | 149 ++++++++---------- .../daemon/container_operations_windows.go | 11 +- components/engine/daemon/oci_linux.go | 21 +-- components/engine/daemon/oci_windows.go | 5 +- .../engine/integration/config/config_test.go | 19 ++- .../engine/integration/secret/secret_test.go | 19 ++- 10 files changed, 126 insertions(+), 202 deletions(-) diff --git a/components/engine/container/container.go b/components/engine/container/container.go index 32389e3804..46e592ab45 100644 --- a/components/engine/container/container.go +++ b/components/engine/container/container.go @@ -68,13 +68,6 @@ type ExitStatus struct { ExitedAt time.Time } -// ConfigReference wraps swarmtypes.ConfigReference to add a Sensitive flag. -type ConfigReference struct { - *swarmtypes.ConfigReference - // Sensitive is set if this config should not be written to disk. - Sensitive bool -} - // Container holds the structure defining a container object. type Container struct { StreamConfig *stream.Config @@ -106,7 +99,7 @@ type Container struct { ExecCommands *exec.Store `json:"-"` DependencyStore agentexec.DependencyGetter `json:"-"` SecretReferences []*swarmtypes.SecretReference - ConfigReferences []*ConfigReference + ConfigReferences []*swarmtypes.ConfigReference // logDriver for closing LogDriver logger.Logger `json:"-"` LogCopier *logger.Copier `json:"-"` @@ -1056,31 +1049,6 @@ func getSecretTargetPath(r *swarmtypes.SecretReference) string { return filepath.Join(containerSecretMountPath, r.File.Name) } -// ConfigsDirPath returns the path to the directory where configs are stored on -// disk. -func (container *Container) ConfigsDirPath() (string, error) { - return container.GetRootResourcePath("configs") -} - -// ConfigFilePath returns the path to the on-disk location of a config. -func (container *Container) ConfigFilePath(configRef swarmtypes.ConfigReference) (string, error) { - configs, err := container.ConfigsDirPath() - if err != nil { - return "", err - } - return filepath.Join(configs, configRef.ConfigID), nil -} - -// SensitiveConfigFilePath returns the path to the location of a config mounted -// as a secret. -func (container *Container) SensitiveConfigFilePath(configRef swarmtypes.ConfigReference) (string, error) { - secretMountPath, err := container.SecretMountPath() - if err != nil { - return "", err - } - return filepath.Join(secretMountPath, configRef.ConfigID+"c"), nil -} - // CreateDaemonEnvironment creates a new environment variable slice for this container. func (container *Container) CreateDaemonEnvironment(tty bool, linkedEnv []string) []string { // Setup environment diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index ec90921acc..e5cecf3166 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -5,11 +5,13 @@ package container // import "github.com/docker/docker/container" import ( "io/ioutil" "os" + "path/filepath" "github.com/containerd/continuity/fs" "github.com/docker/docker/api/types" containertypes "github.com/docker/docker/api/types/container" mounttypes "github.com/docker/docker/api/types/mount" + swarmtypes "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/stringid" "github.com/docker/docker/volume" @@ -234,10 +236,7 @@ func (container *Container) SecretMounts() ([]Mount, error) { }) } for _, r := range container.ConfigReferences { - if !r.Sensitive || r.File == nil { - continue - } - fPath, err := container.SensitiveConfigFilePath(*r.ConfigReference) + fPath, err := container.ConfigFilePath(*r) if err != nil { return nil, err } @@ -267,27 +266,6 @@ func (container *Container) UnmountSecrets() error { return mount.RecursiveUnmount(p) } -// ConfigMounts returns the mounts for configs. -func (container *Container) ConfigMounts() ([]Mount, error) { - var mounts []Mount - for _, configRef := range container.ConfigReferences { - if configRef.Sensitive || configRef.File == nil { - continue - } - src, err := container.ConfigFilePath(*configRef.ConfigReference) - if err != nil { - return nil, err - } - mounts = append(mounts, Mount{ - Source: src, - Destination: configRef.File.Name, - Writable: false, - }) - } - - return mounts, nil -} - type conflictingUpdateOptions string func (e conflictingUpdateOptions) Error() string { @@ -471,3 +449,13 @@ func (container *Container) GetMountPoints() []types.MountPoint { } return mountPoints } + +// ConfigFilePath returns the path to the on-disk location of a config. +// On unix, configs are always considered secret +func (container *Container) ConfigFilePath(configRef swarmtypes.ConfigReference) (string, error) { + mounts, err := container.SecretMountPath() + if err != nil { + return "", err + } + return filepath.Join(mounts, configRef.ConfigID), nil +} diff --git a/components/engine/container/container_windows.go b/components/engine/container/container_windows.go index 44b646a1ad..b5bdb5bc34 100644 --- a/components/engine/container/container_windows.go +++ b/components/engine/container/container_windows.go @@ -7,6 +7,7 @@ import ( "github.com/docker/docker/api/types" containertypes "github.com/docker/docker/api/types/container" + swarmtypes "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/pkg/system" ) @@ -102,23 +103,20 @@ func (container *Container) CreateConfigSymlinks() error { } // ConfigMounts returns the mount for configs. -// All configs are stored in a single mount on Windows. Target symlinks are -// created for each config, pointing to the files in this mount. -func (container *Container) ConfigMounts() ([]Mount, error) { +// TODO: Right now Windows doesn't really have a "secure" storage for secrets, +// however some configs may contain secrets. Once secure storage is worked out, +// configs and secret handling should be merged. +func (container *Container) ConfigMounts() []Mount { var mounts []Mount if len(container.ConfigReferences) > 0 { - src, err := container.ConfigsDirPath() - if err != nil { - return nil, err - } mounts = append(mounts, Mount{ - Source: src, + Source: container.ConfigsDirPath(), Destination: containerInternalConfigsDirPath, Writable: false, }) } - return mounts, nil + return mounts } // DetachAndUnmount unmounts all volumes. @@ -204,3 +202,12 @@ func (container *Container) GetMountPoints() []types.MountPoint { } return mountPoints } + +func (container *Container) ConfigsDirPath() string { + return filepath.Join(container.Root, "configs") +} + +// ConfigFilePath returns the path to the on-disk location of a config. +func (container *Container) ConfigFilePath(configRef swarmtypes.ConfigReference) string { + return filepath.Join(container.ConfigsDirPath(), configRef.ConfigID) +} diff --git a/components/engine/daemon/configs.go b/components/engine/daemon/configs.go index c19b7ff3fd..4fd0d2272c 100644 --- a/components/engine/daemon/configs.go +++ b/components/engine/daemon/configs.go @@ -2,7 +2,6 @@ package daemon // import "github.com/docker/docker/daemon" import ( swarmtypes "github.com/docker/docker/api/types/swarm" - "github.com/docker/docker/container" "github.com/sirupsen/logrus" ) @@ -17,10 +16,6 @@ func (daemon *Daemon) SetContainerConfigReferences(name string, refs []*swarmtyp if err != nil { return err } - - for _, ref := range refs { - c.ConfigReferences = append(c.ConfigReferences, &container.ConfigReference{ConfigReference: ref}) - } - + c.ConfigReferences = append(c.ConfigReferences, refs...) return nil } diff --git a/components/engine/daemon/container_operations_unix.go b/components/engine/daemon/container_operations_unix.go index 8c23b36e3c..4e92b6392e 100644 --- a/components/engine/daemon/container_operations_unix.go +++ b/components/engine/daemon/container_operations_unix.go @@ -161,20 +161,16 @@ func (daemon *Daemon) setupIpcDirs(c *container.Container) error { } func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) { - if len(c.SecretReferences) == 0 { + if len(c.SecretReferences) == 0 && len(c.ConfigReferences) == 0 { return nil } - localMountPath, err := c.SecretMountPath() - if err != nil { - return errors.Wrap(err, "error getting secrets mount path for container") - } - if err := daemon.createSecretsDir(localMountPath); err != nil { + if err := daemon.createSecretsDir(c); err != nil { return err } defer func() { if setupErr != nil { - daemon.cleanupSecretDir(localMountPath) + daemon.cleanupSecretDir(c) } }() @@ -231,88 +227,16 @@ func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) { } } - return daemon.remountSecretDir(c.MountLabel, localMountPath) -} - -// createSecretsDir is used to create a dir suitable for storing container secrets. -// In practice this is using a tmpfs mount and is used for both "configs" and "secrets" -func (daemon *Daemon) createSecretsDir(dir string) error { - // retrieve possible remapped range start for root UID, GID - rootIDs := daemon.idMappings.RootPair() - // create tmpfs - if err := idtools.MkdirAllAndChown(dir, 0700, rootIDs); err != nil { - return errors.Wrap(err, "error creating secret local mount path") - } - - tmpfsOwnership := fmt.Sprintf("uid=%d,gid=%d", rootIDs.UID, rootIDs.GID) - if err := mount.Mount("tmpfs", dir, "tmpfs", "nodev,nosuid,noexec,"+tmpfsOwnership); err != nil { - return errors.Wrap(err, "unable to setup secret mount") - } - - return nil -} - -func (daemon *Daemon) remountSecretDir(mountLabel, dir string) error { - if err := label.Relabel(dir, mountLabel, false); err != nil { - logrus.WithError(err).WithField("dir", dir).Warn("Error while attempting to set selinux label") - } - rootIDs := daemon.idMappings.RootPair() - tmpfsOwnership := fmt.Sprintf("uid=%d,gid=%d", rootIDs.UID, rootIDs.GID) - - // remount secrets ro - if err := mount.Mount("tmpfs", dir, "tmpfs", "remount,ro,"+tmpfsOwnership); err != nil { - return errors.Wrap(err, "unable to remount dir as readonly") - } - - return nil -} - -func (daemon *Daemon) cleanupSecretDir(dir string) { - if err := mount.RecursiveUnmount(dir); err != nil { - logrus.WithField("dir", dir).WithError(err).Warn("Error while attmepting to unmount dir, this may prevent removal of container.") - } - if err := os.RemoveAll(dir); err != nil && !os.IsNotExist(err) { - logrus.WithField("dir", dir).WithError(err).Error("Error removing dir.") - } -} - -func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) { - if len(c.ConfigReferences) == 0 { - return nil - } - - localPath, err := c.ConfigsDirPath() - if err != nil { - return err - } - logrus.Debugf("configs: setting up config dir: %s", localPath) - if err := daemon.createSecretsDir(localPath); err != nil { - return err - } - defer func() { - if setupErr != nil { - daemon.cleanupSecretDir(localPath) - } - }() - - if c.DependencyStore == nil { - return errors.New("config store is not initialized") - } - - // retrieve possible remapped range start for root UID, GID - rootIDs := daemon.idMappings.RootPair() - for _, ref := range c.ConfigReferences { // TODO (ehazlett): use type switch when more are supported if ref.File == nil { logrus.Error("config target type is not a file target") continue } - // configs are created in the ConfigsDirPath on the host, at a - // single level - fPath, err := c.ConfigFilePath(*ref.ConfigReference) + + fPath, err := c.ConfigFilePath(*ref) if err != nil { - return err + return errors.Wrap(err, "error getting config file path for container") } if err := idtools.MkdirAllAndChown(filepath.Dir(fPath), 0700, rootIDs); err != nil { return errors.Wrap(err, "error creating config mount path") @@ -342,14 +266,67 @@ func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) { if err := os.Chown(fPath, rootIDs.UID+uid, rootIDs.GID+gid); err != nil { return errors.Wrap(err, "error setting ownership for config") } - if err := os.Chmod(fPath, configRef.File.Mode); err != nil { + if err := os.Chmod(fPath, ref.File.Mode); err != nil { return errors.Wrap(err, "error setting file mode for config") } - - label.Relabel(fPath, c.MountLabel, false) } - return daemon.remountSecretDir(c.MountLabel, localPath) + return daemon.remountSecretDir(c) +} + +// createSecretsDir is used to create a dir suitable for storing container secrets. +// In practice this is using a tmpfs mount and is used for both "configs" and "secrets" +func (daemon *Daemon) createSecretsDir(c *container.Container) error { + // retrieve possible remapped range start for root UID, GID + rootIDs := daemon.idMappings.RootPair() + dir, err := c.SecretMountPath() + if err != nil { + return errors.Wrap(err, "error getting container secrets dir") + } + + // create tmpfs + if err := idtools.MkdirAllAndChown(dir, 0700, rootIDs); err != nil { + return errors.Wrap(err, "error creating secret local mount path") + } + + tmpfsOwnership := fmt.Sprintf("uid=%d,gid=%d", rootIDs.UID, rootIDs.GID) + if err := mount.Mount("tmpfs", dir, "tmpfs", "nodev,nosuid,noexec,"+tmpfsOwnership); err != nil { + return errors.Wrap(err, "unable to setup secret mount") + } + + return nil +} + +func (daemon *Daemon) remountSecretDir(c *container.Container) error { + dir, err := c.SecretMountPath() + if err != nil { + return errors.Wrap(err, "error getting container secrets path") + } + if err := label.Relabel(dir, c.MountLabel, false); err != nil { + logrus.WithError(err).WithField("dir", dir).Warn("Error while attempting to set selinux label") + } + rootIDs := daemon.idMappings.RootPair() + tmpfsOwnership := fmt.Sprintf("uid=%d,gid=%d", rootIDs.UID, rootIDs.GID) + + // remount secrets ro + if err := mount.Mount("tmpfs", dir, "tmpfs", "remount,ro,"+tmpfsOwnership); err != nil { + return errors.Wrap(err, "unable to remount dir as readonly") + } + + return nil +} + +func (daemon *Daemon) cleanupSecretDir(c *container.Container) { + dir, err := c.SecretMountPath() + if err != nil { + logrus.WithError(err).WithField("container", c.ID).Warn("error getting secrets mount path for container") + } + if err := mount.RecursiveUnmount(dir); err != nil { + logrus.WithField("dir", dir).WithError(err).Warn("Error while attmepting to unmount dir, this may prevent removal of container.") + } + if err := os.RemoveAll(dir); err != nil && !os.IsNotExist(err) { + logrus.WithField("dir", dir).WithError(err).Error("Error removing dir.") + } } func killProcessDirectly(cntr *container.Container) error { diff --git a/components/engine/daemon/container_operations_windows.go b/components/engine/daemon/container_operations_windows.go index fe9795b8ba..0559b8ac3e 100644 --- a/components/engine/daemon/container_operations_windows.go +++ b/components/engine/daemon/container_operations_windows.go @@ -21,10 +21,7 @@ func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) { return nil } - localPath, err := c.ConfigsDirPath() - if err != nil { - return err - } + localPath := c.ConfigsDirPath() logrus.Debugf("configs: setting up config dir: %s", localPath) // create local config root @@ -51,11 +48,7 @@ func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) { continue } - fPath, err := c.ConfigFilePath(*configRef.ConfigReference) - if err != nil { - return err - } - + fPath := c.ConfigFilePath(*configRef) log := logrus.WithFields(logrus.Fields{"name": configRef.File.Name, "path": fPath}) log.Debug("injecting config") diff --git a/components/engine/daemon/oci_linux.go b/components/engine/daemon/oci_linux.go index 52a944f2a2..68311bd605 100644 --- a/components/engine/daemon/oci_linux.go +++ b/components/engine/daemon/oci_linux.go @@ -842,18 +842,9 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e return nil, err } - secretMountPath, err := c.SecretMountPath() - if err != nil { - return nil, err - } - configsMountPath, err := c.ConfigsDirPath() - if err != nil { - return nil, err - } defer func() { if err != nil { - daemon.cleanupSecretDir(secretMountPath) - daemon.cleanupSecretDir(configsMountPath) + daemon.cleanupSecretDir(c) } }() @@ -861,10 +852,6 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e return nil, err } - if err := daemon.setupConfigDir(c); err != nil { - return nil, err - } - ms, err := daemon.setupMounts(c) if err != nil { return nil, err @@ -886,12 +873,6 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e } ms = append(ms, secretMounts...) - configMounts, err := c.ConfigMounts() - if err != nil { - return nil, err - } - ms = append(ms, configMounts...) - sort.Sort(mounts(ms)) if err := setMounts(daemon, &s, c, ms); err != nil { return nil, fmt.Errorf("linux mounts: %v", err) diff --git a/components/engine/daemon/oci_windows.go b/components/engine/daemon/oci_windows.go index 47b1301eee..64c651c4af 100644 --- a/components/engine/daemon/oci_windows.go +++ b/components/engine/daemon/oci_windows.go @@ -102,10 +102,7 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) { mounts = append(mounts, secretMounts...) } - configMounts, err := c.ConfigMounts() - if err != nil { - return nil, err - } + configMounts := c.ConfigMounts() if configMounts != nil { mounts = append(mounts, configMounts...) } diff --git a/components/engine/integration/config/config_test.go b/components/engine/integration/config/config_test.go index 6a96986e4a..c152be59bf 100644 --- a/components/engine/integration/config/config_test.go +++ b/components/engine/integration/config/config_test.go @@ -292,15 +292,24 @@ func TestTemplatedConfig(t *testing.T) { AttachStderr: true, }) - buf := bytes.NewBuffer(nil) - _, err = stdcopy.StdCopy(buf, buf, attach.Reader) - require.NoError(t, err) - expect := "SERVICE_NAME=svc\n" + "this is a secret\n" + "this is a config\n" + assertAttachedStream(t, attach, expect) - assert.Equal(t, expect, buf.String()) + attach = swarm.ExecTask(t, d, task, types.ExecConfig{ + Cmd: []string{"mount"}, + AttachStdout: true, + AttachStderr: true, + }) + assertAttachedStream(t, attach, "tmpfs on /templated_config type tmpfs") +} + +func assertAttachedStream(t *testing.T, attach types.HijackedResponse, expect string) { + buf := bytes.NewBuffer(nil) + _, err := stdcopy.StdCopy(buf, buf, attach.Reader) + require.NoError(t, err) + assert.Contains(t, buf.String(), expect) } func waitAndAssert(t *testing.T, timeout time.Duration, f func(*testing.T) bool) { diff --git a/components/engine/integration/secret/secret_test.go b/components/engine/integration/secret/secret_test.go index e26d8a698d..3b5e66a5bf 100644 --- a/components/engine/integration/secret/secret_test.go +++ b/components/engine/integration/secret/secret_test.go @@ -336,15 +336,24 @@ func TestTemplatedSecret(t *testing.T) { AttachStderr: true, }) - buf := bytes.NewBuffer(nil) - _, err = stdcopy.StdCopy(buf, buf, attach.Reader) - require.NoError(t, err) - expect := "SERVICE_NAME=svc\n" + "this is a secret\n" + "this is a config\n" + assertAttachedStream(t, attach, expect) - assert.Equal(t, expect, buf.String()) + attach = swarm.ExecTask(t, d, task, types.ExecConfig{ + Cmd: []string{"mount"}, + AttachStdout: true, + AttachStderr: true, + }) + assertAttachedStream(t, attach, "tmpfs on /run/secrets/templated_secret type tmpfs") +} + +func assertAttachedStream(t *testing.T, attach types.HijackedResponse, expect string) { + buf := bytes.NewBuffer(nil) + _, err := stdcopy.StdCopy(buf, buf, attach.Reader) + require.NoError(t, err) + assert.Contains(t, buf.String(), expect) } func waitAndAssert(t *testing.T, timeout time.Duration, f func(*testing.T) bool) {