unprivileged: add CLI options for isolation and storage #220

nalind · 2021-03-09T16:41:50Z

Add CLI flags for controlling the type of process isolation we use for RUN instructions, and for controlling the storage driver and options that we pass to it, along with logic for guessing the right defaults. Drop environment variables that controlled these settings. This renders the static storage.conf file in the builder image largely irrelevant.

~~Add /etc/subuid and /etc/subgid files to the builder image that allow root to map the entire ID namespace.~~

Have the docker and sti builders re-exec themselves in user namespaces when they're run as non-UID=0 or without the CAP_SYS_ADMIN capabiity, so that they will be able to create new namespaces when processing RUN instructions.

With no CLI flags supplied and in a privileged pod, the builder should continue using the overlay driver using the kernel's overlay filesystem, and the OCI runtime.

gabemontero

@nalind @adambkaplan - perhaps I missed it in scrum, but is this change the final step for the builder image at least (i.e. ignore OCM changes we would make at some point, or global build config defaults) to support unprivileged builds? weren't there some k8s changes we were waiting on as well? if so, are those now there? or were those k8s changes for caching (I've forgotten these details more than once before :-) )

Should this PR be assoicated with https://issues.redhat.com/browse/BUILD-119

all in all, auspicious stuff :-)

gabemontero · 2021-03-09T17:57:45Z

pkg/build/builder/daemonless.go

+	switch isolationSpec {
+	case "", "oci":
+		isolation = buildah.IsolationOCI
+	case "chroot":


should we be adding chroot isolation back @nalind, given the now unembargoed CVE that resulted in us removing it ?

or is there some non-root user combination with chroot isolation that avoids the concerns the CVE cited?

chrooting doesn't provide the isolation that you'd expect from a container, so a process we execute using it has privileges similar to the process that called it. When called from inside of an unprivileged container, the unprivilegedness of that container provides the isolation we'd expect.

ok, so to map that back to my wording, chrooting when we are unprivileged avoids the pitfalls the CVE unveiled

agree with ^^ @nalind ?

Not having a device control group set up is not an issue we have to avoid.

ok that sounds like the "why" for "chrooting when we are unprivileged avoids the pitfalls the CVE unveiled"

nalind · 2021-03-09T18:35:08Z

When we're launched with privileged: false, we don't have enough privileges to run runc.

@nalind @adambkaplan - perhaps I missed it in scrum, but is this change the final step for the builder image at least (i.e. ignore OCM changes we would make at some point, or global build config defaults) to support unprivileged builds? weren't there some k8s changes we were waiting on as well? if so, are those now there? or were those k8s changes for caching (I've forgotten these details more than once before :-) )

The build controller would still need to set "privileged: false" for the build pod, and optionally specify what type of isolation to use using the CLI "--isolation" flag to override the default that this PR adds.

Should this PR be assoicated with https://issues.redhat.com/browse/BUILD-119

If "rootless" there means "uid != 0" from the node's point of view, then no, this doesn't get us that.

gabemontero · 2021-03-09T19:23:05Z

When we're launched with privileged: false, we don't have enough privileges to run runc.

@nalind @adambkaplan - perhaps I missed it in scrum, but is this change the final step for the builder image at least (i.e. ignore OCM changes we would make at some point, or global build config defaults) to support unprivileged builds? weren't there some k8s changes we were waiting on as well? if so, are those now there? or were those k8s changes for caching (I've forgotten these details more than once before :-) )

The build controller would still need to set "privileged: false" for the build pod, and optionally specify what type of isolation to use using the CLI "--isolation" flag to override the default that this PR adds.

Yeah that is the OCM related changes I'm referring to

Should this PR be assoicated with https://issues.redhat.com/browse/BUILD-119

If "rootless" there means "uid != 0" from the node's point of view, then no, this doesn't get us that.

I honestly don't remember what was meant by "rootless" when Mrunal opened BUILD-119 and when @siamaksade opened https://issues.redhat.com/browse/BUILD-141

I'll defer to @adambkaplan

nalind · 2021-03-09T22:45:04Z

/test e2e-aws-builds

nalind · 2021-03-10T16:52:19Z

/hold

