pkg/process: Only use idmap mounts if runc supports it

runc, as mandated by the runtime-spec, ignores unknown fields in the config.json. This is unfortunate for cases where we _must_ enable that feature or fail. For example, if we want to start a container with user namespaces and volumes, using the uidMappings/gidMappings field is needed so the UID/GIDs in the volume don't end up with garbage. However, if we don't fail when runc will ignore these fields (because they are unknown to runc), we will just start a container without using the mappings and the UID/GIDs the container will persist to volumes the hostUID/GID, that can change if the container is re-scheduled by Kubernetes. This will end up in volumes having "garbage" and unmapped UIDs that the container can no longer change. So, let's avoid this entirely by just checking that runc supports idmap mounts if the container we are about to create needs them. Please note that the "runc features" subcommand is only run when we are using idmap mounts. If idmap mounts are not used, the subcommand is not run and therefore this should not affect containers that don't use idmap mounts in any way. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
containerd · Sep 13, 2023 · 2e13d39 · 2e13d39
1 parent fce1b95
commit 2e13d39
Show file tree

Hide file tree

Showing 3 changed files with 107 additions and 0 deletions.
diff --git a/integration/pod_userns_linux_test.go b/integration/pod_userns_linux_test.go
@@ -17,6 +17,8 @@
 package integration
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"os"
 	"os/user"
@@ -27,6 +29,7 @@ import (
 	"time"
 
 	"github.com/containerd/containerd/integration/images"
+	runc "github.com/containerd/go-runc"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	exec "golang.org/x/sys/execabs"
@@ -234,6 +237,9 @@ func TestPodUserNS(t *testing.T) {
 			if test.hostVolumes && !supportsIDMap(volumeHostPath) {
 				t.Skipf("ID mappings are not supported host volume filesystem: %v", volumeHostPath)
 			}
+			if err := supportsRuncIDMap(); err != nil {
+				t.Skipf("OCI runtime doesn't support idmap mounts: %v", err)
+			}
 
 			testPodLogDir := t.TempDir()
 			sandboxOpts := append(test.sandboxOpts, WithPodLogDirectory(testPodLogDir))
@@ -297,3 +303,22 @@ func TestPodUserNS(t *testing.T) {
 		})
 	}
 }
+
+func supportsRuncIDMap() error {
+	var r runc.Runc
+	features, err := r.Features(context.Background())
+	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
+}
diff --git a/pkg/process/init.go b/pkg/process/init.go
@@ -21,6 +21,7 @@ package process
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -140,6 +141,13 @@ func (p *Init) Create(ctx context.Context, r *CreateConfig) error {
 	if socket != nil {
 		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")
 	}
@@ -173,6 +181,56 @@ 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 !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 {

diff --git a/pkg/process/utils.go b/pkg/process/utils.go
@@ -21,6 +21,7 @@ package process
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -31,6 +32,7 @@ import (
 
 	"github.com/containerd/containerd/errdefs"
 	runc "github.com/containerd/go-runc"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"golang.org/x/sys/unix"
 )
 
@@ -39,6 +41,8 @@ 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.
@@ -184,3 +188,23 @@ 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
+}