chrootarchive: Pass root via fd #2049

cgwalters · 2024-07-29T12:57:14Z

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

Note that as part of doing this, I refactored code here to move the "split root and dest" logic into the caller... there was some logically dead code here because

cgwalters · 2024-07-29T12:58:14Z

See containers/bootc#731 particularly https://github.com/containers/bootc/pull/724/files#diff-30bb05ef4e35724331b84e50edacf12aaf7eb21e3a5576a341d556accb0d1400R75 which is doing a complex dance right now to bridge from our internal fds to the absolute paths this package currently requires.

cgwalters · 2024-07-29T17:27:41Z

Fixed up the cross-compilation issues.

cgwalters · 2024-07-29T17:37:59Z

Actually...is this macos/windows code ever used?

rhatdan · 2024-07-30T15:04:52Z

Most likely not.

mtrmac · 2024-07-30T19:07:57Z

pkg/chrootarchive/archive.go

+	if relDest[0] != '/' {
+		relDest = "/" + relDest
+	}
+	dest = relDest


I didn’t try this and I might well be missing something, but how does this work on non-Linux where root/rootfd are completely ignored?

I don't think we should continue the charade of supporting containers/storage on non linux/freeBSD machines.

I vaguely remember the tar/untar functions being used for non-graph-driver content as well. E.g. Podman’s pkg/machine/ocipull/oci.go.

… and if we were to drop support for other systems, I think the code would have to be modified to fail to compile, rather than being allowed to write to unexpected destinations by neglect.

I didn’t try this and I might well be missing something, but how does this work on non-Linux where root/rootfd are completely ignored?

It doesn't.

I vaguely remember the tar/untar functions being used for non-graph-driver content as well. E.g. Podman’s pkg/machine/ocipull/oci.go.

Hmm, good point, but in glancing slightly more deeply it seems like that...ah yes, notice the archive vs chrootarchive, so it's not using this code I think.

Of course, crossing with other threads you know of, I don't see why the podman-machine disk images are actually stored as container images (including being wrapped in a tarball) that we manually unpack vs directly as an OCI artifact which we fetch and stream directly to disk without an extra copy, which would be inherently more secure too. (Of course, if the podman machine archive today is malicious, you're kind of boned anyways because that machine ends up with write access to your home directory)

on non linux/freeBSD

Of course already needing to abstract over those two is pretty nontrivial...Linux has openat2 but FreeBSD ended up doing things differently with O_RESOLVE_BENEATH etc. The current zstd:chunked code is hardcoded to Linux.

