Cross-platform packager: OCI symlink resolution, rewrite-include, rtld_audit removal#741
Conversation
35d845d to
434eb38
Compare
7f33b78 to
aa7e539
Compare
litebox_packager/src/oci.rs
Outdated
| std::path::Component::ParentDir => { | ||
| result.pop(); | ||
| } | ||
| std::path::Component::CurDir | std::path::Component::RootDir => {} |
There was a problem hiding this comment.
We might need std::path::Component::Prefix(_) => {} for Windows hosts.
jaybosamiya-ms
left a comment
There was a problem hiding this comment.
I haven't done a deep review of this PR, since it does not seem to require an in-depth review afaict. It is also an overlong PR. Nonetheless, I've added a few inline comments.
Also top-level comment: please simplify the PR description to make sure we don't have overlong descriptions showing up in the squashed history.
Also, maybe a better title is just "Cross-platform packager"?
15eb87e to
8ea3756
Compare
…olution, rewrite-include flag Make litebox_packager compile and work on non-Linux hosts (primarily Windows) by: - Remove #![cfg(target_os = "linux")] crate-level gate and the dual-main pattern; gate only the host-mode code path behind cfg(target_os) - Add file_mode() helper with unix/non-unix variants to replace MetadataExt::mode() calls - Extract run_host_mode() behind #[cfg(target_os = "linux")] - Track OCI layer symlinks in-memory instead of creating OS symlinks (Windows requires special privileges for symlinks); materialize them after all layers are extracted via resolve_symlink_in_rootfs() - Add is_unix_absolute(), strip_unix_root(), normalize_path() helpers for cross-platform path handling - Force linux/amd64 platform when pulling OCI images - Normalize path separators to Unix-style in tar entries - Add --rewrite-include CLI flag for dlopen'd libraries - Change Bun executable detection from warning to hard error - Switch tar headers from GNU to UStar format
…tion
Two bugs found during review:
1. Opaque whiteouts (.wh..wh..opq) and regular whiteouts (.wh.<name>)
removed files from disk but did not prune corresponding entries from
the in-memory symlinks vec. This caused materialize_symlinks() to
resurrect deleted symlinks that a later layer intended to remove.
2. resolve_symlink_in_rootfs() could return Some(rootfs) when a
degenerate symlink target with excess .. segments normalized to an
empty path via normalize_path(). rootfs.join("") == rootfs, which
exists as a directory, causing the entire rootfs to be treated as
a resolution target. Guard against empty rel_path at function entry.
- Store Unix permission modes from tar headers in a HashMap during extraction, so permission bits are accurate on non-Unix hosts (Windows) instead of relying on the file_mode() heuristic which returns wrong answers (0o755 for most files). - Build symlink_map once in pull_and_extract and pass through to materialize_symlinks and scan_rootfs (was duplicated in both). - Add lookup_mode() helper that prefers tar header permissions, falls back to file_mode(), defaults to 0o644. - Add existence check for --rewrite-include in finalize_tar (was missing). - Remove redundant --include/--rewrite-include parsing from run_host_mode. - Replace Bun test with rewrite_elf_skips_non_elf_files test. - Add 22 unit tests for normalize_path, is_unix_absolute, strip_unix_root, resolve_symlink_in_rootfs, and lookup_mode. - Add litebox_packager to build_and_test_windows CI job.
… and Windows colon parsing
- Normalize all tar entry paths with normalize_path() to prevent path
traversal via absolute paths, ../ escape, and ./prefix inconsistency
- Normalize hard link source paths for the same protection
- Fix root-level opaque whiteout where Path::starts_with("") matches
all paths, wiping all in-memory symlinks and permissions
- Handle Windows drive letter colons (C:\path) in parse_include so
--include/--rewrite-include work correctly on Windows
- Merge --rewrite-include into --include (auto-detect ELF via magic bytes) - Make --include host-mode-only (conflicts_with oci_image) - Remove file_mode() helper; inline MetadataExt::mode() in Linux-only paths - Simplify lookup_mode() to use OCI permissions map only (no host FS fallback) - Add Component::Prefix(_) to normalize_path for Windows path completeness - Remove redundant path.is_absolute() fallback from is_unix_absolute - Consolidate and prune tests in oci.rs - Fix stale UnsupportedBunExecutable match arm from rebase
aa7e539 to
74b27eb
Compare
|
|
||
| for entry in entries { | ||
| let mut header = Header::new_gnu(); | ||
| let mut header = Header::new_ustar(); |
There was a problem hiding this comment.
Do we need to use USTAR? It does support up to 255-byte path lengths by default.
| // A later layer may override this symlink, so remove any stale | ||
| // entry with the same rel_path. | ||
| symlinks.retain(|s| s.rel_path != path); | ||
| symlinks.push(DeferredSymlink { | ||
| rel_path: path.clone(), | ||
| link_target, | ||
| }); |
There was a problem hiding this comment.
A symlink can be overridden by not only a symlink but also any regular file/directory.
| .map(|m| m.mode()) | ||
| .unwrap_or(0o755) | ||
| }; | ||
| let rewritten = rewrite_elf(&data, &inc.host_path, args.verbose)?; |
There was a problem hiding this comment.
This ignores --no-rewrite.
| use std::os::unix::fs::MetadataExt as _; | ||
| std::fs::metadata(&inc.host_path) | ||
| .map(|m| m.mode()) | ||
| .unwrap_or(0o755) |
There was a problem hiding this comment.
lookup_mode in oci.rs uses 0o644 as a default value.
| .into_owned(); | ||
| // A later layer may override this symlink, so remove any stale | ||
| // entry with the same rel_path. | ||
| symlinks.retain(|s| s.rel_path != path); |
There was a problem hiding this comment.
nits. this linearly scans all symlinks.
jaybosamiya-ms
left a comment
There was a problem hiding this comment.
A couple of minor comments.
Also, reminder for earlier top-level comment #741 (review)
| // Always pull linux/amd64 images regardless of host platform. | ||
| platform_resolver: Some(Box::new(|entries| { | ||
| entries | ||
| .iter() | ||
| .find(|entry| { | ||
| entry.platform.as_ref().is_some_and(|p| { | ||
| p.os == "linux".into() && p.architecture == "amd64".into() | ||
| }) | ||
| }) | ||
| .map(|e| e.digest.clone()) | ||
| })), |
There was a problem hiding this comment.
Shouldn't you pull linux image, but the arch should match the actual host you are running on? Otherwise, running this on an M-series MacBook might be quite surprising, no?
| pub fn scan_rootfs(rootfs: &Path, verbose: bool) -> anyhow::Result<RootfsFileMap> { | ||
| /// `permissions` provides Unix permission modes captured from tar headers | ||
| /// during extraction, so permission bits are accurate on non-Unix hosts. | ||
| #[allow(clippy::implicit_hasher)] |
There was a problem hiding this comment.
Nit: prefer expect over allow
Summary
Make
litebox_packagercompile and work on non-Linux hosts (primarily Windows) for the OCI image packaging path.#![cfg(target_os = "linux")]gate; gate only ldd-based host mode behind#[cfg(target_os = "linux")]file_mode()helper (MetadataExt::mode()on Unix, permission-based heuristic elsewhere)DeferredSymlink) instead of creating OS symlinks (which require special privileges on Windows). Resolve symlink chains through an in-memory map and materialize as file copies or directory placeholders--rewrite-includeCLI flag for dlopen'd libraries (e.g., NSS modules) that aren't discovered by the automatic dependency scanlinux/amd64images regardless of host platform\to/in tar entry paths for Windows compatibilitybuild.rs(rtld_audit fully removed from packager)Bug fixes (not in feature branch)
.wh..wh..opq) and regular whiteouts (.wh.<name>) removed files from disk but didn't prune corresponding entries from the in-memorysymlinksvec, causingmaterialize_symlinks()to resurrect deleted symlinksnormalize_path()could return an emptyPathBuffor targets with excess..segments, causingresolve_symlink_in_rootfs()to match the rootfs directory itselfStack
Test results