Skip to content

Commit

Permalink
Merge pull request #5925 from nalind/mount-flags-parsing
Browse files Browse the repository at this point in the history 
Add more checks to the --mount flag parsing logic
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored Jan 21, 2025
2 parents 833420e + 9b9c161 commit eac9331
Show file tree
Hide file tree
Showing 2 changed files with 153 additions and 53 deletions.
138 changes: 85 additions & 53 deletions internal/volumes/volumes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -413,30 +442,30 @@ 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)
if err != nil {
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)
if err != nil {
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)
Expand All @@ -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
}

Expand All @@ -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
Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -727,9 +756,6 @@ func getMounts(ctx *types.SystemContext, store storage.Store, mountLabel string,

errInvalidSyntax := errors.New("incorrect mount format: should be --mount type=<bind|tmpfs>,[src=<host-dir>,]target=<ctr-dir>[,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 {
Expand Down Expand Up @@ -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
Expand Down
68 changes: 68 additions & 0 deletions internal/volumes/volumes_test.go
View file
Original file line number Diff line number Diff line change
@@ -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)
}
})
}

0 comments on commit eac9331

Please sign in to comment.