Skip to content

oci-worker: experimental support for rootless mode#419

Merged
tonistiigi merged 1 commit into
moby:masterfrom
AkihiroSuda:rootless
Jun 1, 2018
Merged

oci-worker: experimental support for rootless mode#419
tonistiigi merged 1 commit into
moby:masterfrom
AkihiroSuda:rootless

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

This PR allows running BuildKit without root privileges using the latest unpatched runc.

  • The username space needs to be unshared before running buildkitd. To allow subuid/subgid/setgroups (typically required by apt), newuidmap and newgidmap can be used. (img automatically does this)
  • Network namespace is not used now, but we could set up it with either SUID lxc-usernet or slirp/netstack. I'll soon implement a utility for this, probably as a separate project.

For further information, please refer to docs/rootless.md.

Closes #252

cc @tonistiigi @jessfraz @alexellis @cyphar

Comment thread docs/rootless.md
# Rootless mode (Experimental)

Requirements:
- runc (May 30, 2018) or later
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upstream master is broken as of writing due to a merge conflict but will be fixed immediately in opencontainers/runc#1808

Comment thread cmd/buildkitd/main.go Outdated
Usage: "ca certificate to verify clients",
},
cli.BoolFlag{
Name: "rootless",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be oci-worker-rootless ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be global so that we can switch the default root dir and the default socket path?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe both then and change the description of this field to "set all the default options to be compatible with rootless containers". Global --rootless would then behave as a shortcut that is the only flag that needs to be set for quickly running in rootless mode. But if you set all the other flags (addr, root, worker-rootless) manually then you don't even need it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment thread docs/rootless.md
# Rootless mode (Experimental)

Requirements:
- runc (May 30, 2018) or later
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update test.Dockerfile to this? Also, maybe create buildkit-runc with make binaries as executor already supports this optional override.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we get the integration tests running using the rootless worker? Can be follow-up if there are complications.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably after we have the tool for setting up newuidmap newgidmap?

The test has been substantially covered in genuinetools/img.

Comment thread docs/rootless.md
- Some distros such as Arch Linux require `echo 1 > /proc/sys/kernel/unprivileged_ns_clone`
- To run in a Docker container with non-root `USER`, `docker run --privileged` is still required. See also Jessie's blog: https://blog.jessfraz.com/post/building-container-images-securely-on-kubernetes/

Setting up rootless mode also requires some bothersome steps as follows, but we will soon have automation tool.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: can this all run in buildkitd startup in the future?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it will be a separate binary so that it can be shared across different projects like

$ rootlesskit --user=suid --net=usermode buildkitd

But I'll try to implement rootlesskit so that it can be also used as a Go (>= 1.10) library that can be imported from buildkitd

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does this setup require setuid binary or not?

Copy link
Copy Markdown
Member Author

@AkihiroSuda AkihiroSuda May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still newuidmap and newgidmap with setuid bit are required.

We could also use ptrace instead as in https://github.com/rootless-containers/runrootless , but slow and unstable. (although probably acceptable for just installing binary packages: rootless-containers/runrootless#14 )

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread executor/runcexecutor/executor.go Outdated
// Set the oom_score_adj of our children containers to that of the current process.
b, err := ioutil.ReadFile("/proc/self/oom_score_adj")
if err != nil {
return fmt.Errorf("reading /proc/self/oom_score_adj failed: %v", err)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: errors.Errorf, line 243 as well

Comment thread docs/rootless.md Outdated

Requirements:
- runc (May 30, 2018) or later
- Some distros such as Arch Linux require `echo 1 > /proc/sys/kernel/unprivileged_ns_clone`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this unprivileged_userns_clone ?

Comment thread docs/rootless.md
penguin:231072:65536
$ grep $(whoami) /etc/subgid
penguin:231072:65536
$ newuidmap $(cat /tmp/pid) 0 1001 1 1 231072 65536
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend installing uidmap pkg ?

@AkihiroSuda AkihiroSuda force-pushed the rootless branch 7 times, most recently from 847f240 to 4bb4e25 Compare May 30, 2018 09:33
@AkihiroSuda
Copy link
Copy Markdown
Member Author

Opened containerd PR containerd/containerd#2006

Some duplicated codes can be eliminated after the containerd PR gets merged.

Comment thread cmd/buildkitd/main.go
})
defaultRoot = appdefaults.UserRoot()
defaultAddress = appdefaults.UserAddress()
appdefaults.EnsureUserAddressDir()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an easy way to switch the snapshotter as well when overlay is not supported? Or maybe just default to naive then.

Comment thread cmd/buildkitd/main_containerd_worker.go Outdated
Usage: "user-specific annotation labels (com.example.foo=bar)",
},
}
n := "containerd-worker-rootless"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to add this until containerd is not actually supported?

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda
Copy link
Copy Markdown
Member Author

updated

Copy link
Copy Markdown
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AkihiroSuda
Copy link
Copy Markdown
Member Author

Mergeable?

@tonistiigi tonistiigi merged commit 65b5264 into moby:master Jun 1, 2018
@jessfraz
Copy link
Copy Markdown
Contributor

jessfraz commented Jun 1, 2018

Awesome!

Comment thread README.md

### Running BuildKit without root privileges

Please refer to `[docs/rootless.md]`(docs/rootless.md).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkihiroSuda (edit) this doesn't show as link because of quotes

@alexellis
Copy link
Copy Markdown

Very cool Akihiro - I think there will be a lot of interest for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants