fix(#2733): write the agent pause marker via hive-priv
`Coordinator::set_paused` wrote the marker directly with `std::fs::write`
from hive-c0re, which runs as the unprivileged `hive-core` user. The
agent's harness dir is chowned to the agent user on every container boot
(`user.nix`'s activation chown), mode 0755 — so hive-core can stat the
marker but gets EACCES creating or unlinking it. Pause therefore only
ever worked on an agent that had never booted; the read side works
because a stat needs traverse, not write, which is why the paused pill
and `is_paused` looked healthy.
Route both directions through hive-priv, the root helper that already
owns the other writes into agent-owned directories:
- `PrivRequest::SetAgentPaused { agent_name, paused }`, with the marker
filename constant moved to hive-priv-sock. That is the narrowest crate
all three sides share (hive-priv deliberately does not depend on
hive-sh4re, which re-exports it for the in-container resolver). A
private copy on any one side would break pause silently, since every
reader just sees "no marker".
- `write_agent_state_file` generalised to `write_agent_dir_file`, taking
the target directory: `state/` and `harness/` are both agent-owned,
which is the same reason both need root.
- resume unlinks via `remove_file`, which acts on the leaf and never
follows a symlink — an agent could otherwise plant a link at the
marker path and have root delete an arbitrary file.
`Coordinator::set_paused` becomes an async round-trip; its three call
sites were already async. Both directions stay idempotent because the
dashboard toggle and `hivectl pause|resume` fire without reading the
current state first.
This commit is contained in:
parent
270d3aafa2
commit
de09628c7c
9 changed files with 193 additions and 36 deletions
|
|
@ -22,8 +22,9 @@ use std::path::{Path, PathBuf};
|
|||
use anyhow::{Context as _, Result, bail};
|
||||
use hive_priv_sock::{
|
||||
AGENT_PREFIX, AGENT_RUNTIME_ROOT, AGENT_STATE_ROOT, BindMount, CredentialMount, InfraAction,
|
||||
InfraContainer, JournalQuery, META_DIR, MIGRATE_STAGING_ROOT, NetworkIsolation, PRIV_SOCK,
|
||||
PrivEvent, PrivRequest, PrivResponse, PrivStream, PrivStreamLine, SIBLING_CONTAINERS,
|
||||
InfraContainer, JournalQuery, META_DIR, MIGRATE_STAGING_ROOT, NetworkIsolation,
|
||||
PAUSED_MARKER_FILE, PRIV_SOCK, PrivEvent, PrivRequest, PrivResponse, PrivStream,
|
||||
PrivStreamLine, SIBLING_CONTAINERS,
|
||||
};
|
||||
use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
|
||||
use tokio::net::unix::OwnedWriteHalf;
|
||||
|
|
@ -248,6 +249,14 @@ async fn exec(req: PrivRequest, writer: &mut OwnedWriteHalf) -> Result<(String,
|
|||
run_forge_admin(args).await
|
||||
}
|
||||
|
||||
PrivRequest::SetAgentPaused {
|
||||
ref agent_name,
|
||||
paused,
|
||||
} => {
|
||||
validate_agent_name(agent_name)?;
|
||||
set_agent_paused(agent_name, paused)
|
||||
}
|
||||
|
||||
PrivRequest::WriteAgentForgeToken {
|
||||
ref agent_name,
|
||||
ref token,
|
||||
|
|
@ -714,12 +723,61 @@ fn write_agent_state_file(
|
|||
filename: &str,
|
||||
content: &str,
|
||||
) -> Result<(String, String)> {
|
||||
use std::os::fd::AsRawFd as _;
|
||||
use std::os::unix::fs::MetadataExt as _;
|
||||
|
||||
let state_dir = PathBuf::from(AGENT_STATE_ROOT)
|
||||
.join(agent_name)
|
||||
.join("state");
|
||||
write_agent_dir_file(agent_name, &state_dir, filename, content)
|
||||
}
|
||||
|
||||
/// Create or remove an agent's pause marker under its harness dir. The
|
||||
/// marker is written empty and chowned to the harness dir's owner (the
|
||||
/// agent), matching how the harness itself would have created it.
|
||||
///
|
||||
/// Both directions are idempotent: re-pausing truncates the existing empty
|
||||
/// marker rather than failing, and a `NotFound` on removal is the
|
||||
/// already-resumed case, not an error.
|
||||
fn set_agent_paused(agent_name: &str, paused: bool) -> Result<(String, String)> {
|
||||
let harness_dir = PathBuf::from(AGENT_STATE_ROOT)
|
||||
.join(agent_name)
|
||||
.join("harness");
|
||||
if paused {
|
||||
return write_agent_dir_file(agent_name, &harness_dir, PAUSED_MARKER_FILE, "");
|
||||
}
|
||||
remove_marker_in(&harness_dir, PAUSED_MARKER_FILE)?;
|
||||
tracing::info!(agent = %agent_name, "cleared pause marker");
|
||||
Ok((String::new(), String::new()))
|
||||
}
|
||||
|
||||
/// Unlink `dir/filename`, treating "already gone" as success.
|
||||
///
|
||||
/// `remove_file` unlinks the leaf itself and never follows a symlink, so an
|
||||
/// agent-planted link at the marker path cannot redirect this root unlink
|
||||
/// at another file — the same threat `write_state_file_nofollow` closes on
|
||||
/// the create side.
|
||||
fn remove_marker_in(dir: &Path, filename: &str) -> Result<()> {
|
||||
let path = dir.join(filename);
|
||||
match std::fs::remove_file(&path) {
|
||||
Ok(()) => Ok(()),
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()),
|
||||
Err(e) => Err(e).with_context(|| format!("remove {}", path.display())),
|
||||
}
|
||||
}
|
||||
|
||||
/// Write `content` to `dir/filename` as root, chowning the result to `dir`'s
|
||||
/// owner so the agent process can read it back. Shared by the credential
|
||||
/// writes (which target `state/`) and the pause marker (which targets
|
||||
/// `harness/`) — both write into a directory owned by the agent, which is
|
||||
/// precisely why they need hive-priv at all.
|
||||
fn write_agent_dir_file(
|
||||
agent_name: &str,
|
||||
dir: &Path,
|
||||
filename: &str,
|
||||
content: &str,
|
||||
) -> Result<(String, String)> {
|
||||
use std::os::fd::AsRawFd as _;
|
||||
use std::os::unix::fs::MetadataExt as _;
|
||||
|
||||
let state_dir = dir.to_path_buf();
|
||||
// NOTE: `create_dir_all` is normally a no-op — lifecycle creates and chowns
|
||||
// the state dir during spawn. On the rare edge where the dir doesn't exist
|
||||
// yet (container being provisioned for the first time), the newly created dir
|
||||
|
|
@ -764,7 +822,12 @@ fn write_agent_state_file(
|
|||
);
|
||||
}
|
||||
}
|
||||
tracing::info!(agent = %agent_name, file = %filename, "wrote agent state file");
|
||||
tracing::info!(
|
||||
agent = %agent_name,
|
||||
dir = %state_dir.display(),
|
||||
file = %filename,
|
||||
"wrote agent file"
|
||||
);
|
||||
Ok((String::new(), String::new()))
|
||||
}
|
||||
|
||||
|
|
@ -2121,7 +2184,9 @@ async fn sync_agent_tmpfiles(agents: &[String]) -> Result<(String, String)> {
|
|||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::{redact_password_line, write_state_file_nofollow};
|
||||
use super::{
|
||||
PAUSED_MARKER_FILE, redact_password_line, remove_marker_in, write_state_file_nofollow,
|
||||
};
|
||||
use std::path::PathBuf;
|
||||
use std::sync::atomic::{AtomicU32, Ordering};
|
||||
|
||||
|
|
@ -2198,4 +2263,51 @@ mod tests {
|
|||
assert_eq!(std::fs::read_to_string(&path).unwrap(), "new");
|
||||
std::fs::remove_dir_all(&dir).ok();
|
||||
}
|
||||
|
||||
/// The pause marker round-trips through the same root-only path the
|
||||
/// credential writes use, and BOTH directions are idempotent — the
|
||||
/// dashboard toggle and `hivectl pause|resume` fire blind, without
|
||||
/// reading the current state first.
|
||||
#[test]
|
||||
fn pause_marker_create_and_remove_are_idempotent() {
|
||||
let dir = scratch();
|
||||
let path = dir.join(PAUSED_MARKER_FILE);
|
||||
|
||||
for _ in 0..2 {
|
||||
write_state_file_nofollow(&dir, PAUSED_MARKER_FILE, "").unwrap();
|
||||
assert!(path.exists(), "marker must exist after pause");
|
||||
assert_eq!(std::fs::read_to_string(&path).unwrap(), "");
|
||||
}
|
||||
for _ in 0..2 {
|
||||
remove_marker_in(&dir, PAUSED_MARKER_FILE).unwrap();
|
||||
assert!(!path.exists(), "marker must be gone after resume");
|
||||
}
|
||||
std::fs::remove_dir_all(&dir).ok();
|
||||
}
|
||||
|
||||
/// A resume must never follow an agent-planted symlink at the marker
|
||||
/// path: this unlink runs as root, so following it would let an agent
|
||||
/// delete an arbitrary file on the host.
|
||||
#[test]
|
||||
fn resume_unlinks_the_symlink_not_its_target() {
|
||||
let dir = scratch();
|
||||
let target = dir.join("target");
|
||||
std::fs::write(&target, "original").unwrap();
|
||||
let link = dir.join(PAUSED_MARKER_FILE);
|
||||
std::os::unix::fs::symlink(&target, &link).unwrap();
|
||||
|
||||
remove_marker_in(&dir, PAUSED_MARKER_FILE).unwrap();
|
||||
// `exists()` follows the link, so it can't tell "link removed" from
|
||||
// "target removed, dangling link left" — stat the link itself.
|
||||
assert!(
|
||||
std::fs::symlink_metadata(&link).is_err(),
|
||||
"the link itself must be unlinked"
|
||||
);
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(&target).unwrap(),
|
||||
"original",
|
||||
"symlink target must survive the root unlink"
|
||||
);
|
||||
std::fs::remove_dir_all(&dir).ok();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue