From 5c9fe884489df6e89d54c5ecdcf1365ace69b33d Mon Sep 17 00:00:00 2001 From: Johan Thomsen Date: Fri, 1 Aug 2025 11:52:50 +0200 Subject: [PATCH] filepersist: allow being explicit about ownership and perms for public and private keys - and drop the hardcoded chgrp certpull --- nixos/file-test.nix | 48 +++++++++++++++++++++++++++++++-- src/common.rs | 37 +++++++++++++++++++++++-- src/file.rs | 66 ++++++++++++++++++++++++++++++++++----------- src/monitor.rs | 2 ++ 4 files changed, 133 insertions(+), 20 deletions(-) diff --git a/nixos/file-test.nix b/nixos/file-test.nix index c72c994..e5c4f1a 100644 --- a/nixos/file-test.nix +++ b/nixos/file-test.nix @@ -16,6 +16,12 @@ testLib.mkFaytheTest ({ nodes, ... }: { systemd.services.faythe.preStart = '' mkdir -p ${cert_path} ''; + + users.users.certuser = { + isNormalUser = true; + }; + + users.groups.certgroup = {}; }) ]; faytheExtraConfig = { @@ -29,6 +35,20 @@ testLib.mkFaytheTest ({ nodes, ... }: { cn = "path1.${domain}"; key_file_name = "key.pem"; } + { + name = "path2-test"; + cn = "path2.${domain}"; + key_file_name = "key.pem"; + cert_file_perms = { + user = "certuser"; + group = "certgroup"; + mode = "644"; + }; + key_file_perms = { + user = "certuser"; + mode = "600"; + }; + } ]; } ]; @@ -39,11 +59,35 @@ testLib.mkFaytheTest ({ nodes, ... }: { client.wait_until_succeeds(""" journalctl -u faythe | grep "path1-test" | grep -q "touched" - journalctl -u faythe | grep -q "changing group for" + openssl x509 -in ${cert_path}/path1-test/fullchain.pem -text -noout | grep -q "Issuer: CN=Pebble Intermediate" """) client.succeed(""" - openssl x509 -in ${cert_path}/path1-test/fullchain.pem -text -noout | grep -q "Issuer: CN=Pebble Intermediate" + test "$(stat -c %a ${cert_path}/path1-test/fullchain.pem)" == "644" + test "$(stat -c %a ${cert_path}/path1-test/key.pem)" == "640" + """) + + with subtest("First time issue with custom permissions and user"): + client.wait_until_succeeds("stat ${cert_path}/path2-test") + + client.wait_until_succeeds(""" + journalctl -u faythe | grep "path2-test" | grep -q "touched" + openssl x509 -in ${cert_path}/path2-test/fullchain.pem -text -noout | grep -q "Issuer: CN=Pebble Intermediate" + """) + + client.succeed(""" + test "$(stat -c %U ${cert_path}/path2-test/fullchain.pem)" == "certuser" + test "$(stat -c %U ${cert_path}/path2-test/key.pem)" == "certuser" + """) + + client.succeed(""" + test "$(stat -c %G ${cert_path}/path2-test/fullchain.pem)" == "certgroup" + test "$(stat -c %G ${cert_path}/path2-test/key.pem)" == "root" + """) + + client.succeed(""" + test "$(stat -c %a ${cert_path}/path2-test/fullchain.pem)" == "644" + test "$(stat -c %a ${cert_path}/path2-test/key.pem)" == "600" """) ''; }) diff --git a/src/common.rs b/src/common.rs index a09bd0b..53c90b5 100644 --- a/src/common.rs +++ b/src/common.rs @@ -21,6 +21,7 @@ use std::fmt::Formatter; use std::path::PathBuf; use chrono::{TimeZone, Utc}; pub type CertName = String; +use serde::{Deserialize, Deserializer}; #[derive(Debug, Clone, Serialize)] pub struct CertSpec { @@ -158,11 +159,41 @@ pub trait Persistable { async fn persist(&self, cert: Certificate) -> Result<(), PersistError>; } -#[derive(Debug, Clone, Serialize)] +#[derive(Debug, Default, Clone, Serialize)] pub struct FilePersistSpec { pub private_key_path: PathBuf, - pub public_key_path: PathBuf + pub public_key_path: PathBuf, + pub cert_file_perms: Option, + pub key_file_perms: Option, +} + +#[derive(Clone, Deserialize, Serialize, Debug)] +pub struct FilePermissions { + pub user: Option, + pub group: Option, + #[serde(deserialize_with = "deserialize_file_mode")] + pub mode: Option, +} + +#[derive(Clone, Deserialize, Serialize, Debug)] +pub struct FileMode(pub u32); + + +fn deserialize_file_mode<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let opt = >::deserialize(deserializer)?; + match opt { + Some(octal_str) => { + u32::from_str_radix(&octal_str, 8) + .map_err(serde::de::Error::custom) + .map(|mode| Some(FileMode(mode))) + } + None => Ok(None), + } } + // We don't care about the mem use difference here because we're only using VaultPersistSpec in // prod, which is the largest variant anyway, so we're always paying the full cost anyway (and // saving ~240B per spec doesn't matter in the order of 100s of specs.) @@ -459,6 +490,8 @@ pub mod tests { sub_directory: None, cert_file_name: None, key_file_name: None, + cert_file_perms: None, + key_file_perms: None, } } diff --git a/src/file.rs b/src/file.rs index a1859dc..9537d6c 100644 --- a/src/file.rs +++ b/src/file.rs @@ -3,7 +3,7 @@ extern crate walkdir; use crate::config::{FileMonitorConfig, FaytheConfig, ConfigContainer}; use std::collections::{HashMap, HashSet}; -use crate::common::{ValidityVerifier, CertSpecable, CertSpec, SpecError, PersistSpec, TouchError, IssueSource, FilePersistSpec, Cert, PersistError, CertName}; +use crate::common::{ValidityVerifier, CertSpecable, CertSpec, FileMode, FilePermissions, SpecError, PersistSpec, TouchError, IssueSource, FilePersistSpec, Cert, PersistError, CertName}; use std::fs::{File, OpenOptions}; use std::path::{Path, PathBuf}; use acme_lib::Certificate; @@ -13,8 +13,10 @@ use std::time::SystemTime; use std::os::unix::fs::PermissionsExt; use crate::log; use std::fs; +use std::fs::Permissions; use self::walkdir::WalkDir; use std::process::Command; +use std::process::Stdio; pub fn read_certs(config: &FileMonitorConfig) -> Result, FileError> { let mut certs = HashMap::new(); @@ -121,6 +123,10 @@ pub struct FileSpec { pub cert_file_name: Option, #[serde(default)] pub key_file_name: Option, + #[serde(default)] + pub cert_file_perms: Option, + #[serde(default)] + pub key_file_perms: Option, } impl IssueSource for FileSpec { @@ -144,6 +150,8 @@ impl CertSpecable for FileSpec { persist_spec: PersistSpec::File(FilePersistSpec{ private_key_path: absolute_file_path(monitor_config, &names, &names.key), public_key_path: absolute_file_path(monitor_config, &names, &names.cert), + cert_file_perms: self.cert_file_perms.clone(), + key_file_perms: self.key_file_perms.clone(), }), }) } @@ -154,7 +162,7 @@ impl CertSpecable for FileSpec { let sub_dir = absolute_dir_path(monitor_config, names.sub_directory.as_ref()); if names.sub_directory.is_some() && !sub_dir.exists() { fs::create_dir(&sub_dir)?; - sub_dir.metadata()?.permissions().set_mode(0o655) // rw-r-xr-x + sub_dir.metadata()?.permissions().set_mode(0o755) // rwxr-xr-x } let file_path = absolute_file_path(monitor_config, &names, &names.meta); let mut _file = OpenOptions::new().truncate(true).write(true).create(true).open(file_path)?; @@ -216,28 +224,54 @@ pub fn persist(spec: &FilePersistSpec, cert: &Certificate) -> Result<(), Persist let mut priv_file = File::create(&spec.private_key_path)?; let pub_buf = cert.certificate().as_bytes(); let priv_buf = cert.private_key().as_bytes(); + + // Ownership and permissions + let pub_key_permissions = Permissions::from_mode(spec.cert_file_perms.as_ref().map_or(FileMode(0o644), |p| p.mode.as_ref().map(|m| m.clone()).unwrap_or(FileMode(0o644))).0); // default: rw-r--r-- + let priv_key_permissions = Permissions::from_mode(spec.key_file_perms.as_ref().map_or(FileMode(0o640), |p| p.mode.as_ref().map(|m| m.clone()).unwrap_or(FileMode(0o640))).0); // default: rw-r----- + pub_file.set_permissions(pub_key_permissions)?; + priv_file.set_permissions(priv_key_permissions)?; + if let Some(perms) = &spec.cert_file_perms { + if perms.user.is_some() || perms.group.is_some() { + chown(perms.user.as_deref(), perms.group.as_deref(), &spec.public_key_path)?; + } + } + if let Some(perms) = &spec.key_file_perms { + if perms.user.is_some() || perms.group.is_some() { + chown(perms.user.as_deref(), perms.group.as_deref(), &spec.private_key_path)?; + } + } + + // Flush contents to files pub_file.write_all(pub_buf)?; priv_file.write_all(priv_buf)?; - let mut priv_permissions = priv_file.metadata()?.permissions(); - priv_permissions.set_mode(0o640); // rw-r------ - match spec.public_key_path.parent() { - Some(d) => chgrp("certpull", d), //TODO: don't hardcode group - None => Err(PersistError::File(FileError::IO)) - }?; Ok(()) } -fn chgrp(group: &str, path: &Path) -> Result<(), PersistError> { - log::data("changing group for", &path.as_os_str()); +fn chown(user: Option<&str>, group: Option<&str>, path: &Path) -> Result<(), PersistError> { + let mut ownership_arg = user.unwrap_or("").to_string(); + if let Some(g) = group { + ownership_arg.push(':'); + ownership_arg.push_str(g); + } + log::info(&format!("changing ownership to: {} for path: {}", &ownership_arg, &path.display())); - let mut cmd = Command::new("chgrp"); - match cmd.arg("-R") - .arg(group) - .arg(path.as_os_str()) + let mut cmd = Command::new("chown"); + match cmd + .arg(ownership_arg) + .arg(path) + .stderr(Stdio::piped()) .output() { - Ok(_) => Ok(()), - Err(e) => { log::error("chgroup failed", &e); Err(PersistError::File(FileError::IO)) } + Ok(output) => { + match output.status.success() { + true => Ok(()), + false => { + log::error("chown failed", &output.stderr); + Err(PersistError::File(FileError::IO)) + } + } + }, + Err(e) => { log::error("chown failed", &e); Err(PersistError::File(FileError::IO)) } } } diff --git a/src/monitor.rs b/src/monitor.rs index f69ff7e..13a2690 100644 --- a/src/monitor.rs +++ b/src/monitor.rs @@ -128,6 +128,8 @@ mod tests { sub_directory: None, cert_file_name: None, key_file_name: None, + cert_file_perms: None, + key_file_perms: None, }] .to_vec() }