1. docker
  2. Commit 6d1eceb5095a2fc7d3ada8a21e68d2b0242adbea
Browse code

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 <github@gone.nl>

Sebastiaan van Stijn authored on 2021/03/14 05:41:32
Showing 1 changed files
daemon/exec_linux_test.go
History View file @ 6d1eceb
... ... 
@@ -8,46 +8,83 @@ import (
8 8 
 	"github.com/containerd/containerd/pkg/apparmor"
9 9 
 	containertypes "github.com/docker/docker/api/types/container"
10 10 
 	"github.com/docker/docker/container"
11 
+	"github.com/docker/docker/daemon/config"
11 12 
 	"github.com/docker/docker/daemon/exec"
12 13 
 	specs "github.com/opencontainers/runtime-spec/specs-go"
13 14 
 	"gotest.tools/v3/assert"
14 15 
 )
15 16
16 
-func TestExecSetPlatformOpt(t *testing.T) {
17 
-	if !apparmor.HostSupports() {
18 
-		t.Skip("requires AppArmor to be enabled")
17 
+func TestExecSetPlatformOptAppArmor(t *testing.T) {
18 
+	appArmorEnabled := apparmor.HostSupports()
19 
+
20 
+	tests := []struct {
21 
+		doc             string
22 
+		privileged      bool
23 
+		appArmorProfile string
24 
+		expectedProfile string
25 
+	}{
26 
+		{
27 
+			doc:             "default options",
28 
+			expectedProfile: defaultAppArmorProfile,
29 
+		},
30 
+		{
31 
+			doc:             "custom profile",
32 
+			appArmorProfile: "my-custom-profile",
33 
+			expectedProfile: "my-custom-profile",
34 
+		},
35 
+		{
36 
+			doc:             "privileged container",
37 
+			privileged:      true,
38 
+			expectedProfile: unconfinedAppArmorProfile,
39 
+		},
40 
+		{
41 
+			doc:             "privileged container, custom profile",
42 
+			privileged:      true,
43 
+			appArmorProfile: "my-custom-profile",
44 
+			expectedProfile: "my-custom-profile",
45 
+			// FIXME: execSetPlatformOpts prefers custom profiles over "privileged",
46 
+			//        which looks like a bug (--privileged on the container should
47 
+			//        disable apparmor, seccomp, and selinux); see the code at:
48 
+			//        https://github.com/moby/moby/blob/46cdcd206c56172b95ba5c77b827a722dab426c5/daemon/exec_linux.go#L32-L40
49 
+			// expectedProfile: unconfinedAppArmorProfile,
50 
+		},
19 51 
 	}
20 
-	d := &Daemon{}
21 
-	c := &container.Container{AppArmorProfile: "my-custom-profile"}
22 
-	ec := &exec.Config{}
23 
-	p := &specs.Process{}
24 
-
25 
-	err := d.execSetPlatformOpt(c, ec, p)
26 
-	assert.NilError(t, err)
27 
-	assert.Equal(t, "my-custom-profile", p.ApparmorProfile)
28 
-}
29 52
30 
-// TestExecSetPlatformOptPrivileged verifies that `docker exec --privileged`
31 
-// does not disable AppArmor profiles. Exec currently inherits the `Privileged`
32 
-// configuration of the container. See https://github.com/moby/moby/pull/31773#discussion_r105586900
33 
-//
34 
-// This behavior may change in future, but test for the behavior to prevent it
35 
-// from being changed accidentally.
36 
-func TestExecSetPlatformOptPrivileged(t *testing.T) {
37 
-	if !apparmor.HostSupports() {
38 
-		t.Skip("requires AppArmor to be enabled")
53 
+	d := &Daemon{configStore: &config.Config{}}
54 
+
55 
+	// Currently, `docker exec --privileged` inherits the Privileged configuration
56 
+	// of the container, and does not disable AppArmor.
57 
+	// See https://github.com/moby/moby/pull/31773#discussion_r105586900
58 
+	//
59 
+	// This behavior may change in future, but to verify the current behavior,
60 
+	// we run the test both with "exec" and "exec --privileged", which should
61 
+	// both give the same result.
62 
+	for _, execPrivileged := range []bool{false, true} {
63 
+		for _, tc := range tests {
64 
+			tc := tc
65 
+			doc := tc.doc
66 
+			if !appArmorEnabled {
67 
+				// no profile should be set if the host does not support AppArmor
68 
+				doc += " (apparmor disabled)"
69 
+				tc.expectedProfile = ""
70 
+			}
71 
+			if execPrivileged {
72 
+				doc += " (exec privileged)"
73 
+			}
74 
+			t.Run(doc, func(t *testing.T) {
75 
+				c := &container.Container{
76 
+					AppArmorProfile: tc.appArmorProfile,
77 
+					HostConfig: &containertypes.HostConfig{
78 
+						Privileged: tc.privileged,
79 
+					},
80 
+				}
81 
+				ec := &exec.Config{Privileged: execPrivileged}
82 
+				p := &specs.Process{}
83 
+
84 
+				err := d.execSetPlatformOpt(c, ec, p)
85 
+				assert.NilError(t, err)
86 
+				assert.Equal(t, p.ApparmorProfile, tc.expectedProfile)
87 
+			})
88 
+		}
39 89 
 	}
40 
-	d := &Daemon{}
41 
-	c := &container.Container{AppArmorProfile: "my-custom-profile"}
42 
-	ec := &exec.Config{Privileged: true}
43 
-	p := &specs.Process{}
44 
-
45 
-	err := d.execSetPlatformOpt(c, ec, p)
46 
-	assert.NilError(t, err)
47 
-	assert.Equal(t, "my-custom-profile", p.ApparmorProfile)
48 
-
49 
-	c.HostConfig = &containertypes.HostConfig{Privileged: true}
50 
-	err = d.execSetPlatformOpt(c, ec, p)
51 
-	assert.NilError(t, err)
52 
-	assert.Equal(t, unconfinedAppArmorProfile, p.ApparmorProfile)
53 90 
 }