diff --git a/src/agent_publish.rs b/src/agent_publish.rs index fa252bc8..7fd99dc1 100644 --- a/src/agent_publish.rs +++ b/src/agent_publish.rs @@ -21,6 +21,7 @@ use crate::catalog_transaction::sync_dir; const SCHEMA: &str = "st2.agent-publish.v2"; const DIGEST_SCHEMA: &str = "st2.agent-source-digest.v1"; const BUNDLE_DIGEST_DOMAIN: &[u8] = b"st2.agent-publish-bundle.v1\0"; +const CANONICAL_DECLARATION_MODE: u32 = 0o644; #[derive(Debug, Clone)] pub enum PublishSource { @@ -80,6 +81,12 @@ struct Candidate { input_sha256: String, } +#[derive(Debug)] +struct ExistingSpec { + bytes: Vec, + mode: u32, +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] #[serde(rename_all = "lowercase")] pub enum SourceKind { @@ -124,14 +131,6 @@ impl Candidate { } }; let spec_path = stage.path().join("agent.kdl"); - anyhow::ensure!( - match &source { - PublishSource::Spec(path) => - path.extension().and_then(|value| value.to_str()) == Some("kdl"), - PublishSource::Bundle(_) => true, - }, - "published spec must be canonical KDL" - ); let metadata = fs::symlink_metadata(&spec_path) .with_context(|| format!("read candidate spec {}", spec_path.display()))?; anyhow::ensure!( @@ -236,16 +235,22 @@ pub fn publish(request: PublishRequest) -> Result { .join(&candidate.identity); let target_spec = target_dir.join("agent.kdl"); validate_existing_ancestry(&catalog, &target_dir)?; - let before = read_regular_optional(&target_spec)?; - let same_spec = before.as_deref() == Some(candidate.bytes.as_slice()); - let before_hash = before.as_deref().map(sha256); + let before = read_existing_spec(&target_spec)?; + let same_spec = before + .as_ref() + .is_some_and(|current| current.bytes == candidate.bytes); + let before_hash = before.as_ref().map(|current| sha256(¤t.bytes)); + let target_mode = before + .as_ref() + .map(|current| current.mode) + .unwrap_or(CANONICAL_DECLARATION_MODE); let after_hash = sha256(&candidate.bytes); match &request.expectation { PublishExpectation::Absent => { if let Some(current) = &before { anyhow::ensure!( - current == &candidate.bytes, + current.bytes == candidate.bytes, "publish precondition failed: {} already exists with sha256 {}", target_spec.display(), before_hash.as_deref().unwrap_or("") @@ -331,6 +336,7 @@ pub fn publish(request: PublishRequest) -> Result { &target_spec, &candidate.bytes, before.is_some(), + target_mode, )?; } CandidateKind::Bundle => { @@ -448,17 +454,24 @@ fn sha256(bytes: &[u8]) -> String { format!("{:x}", Sha256::digest(bytes)) } -fn read_regular_optional(path: &Path) -> Result>> { - match fs::symlink_metadata(path) { - Ok(metadata) => { +fn read_existing_spec(path: &Path) -> Result> { + match OpenOptions::new() + .read(true) + .custom_flags(libc::O_CLOEXEC | libc::O_NOFOLLOW | libc::O_NONBLOCK) + .open(path) + { + Ok(mut file) => { + let metadata = file.metadata()?; anyhow::ensure!( - metadata.is_file() && !metadata.file_type().is_symlink(), + metadata.is_file(), "publication target is not a regular file: {}", path.display() ); - Ok(Some( - fs::read(path).with_context(|| format!("read {}", path.display()))?, - )) + let mode = metadata.permissions().mode() & 0o7777; + let mut bytes = Vec::new(); + file.read_to_end(&mut bytes) + .with_context(|| format!("read {}", path.display()))?; + Ok(Some(ExistingSpec { bytes, mode })) } Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(None), Err(error) => Err(error).with_context(|| format!("read {}", path.display())), @@ -674,6 +687,7 @@ fn atomic_write_spec( target: &Path, bytes: &[u8], replace: bool, + mode: u32, ) -> Result<()> { let parent = target.parent().context("spec target has no parent")?; let control = crate::catalog_transaction::retained_dir_path(control_file)?; @@ -682,6 +696,8 @@ fn atomic_write_spec( .tempfile_in(&control) .with_context(|| format!("create temporary spec in {}", control.display()))?; temp.write_all(bytes)?; + temp.as_file() + .set_permissions(fs::Permissions::from_mode(mode))?; temp.as_file().sync_all()?; test_crash_after_temporary_write(); if replace { diff --git a/tests/agent_publish.rs b/tests/agent_publish.rs index e6932aa8..836be9e0 100644 --- a/tests/agent_publish.rs +++ b/tests/agent_publish.rs @@ -1,6 +1,7 @@ use std::fs; use std::io::Write as _; -use std::os::unix::process::ExitStatusExt as _; +use std::os::unix::fs::{MetadataExt as _, PermissionsExt as _}; +use std::os::unix::process::{CommandExt as _, ExitStatusExt as _}; use std::path::{Path, PathBuf}; use std::process::Stdio; use std::process::{Command, Output}; @@ -113,6 +114,121 @@ fn spec_create_is_typed_and_idempotent() { assert_eq!(second["status"], "unchanged"); } +#[test] +fn spec_source_filename_is_not_semantic_after_strict_parsing() { + let temp = tempfile::tempdir().unwrap(); + let catalog = temp.path().join("catalog"); + fs::create_dir(&catalog).unwrap(); + let candidate = temp.path().join("agent.kdl.candidate"); + fs::write(&candidate, valid_spec(false)).unwrap(); + + let digest = source_digest("--spec", &candidate); + let published = publish(&catalog, &candidate, &["--expect-absent"]); + assert!( + published.status.success(), + "{}", + String::from_utf8_lossy(&published.stderr) + ); + assert_eq!(digest, sha256(valid_spec(false).as_bytes())); + + let malformed = temp.path().join("malformed.candidate"); + fs::write(&malformed, "agent \"worker\" {").unwrap(); + let rejected = st2() + .args(["agent", "digest", "--spec"]) + .arg(&malformed) + .output() + .unwrap(); + assert!(!rejected.status.success()); + assert!( + String::from_utf8_lossy(&rejected.stderr).contains("strict declaration parsing"), + "{}", + String::from_utf8_lossy(&rejected.stderr) + ); +} + +#[test] +fn spec_creation_uses_the_canonical_readable_declaration_mode() { + let temp = tempfile::tempdir().unwrap(); + let catalog = temp.path().join("catalog"); + fs::create_dir(&catalog).unwrap(); + let candidate = temp.path().join("candidate.kdl"); + fs::write(&candidate, valid_spec(false)).unwrap(); + let input_sha256 = sha256(&fs::read(&candidate).unwrap()); + + let mut command = st2(); + command.args([ + "agent", + "publish", + "--catalog", + catalog.to_str().unwrap(), + "--spec", + candidate.to_str().unwrap(), + "--input-sha256", + &input_sha256, + "--expect-absent", + ]); + unsafe { + command.pre_exec(|| { + libc::umask(0o077); + Ok(()) + }); + } + let published = command.output().unwrap(); + assert!( + published.status.success(), + "{}", + String::from_utf8_lossy(&published.stderr) + ); + assert_eq!( + fs::metadata(target(&catalog)).unwrap().mode() & 0o7777, + 0o644 + ); +} + +#[test] +fn spec_replacement_preserves_the_accepted_target_mode() { + let temp = tempfile::tempdir().unwrap(); + let catalog = temp.path().join("catalog"); + let agent = catalog.join("agents/host/worker"); + fs::create_dir_all(&agent).unwrap(); + let current = valid_spec(false); + fs::write(agent.join("agent.kdl"), ¤t).unwrap(); + fs::set_permissions(agent.join("agent.kdl"), fs::Permissions::from_mode(0o640)).unwrap(); + let candidate = temp.path().join("candidate.kdl"); + fs::write(&candidate, valid_spec(true)).unwrap(); + let input_sha256 = sha256(&fs::read(&candidate).unwrap()); + + let mut command = st2(); + command.args([ + "agent", + "publish", + "--catalog", + catalog.to_str().unwrap(), + "--spec", + candidate.to_str().unwrap(), + "--input-sha256", + &input_sha256, + "--expect-sha256", + &sha256(current.as_bytes()), + ]); + unsafe { + command.pre_exec(|| { + libc::umask(0o077); + Ok(()) + }); + } + let published = command.output().unwrap(); + assert!( + published.status.success(), + "{}", + String::from_utf8_lossy(&published.stderr) + ); + assert_eq!( + fs::metadata(target(&catalog)).unwrap().mode() & 0o7777, + 0o640 + ); +} + #[test] fn caller_source_digest_rejects_mutation_and_symlink_swaps_before_publication() { let temp = tempfile::tempdir().unwrap();