Skip to content

Commit d8df79d

Browse files
committed
fix(supervisor-network): resolve symlink chains at the SYMLOOP_MAX limit
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
1 parent be490de commit d8df79d

1 file changed

Lines changed: 107 additions & 26 deletions

File tree

  • crates/openshell-supervisor-network/src

crates/openshell-supervisor-network/src/opa.rs

Lines changed: 107 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1624,19 +1624,6 @@ fn normalize_l7_rule_aliases(
16241624
}
16251625
}
16261626

1627-
/// Resolve a policy binary path through the container's root filesystem.
1628-
///
1629-
/// On Linux, `/proc/<pid>/root/` provides access to the container's mount
1630-
/// namespace. If the policy path is a symlink inside the container
1631-
/// (e.g., `/usr/bin/python3` → `/usr/bin/python3.11`), returns the
1632-
/// canonical target path. Returns `None` if:
1633-
/// - Not on Linux
1634-
/// - `entrypoint_pid` is 0 (container not yet started)
1635-
/// - Path contains glob characters
1636-
/// - Path is not a symlink
1637-
/// - Resolution fails (binary doesn't exist in container)
1638-
/// - Resolved path equals the original
1639-
///
16401627
/// Normalize a path by resolving `.` and `..` components without touching
16411628
/// the filesystem. Only works correctly for absolute paths.
16421629
#[cfg(any(target_os = "linux", test))]
@@ -1690,6 +1677,23 @@ impl BinaryResolution {
16901677
}
16911678
}
16921679

