Merge pull request #5686 from dtnyn/issue-5679

kzys · web-flow · commit f140400c0e6a · 2022-04-25T11:44:01.000-07:00
Add flag to allow oci.WithAllDevicesAllowed on PrivilegedWithoutHostDevices
diff --git a/docs/cri/config.md b/docs/cri/config.md
@@ -294,6 +294,14 @@ version = 2
       # i.e pass host devices through to privileged containers.
       privileged_without_host_devices = false
 
+      # privileged_without_host_devices_all_devices_allowed allows the allowlisting of all devices when
+      # privileged_without_host_devices is enabled.
+      # In plain privileged mode all host device nodes are added to the container's spec and all devices
+      # are put in the container's device allowlist. This flags is for the modification of the privileged_without_host_devices
+      # option so that even when no host devices are implicitly added to the container, all devices allowlisting is still enabled.
+      # Requires privileged_without_host_devices to be enabled. Defaults to false.
+      privileged_without_host_devices_all_devices_allowed = false
+
       # base_runtime_spec is a file path to a JSON file with the OCI spec that will be used as the base spec that all
       # container's are created from.
       # Use containerd's `ctr oci spec > /etc/containerd/cri-base.json` to output initial spec file.
diff --git a/pkg/cri/config/config.go b/pkg/cri/config/config.go
@@ -59,6 +59,10 @@ type Runtime struct {
 	// 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"`
+	// PrivilegedWithoutHostDevicesAllDevicesAllowed overloads the default behaviour device allowlisting when
+	// to the runtime spec when the container when PrivilegedWithoutHostDevices is already enabled. Requires
+	// PrivilegedWithoutHostDevices to be enabled. Defaults to false.
+	PrivilegedWithoutHostDevicesAllDevicesAllowed bool `toml:"privileged_without_host_devices_all_devices_allowed" json:"privileged_without_host_devices_all_devices_allowed"`
 	// BaseRuntimeSpec is a json file with OCI spec to use as base spec that all container's will be created from.
 	BaseRuntimeSpec string `toml:"base_runtime_spec" json:"baseRuntimeSpec"`
 	// NetworkPluginConfDir is a directory containing the CNI network information for the runtime class.
@@ -401,6 +405,9 @@ func ValidatePluginConfig(ctx context.Context, c *PluginConfig) error {
 			}
 			log.G(ctx).Warning("`runtime_root` is deprecated, please use runtime `options` instead")
 		}
+		if !r.PrivilegedWithoutHostDevices && r.PrivilegedWithoutHostDevicesAllDevicesAllowed {
+			return errors.Errorf("`privileged_without_host_devices_all_devices_allowed` requires `privileged_without_host_devices` to be enabled")
+		}
 	}
 
 	useConfigPath := c.Registry.ConfigPath != ""
diff --git a/pkg/cri/config/config_test.go b/pkg/cri/config/config_test.go
@@ -360,6 +360,21 @@ func TestValidateConfig(t *testing.T) {
 			},
 			expectedErr: "`configs.tls` cannot be set when `config_path` is provided",
 		},
