Improve comments, minor tidy ups

plugincontainer/compatibility_matrix_test.go

@@ -71,7 +71,7 @@ func skipIfUnsupported(t *testing.T, i matrixInput) {
 	switch {
 	case i.rootlessEngine && i.containerRuntime == runtimeRunc:
 		if i.rootlessUser {
-			t.Skip("runc requires rootlesskit to have DAC_OVERRIDE capability itself, and that's a very powerful capability")
+			t.Skip("runc requires rootlesskit to have DAC_OVERRIDE capability itself, and that undermines being a rootless runtime")
 		} else if i.mlock {
 			t.Skip("TODO: Partially working, but tests not yet reliably and repeatably passing")
 		}
@@ -110,14 +110,15 @@ func runExamplePlugin(t *testing.T, i matrixInput) {
 		Image:    goPluginCounterImage,
 		Tag:      target,
 		Runtime:  i.containerRuntime,
-		GroupAdd: os.Getgid(),
-		Debug:    true,
+		GroupAdd: os.Getegid(),
 		Rootless: i.rootlessEngine && i.rootlessUser,
+		Debug:    true,
 
 		CapIPCLock: i.mlock,
 	}
 	if i.mlock {
 		cfg.Env = append(cfg.Env, "MLOCK=true")
 	}
+
 	exerciseExamplePlugin(t, cfg)
 }

plugincontainer/config.go

@@ -11,25 +11,46 @@ import (
 // Currently only compatible with Linux due to the requirements we have for
 // establishing communication over a unix socket.
 //
-// A temporary directory will be mounted into the container and both the host
-// and the plugin will create unix sockets that need to be writable from the
-// other side. To achieve this, there are broadly 2 runtime options (i.e. not
-// including build-time options):
+// A temporary directory will be mounted into the container, which needs to be
+// writable by the plugin so it can create a unix socket, which in turn needs
+// to be writable from the host. To achieve these 2-way write perimissions,
+// this library implements two different strategies:
 //
 //  1. Set up a uid or gid common to both the host and container processes, and
-//     ensure the unix socket is writable by that shared id. Set GroupAdd in this
-//     config and go-plugin ClientConfig's UnixSocketConfig Group with the same
-//     numeric gid to set up a common group. go-plugin will handle making all
-//     sockets writable by the gid.
-//  2. Use a rootless container runtime, in which case the container process will
-//     be run as the same unpriveleged user as the client.
+//     ensure the unix socket is writable by that shared id.
+//
+//     a) For a shared uid, run as root inside the container to avoid being mapped
+//     to a different uid within the user namespace. No need to set GroupAdd or
+//     Rootless options, but note this is highly inadvisable unless your container
+//     runtime is unprivileged/rootless.
+//
+//     b) For a shared gid, use the same numeric gid for GroupAdd in this config
+//     and go-plugin's ClientConfig.UnixSocketConfig.Group. go-plugin will handle
+//     making all sockets writable by the gid. Not sufficient on its own for
+//     rootless runtimes, as the gid will be mapped to a different actual group
+//     inside the container.
+//
+//  2. If the container runtime and the container itself are both configured to
+//     run as non-root users, it's not possible to set up a shared uid or gid.
+//     In this case, set the Rootless option to enable two changes:
+//
+//     a) Enable the DAC_OVERRIDE capability for the container to allow the
+//     plugin to create a file in the shared directory. Note it is recommended
+//     to limit usage of this functionality to gVisor containers, because other
+//     runtimes will need to be given DAC_OVERRIDE themselves, which undermines
+//     some of the benefit of using a rootless container runtime.
+//
+//     b) Apply a default ACL to the shared directory, allowing the host to
+//     write to any socket files created in it. The socket must be group-
+//     writable for the default ACL to take effect, so GroupAdd must also be
+//     set.
 type Config struct {
 	// GroupAdd sets an additional group that the container should run as. Should
-	// match the UnixSocketConfig Group passed to go-plugin. It should be set if
-	// the container runtime runs as root.
+	// match the UnixSocketConfig Group passed to go-plugin.
 	GroupAdd int
 
-	// Rootless is an alternative to GroupAdd, useful for rootless installs. It
+	// Rootless enables extra steps necessary to make the plugin's Unix socket
+	// writable by both sides when using a rootless container runtime. It
 	// should be set if both the host's container runtime and the container
 	// itself are configured to run as non-privileged users. It requires a file
 	// system that supports POSIX 1e ACLs, which should be available by default

plugincontainer/container_runner.go

@@ -450,7 +450,7 @@ Stderr:
 //
 //  2. Run as non-root within the container fails. The container runs as a
 //     subordinate uid, with the mapping defined by /etc/subuid. e.g. if the host
-//     unprivileged user is 1000(ubuntu), and /etc/subuid has the following entry:
+//     unprivileged user is 1001(ubuntu), and /etc/subuid has the following entry:
 //     ubuntu:100000:65536
 //
 //     Then running as user 1 inside the container will map to user 100000
@@ -466,9 +466,10 @@ Stderr:
 //     To fix the host permissions, we set default permissions on the folder
 //     so any Unix sockets created in it are automatically writable.
 //
-//     To fix the container permissions, we give it the DAC_OVERRIDE capability
+//     To fix the container permissions, we give it the DAC_OVERRIDE capability,
 //     which is normally on by default, and allows the container process to
-//     ignore file system permissions for any files mounted inside the container.
+//     ignore file system permission restrictions. The only bit of the host file
+//     system it has access to though is the empty shared folder.
 //
 //     Similar to mlock and the IPC_LOCK capability, runc requires rootlesskit
 //     (the container's parent process) to have the DAC_OVERRIDE capability
@@ -480,7 +481,7 @@ Stderr:
 // and the host, but the same file permission principles apply.
 func configureDefaultACLsForRootless(hostSocketDir string) error {
 	// Setting default ACLs for the socket folder using unix xattr.
-	a := acl.FromUnix(0o600)
+	a := acl.FromUnix(0o660)
 	a = append(a, acl.Entry{
 		Tag:       acl.TagUser,
 		Qualifier: strconv.Itoa(os.Geteuid()),

plugincontainer/examples/container/Dockerfile

@@ -7,13 +7,13 @@ COPY go-plugin-counter /bin/go-plugin-counter
 
 ENTRYPOINT [ "/bin/go-plugin-counter" ]
 
-FROM docker.mirror.hashicorp.services/alpine as nonroot
+FROM docker.mirror.hashicorp.services/ubuntu as nonroot
 
 COPY go-plugin-counter /bin/go-plugin-counter
 
-RUN apk add libcap && \
-    addgroup -S nonroot && \
-    adduser -S -G nonroot nonroot && \
+RUN apt-get update && apt-get install -y libcap2-bin acl && \
+    addgroup --system nonroot && \
+    adduser --system --ingroup nonroot nonroot && \
     chown -R nonroot:nonroot /bin/go-plugin-counter && \
     cp /bin/go-plugin-counter /bin/go-plugin-counter-mlock && \
     setcap cap_ipc_lock=+ep /bin/go-plugin-counter-mlock

plugincontainer/examples/container/plugin-counter/main.go

@@ -6,6 +6,7 @@ package main
 import (
 	"log"
 	"os"
+	"strconv"
 	"syscall"
 
 	"github.com/hashicorp/go-plugin"
@@ -32,7 +33,7 @@ func (c *Counter) Increment(key string, value int64, storage shared.Storage) (in
 }
 
 func main() {
-	if mlock := os.Getenv("MLOCK"); mlock == "true" || mlock == "1" {
+	if mlock, _ := strconv.ParseBool(os.Getenv("MLOCK")); mlock {
 		err := unix.Mlockall(syscall.MCL_CURRENT | syscall.MCL_FUTURE)
 		if err != nil {
 			log.Fatalf("failed to call unix.Mlockall: %s", err)