Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "libct/validator: Error out on non-abs paths" #3971

Merged
merged 2 commits into from
Aug 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions libcontainer/configs/validate/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/intelrdt"
selinux "github.com/opencontainers/selinux/go-selinux"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
)

Expand All @@ -28,13 +29,22 @@ func Validate(config *configs.Config) error {
sysctl,
intelrdtCheck,
rootlessEUIDCheck,
mounts,
mountsStrict,
}
for _, c := range checks {
if err := c(config); err != nil {
return err
}
}
// Relaxed validation rules for backward compatibility
warns := []check{
mounts, // TODO (runc v1.x.x): make this an error instead of a warning
}
for _, c := range warns {
if err := c(config); err != nil {
logrus.WithError(err).Warn("invalid configuration")
}
}
return nil
}

Expand Down Expand Up @@ -282,19 +292,19 @@ func checkIDMapMounts(config *configs.Config, m *configs.Mount) error {

func mounts(config *configs.Config) error {
for _, m := range config.Mounts {
// We upgraded this to an error in runc 1.2. We might need to
// revert this change if some users haven't still moved to use
// abs paths, in that please move this check inside
// checkIDMapMounts() as we do want to ensure that for idmap
// mounts anyways.
if !filepath.IsAbs(m.Destination) {
return fmt.Errorf("invalid mount %+v: mount destination not absolute", m)
}
}
return nil
}

func mountsStrict(config *configs.Config) error {
for _, m := range config.Mounts {
if err := checkIDMapMounts(config, m); err != nil {
return fmt.Errorf("invalid mount %+v: %w", m, err)
}
}

return nil
}

Expand Down
15 changes: 7 additions & 8 deletions libcontainer/configs/validate/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,10 +360,11 @@ func TestValidateMounts(t *testing.T) {
isErr bool
dest string
}{
{isErr: true, dest: "not/an/abs/path"},
{isErr: true, dest: "./rel/path"},
{isErr: true, dest: "./rel/path"},
{isErr: true, dest: "../../path"},
// TODO (runc v1.x.x): make these relative paths an error. See https://github.com/opencontainers/runc/pull/3004
{isErr: false, dest: "not/an/abs/path"},
{isErr: false, dest: "./rel/path"},
{isErr: false, dest: "./rel/path"},
{isErr: false, dest: "../../path"},

{isErr: false, dest: "/abs/path"},
{isErr: false, dest: "/abs/but/../unclean"},
Expand Down Expand Up @@ -514,8 +515,7 @@ func TestValidateIDMapMounts(t *testing.T) {
},
},
{
name: "idmap mounts without abs dest path",
isErr: true,
name: "idmap mounts without abs dest path",
config: &configs.Config{
UIDMappings: mapping,
GIDMappings: mapping,
Expand All @@ -530,7 +530,6 @@ func TestValidateIDMapMounts(t *testing.T) {
},
},
},

{
name: "simple idmap mount",
config: &configs.Config{
Expand Down Expand Up @@ -571,7 +570,7 @@ func TestValidateIDMapMounts(t *testing.T) {
config := tc.config
config.Rootfs = "/var"

err := mounts(config)
err := mountsStrict(config)
if tc.isErr && err == nil {
t.Error("expected error, got nil")
}
Expand Down
2 changes: 1 addition & 1 deletion libcontainer/rootfs_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error {
if m.srcFD == nil {
return fmt.Errorf("error creating mount %+v: idmapFD is invalid, should point to a valid fd", m)
}
if err := unix.MoveMount(*m.srcFD, "", -1, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil {
if err := unix.MoveMount(*m.srcFD, "", unix.AT_FDCWD, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil {
return fmt.Errorf("error on unix.MoveMount %+v: %w", m, err)
}

Expand Down
8 changes: 8 additions & 0 deletions tests/integration/idmap.bats
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ function teardown() {
[[ "$output" == *"shared"* ]]
}

@test "idmap mount with relative path" {
update_config ' .mounts |= map((select(.source == "source-1/") | .destination = "tmp/mount-1") // .)'

runc run test_debian
[ "$status" -eq 0 ]
[[ "$output" == *"=0=0="* ]]
}

@test "idmap mount with bind mount" {
update_config ' .mounts += [
{
Expand Down
Loading