Add flag to overload default privileged host device behaviour

This commit adds a flag to the runtime config that allows overloading of the default
privileged behaviour. When the flag is enabled on a runtime, host devices won't
be appended to the runtime spec if the container is run as privileged.

By default the flag is false to maintain the current behaviour of privileged.

Fixes #1213

Signed-off-by: Alex Price <aprice@atlassian.com>

docs/config.md

@@ -117,6 +117,12 @@ version = 2
       # * OCI: https://github.com/opencontainers/image-spec/blob/master/annotations.md
       pod_annotations = []
 
+      # privileged_without_host_devices allows overloading the default behaviour of passing host
+      # devices through to privileged containers. This is useful when using a runtime where it does
+      # not make sense to pass host devices to the container when privileged. Defaults to false -
+      # i.e pass host devices through to privileged containers.
+      privileged_without_host_devices = false
+
       # 'plugins."io.containerd.grpc.v1.cri".containerd.runtimes.runc.options' is options specific to
       # "io.containerd.runc.v1". Its corresponding options type is:
       #   https://github.com/containerd/containerd/blob/v1.2.0-rc.1/runtime/v2/runc/options/oci.pb.go#L39.

pkg/config/config.go

@@ -41,6 +41,9 @@ type Runtime struct {
 	// Options are config options for the runtime. If options is loaded
 	// from toml config, it will be toml.Primitive.
 	Options *toml.Primitive `toml:"options" json:"options"`
+	// PrivilegedWithoutHostDevices overloads the default behaviour for adding host devices to the
+	// runtime spec when the container is privileged. Defaults to false.
+	PrivilegedWithoutHostDevices bool `toml:"privileged_without_host_devices" json:"privileged_without_host_devices"`
 }
 
 // ContainerdConfig contains toml config related to containerd

pkg/server/container_create.go

@@ -28,6 +28,13 @@ import (
 	"github.com/containerd/containerd/contrib/seccomp"
 	"github.com/containerd/containerd/log"
 	"github.com/containerd/containerd/oci"
+	"github.com/containerd/cri/pkg/annotations"
+	"github.com/containerd/cri/pkg/config"
+	customopts "github.com/containerd/cri/pkg/containerd/opts"
+	ctrdutil "github.com/containerd/cri/pkg/containerd/util"
+	cio "github.com/containerd/cri/pkg/server/io"
+	containerstore "github.com/containerd/cri/pkg/store/container"
+	"github.com/containerd/cri/pkg/util"
 	"github.com/containerd/typeurl"
 	"github.com/davecgh/go-spew/spew"
 	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
@@ -35,13 +42,6 @@ import (
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
-
-	"github.com/containerd/cri/pkg/annotations"
-	customopts "github.com/containerd/cri/pkg/containerd/opts"
-	ctrdutil "github.com/containerd/cri/pkg/containerd/util"
-	cio "github.com/containerd/cri/pkg/server/io"
-	containerstore "github.com/containerd/cri/pkg/store/container"
-	"github.com/containerd/cri/pkg/util"
 )
 
 const (
@@ -168,7 +168,7 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 	log.G(ctx).Debugf("Use OCI runtime %+v for sandbox %q and container %q", ociRuntime, sandboxID, id)
 
 	spec, err := c.generateContainerSpec(id, sandboxID, sandboxPid, config, sandboxConfig,
-		&image.ImageSpec.Config, append(mounts, volumeMounts...), ociRuntime.PodAnnotations)
+		&image.ImageSpec.Config, append(mounts, volumeMounts...), ociRuntime)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to generate container %q spec", id)
 	}
@@ -323,7 +323,8 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 }
 
 func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxPid uint32, config *runtime.ContainerConfig,
