diff --git a/server/src/private.rs b/server/src/private.rs index 29b0da0..22bd891 100644 --- a/server/src/private.rs +++ b/server/src/private.rs @@ -27,16 +27,35 @@ pub fn create_dir(dir: &Path) -> Result<()> { .with_context(|| format!("restrict {}", dir.display())) } -/// Opens `path` for writing, truncating it, owner-readable only from the -/// moment it exists rather than by a `chmod` afterwards. +/// Opens `path` for writing, truncating it, owner-readable only. +/// +/// `mode` covers the file this call creates; [`restrict`] covers the one +/// that was already there. Both are needed, and the second is the one that +/// is easy to miss: the leaf certificate's private key is rewritten on +/// every start, so a key that ever existed with loose permissions would +/// keep them for the rest of its life. pub fn create_file(path: &Path) -> Result { - std::fs::OpenOptions::new() + let file = std::fs::OpenOptions::new() .write(true) .create(true) .truncate(true) .mode(0o600) .open(path) - .with_context(|| format!("write {}", path.display())) + .with_context(|| format!("write {}", path.display()))?; + restrict(&file, path)?; + Ok(file) +} + +/// Narrows an already-open file to owner-only. +/// +/// Through the handle rather than the path, deliberately: `set_permissions` +/// on a path re-resolves it, so between opening and chmod-ing something +/// could put a different file -- or a symlink to one -- where this one was, +/// and the mode would land on that instead. The handle cannot be +/// redirected. +fn restrict(file: &File, path: &Path) -> Result<()> { + file.set_permissions(std::fs::Permissions::from_mode(0o600)) + .with_context(|| format!("restrict {}", path.display())) } /// Writes `contents` to `path`, owner-readable only. @@ -46,3 +65,52 @@ pub fn write_file(path: &Path, contents: &[u8]) -> Result<()> { .write_all(contents) .with_context(|| format!("write {}", path.display())) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + /// The case `mode` alone does not cover: a file that already exists + /// keeps whatever permissions it had, because `mode` applies only to a + /// file this call creates. `create_dir` three functions up has carried + /// a comment about exactly this since it was written; the file path + /// did not, until dev-updater's session read the module as a unit and + /// noticed the same line was wrong twice. + fn rewriting_a_file_narrows_permissions_it_did_not_set() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("secret"); + + // As an older version, or a hand-edit, might have left it. + std::fs::write(&path, b"old").expect("plant"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).expect("loosen"); + + write_file(&path, b"new").expect("rewrite"); + let mode = std::fs::metadata(&path).expect("stat").permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "rewriting left it at {mode:o}"); + } + + #[test] + fn a_new_file_is_owner_only_from_the_start() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("fresh"); + write_file(&path, b"x").expect("write"); + let mode = std::fs::metadata(&path).expect("stat").permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + } + + #[test] + fn an_existing_directory_is_narrowed_too() { + let dir = tempfile::tempdir().expect("tempdir"); + let nested = dir.path().join("state"); + std::fs::create_dir(&nested).expect("create"); + std::fs::set_permissions(&nested, std::fs::Permissions::from_mode(0o755)).expect("loosen"); + create_dir(&nested).expect("recreate"); + let mode = std::fs::metadata(&nested) + .expect("stat") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o700, "left at {mode:o}"); + } +}