From 2e13d39546cf1e2710d561589a9b9dfe0ec7b6d5 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Thu, 17 Aug 2023 12:31:21 +0200 Subject: [PATCH] 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 --- integration/pod_userns_linux_test.go | 25 ++++++++++++ pkg/process/init.go | 58 ++++++++++++++++++++++++++++ pkg/process/utils.go | 24 ++++++++++++ 3 files changed, 107 insertions(+) diff --git a/integration/pod_userns_linux_test.go b/integration/pod_userns_linux_test.go index 9553a48cc..0980ec664 100644 --- 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 index 00343f854..13de95a57 100644 --- 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 index 34b1f633d..58f9a21d2 100644 --- 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 +}