From 49ab250cf94562a4e82a4ed6110131ee8b8230a6 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 21 Mar 2024 11:06:11 +0100 Subject: [PATCH 1/4] util: rename files to snake case use the same convention used for other files. Signed-off-by: Giuseppe Scrivano --- pkg/util/{mountOpts.go => mount_opts.go} | 0 pkg/util/{mountOpts_linux.go => mount_opts_linux.go} | 0 pkg/util/{mountOpts_other.go => mount_opts_other.go} | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename pkg/util/{mountOpts.go => mount_opts.go} (100%) rename pkg/util/{mountOpts_linux.go => mount_opts_linux.go} (100%) rename pkg/util/{mountOpts_other.go => mount_opts_other.go} (100%) diff --git a/pkg/util/mountOpts.go b/pkg/util/mount_opts.go similarity index 100% rename from pkg/util/mountOpts.go rename to pkg/util/mount_opts.go diff --git a/pkg/util/mountOpts_linux.go b/pkg/util/mount_opts_linux.go similarity index 100% rename from pkg/util/mountOpts_linux.go rename to pkg/util/mount_opts_linux.go diff --git a/pkg/util/mountOpts_other.go b/pkg/util/mount_opts_other.go similarity index 100% rename from pkg/util/mountOpts_other.go rename to pkg/util/mount_opts_other.go From 50d764b0e64ad74f6a7796e7b32ad4e9a8cb4339 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 21 Mar 2024 11:17:36 +0100 Subject: [PATCH 2/4] util: refactor ProcessOptions into an internal function this is needed to add tests for the function without accessing the file system. Signed-off-by: Giuseppe Scrivano --- pkg/util/mount_opts.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/util/mount_opts.go b/pkg/util/mount_opts.go index 2ae1fbbede..ab6dbf9551 100644 --- a/pkg/util/mount_opts.go +++ b/pkg/util/mount_opts.go @@ -22,11 +22,17 @@ type defaultMountOptions struct { nodev bool } +type getDefaultMountOptionsFn func(path string) (defaultMountOptions, error) + // ProcessOptions parses the options for a bind or tmpfs mount and ensures that // they are sensible and follow convention. The isTmpfs variable controls // whether extra, tmpfs-specific options will be allowed. // The sourcePath variable, if not empty, contains a bind mount source. func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string, error) { + return processOptionsInternal(options, isTmpfs, sourcePath, getDefaultMountOptions) +} + +func processOptionsInternal(options []string, isTmpfs bool, sourcePath string, getDefaultMountOptions getDefaultMountOptionsFn) ([]string, error) { var ( foundWrite, foundSize, foundProp, foundMode, foundExec, foundSuid, foundDev, foundCopyUp, foundBind, foundZ, foundU, foundOverlay, foundIdmap, foundCopy, foundNoSwap, foundNoDereference bool ) From 9a13b8f17d164e8939f54816c78fd5ccea1b6070 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 21 Mar 2024 11:27:45 +0100 Subject: [PATCH 3/4] util: add some tests for ProcessOptions Signed-off-by: Giuseppe Scrivano --- pkg/util/utils_test.go | 122 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go index 36670caf17..dbe7822d38 100644 --- a/pkg/util/utils_test.go +++ b/pkg/util/utils_test.go @@ -3,6 +3,7 @@ package util import ( "fmt" "math" + "sort" "testing" "time" @@ -636,3 +637,124 @@ func TestGetRootlessKeepIDMapping(t *testing.T) { assert.Equal(t, test.expectedGID, gid) } } + +func getDefaultMountOptionsNoStat(path string) (defaultMountOptions, error) { + return defaultMountOptions{false, true, true}, nil +} + +func TestProcessOptions(t *testing.T) { + tests := []struct { + name string + options []string + isTmpfs bool + sourcePath string + expected []string + expectErr bool + }{ + { + name: "tmpfs", + options: []string{"rw", "size=512m"}, + isTmpfs: true, + sourcePath: "", + expected: []string{"nodev", "nosuid", "rprivate", "rw", "size=512m", "tmpcopyup"}, + }, + { + name: "duplicate idmap option", + sourcePath: "/path/to/source", + options: []string{"idmap", "idmap"}, + expectErr: true, + }, + { + name: "mode allowed only with tmpfs", + sourcePath: "/path/to/source", + options: []string{"rw", "rbind", "mode=0123"}, + expectErr: true, + }, + { + name: "noswap allowed only with tmpfs", + sourcePath: "/path/to/source", + options: []string{"noswap"}, + expectErr: true, + }, + { + name: "tmpcopyup allowed only with tmpfs", + sourcePath: "/path/to/source", + options: []string{"tmpcopyup"}, + expectErr: true, + }, + { + name: "notmpcopyup allowed only with tmpfs", + sourcePath: "/path/to/source", + options: []string{"notmpcopyup"}, + expectErr: true, + }, + { + name: "z not allowed with tmpfs", + isTmpfs: true, + sourcePath: "/path/to/source", + options: []string{"z"}, + expectErr: true, + }, + { + name: "size allowed only with tmpfs", + sourcePath: "/path/to/source", + options: []string{"size=123456"}, + expectErr: true, + }, + { + name: "conflicting option dev/nodev", + sourcePath: "/path/to/source", + options: []string{"dev", "nodev"}, + expectErr: true, + }, + { + name: "conflicting option suid/nosuid", + sourcePath: "/path/to/source", + options: []string{"suid", "nosuid"}, + expectErr: true, + }, + { + name: "conflicting option exec/noexec", + sourcePath: "/path/to/source", + options: []string{"noexec", "exec"}, + expectErr: true, + }, + { + name: "conflicting option ro/rw", + sourcePath: "/path/to/source", + options: []string{"ro", "rw"}, + expectErr: true, + }, + { + name: "conflicting option bind/rbind", + sourcePath: "/path/to/source", + options: []string{"bind", "rbind"}, + expectErr: true, + }, + { + name: "conflicting option bind/rbind", + sourcePath: "/path/to/source", + options: []string{"bind", "rbind"}, + expectErr: true, + }, + { + name: "default bind mount", + sourcePath: "/path/to/source", + expected: []string{"nodev", "nosuid", "rbind", "rprivate", "rw"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts, err := processOptionsInternal(tt.options, tt.isTmpfs, tt.sourcePath, getDefaultMountOptionsNoStat) + if tt.expectErr { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + sort.Strings(opts) + sort.Strings(tt.expected) + assert.Equal(t, opts, tt.expected) + } + }) + } +} From 4740367330b1e0c0f7bcd65a8c00d67582ddda5d Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 21 Mar 2024 11:31:37 +0100 Subject: [PATCH 4/4] util: use private propagation with bind when the "bind" option is used, do not use the "rprivate" propagation as it would inhibit the effect of "bind", instead default to "private". Closes: https://github.com/containers/podman/issues/22107 Signed-off-by: Giuseppe Scrivano --- pkg/util/mount_opts.go | 13 +++++++++++-- pkg/util/utils_test.go | 6 ++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/pkg/util/mount_opts.go b/pkg/util/mount_opts.go index ab6dbf9551..c9a773093e 100644 --- a/pkg/util/mount_opts.go +++ b/pkg/util/mount_opts.go @@ -37,6 +37,8 @@ func processOptionsInternal(options []string, isTmpfs bool, sourcePath string, g foundWrite, foundSize, foundProp, foundMode, foundExec, foundSuid, foundDev, foundCopyUp, foundBind, foundZ, foundU, foundOverlay, foundIdmap, foundCopy, foundNoSwap, foundNoDereference bool ) + recursiveBind := true + newOptions := make([]string, 0, len(options)) for _, opt := range options { // Some options have parameters - size, mode @@ -159,7 +161,10 @@ func processOptionsInternal(options []string, isTmpfs bool, sourcePath string, g return nil, fmt.Errorf("the 'no-dereference' option can only be set once: %w", ErrDupeMntOption) } foundNoDereference = true - case define.TypeBind, "rbind": + case define.TypeBind: + recursiveBind = false + fallthrough + case "rbind": if isTmpfs { return nil, fmt.Errorf("the 'bind' and 'rbind' options are not allowed with tmpfs mounts: %w", ErrBadMntOption) } @@ -190,7 +195,11 @@ func processOptionsInternal(options []string, isTmpfs bool, sourcePath string, g newOptions = append(newOptions, "rw") } if !foundProp { - newOptions = append(newOptions, "rprivate") + if recursiveBind { + newOptions = append(newOptions, "rprivate") + } else { + newOptions = append(newOptions, "private") + } } defaults, err := getDefaultMountOptions(sourcePath) if err != nil { diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go index dbe7822d38..efc256e484 100644 --- a/pkg/util/utils_test.go +++ b/pkg/util/utils_test.go @@ -742,6 +742,12 @@ func TestProcessOptions(t *testing.T) { sourcePath: "/path/to/source", expected: []string{"nodev", "nosuid", "rbind", "rprivate", "rw"}, }, + { + name: "default bind mount with bind", + sourcePath: "/path/to/source", + options: []string{"bind"}, + expected: []string{"nodev", "nosuid", "bind", "private", "rw"}, + }, } for _, tt := range tests {