From bb6f61424d1ce7e59a89140032afe5dbe8b4f892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 15 Feb 2024 13:18:47 +0100 Subject: [PATCH] api/pre-1.44: Default `ReadOnlyNonRecursive` to true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don't change the behavior for older clients and keep the same behavior. Otherwise client can't opt-out (because `ReadOnlyNonRecursive` is unsupported before 1.44). Signed-off-by: Paweł Gronowski (cherry picked from commit 0f8270b30286cffde3e40ad3291e60bbbbf3f202) Signed-off-by: Paweł Gronowski --- .../router/container/container_routes.go | 31 +++++--- api/swagger.yaml | 6 +- api/types/backend/backend.go | 13 ++-- daemon/container.go | 4 +- daemon/create.go | 2 +- daemon/volumes.go | 24 ++++++- integration-cli/docker_cli_inspect_test.go | 19 +++-- integration/container/mounts_linux_test.go | 70 ++++++++++++++++++- 8 files changed, 138 insertions(+), 31 deletions(-) diff --git a/api/server/router/container/container_routes.go b/api/server/router/container/container_routes.go index 08a9f3c0ad6e1..254250d659eac 100644 --- a/api/server/router/container/container_routes.go +++ b/api/server/router/container/container_routes.go @@ -602,17 +602,27 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo hostConfig.Annotations = nil } + defaultReadOnlyNonRecursive := false if versions.LessThan(version, "1.44") { if config.Healthcheck != nil { // StartInterval was added in API 1.44 config.Healthcheck.StartInterval = 0 } + // Set ReadOnlyNonRecursive to true because it was added in API 1.44 + // Before that all read-only mounts were non-recursive. + // Keep that behavior for clients on older APIs. + defaultReadOnlyNonRecursive = true + for _, m := range hostConfig.Mounts { - if m.BindOptions != nil { - // Ignore ReadOnlyNonRecursive because it was added in API 1.44. - m.BindOptions.ReadOnlyNonRecursive = false - if m.BindOptions.ReadOnlyForceRecursive { + if m.Type == mount.TypeBind { + if m.BindOptions != nil && m.BindOptions.ReadOnlyForceRecursive { + // NOTE: that technically this is a breaking change for older + // API versions, and we should ignore the new field. + // However, this option may be incorrectly set by a client with + // the expectation that the failing to apply recursive read-only + // is enforced, so we decided to produce an error instead, + // instead of silently ignoring. return errdefs.InvalidParameter(errors.New("BindOptions.ReadOnlyForceRecursive needs API v1.44 or newer")) } } @@ -644,12 +654,13 @@ func (s *containerRouter) postContainersCreate(ctx context.Context, w http.Respo } ccr, err := s.backend.ContainerCreate(ctx, backend.ContainerCreateConfig{ - Name: name, - Config: config, - HostConfig: hostConfig, - NetworkingConfig: networkingConfig, - AdjustCPUShares: adjustCPUShares, - Platform: platform, + Name: name, + Config: config, + HostConfig: hostConfig, + NetworkingConfig: networkingConfig, + AdjustCPUShares: adjustCPUShares, + Platform: platform, + DefaultReadOnlyNonRecursive: defaultReadOnlyNonRecursive, }) if err != nil { return err diff --git a/api/swagger.yaml b/api/swagger.yaml index 5e448edad65ef..25d960c677348 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -391,7 +391,11 @@ definitions: ReadOnlyNonRecursive: description: | Make the mount non-recursively read-only, but still leave the mount recursive - (unless NonRecursive is set to true in conjunction). + (unless NonRecursive is set to `true` in conjunction). + + Addded in v1.44, before that version all read-only mounts were + non-recursive by default. To match the previous behaviour this + will default to `true` for clients on versions prior to v1.44. type: "boolean" default: false ReadOnlyForceRecursive: diff --git a/api/types/backend/backend.go b/api/types/backend/backend.go index 613da5517df19..ee913d247e3c8 100644 --- a/api/types/backend/backend.go +++ b/api/types/backend/backend.go @@ -13,12 +13,13 @@ import ( // ContainerCreateConfig is the parameter set to ContainerCreate() type ContainerCreateConfig struct { - Name string - Config *container.Config - HostConfig *container.HostConfig - NetworkingConfig *network.NetworkingConfig - Platform *ocispec.Platform - AdjustCPUShares bool + Name string + Config *container.Config + HostConfig *container.HostConfig + NetworkingConfig *network.NetworkingConfig + Platform *ocispec.Platform + AdjustCPUShares bool + DefaultReadOnlyNonRecursive bool } // ContainerRmConfig holds arguments for the container remove diff --git a/daemon/container.go b/daemon/container.go index ccf29e27cf72d..c86ecf8c98509 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -203,10 +203,10 @@ func (daemon *Daemon) setSecurityOptions(cfg *config.Config, container *containe return daemon.parseSecurityOpt(cfg, &container.SecurityOptions, hostConfig) } -func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *containertypes.HostConfig) error { +func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *containertypes.HostConfig, defaultReadOnlyNonRecursive bool) error { // Do not lock while creating volumes since this could be calling out to external plugins // Don't want to block other actions, like `docker ps` because we're waiting on an external plugin - if err := daemon.registerMountPoints(container, hostConfig); err != nil { + if err := daemon.registerMountPoints(container, hostConfig, defaultReadOnlyNonRecursive); err != nil { return err } diff --git a/daemon/create.go b/daemon/create.go index c524c03b8ed04..1c8024218883a 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -218,7 +218,7 @@ func (daemon *Daemon) create(ctx context.Context, daemonCfg *config.Config, opts return nil, err } - if err := daemon.setHostConfig(ctr, opts.params.HostConfig); err != nil { + if err := daemon.setHostConfig(ctr, opts.params.HostConfig, opts.params.DefaultReadOnlyNonRecursive); err != nil { return nil, err } diff --git a/daemon/volumes.go b/daemon/volumes.go index c1e62cf73afc2..017604889f512 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -54,7 +54,7 @@ func (m mounts) parts(i int) int { // 2. Select the volumes mounted from another containers. Overrides previously configured mount point destination. // 3. Select the bind mounts set by the client. Overrides previously configured mount point destinations. // 4. Cleanup old volumes that are about to be reassigned. -func (daemon *Daemon) registerMountPoints(container *container.Container, hostConfig *containertypes.HostConfig) (retErr error) { +func (daemon *Daemon) registerMountPoints(container *container.Container, hostConfig *containertypes.HostConfig, defaultReadOnlyNonRecursive bool) (retErr error) { binds := map[string]bool{} mountPoints := map[string]*volumemounts.MountPoint{} parser := volumemounts.NewParser() @@ -158,6 +158,15 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo } } + if bind.Type == mount.TypeBind && !bind.RW { + if defaultReadOnlyNonRecursive { + if bind.Spec.BindOptions == nil { + bind.Spec.BindOptions = &mounttypes.BindOptions{} + } + bind.Spec.BindOptions.ReadOnlyNonRecursive = true + } + } + binds[bind.Destination] = true dereferenceIfExists(bind.Destination) mountPoints[bind.Destination] = bind @@ -212,8 +221,17 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo } } - if mp.Type == mounttypes.TypeBind && (cfg.BindOptions == nil || !cfg.BindOptions.CreateMountpoint) { - mp.SkipMountpointCreation = true + if mp.Type == mounttypes.TypeBind { + if cfg.BindOptions == nil || !cfg.BindOptions.CreateMountpoint { + mp.SkipMountpointCreation = true + } + + if !mp.RW && defaultReadOnlyNonRecursive { + if mp.Spec.BindOptions == nil { + mp.Spec.BindOptions = &mounttypes.BindOptions{} + } + mp.Spec.BindOptions.ReadOnlyNonRecursive = true + } } binds[mp.Destination] = true diff --git a/integration-cli/docker_cli_inspect_test.go b/integration-cli/docker_cli_inspect_test.go index 6eddbad66a250..2b495f3d29e96 100644 --- a/integration-cli/docker_cli_inspect_test.go +++ b/integration-cli/docker_cli_inspect_test.go @@ -175,15 +175,22 @@ func (s *DockerCLIInspectSuite) TestInspectContainerFilterInt(c *testing.T) { } func (s *DockerCLIInspectSuite) TestInspectBindMountPoint(c *testing.T) { - modifier := ",z" prefix, slash := getPrefixAndSlashFromDaemonPlatform() + mopt := prefix + slash + "data:" + prefix + slash + "data" + + mode := "" if testEnv.DaemonInfo.OSType == "windows" { - modifier = "" // Linux creates the host directory if it doesn't exist. Windows does not. os.Mkdir(`c:\data`, os.ModeDir) + } else { + mode = "z" // Relabel + } + + if mode != "" { + mopt += ":" + mode } - cli.DockerCmd(c, "run", "-d", "--name", "test", "-v", prefix+slash+"data:"+prefix+slash+"data:ro"+modifier, "busybox", "cat") + cli.DockerCmd(c, "run", "-d", "--name", "test", "-v", mopt, "busybox", "cat") vol := inspectFieldJSON(c, "test", "Mounts") @@ -200,10 +207,8 @@ func (s *DockerCLIInspectSuite) TestInspectBindMountPoint(c *testing.T) { assert.Equal(c, m.Driver, "") assert.Equal(c, m.Source, prefix+slash+"data") assert.Equal(c, m.Destination, prefix+slash+"data") - if testEnv.DaemonInfo.OSType != "windows" { // Windows does not set mode - assert.Equal(c, m.Mode, "ro"+modifier) - } - assert.Equal(c, m.RW, false) + assert.Equal(c, m.Mode, mode) + assert.Equal(c, m.RW, true) } func (s *DockerCLIInspectSuite) TestInspectNamedMountPoint(c *testing.T) { diff --git a/integration/container/mounts_linux_test.go b/integration/container/mounts_linux_test.go index 47f5f32d8f40c..aedde46931046 100644 --- a/integration/container/mounts_linux_test.go +++ b/integration/container/mounts_linux_test.go @@ -426,6 +426,70 @@ func TestContainerCopyLeaksMounts(t *testing.T) { assert.Equal(t, mountsBefore, mountsAfter) } +func TestContainerBindMountReadOnlyDefault(t *testing.T) { + skip.If(t, testEnv.IsRemoteDaemon) + skip.If(t, !isRROSupported(), "requires recursive read-only mounts") + + ctx := setupTest(t) + + // The test will run a container with a simple readonly /dev bind mount (-v /dev:/dev:ro) + // It will then check /proc/self/mountinfo for the mount type of /dev/shm (submount of /dev) + // If /dev/shm is rw, that will mean that the read-only mounts are NOT recursive by default. + const nonRecursive = " /dev/shm rw," + // If /dev/shm is ro, that will mean that the read-only mounts ARE recursive by default. + const recursive = " /dev/shm ro," + + for _, tc := range []struct { + version string + expectedOut string + name string + }{ + {version: "", expectedOut: recursive, name: "latest should be the same as 1.44"}, + {version: "1.44", expectedOut: recursive, name: "submount should be recursive by default on 1.44"}, + + {version: "1.43", expectedOut: nonRecursive, name: "older should be non-recursive by default"}, + } { + t.Run(tc.name, func(t *testing.T) { + apiClient := testEnv.APIClient() + + if tc.version != "" { + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), tc.version), "requires API v"+tc.version) + c, err := client.NewClientWithOpts(client.FromEnv, client.WithVersion(tc.version)) + assert.NilError(t, err, "failed to create client with version v%s", tc.version) + apiClient = c + } + + for _, tc2 := range []struct { + subname string + mountOpt func(*container.TestContainerConfig) + }{ + {"mount", container.WithMount(mounttypes.Mount{ + Type: mounttypes.TypeBind, + Source: "/dev", + Target: "/dev", + ReadOnly: true, + })}, + {"bind mount", container.WithBindRaw("/dev:/dev:ro")}, + } { + t.Run(tc2.subname, func(t *testing.T) { + cid := container.Run(ctx, t, apiClient, tc2.mountOpt, + container.WithCmd("sh", "-c", "grep /dev/shm /proc/self/mountinfo"), + ) + out, err := container.Output(ctx, apiClient, cid) + assert.NilError(t, err) + + assert.Check(t, is.Equal(out.Stderr, "")) + // Output should be either: + // 545 526 0:160 / /dev/shm ro,nosuid,nodev,noexec,relatime shared:90 - tmpfs shm rw,size=65536k + // or + // 545 526 0:160 / /dev/shm rw,nosuid,nodev,noexec,relatime shared:90 - tmpfs shm rw,size=65536k + assert.Check(t, is.Contains(out.Stdout, tc.expectedOut)) + }) + } + }) + } +} + func TestContainerBindMountRecursivelyReadOnly(t *testing.T) { skip.If(t, testEnv.IsRemoteDaemon) skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.44"), "requires API v1.44") @@ -450,7 +514,7 @@ func TestContainerBindMountRecursivelyReadOnly(t *testing.T) { } }() - rroSupported := kernel.CheckKernelVersion(5, 12, 0) + rroSupported := isRROSupported() nonRecursiveVerifier := []string{`/bin/sh`, `-xc`, `touch /foo/mnt/file; [ $? = 0 ]`} forceRecursiveVerifier := []string{`/bin/sh`, `-xc`, `touch /foo/mnt/file; [ $? != 0 ]`} @@ -504,3 +568,7 @@ func TestContainerBindMountRecursivelyReadOnly(t *testing.T) { poll.WaitOn(t, container.IsSuccessful(ctx, apiClient, c), poll.WithDelay(100*time.Millisecond)) } } + +func isRROSupported() bool { + return kernel.CheckKernelVersion(5, 12, 0) +}