diff --git a/Cargo.lock b/Cargo.lock index d8f9d59..85d2a61 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -436,7 +436,7 @@ dependencies = [ [[package]] name = "dosh" -version = "0.1.12" +version = "0.1.13" dependencies = [ "anyhow", "base64", diff --git a/Cargo.toml b/Cargo.toml index cae6fd2..a2fafea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "dosh" -version = "0.1.12" +version = "0.1.13" edition = "2024" license = "MIT" diff --git a/install.sh b/install.sh index b9ff889..9db9f50 100755 --- a/install.sh +++ b/install.sh @@ -429,15 +429,6 @@ ExecStart=$bindir/dosh-server serve Restart=on-failure RestartSec=1 KillMode=process -NoNewPrivileges=true -PrivateTmp=true -ProtectSystem=strict -ProtectHome=read-only -ReadWritePaths=$config_dir $data_dir -RestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK -RestrictRealtime=true -LockPersonality=true -MemoryDenyWriteExecute=true [Install] WantedBy=default.target diff --git a/packaging/systemd/dosh-server.service b/packaging/systemd/dosh-server.service index 3f2ced1..7756f26 100644 --- a/packaging/systemd/dosh-server.service +++ b/packaging/systemd/dosh-server.service @@ -9,15 +9,6 @@ ExecStart=%h/.local/bin/dosh-server serve Restart=on-failure RestartSec=1 KillMode=process -NoNewPrivileges=true -PrivateTmp=true -ProtectSystem=strict -ProtectHome=read-only -ReadWritePaths=%h/.config/dosh %h/.local/share/dosh -RestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK -RestrictRealtime=true -LockPersonality=true -MemoryDenyWriteExecute=true [Install] WantedBy=default.target diff --git a/src/bin/dosh-server.rs b/src/bin/dosh-server.rs index 22c32fa..e7aa86b 100644 --- a/src/bin/dosh-server.rs +++ b/src/bin/dosh-server.rs @@ -512,12 +512,13 @@ impl ServerState { /// Whether a session named `name` should be backed by a persistent holder. /// - /// With persistence enabled, every PTY session gets a detached holder. That - /// includes implicit `term--` sessions: a reconnecting client has - /// the exact session name in its ticket cache, so it can reattach after a - /// quick server restart without forcing users to name sessions manually. - fn should_persist_session(&self, _name: &str) -> bool { - self.config.persist_sessions + /// With persistence enabled, explicitly named sessions get a detached + /// holder. Implicit `term--` sessions are one-off terminals + /// created by `dosh host`; users cannot intentionally reattach to them by + /// name, so persisting them only leaves stale holders that can be + /// accidentally re-adopted after restarts. + fn should_persist_session(&self, name: &str) -> bool { + self.config.persist_sessions && !protocol::is_implicit_session_name(name) } /// Spawn (or, in the persistent case, spawn-and-adopt) a PTY for a session. @@ -605,6 +606,20 @@ impl ServerState { if self.sessions.contains_key(&name) { continue; } + if !self.should_persist_session(&name) { + eprintln!( + "discarding stale implicit persistent session {name} (shell pid {})", + meta.shell_pid + ); + persist::request_shutdown(&sessions_dir, &name, None); + if meta.shell_pid > 0 { + let _ = unsafe { libc::kill(meta.shell_pid, libc::SIGHUP) }; + let _ = unsafe { libc::kill(meta.shell_pid, libc::SIGTERM) }; + std::thread::sleep(Duration::from_millis(50)); + let _ = unsafe { libc::kill(meta.shell_pid, libc::SIGKILL) }; + } + continue; + } let (pty, control) = match self.adopt_persistent_pty(&name) { Ok(pair) => pair, Err(err) => { @@ -3841,7 +3856,7 @@ mod tests { use super::*; #[test] - fn persists_all_sessions_when_enabled() { + fn persists_named_sessions_when_enabled() { let (pty_tx, _rx) = mpsc::unbounded_channel(); let config = ServerConfig { persist_sessions: true, @@ -3851,7 +3866,7 @@ mod tests { let state = ServerState::new(config, [0u8; 32], pty_tx); assert!(state.should_persist_session("default")); // prewarmed assert!(state.should_persist_session("work")); // explicitly named - assert!(state.should_persist_session("term-1781470634216-76685")); // implicit reconnect + assert!(!state.should_persist_session("term-1781470634216-76685")); // one-off terminal } #[test] diff --git a/src/persist.rs b/src/persist.rs index 5af3c8f..25d1d71 100644 --- a/src/persist.rs +++ b/src/persist.rs @@ -435,6 +435,10 @@ pub fn run_holder( let mut cmd = [0u8; 1]; match stream.read(&mut cmd) { Ok(1) if cmd[0] == HOLDER_CMD_SHUTDOWN => { + if shell_pid > 0 { + let _ = unsafe { libc::kill(shell_pid, libc::SIGHUP) }; + let _ = unsafe { libc::kill(shell_pid, libc::SIGTERM) }; + } let _ = std::fs::remove_dir_all(runtime_dir); std::process::exit(0); } diff --git a/tests/integration_smoke.rs b/tests/integration_smoke.rs index d162418..4ba8e72 100644 --- a/tests/integration_smoke.rs +++ b/tests/integration_smoke.rs @@ -2518,6 +2518,9 @@ fn write_persistent_server_config(dir: &tempfile::TempDir, port: u16) -> std::pa let mut raw = fs::read_to_string(&config).unwrap(); // The base writer hardcodes `persist_sessions = false`; flip it on. raw = raw.replace("persist_sessions = false", "persist_sessions = true"); + // Persistence tests attach the sessions they need explicitly. Leaving the + // base prewarm on here creates a detached default holder in every test. + raw = raw.replace("prewarm_sessions = [\"default\"]", "prewarm_sessions = []"); fs::write(&config, raw).unwrap(); config } @@ -2553,6 +2556,45 @@ fn kill_holder(sessions_dir: &Path, session: &str) { .status(); } +/// Create a holder the way older Dosh builds did for implicit sessions, so +/// startup migration can be tested without depending on the new spawn policy. +fn spawn_legacy_holder(sessions_dir: &Path, session: &str) { + let server = env!("CARGO_BIN_EXE_dosh-server"); + let runtime_dir = dosh::persist::ensure_runtime_dir(sessions_dir, session).unwrap(); + let mut launcher = Command::new(server) + .arg("hold") + .arg("--runtime-dir") + .arg(&runtime_dir) + .arg("--session") + .arg(session) + .arg("--shell") + .arg("/bin/sh") + .arg("--cols") + .arg("80") + .arg("--rows") + .arg("24") + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .unwrap(); + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while std::time::Instant::now() < deadline { + if holder_shell_pid(sessions_dir, session).is_some() + && runtime_dir.join("holder.sock").exists() + { + let _ = launcher.wait(); + return; + } + if let Some(status) = launcher.try_wait().unwrap() { + panic!("legacy holder launcher exited before ready: {status}"); + } + thread::sleep(Duration::from_millis(20)); + } + let _ = launcher.kill(); + let _ = launcher.wait(); + panic!("legacy holder did not become ready"); +} + /// Send one encrypted Input line and give the shell a moment to act. fn type_line(socket: &UdpSocket, port: u16, ok: &AttachOk, key: &[u8; 32], seq: u64, line: &str) { let input = Input { @@ -2690,7 +2732,7 @@ fn session_survives_server_restart_same_shell_and_screen() { } #[test] -fn implicit_session_survives_server_restart_for_same_ticket_holder() { +fn implicit_session_does_not_create_restart_holder() { let dir = tempfile::tempdir().unwrap(); let sessions_dir = dir.path().join("sessions"); let port = free_udp_port(); @@ -2713,20 +2755,14 @@ fn implicit_session_survives_server_restart_for_same_ticket_holder() { let shell_pid_before = holder_shell_pid(&sessions_dir, &session); assert!( - shell_pid_before.is_some(), - "implicit sessions should get a persistent holder when persistence is enabled" + shell_pid_before.is_none(), + "implicit sessions must not get persistent holders when persistence is enabled" ); let _ = server.kill(); let _ = server.wait(); thread::sleep(Duration::from_millis(300)); - let pid = shell_pid_before.unwrap(); - assert!( - unsafe { libc::kill(pid, 0) } == 0, - "implicit session shell pid {pid} must survive server restart" - ); - let mut server = start_server(&dir, &config); let (socket2, bootstrap2, ok2) = direct_attach_session(&config, port, "read-write", &session); let key2 = bootstrap2.session_key; @@ -2743,15 +2779,44 @@ fn implicit_session_survives_server_restart_for_same_ticket_holder() { let _ = server.kill(); let _ = server.wait(); - kill_holder(&sessions_dir, &session); - assert_eq!( - shell_pid_after, shell_pid_before, - "implicit session should re-adopt the same shell pid" + assert!( + shell_pid_after.is_none(), + "implicit restart attach must stay ephemeral" ); assert!( - post.contains("IMPLICIT_MARK=implicit_restart_42"), - "implicit session state must survive restart, got {post:?}" + post.contains("IMPLICIT_MARK=") && !post.contains("implicit_restart_42"), + "implicit session must start fresh after server restart, got {post:?}" + ); +} + +#[test] +fn startup_discards_legacy_implicit_holders() { + let dir = tempfile::tempdir().unwrap(); + let sessions_dir = dir.path().join("sessions"); + let port = free_udp_port(); + let config = write_persistent_server_config(&dir, port); + let session = protocol::generate_implicit_session_name(); + + spawn_legacy_holder(&sessions_dir, &session); + let shell_pid = holder_shell_pid(&sessions_dir, &session).expect("legacy holder shell pid"); + assert!( + unsafe { libc::kill(shell_pid, 0) } == 0, + "legacy holder shell must be live before startup" + ); + + let mut server = start_server(&dir, &config); + thread::sleep(Duration::from_millis(300)); + + let _ = server.kill(); + let _ = server.wait(); + assert!( + holder_shell_pid(&sessions_dir, &session).is_none(), + "startup must remove old implicit holder metadata" + ); + assert!( + unsafe { libc::kill(shell_pid, 0) } != 0, + "startup must shut down old implicit holder shell" ); } diff --git a/tests/release_scripts.rs b/tests/release_scripts.rs index 2785bb4..084a66c 100644 --- a/tests/release_scripts.rs +++ b/tests/release_scripts.rs @@ -20,13 +20,29 @@ fn release_scripts_skip_stale_artifacts_when_auto_discovering() { } #[test] -fn systemd_sandbox_allows_netlink_for_terminal_tools() { +fn systemd_service_does_not_sandbox_remote_shells() { let service = include_str!("../packaging/systemd/dosh-server.service"); let install = include_str!("../install.sh"); for raw in [service, install] { + assert!(raw.contains("KillMode=process")); + for directive in [ + "NoNewPrivileges=", + "PrivateTmp=", + "ProtectSystem=", + "ProtectHome=", + "RestrictAddressFamilies=", + "RestrictRealtime=", + "LockPersonality=", + "MemoryDenyWriteExecute=", + ] { + assert!( + !raw.contains(directive), + "dosh sessions are user shells; systemd sandbox directive {directive} changes terminal/app behavior" + ); + } assert!( - raw.contains("RestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX AF_NETLINK"), - "dosh sessions must allow AF_NETLINK so tools like btop can enumerate network interfaces" + !raw.contains("ReadWritePaths="), + "remote shells must not be restricted to dosh config/data paths" ); } }