address review comments

LICENSE_DEPENDENCIES.md

@@ -14926,6 +14926,36 @@ SOFTWARE.
 ```
 
 
+## github.com/samber/mo
+
+**License:** MIT
+
+```
+MIT License
+
+Copyright (c) 2022 Samuel Berthe
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.
+
+```
+
+
 ## github.com/secure-systems-lab/go-securesystemslib
 
 **License:** MIT

go.mod

@@ -37,6 +37,7 @@ require (
 	github.com/pelletier/go-toml/v2 v2.1.0
 	github.com/pkg/errors v0.9.1
 	github.com/samber/lo v1.38.1
+	github.com/samber/mo v1.8.0
 	github.com/seccomp/libseccomp-golang v0.10.0
 	github.com/shopspring/decimal v1.3.1
 	github.com/sigstore/sigstore v1.7.3

go.sum

@@ -460,6 +460,8 @@ github.com/safchain/ethtool v0.3.0 h1:gimQJpsI6sc1yIqP/y8GYgiXn/NjgvpM0RNoWLVVmP
 github.com/safchain/ethtool v0.3.0/go.mod h1:SA9BwrgyAqNo7M+uaL6IYbxpm5wk3L7Mm6ocLW+CJUs=
 github.com/samber/lo v1.38.1 h1:j2XEAqXKb09Am4ebOg31SpvzUTTs6EN3VfgeLUhPdXM=
 github.com/samber/lo v1.38.1/go.mod h1:+m/ZKRl6ClXCE2Lgf3MsQlWfh4bn1bz6CXEOxnEXnEA=
+github.com/samber/mo v1.8.0 h1:vYjHTfg14JF9tD2NLhpoUsRi9bjyRoYwa4+do0nvbVw=
+github.com/samber/mo v1.8.0/go.mod h1:BfkrCPuYzVG3ZljnZB783WIJIGk1mcZr9c9CPf8tAxs=
 github.com/sebdah/goldie/v2 v2.5.3 h1:9ES/mNN+HNUbNWpVAlrzuZ7jE+Nrczbj8uFRjM7624Y=
 github.com/seccomp/libseccomp-golang v0.10.0 h1:aA4bp+/Zzi0BnWZ2F1wgNBs5gTpm+na2rWM6M9YjLpY=
 github.com/seccomp/libseccomp-golang v0.10.0/go.mod h1:JA8cRccbGaA1s33RQf7Y1+q9gHmZX1yB/z9WDN1C6fg=

internal/pkg/runtime/engine/singularity/cleanup_host_linux.go

@@ -19,7 +19,9 @@ import (
 func (e *EngineOperations) CleanupHost(ctx context.Context) (err error) {
 	if e.EngineConfig.GetImageFuse() {
 		sylog.Infof("Unmounting SIF with FUSE...")
-		squashfs.FUSEUnmount(ctx, e.EngineConfig.GetImage())
+		if err := squashfs.FUSEUnmount(ctx, e.EngineConfig.GetImage()); err != nil {
+			return fmt.Errorf("while unmounting fuse directory: %s: %w", e.EngineConfig.GetImage(), err)
+		}
 
 		if tempDir := e.EngineConfig.GetDeleteTempDir(); tempDir != "" {
 			sylog.Infof("Removing image tempDir %s", tempDir)

internal/pkg/runtime/launcher/native/launcher_linux.go

@@ -1161,7 +1161,6 @@ func squashfuseMount(ctx context.Context, img *imgutil.Image, imageDir string) (
 	if err != nil {
 		return fmt.Errorf("failed to load image: %w", err)
 	}
-	defer func() { _ = f.UnloadContainer() }()
 
 	d, err := f.GetDescriptor(sif.WithPartitionType(sif.PartPrimSys))
 	if err != nil {

internal/pkg/util/fs/fuse/fuse_mount_linux.go

@@ -12,9 +12,12 @@ import (
 	"os/exec"
 	"strings"
 
+	"github.com/samber/lo"
+	"github.com/samber/mo"
 	"github.com/sylabs/singularity/v4/internal/pkg/util/bin"
 	"github.com/sylabs/singularity/v4/pkg/image"
 	"github.com/sylabs/singularity/v4/pkg/sylog"
+	"github.com/sylabs/singularity/v4/pkg/util/maps"
 )
 
 type ImageMount struct {
@@ -45,9 +48,9 @@ type ImageMount struct {
 	// AllowOther is set to true to mount the image with the "allow_other" option.
 	AllowOther bool
 
-	// ExtraMountOpts are options to be passed to the mount command (in the "-o"
+	// ExtraOpts are options to be passed to the mount command (in the "-o"
 	// argument) beyond the ones autogenerated from other ImageMount fields.
-	ExtraMountOpts []string
+	ExtraOpts []string
 }
 
 // Mount mounts an image to a temporary directory. It also verifies that
@@ -129,34 +132,75 @@ func (i *ImageMount) generateCmdArgs() ([]string, error) {
 		}
 	}()
 
+	opts := i.generateMountOpts()
+
+	if len(opts) > 0 {
+		args = append(args, "-o", strings.Join(opts, ","))
+	}
+
+	args = append(args, i.SourcePath)
+	args = append(args, i.mountpoint)
+
+	return args, nil
+}
+
+func (i ImageMount) generateMountOpts() []string {
 	// TODO: Think through what makes sense for file ownership in FUSE-mounted
 	// images, vis a vis id-mappings and user-namespaces.
 	opts := []string{"uid=0", "gid=0"}
+
+	// Create a map of the extra mount options that have been requested, so we
+	// can weed out attempts to overwrite builtin struct fields.
+	extraOptsMap := lo.SliceToMap(i.ExtraOpts, func(s string) (string, mo.Option[string]) {
+		splitted := strings.SplitN(s, "=", 2)
+		if len(splitted) < 2 {
+			return strings.ToLower(s), mo.None[string]()
+		}
+
+		return strings.ToLower(splitted[0]), mo.Some(splitted[1])
+	})
+
+	weedOutMountOpt(&extraOptsMap, "ro")
+	weedOutMountOpt(&extraOptsMap, "rw")
 	if i.Readonly {
 		// Not strictly necessary as will be read-only in assembled overlay,
 		// however this stops any erroneous writes through the stagingDir.
 		opts = append(opts, "ro")
 	}
+
 	// FUSE defaults to nosuid,nodev - attempt to reverse if AllowDev/Setuid requested.
+	weedOutMountOpt(&extraOptsMap, "dev")
 	if i.AllowDev {
 		opts = append(opts, "dev")
 	}
+	weedOutMountOpt(&extraOptsMap, "suid")
 	if i.AllowSetuid {
 		opts = append(opts, "suid")
 	}
+
+	weedOutMountOpt(&extraOptsMap, "allow_other")
 	if i.AllowOther {
 		opts = append(opts, "allow_other")
 	}
-	opts = append(opts, i.ExtraMountOpts...)
 
-	if len(opts) > 0 {
-		args = append(args, "-o", strings.Join(opts, ","))
-	}
+	filteredExtraOpts := lo.MapToSlice(extraOptsMap, rebuildOpt)
+	opts = append(opts, filteredExtraOpts...)
 
-	args = append(args, i.SourcePath)
-	args = append(args, i.mountpoint)
+	return opts
+}
 
-	return args, nil
+func weedOutMountOpt(extraOptsMap *map[string]mo.Option[string], k string) {
+	if maps.HasKey(*extraOptsMap, k) {
+		sylog.Warningf("Passing extra FUSE-mount option %q would override built-in field; ignoring", rebuildOpt(k, (*extraOptsMap)[k]))
+		delete(*extraOptsMap, k)
+	}
+}
+
+func rebuildOpt(k string, v mo.Option[string]) string {
+	if v.IsAbsent() {
+		return k
+	}
+	return k + "=" + v.MustGet()
 }
 
 func (i ImageMount) GetMountPoint() string {

internal/pkg/util/fs/fuse/fuse_mount_linux_test.go

@@ -0,0 +1,117 @@
+// Copyright (c) 2023, Sylabs Inc. All rights reserved.
+// This software is licensed under a 3-clause BSD license. Please consult the
+// LICENSE.md file distributed with the sources of this project regarding your
+// rights to use or distribute this software.
+
+package fuse
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/samber/lo"
+	"github.com/sylabs/singularity/v4/pkg/image"
+)
+
+func TestReadonlyOverride(t *testing.T) {
+	m1 := ImageMount{
+		Type:      image.SQUASHFS,
+		Readonly:  true,
+		ExtraOpts: []string{"rw"},
+	}
+
+	opts := m1.generateMountOpts()
+	if lo.ContainsBy(opts, func(s string) bool {
+		splitted := strings.SplitN(s, "=", 2)
+		return (strings.ToLower(splitted[0]) == "rw")
+	}) {
+		t.Errorf("Failed to weed out 'rw' mount option; opts: %#v", opts)
+	}
+
+	m2 := ImageMount{
+		Type:      image.SQUASHFS,
+		Readonly:  false,
+		ExtraOpts: []string{"ro"},
+	}
+
+	opts = m2.generateMountOpts()
+	if lo.ContainsBy(opts, func(s string) bool {
+		splitted := strings.SplitN(s, "=", 2)
+		return (strings.ToLower(splitted[0]) == "ro")
+	}) {
+		t.Errorf("Failed to weed out 'ro' mount option; opts: %#v", opts)
+	}
+}
+
+func TestDevOverride(t *testing.T) {
+	m := ImageMount{
+		Type:      image.SQUASHFS,
+		AllowDev:  false,
+		ExtraOpts: []string{"dev"},
+	}
+
+	opts := m.generateMountOpts()
+	if lo.ContainsBy(opts, func(s string) bool {
+		splitted := strings.SplitN(s, "=", 2)
+		return (strings.ToLower(splitted[0]) == "dev")
+	}) {
+		t.Errorf("Failed to weed out 'dev' mount option; opts: %#v", opts)
+	}
+}
+
+func TestSetuidOverride(t *testing.T) {
+	m := ImageMount{
+		Type:        image.SQUASHFS,
+		AllowSetuid: false,
+		ExtraOpts:   []string{"suid"},
+	}
+
+	opts := m.generateMountOpts()
+	if lo.ContainsBy(opts, func(s string) bool {
+		splitted := strings.SplitN(s, "=", 2)
+		return (strings.ToLower(splitted[0]) == "suid")
+	}) {
+		t.Errorf("Failed to weed out 'suid' mount option; opts: %#v", opts)
+	}
+}
+
+func TestAllowOtherOverride(t *testing.T) {
+	m := ImageMount{
+		Type:       image.SQUASHFS,
+		AllowOther: false,
+		ExtraOpts:  []string{"allow_other"},
+	}
+
+	opts := m.generateMountOpts()
+	if lo.ContainsBy(opts, func(s string) bool {
+		splitted := strings.SplitN(s, "=", 2)
+		return (strings.ToLower(splitted[0]) == "allow_other")
+	}) {
+		t.Errorf("Failed to weed out 'allow_other' mount option; opts: %#v", opts)
+	}
+}
+
+func TestAllOverridesAtOnce(t *testing.T) {
+	m := ImageMount{
+		Type:        image.SQUASHFS,
+		Readonly:    true,
+		AllowDev:    false,
+		AllowSetuid: false,
+		AllowOther:  false,
+		ExtraOpts:   []string{"suid", "allow_other", "rw", "dev"},
+	}
+
+	opts := m.generateMountOpts()
+	offendingOpts := lo.Filter(opts, func(s string, _ int) bool {
+		splitted := strings.SplitN(s, "=", 2)
+		switch splitted[0] {
+		case "rw", "dev", "suid", "allow_other":
+			return true
+		default:
+			return false
+		}
+	})
+	if len(offendingOpts) > 0 {
+		t.Errorf("Failed to properly filter mount options; opts: %#v (offending options: %#v)", opts, offendingOpts)
+	}
+}

internal/pkg/util/fs/squashfs/squashfs.go

@@ -20,7 +20,7 @@ func FUSEMount(ctx context.Context, offset uint64, path, mountPath string) (*fus
 		Type:       image.SQUASHFS,
 		Readonly:   true,
 		SourcePath: filepath.Clean(path),
-		ExtraMountOpts: []string{
+		ExtraOpts: []string{
 			"ro",
 			fmt.Sprintf("offset=%d", offset),
 			fmt.Sprintf("uid=%d", os.Getuid()),

pkg/util/maps/maps.go

@@ -0,0 +1,12 @@
+// Copyright (c) 2023, Sylabs Inc. All rights reserved.
+// This software is licensed under a 3-clause BSD license. Please consult the
+// LICENSE.md file distributed with the sources of this project regarding your
+// rights to use or distribute this software.
+
+package maps
+
+func HasKey[K comparable, V any](m map[K]V, k K) bool {
+	_, ok := m[k]
+
+	return ok
+}