nalind · 2021-03-16T13:59:52Z

/retest

gabemontero · 2021-03-16T14:40:20Z

e2e-aws-builds -> Cannot exceed quota for InstanceProfilesPerAccount: 1000

cluster could not get off the ground

nalind · 2021-03-19T14:07:06Z

/hold cancel

nalind · 2021-03-19T16:57:54Z

/test e2e-aws-image-ecosystem
/test e2e-aws-builds

gabemontero · 2021-03-19T18:20:59Z

@nalind you'll need openshift/origin#25985 to merge in order to get a passing image-ecosystem

nalind · 2021-03-19T20:00:05Z

/test e2e-aws-builds

nalind · 2021-03-23T13:58:34Z

/test e2e-aws-builds

nalind · 2021-03-23T18:00:13Z

/test e2e-aws-image-ecosystem

nalind · 2021-03-24T13:49:41Z

Rebased.

nalind · 2021-04-06T15:04:29Z

Rebased. This may solve bug 1937069, where "chown" during a build takes a while, which I suspect due to copy-up that can be avoided if we use the metacopy feature, which this PR attempts to detect and use if it's supported.

nalind · 2021-06-23T17:17:45Z

/retest

nalind · 2021-06-28T12:55:16Z

/retest

rhatdan · 2021-06-28T13:29:06Z

@gabemontero @coreydaley PTAL

We don't use the CreateContainer() method of pkg/build/builder.DockerClient, so remove it and the implementation we have of it. Signed-off-by: Nalin Dahyabhai <[email protected]>

gabemontero · 2021-07-01T14:00:06Z

per discussion in team scrum including with @nalind

/assign @adambkaplan

to minimally get the approve label on this, and if he has cycles decide whether it lgtm to him

I'll be taking another pass after posting this comment

gabemontero

Aside from the request for a comment below @nalind ideally some added unit tests in https://github.com/openshift/builder/blob/master/pkg/build/builder/cmd/builder_test.go and https://github.com/openshift/builder/blob/master/pkg/build/builder/daemonless_test.go to validate that the new parameters being passed in are processed correctly seems warranted.

Otherwise looks great / thanks !!

gabemontero · 2021-07-01T14:11:48Z

cmd/defaults.go

+		{"overlay", ``, nil},
+		{"overlay", `["mount_program=/usr/bin/fuse-overlayfs","mountopt=metacopy=on"]`, builderCanUseOverlayFUSE},
+		{"overlay", `["mount_program=/usr/bin/fuse-overlayfs"]`, builderCanUseOverlayFUSE},
+		{"vfs", "", nil},


ah the order of precedence @nalind mentioned in office hours today :-)

gabemontero · 2021-07-01T14:14:05Z

cmd/main.go

