lib: Pass absolute authfile path when pulling LBIs #1852
Conversation
ostree-ext explicitly handles authfile paths as relative; this works fine for most callers of get_global_authfile, as they only read the returned open file descriptor, and ignore the path. However, pulling logically bound images requires passing the actual authfile path to Podman, so we must resolve the absolute path in this case - otherwise, we see errors like the following: ``` [root@fedora ~]# bootc upgrade layers already present: 69; layers needed: 1 (242.2 MB) Fetched layers: 230.95 MiB in 3 seconds (90.88 MiB/s) Deploying: done (3 seconds) Fetching bound image: quay.io/prometheus/node-exporter:v1.10.2: done (0 seconds) error: Upgrading: Staging: Pulling bound images: Pulling bound images: Failed to pull image: Subprocess failed: ExitStatus(unix_wait_status(32000)) Error: credential file is not accessible: faccessat etc/ostree/auth.json: no such file or directory ``` Since cap_std::fs::Dir intentionally does not expose its filesystem path, we must resort to reconstructing it from a file descriptor. We could do this by inspectingthe file descriptor for `sysroot` and combining that with the relative path returned by get_global_authfile, but since get_global_authfile returns the descriptor of the actual authfile, we can simply read that directly. Signed-off-by: James Forcier <csssuf@csssuf.net>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with pulling logically bound images where a relative authfile path was being passed to Podman. The change correctly resolves the absolute path of the authfile by reading the symbolic link from /proc/self/fd/{fd}. The approach is sound and directly fixes the described problem. I have one suggestion to improve the error handling to provide more context, which will be helpful for debugging.
|
Hm, this is a CI gap for sure in terms of LBIs with auth. #1847 would help here. |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Thanks for finding this bug and trying a patch!
Can you explain if you built and tested this, or was this more of a drive-by?
crates/lib/src/podstorage.rs
Outdated
| let authfile_fd = | ||
| ostree_ext::globals::get_global_authfile(&self.sysroot)?.map(|(_authfile, fd)| fd); | ||
| if let Some(fd) = authfile_fd { | ||
| let authfile_path = std::fs::read_link(format!("/proc/self/fd/{}", fd.as_raw_fd())) |
There was a problem hiding this comment.
I don't think this will work because the fd should have O_CLOEXEC and won't be passed to the child by default.
The general pattern we use here instead is using https://docs.rs/cap-std-ext/latest/cap_std_ext/cmdext/trait.CapStdExtCommandExt.html#tymethod.take_fd_n
In fact we should just make this code identical to the one in containers-image-proxy-rs: https://github.com/bootc-dev/containers-image-proxy-rs/blob/b5e0861ad5065f47eaf9cda0d48da3529cc1bc43/src/imageproxy.rs#L310
There was a problem hiding this comment.
I don't believe we rely on passing it to the child here - we only use the fd in the current process to look up the absolute path through /proc, and just pass the path to the child since podman's --authfile option expects a path.
Using /proc here does feel a little hacky, but as far as I can tell cap_std::fs::Dir is very deliberate in not exposing the underlying path, but open to other suggestions here - would copying the contents as in the imageproxy implementation be preferable to resolving the absolute path this way?
There was a problem hiding this comment.
I pushed a followup commit here, can you review it?
|
I have built and tested it! From an earlier testing iteration with a debug print still in there: |
We should match exactly the logic we use with containers-image-proxy. - If bootc doesn't have auth setup, then we need to not let podman fall back to the defaults - Always pass a copy of the auth in a tempfile so we aren't reliant on absolute paths as we're continually trying to reduce our usage of those. Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters
force-pushed
the
jmf/lbi-authfile-absolute-path
branch
from
040be24 to
37382b6
Compare
|
Since we have two people who wrote code here I think SOP should be for a 3rd to review so passing this over to @ckyrouac for final review/approval |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with pulling logically bound images when an authfile is present. The fix involves creating a temporary authfile and passing its file descriptor to Podman via the REGISTRY_AUTH_FILE environment variable, which correctly handles authentication without issues related to relative paths. The changes are logical and well-implemented. The accompanying test modifications are also appropriate for verifying the fix.
I have a couple of suggestions to simplify the code that creates the temporary authfile by removing an unnecessary BufWriter.
3 participants
ostree-ext explicitly handles authfile paths as relative; this works fine for most callers of get_global_authfile, as they only read the returned open file descriptor, and ignore the path. However, pulling logically bound images requires passing the actual authfile path to Podman, so we must resolve the absolute path in this case - otherwise, we see errors like the following:
Since cap_std::fs::Dir intentionally does not expose its filesystem path, we must resort to reconstructing it from a file descriptor. We could do this by inspectingthe file descriptor for
sysrootand combining that with the relative path returned by get_global_authfile, but since get_global_authfile returns the descriptor of the actual authfile, we can simply read that directly.