From f1e265b1388ad68b39baebdfe4bfe16e9cefce58 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Thu, 7 Sep 2023 12:09:09 +0200 Subject: [PATCH] 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 --- cmd/containerd-shim-runc-v2/process/init.go | 70 ----------------- cmd/containerd-shim-runc-v2/process/utils.go | 24 ------ core/runtime/v2/task_manager.go | 79 ++++++++++++++++++++ 3 files changed, 79 insertions(+), 94 deletions(-) diff --git a/cmd/containerd-shim-runc-v2/process/init.go b/cmd/containerd-shim-runc-v2/process/init.go index 47097faa7..ca808bb6e 100644 --- a/cmd/containerd-shim-runc-v2/process/init.go +++ b/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 -} diff --git a/cmd/containerd-shim-runc-v2/process/utils.go b/cmd/containerd-shim-runc-v2/process/utils.go index 5a1c2ffaf..848ce8cc1 100644 --- a/cmd/containerd-shim-runc-v2/process/utils.go +++ b/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 -} diff --git a/core/runtime/v2/task_manager.go b/core/runtime/v2/task_manager.go index 3921053e3..99414b66a 100644 --- a/core/runtime/v2/task_manager.go +++ b/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 +}