Skip to content

Commit f5fb78f

Browse files
ammarioammario
authored
fix: optimize Linux privilege dropping
- Removed redundant environment variable exports in Linux jail command execution - Replaced `su` → `runuser` → **`setpriv`** for optimal privilege dropping - Environment variables are now only set via Command::env() - Added `scripts/wait-pr-checks.sh` for CI monitoring
1 parent 173d686 commit f5fb78f

3 files changed

Lines changed: 120 additions & 55 deletions

File tree

‎CLAUDE.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,10 @@ The CI workspace is located at `/home/ci/actions-runner/_work/httpjail/httpjail`
9393
# SCP files to/from CI-1
9494
./scripts/ci-scp.sh src/ /tmp/httpjail-docker-run/ # Upload
9595
./scripts/ci-scp.sh root@ci-1:/path/to/file ./ # Download
96+
97+
# Wait for PR checks to pass or fail
98+
./scripts/wait-pr-checks.sh 47 # Monitor PR #47
99+
./scripts/wait-pr-checks.sh 47 coder/httpjail # Specify repo explicitly
96100
```
97101

98102
### Manual Testing on CI

‎scripts/wait-pr-checks.sh‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
#!/bin/bash
2+
# wait-pr-checks.sh - Poll GitHub Actions status for a PR and exit on first failure or when all pass
3+
#
4+
# Usage: ./scripts/wait-pr-checks.sh <pr-number> [repo]
5+
# pr-number: The PR number to check
6+
# repo: Optional repository in format owner/repo (defaults to coder/httpjail)
7+
#
8+
# Exit codes:
9+
# 0 - All checks passed
10+
# 1 - A check failed
11+
# 2 - Invalid arguments
12+
#
13+
# Requires: gh, jq
14+
15+
set -euo pipefail
16+
17+
# Parse arguments
18+
if [ $# -lt 1 ]; then
19+
echo "Usage: $0 <pr-number> [repo]" >&2
20+
echo "Example: $0 47" >&2
21+
echo "Example: $0 47 coder/httpjail" >&2
22+
exit 2
23+
fi
24+
25+
PR_NUMBER="$1"
26+
REPO="${2:-coder/httpjail}"
27+
28+
# Check for required tools
29+
if ! command -v jq &> /dev/null; then
30+
echo "Error: jq is required but not installed" >&2
31+
exit 2
32+
fi
33+
34+
# Colors for output
35+
RED='\033[0;31m'
36+
GREEN='\033[0;32m'
37+
YELLOW='\033[1;33m'
38+
NC='\033[0m' # No Color
39+
40+
echo "Monitoring PR #${PR_NUMBER} in ${REPO}..."
41+
echo "Polling every second. Press Ctrl+C to stop."
42+
echo ""
43+
44+
# Track the last status to avoid duplicate output
45+
last_status=""
46+
47+
while true; do
48+
# Get check status as JSON
49+
if ! json_output=$(gh pr checks "${PR_NUMBER}" --repo "${REPO}" --json name,state,link 2>/dev/null); then
50+
echo -e "${YELLOW}Waiting for checks to start...${NC}"
51+
sleep 1
52+
continue
53+
fi
54+
55+
# Parse JSON to get counts
56+
pending_count=$(echo "$json_output" | jq '[.[] | select(.state == "PENDING" or .state == "IN_PROGRESS" or .state == "QUEUED")] | length')
57+
failed_count=$(echo "$json_output" | jq '[.[] | select(.state == "FAILURE" or .state == "ERROR")] | length')
58+
passed_count=$(echo "$json_output" | jq '[.[] | select(.state == "SUCCESS")] | length')
59+
total_count=$(echo "$json_output" | jq 'length')
60+
61+
# Build status string
62+
current_status="✓ ${passed_count} passed | ⏳ ${pending_count} pending | ✗ ${failed_count} failed"
63+
64+
# Only print if status changed
65+
if [ "$current_status" != "$last_status" ]; then
66+
echo -ne "\r\033[K${current_status}"
67+
last_status="$current_status"
68+
fi
69+
70+
# Check for failures
71+
if [ $failed_count -gt 0 ]; then
72+
echo -e "\n\n${RED}❌ The following check(s) failed:${NC}"
73+
echo "$json_output" | jq -r '.[] | select(.state == "FAILURE" or .state == "ERROR") | " - \(.name)"'
74+
echo -e "\nView details at: https://github.com/${REPO}/pull/${PR_NUMBER}/checks"
75+
exit 1
76+
fi
77+
78+
# Check if all passed
79+
if [ $total_count -gt 0 ] && [ $pending_count -eq 0 ] && [ $failed_count -eq 0 ]; then
80+
echo -e "\n\n${GREEN}✅ All ${passed_count} checks passed!${NC}"
81+
echo -e "PR #${PR_NUMBER} is ready to merge."
82+
exit 0
83+
fi
84+
85+
sleep 1
86+
done

‎src/jail/linux/mod.rs‎

Lines changed: 30 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -623,72 +623,45 @@ impl Jail for LinuxJail {
623623

624624
// Check if we're running as root and should drop privileges
625625
let current_uid = unsafe { libc::getuid() };
626-
let target_user = if current_uid == 0 {
627-
// Running as root - check for SUDO_USER to drop privileges to original user
628-
std::env::var("SUDO_USER").ok()
626+
let drop_privs = if current_uid == 0 {
627+
// Running as root - check for SUDO_UID/SUDO_GID to drop privileges to original user
628+
match (std::env::var("SUDO_UID"), std::env::var("SUDO_GID")) {
629+
(Ok(uid), Ok(gid)) => {
630+
debug!(
631+
"Will drop privileges to uid={} gid={} after entering namespace",
632+
uid, gid
633+
);
634+
Some((uid, gid))
635+
}
636+
_ => {
637+
debug!("Running as root but no SUDO_UID/SUDO_GID found, continuing as root");
638+
None
639+
}
640+
}
629641
} else {
630642
// Not root - no privilege dropping needed
631643
None
632644
};
633645

634-
if let Some(ref user) = target_user {
635-
debug!(
636-
"Will drop to user '{}' (from SUDO_USER) after entering namespace",
637-
user
638-
);
639-
}
640-
641646
// Build command: ip netns exec <namespace> <command>
642-
// If we need to drop privileges, we wrap with su
647+
// If we need to drop privileges, we wrap with setpriv
643648
let mut cmd = Command::new("ip");
644649
cmd.args(["netns", "exec", &self.namespace_name()]);
645650

646-
// When we have environment variables to pass OR need to drop privileges,
647-
// use a shell wrapper to ensure proper environment handling
648-
if target_user.is_some() || !extra_env.is_empty() {
649-
// Build shell command with explicit environment exports
650-
let mut shell_command = String::new();
651-
652-
// Export environment variables explicitly in the shell command
653-
for (key, value) in extra_env {
654-
// Escape the value for shell safety
655-
let escaped_value = value.replace('\'', "'\\''");
656-
shell_command.push_str(&format!("export {}='{}'; ", key, escaped_value));
657-
}
658-
659-
// Add the actual command with proper escaping
660-
shell_command.push_str(
661-
&command
662-
.iter()
663-
.map(|arg| {
664-
// Simple escaping: wrap in single quotes and escape existing single quotes
665-
if arg.contains('\'') {
666-
format!("\"{}\"", arg.replace('"', "\\\""))
667-
} else {
668-
format!("'{}'", arg)
669-
}
670-
})
671-
.collect::<Vec<_>>()
672-
.join(" "),
673-
);
674-
675-
if let Some(user) = target_user {
676-
// Use su to drop privileges to the original user
677-
cmd.arg("su");
678-
cmd.arg("-s"); // Specify shell explicitly
679-
cmd.arg("/bin/sh"); // Use sh for compatibility
680-
cmd.arg("-p"); // Preserve environment
681-
cmd.arg(&user); // Username from SUDO_USER
682-
cmd.arg("-c"); // Execute command
683-
cmd.arg(shell_command);
684-
} else {
685-
// No privilege dropping but need shell for env vars
686-
cmd.arg("sh");
687-
cmd.arg("-c");
688-
cmd.arg(shell_command);
651+
// Handle privilege dropping and command execution
652+
if let Some((uid, gid)) = drop_privs {
653+
// Use setpriv to drop privileges to the original user
654+
// setpriv is lighter than runuser - no PAM, direct execve()
655+
cmd.arg("setpriv");
656+
cmd.arg(format!("--reuid={}", uid)); // Set real and effective UID
657+
cmd.arg(format!("--regid={}", gid)); // Set real and effective GID
658+
cmd.arg("--init-groups"); // Initialize supplementary groups
659+
cmd.arg("--"); // End of options
660+
for arg in command {
661+
cmd.arg(arg);
689662
}
690663
} else {
691-
// No privilege dropping and no env vars, execute directly
664+
// No privilege dropping, execute directly
692665
cmd.arg(&command[0]);
693666
for arg in &command[1..] {
694667
cmd.arg(arg);
@@ -711,6 +684,8 @@ impl Jail for LinuxJail {
711684
cmd.env("SUDO_GID", sudo_gid);
712685
}
713686

687+
debug!("Executing command: {:?}", cmd);
688+
714689
// Note: We do NOT set HTTP_PROXY/HTTPS_PROXY environment variables here.
715690
// The jail uses nftables rules to transparently redirect traffic to the proxy,
716691
// making it work with applications that don't respect proxy environment variables.

0 commit comments

Comments
 (0)