From 3fb93264c1f88e1f90d50fc5684234e0606a1868 Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Thu, 5 May 2022 14:43:28 -0500 Subject: [PATCH] fix: Pass CleanupHost boolean to starter correctly The experimental `--sif-fuse` code was attempting to pass the boolean that controls whether starter spawns the CleanupHost process, for unmounting the FUSE SIF mount, in the starter config structure. The starter config structure is setup from the Go engine 'prepare' code, which runs in stage1 *after* the point at which CleanupHost is spawned from the starter C code... so the boolean was never true. Unfortunately, all the tests we have were passing, because due to a bad negation in an if statement, the `CleanupHost` process was *always* executed. Switch to using an env var (`CLEANUP_HOST`) to control whether the `CleanupHost` process is spawned by starter. This is the right way to control starter behavior that needs to occur before 'prepare' in stage1. It's the same approach used to control whether the overlay module is loaded via `LOAD_OVERLAY_MODULE`. While we are at it, ensure a `CleanupHost` process is never run in setuid mode. We don't support this yet, and if support is added we need to pay attention to permanent priv drop etc. Add a bit more debug logging so it's easier to see what's going on. Fixes #777 --- CHANGELOG.md | 6 ++++ cmd/internal/cli/actions_linux.go | 6 ++++ cmd/starter/c/include/starter.h | 3 -- cmd/starter/c/starter.c | 29 ++++++++++++------- e2e/actions/actions.go | 25 ++++++++++++++++ .../engine/config/starter/starter_linux.go | 11 ------- .../engine/singularity/prepare_linux.go | 4 --- internal/pkg/util/starter/starter.go | 10 +++++++ 8 files changed, 65 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fb9b1e1c6..6b594f7835 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # SingularityCE Changelog +## Changes Since Last Release + +### Bug Fixes + +- Correctly launch CleanupHost process only when needed in `--sif-fuse` flow. + ## 3.10.0-rc.1 \[2022-05-04\] This is the first release candidate for the upcoming SingularityCE 3.10 release. diff --git a/cmd/internal/cli/actions_linux.go b/cmd/internal/cli/actions_linux.go index 878a5d68b2..889107a6e0 100644 --- a/cmd/internal/cli/actions_linux.go +++ b/cmd/internal/cli/actions_linux.go @@ -791,6 +791,10 @@ func execStarter(cobraCmd *cobra.Command, image string, args []string, name stri } } + if SIFFUSE && !(UserNamespace || insideUserNs) { + sylog.Warningf("--sif-fuse is not supported without user namespace, ignoring.") + } + // setuid workflow set RLIMIT_STACK to its default value, // get the original value to restore it before executing // container process @@ -835,6 +839,7 @@ func execStarter(cobraCmd *cobra.Command, image string, args []string, name stri starter.WithStdout(stdout), starter.WithStderr(stderr), starter.LoadOverlayModule(loadOverlay), + starter.CleanupHost(engineConfig.GetImageFuse()), ) if sylog.GetLevel() != 0 { @@ -866,6 +871,7 @@ func execStarter(cobraCmd *cobra.Command, image string, args []string, name stri cfg, starter.UseSuid(useSuid), starter.LoadOverlayModule(loadOverlay), + starter.CleanupHost(engineConfig.GetImageFuse()), ) sylog.Fatalf("%s", err) } diff --git a/cmd/starter/c/include/starter.h b/cmd/starter/c/include/starter.h index f58d90164a..c8f12161c8 100644 --- a/cmd/starter/c/include/starter.h +++ b/cmd/starter/c/include/starter.h @@ -161,9 +161,6 @@ struct starter { /* bounding capability set will include caps needed by nvidia-container-cli */ bool nvCCLICaps; - - /* is a CLEANUP_HOST process require outside of namespaces for SIF FUSE cleanup */ - bool cleanupHost; }; /* engine configuration */ diff --git a/cmd/starter/c/starter.c b/cmd/starter/c/starter.c index 68b4f3982b..81f1708215 100644 --- a/cmd/starter/c/starter.c +++ b/cmd/starter/c/starter.c @@ -1291,17 +1291,13 @@ __attribute__((constructor)) static void init(void) { /* retrieve engine configuration from environment variables */ read_engine_config(&sconfig->engine); - /* cleanup environment variables */ - cleanenv(); - - /* fix I/O streams to point to /dev/null if they are closed */ - fix_streams(); - - /* set an invalid value for check */ - sconfig->starter.workingDirectoryFd = -1; - // Unpriv host cleanup in calling namespaces for SIF FUSE mount - if ( !sconfig->starter.cleanupHost ) { + if ( getenv("CLEANUP_HOST") != NULL ) { + // FUSE SIF mount isn't supported in setuid flow at present. + // We should never have a CleanupHost process in setuid mode - enforce this. + if ( sconfig->starter.isSuid ) { + fatalf("CleanupHost process requested in setuid mode. Not permitted.\n"); + } debugf("Create socketpair for cleanup communication channel\n"); if ( socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, cleanup_socket) < 0 ) { fatalf("Failed to create communication socket: %s\n", strerror(errno)); @@ -1315,12 +1311,23 @@ __attribute__((constructor)) static void init(void) { verbosef("Spawn CleanupHost\n"); goexecute = CLEANUP_HOST; return; - }else{ + } else { // In master - Close child end of cleanup socket close(cleanup_socket[1]); } + } else { + debugf("CleanupHost not requested\n"); } + /* cleanup environment variables */ + cleanenv(); + + /* fix I/O streams to point to /dev/null if they are closed */ + fix_streams(); + + /* set an invalid value for check */ + sconfig->starter.workingDirectoryFd = -1; + /* save opened file descriptors that won't be closed when stage 1 exits */ master_fds = list_fd(); diff --git a/e2e/actions/actions.go b/e2e/actions/actions.go index 7b8c6c3621..6547c6ddbd 100644 --- a/e2e/actions/actions.go +++ b/e2e/actions/actions.go @@ -2352,6 +2352,30 @@ func (c actionTests) actionSIFFUSE(t *testing.T) { } } +// Verify that the FUSE mounts, and the CleanupHost() process are not seen when +// --sif-fuse should not be in effect. +func (c actionTests) actionNoSIFFUSE(t *testing.T) { + e2e.EnsureImage(t, c.env) + + profiles := []e2e.Profile{e2e.UserProfile, e2e.RootProfile, e2e.FakerootProfile, e2e.UserNamespaceProfile} + + for _, p := range profiles { + c.env.RunSingularity( + t, + e2e.AsSubtest(p.String()), + e2e.WithProfile(p), + e2e.WithCommand("exec"), + e2e.WithGlobalOptions("-d"), + e2e.WithArgs(c.env.ImagePath, "mount"), + e2e.ExpectExit( + 0, + e2e.ExpectError(e2e.UnwantedContainMatch, "squashfuse"), + e2e.ExpectError(e2e.UnwantedContainMatch, "CleanupHost()"), + ), + ) + } +} + func countSquashfuseMounts(t *testing.T) int { count := 0 @@ -2413,5 +2437,6 @@ func E2ETests(env e2e.TestEnv) testhelper.Tests { "compat": c.actionCompat, // test --compat "invalidRemote": np(c.invalidRemote), // GHSA-5mv9-q7fq-9394 "SIFFUSE": np(c.actionSIFFUSE), // test --sif-fuse + "NoSIFFUSE": np(c.actionNoSIFFUSE), // test absence of squashfs and CleanupHost() } } diff --git a/internal/pkg/runtime/engine/config/starter/starter_linux.go b/internal/pkg/runtime/engine/config/starter/starter_linux.go index 355a106a1d..033e8ea5b8 100644 --- a/internal/pkg/runtime/engine/config/starter/starter_linux.go +++ b/internal/pkg/runtime/engine/config/starter/starter_linux.go @@ -200,17 +200,6 @@ func (c *Config) SetAllowSetgroups(allow bool) { } } -// SetCleanupHost sets the flag to tell starter container setup -// to spawn a cleanup process for SIF FUSE mounts early, in the -// original host namespaces. -func (c *Config) SetCleanupHost(enabled bool) { - if enabled { - c.config.starter.cleanupHost = C.true - } else { - c.config.starter.cleanupHost = C.false - } -} - // GetJSONConfig returns pointer to the engine's JSON configuration. // A copy of the original bytes allocated on C heap is returned. func (c *Config) GetJSONConfig() []byte { diff --git a/internal/pkg/runtime/engine/singularity/prepare_linux.go b/internal/pkg/runtime/engine/singularity/prepare_linux.go index 11d5ca8ef8..9c41aeb585 100644 --- a/internal/pkg/runtime/engine/singularity/prepare_linux.go +++ b/internal/pkg/runtime/engine/singularity/prepare_linux.go @@ -195,10 +195,6 @@ func (e *EngineOperations) PrepareConfig(starterConfig *starter.Config) error { if e.EngineConfig.GetNvCCLI() { starterConfig.SetNvCCLICaps(true) } - // If using a SIF FUSE mount, we need to cleanup in host namespaces. - if e.EngineConfig.GetImageFuse() { - starterConfig.SetCleanupHost(true) - } return nil } diff --git a/internal/pkg/util/starter/starter.go b/internal/pkg/util/starter/starter.go index 3af597e15e..73edcf9c9d 100644 --- a/internal/pkg/util/starter/starter.go +++ b/internal/pkg/util/starter/starter.go @@ -74,6 +74,16 @@ func LoadOverlayModule(load bool) CommandOp { } } +// CleanupHost sets CLEANUP_HOST environment variable +// which telsl starter to spawn the unprivileged host cleanup process. +func CleanupHost(spawn bool) CommandOp { + return func(c *Command) { + if spawn { + c.env = append(c.env, "CLEANUP_HOST=1") + } + } +} + // Command a starter command to execute. type Command struct { path string