Disable unsafe container options #1260

helsaawy · 2022-01-03T19:33:42Z

Add annotations to disable unsafe container operations, regardless of container spec:

adding writable vSMB or plan9 file shares to hypervisor isolated containers' UVM
using gMSAs for WCOW containers
(Annotation to disable vSMB direct maps already exists)

This PR leverages the default annotations field in the shim options to disable them globally, so orchestrators and clients do not have to vet and process specs for containers.

A disable GMSA flag is parsed from annotations in containerd-shim-runhcs-v1\task_hcs.go:createContainer, which is common to all pod and task creation, process and hypervisor isolated, and sets a flag in hcsoci.CreateOptions. The flag is checked in hcsoci.validateContainerConfig. This will also prevent LCOW container creation if the annotation is set and the Windows.CredentialSpec field in the OCI spec is set.

The disable writable shares flag in uvm.Options is parsed from the annotations and sets the same flag in uvm.UtilityVM. uvm.AddVSMB and uvm.AddPlan9 both error on creating a writable file share in a VM where they are disabled.

Added functionality to expand an annotation into a group of annotations with the same value set. This way a single annotation to disable unsafe annotations can be expanded transparently into the individual annotations that each control one feature.

Moved annotation related code from oci/uvm.go to oci/annotations.go, including code to perform annotation expansion

Grouped together setting common [LW]COW options in one function to improve readability.

Added unit tests for annotation expansion and proper initialization of common uVM creation options from the spec; cri-containerd tests for disabling gMSA creds and writable fileshares; and functional tests for disabling writable file shares directly on uVMs.

Currently, all functional tests are broken.

Signed-off-by: Hamza El-Saawy [email protected]

pkg/annotations/annotations.go

test/functional/uvm_vsmb_test.go

internal/oci/uvm_test.go

pkg/annotations/annotations.go

dcantah · 2022-01-03T20:10:56Z

Did a quick run for the small things, will check again by EOD hopefully for a more in depth review

katiewasnothere · 2022-01-03T20:14:24Z

We should probably check that anywhere we call DefaultVSMBOptions will still work as intended. Specifically, if it's a mount we're trying to add as part of our setup work (aka not a mount that the user is trying to make) that we do the right thing.

pkg/annotations/annotations.go

test/cri-containerd/container_fileshare_test.go

test/cri-containerd/container_gmsa_test.go

kevpar · 2022-01-03T20:31:56Z

It would be ideal to have:

A single annotation to disable all unsafe behavior for hostile multi-tenant environments. I don't have a preference on if we keep individual annotations for each type of resource as well, but a single way to say "make this safe" is important. If we do keep individual annotations as well then it should be an error to set one of the other annotations in a way that conflicts.
A shim config option that sets the default value for the "make this safe" annotation.

kevpar · 2022-01-03T20:32:57Z

It would be ideal to have:

A single annotation to disable all unsafe behavior for hostile multi-tenant environments. I don't have a preference on if we keep individual annotations for each type of resource as well, but a single way to say "make this safe" is important. If we do keep individual annotations as well then it should be an error to set one of the other annotations in a way that conflicts.

A shim config option that sets the default value for the "make this safe" annotation.

Actually, we probably don't need the second item, and can just use the default annotations feature instead.

katiewasnothere · 2022-01-03T20:34:56Z

I agree with @kevpar. We have a way to validate a container's spec before proceeding, but it would be worth it to invest some time to add conflict detection for all the annotations specifically for the UVM.

helsaawy · 2022-01-03T20:35:09Z

It would be ideal to have:

A single annotation to disable all unsafe behavior for hostile multi-tenant environments. I don't have a preference on if we keep individual annotations for each type of resource as well, but a single way to say "make this safe" is important. If we do keep individual annotations as well then it should be an error to set one of the other annotations in a way that conflicts.

A shim config option that sets the default value for the "make this safe" annotation.

That works for me: I should be able to add a translation early on in creation process to translate the uni-bus "disable all unsafe behavior" annotation into the sub-annotations for the individual features and check for conflicts

helsaawy · 2022-01-04T03:15:06Z