-	sandboxConfig *runtime.PodSandboxConfig, imageConfig *imagespec.ImageConfig, extraMounts []*runtime.Mount, runtimePodAnnotations []string) (*runtimespec.Spec, error) {
+	sandboxConfig *runtime.PodSandboxConfig, imageConfig *imagespec.ImageConfig, extraMounts []*runtime.Mount,
+	ociRuntime config.Runtime) (*runtimespec.Spec, error) {
 
 	specOpts := []oci.SpecOpts{
 		customopts.WithoutRunMount,
@@ -385,7 +386,10 @@ func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxP
 		if !sandboxConfig.GetLinux().GetSecurityContext().GetPrivileged() {
 			return nil, errors.New("no privileged container allowed in sandbox")
 		}
-		specOpts = append(specOpts, oci.WithPrivileged, customopts.WithPrivilegedDevices)
+		specOpts = append(specOpts, oci.WithPrivileged)
+		if !ociRuntime.PrivilegedWithoutHostDevices {
+			specOpts = append(specOpts, customopts.WithPrivilegedDevices)
+		}
 	} else { // not privileged
 		specOpts = append(specOpts, customopts.WithDevices(c.os, config), customopts.WithCapabilities(securityContext))
 	}
@@ -421,7 +425,7 @@ func (c *criService) generateContainerSpec(id string, sandboxID string, sandboxP
 	supplementalGroups := securityContext.GetSupplementalGroups()
 
 	for pKey, pValue := range getPassthroughAnnotations(sandboxConfig.Annotations,
-		runtimePodAnnotations) {
+		ociRuntime.PodAnnotations) {
 		specOpts = append(specOpts, customopts.WithAnnotation(pKey, pValue))
 	}
 

pkg/server/container_create_test.go

@@ -29,18 +29,19 @@ import (
 	"github.com/containerd/containerd/contrib/seccomp"
 	"github.com/containerd/containerd/mount"
 	"github.com/containerd/containerd/oci"
+	"github.com/containerd/cri/pkg/annotations"
+	"github.com/containerd/cri/pkg/config"
+	"github.com/containerd/cri/pkg/containerd/opts"
 	ctrdutil "github.com/containerd/cri/pkg/containerd/util"
+	ostesting "github.com/containerd/cri/pkg/os/testing"
+	"github.com/containerd/cri/pkg/util"
 	imagespec "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/opencontainers/runc/libcontainer/devices"
 	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
-
-	"github.com/containerd/cri/pkg/annotations"
-	"github.com/containerd/cri/pkg/containerd/opts"
-	ostesting "github.com/containerd/cri/pkg/os/testing"
-	"github.com/containerd/cri/pkg/util"
 )
 
 func checkMount(t *testing.T, mounts []runtimespec.Mount, src, dest, typ string,
@@ -195,10 +196,11 @@ func getCreateContainerTestData() (*runtime.ContainerConfig, *runtime.PodSandbox
 func TestGeneralContainerSpec(t *testing.T) {
 	testID := "test-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 	testSandboxID := "sandbox-id"
-	spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
+	spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 	require.NoError(t, err)
 	specCheck(t, testID, testSandboxID, testPid, spec)
 }
@@ -250,11 +252,12 @@ func TestContainerCapabilities(t *testing.T) {
 		},
 	} {
 		t.Logf("TestCase %q", desc)
-		config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+		containerConfig, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+		ociRuntime := config.Runtime{}
 		c := newTestCRIService()
 
-		config.Linux.SecurityContext.Capabilities = test.capability
+		containerConfig.Linux.SecurityContext.Capabilities = test.capability
-		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
+		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 		require.NoError(t, err)
 		specCheck(t, testID, testSandboxID, testPid, spec)
 		for _, include := range test.includes {
@@ -277,11 +280,12 @@ func TestContainerSpecTty(t *testing.T) {
 	testID := "test-id"
 	testSandboxID := "sandbox-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 	for _, tty := range []bool{true, false} {
-		config.Tty = tty
+		containerConfig.Tty = tty
-		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
+		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 		require.NoError(t, err)
 		specCheck(t, testID, testSandboxID, testPid, spec)
 		assert.Equal(t, tty, spec.Process.Terminal)
@@ -343,13 +347,16 @@ func TestPodAnnotationPassthroughContainerSpec(t *testing.T) {
 	} {
 		t.Run(desc, func(t *testing.T) {
 			c := newTestCRIService()
-			config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+			containerConfig, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
 			if test.configChange != nil {
 				test.configChange(sandboxConfig)
 			}
 
+			ociRuntime := config.Runtime{
+				PodAnnotations: test.podAnnotations,
+			}
 			spec, err := c.generateContainerSpec(testID, testSandboxID, testPid,
-				config, sandboxConfig, imageConfig, nil, test.podAnnotations)
+				containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 			assert.NoError(t, err)
 			assert.NotNil(t, spec)
 			specCheck(t, testID, testSandboxID, testPid, spec)
@@ -365,11 +372,12 @@ func TestContainerSpecReadonlyRootfs(t *testing.T) {
 	testID := "test-id"
 	testSandboxID := "sandbox-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 	for _, readonly := range []bool{true, false} {
-		config.Linux.SecurityContext.ReadonlyRootfs = readonly
+		containerConfig.Linux.SecurityContext.ReadonlyRootfs = readonly
-		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
+		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 		require.NoError(t, err)
 		specCheck(t, testID, testSandboxID, testPid, spec)
 		assert.Equal(t, readonly, spec.Root.Readonly)
@@ -380,7 +388,8 @@ func TestContainerSpecWithExtraMounts(t *testing.T) {
 	testID := "test-id"
 	testSandboxID := "sandbox-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 	mountInConfig := &runtime.Mount{
 		// Test cleanpath
@@ -388,7 +397,7 @@ func TestContainerSpecWithExtraMounts(t *testing.T) {
 		HostPath:      "test-host-path",
 		Readonly:      false,
 	}
-	config.Mounts = append(config.Mounts, mountInConfig)
+	containerConfig.Mounts = append(containerConfig.Mounts, mountInConfig)
 	extraMounts := []*runtime.Mount{
 		{
 			ContainerPath: "test-container-path",
@@ -406,7 +415,7 @@ func TestContainerSpecWithExtraMounts(t *testing.T) {
 			Readonly:      false,
 		},
 	}
-	spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, extraMounts, []string{})
+	spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, extraMounts, ociRuntime)
 	require.NoError(t, err)
 	specCheck(t, testID, testSandboxID, testPid, spec)
 	var mounts, sysMounts, devMounts []runtimespec.Mount
@@ -439,7 +448,8 @@ func TestContainerAndSandboxPrivileged(t *testing.T) {
 	testID := "test-id"
 	testSandboxID := "sandbox-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 	for desc, test := range map[string]struct {
 		containerPrivileged bool
@@ -468,11 +478,11 @@ func TestContainerAndSandboxPrivileged(t *testing.T) {
 		},
 	} {
 		t.Logf("TestCase %q", desc)
-		config.Linux.SecurityContext.Privileged = test.containerPrivileged
+		containerConfig.Linux.SecurityContext.Privileged = test.containerPrivileged
 		sandboxConfig.Linux.SecurityContext = &runtime.LinuxSandboxSecurityContext{
 			Privileged: test.sandboxPrivileged,
 		}
-		_, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
+		_, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 		if test.expectError {
 			assert.Error(t, err)
 		} else {
@@ -769,7 +779,8 @@ func TestPrivilegedBindMount(t *testing.T) {
 	testPid := uint32(1234)
 	c := newTestCRIService()
 	testSandboxID := "sandbox-id"
-	config, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 
 	for desc, test := range map[string]struct {
 		privileged         bool
@@ -788,10 +799,10 @@ func TestPrivilegedBindMount(t *testing.T) {
 	} {
 		t.Logf("TestCase %q", desc)
 
-		config.Linux.SecurityContext.Privileged = test.privileged
+		containerConfig.Linux.SecurityContext.Privileged = test.privileged
 		sandboxConfig.Linux.SecurityContext.Privileged = test.privileged
 
-		spec, err := c.generateContainerSpec(t.Name(), testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, nil)
+		spec, err := c.generateContainerSpec(t.Name(), testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 
 		assert.NoError(t, err)
 		if test.expectedSysFSRO {
@@ -917,7 +928,8 @@ func TestPidNamespace(t *testing.T) {
 	testID := "test-id"
 	testPid := uint32(1234)
 	testSandboxID := "sandbox-id"
-	config, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 	for desc, test := range map[string]struct {
 		pidNS    runtime.NamespaceMode
@@ -945,8 +957,8 @@ func TestPidNamespace(t *testing.T) {
 		},
 	} {
 		t.Logf("TestCase %q", desc)
-		config.Linux.SecurityContext.NamespaceOptions = &runtime.NamespaceOption{Pid: test.pidNS}
+		containerConfig.Linux.SecurityContext.NamespaceOptions = &runtime.NamespaceOption{Pid: test.pidNS}
-		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
+		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 		require.NoError(t, err)
 		assert.Contains(t, spec.Linux.Namespaces, test.expected)
 	}
@@ -956,10 +968,11 @@ func TestNoDefaultRunMount(t *testing.T) {
 	testID := "test-id"
 	testPid := uint32(1234)
 	testSandboxID := "sandbox-id"
-	config, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 
-	spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, nil)
+	spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 	assert.NoError(t, err)
 	for _, mount := range spec.Mounts {
 		assert.NotEqual(t, "/run", mount.Destination)
@@ -1093,7 +1106,8 @@ func TestMaskedAndReadonlyPaths(t *testing.T) {
 	testID := "test-id"
 	testSandboxID := "sandbox-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 
 	defaultSpec, err := oci.GenerateSpec(ctrdutil.NamespacedContext(), nil, &containers.Container{ID: testID})
@@ -1159,13 +1173,13 @@ func TestMaskedAndReadonlyPaths(t *testing.T) {
 	} {
 		t.Logf("TestCase %q", desc)
 		c.config.DisableProcMount = test.disableProcMount
-		config.Linux.SecurityContext.MaskedPaths = test.masked
+		containerConfig.Linux.SecurityContext.MaskedPaths = test.masked
-		config.Linux.SecurityContext.ReadonlyPaths = test.readonly
+		containerConfig.Linux.SecurityContext.ReadonlyPaths = test.readonly
-		config.Linux.SecurityContext.Privileged = test.privileged
+		containerConfig.Linux.SecurityContext.Privileged = test.privileged
 		sandboxConfig.Linux.SecurityContext = &runtime.LinuxSandboxSecurityContext{
 			Privileged: test.privileged,
 		}
-		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
+		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 		require.NoError(t, err)
 		if !test.privileged { // specCheck presumes an unprivileged container
 			specCheck(t, testID, testSandboxID, testPid, spec)
@@ -1179,7 +1193,8 @@ func TestHostname(t *testing.T) {
 	testID := "test-id"
 	testSandboxID := "sandbox-id"
 	testPid := uint32(1234)
-	config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 	c.os.(*ostesting.FakeOS).HostnameFn = func() (string, error) {
 		return "real-hostname", nil
@@ -1210,7 +1225,7 @@ func TestHostname(t *testing.T) {
 		sandboxConfig.Linux.SecurityContext = &runtime.LinuxSandboxSecurityContext{
 			NamespaceOptions: &runtime.NamespaceOption{Network: test.networkNs},
 		}
-		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
+		spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 		require.NoError(t, err)
 		specCheck(t, testID, testSandboxID, testPid, spec)
 		assert.Contains(t, spec.Process.Env, test.expectedEnv)
@@ -1218,10 +1233,11 @@ func TestHostname(t *testing.T) {
 }
 
 func TestDisableCgroup(t *testing.T) {
-	config, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+	ociRuntime := config.Runtime{}
 	c := newTestCRIService()
 	c.config.DisableCgroup = true
-	spec, err := c.generateContainerSpec("test-id", "sandbox-id", 1234, config, sandboxConfig, imageConfig, nil, []string{})
+	spec, err := c.generateContainerSpec("test-id", "sandbox-id", 1234, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 	require.NoError(t, err)
 
 	t.Log("resource limit should not be set")
@@ -1302,3 +1318,57 @@ func TestGenerateUserString(t *testing.T) {
 		})
 	}
 }
+
+func TestPrivilegedDevices(t *testing.T) {
+	testPid := uint32(1234)
+	c := newTestCRIService()
+	testSandboxID := "sandbox-id"
+	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
+
+	for desc, test := range map[string]struct {
+		privileged                   bool
+		privilegedWithoutHostDevices bool
+		expectHostDevices            bool
+	}{
+		"expect no host devices when privileged is false": {
+			privileged:                   false,
+			privilegedWithoutHostDevices: false,
+			expectHostDevices:            false,
+		},
+		"expect no host devices when privileged is false and privilegedWithoutHostDevices is true": {
+			privileged:                   false,
+			privilegedWithoutHostDevices: true,
+			expectHostDevices:            false,
+		},
+		"expect host devices when privileged is true": {
+			privileged:                   true,
+			privilegedWithoutHostDevices: false,
+			expectHostDevices:            true,
+		},
+		"expect no host devices when privileged is true and privilegedWithoutHostDevices is true": {
+			privileged:                   true,
+			privilegedWithoutHostDevices: true,
+			expectHostDevices:            false,
+		},
+	} {
+		t.Logf("TestCase %q", desc)
+
+		containerConfig.Linux.SecurityContext.Privileged = test.privileged
+		sandboxConfig.Linux.SecurityContext.Privileged = test.privileged
+
+		ociRuntime := config.Runtime{
+			PrivilegedWithoutHostDevices: test.privilegedWithoutHostDevices,
+		}
+		spec, err := c.generateContainerSpec(t.Name(), testSandboxID, testPid, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
+		assert.NoError(t, err)
+
+		hostDevices, err := devices.HostDevices()
+		assert.NoError(t, err)
+
+		if test.expectHostDevices {
+			assert.Len(t, spec.Linux.Devices, len(hostDevices))
+		} else {
+			assert.Empty(t, spec.Linux.Devices)
+		}
+	}
+}