-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable unsafe container options #1260
Conversation
Did a quick run for the small things, will check again by EOD hopefully for a more in depth review |
We should probably check that anywhere we call |
It would be ideal to have:
|
Actually, we probably don't need the second item, and can just use the default annotations feature instead. |
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. |
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 |
Tracking this down, the only two places that All the setup works seems to mount as read only. |
b115da0
to
8f34d93
Compare
5a0f624
to
21a61d2
Compare
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. |
bb3c9ab
to
36ea1a9
Compare
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 <hamzaelsaawy@microsoft.com>
Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments but mostly lgtm
14bd51b
to
e09cab2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm again, thanks
ioutil.WriteFile( | ||
filepath.Join(tempDir, testFile), | ||
[]byte(testContent), | ||
0644, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check if the write actually worked, otherwise the rest of the test will be fun to debug haha
ioutil.WriteFile( | ||
filepath.Join(tempDir, testFile), | ||
[]byte(testContent), | ||
0644, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <hamzaelsaawy@microsoft.com>
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 hamzaelsaawy@microsoft.com
Add annotations to disable unsafe container operations, regardless of container spec:
(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 inhcsoci.CreateOptions
. The flag is checked inhcsoci.validateContainerConfig
. This will also prevent LCOW container creation if the annotation is set and theWindows.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 inuvm.UtilityVM
.uvm.AddVSMB
anduvm.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
tooci/annotations.go
, including code to perform annotation expansionGrouped 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 hamzaelsaawy@microsoft.com