1680+
/// Resolve a policy binary path through the container's root filesystem.
1681+
///
1682+
/// On Linux, `/proc/<pid>/root/` provides access to the container's mount
1683+
/// namespace. If the policy path is a symlink inside the container
1684+
/// (e.g., `/usr/bin/python3` → `/usr/bin/python3.11`), the canonical target is
1685+
/// returned as [`BinaryResolution::Resolved`]. The outcome is classified as:
1686+
/// - [`BinaryResolution::Literal`] — not on Linux, `entrypoint_pid` is 0
1687+
/// (container not yet started), the path contains glob characters, it is not
1688+
/// a symlink, or the resolved path equals the original.
1689+
/// - [`BinaryResolution::Absent`] — the candidate does not exist under an
1690+
/// otherwise reachable process root (expected; the caller logs it quietly).
1691+
/// - [`BinaryResolution::Inaccessible`] — `/proc/<pid>/root` itself is
1692+
/// unreachable (pid gone or access denied).
1693+
/// - [`BinaryResolution::CandidateInaccessible`] — the process root is
1694+
/// reachable but the candidate path failed for a non-NotFound reason.
1695+
/// - [`BinaryResolution::ChainBroken`] — a component mid symlink-chain failed,
1696+
/// or the chain forms a cycle / exceeds the kernel symlink limit.
16931697
#[cfg(target_os = "linux")]
16941698
fn resolve_binary_in_container(policy_path: &str, entrypoint_pid: u32) -> BinaryResolution {
16951699
if policy_path.contains('*') || entrypoint_pid == 0 {
@@ -1780,14 +1784,26 @@ fn resolve_binary_in_container(policy_path: &str, entrypoint_pid: u32) -> Binary
17801784
}
17811785

17821786
if !reached_target {
1783-
// The cap was exhausted while every component was still a symlink: a
1784-
// cycle such as a -> b -> a. read_link resolves one hop at a time, so
1785-
// the kernel never surfaces ELOOP; without this the current mid-cycle
1786-
// path would be accepted as Resolved/Literal with no warning. Treat it
1787-
// as a broken chain so the caller logs it and matches literally only.
1788-
return BinaryResolution::ChainBroken(
1789-
std::io::Error::from_raw_os_error(libc::ELOOP).kind(),
1790-
);
1787+
// The cap was exhausted while following symlinks. Linux SYMLOOP_MAX is
1788+
// 40, so a chain of exactly 40 symlinks ending at a real file is still
1789+
// valid — after the 40th hop `resolved` may already point at that file.
1790+
// Inspect it once more without following another link: a non-symlink is
1791+
// the valid final target (fall through to the Literal/Resolved logic
1792+
// below), while another symlink (or an error) is a genuine cycle
1793+
// (a -> b -> a) or a chain deeper than the kernel allows. read_link
1794+
// resolves one hop at a time, so the kernel never surfaces ELOOP; treat
1795+
// those as a broken chain so the caller logs it and matches literally.
1796+
let container_path = format!("/proc/{entrypoint_pid}/root{}", resolved.display());
1797+
1798+
match std::fs::symlink_metadata(&container_path) {
1799+
Ok(meta) if !meta.file_type().is_symlink() => {}
1800+
Ok(_) => {
1801+
return BinaryResolution::ChainBroken(
1802+
std::io::Error::from_raw_os_error(libc::ELOOP).kind(),
1803+
);
1804+
}
1805+
Err(e) => return BinaryResolution::ChainBroken(e.kind()),
1806+
}
17911807
}
17921808

17931809
let resolved_str = resolved.to_string_lossy().into_owned();
@@ -7610,16 +7626,76 @@ network_policies:
76107626

76117627
#[test]
76127628
#[cfg(target_os = "linux")]
7613-
fn absent_candidates_emit_no_warnings() {
7614-
// Regression for #2883: several expected-but-absent compatibility
7615-
// candidates under an accessible process root must not produce a burst
7616-
// of WARN-level noise. Logging now lives at the caller, so drive the
7617-
// real caller (proto_to_opa_data_json) and count WARN events.
7629+
fn symlink_chain_at_limit_resolves_to_target() {
7630+
// Linux SYMLOOP_MAX is 40: a chain of exactly 40 symlinks ending at a
7631+
// real file resolves successfully in the kernel. The manual walk must
7632+
// follow all 40 hops and accept the final target rather than exhausting
7633+
// the cap and reporting ChainBroken (off-by-one regression).
7634+
use std::os::unix::fs::symlink;
7635+
76187636
if !procfs_root_accessible() {
76197637
eprintln!("Skipping: /proc/<pid>/root/ not accessible in this environment");
76207638
return;
76217639
}
76227640

7641+
let dir = tempfile::tempdir().unwrap();
7642+
let target = dir.path().join("real");
7643+
std::fs::write(&target, b"").unwrap();
7644+
7645+
// link0 -> link1 -> ... -> link39 -> real (40 symlinks, absolute
7646+
// targets so the resolver takes the is_absolute() branch).
7647+
let link = |i: usize| dir.path().join(format!("link{i}"));
7648+
symlink(&target, link(39)).unwrap();
7649+
for i in (0..39).rev() {
7650+
symlink(link(i + 1), link(i)).unwrap();
7651+
}
7652+
7653+
let pid = std::process::id();
7654+
let result = resolve_binary_in_container(link(0).to_str().unwrap(), pid);
7655+
assert!(
7656+
matches!(result, BinaryResolution::Resolved(_)),
7657+
"a 40-link chain ending at a real file must resolve, got {result:?}"
7658+
);
7659+
}
7660+
7661+
#[test]
7662+
#[cfg(target_os = "linux")]
7663+
fn symlink_chain_over_limit_is_chain_broken() {
7664+
// A chain of 41 symlinks exceeds SYMLOOP_MAX: the walk must give up and
7665+
// classify as ChainBroken, proving the 40-hop budget stays enforced.
7666+
use std::os::unix::fs::symlink;
7667+
7668+
if !procfs_root_accessible() {
7669+
eprintln!("Skipping: /proc/<pid>/root/ not accessible in this environment");
7670+
return;
7671+
}
7672+
7673+
let dir = tempfile::tempdir().unwrap();
7674+
let target = dir.path().join("real");
7675+
std::fs::write(&target, b"").unwrap();
7676+
7677+
// link0 -> ... -> link40 -> real (41 symlinks).
7678+
let link = |i: usize| dir.path().join(format!("link{i}"));
7679+
symlink(&target, link(40)).unwrap();
7680+
for i in (0..40).rev() {
7681+
symlink(link(i + 1), link(i)).unwrap();
7682+
}
7683+
7684+
let pid = std::process::id();
7685+
let result = resolve_binary_in_container(link(0).to_str().unwrap(), pid);
7686+
assert!(
7687+
matches!(result, BinaryResolution::ChainBroken(_)),
7688+
"a 41-link chain must classify as ChainBroken, got {result:?}"
7689+
);
7690+
}
7691+
7692+
#[test]
7693+
#[cfg(target_os = "linux")]
7694+
fn absent_candidates_emit_no_warnings() {
7695+
// Regression for #2883: several expected-but-absent compatibility
7696+
// candidates under an accessible process root must not produce a burst
7697+
// of WARN-level noise. Logging now lives at the caller, so drive the
7698+
// real caller (proto_to_opa_data_json) and count WARN events.
76237699
use std::sync::Arc;
76247700
use std::sync::atomic::{AtomicUsize, Ordering};
76257701
use tracing::Subscriber;
@@ -7636,6 +7712,11 @@ network_policies:
76367712
}
76377713
}
76387714

7715+
if !procfs_root_accessible() {
7716+
eprintln!("Skipping: /proc/<pid>/root/ not accessible in this environment");
7717+
return;
7718+
}
7719+
76397720
let warns = Arc::new(AtomicUsize::new(0));
76407721
let subscriber = tracing_subscriber::registry().with(WarnCounter(Arc::clone(&warns)));
76417722

0 commit comments

Comments
 (0)