support --oci-worker-no-process-sandbox

Note that this mode allows build executor containers to kill (and potentially ptrace) an arbitrary process in the BuildKit host namespace. This mode should be enabled only when the BuildKit is running in a container as an unprivileged user. Signed-off-by: Akihiro Suda <[email protected]>
moby · Jan 8, 2019 · c54f4a9 · c54f4a9
1 parent a4e0df8
commit c54f4a9
Show file tree

Hide file tree

Showing 10 changed files with 351 additions and 46 deletions.
diff --git a/cmd/buildkitd/config/config.go b/cmd/buildkitd/config/config.go
@@ -49,12 +49,13 @@ type TLSConfig struct {
 }
 
 type OCIConfig struct {
-	Enabled     *bool             `toml:"enabled"`
-	Labels      map[string]string `toml:"labels"`
-	Platforms   []string          `toml:"platforms"`
-	Snapshotter string            `toml:"snapshotter"`
-	Rootless    bool              `toml:"rootless"`
-	GCPolicy    []GCPolicy        `toml:"gcpolicy"`
+	Enabled          *bool             `toml:"enabled"`
+	Labels           map[string]string `toml:"labels"`
+	Platforms        []string          `toml:"platforms"`
+	Snapshotter      string            `toml:"snapshotter"`
+	Rootless         bool              `toml:"rootless"`
+	NoProcessSandbox bool              `toml:"noProcessSandbox"`
+	GCPolicy         []GCPolicy        `toml:"gcpolicy"`
 }
 
 type ContainerdConfig struct {

diff --git a/cmd/buildkitd/main_oci_worker.go b/cmd/buildkitd/main_oci_worker.go
@@ -10,6 +10,7 @@ import (
 	"github.com/containerd/containerd/snapshots/native"
 	"github.com/containerd/containerd/snapshots/overlay"
 	"github.com/moby/buildkit/cmd/buildkitd/config"
+	"github.com/moby/buildkit/executor/oci"
 	"github.com/moby/buildkit/worker"
 	"github.com/moby/buildkit/worker/base"
 	"github.com/moby/buildkit/worker/runc"
@@ -66,6 +67,11 @@ func init() {
 			Usage: u,
 		})
 	}
