core/runtime: Check shim PluginInfo to enforce idmap support

This commit gets rid of the TODO by moving the check to use the
pluginInfo() infrastructure.

The check is only enforced for shims that return info that can be read
as type runtime.Features. For shims that don't provide that, we just
ignore it, as those shims might not be affected by this.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

cmd/containerd-shim-runc-v2/process/init.go

@@ -21,7 +21,6 @@ package process
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -142,12 +141,6 @@ func (p *Init) Create(ctx context.Context, r *CreateConfig) error {
 		opts.ConsoleSocket = socket
 	}
 
-	// runc ignores silently features it doesn't know about, so for things that this is
-	// problematic let's check if this runc version supports them.
-	if err := p.validateRuncFeatures(ctx, r.Bundle); err != nil {
-		return fmt.Errorf("failed to detect OCI runtime features: %w", err)
-	}
-
 	if err := p.runtime.Create(ctx, r.ID, r.Bundle, opts); err != nil {
 		return p.runtimeError(err, "OCI runtime create failed")
 	}
@@ -181,60 +174,6 @@ func (p *Init) Create(ctx context.Context, r *CreateConfig) error {
 	return nil
 }
 
-func (p *Init) validateRuncFeatures(ctx context.Context, bundle string) error {
-	// TODO: We should remove the logic from here and rebase on #8509.
-	// This way we can avoid the call to readConfig() here and the call to p.runtime.Features()
-	// in validateIDMapMounts().
-	// But that PR is not yet merged nor it is clear if it will be refactored.
-	// Do this contained hack for now.
-	spec, err := readConfig(bundle)
-	if err != nil {
-		return fmt.Errorf("failed to read config: %w", err)
-	}
-
-	if err := p.validateIDMapMounts(ctx, spec); err != nil {
-		return fmt.Errorf("OCI runtime doesn't support idmap mounts: %w", err)
-	}
-
-	return nil
-}
-
-func (p *Init) validateIDMapMounts(ctx context.Context, spec *specs.Spec) error {
-	var used bool
-	for _, m := range spec.Mounts {
-		if m.UIDMappings != nil || m.GIDMappings != nil {
-			used = true
-			break
-		}
-		if sliceContainsStr(m.Options, "idmap") || sliceContainsStr(m.Options, "ridmap") {
-			used = true
-			break
-		}
-	}
-
-	if !used {
-		return nil
-	}
-
-	// From here onwards, we require idmap mounts. So if we fail to check, we return an error.
-	features, err := p.runtime.Features(ctx)
-	if err != nil {
-		// If the features command is not implemented, then runc is too old.
-		return fmt.Errorf("features command failed: %w", err)
-
-	}
-
-	if features.Linux.MountExtensions == nil || features.Linux.MountExtensions.IDMap == nil {
-		return errors.New("missing `mountExtensions.idmap` entry in `features` command")
-	}
-
-	if enabled := features.Linux.MountExtensions.IDMap.Enabled; enabled == nil || !*enabled {
-		return errors.New("idmap mounts not supported")
-	}
-
-	return nil
-}
-
 func (p *Init) openStdin(path string) error {
 	sc, err := fifo.OpenFifo(context.Background(), path, unix.O_WRONLY|unix.O_NONBLOCK, 0)
 	if err != nil {
@@ -556,12 +495,3 @@ func withConditionalIO(c stdio.Stdio) runc.IOOpt {
 		o.OpenStderr = c.Stderr != ""
 	}
 }
-
-func sliceContainsStr(s []string, str string) bool {
-	for _, s := range s {
-		if s == str {
-			return true
-		}
-	}
-	return false
-}

cmd/containerd-shim-runc-v2/process/utils.go

@@ -21,7 +21,6 @@ package process
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -32,7 +31,6 @@ import (
 
 	"github.com/containerd/errdefs"
 	runc "github.com/containerd/go-runc"
-	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"golang.org/x/sys/unix"
 )
 
@@ -41,8 +39,6 @@ const (
 	RuncRoot = "/run/containerd/runc"
 	// InitPidFile name of the file that contains the init pid
 	InitPidFile = "init.pid"
-	// configFile is the name of the runc config file
-	configFile = "config.json"
 )
 
 // safePid is a thread safe wrapper for pid.
@@ -188,23 +184,3 @@ func stateName(v interface{}) string {
 	}
 	panic(fmt.Errorf("invalid state %v", v))
 }
