From 9b9c161ff54820dfd9c7caac11e6aed76cca7173 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Fri, 6 Dec 2024 10:16:14 -0500 Subject: [PATCH] Add more checks to the --mount flag parsing logic * Make volumes.GetBindMount(), volumes.GetCacheMount(), and volumes.GetTmpfsMount() return errors when flags which expect arguments are given empty arguments, when flags which don't expect arguments are given arguments, and when the "relabel" flag, which expects an argument, doesn't get one. * Make volumes.GetCacheMount() not treat the "U" flag as affecting bind propagation. * Drop the special-case error message when a caller attempts to use "src" or "source" options in volumes.GetTmpfsMount(), which would already be covered by the general-purpose "unrecognized option" default. Signed-off-by: Nalin Dahyabhai --- internal/volumes/volumes.go | 138 +++++++++++++++++++------------ internal/volumes/volumes_test.go | 68 +++++++++++++++ 2 files changed, 153 insertions(+), 53 deletions(-) create mode 100644 internal/volumes/volumes_test.go diff --git a/internal/volumes/volumes.go b/internal/volumes/volumes.go index 8ad37af670e..53583af6bd7 100644 --- a/internal/volumes/volumes.go +++ b/internal/volumes/volumes.go @@ -47,11 +47,12 @@ const ( ) var ( - errBadMntOption = errors.New("invalid mount option") - errBadOptionArg = errors.New("must provide an argument for option") - errBadVolDest = errors.New("must set volume destination") - errBadVolSrc = errors.New("must set volume source") - errDuplicateDest = errors.New("duplicate mount destination") + errBadMntOption = errors.New("invalid mount option") + errBadOptionArg = errors.New("must provide an argument for option") + errBadOptionNoArg = errors.New("must not provide an argument for option") + errBadVolDest = errors.New("must set volume destination") + errBadVolSrc = errors.New("must set volume source") + errDuplicateDest = errors.New("duplicate mount destination") ) // CacheParent returns a cache parent for --mount=type=cache @@ -144,33 +145,43 @@ func GetBindMount(sys *types.SystemContext, args []string, contextDir string, st argName, argValue, hasArgValue := strings.Cut(val, "=") switch argName { case "type": - // This is already processed + // This is already processed, and should be "bind" continue case "bind-nonrecursive": + if hasArgValue { + return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, "bind") bindNonRecursive = true case "nosuid", "nodev", "noexec": - // TODO: detect duplication of these options. - // (Is this necessary?) + if hasArgValue { + return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, argName) case "rw", "readwrite": + if hasArgValue { + return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, "rw") mountReadability = "rw" case "ro", "readonly": + if hasArgValue { + return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, "ro") mountReadability = "ro" case "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z", "U", "no-dereference": if hasArgValue { - return newMount, "", "", "", fmt.Errorf("%v: %w", val, errBadOptionArg) + return newMount, "", "", "", fmt.Errorf("%v: %w", val, errBadOptionNoArg) } newMount.Options = append(newMount.Options, argName) case "from": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg) } fromImage = argValue case "bind-propagation": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg) } switch argValue { @@ -181,12 +192,12 @@ func GetBindMount(sys *types.SystemContext, args []string, contextDir string, st } newMount.Options = append(newMount.Options, argValue) case "src", "source": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg) } newMount.Source = argValue case "target", "dst", "destination": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg) } targetPath := argValue @@ -199,6 +210,9 @@ func GetBindMount(sys *types.SystemContext, args []string, contextDir string, st } newMount.Destination = targetPath case "relabel": + if !hasArgValue || argValue == "" { + return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg) + } if setRelabel != "" { return newMount, "", "", "", fmt.Errorf("cannot pass 'relabel' option more than once: %w", errBadOptionArg) } @@ -248,6 +262,7 @@ func GetBindMount(sys *types.SystemContext, args []string, contextDir string, st return newMount, "", "", "", err } mountedImage = image.ID() + // unmount the image if we don't end up returning successfully defer func() { if !succeeded { if _, err := store.UnmountImage(mountedImage, false); err != nil { @@ -335,12 +350,10 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag var err error var mode uint64 var buildahLockFilesDir string - var ( - setDest bool - setShared bool - setReadOnly bool - foundSElinuxLabel bool - ) + var setShared bool + setDest := "" + setRelabel := "" + setReadOnly := "" fromStage := "" newMount := specs.Mount{ Type: define.TypeBind, @@ -360,28 +373,44 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag argName, argValue, hasArgValue := strings.Cut(val, "=") switch argName { case "type": - // This is already processed + // This is already processed, and should be "cache" continue - case "nosuid", "nodev", "noexec": - // TODO: detect duplication of these options. - // (Is this necessary?) + case "nosuid", "nodev", "noexec", "U": + if hasArgValue { + return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, argName) case "rw", "readwrite": + if hasArgValue { + return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, "rw") + setReadOnly = "rw" case "readonly", "ro": - // Alias for "ro" + if hasArgValue { + return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, "ro") - setReadOnly = true + setReadOnly = "ro" case "Z", "z": + if hasArgValue { + return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, argName) - foundSElinuxLabel = true - case "shared", "rshared", "private", "rprivate", "slave", "rslave", "U": + setRelabel = argName + case "shared", "rshared", "private", "rprivate", "slave", "rslave": + if hasArgValue { + return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, argName) setShared = true case "sharing": + if !hasArgValue || argValue == "" { + return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) + } sharing = argValue case "bind-propagation": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) } switch argValue { @@ -392,17 +421,17 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag } newMount.Options = append(newMount.Options, argValue) case "id": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) } id = argValue case "from": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) } fromStage = argValue case "target", "dst", "destination": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) } targetPath := argValue @@ -413,14 +442,14 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag return newMount, "", nil, err } newMount.Destination = targetPath - setDest = true + setDest = targetPath case "src", "source": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) } newMount.Source = argValue case "mode": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) } mode, err = strconv.ParseUint(argValue, 8, 32) @@ -428,7 +457,7 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag return newMount, "", nil, fmt.Errorf("unable to parse cache mode: %w", err) } case "uid": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) } uid, err = strconv.Atoi(argValue) @@ -436,7 +465,7 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag return newMount, "", nil, fmt.Errorf("unable to parse cache uid: %w", err) } case "gid": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg) } gid, err = strconv.Atoi(argValue) @@ -450,11 +479,11 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag // If selinux is enabled and no selinux option was configured // default to `z` i.e shared content label. - if !foundSElinuxLabel && (selinux.EnforceMode() != selinux.Disabled) && fromStage == "" { + if setRelabel == "" && (selinux.EnforceMode() != selinux.Disabled) && fromStage == "" { newMount.Options = append(newMount.Options, "z") } - if !setDest { + if setDest == "" { return newMount, "", nil, errBadVolDest } @@ -478,7 +507,7 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag } thisCacheRoot = mountPoint } else { - // we need to create the cache directory on the host if no image is being used + // we need to create the cache directory on the host if no stage is being used // since type is cache and a cache can be reused by consecutive builds // create a common cache directory, which persists on hosts within temp lifecycle @@ -561,8 +590,8 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag newMount.Options = append(newMount.Options, "shared") } - // buildkit parity: cache must writable unless `ro` or `readonly` is configured explicitly - if !setReadOnly { + // buildkit parity: cache must be writable unless `ro` or `readonly` is configured explicitly + if setReadOnly == "" { newMount.Options = append(newMount.Options, "rw") } @@ -727,9 +756,6 @@ func getMounts(ctx *types.SystemContext, store storage.Store, mountLabel string, errInvalidSyntax := errors.New("incorrect mount format: should be --mount type=,[src=,]target=[,options]") - // TODO(vrothberg): the manual parsing can be replaced with a regular expression - // to allow a more robust parsing of the mount format and to give - // precise errors regarding supported format versus supported options. for _, mount := range mounts { tokens := strings.Split(mount, ",") if len(tokens) < 2 { @@ -809,30 +835,36 @@ func GetTmpfsMount(args []string, workDir string) (specs.Mount, error) { argName, argValue, hasArgValue := strings.Cut(val, "=") switch argName { case "type": - // This is already processed + // This is already processed, and should be "tmpfs" continue - case "ro", "nosuid", "nodev", "noexec": + case "nosuid", "nodev", "noexec": + if hasArgValue { + return newMount, fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, argName) - case "readonly": - // Alias for "ro" + case "ro", "readonly": + if hasArgValue { + return newMount, fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } newMount.Options = append(newMount.Options, "ro") case "tmpcopyup": + if hasArgValue { + return newMount, fmt.Errorf("%v: %w", argName, errBadOptionNoArg) + } // the path that is shadowed by the tmpfs mount is recursively copied up to the tmpfs itself. newMount.Options = append(newMount.Options, argName) case "tmpfs-mode": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, fmt.Errorf("%v: %w", argName, errBadOptionArg) } newMount.Options = append(newMount.Options, fmt.Sprintf("mode=%s", argValue)) case "tmpfs-size": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, fmt.Errorf("%v: %w", argName, errBadOptionArg) } newMount.Options = append(newMount.Options, fmt.Sprintf("size=%s", argValue)) - case "src", "source": - return newMount, errors.New("source is not supported with tmpfs mounts") case "target", "dst", "destination": - if !hasArgValue { + if !hasArgValue || argValue == "" { return newMount, fmt.Errorf("%v: %w", argName, errBadOptionArg) } targetPath := argValue diff --git a/internal/volumes/volumes_test.go b/internal/volumes/volumes_test.go new file mode 100644 index 00000000000..ca542fd1ff7 --- /dev/null +++ b/internal/volumes/volumes_test.go @@ -0,0 +1,68 @@ +package volumes + +import ( + "testing" + + "github.com/containers/image/v5/types" + "github.com/containers/storage" + storageTypes "github.com/containers/storage/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetMount(t *testing.T) { + tempDir := t.TempDir() + rootDir := t.TempDir() + runDir := t.TempDir() + + store, err := storage.GetStore(storageTypes.StoreOptions{ + GraphDriverName: "vfs", + GraphRoot: rootDir, + RunRoot: runDir, + }) + require.NoError(t, err) + t.Cleanup(func() { + if _, err := store.Shutdown(true); err != nil { + t.Logf("shutting down temporary store: %v", err) + } + }) + + t.Run("GetBindMount", func(t *testing.T) { + for _, argNeeder := range []string{"from", "bind-propagation", "src", "source", "target", "dst", "destination", "relabel"} { + _, _, _, _, err := GetBindMount(&types.SystemContext{}, []string{argNeeder}, tempDir, store, "", nil, tempDir, tempDir) + assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder) + _, _, _, _, err = GetBindMount(&types.SystemContext{}, []string{argNeeder + "="}, tempDir, store, "", nil, tempDir, tempDir) + assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder) + } + for _, argHater := range []string{"bind-nonrecursive", "nodev", "noexec", "nosuid", "ro", "readonly", "rw", "readwrite", "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z", "U", "no-dereference"} { + _, _, _, _, err := GetBindMount(&types.SystemContext{}, []string{argHater + "=nonce"}, tempDir, store, "", nil, tempDir, tempDir) + assert.ErrorIsf(t, err, errBadOptionNoArg, "option %q is not supposed to have an arg, but wasn't flagged when it tried to supply one", argHater) + } + }) + + t.Run("GetCacheMount", func(t *testing.T) { + for _, argNeeder := range []string{"sharing", "id", "from", "bind-propagation", "src", "source", "target", "dst", "destination", "mode", "uid", "gid"} { + _, _, _, err := GetCacheMount([]string{argNeeder}, nil, tempDir, tempDir) + assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder) + _, _, _, err = GetCacheMount([]string{argNeeder + "="}, nil, tempDir, tempDir) + assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder) + } + for _, argHater := range []string{"nodev", "noexec", "nosuid", "U", "rw", "readwrite", "ro", "readonly", "shared", "Z", "z", "rshared", "private", "rprivate", "slave", "rslave"} { + _, _, _, err := GetCacheMount([]string{argHater + "=nonce"}, nil, tempDir, tempDir) + assert.ErrorIsf(t, err, errBadOptionNoArg, "option %q is not supposed to have an arg, but wasn't flagged when it tried to supply one", argHater) + } + }) + + t.Run("GetTmpfsMount", func(t *testing.T) { + for _, argNeeder := range []string{"tmpfs-mode", "tmpfs-size", "target", "dst", "destination"} { + _, err := GetTmpfsMount([]string{argNeeder}, tempDir) + assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder) + _, err = GetTmpfsMount([]string{argNeeder + "="}, tempDir) + assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder) + } + for _, argHater := range []string{"nodev", "noexec", "nosuid", "ro", "readonly", "tmpcopyup"} { + _, err := GetTmpfsMount([]string{argHater + "=nonce"}, tempDir) + assert.ErrorIsf(t, err, errBadOptionNoArg, "option %q is not supposed to have an arg, but wasn't flagged when it tried to supply one", argHater) + } + }) +}