Skip to content

Commit 42d37eb

Browse files
committed
test(supervisor): align hot-swap identity regression
Signed-off-by: Taylor Mutch <taylormutch@gmail.com>
1 parent ed858eb commit 42d37eb

1 file changed

Lines changed: 44 additions & 56 deletions

File tree

  • crates/openshell-supervisor-network/src

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

Lines changed: 44 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1503,10 +1503,10 @@ fn collect_ancestor_identities(start_pid: u32, stop_pid: u32) -> Vec<(u32, PathB
15031503
///
15041504
/// This is the identity-resolution block of [`evaluate_opa_tcp`] extracted
15051505
/// into a standalone helper so it can be exercised by Linux-only regression
1506-
/// tests without a full OPA engine. The key invariant under test is that on
1507-
/// a hot-swap of the peer binary, the failure mode is
1508-
/// `"Binary integrity violation"` (from the identity cache) rather than
1509-
/// `"Failed to stat ... (deleted)"` (from the kernel-tainted path).
1506+
/// tests without a full OPA engine. The key hot-swap invariant under test is
1507+
/// that display paths are stripped for policy/logging, while integrity hashing
1508+
/// reads the live executable via `/proc/<pid>/exe` instead of the replacement
1509+
/// file that now exists at the display path.
15101510
#[cfg(target_os = "linux")]
15111511
fn resolve_process_identity(
15121512
entrypoint_pid: u32,
@@ -7973,27 +7973,23 @@ network_policies:
79737973
assert_eq!(resp_str[body_start..].len(), cl);
79747974
}
79757975

7976-
/// End-to-end regression for the `docker cp` hot-swap hazard that
7977-
/// motivated `binary_path()` stripping the kernel's `" (deleted)"`
7978-
/// suffix (PR #844).
7976+
/// End-to-end regression for the `docker cp` hot-swap hazard around
7977+
/// unlinked process executables.
79797978
///
7980-
/// Before the strip, the identity-resolution chain inside
7981-
/// `evaluate_opa_tcp` failed with `"Failed to stat
7982-
/// /opt/openshell/bin/openshell-sandbox (deleted)"` because
7983-
/// `BinaryIdentityCache::verify_or_cache()` tried to `metadata()` the
7984-
/// tainted path. That masked the real security signal: a live process
7985-
/// was now bound to a *different* binary on disk than the one that was
7986-
/// TOFU-cached. After the strip, `binary_path()` returns a path that
7987-
/// stats fine, the cache rehashes the new bytes, and the hash mismatch
7988-
/// surfaces as a `Binary integrity violation` error — the contract this
7989-
/// PR is trying to establish.
7979+
/// `binary_path()` strips the kernel's `" (deleted)"` suffix so policy
7980+
/// identity and logs use a clean display path. Integrity verification must
7981+
/// not hash that display path after a hot-swap, because it may now point to
7982+
/// unrelated replacement bytes. It hashes `/proc/<pid>/exe` instead, which
7983+
/// resolves to the live executable inode even after the original path was
7984+
/// unlinked.
79907985
///
79917986
/// Test shape (from the review comment on the initial PR):
79927987
/// 1. Start a `TcpListener` in the test process.
79937988
/// 2. Copy `/bin/bash` to a temp path we control.
79947989
/// 3. Prime `BinaryIdentityCache` with that temp binary's hash.
79957990
/// 4. Spawn the temp bash as a child with a `/dev/tcp` one-liner that
7996-
/// opens a real TCP connection to the listener and holds it open.
7991+
/// opens a real TCP connection to the listener and holds it open
7992+
/// inside the bash process.
79977993
/// 5. Accept the connection on the listener side and capture the peer's
79987994
/// ephemeral port — that's what `resolve_process_identity` uses to
79997995
/// walk `/proc/net/tcp` back to the child PID.
@@ -8003,13 +7999,12 @@ network_policies:
80037999
/// now readlink to `" (deleted)"` OR the overwritten file, depending
80048000
/// on whether the filesystem reused the inode.
80058001
/// 7. Call `resolve_process_identity` and assert:
8006-
/// - the error reason contains `"Binary integrity violation"` (the
8007-
/// cache detected the tampered on-disk bytes), and
8008-
/// - the error reason does NOT contain `"Failed to stat"` or
8009-
/// `"(deleted)"` (the old pre-strip failure mode).
8002+
/// - identity resolution succeeds using the live executable hash, and
8003+
/// - the returned display path does not contain the kernel-added
8004+
/// `"(deleted)"` suffix.
80108005
#[cfg(target_os = "linux")]
80118006
#[test]
8012-
fn resolve_process_identity_surfaces_binary_integrity_violation_on_hot_swap() {
8007+
fn resolve_process_identity_hashes_live_exe_after_hot_swap() {
80138008
use crate::identity::BinaryIdentityCache;
80148009
use std::io::Read;
80158010
use std::net::TcpListener;
@@ -8041,9 +8036,12 @@ network_policies:
80418036
assert!(!v1_hash.is_empty());
80428037

80438038
// 4. Spawn the temp bash with a /dev/tcp one-liner that opens a real
8044-
// connection to the listener and sleeps to keep it open. The
8045-
// `read -t` blocks on stdin so the shell stays resident.
8046-
let script = format!("exec 3<>/dev/tcp/127.0.0.1/{listener_port}; sleep 30 <&3");
8039+
// connection to the listener and blocks in bash's `read` builtin
8040+
// to keep it open. Do not use an external command like `sleep`:
8041+
// it inherits the socket fd and intentionally trips the shared
8042+
// socket ambiguity guard instead of exercising the hot-swap path.
8043+
let script =
8044+
format!("exec 3<>/dev/tcp/127.0.0.1/{listener_port}; read -r -t 30 _ <&3 || true");
80478045
let mut child = Command::new(&bash_v1)
80488046
.arg("-c")
80498047
.arg(&script)
@@ -8087,51 +8085,41 @@ network_policies:
80878085
std::fs::write(&bash_v1, tampered_bytes).expect("write replacement bytes");
80888086

80898087
// 7. Resolve identity through the real helper and assert the
8090-
// contract: we want "Binary integrity violation", not
8091-
// "Failed to stat ... (deleted)".
8088+
// contract: hash the live executable via /proc/<pid>/exe while
8089+
// returning a clean display path for policy/logging.
80928090
let test_pid = std::process::id();
80938091
let result = resolve_process_identity(test_pid, peer_port, &cache);
8092+
let child_pid = child.id();
80948093

80958094
// Always clean up the child before asserting so a failure doesn't
80968095
// leak a sleeping process across test runs.
80978096
let _ = child.kill();
80988097
let _ = child.wait();
80998098

81008099
match result {
8101-
Ok(_) => panic!(
8102-
"resolve_process_identity unexpectedly succeeded after hot-swap; \
8103-
the cache should have detected the tampered on-disk bytes"
8104-
),
8105-
Err(err) => {
8106-
assert!(
8107-
err.reason.contains("Binary integrity violation"),
8108-
"expected 'Binary integrity violation' error, got: {}",
8109-
err.reason
8100+
Ok(identity) => {
8101+
assert_eq!(
8102+
identity.binary_pid, child_pid,
8103+
"expected the hot-swapped bash child to own the socket"
81108104
);
8111-
assert!(
8112-
!err.reason.contains("Failed to stat"),
8113-
"pre-PR-#844 failure mode leaked: {}",
8114-
err.reason
8105+
assert_eq!(
8106+
identity.bin_path, bash_v1,
8107+
"expected stripped display path to remain the original binary path"
81158108
);
81168109
assert!(
8117-
!err.reason.contains("(deleted)"),
8118-
"resolved path still contains '(deleted)' suffix: {}",
8119-
err.reason
8110+
!identity.bin_path.to_string_lossy().contains("(deleted)"),
8111+
"resolved binary path still tainted: {}",
8112+
identity.bin_path.display()
81208113
);
8121-
// The binary field should be populated — we did resolve a
8122-
// path before failing.
8123-
assert!(
8124-
err.binary.is_some(),
8125-
"expected resolved binary path on integrity failure"
8114+
assert_eq!(
8115+
identity.bin_hash, v1_hash,
8116+
"expected integrity hash from the live executable, not replacement bytes"
81268117
);
8127-
if let Some(path) = &err.binary {
8128-
assert!(
8129-
!path.to_string_lossy().contains("(deleted)"),
8130-
"resolved binary path still tainted: {}",
8131-
path.display()
8132-
);
8133-
}
81348118
}
8119+
Err(err) => panic!(
8120+
"resolve_process_identity failed after hot-swap; expected live-exe identity: {}",
8121+
err.reason
8122+
),
81358123
}
81368124
}
81378125

0 commit comments

Comments
 (0)