+		"privileged_without_host_devices_all_devices_allowed without privileged_without_host_devices": {
+			config: &PluginConfig{
+				ContainerdConfig: ContainerdConfig{
+					DefaultRuntimeName: RuntimeDefault,
+					Runtimes: map[string]Runtime{
+						RuntimeDefault: {
+							PrivilegedWithoutHostDevices:                  false,
+							PrivilegedWithoutHostDevicesAllDevicesAllowed: true,
+							Type: "default",
+						},
+					},
+				},
+			},
+			expectedErr: "`privileged_without_host_devices_all_devices_allowed` requires `privileged_without_host_devices` to be enabled",
+		},
 	} {
 		t.Run(desc, func(t *testing.T) {
 			err := ValidatePluginConfig(context.Background(), test.config)
diff --git a/pkg/cri/server/container_create_linux.go b/pkg/cri/server/container_create_linux.go
@@ -227,6 +227,9 @@ func (c *criService) containerSpec(
 		specOpts = append(specOpts, oci.WithPrivileged)
 		if !ociRuntime.PrivilegedWithoutHostDevices {
 			specOpts = append(specOpts, oci.WithHostDevices, oci.WithAllDevicesAllowed)
+		} else if ociRuntime.PrivilegedWithoutHostDevicesAllDevicesAllowed {
+			// allow rwm on all devices for the container
+			specOpts = append(specOpts, oci.WithAllDevicesAllowed)
 		}
 	}
 
diff --git a/pkg/cri/server/container_create_linux_test.go b/pkg/cri/server/container_create_linux_test.go
@@ -1392,29 +1392,46 @@ func TestPrivilegedDevices(t *testing.T) {
 	containerConfig, sandboxConfig, imageConfig, _ := getCreateContainerTestData()
 
 	for desc, test := range map[string]struct {
-		privileged                   bool
-		privilegedWithoutHostDevices bool
-		expectHostDevices            bool
+		privileged                                    bool
+		privilegedWithoutHostDevices                  bool
+		privilegedWithoutHostDevicesAllDevicesAllowed bool
+		expectHostDevices                             bool
+		expectAllDevicesAllowed                       bool
 	}{
 		"expect no host devices when privileged is false": {
 			privileged:                   false,
 			privilegedWithoutHostDevices: false,
-			expectHostDevices:            false,
+			privilegedWithoutHostDevicesAllDevicesAllowed: false,
+			expectHostDevices:       false,
+			expectAllDevicesAllowed: false,
 		},
 		"expect no host devices when privileged is false and privilegedWithoutHostDevices is true": {
 			privileged:                   false,
 			privilegedWithoutHostDevices: true,
-			expectHostDevices:            false,
+			privilegedWithoutHostDevicesAllDevicesAllowed: false,
+			expectHostDevices:       false,
+			expectAllDevicesAllowed: false,
 		},
-		"expect host devices when privileged is true": {
+		"expect host devices and all device allowlist when privileged is true": {
 			privileged:                   true,
 			privilegedWithoutHostDevices: false,
-			expectHostDevices:            true,
+			privilegedWithoutHostDevicesAllDevicesAllowed: false,
+			expectHostDevices:       true,
+			expectAllDevicesAllowed: true,
 		},
 		"expect no host devices when privileged is true and privilegedWithoutHostDevices is true": {
 			privileged:                   true,
 			privilegedWithoutHostDevices: true,
-			expectHostDevices:            false,
+			privilegedWithoutHostDevicesAllDevicesAllowed: false,
+			expectHostDevices:       false,
+			expectAllDevicesAllowed: false,
+		},
+		"expect host devices and all devices allowlist when privileged is true and privilegedWithoutHostDevices is true and privilegedWithoutHostDevicesAllDevicesAllowed is true": {
+			privileged:                   true,
+			privilegedWithoutHostDevices: true,
+			privilegedWithoutHostDevicesAllDevicesAllowed: true,
+			expectHostDevices:       false,
+			expectAllDevicesAllowed: true,
 		},
 	} {
 		t.Logf("TestCase %q", desc)
@@ -1423,7 +1440,8 @@ func TestPrivilegedDevices(t *testing.T) {
 		sandboxConfig.Linux.SecurityContext.Privileged = test.privileged
 
 		ociRuntime := config.Runtime{
-			PrivilegedWithoutHostDevices: test.privilegedWithoutHostDevices,
+			PrivilegedWithoutHostDevices:                  test.privilegedWithoutHostDevices,
+			PrivilegedWithoutHostDevicesAllDevicesAllowed: test.privilegedWithoutHostDevicesAllDevicesAllowed,
 		}
 		spec, err := c.containerSpec(t.Name(), testSandboxID, testPid, "", testContainerName, testImageName, containerConfig, sandboxConfig, imageConfig, nil, ociRuntime)
 		assert.NoError(t, err)
@@ -1443,6 +1461,11 @@ func TestPrivilegedDevices(t *testing.T) {
 		} else {
 			assert.Empty(t, spec.Linux.Devices)
 		}
+
+		assert.Len(t, spec.Linux.Resources.Devices, 1)
+		assert.Equal(t, spec.Linux.Resources.Devices[0].Allow, test.expectAllDevicesAllowed)
+		assert.Equal(t, spec.Linux.Resources.Devices[0].Access, "rwm")
+
 	}
 }
 

Original file line number	Diff line number	Diff line change
`@@ -59,6 +59,10 @@ type Runtime struct {`
`59`	`59`	`// PrivilegedWithoutHostDevices overloads the default behaviour for adding host devices to the`
`60`	`60`	`// runtime spec when the container is privileged. Defaults to false.`
`61`	`61`	PrivilegedWithoutHostDevices bool `toml:"privileged_without_host_devices" json:"privileged_without_host_devices"`
	`62`	`+ // PrivilegedWithoutHostDevicesAllDevicesAllowed overloads the default behaviour device allowlisting when`
	`63`	`+ // to the runtime spec when the container when PrivilegedWithoutHostDevices is already enabled. Requires`
	`64`	`+ // PrivilegedWithoutHostDevices to be enabled. Defaults to false.`
	`65`	+ PrivilegedWithoutHostDevicesAllDevicesAllowed bool `toml:"privileged_without_host_devices_all_devices_allowed" json:"privileged_without_host_devices_all_devices_allowed"`
`62`	`66`	`// BaseRuntimeSpec is a json file with OCI spec to use as base spec that all container's will be created from.`
`63`	`67`	BaseRuntimeSpec string `toml:"base_runtime_spec" json:"baseRuntimeSpec"`
`64`	`68`	`// NetworkPluginConfDir is a directory containing the CNI network information for the runtime class.`
`@@ -401,6 +405,9 @@ func ValidatePluginConfig(ctx context.Context, c *PluginConfig) error {`
`401`	`405`	`}`
`402`	`406`	log.G(ctx).Warning("`runtime_root` is deprecated, please use runtime `options` instead")
`403`	`407`	`}`
	`408`	`+ if !r.PrivilegedWithoutHostDevices && r.PrivilegedWithoutHostDevicesAllDevicesAllowed {`
	`409`	+ return errors.Errorf("`privileged_without_host_devices_all_devices_allowed` requires `privileged_without_host_devices` to be enabled")
	`410`	`+ }`
`404`	`411`	`}`
`405`	`412`
`406`	`413`	`useConfigPath := c.Registry.ConfigPath != ""`
Original file line number	Diff line number	Diff line change
`@@ -227,6 +227,9 @@ func (c *criService) containerSpec(`
`227`	`227`	`specOpts = append(specOpts, oci.WithPrivileged)`
`228`	`228`	`if !ociRuntime.PrivilegedWithoutHostDevices {`
`229`	`229`	`specOpts = append(specOpts, oci.WithHostDevices, oci.WithAllDevicesAllowed)`
	`230`	`+ } else if ociRuntime.PrivilegedWithoutHostDevicesAllDevicesAllowed {`
	`231`	`+ // allow rwm on all devices for the container`
	`232`	`+ specOpts = append(specOpts, oci.WithAllDevicesAllowed)`
`230`	`233`	`}`
`231`	`234`	`}`
`232`	`235`