From e65d4cac663aa4e720cf14ba1bfd3a8900801976 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 6 Dec 2023 22:54:53 +1100 Subject: [PATCH] specconv: temporarily allow userns path and mapping if they match (This is a cherry-pick of ebcef3e651e61aeee96546301d8db9e92b505ce6.) It turns out that the error added in commit 09822c3da8ad ("configs: disallow ambiguous userns and timens configurations") causes issues with containerd and CRIO because they pass both userns mappings and a userns path. These configurations are broken, but to avoid the regression in this one case, output a warning to tell the user that the configuration is incorrect but we will continue to use it if and only if the configured mappings are identical to the mappings of the provided namespace. Fixes: 09822c3da8ad ("configs: disallow ambiguous userns and timens configurations") Signed-off-by: Aleksa Sarai --- libcontainer/specconv/spec_linux.go | 24 +++++++++++++++++++----- libcontainer/specconv/spec_linux_test.go | 4 ++-- libcontainer/userns/userns_maps_linux.go | 15 +++++++++++++++ 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 0cbf578112d..e0bb528f39a 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -941,11 +941,6 @@ func setupUserNamespace(spec *specs.Spec, config *configs.Config) error { } } if path := config.Namespaces.PathOf(configs.NEWUSER); path != "" { - // We cannot allow uid or gid mappings to be set if we are also asked - // to join a userns. - if config.UidMappings != nil || config.GidMappings != nil { - return errors.New("user namespaces enabled, but both namespace path and mapping specified -- you may only provide one") - } // Cache the current userns mappings in our configuration, so that we // can calculate uid and gid mappings within runc. These mappings are // never used for configuring the container if the path is set. @@ -953,6 +948,25 @@ func setupUserNamespace(spec *specs.Spec, config *configs.Config) error { if err != nil { return fmt.Errorf("failed to cache mappings for userns: %w", err) } + // We cannot allow uid or gid mappings to be set if we are also asked + // to join a userns. + if config.UidMappings != nil || config.GidMappings != nil { + // FIXME: It turns out that containerd and CRIO pass both a userns + // path and the mappings of the namespace in the same config.json. + // Such a configuration is technically not valid, but we used to + // require mappings be specified, and thus users worked around our + // bug -- so we can't regress it at the moment. But we also don't + // want to produce broken behaviour if the mapping doesn't match + // the userns. So (for now) we output a warning if the actual + // userns mappings match the configuration, otherwise we return an + // error. + if !userns.IsSameMapping(uidMap, config.UidMappings) || + !userns.IsSameMapping(gidMap, config.GidMappings) { + return errors.New("user namespaces enabled, but both namespace path and non-matching mapping specified -- you may only provide one") + } + logrus.Warnf("config.json has both a userns path to join and a matching userns mapping specified -- you may only provide one. Future versions of runc may return an error with this configuration, please report a bug on if you see this warning and cannot update your configuration.") + } + config.UidMappings = uidMap config.GidMappings = gidMap logrus.WithFields(logrus.Fields{ diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go index 68e57a08308..f79ea0e3727 100644 --- a/libcontainer/specconv/spec_linux_test.go +++ b/libcontainer/specconv/spec_linux_test.go @@ -639,8 +639,8 @@ func TestUserNamespaceMappingAndPath(t *testing.T) { Spec: spec, }) - if !strings.Contains(err.Error(), "user namespaces enabled, but both namespace path and mapping specified") { - t.Errorf("user namespace with mapping and namespace path should be forbidden") + if !strings.Contains(err.Error(), "both namespace path and non-matching mapping specified") { + t.Errorf("user namespace with path and non-matching mapping should be forbidden, got error %v", err) } } diff --git a/libcontainer/userns/userns_maps_linux.go b/libcontainer/userns/userns_maps_linux.go index d81290de2cb..7a8c2b023b3 100644 --- a/libcontainer/userns/userns_maps_linux.go +++ b/libcontainer/userns/userns_maps_linux.go @@ -169,3 +169,18 @@ func GetUserNamespaceMappings(nsPath string) (uidMap, gidMap []configs.IDMap, er return uidMap, gidMap, nil } + +// IsSameMapping returns whether or not the two id mappings are the same. Note +// that if the order of the mappings is different, or a mapping has been split, +// the mappings will be considered different. +func IsSameMapping(a, b []configs.IDMap) bool { + if len(a) != len(b) { + return false + } + for idx := range a { + if a[idx] != b[idx] { + return false + } + } + return true +}