Since it's been at least a whole day since I've mentioned Rust here...in bootc/ostree we've been using https://github.com/bytecodealliance/cap-std/ and it's quite nice and elegant and well designed (I've also been contributing), and has backends for both linux and freebsd. In theory we could fork off a helper process using that to handle untar...it even has a whole "userspace fs resolver fallback" path that is what's used today on MacOS.

I vaguely remember the tar/untar functions being used for non-graph-driver content as well. E.g. Podman’s pkg/machine/ocipull/oci.go.

Hmm, good point, but in glancing slightly more deeply it seems like that...ah yes, notice the archive vs chrootarchive, so it's not using this code I think.

Thanks, that’s an important nuance (while we do need to partially continue to support c/storage on the other systems).

A longer comment in #2051 (comment) , but basically I’m leaning towards the thought that a Linux-only code path is not an improvement.

The untar code is supposed to be secure in any environment; AFAICT the chroot is an additional isolation, not something the callers specifically require because they depend on some chroot-only property.

In theory we could fork off a helper process using that to handle untar...

The way pkg/reexec works here, there are no external helper binaries. In the post-RPM world of people just copying binaries around, that makes deployment easier — probably not for Podman and all its dependencies, but for some of the other lighter-weight callers.

I don't see why the podman-machine disk images are actually stored as container images

At least for the pkg/machine/compression code, that’s not necessarily the case — it’s a very generic decompressor handling things like ZIP, not compatible with traditional container images anyway.

cgwalters · 2024-07-30T21:05:37Z

This triggered a memory for me that we also ship oc image extract...I went to look at what it does, and it forked from moby.

I am not sure if oc image extract is currently load bearing on windows/macos? Not much happening there but it looks like it compiles...

cgwalters · 2024-07-30T22:36:33Z

Tossed up #2051

mtrmac

(Just to have a tool-visible marker that the dest = relDest move is breaking non-Linux systems; let’s continue the discussion in the original thread.)

cgwalters · 2024-07-31T17:54:10Z

OK I reworked this in a way which works on Linux (tested) and FreeBSD (not tested, but in theory), and leaves other platforms unchanged (not tested).

mtrmac

ACK to the principle of the thing.

Consider structuring this as a per-platform data type:

type unpackDestination struct { /* platform-specific */ }
func newUnpackDestination(root, dest string) unpackDestination { … }
func (dest unpackDestination) invokeUnpack(decompressedArchive …, options …)

That would hide the platform-specific rootFd in the Linux code, entirely avoid the unused parameter on non-Linux, and make it easier to close the file descriptor in the caller.

(Arguably, when I added the _ = warning silencers, I probably should have done that already; I was too focused on the warnings to actually think about the code. The addition of a file descriptor makes the case even stronger.)

pkg/chrootarchive/archive_unix.go

pkg/chrootarchive/archive.go

pkg/chrootarchive/archive_unix.go

cgwalters · 2024-07-31T19:20:12Z

A note: If we take this out farther, what would make sense is for c/storage to hold a fd (not an absolute path) for its storage root, then this code would need more refactoring as dest wouldn't necessarily be prepended with the root by default. That'd probably be a distinct new API calling into the shared bits, we just wouldn't need to do the "strip off the root" on the Unix paths in that case.

mtrmac

LGTM otherwise.

pkg/chrootarchive/archive.go

rhatdan · 2024-08-01T15:31:49Z

@giuseppe @nalind PTAL

pkg/chrootarchive/archive_unix.go

giuseppe

a nit, otherwise LGTM

pkg/chrootarchive/archive_unix.go

openshift-ci · 2024-08-06T10:35:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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 <[email protected]>

Minor unrelated cleanup. Signed-off-by: Colin Walters <[email protected]>

rhatdan · 2024-08-12T18:11:23Z

/lgtm

cgwalters force-pushed the unpack-via-fd branch from bcabd2d to 309b451 Compare July 29, 2024 15:50

cgwalters force-pushed the unpack-via-fd branch from 309b451 to f1ad832 Compare July 29, 2024 17:29

cgwalters marked this pull request as draft July 29, 2024 17:30

openshift-ci bot added the do-not-merge/work-in-progress label Jul 29, 2024

mtrmac reviewed Jul 30, 2024

View reviewed changes

mtrmac requested changes Jul 31, 2024

View reviewed changes

cgwalters force-pushed the unpack-via-fd branch from f1ad832 to 0d2975e Compare July 31, 2024 17:53

cgwalters marked this pull request as ready for review July 31, 2024 17:53

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 31, 2024

mtrmac reviewed Jul 31, 2024

View reviewed changes

pkg/chrootarchive/archive_unix.go Show resolved Hide resolved

pkg/chrootarchive/archive.go Outdated Show resolved Hide resolved

pkg/chrootarchive/archive_unix.go Show resolved Hide resolved

cgwalters force-pushed the unpack-via-fd branch from 0d2975e to 0c100ab Compare July 31, 2024 19:16

cgwalters force-pushed the unpack-via-fd branch from 0c100ab to 4d8807c Compare July 31, 2024 19:20

mtrmac reviewed Jul 31, 2024

View reviewed changes

pkg/chrootarchive/archive.go Outdated Show resolved Hide resolved

cgwalters force-pushed the unpack-via-fd branch 2 times, most recently from c28e092 to 692352a Compare July 31, 2024 19:35

mtrmac reviewed Aug 5, 2024

View reviewed changes

pkg/chrootarchive/archive_unix.go Outdated Show resolved Hide resolved

giuseppe approved these changes Aug 6, 2024

View reviewed changes

pkg/chrootarchive/archive_unix.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved label Aug 6, 2024

cgwalters added 2 commits August 12, 2024 09:47

chrootarchive: Add a const for tar options fd

d5fab15

Minor unrelated cleanup. Signed-off-by: Colin Walters <[email protected]>

cgwalters force-pushed the unpack-via-fd branch from 692352a to d5fab15 Compare August 12, 2024 13:47

openshift-ci bot assigned rhatdan Aug 12, 2024

openshift-ci bot added the lgtm label Aug 12, 2024

openshift-merge-bot bot merged commit d1b0a56 into containers:main Aug 12, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chrootarchive: Pass root via fd #2049

chrootarchive: Pass root via fd #2049

cgwalters commented Jul 29, 2024

cgwalters commented Jul 29, 2024

cgwalters commented Jul 29, 2024

cgwalters commented Jul 29, 2024

rhatdan commented Jul 30, 2024

mtrmac Jul 30, 2024 •

edited

Loading

rhatdan Jul 30, 2024

mtrmac Jul 30, 2024

mtrmac Jul 30, 2024

cgwalters Jul 30, 2024

cgwalters Jul 30, 2024 •

edited

Loading

mtrmac Jul 31, 2024 •

edited

Loading

mtrmac Jul 31, 2024

cgwalters commented Jul 30, 2024 •

edited

Loading

cgwalters commented Jul 30, 2024

mtrmac left a comment

cgwalters commented Jul 31, 2024

mtrmac left a comment

cgwalters commented Jul 31, 2024

mtrmac left a comment

rhatdan commented Aug 1, 2024

giuseppe left a comment

openshift-ci bot commented Aug 6, 2024

rhatdan commented Aug 12, 2024

chrootarchive: Pass root via fd #2049

chrootarchive: Pass root via fd #2049

Conversation

cgwalters commented Jul 29, 2024

cgwalters commented Jul 29, 2024

cgwalters commented Jul 29, 2024

cgwalters commented Jul 29, 2024

rhatdan commented Jul 30, 2024

mtrmac Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

rhatdan Jul 30, 2024

Choose a reason for hiding this comment

mtrmac Jul 30, 2024

Choose a reason for hiding this comment

mtrmac Jul 30, 2024

Choose a reason for hiding this comment

cgwalters Jul 30, 2024

Choose a reason for hiding this comment

cgwalters Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

mtrmac Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

mtrmac Jul 31, 2024

Choose a reason for hiding this comment

cgwalters commented Jul 30, 2024 • edited Loading

cgwalters commented Jul 30, 2024

mtrmac left a comment

Choose a reason for hiding this comment

cgwalters commented Jul 31, 2024

mtrmac left a comment

Choose a reason for hiding this comment

cgwalters commented Jul 31, 2024

mtrmac left a comment

Choose a reason for hiding this comment

rhatdan commented Aug 1, 2024

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 6, 2024

rhatdan commented Aug 12, 2024

mtrmac Jul 30, 2024 •

edited

Loading

cgwalters Jul 30, 2024 •

edited

Loading

mtrmac Jul 31, 2024 •

edited

Loading

cgwalters commented Jul 30, 2024 •

edited

Loading