+	flags = append(flags, cli.BoolFlag{
+		Name:  "oci-worker-no-process-sandbox",
+		Usage: "use the host PID namespace and procfs (WARNING: allows build containers to kill (and potentially ptrace) an arbitrary process in the host namespace)",
+	})
+
 	registerWorkerInitializer(
 		workerInitializer{
 			fn:       ociWorkerInitializer,
@@ -109,6 +115,9 @@ func applyOCIFlags(c *cli.Context, cfg *config.Config) error {
 	if c.GlobalIsSet("oci-worker-rootless") {
 		cfg.Workers.OCI.Rootless = c.GlobalBool("oci-worker-rootless")
 	}
+	if c.GlobalIsSet("oci-worker-no-process-sandbox") {
+		cfg.Workers.OCI.NoProcessSandbox = c.GlobalBool("oci-worker-no-process-sandbox")
+	}
 
 	if platforms := c.GlobalStringSlice("oci-worker-platform"); len(platforms) != 0 {
 		cfg.Workers.OCI.Platforms = platforms
@@ -136,7 +145,17 @@ func ociWorkerInitializer(c *cli.Context, common workerInitializerOpt) ([]worker
 	if cfg.Rootless {
 		logrus.Debugf("running in rootless mode")
 	}
-	opt, err := runc.NewWorkerOpt(common.config.Root, snFactory, cfg.Rootless, cfg.Labels)
+
+	processMode := oci.ProcessSandbox
+	if cfg.NoProcessSandbox {
+		logrus.Warn("NoProcessSandbox is enabled. Note that NoProcessSandbox allows build containers to kill (and potentially ptrace) an arbitrary process in the BuildKit host namespace. NoProcessSandbox should be enabled only when the BuildKit is running in a container as an unprivileged user.")
+		if !cfg.Rootless {
+			return nil, errors.New("can't enable NoProcessSandbox without Rootless")
+		}
+		processMode = oci.NoProcessSandbox
+	}
+
+	opt, err := runc.NewWorkerOpt(common.config.Root, snFactory, cfg.Rootless, processMode, cfg.Labels)
 	if err != nil {
 		return nil, err
 	}

diff --git a/docs/rootless.md b/docs/rootless.md
@@ -6,7 +6,6 @@ Requirements:
 - RHEL/CentOS 7 requires `sudo sh -c "echo 28633 > /proc/sys/user/max_user_namespaces"`. You may also need `sudo grubby --args="namespace.unpriv_enable=1 user_namespace.enable=1" --update-kernel="$(grubby --default-kernel)"`.
 - `newuidmap` and `newgidmap` need to be installed on the host. These commands are provided by the `uidmap` package. For RHEL/CentOS 7, RPM is not officially provided but available at https://copr.fedorainfracloud.org/coprs/vbatts/shadow-utils-newxidmap/ .
 - `/etc/subuid` and `/etc/subgid` should contain >= 65536 sub-IDs. e.g. `penguin:231072:65536`.
-- 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/
 
 
 ## Set up
@@ -63,8 +62,29 @@ Docker image is available as [`moby/buildkit:rootless`](https://hub.docker.com/r
 $ docker run --name buildkitd -d --privileged -p 1234:1234  moby/buildkit:rootless --addr tcp://0.0.0.0:1234
 ```
 
-`docker run` requires `--privileged` but the BuildKit daemon is executed as a normal user.
-See [`docker/cli#1347`](https://github.com/docker/cli/pull/1347) for the ongoing work to remove this requirement
+```
+$ go get ./examples/build-using-dockerfile
+$ build-using-dockerfile --buildkit-addr tcp://127.0.0.1:1234 -t foo /path/to/somewhere
+```
+
+### Security consideration
+
+Although `moby/buildkit:rootless` executes the BuildKit daemon as a normal user, `docker run` still requires `--privileged`.
+This is to allow build executor containers to mount `/proc`, by providing "unmasked" `/proc` to the BuildKit daemon container.
+
+See [`docker/cli#1347`](https://github.com/docker/cli/pull/1347) for the ongoing work to remove this requirement.
+See also [Disabling process sandbox](#disabling-process-sandbox).
+
+#### UID/GID
+
+The `moby/buildkit:rootless` image has the following UID/GID configuration:
+
+Actual ID (shown in the host and the BuildKit daemon container)| Mapped ID (shown in build executor containers)
+----------|----------
+1000      | 0
+100000    | 1
+...       | ...
+165535    | 65536
 
 ```
 $ docker exec buildkitd id
@@ -75,9 +95,91 @@ PID   USER     TIME   COMMAND
    13 user       0:00 /proc/self/exe buildkitd --addr tcp://0.0.0.0:1234
    21 user       0:00 buildkitd --addr tcp://0.0.0.0:1234
    29 user       0:00 ps aux
+$ docker exec cat /etc/subuid
+user:100000:65536
 ```
 
+To change the UID/GID configuration, you need to modify and build the BuildKit image manually.
 ```
-$ go get ./examples/build-using-dockerfile
-$ build-using-dockerfile --buildkit-addr tcp://127.0.0.1:1234 -t foo /path/to/somewhere
+$ vi hack/dockerfiles/test.Dockerfile
+$ docker build -t buildkit-rootless-custom --target rootless -f hack/dockerfiles/test.Dockerfile .
+```
+
+#### Disabling process sandbox
+
+By passing `--oci-worker-no-process-sandbox` to the `buildkitd` arguments, BuildKit can be executed in a container without `--privileged`.
+However, you still need to pass `--security-opt seccomp=unconfined --security-opt apparmor=unconfined` to `docker run`.
+
+```
+$ docker run --name buildkitd -d --security-opt seccomp=unconfined --security-opt apparmor=unconfined -p 1234:1234 moby/buildkit:rootless --addr tcp://0.0.0.0:1234 --oci-worker-no-process-sandbox
 ```
+
+Note that `--oci-worker-no-process-sandbox` allows build executor containers to `kill` (and potentially `ptrace` depending on the seccomp configuration) an arbitrary process in the BuildKit daemon container.
+
+
+## Set up (using Kubernetes)
+
+### With `securityContext`
+
+```yaml
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    app: buildkitd
+  name: buildkitd
+spec:
+  selector:
+    matchLabels:
+      app: buildkitd
+  template:
+    metadata:
+      labels:
+        app: buildkitd
+    spec:
+      containers:
+      - image: moby/buildkit:rootless
+        args:
+        - --addr
+        - tcp://0.0.0.0:1234
+        name: buildkitd
+        ports:
+        - containerPort: 1234
+        securityContext:
+          privileged: true
+```
+
+This configuration requires privileged containers to be enabled.
+
+If you are using Kubernetes v1.12+ with either Docker v18.06+, containerd v1.2+, or CRI-O v1.12+ as the CRI runtime, you can replace `privileged: true` with `procMount: Unmasked`.
+
+### Without `securityContext` but with `--oci-worker-no-process-sandbox`
+
+```yaml
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    app: buildkitd
+  name: buildkitd
+spec:
+  selector:
+    matchLabels:
+      app: buildkitd
+  template:
+    metadata:
+      labels:
+        app: buildkitd
+    spec:
+      containers:
+      - image: moby/buildkit:rootless
+        args:
+        - --addr
+        - tcp://0.0.0.0:1234
+        - --oci-worker-no-process-sandbox
+        name: buildkitd
+        ports:
+        - containerPort: 1234
+```
+
+See [Disabling process sandbox](#disabling-process-sandbox) for security notice.
diff --git a/executor/containerdexecutor/executor.go b/executor/containerdexecutor/executor.go
@@ -120,7 +120,8 @@ func (w containerdExecutor) Exec(ctx context.Context, meta executor.Meta, root c
 		}
 		opts = append(opts, containerdoci.WithCgroup(cgroupsPath))
 	}
-	spec, cleanup, err := oci.GenerateSpec(ctx, meta, mounts, id, resolvConf, hostsFile, namespace, opts...)
+	processMode := oci.ProcessSandbox // FIXME(AkihiroSuda)
+	spec, cleanup, err := oci.GenerateSpec(ctx, meta, mounts, id, resolvConf, hostsFile, namespace, processMode, opts...)
 	if err != nil {
 		return err
 	}

diff --git a/executor/oci/mounts.go b/executor/oci/mounts.go
@@ -2,16 +2,19 @@ package oci
 
 import (
 	"context"
+	"path/filepath"
+	"strings"
 
 	specs "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
 )
 
 // MountOpts sets oci spec specific info for mount points
-type MountOpts func([]specs.Mount) []specs.Mount
+type MountOpts func([]specs.Mount) ([]specs.Mount, error)
 
 //GetMounts returns default required for buildkit
 // https://github.com/moby/buildkit/issues/429
-func GetMounts(ctx context.Context, mountOpts ...MountOpts) []specs.Mount {
+func GetMounts(ctx context.Context, mountOpts ...MountOpts) ([]specs.Mount, error) {
 	mounts := []specs.Mount{
 		{
 			Destination: "/proc",
@@ -49,20 +52,66 @@ func GetMounts(ctx context.Context, mountOpts ...MountOpts) []specs.Mount {
 			Options:     []string{"nosuid", "noexec", "nodev", "ro"},
 		},
 	}
+	var err error
 	for _, o := range mountOpts {
-		mounts = o(mounts)
+		mounts, err = o(mounts)
+		if err != nil {
+			return nil, err
+		}
 	}
-	return mounts
+	return mounts, nil
 }
 
-func withROBind(src, dest string) func(m []specs.Mount) []specs.Mount {
-	return func(m []specs.Mount) []specs.Mount {
+func withROBind(src, dest string) func(m []specs.Mount) ([]specs.Mount, error) {
+	return func(m []specs.Mount) ([]specs.Mount, error) {
 		m = append(m, specs.Mount{
 			Destination: dest,
 			Type:        "bind",
 			Source:      src,
 			Options:     []string{"rbind", "ro"},
 		})
-		return m
+		return m, nil
+	}
+}
+
+func hasPrefix(p, prefixDir string) bool {
+	prefixDir = filepath.Clean(prefixDir)
+	if prefixDir == "/" {
+		return true
+	}
+	p = filepath.Clean(p)
+	return p == prefixDir || strings.HasPrefix(p, prefixDir+"/")
+}
+
+func removeMountsWithPrefix(mounts []specs.Mount, prefixDir string) []specs.Mount {
+	var ret []specs.Mount
+	for _, m := range mounts {
+		if !hasPrefix(m.Destination, prefixDir) {
+			ret = append(ret, m)
+		}
+	}
+	return ret
+}
+
+func withProcessMode(processMode ProcessMode) func([]specs.Mount) ([]specs.Mount, error) {
+	return func(m []specs.Mount) ([]specs.Mount, error) {
+		switch processMode {
+		case ProcessSandbox:
+			// keep the default
+		case NoProcessSandbox:
+			m = removeMountsWithPrefix(m, "/proc")
+			procMount := specs.Mount{
+				Destination: "/proc",
+				Type:        "bind",
+				Source:      "/proc",
+				// NOTE: "rbind"+"ro" does not make /proc read-only recursively.
+				// So we keep maskedPath and readonlyPaths (although not mandatory for rootless mode)
+				Options: []string{"rbind"},
+			}
+			m = append([]specs.Mount{procMount}, m...)
+		default:
+			return nil, errors.Errorf("unknown process mode: %v", processMode)
+		}
+		return m, nil
 	}
 }
diff --git a/executor/oci/mounts_test.go b/executor/oci/mounts_test.go
@@ -0,0 +1,56 @@
+package oci
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestHasPrefix(t *testing.T) {
+	type testCase struct {
+		path     string
+		prefix   string
+		expected bool
+	}
+	testCases := []testCase{
+		{
+			path:     "/foo/bar",
+			prefix:   "/foo",
+			expected: true,
+		},
+		{
+			path:     "/foo/bar",
+			prefix:   "/foo/",
+			expected: true,
+		},
+		{
+			path:     "/foo/bar",
+			prefix:   "/",
+			expected: true,
+		},
+		{
+			path:     "/foo",
+			prefix:   "/foo",
+			expected: true,
+		},
+		{
+			path:     "/foo/bar",
+			prefix:   "/bar",
+			expected: false,
+		},
+		{
+			path:     "/foo/bar",
+			prefix:   "foo",
+			expected: false,
+		},
+		{
+			path:     "/foobar",
+			prefix:   "/foo",
+			expected: false,
+		},
+	}
+	for i, tc := range testCases {
+		actual := hasPrefix(tc.path, tc.prefix)
+		require.Equal(t, tc.expected, actual, "#%d: under(%q,%q)", i, tc.path, tc.prefix)
+	}
+}