From d7679480f630dbcccc3cbf25c36c4dbd36c8f06a Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 29 Jul 2024 08:31:56 -0400 Subject: [PATCH 1/2] chrootarchive(unix): Pass root via fd I'd like to support passing a file descriptor root for the container storage, and not an absolute path. In the bootc codebase (partially a philosophy inherited from ostree) we've heavily invested in fd-relative accesses, primarily because it's common for us to operate in different namespaces/roots, and fd-relative access avoids a lot of possible footguns when dealing with absolute paths. It's also more efficient, avoiding the need for the kernel to traverse full paths a lot. This is just one of a few preparatory changes necessary in making it work to do: `podman --root=/proc/self/fd/3 --runroot=... pull busybox` That was breaking because the fd was being closed when forking the child untar process here. Fix this by switching over to always passing the root via fd on Unix. Signed-off-by: Colin Walters --- pkg/chrootarchive/archive.go | 8 ++- pkg/chrootarchive/archive_darwin.go | 22 ++++++-- pkg/chrootarchive/archive_unix.go | 83 +++++++++++++++++++++------- pkg/chrootarchive/archive_windows.go | 21 ++++++- 4 files changed, 107 insertions(+), 27 deletions(-) diff --git a/pkg/chrootarchive/archive.go b/pkg/chrootarchive/archive.go index 5ff9f6b511..710167d241 100644 --- a/pkg/chrootarchive/archive.go +++ b/pkg/chrootarchive/archive.go @@ -83,6 +83,12 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions } } + destVal, err := newUnpackDestination(root, dest) + if err != nil { + return err + } + defer destVal.Close() + r := tarArchive if decompress { decompressedArchive, err := archive.DecompressStream(tarArchive) @@ -93,7 +99,7 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions r = decompressedArchive } - return invokeUnpack(r, dest, options, root) + return invokeUnpack(r, destVal, options) } // Tar tars the requested path while chrooted to the specified root. diff --git a/pkg/chrootarchive/archive_darwin.go b/pkg/chrootarchive/archive_darwin.go index a0a578d3e7..caf3484932 100644 --- a/pkg/chrootarchive/archive_darwin.go +++ b/pkg/chrootarchive/archive_darwin.go @@ -6,12 +6,26 @@ import ( "github.com/containers/storage/pkg/archive" ) +type unpackDestination struct { + dest string +} + +func (dst *unpackDestination) Close() error { + return nil +} + +// newUnpackDestination is a no-op on this platform +func newUnpackDestination(root, dest string) (*unpackDestination, error) { + return &unpackDestination{ + dest: dest, + }, nil +} + func invokeUnpack(decompressedArchive io.Reader, - dest string, - options *archive.TarOptions, root string, + dest *unpackDestination, + options *archive.TarOptions, ) error { - _ = root // Restricting the operation to this root is not implemented on macOS - return archive.Unpack(decompressedArchive, dest, options) + return archive.Unpack(decompressedArchive, dest.dest, options) } func invokePack(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) { diff --git a/pkg/chrootarchive/archive_unix.go b/pkg/chrootarchive/archive_unix.go index 9907d24c30..6aace28c70 100644 --- a/pkg/chrootarchive/archive_unix.go +++ b/pkg/chrootarchive/archive_unix.go @@ -9,15 +9,38 @@ import ( "flag" "fmt" "io" + "io/fs" "os" "path/filepath" "runtime" "strings" + "golang.org/x/sys/unix" + "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/reexec" ) +type unpackDestination struct { + root *os.File + dest string +} + +func (dst *unpackDestination) Close() error { + return dst.root.Close() +} + +// rootFileDescriptor is passed as an extra file +const rootFileDescriptor = 4 + +// procPathForFd gives us a string for a descriptor. +// Note that while Linux supports actually *reading* this +// path, FreeBSD and other platforms don't; but in this codebase +// we only compare strings. +func procPathForFd(fd int) string { + return fmt.Sprintf("/proc/self/fd/%d", fd) +} + // untar is the entry-point for storage-untar on re-exec. This is not used on // Windows as it does not support chroot, hence no point sandboxing through // chroot and rexec. @@ -38,7 +61,17 @@ func untar() { root = flag.Arg(1) } - if root == "" { + // FreeBSD doesn't have proc/self, but we can handle it here + if root == procPathForFd(rootFileDescriptor) { + // Take ownership to ensure it's closed; no need to leak + // this afterwards. + rootFd := os.NewFile(rootFileDescriptor, "tar-root") + defer rootFd.Close() + if err := unix.Fchdir(int(rootFd.Fd())); err != nil { + fatal(err) + } + root = "." + } else if root == "" { root = dst } @@ -57,11 +90,35 @@ func untar() { os.Exit(0) } -func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.TarOptions, root string) error { +// newUnpackDestination takes a root directory and a destination which +// must be underneath it, and returns an object that can unpack +// in the target root using a file descriptor. +func newUnpackDestination(root, dest string) (*unpackDestination, error) { if root == "" { - return errors.New("must specify a root to chroot to") + return nil, errors.New("must specify a root to chroot to") + } + relDest, err := filepath.Rel(root, dest) + if err != nil { + return nil, err } + if relDest == "." { + relDest = "/" + } + if relDest[0] != '/' { + relDest = "/" + relDest + } + + rootfdRaw, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return nil, &fs.PathError{Op: "open", Path: root, Err: err} + } + return &unpackDestination{ + root: os.NewFile(uintptr(rootfdRaw), "rootfs"), + dest: relDest, + }, nil +} +func invokeUnpack(decompressedArchive io.Reader, dest *unpackDestination, options *archive.TarOptions) error { // We can't pass a potentially large exclude list directly via cmd line // because we easily overrun the kernel's max argument/environment size // when the full image list is passed (e.g. when this is used by @@ -72,24 +129,12 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T return fmt.Errorf("untar pipe failure: %w", err) } - if root != "" { - relDest, err := filepath.Rel(root, dest) - if err != nil { - return err - } - if relDest == "." { - relDest = "/" - } - if relDest[0] != '/' { - relDest = "/" + relDest - } - dest = relDest - } - - cmd := reexec.Command("storage-untar", dest, root) + cmd := reexec.Command("storage-untar", dest.dest, procPathForFd(rootFileDescriptor)) cmd.Stdin = decompressedArchive - cmd.ExtraFiles = append(cmd.ExtraFiles, r) + cmd.ExtraFiles = append(cmd.ExtraFiles, r) // fd 3 + // If you change this, change rootFileDescriptor above too + cmd.ExtraFiles = append(cmd.ExtraFiles, dest.root) // fd 4 output := bytes.NewBuffer(nil) cmd.Stdout = output cmd.Stderr = output diff --git a/pkg/chrootarchive/archive_windows.go b/pkg/chrootarchive/archive_windows.go index 7455022040..6611cbade7 100644 --- a/pkg/chrootarchive/archive_windows.go +++ b/pkg/chrootarchive/archive_windows.go @@ -7,19 +7,34 @@ import ( "github.com/containers/storage/pkg/longpath" ) +type unpackDestination struct { + dest string +} + +func (dst *unpackDestination) Close() error { + return nil +} + +// newUnpackDestination is a no-op on this platform +func newUnpackDestination(root, dest string) (*unpackDestination, error) { + return &unpackDestination{ + dest: dest, + }, nil +} + // chroot is not supported by Windows func chroot(path string) error { return nil } func invokeUnpack(decompressedArchive io.Reader, - dest string, - options *archive.TarOptions, root string, + dest *unpackDestination, + options *archive.TarOptions, ) error { // Windows is different to Linux here because Windows does not support // chroot. Hence there is no point sandboxing a chrooted process to // do the unpack. We call inline instead within the daemon process. - return archive.Unpack(decompressedArchive, longpath.AddPrefix(dest), options) + return archive.Unpack(decompressedArchive, longpath.AddPrefix(dest.dest), options) } func invokePack(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) { From d5fab15c83d73629a217f654a6ab68e6dfa563ba Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 31 Jul 2024 15:13:30 -0400 Subject: [PATCH 2/2] chrootarchive: Add a const for tar options fd Minor unrelated cleanup. Signed-off-by: Colin Walters --- pkg/chrootarchive/archive_unix.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/chrootarchive/archive_unix.go b/pkg/chrootarchive/archive_unix.go index 6aace28c70..a20d7eb7b6 100644 --- a/pkg/chrootarchive/archive_unix.go +++ b/pkg/chrootarchive/archive_unix.go @@ -30,6 +30,9 @@ func (dst *unpackDestination) Close() error { return dst.root.Close() } +// tarOptionsDescriptor is passed as an extra file +const tarOptionsDescriptor = 3 + // rootFileDescriptor is passed as an extra file const rootFileDescriptor = 4 @@ -51,7 +54,7 @@ func untar() { var options archive.TarOptions // read the options from the pipe "ExtraFiles" - if err := json.NewDecoder(os.NewFile(3, "options")).Decode(&options); err != nil { + if err := json.NewDecoder(os.NewFile(tarOptionsDescriptor, "options")).Decode(&options); err != nil { fatal(err) } @@ -132,6 +135,7 @@ func invokeUnpack(decompressedArchive io.Reader, dest *unpackDestination, option cmd := reexec.Command("storage-untar", dest.dest, procPathForFd(rootFileDescriptor)) cmd.Stdin = decompressedArchive + // If you change this, change tarOptionsDescriptor above cmd.ExtraFiles = append(cmd.ExtraFiles, r) // fd 3 // If you change this, change rootFileDescriptor above too cmd.ExtraFiles = append(cmd.ExtraFiles, dest.root) // fd 4