Skip to content

Commit

Permalink
Add flag to overload default privileged host device behaviour
Browse files Browse the repository at this point in the history 
Fixes containerd#1213

Signed-off-by: Alex Price <[email protected]>
  • Loading branch information
@awprice
awprice committed Aug 7, 2019
1 parent c7b48c0 commit 4959316
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 16 deletions.
3 changes: 3 additions & 0 deletions pkg/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 8 additions & 3 deletions pkg/server/container_create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,8 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
logrus.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.PodAnnotations,
ociRuntime.PrivilegedWithoutHostDevices)
if err != nil {
return nil, errors.Wrapf(err, "failed to generate container %q spec", id)
}
Expand Down Expand Up @@ -318,7 +319,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,
runtimePodAnnotations []string, privilegedWithoutHostDevices bool) (*runtimespec.Spec, error) {

specOpts := []oci.SpecOpts{
customopts.WithoutRunMount,
Expand Down Expand Up @@ -380,7 +382,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 !privilegedWithoutHostDevices {
specOpts = append(specOpts, customopts.WithPrivilegedDevices)
}
} else { // not privileged
specOpts = append(specOpts, customopts.WithDevices(c.os, config), customopts.WithCapabilities(securityContext))
}
Expand Down
78 changes: 65 additions & 13 deletions pkg/server/container_create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package server

import (
"github.com/opencontainers/runc/libcontainer/devices"
"os"
"path/filepath"
"reflect"
Expand Down Expand Up @@ -197,7 +198,7 @@ func TestGeneralContainerSpec(t *testing.T) {
config, sandboxConfig, imageConfig, specCheck := getCreateContainerTestData()
c := newTestCRIService()
testSandboxID := "sandbox-id"
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}, false)
require.NoError(t, err)
specCheck(t, testID, testSandboxID, testPid, spec)
}
Expand Down Expand Up @@ -253,7 +254,7 @@ func TestContainerCapabilities(t *testing.T) {
c := newTestCRIService()

config.Linux.SecurityContext.Capabilities = test.capability
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}, false)
require.NoError(t, err)
specCheck(t, testID, testSandboxID, testPid, spec)
for _, include := range test.includes {
Expand All @@ -280,7 +281,7 @@ func TestContainerSpecTty(t *testing.T) {
c := newTestCRIService()
for _, tty := range []bool{true, false} {
config.Tty = tty
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{})
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}, false)
require.NoError(t, err)
specCheck(t, testID, testSandboxID, testPid, spec)
assert.Equal(t, tty, spec.Process.Terminal)
Expand Down Expand Up @@ -348,7 +349,7 @@ func TestPodAnnotationPassthroughContainerSpec(t *testing.T) {
}

spec, err := c.generateContainerSpec(testID, testSandboxID, testPid,
config, sandboxConfig, imageConfig, nil, test.podAnnotations)
config, sandboxConfig, imageConfig, nil, test.podAnnotations, false)
assert.NoError(t, err)
assert.NotNil(t, spec)
specCheck(t, testID, testSandboxID, testPid, spec)
Expand All @@ -368,7 +369,7 @@ func TestContainerSpecReadonlyRootfs(t *testing.T) {
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{})
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}, false)
require.NoError(t, err)
specCheck(t, testID, testSandboxID, testPid, spec)
assert.Equal(t, readonly, spec.Root.Readonly)
Expand Down Expand Up @@ -405,7 +406,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, config, sandboxConfig, imageConfig, extraMounts, []string{}, false)
require.NoError(t, err)
specCheck(t, testID, testSandboxID, testPid, spec)
var mounts, sysMounts, devMounts []runtimespec.Mount
Expand Down Expand Up @@ -471,7 +472,7 @@ func TestContainerAndSandboxPrivileged(t *testing.T) {
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, config, sandboxConfig, imageConfig, nil, []string{}, false)
if test.expectError {
assert.Error(t, err)
} else {
Expand Down Expand Up @@ -790,7 +791,7 @@ func TestPrivilegedBindMount(t *testing.T) {
config.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, config, sandboxConfig, imageConfig, nil, nil, false)

assert.NoError(t, err)
if test.expectedSysFSRO {
Expand Down Expand Up @@ -945,7 +946,7 @@ 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{})
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, []string{}, false)
require.NoError(t, err)
assert.Contains(t, spec.Linux.Namespaces, test.expected)
}
Expand All @@ -958,7 +959,7 @@ func TestNoDefaultRunMount(t *testing.T) {
config, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
c := newTestCRIService()

spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, nil)
spec, err := c.generateContainerSpec(testID, testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, nil, false)
assert.NoError(t, err)
for _, mount := range spec.Mounts {
assert.NotEqual(t, "/run", mount.Destination)
Expand Down Expand Up @@ -1164,7 +1165,7 @@ func TestMaskedAndReadonlyPaths(t *testing.T) {
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, config, sandboxConfig, imageConfig, nil, []string{}, false)
require.NoError(t, err)
if !test.privileged { // specCheck presumes an unprivileged container
specCheck(t, testID, testSandboxID, testPid, spec)
Expand Down Expand Up @@ -1209,7 +1210,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, config, sandboxConfig, imageConfig, nil, []string{}, false)
require.NoError(t, err)
specCheck(t, testID, testSandboxID, testPid, spec)
assert.Contains(t, spec.Process.Env, test.expectedEnv)
Expand All @@ -1220,7 +1221,7 @@ func TestDisableCgroup(t *testing.T) {
config, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
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, config, sandboxConfig, imageConfig, nil, []string{}, false)
require.NoError(t, err)

t.Log("resource limit should not be set")
Expand All @@ -1230,3 +1231,54 @@ func TestDisableCgroup(t *testing.T) {
t.Log("cgroup path should be empty")
assert.Empty(t, spec.Linux.CgroupsPath)
}

func TestPrivilegedDevices(t *testing.T) {
testPid := uint32(1234)
c := newTestCRIService()
testSandboxID := "sandbox-id"
config, 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)

config.Linux.SecurityContext.Privileged = test.privileged
sandboxConfig.Linux.SecurityContext.Privileged = test.privileged

spec, err := c.generateContainerSpec(t.Name(), testSandboxID, testPid, config, sandboxConfig, imageConfig, nil, nil, test.privilegedWithoutHostDevices)
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)
}
}
}

0 comments on commit 4959316

Please sign in to comment.