unprivileged: add CLI options for isolation and storage #220
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
|
|
||
| "github.com/containers/storage" | ||
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| // Return "chroot" if we know we're not actually root, "oci" otherwise. | ||
| func builderDefaultIsolation() (string, error) { | ||
| if inUserNamespace() { | ||
| // We probably don't have enough privileges to use a proper | ||
| // runtime. | ||
| // Lean on the container that we're in being itself | ||
| // unprivileged (i.e., having control groups including the | ||
| // device cgroup configured for us, being provided with a | ||
| // smaller set of devices in /dev, and likely running without a | ||
| // few capabilities that we don't need), and reduce the degree | ||
| // of isolation that we try to use to what we know we're | ||
| // actually allowed to do in an unprivileged container. | ||
| return "chroot", nil | ||
| } | ||
| // Use the proper runtime. | ||
| return "oci", nil | ||
| } | ||
|
|
||
| // Check that /dev/fuse is usable and we have the fuse-overlayfs helper. | ||
| func builderCanUseOverlayFUSE() error { | ||
| device, err := os.Open("/dev/fuse") | ||
| if err != nil { | ||
| return fmt.Errorf("error opening device: %v", err) | ||
| } | ||
| defer device.Close() | ||
| if _, err := os.Stat("/usr/bin/fuse-overlayfs"); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Try various storage setups until we find one that works with the privileges | ||
| // that we currently have. | ||
| func builderDefaultStorage() (string, string, error) { | ||
| for _, candidate := range []struct { | ||
| driver, options string | ||
| also func() error | ||
| }{ | ||
| {"overlay", `["mountopt=metacopy=on"]`, nil}, | ||
| {"overlay", ``, nil}, | ||
| {"overlay", `["mount_program=/usr/bin/fuse-overlayfs","mountopt=metacopy=on"]`, builderCanUseOverlayFUSE}, | ||
| {"overlay", `["mount_program=/usr/bin/fuse-overlayfs"]`, builderCanUseOverlayFUSE}, | ||
| {"vfs", "", nil}, | ||
|
Contributor
There was a problem hiding this comment. ah the order of precedence @nalind mentioned in office hours today :-) |
||
| } { | ||
| var options []string | ||
| // Is there an additional test? | ||
| if candidate.also != nil { | ||
| if why := candidate.also(); why != nil { | ||
| klog.V(2).Info(why.Error()) | ||
| continue | ||
| } | ||
| } | ||
| // Are there options for this case? | ||
| if candidate.options != "" { | ||
| err := json.Unmarshal([]byte(candidate.options), &options) | ||
| if err != nil { | ||
| klog.Errorf("internal error parsing options %q: %v", candidate.options, err) | ||
| continue | ||
| } | ||
| } | ||
| // Precreate some things. | ||
| if _, err := os.Stat(fmt.Sprintf("/var/lib/shared/%s-layers/layers.lock", candidate.driver)); err == nil { | ||
| if _, err := os.Stat(fmt.Sprintf("/var/lib/shared/%s-images/images.lock", candidate.driver)); err == nil { | ||
| if _, err := os.Stat(fmt.Sprintf("/var/lib/shared/%s-containers/containers.lock", candidate.driver)); err == nil { | ||
| options = append(options, fmt.Sprintf("%s.imagestore=/var/lib/shared", candidate.driver)) | ||
| } | ||
| } | ||
| } | ||
| // Clear out the directory we're about to use. | ||
| os.RemoveAll("/var/lib/containers/storage/tmp") | ||
| os.RemoveAll("/run/containers/storage/tmp") | ||
| // Try to initialize storage. | ||
| store, err := storage.GetStore(storage.StoreOptions{ | ||
| GraphRoot: "/var/lib/containers/storage/tmp", | ||
| RunRoot: "/run/containers/storage/tmp", | ||
| GraphDriverName: candidate.driver, | ||
| GraphDriverOptions: options, | ||
| }) | ||
| if err != nil { | ||
| klog.V(2).Infof("Unable to initialize storage %q with options %v: %v\n", candidate.driver, options, err) | ||
| continue | ||
| } | ||
| // Shut down the storage that we were able to initialize. | ||
| _, err = store.Shutdown(true) | ||
| // Re-encode the options before returning them. | ||
| reencodedOptions, err := json.Marshal(options) | ||
| if err != nil { | ||
| klog.Errorf("Error re-encoding options %v: %v\n", options, err) | ||
| continue | ||
| } | ||
| klog.Infof("Defaulting to storage driver %q with options %v.\n", candidate.driver, options) | ||
| return candidate.driver, string(reencodedOptions), nil | ||
| } | ||
| return "", "", nil | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If we are in a user namespace, does this make us vulnerable to https://access.redhat.com/security/cve/CVE-2021-20182?
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.
In an unprivileged container, the builder container will already be have a device control group configured for limiting which devices the kernel will allow direct access to, even when the nodes exist, and
/devwill have the smaller set of entries in it which we grant to unprivileged containers. I expect we'll also be running without CAP_MKNOD.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.
fwiw @adambkaplan we also dove into the CVE ramifications back in March with #220 (comment)
bottom line: "we appear good"
that said, I think @nalind 's comment here ^^ would make a good comment when we see this code again in 6 months and ask the same question, not remembering this discussion :-)
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.
Adding
Lean on the container that we're in being itself unprivileged (i.e., having control groups including the device cgroup configured for us, being provided with a smaller set of devices in /dev, and likely running without a few capabilities that we don't need), and reduce the degree of isolation that we try to use to what we know we're actually allowed to do in an unprivileged container.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.
thanks @nalind