-	if err != nil {
-		fmt.Printf("Error setting up service CA cert: %v", err)
-		os.Exit(1)
+	if !inUserNamespace() {


So why do we only do this when not in user namespaces?

How is this setup provided when we are in user namespaces?

Looking for a comment that answers those questions. Thanks.

If we've set up a user namespace, then we know that we weren't started as UID 0 in the container, so these locations are not writable. If we try to write to those locations anyway, we hit a permissions error and exit. It's an unsolved problem for cases where we start the container as a UID other than 0.

OK capture that in a comment in the code and I'm good on this one

So as non-root we can't write to /etc/docker/certs.d? Interesting, may need to determine how we can have the build controller mount these for us.

Will this also impact our entrypoint script that generates the trust bundle?

Mounting the certs.d subpath of the build-ca-bundles volume to /etc/docker/certs.d should work, since the directory isn't even in the builder image to begin with. The cluster.crt that we drop in /etc/pki/tls/certs is trickier, since the directory already includes content that we want to still have there. We should be able to mount the file itself from the volume that contains the service account secrets, but I haven't personally tried that.
This may all be moot, though, since I think ensuring the directories exist and setting them group-writeable in the image will also work, with no code changes.

Going to try creating the directories and setting them group-writable in the Dockerfile instead. That'll probably work better for the entrypoint script, too.

OK if !inUserNamespace() condition has been removed along with the Dockerfile changes @nalind mentions ^^

adambkaplan

/approve

There are definite issues if we run in a user namespace around certs that need to be addressed. The scope is way beyond this PR and should be captured in JIRA.

adambkaplan · 2021-07-01T21:01:21Z

cmd/builder.go


+	flags := cmd.Flags()
+	flags.StringVar(&isolation, "isolation", isolation, "type of process `isolation` to use for RUN instructions")
+	flags.StringVar(&ociRuntime, "oci-runtime", ociRuntime, "runtime to invoke for OCI isolation")


What happens if --oci-runtime is not specified? Is there a reasonable default for the empty string?

We can retrieve a default from github.com/containers/common/pkg/config's DefaultConfig() function. I'll add that.

ok see that with https://github.com/openshift/builder/pull/220/files#diff-be76072c86dc24313ebc27eca7d613a1068dc337b068610f3f0e98f6e97ac5b0R71

adambkaplan · 2021-07-01T21:01:35Z

cmd/builder.go

+
+	flags := cmd.Flags()
+	flags.StringVar(&isolation, "isolation", isolation, "type of process `isolation` to use for RUN instructions")
+	flags.StringVar(&ociRuntime, "oci-runtime", ociRuntime, "runtime to invoke for OCI isolation")


Same as above

addressed via https://github.com/openshift/builder/pull/220/files#diff-be76072c86dc24313ebc27eca7d613a1068dc337b068610f3f0e98f6e97ac5b0R71 per @nalind

adambkaplan · 2021-07-01T21:06:58Z

cmd/defaults.go

+func builderDefaultIsolation() (string, error) {
+	if inUserNamespace() {
+		// We probably don't have enough privileges to use a proper runtime.
+		return "chroot", nil


If we are in a user namespace, does this make us vulnerable to https://access.redhat.com/security/cve/CVE-2021-20182?

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 /dev will have the smaller set of entries in it which we grant to unprivileged containers. I expect we'll also be running without CAP_MKNOD.

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 :-)

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.

thanks @nalind

adambkaplan · 2021-07-01T21:12:17Z

cmd/main.go

-	if err != nil {
-		fmt.Printf("Error setting up service CA cert: %v", err)
-		os.Exit(1)
+	if !inUserNamespace() {


So as non-root we can't write to /etc/docker/certs.d? Interesting, may need to determine how we can have the build controller mount these for us.

adambkaplan · 2021-07-01T21:13:38Z

cmd/main.go

-	if err != nil {
-		fmt.Printf("Error setting up service CA cert: %v", err)
-		os.Exit(1)
+	if !inUserNamespace() {


Will this also impact our entrypoint script that generates the trust bundle?

gabemontero · 2021-07-02T13:48:25Z

/approve

There are definite issues if we run in a user namespace around certs that need to be addressed. The scope is way beyond this PR and should be captured in JIRA.

I'm good with that ^^ but for me this highlights the need for comments I noted earlier. Even a TODO comment with a ref to whatever Jira gets opened for this.

nalind · 2021-07-02T19:14:50Z

/test e2e-aws-builds

Add CLI flags for controlling the type of process isolation we use for RUN instructions, and for controlling the storage driver and options that we pass to it, along with logic for guessing the right defaults. Drop environment variables that controlled these settings. Have the docker and sti builders re-exec themselves in user namespaces when they're passed --uidmap or --gidmap flags, or run as non-root or without CAP_SYS_ADMIN, so that they will be able to create new namespaces when processing RUN instructions. Signed-off-by: Nalin Dahyabhai <[email protected]>

gabemontero · 2021-07-02T19:48:18Z

last e2e-aws-builds run died in aws install

all the prior comments have been addressed, so

/lgtm

but let's keep an eye on whether possible future e2e-aws-build fail in the actual tests

also, given the fact that we are touching the cert path, I'm launching a

/test e2e-aws-proxy

(pretty sure we made that an optional possibility with this repo)

nalind · 2021-07-02T20:44:12Z

/test e2e-aws-proxy

gabemontero · 2021-07-02T23:22:41Z

any hint what changed from f0950af to f1de8bd @nalind ?

gabemontero · 2021-07-02T23:24:55Z

the one e2e-aws failure is suppose to be a low rate flake according to sippy with

Associated Bugs: 1972490 1973266 1978829

/test e2e-aws

nalind · 2021-07-12T12:55:40Z

any hint what changed from f0950af to f1de8bd @nalind ?

That should have dropped a helper function that was introduced in an earlier version of this PR but which is no longer used.

gabemontero · 2021-07-12T12:57:32Z

/lgtm

openshift-ci · 2021-07-12T12:57:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero, nalind, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from coreydaley and gabemontero March 9, 2021 16:43

gabemontero reviewed Mar 9, 2021

View reviewed changes

nalind force-pushed the unprivileged branch from 3c49829 to f0b70ce Compare March 9, 2021 18:35

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2021

nalind force-pushed the unprivileged branch from f0b70ce to 7e159c9 Compare March 15, 2021 21:38

nalind force-pushed the unprivileged branch 4 times, most recently from bd6d4de to 9b489dc Compare March 17, 2021 14:48

nalind mentioned this pull request Mar 17, 2021

BUILD-433: Run builds in user namespaces without seccomp if BUILD_PRIVILEGED=false openshift/openshift-controller-manager#173

Merged

nalind force-pushed the unprivileged branch 2 times, most recently from 1291d5a to 85fb2b7 Compare March 19, 2021 14:06

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2021

nalind force-pushed the unprivileged branch from 85fb2b7 to c60a2a7 Compare March 24, 2021 13:49

nalind force-pushed the unprivileged branch from c60a2a7 to 455f60f Compare April 6, 2021 14:46

nalind force-pushed the unprivileged branch from 8700b0e to 8ff945d Compare June 25, 2021 14:59

Remove dead code

2229dfd

We don't use the CreateContainer() method of pkg/build/builder.DockerClient, so remove it and the implementation we have of it. Signed-off-by: Nalin Dahyabhai <[email protected]>

nalind force-pushed the unprivileged branch from 8ff945d to f48f211 Compare June 30, 2021 20:52

openshift-ci bot assigned adambkaplan Jul 1, 2021

gabemontero suggested changes Jul 1, 2021

View reviewed changes

adambkaplan reviewed Jul 1, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2021

nalind force-pushed the unprivileged branch from f48f211 to f0950af Compare July 2, 2021 17:15

openshift-ci bot assigned gabemontero Jul 2, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2021

nalind force-pushed the unprivileged branch from f0950af to f1de8bd Compare July 2, 2021 19:56

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2021

openshift-merge-robot merged commit 80add46 into openshift:master Jul 12, 2021

nalind deleted the unprivileged branch July 12, 2021 13:02

nalind mentioned this pull request Jul 14, 2021

unprivileged: don't try to set up user namespaces for manage-dockerfile/git-clone #254

Merged

unprivileged: add CLI options for isolation and storage #220

unprivileged: add CLI options for isolation and storage #220

Uh oh!

Conversation

nalind commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabemontero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind commented Mar 9, 2021

Uh oh!

gabemontero commented Mar 9, 2021 • edited by openshift-ci-robot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nalind commented Mar 9, 2021

Uh oh!

nalind commented Mar 10, 2021

Uh oh!

nalind commented Mar 16, 2021

Uh oh!

gabemontero commented Mar 16, 2021

Uh oh!

nalind commented Mar 19, 2021

Uh oh!

nalind commented Mar 19, 2021

Uh oh!

gabemontero commented Mar 19, 2021

Uh oh!

nalind commented Mar 19, 2021

Uh oh!

nalind commented Mar 23, 2021

Uh oh!

nalind commented Mar 23, 2021

Uh oh!

nalind commented Mar 24, 2021

Uh oh!

nalind commented Apr 6, 2021

Uh oh!

nalind commented Jun 23, 2021

Uh oh!

nalind commented Jun 28, 2021

Uh oh!

rhatdan commented Jun 28, 2021

Uh oh!

gabemontero commented Jul 1, 2021

Uh oh!

gabemontero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabemontero Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind commented Mar 9, 2021 •

edited

Loading

gabemontero commented Mar 9, 2021 •

edited by openshift-ci-robot

Loading

gabemontero Jul 1, 2021 •

edited

Loading