We should probably check that anywhere we call DefaultVSMBOptions will still work as intended. Specifically, if it's a mount we're trying to add as part of our setup work (aka not a mount that the user is trying to make) that we do the right thing.

Tracking this down, the only two places that DefaultVSMBOptions is called in hcsshim with readOnly not hardwired to true is for adding non-root mounts (hcsoci.setupMounts) and sharing in files to the UVM (uvm.Share) (which looks like is only used for shimdiag share requests).

All the setup works seems to mount as read only.

internal/oci/uvm.go

jterry75 · 2022-01-05T21:57:55Z

Can someone inform me why we need to disable gMSA versus just not passing in a gMSA cred? Don't those do the same thing? Isnt it up to the orchestrator to pass in the right opts?

helsaawy · 2022-01-05T22:23:47Z

Can someone inform me why we need to disable gMSA versus just not passing in a gMSA cred? Don't those do the same thing? Isnt it up to the orchestrator to pass in the right opts?

The motivation was for situations where the container options are passed in from customers, this adds an additional layer of security to explicitly deny gMSA in certain situations, regardless of the spec.

internal/oci/uvm.go

internal/hcsoci/create.go

internal/oci/uvm.go

pkg/annotations/annotations.go

internal/hcsoci/create.go

Added annotation to disable adding writable vSMB or plan 9 file shares to a UVM. Grouped together setting common *COW options into a common function to improve readability. Added tests for proper initialization and processing of common creation options, as well as functional tests for disabling writable shares. Currently, functional tests do not run smoothly. Signed-off-by: Hamza El-Saawy <[email protected]>

Signed-off-by: Hamza El-Saawy <[email protected]>

internal/oci/annotations_test.go

internal/uvm/create.go

internal/uvm/types.go

katiewasnothere

Couple comments but mostly lgtm

katiewasnothere

lgtm again, thanks

dcantah · 2022-02-24T01:06:40Z

test/cri-containerd/container_fileshare_test.go

+	ioutil.WriteFile(
+		filepath.Join(tempDir, testFile),
+		[]byte(testContent),
+		0644,
+	)


Should check if the write actually worked, otherwise the rest of the test will be fun to debug haha

dcantah · 2022-02-24T01:11:41Z

test/cri-containerd/container_fileshare_test.go

+	ioutil.WriteFile(
+		filepath.Join(tempDir, testFile),
+		[]byte(testContent),
+		0644,
+	)


dcantah

lgtm, small test changes

Can now expand an annotation into several sub annotations to toggle groups of features on and off. Added tests for this as well. Moved general annotation handling and parsing from `internal/oci/uvm.go` to `internal/oci/annotations`, and updated tests. Combined common OCI and gMSA tests together with subtests Made capitalization for GMSA consistent throughout. Removed changes to test/functional/utilities Removed unnecessary error return. Removed creation option for disabling gMSA and instead parse the annotation directly from the spec. Signed-off-by: Hamza El-Saawy <[email protected]>

Related work items: #1240, #1241, #1258, #1260, #1270, #1271, #1272, #1279, #1286, #1288, #1289, #1292, #1293, #1294, #1295, #1297, #1298, #1299, #1301, #1305, #1307, #1308, #1309, #1310, #1313, #1314, #1315, #1318, #1320, #1321, #1322, #1324, #1326, #1328, #1332, #1333, #1334, #1335, #1337

Add annotations to disable unsafe container operations, regardless of container spec: * adding writable vSMB or plan9 file shares to hypervisor isolated containers' UVM * using gMSAs for WCOW containers (Annotation to disable vSMB direct maps already exists) Signed-off-by: Hamza El-Saawy [email protected]

helsaawy requested a review from a team as a code owner January 3, 2022 19:33

dcantah reviewed Jan 3, 2022

View reviewed changes

pkg/annotations/annotations.go Outdated Show resolved Hide resolved

dcantah reviewed Jan 3, 2022

View reviewed changes

test/functional/uvm_vsmb_test.go Outdated Show resolved Hide resolved

dcantah self-assigned this Jan 3, 2022