-
-func readConfig(path string) (spec *specs.Spec, err error) {
-	cfg := filepath.Join(path, configFile)
-	f, err := os.Open(cfg)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return nil, fmt.Errorf("JSON specification file %s not found", cfg)
-		}
-		return nil, err
-	}
-	defer f.Close()
-
-	if err = json.NewDecoder(f).Decode(&spec); err != nil {
-		return nil, fmt.Errorf("failed to parse config: %w", err)
-	}
-	if spec == nil {
-		return nil, errors.New("config cannot be null")
-	}
-	return spec, nil
-}

core/runtime/v2/task_manager.go

@@ -19,13 +19,18 @@ package v2
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
+	"slices"
 
 	"github.com/containerd/errdefs"
 	"github.com/containerd/plugin"
 	"github.com/containerd/plugin/registry"
+	"github.com/containerd/typeurl/v2"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/opencontainers/runtime-spec/specs-go/features"
 
 	apitypes "github.com/containerd/containerd/v2/api/types"
 	"github.com/containerd/containerd/v2/core/runtime"
@@ -111,6 +116,12 @@ func (m *TaskManager) Create(ctx context.Context, taskID string, opts runtime.Cr
 		return nil, err
 	}
 
+	// runc ignores silently features it doesn't know about, so for things that this is
+	// problematic let's check if this runc version supports them.
+	if err := m.validateRuntimeFeatures(ctx, opts); err != nil {
+		return nil, fmt.Errorf("failed to validate OCI runtime features: %w", err)
+	}
+
 	t, err := shimTask.Create(ctx, opts)
 	if err != nil {
 		// NOTE: ctx contains required namespace information.
@@ -224,3 +235,71 @@ func (m *TaskManager) PluginInfo(ctx context.Context, request interface{}) (inte
 	}
 	return &info, nil
 }
+
+func (m *TaskManager) validateRuntimeFeatures(ctx context.Context, opts runtime.CreateOpts) error {
+	var spec specs.Spec
+	if err := typeurl.UnmarshalTo(opts.Spec, &spec); err != nil {
+		return fmt.Errorf("unmarshal spec: %w", err)
+	}
+
+	// Only ask for the PluginInfo if idmap mounts are used.
+	if !usesIDMapMounts(spec) {
+		return nil
+	}
+
+	pInfo, err := m.PluginInfo(ctx, &apitypes.RuntimeRequest{RuntimePath: opts.Runtime})
+	if err != nil {
+		return fmt.Errorf("runtime info: %w", err)
+	}
+
+	pluginInfo, ok := pInfo.(*apitypes.RuntimeInfo)
+	if !ok {
+		return fmt.Errorf("invalid runtime info type: %T", pInfo)
+	}
+
+	feat, err := typeurl.UnmarshalAny(pluginInfo.Features)
+	if err != nil {
+		return fmt.Errorf("unmarshal runtime features: %w", err)
+	}
+
+	// runc-compatible runtimes silently ignores features it doesn't know about. But ignoring
+	// our request to use idmap mounts can break permissions in the volume, so let's make sure
+	// it supports it. For more info, see:
+	//	https://github.com/opencontainers/runtime-spec/pull/1219
+	//
+	features, ok := feat.(*features.Features)
+	if !ok {
+		// Leave alone non runc-compatible runtimes that don't provide the features info,
+		// they might not be affected by this.
+		return nil
+	}
+
+	if err := supportsIDMapMounts(features); err != nil {
+		return fmt.Errorf("idmap mounts not supported: %w", err)
+	}
+
+	return nil
+}
+
+func usesIDMapMounts(spec specs.Spec) bool {
+	for _, m := range spec.Mounts {
+		if m.UIDMappings != nil || m.GIDMappings != nil {
+			return true
+		}
+		if slices.Contains(m.Options, "idmap") || slices.Contains(m.Options, "ridmap") {
+			return true
+		}
+
+	}
+	return false
+}
+
+func supportsIDMapMounts(features *features.Features) error {
+	if features.Linux.MountExtensions == nil || features.Linux.MountExtensions.IDMap == nil {
+		return errors.New("missing `mountExtensions.idmap` entry in `features` command")
+	}
+	if enabled := features.Linux.MountExtensions.IDMap.Enabled; enabled == nil || !*enabled {
+		return errors.New("entry `mountExtensions.idmap.Enabled` in `features` command not present or disabled")
+	}
+	return nil
+}