From 3353ab76d9223c7a9cfc876ec412ada906ca3cde Mon Sep 17 00:00:00 2001 From: Alex Price Date: Wed, 7 Aug 2019 16:22:26 +1000 Subject: [PATCH] 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 --- docs/config.md | 6 ++ pkg/config/config.go | 3 + pkg/server/container_create.go | 26 +++-- pkg/server/container_create_test.go | 152 ++++++++++++++++++++-------- 4 files changed, 135 insertions(+), 52 deletions(-) diff --git a/docs/config.md b/docs/config.md index 2ae24647a..6f363a8b1 100644 --- a/docs/config.md +++ b/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. diff --git a/pkg/config/config.go b/pkg/config/config.go index b3c7a574f..dc39616e1 100644 --- a/pkg/config/config.go +++ b/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 diff --git a/pkg/server/container_create.go b/pkg/server/container_create.go index e8ef19d83..d6f9779c4 100644 --- a/pkg/server/container_create.go +++ b/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)) } diff --git a/pkg/server/container_create_test.go b/pkg/server/container_create_test.go index 4a20225ca..d340c27d5 100644 --- a/pkg/server/container_create_test.go +++ b/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 - spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}) + containerConfig.Linux.SecurityContext.Capabilities = test.capability + 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 - spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}) + containerConfig.Tty = tty + 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 - spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}) + containerConfig.Linux.SecurityContext.ReadonlyRootfs = readonly + 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} - spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}) + containerConfig.Linux.SecurityContext.NamespaceOptions = &runtime.NamespaceOption{Pid: test.pidNS} + 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 - config.Linux.SecurityContext.ReadonlyPaths = test.readonly - config.Linux.SecurityContext.Privileged = test.privileged + containerConfig.Linux.SecurityContext.MaskedPaths = test.masked + containerConfig.Linux.SecurityContext.ReadonlyPaths = test.readonly + 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) + } + } +}