From 6d1eceb5095a2fc7d3ada8a21e68d2b0242adbea Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 13 Mar 2021 21:41:32 +0100 Subject: [PATCH] Fix panic in TestExecSetPlatformOpt, TestExecSetPlatformOptPrivileged These tests would panic; - in WithRLimits(), because HostConfig was not set; https://github.com/moby/moby/blob/470ae8422fc6f1845288eb7572253b08f1e6edf8/daemon/oci_linux.go#L46-L47 - in daemon.mergeUlimits(), because daemon.configStore was not set; https://github.com/moby/moby/blob/470ae8422fc6f1845288eb7572253b08f1e6edf8/daemon/oci_linux.go#L1069 This panic was not discovered because the current version of runc/libcontainer that we vendor would not always return false for `apparmor.IsEnabled()` when running docker-in-docker or if `apparmor_parser` is not found. Starting with v1.0.0-rc93 of libcontainer, this is no longer the case (changed in https://github.com/opencontainers/runc/commit/bfb4ea1b1b2452bb3ddc8183b35dec4914bb5128) This patch; - changes the tests to initialize Daemon.configStore and Container.HostConfig - Combines TestExecSetPlatformOpt and TestExecSetPlatformOptPrivileged into a new test (TestExecSetPlatformOptAppArmor) - Runs the test both if AppArmor is enabled and if not (in which case it tests that the container's AppArmor profile is left empty). - Adds a FIXME comment for a possible bug in execSetPlatformOpts, which currently prefers custom profiles over "privileged". Signed-off-by: Sebastiaan van Stijn --- daemon/exec_linux_test.go | 103 ++++++++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 33 deletions(-) diff --git a/daemon/exec_linux_test.go b/daemon/exec_linux_test.go index cdebeb1cd7..fc011a4d35 100644 --- a/daemon/exec_linux_test.go +++ b/daemon/exec_linux_test.go @@ -8,46 +8,83 @@ import ( "github.com/containerd/containerd/pkg/apparmor" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/container" + "github.com/docker/docker/daemon/config" "github.com/docker/docker/daemon/exec" specs "github.com/opencontainers/runtime-spec/specs-go" "gotest.tools/v3/assert" ) -func TestExecSetPlatformOpt(t *testing.T) { - if !apparmor.HostSupports() { - t.Skip("requires AppArmor to be enabled") +func TestExecSetPlatformOptAppArmor(t *testing.T) { + appArmorEnabled := apparmor.HostSupports() + + tests := []struct { + doc string + privileged bool + appArmorProfile string + expectedProfile string + }{ + { + doc: "default options", + expectedProfile: defaultAppArmorProfile, + }, + { + doc: "custom profile", + appArmorProfile: "my-custom-profile", + expectedProfile: "my-custom-profile", + }, + { + doc: "privileged container", + privileged: true, + expectedProfile: unconfinedAppArmorProfile, + }, + { + doc: "privileged container, custom profile", + privileged: true, + appArmorProfile: "my-custom-profile", + expectedProfile: "my-custom-profile", + // FIXME: execSetPlatformOpts prefers custom profiles over "privileged", + // which looks like a bug (--privileged on the container should + // disable apparmor, seccomp, and selinux); see the code at: + // https://github.com/moby/moby/blob/46cdcd206c56172b95ba5c77b827a722dab426c5/daemon/exec_linux.go#L32-L40 + // expectedProfile: unconfinedAppArmorProfile, + }, } - d := &Daemon{} - c := &container.Container{AppArmorProfile: "my-custom-profile"} - ec := &exec.Config{} - p := &specs.Process{} - err := d.execSetPlatformOpt(c, ec, p) - assert.NilError(t, err) - assert.Equal(t, "my-custom-profile", p.ApparmorProfile) -} + d := &Daemon{configStore: &config.Config{}} -// TestExecSetPlatformOptPrivileged verifies that `docker exec --privileged` -// does not disable AppArmor profiles. Exec currently inherits the `Privileged` -// configuration of the container. See https://github.com/moby/moby/pull/31773#discussion_r105586900 -// -// This behavior may change in future, but test for the behavior to prevent it -// from being changed accidentally. -func TestExecSetPlatformOptPrivileged(t *testing.T) { - if !apparmor.HostSupports() { - t.Skip("requires AppArmor to be enabled") + // Currently, `docker exec --privileged` inherits the Privileged configuration + // of the container, and does not disable AppArmor. + // See https://github.com/moby/moby/pull/31773#discussion_r105586900 + // + // This behavior may change in future, but to verify the current behavior, + // we run the test both with "exec" and "exec --privileged", which should + // both give the same result. + for _, execPrivileged := range []bool{false, true} { + for _, tc := range tests { + tc := tc + doc := tc.doc + if !appArmorEnabled { + // no profile should be set if the host does not support AppArmor + doc += " (apparmor disabled)" + tc.expectedProfile = "" + } + if execPrivileged { + doc += " (exec privileged)" + } + t.Run(doc, func(t *testing.T) { + c := &container.Container{ + AppArmorProfile: tc.appArmorProfile, + HostConfig: &containertypes.HostConfig{ + Privileged: tc.privileged, + }, + } + ec := &exec.Config{Privileged: execPrivileged} + p := &specs.Process{} + + err := d.execSetPlatformOpt(c, ec, p) + assert.NilError(t, err) + assert.Equal(t, p.ApparmorProfile, tc.expectedProfile) + }) + } } - d := &Daemon{} - c := &container.Container{AppArmorProfile: "my-custom-profile"} - ec := &exec.Config{Privileged: true} - p := &specs.Process{} - - err := d.execSetPlatformOpt(c, ec, p) - assert.NilError(t, err) - assert.Equal(t, "my-custom-profile", p.ApparmorProfile) - - c.HostConfig = &containertypes.HostConfig{Privileged: true} - err = d.execSetPlatformOpt(c, ec, p) - assert.NilError(t, err) - assert.Equal(t, unconfinedAppArmorProfile, p.ApparmorProfile) }