Merge pull request #43978 from cpuguy83/default_runtime_shim

Allow containerd shim refs in default-runtime

daemon/config/config.go

@@ -7,9 +7,11 @@ import (
 	"net"
 	"net/url"
 	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 
+	"github.com/containerd/containerd/runtime/v2/shim"
 	"github.com/docker/docker/opts"
 	"github.com/docker/docker/pkg/authorization"
 	"github.com/docker/docker/registry"
@@ -623,7 +625,7 @@ func Validate(config *Config) error {
 	if defaultRuntime := config.GetDefaultRuntimeName(); defaultRuntime != "" {
 		if !builtinRuntimes[defaultRuntime] {
 			runtimes := config.GetAllRuntimes()
-			if _, ok := runtimes[defaultRuntime]; !ok {
+			if _, ok := runtimes[defaultRuntime]; !ok && !IsPermissibleC8dRuntimeName(defaultRuntime) {
 				return fmt.Errorf("specified default runtime '%s' does not exist", defaultRuntime)
 			}
 		}
@@ -657,3 +659,37 @@ func MaskCredentials(rawURL string) string {
 	parsedURL.User = url.UserPassword("xxxxx", "xxxxx")
 	return parsedURL.String()
 }
+
+// IsPermissibleC8dRuntimeName tests whether name is safe to pass into
+// containerd as a runtime name, and whether the name is well-formed.
+// It does not check if the runtime is installed.
+//
+// A runtime name containing slash characters is interpreted by containerd as
+// the path to a runtime binary. If we allowed this, anyone with Engine API
+// access could get containerd to execute an arbitrary binary as root. Although
+// Engine API access is already equivalent to root on the host, the runtime name
+// has not historically been a vector to run arbitrary code as root so users are
+// not expecting it to become one.
+//
+// This restriction is not configurable. There are viable workarounds for
+// legitimate use cases: administrators and runtime developers can make runtimes
+// available for use with Docker by installing them onto PATH following the
+// [binary naming convention] for containerd Runtime v2.
+//
+// [binary naming convention]: https://github.com/containerd/containerd/blob/main/runtime/v2/README.md#binary-naming
+func IsPermissibleC8dRuntimeName(name string) bool {
+	// containerd uses a rather permissive test to validate runtime names:
+	//
+	//   - Any name for which filepath.IsAbs(name) is interpreted as the absolute
+	//     path to a shim binary. We want to block this behaviour.
+	//   - Any name which contains at least one '.' character and no '/' characters
+	//     and does not begin with a '.' character is a valid runtime name. The shim
+	//     binary name is derived from the final two components of the name and
+	//     searched for on the PATH. The name "a.." is technically valid per
+	//     containerd's implementation: it would resolve to a binary named
+	//     "containerd-shim---".
+	//
+	// https://github.com/containerd/containerd/blob/11ded166c15f92450958078cd13c6d87131ec563/runtime/v2/manager.go#L297-L317
+	// https://github.com/containerd/containerd/blob/11ded166c15f92450958078cd13c6d87131ec563/runtime/v2/shim/util.go#L83-L93
+	return !filepath.IsAbs(name) && !strings.ContainsRune(name, '/') && shim.BinaryName(name) != ""
+}

daemon/config/config_linux_test.go

@@ -142,18 +142,6 @@ func TestUnixValidateConfigurationErrors(t *testing.T) {
 			},
 			expectedErr: `runtime name 'runc' is reserved`,
 		},
-		{
-			doc: `default runtime should be present in runtimes`,
-			config: &Config{
-				Runtimes: map[string]types.Runtime{
-					"foo": {},
-				},
-				CommonConfig: CommonConfig{
-					DefaultRuntime: "bar",
-				},
-			},
-			expectedErr: `specified default runtime 'bar' does not exist`,
-		},
 	}
 	for _, tc := range testCases {
 		tc := tc

daemon/daemon_unix.go

@@ -764,9 +764,11 @@ func verifyDaemonSettings(conf *config.Config) error {
 	configureRuntimes(conf)
 	if rtName := conf.GetDefaultRuntimeName(); rtName != "" {
 		if conf.GetRuntime(rtName) == nil {
+			if !config.IsPermissibleC8dRuntimeName(rtName) {
 				return fmt.Errorf("specified default runtime '%s' does not exist", rtName)
 			}
 		}
+	}
 	return nil
 }
 

daemon/info_unix.go

@@ -45,15 +45,16 @@ func (daemon *Daemon) fillPlatformInfo(v *types.Info, sysInfo *sysinfo.SysInfo)
 	v.ContainerdCommit.ID = "N/A"
 	v.InitCommit.ID = "N/A"
 
-	defaultRuntimeBinary := daemon.configStore.GetRuntime(v.DefaultRuntime).Path
-	if rv, err := exec.Command(defaultRuntimeBinary, "--version").Output(); err == nil {
+	if rt := daemon.configStore.GetRuntime(v.DefaultRuntime); rt != nil {
+		if rv, err := exec.Command(rt.Path, "--version").Output(); err == nil {
 			if _, _, commit, err := parseRuntimeVersion(string(rv)); err != nil {
-			logrus.Warnf("failed to parse %s version: %v", defaultRuntimeBinary, err)
+				logrus.Warnf("failed to parse %s version: %v", rt.Path, err)
 			} else {
 				v.RuncCommit.ID = commit
 			}
 		} else {
-		logrus.Warnf("failed to retrieve %s version: %v", defaultRuntimeBinary, err)
+			logrus.Warnf("failed to retrieve %s version: %v", rt.Path, err)
+		}
 	}
 
 	if rv, err := daemon.containerd.Version(context.Background()); err == nil {
@@ -176,10 +177,10 @@ func (daemon *Daemon) fillPlatformVersion(v *types.Version) {
 	}
 
 	defaultRuntime := daemon.configStore.GetDefaultRuntimeName()
-	defaultRuntimeBinary := daemon.configStore.GetRuntime(defaultRuntime).Path
-	if rv, err := exec.Command(defaultRuntimeBinary, "--version").Output(); err == nil {
+	if rt := daemon.configStore.GetRuntime(defaultRuntime); rt != nil {
+		if rv, err := exec.Command(rt.Path, "--version").Output(); err == nil {
 			if _, ver, commit, err := parseRuntimeVersion(string(rv)); err != nil {
-			logrus.Warnf("failed to parse %s version: %v", defaultRuntimeBinary, err)
+				logrus.Warnf("failed to parse %s version: %v", rt.Path, err)
 			} else {
 				v.Components = append(v.Components, types.ComponentVersion{
 					Name:    defaultRuntime,
@@ -190,7 +191,8 @@ func (daemon *Daemon) fillPlatformVersion(v *types.Version) {
 				})
 			}
 		} else {
-		logrus.Warnf("failed to retrieve %s version: %v", defaultRuntimeBinary, err)
+			logrus.Warnf("failed to retrieve %s version: %v", rt.Path, err)
+		}
 	}
 
 	defaultInitBinary := daemon.configStore.GetInitPath()

daemon/runtime_unix.go

@@ -11,7 +11,6 @@ import (
 	"strings"
 
 	v2runcoptions "github.com/containerd/containerd/runtime/v2/runc/options"
-	"github.com/containerd/containerd/runtime/v2/shim"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/daemon/config"
 	"github.com/docker/docker/errdefs"
@@ -117,7 +116,7 @@ func (daemon *Daemon) rewriteRuntimePath(name, p string, args []string) (string,
 func (daemon *Daemon) getRuntime(name string) (*types.Runtime, error) {
 	rt := daemon.configStore.GetRuntime(name)
 	if rt == nil {
-		if !isPermissibleC8dRuntimeName(name) {
+		if !config.IsPermissibleC8dRuntimeName(name) {
 			return nil, errdefs.InvalidParameter(errors.Errorf("unknown or invalid runtime name: %s", name))
 		}
 		return &types.Runtime{Shim: &types.ShimConfig{Binary: name}}, nil
@@ -138,37 +137,3 @@ func (daemon *Daemon) getRuntime(name string) (*types.Runtime, error) {
 
 	return rt, nil
 }
-
-// isPermissibleC8dRuntimeName tests whether name is safe to pass into
-// containerd as a runtime name, and whether the name is well-formed.
-// It does not check if the runtime is installed.
-//
-// A runtime name containing slash characters is interpreted by containerd as
-// the path to a runtime binary. If we allowed this, anyone with Engine API
-// access could get containerd to execute an arbitrary binary as root. Although
-// Engine API access is already equivalent to root on the host, the runtime name
-// has not historically been a vector to run arbitrary code as root so users are
-// not expecting it to become one.
-//
-// This restriction is not configurable. There are viable workarounds for
-// legitimate use cases: administrators and runtime developers can make runtimes
-// available for use with Docker by installing them onto PATH following the
-// [binary naming convention] for containerd Runtime v2.
-//
-// [binary naming convention]: https://github.com/containerd/containerd/blob/main/runtime/v2/README.md#binary-naming
-func isPermissibleC8dRuntimeName(name string) bool {
-	// containerd uses a rather permissive test to validate runtime names:
-	//
-	//   - Any name for which filepath.IsAbs(name) is interpreted as the absolute
-	//     path to a shim binary. We want to block this behaviour.
-	//   - Any name which contains at least one '.' character and no '/' characters
-	//     and does not begin with a '.' character is a valid runtime name. The shim
-	//     binary name is derived from the final two components of the name and
-	//     searched for on the PATH. The name "a.." is technically valid per
-	//     containerd's implementation: it would resolve to a binary named
-	//     "containerd-shim---".
-	//
-	// https://github.com/containerd/containerd/blob/11ded166c15f92450958078cd13c6d87131ec563/runtime/v2/manager.go#L297-L317
-	// https://github.com/containerd/containerd/blob/11ded166c15f92450958078cd13c6d87131ec563/runtime/v2/shim/util.go#L83-L93
-	return !filepath.IsAbs(name) && !strings.ContainsRune(name, '/') && shim.BinaryName(name) != ""
-}

integration/container/run_linux_test.go

@@ -270,4 +270,24 @@ func TestRunWithAlternativeContainerdShim(t *testing.T) {
 	assert.NilError(t, err)
 
 	assert.Equal(t, strings.TrimSpace(b.String()), "Hello, world!")
+
+	d.Stop(t)
+	d.Start(t, "--default-runtime="+"io.containerd.realfake.v42")
+
+	cID = container.Run(ctx, t, client,
+		container.WithImage("busybox"),
+		container.WithCmd("sh", "-c", `echo 'Hello, world!'`),
+	)
+
+	poll.WaitOn(t, container.IsStopped(ctx, client, cID), poll.WithDelay(100*time.Millisecond))
+
+	out, err = client.ContainerLogs(ctx, cID, types.ContainerLogsOptions{ShowStdout: true})
+	assert.NilError(t, err)
+	defer out.Close()
+
+	b.Reset()
+	_, err = stdcopy.StdCopy(&b, io.Discard, out)
+	assert.NilError(t, err)
+
+	assert.Equal(t, strings.TrimSpace(b.String()), "Hello, world!")
 }