diff --git a/Cargo.lock b/Cargo.lock index 3289366..acbc223 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -33,6 +33,7 @@ dependencies = [ "serde", "serde_json", "toml", + "zeroize", ] [[package]] @@ -121,7 +122,6 @@ dependencies = [ "serde", "serde_json", "toml", - "zeroize", ] [[package]] diff --git a/crates/bxcore/Cargo.toml b/crates/bxcore/Cargo.toml index 6312bc8..1c90d41 100644 --- a/crates/bxcore/Cargo.toml +++ b/crates/bxcore/Cargo.toml @@ -13,7 +13,20 @@ serde.workspace = true serde_json.workspace = true humantime.workspace = true emsha.workspace = true +zeroize = { workspace = true, optional = true } [dev-dependencies] toml.workspace = true +# The secret store (M5b spec, section 5). A role that reads secrets enables it; `loopd` never does, +# so its binary does not contain the code that reads them. +[features] +secrets = ["dep:zeroize"] + +[[test]] +name = "secrets" +required-features = ["secrets"] + +[[test]] +name = "secrets_race" +required-features = ["secrets"] diff --git a/crates/bxcore/src/lib.rs b/crates/bxcore/src/lib.rs index 55c8b89..da334a6 100644 --- a/crates/bxcore/src/lib.rs +++ b/crates/bxcore/src/lib.rs @@ -1,4 +1,5 @@ -//! Shared data types and the frame codec for Boxmaker. No policy and no I/O beyond frames. +//! Shared data types and the frame codec for Boxmaker. No policy and no I/O beyond frames, except +//! the secret store (`secrets`), behind the feature of that name. pub mod audit; pub mod chain; @@ -12,6 +13,8 @@ pub mod hosts; pub mod ids; pub mod log; pub mod memory; +#[cfg(feature = "secrets")] +pub mod secrets; pub mod sha1; pub mod tools; pub mod wire; diff --git a/crates/gatewayd/src/secrets.rs b/crates/bxcore/src/secrets.rs similarity index 55% rename from crates/gatewayd/src/secrets.rs rename to crates/bxcore/src/secrets.rs index 8e97d5a..3359444 100644 --- a/crates/gatewayd/src/secrets.rs +++ b/crates/bxcore/src/secrets.rs @@ -1,18 +1,100 @@ -//! The `SecretStore`: a secret from a systemd credential, an environment variable, or an owner-only -//! file (M4a spec, section 4; the brief after P15). A `Secret` cannot be printed. +//! The secret store, for every role that needs one: a secret from a systemd credential, an +//! environment variable, or an owner-only file (M4a spec, section 4; M5b spec, section 5). A +//! `Secret` cannot be printed. Behind the feature `secrets`: `loopd` holds no credentials and never +//! enables it. use std::ffi::OsString; use std::io::Read; use std::os::unix::fs::MetadataExt; -use std::path::Path; +use std::path::{Path, PathBuf}; +use serde::Deserialize; use zeroize::Zeroizing; -use crate::config::SecretSource; - pub const RUNBOOK: &str = "see docs/runbook.md#secret-unavailable"; pub const RUNBOOK_FILE: &str = "see docs/runbook.md#secret-in-a-file"; +/// `[secrets.]` in a role's configuration: exactly one of the three is set (checked by +/// `source`). Our own format: unknown keys are errors. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Default)] +#[serde(deny_unknown_fields)] +pub struct SecretSpec { + #[serde(default)] + pub credential: Option, + #[serde(default)] + pub env: Option, + #[serde(default)] + pub file: Option, +} + +/// A checked `SecretSpec`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SecretSource { + Credential(String), + Env(String), + File(PathBuf), +} + +impl SecretSpec { + /// Exactly one source, well formed. + pub fn source(&self) -> Result { + // Exactly one of the three set, else "needs exactly one of credential, env and file". + // credential: not empty, only ASCII letters, digits, _ . -. env: not empty, only A-Z, 0-9, + // _. file: an absolute path. + let mut count = 0; + if self.credential.is_some() { + count += 1; + } + if self.env.is_some() { + count += 1; + } + if self.file.is_some() { + count += 1; + } + if count != 1 { + return Err("needs exactly one of credential, env and file".to_string()); + } + if let Some(credential) = &self.credential { + if credential.is_empty() + || !credential.bytes().all(|b| { + b.is_ascii_alphabetic() || b.is_ascii_digit() || matches!(b, b'_' | b'.' | b'-') + }) + { + return Err(format!( + "credential {:?} is not a credential name (letters, digits, _ . -)", + credential + )); + } + return Ok(SecretSource::Credential(credential.clone())); + } + if let Some(env) = &self.env { + if env.is_empty() + || !env + .bytes() + .all(|b| matches!(b, b'A'..=b'Z' | b'0'..=b'9' | b'_')) + { + return Err(format!( + "env {:?} is not a variable name (A-Z, 0-9, _)", + env + )); + } + return Ok(SecretSource::Env(env.clone())); + } + // `count == 1` and the two above are `None`, so this is `Some`. + let file = match &self.file { + Some(file) => file, + None => return Err("needs exactly one of credential, env and file".to_string()), + }; + if !file.is_absolute() { + return Err(format!( + "file {:?} must be an absolute path", + file.display() + )); + } + Ok(SecretSource::File(file.clone())) + } +} + /// A secret's text. No `Display`; `Debug` shows nothing of it; wiped when dropped. pub struct Secret(Zeroizing); @@ -54,27 +136,32 @@ impl std::fmt::Display for SecretError { impl std::error::Error for SecretError {} -/// Load secret `name` from `source`. `env` reads an environment variable (in `gatewayd`, -/// `std::env::var_os`); tests pass their own. +/// Load secret `name` from `source` for the program `role` (`"gatewayd"`), which the sentences +/// name. `env` reads an environment variable (in a role, `std::env::var_os`); tests pass their +/// own. pub fn load( + role: &str, name: &str, source: &SecretSource, env: &dyn Fn(&str) -> Option, ) -> Result { - // By source (spec section 4). Credential: read $CREDENTIALS_DIRECTORY/ (through `env`, - // not std::env); an unset variable, or a file that cannot be read, is an error. Env: the + // By source (M4a spec, section 4). Credential: read $CREDENTIALS_DIRECTORY/ (through + // `env`, not std::env); an unset variable, or a file that cannot be read, is an error. Env: the // variable, UTF-8; unset is an error. File: `read_checked` checks the path, opens and re-checks - // the same file handle, reads it, and set `warning` - // to the exact text in the task. Every value goes through `value`. Every error is a - // `SecretError` naming the secret, never the value. + // the same file handle, reads it, and a warning is set. Every value goes through `value`. + // Every error is a `SecretError` naming the secret, never the value. match source { SecretSource::Credential(cred) => { let dir = match env("CREDENTIALS_DIRECTORY") { Some(dir) => dir, - None => return Err(SecretError { - name: name.to_string(), - why: "CREDENTIALS_DIRECTORY is not set: gatewayd was not started by systemd with LoadCredentialEncrypted=".to_string(), - }), + None => { + return Err(SecretError { + name: name.to_string(), + why: format!( + "CREDENTIALS_DIRECTORY is not set: {role} was not started by systemd with LoadCredentialEncrypted=" + ), + }); + } }; let path = Path::new(dir.as_os_str()).join(cred); let bytes = match std::fs::read(&path) { @@ -124,7 +211,7 @@ pub fn load( }) } SecretSource::File(path) => { - let mut bytes = match read_checked(path, &|| {}) { + let mut bytes = match read_checked(role, path, &|| {}) { Ok(bytes) => bytes, Err(why) => { return Err(SecretError { @@ -140,7 +227,7 @@ pub fn load( Ok(Loaded { secret, warning: Some(format!( - "gatewayd: warning: secret {} is read in plaintext from {}; a systemd credential keeps it encrypted at rest ({RUNBOOK_FILE})", + "{role}: warning: secret {} is read in plaintext from {}; a systemd credential keeps it encrypted at rest ({RUNBOOK_FILE})", name, path.display() )), @@ -149,24 +236,17 @@ pub fn load( } } -/// The bytes of an owner-only regular file, read from the very file that was checked. `between` -/// runs after the path is checked and before it is opened: `load` passes `&|| {}`; tests use it -/// to swap the file. -pub fn read_checked(path: &Path, between: &dyn Fn()) -> Result>, String> { - // 1. `path.is_absolute()`, else " is not an absolute path". - // 2. `let named = std::fs::symlink_metadata(path)`, an Err(e) is "cannot read : "; - // `named.file_type().is_symlink()` is " is a symbolic link"; - // `!named.file_type().is_file()` is " is not a regular file". - // 3. `between();` - // 4. `let mut file = std::fs::File::open(path)`, an Err(e) is "cannot read : "; - // `let opened = file.metadata()`, the same error text. - // 5. `if (opened.dev(), opened.ino()) != (named.dev(), named.ino())`: - // " changed while it was read". - // 6. The owner and mode checks from the original file check, word for word, on `opened` (not - // on `named`): the uid of `user_id()`; " is not owned by the user gatewayd runs as"; - // " has mode ; only the owner may read it (0600 or 0400)". - // 7. `let mut bytes = Zeroizing::new(Vec::new());` then `file.read_to_end(&mut bytes)`, an - // Err(e) is "cannot read : ". Ok(bytes). +/// The bytes of an owner-only regular file, read from the very file that was checked. `role` +/// names the program in the owner check's sentence. `between` runs after the path is checked and +/// before it is opened: `load` passes `&|| {}`; tests use it to swap the file. +pub fn read_checked( + role: &str, + path: &Path, + between: &dyn Fn(), +) -> Result>, String> { + // The path is checked (absolute, not a link, a regular file), then opened, and the checks + // that matter are made on the open handle: the same file as the path named, owned by this + // user, no group or other bits. The race it closes is M4a review, finding 2. if !path.is_absolute() { return Err(format!("{} is not an absolute path", path.display())); } @@ -190,7 +270,7 @@ pub fn read_checked(path: &Path, between: &dyn Fn()) -> Result let owner = user_id()?; if opened.uid() != owner { return Err(format!( - "{} is not owned by the user gatewayd runs as", + "{} is not owned by the user {role} runs as", path.display() )); } @@ -221,8 +301,7 @@ fn user_id() -> Result { /// The text without one trailing newline; not empty; UTF-8. fn value(bytes: Vec) -> Result { - // UTF-8 (else an error), one trailing newline removed, not empty. Keep the bytes in `Zeroizing` - // until they are inside the `Secret`. + // Keep the bytes in `Zeroizing` until they are inside the `Secret`. let bytes = Zeroizing::new(bytes); let text = std::str::from_utf8(&bytes).map_err(|_| "the value is not UTF-8".to_string())?; let text = text.strip_suffix('\n').unwrap_or(text); diff --git a/crates/gatewayd/tests/secrets.rs b/crates/bxcore/tests/secrets.rs similarity index 64% rename from crates/gatewayd/tests/secrets.rs rename to crates/bxcore/tests/secrets.rs index abdb57e..822d94a 100644 --- a/crates/gatewayd/tests/secrets.rs +++ b/crates/bxcore/tests/secrets.rs @@ -1,5 +1,6 @@ -//! The secret store (M4a spec, section 4). The environment is passed in as a function, so no test -//! changes the process's environment. Do not edit. +//! The secret store (M4a spec, section 4), moved from `gatewayd` to `bxcore` in M5b (spec, section +//! 5) with its behaviour unchanged. The environment is passed in as a function, so no test changes +//! the process's environment. Run with `--features secrets`. Do not edit. #[path = "support/tmp.rs"] mod tmp; @@ -9,8 +10,7 @@ use std::ffi::OsString; use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; -use gatewayd::config::SecretSource; -use gatewayd::secrets::{RUNBOOK, RUNBOOK_FILE, load}; +use bxcore::secrets::{RUNBOOK, RUNBOOK_FILE, SecretSource, SecretSpec, load}; use tmp::TempDir; const TOKEN: &str = "s3cret-t0ken-value"; @@ -30,7 +30,7 @@ fn owner_file(dir: &TempDir, name: &str, text: &str, mode: u32) -> PathBuf { } fn refused(source: &SecretSource, env: &dyn Fn(&str) -> Option, word: &str) { - let e = load("mattermost_token", source, env).expect_err(word); + let e = load("gatewayd", "mattermost_token", source, env).expect_err(word); let text = e.to_string(); assert!(text.contains(word), "{word}: {text}"); assert!(text.starts_with("secret mattermost_token: "), "{text}"); @@ -48,6 +48,7 @@ fn a_systemd_credential() { let creds = dir.path().join("creds"); let env = env_of(&[("CREDENTIALS_DIRECTORY", creds.to_str().unwrap())]); let got = load( + "gatewayd", "mattermost_token", &SecretSource::Credential("mattermost-token".into()), &env, @@ -76,6 +77,7 @@ fn a_credential_outside_systemd_or_missing_is_refused() { #[test] fn an_environment_variable() { let got = load( + "gatewayd", "mattermost_token", &SecretSource::Env("MM".into()), &env_of(&[("MM", TOKEN)]), @@ -102,6 +104,7 @@ fn an_owner_only_file_is_read_with_a_warning() { mode, ); let got = load( + "gatewayd", "mattermost_token", &SecretSource::File(path.clone()), &env_of(&[]), @@ -159,6 +162,16 @@ fn a_file_anyone_else_can_read_or_that_is_not_a_plain_file_is_refused() { ); } +#[test] +fn a_file_another_user_owns_is_refused() { + // A regular file that root owns on Linux, NixOS and macOS; the tests never run as root. + refused( + &SecretSource::File(PathBuf::from("/etc/passwd")), + &env_of(&[]), + "/etc/passwd is not owned by the user gatewayd runs as", + ); +} + #[test] fn empty_and_non_utf8_values_are_refused() { let dir = TempDir::new("value"); @@ -182,13 +195,14 @@ fn empty_and_non_utf8_values_are_refused() { fn only_one_trailing_newline_is_removed_and_spaces_stay() { let dir = TempDir::new("trim"); let path = owner_file(&dir, "t", " a b \n\n", 0o600); - let got = load("x", &SecretSource::File(path), &env_of(&[])).unwrap(); + let got = load("gatewayd", "x", &SecretSource::File(path), &env_of(&[])).unwrap(); assert_eq!(got.secret.expose(), " a b \n"); } #[test] fn a_secret_prints_nothing_of_itself() { let got = load( + "gatewayd", "mattermost_token", &SecretSource::Env("MM".into()), &env_of(&[("MM", TOKEN)]), @@ -198,3 +212,74 @@ fn a_secret_prints_nothing_of_itself() { assert!(!shown.contains(TOKEN), "{shown}"); assert!(shown.contains("Secret(…)"), "{shown}"); } + +#[test] +fn the_sentences_name_the_role_that_loads() { + let e = load( + "memoryd", + "honcho_key", + &SecretSource::Credential("honcho-key".into()), + &env_of(&[]), + ) + .expect_err("no CREDENTIALS_DIRECTORY"); + assert!( + e.to_string() + .contains("memoryd was not started by systemd with LoadCredentialEncrypted="), + "{e}" + ); + let dir = TempDir::new("role"); + let path = owner_file(&dir, "key", TOKEN, 0o600); + let got = load( + "memoryd", + "honcho_key", + &SecretSource::File(path), + &env_of(&[]), + ) + .unwrap(); + let warning = got.warning.expect("a file secret warns"); + assert!( + warning.starts_with("memoryd: warning: secret honcho_key is read in plaintext from "), + "{warning}" + ); + let e = load( + "memoryd", + "honcho_key", + &SecretSource::File(PathBuf::from("/etc/passwd")), + &env_of(&[]), + ) + .expect_err("root owns it"); + assert!( + e.to_string() + .contains("/etc/passwd is not owned by the user memoryd runs as"), + "{e}" + ); +} + +#[test] +fn a_spec_names_exactly_one_well_formed_source() { + let spec = |text: &str| toml::from_str::(text).unwrap().source(); + assert_eq!( + spec("credential = \"mattermost-token\""), + Ok(SecretSource::Credential("mattermost-token".into())) + ); + assert_eq!(spec("env = \"MM_2\""), Ok(SecretSource::Env("MM_2".into()))); + assert_eq!( + spec("file = \"/run/token\""), + Ok(SecretSource::File(PathBuf::from("/run/token"))) + ); + for (text, word) in [ + ("", "needs exactly one of credential, env and file"), + ("env = \"A\"\nfile = \"/t\"", "needs exactly one"), + ("credential = \"\"", "is not a credential name"), + ("credential = \"a/b\"", "is not a credential name"), + ("env = \"mm\"", "is not a variable name"), + ("file = \"token\"", "must be an absolute path"), + ] { + let why = spec(text).expect_err(text); + assert!(why.contains(word), "{text:?}: {why}"); + } + assert!( + toml::from_str::("env = \"A\"\npath = \"/t\"").is_err(), + "unknown keys are refused" + ); +} diff --git a/crates/gatewayd/tests/secrets_race.rs b/crates/bxcore/tests/secrets_race.rs similarity index 87% rename from crates/gatewayd/tests/secrets_race.rs rename to crates/bxcore/tests/secrets_race.rs index 3faa5cf..76a171a 100644 --- a/crates/gatewayd/tests/secrets_race.rs +++ b/crates/bxcore/tests/secrets_race.rs @@ -1,6 +1,7 @@ //! A secret file is read from the file that was checked, never from whatever the path names a //! moment later (M4a review, finding 2). `read_checked` runs `between` after checking the path and -//! before opening it; each test swaps something there. Do not edit. +//! before opening it; each test swaps something there. Moved from `gatewayd` in M5b; run with +//! `--features secrets`. Do not edit. #[path = "support/tmp.rs"] mod tmp; @@ -8,7 +9,7 @@ mod tmp; use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; -use gatewayd::secrets::read_checked; +use bxcore::secrets::read_checked; use tmp::TempDir; const TOKEN: &str = "the-real-token"; @@ -21,7 +22,7 @@ fn owner_file(dir: &TempDir, name: &str, text: &str) -> PathBuf { } fn refused(path: &Path, between: &dyn Fn(), word: &str) { - let why = read_checked(path, between).expect_err(word); + let why = read_checked("gatewayd", path, between).expect_err(word); assert!(why.contains(word), "{word}: {why}"); assert!(why.contains(&path.display().to_string()), "{why}"); assert!( @@ -34,7 +35,7 @@ fn refused(path: &Path, between: &dyn Fn(), word: &str) { fn an_untouched_file_is_read() { let dir = TempDir::new("race-ok"); let path = owner_file(&dir, "token", &format!("{TOKEN}\n")); - let bytes = read_checked(&path, &|| {}).unwrap(); + let bytes = read_checked("gatewayd", &path, &|| {}).unwrap(); assert_eq!(bytes.as_slice(), format!("{TOKEN}\n").as_bytes()); } @@ -86,6 +87,6 @@ fn the_path_checks_still_come_first() { "is not a regular file", ); let relative = Path::new("relative/token"); - let why = read_checked(relative, &|| panic!("never reached")).unwrap_err(); + let why = read_checked("gatewayd", relative, &|| panic!("never reached")).unwrap_err(); assert!(why.contains("is not an absolute path"), "{why}"); } diff --git a/crates/bxcore/tests/support/tmp.rs b/crates/bxcore/tests/support/tmp.rs new file mode 100644 index 0000000..3a1229a --- /dev/null +++ b/crates/bxcore/tests/support/tmp.rs @@ -0,0 +1,40 @@ +//! Temporary directories for tests. Do not edit. + +#![allow(dead_code)] // each test file uses a different part of this module + +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU32, Ordering}; + +static NEXT: AtomicU32 = AtomicU32::new(0); + +pub struct TempDir(PathBuf); + +impl TempDir { + pub fn new(tag: &str) -> TempDir { + let n = NEXT.fetch_add(1, Ordering::SeqCst); + let path = std::env::temp_dir().join(format!("bx-{tag}-{}-{n}", std::process::id())); + let _ = std::fs::remove_dir_all(&path); + std::fs::create_dir_all(&path).unwrap(); + TempDir(path) + } + + pub fn path(&self) -> &Path { + &self.0 + } + + /// Writes `text` to `name` inside the directory and returns the full path. + pub fn write(&self, name: &str, text: &str) -> PathBuf { + let path = self.0.join(name); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(&path, text).unwrap(); + path + } +} + +impl Drop for TempDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} diff --git a/crates/gatewayd/Cargo.toml b/crates/gatewayd/Cargo.toml index b465fde..b1cb171 100644 --- a/crates/gatewayd/Cargo.toml +++ b/crates/gatewayd/Cargo.toml @@ -9,13 +9,12 @@ publish.workspace = true workspace = true [dependencies] -bxcore.workspace = true +bxcore = { workspace = true, features = ["secrets"] } rustls.workspace = true rustls-native-certs.workspace = true serde.workspace = true serde_json.workspace = true toml.workspace = true -zeroize.workspace = true [dev-dependencies] rustls.workspace = true diff --git a/crates/gatewayd/src/config.rs b/crates/gatewayd/src/config.rs index ab9e22d..9be204c 100644 --- a/crates/gatewayd/src/config.rs +++ b/crates/gatewayd/src/config.rs @@ -3,6 +3,7 @@ use std::collections::BTreeMap; use std::path::{Path, PathBuf}; +use bxcore::secrets::{SecretSource, SecretSpec}; use serde::Deserialize; #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] @@ -39,26 +40,6 @@ pub struct MattermostConfig { pub ca_file: Option, } -/// Where one secret comes from: exactly one of the three is set (checked by `Config::load`). -#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Default)] -#[serde(deny_unknown_fields)] -pub struct SecretSpec { - #[serde(default)] - pub credential: Option, - #[serde(default)] - pub env: Option, - #[serde(default)] - pub file: Option, -} - -/// A checked `SecretSpec`. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum SecretSource { - Credential(String), - Env(String), - File(PathBuf), -} - #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] #[serde(deny_unknown_fields)] pub struct AllowConfig { @@ -267,66 +248,6 @@ impl Config { } } -impl SecretSpec { - /// Exactly one source, well formed. - pub fn source(&self) -> Result { - // Exactly one of the three set, else "needs exactly one of credential, env and file". - // credential: not empty, only ASCII letters, digits, _ . -. env: not empty, only A-Z, 0-9, - // _. file: an absolute path. The messages are in the task. - let mut count = 0; - if self.credential.is_some() { - count += 1; - } - if self.env.is_some() { - count += 1; - } - if self.file.is_some() { - count += 1; - } - if count != 1 { - return Err("needs exactly one of credential, env and file".to_string()); - } - if let Some(credential) = &self.credential { - if credential.is_empty() - || !credential.bytes().all(|b| { - b.is_ascii_alphabetic() || b.is_ascii_digit() || matches!(b, b'_' | b'.' | b'-') - }) - { - return Err(format!( - "credential {:?} is not a credential name (letters, digits, _ . -)", - credential - )); - } - return Ok(SecretSource::Credential(credential.clone())); - } - if let Some(env) = &self.env { - if env.is_empty() - || !env - .bytes() - .all(|b| matches!(b, b'A'..=b'Z' | b'0'..=b'9' | b'_')) - { - return Err(format!( - "env {:?} is not a variable name (A-Z, 0-9, _)", - env - )); - } - return Ok(SecretSource::Env(env.clone())); - } - // `count == 1` and the two above are `None`, so this is `Some`. - let file = match &self.file { - Some(file) => file, - None => return Err("needs exactly one of credential, env and file".to_string()), - }; - if !file.is_absolute() { - return Err(format!( - "file {:?} must be an absolute path", - file.display() - )); - } - Ok(SecretSource::File(file.clone())) - } -} - /// A Mattermost id: 26 characters of `a-z0-9`. pub fn valid_id(id: &str) -> bool { // 26 bytes, each a-z or 0-9. diff --git a/crates/gatewayd/src/lib.rs b/crates/gatewayd/src/lib.rs index d2d2571..0bde44c 100644 --- a/crates/gatewayd/src/lib.rs +++ b/crates/gatewayd/src/lib.rs @@ -8,7 +8,6 @@ pub mod gateway; pub mod http; pub mod mm; pub mod net; -pub mod secrets; pub mod serve; pub mod sessions; pub mod state; diff --git a/crates/gatewayd/src/main.rs b/crates/gatewayd/src/main.rs index ad3fe7b..05e3f96 100644 --- a/crates/gatewayd/src/main.rs +++ b/crates/gatewayd/src/main.rs @@ -7,8 +7,8 @@ use std::process::ExitCode; use std::sync::Arc; use std::sync::atomic::AtomicBool; +use bxcore::secrets; use gatewayd::config::{Config, MATTERMOST_TOKEN}; -use gatewayd::secrets; use gatewayd::serve::{START_FAILED, Tuning, run}; fn main() -> ExitCode { @@ -30,8 +30,9 @@ fn serve(path: &Path) -> ExitCode { // "gatewayd: {e}\n{START_FAILED}". // 2. `let source = match config.token_source() { ... }`: an Err(why) prints // "gatewayd: {}: {why}\n{START_FAILED}" with `path.display()`. - // 3. `let loaded = match secrets::load(MATTERMOST_TOKEN, &source, &|k| std::env::var_os(k))`: - // an Err(e) prints "gatewayd: {e}" (the error carries its own pointer). + // 3. `let env = |k: &str| std::env::var_os(k);`, then + // `let loaded = match secrets::load("gatewayd", MATTERMOST_TOKEN, &source, &env)`: an Err(e) + // prints "gatewayd: {e}" (the error carries its own pointer). // Then `if let Some(warning) = &loaded.warning { eprintln!("{warning}"); }`. // 4. The state file's directory: // `let dir = config.state_path().parent().map(Path::to_path_buf).unwrap_or_default();` @@ -54,7 +55,8 @@ fn serve(path: &Path) -> ExitCode { return ExitCode::from(1); } }; - let loaded = match secrets::load(MATTERMOST_TOKEN, &source, &|k| std::env::var_os(k)) { + let env = |k: &str| std::env::var_os(k); + let loaded = match secrets::load("gatewayd", MATTERMOST_TOKEN, &source, &env) { Ok(loaded) => loaded, Err(e) => { eprintln!("gatewayd: {e}"); diff --git a/crates/gatewayd/src/mm/rest.rs b/crates/gatewayd/src/mm/rest.rs index 3d3d303..4bfc5d7 100644 --- a/crates/gatewayd/src/mm/rest.rs +++ b/crates/gatewayd/src/mm/rest.rs @@ -3,13 +3,13 @@ use std::time::{Duration, SystemTime}; +use bxcore::secrets::Secret; use serde::Deserialize; use crate::config::valid_id; use crate::http::{self, Response, rate_limit_wait}; use crate::mm::{Me, MmError, Post, Since, json, since_list}; use crate::net::Connector; -use crate::secrets::Secret; use crate::ws::conn::host_header; /// The pause before trying a call again after a 5xx. diff --git a/crates/gatewayd/src/serve/mod.rs b/crates/gatewayd/src/serve/mod.rs index d4ffbc0..4c18c7f 100644 --- a/crates/gatewayd/src/serve/mod.rs +++ b/crates/gatewayd/src/serve/mod.rs @@ -13,6 +13,7 @@ use std::sync::mpsc::{self, Receiver, Sender}; use std::time::{Duration, Instant}; use bxcore::SessionId; +use bxcore::secrets::Secret; use crate::approvals::Registry; use crate::config::Config; @@ -20,7 +21,6 @@ use crate::gateway::{self, ToLoop}; use crate::mm::rest::Client; use crate::mm::{Event, Me, MmError, parse_event}; use crate::net::Connector; -use crate::secrets::Secret; use crate::sessions::{Queues, Router}; use crate::state::{State, StateError}; use crate::ws::conn::{Timing, Ws}; diff --git a/crates/gatewayd/tests/config.rs b/crates/gatewayd/tests/config.rs index 12b6fdc..6c5b906 100644 --- a/crates/gatewayd/tests/config.rs +++ b/crates/gatewayd/tests/config.rs @@ -5,7 +5,8 @@ mod tmp; use std::path::PathBuf; -use gatewayd::config::{Config, ConfigError, SecretSource, ServerUrl, parse_url, valid_id}; +use bxcore::secrets::SecretSource; +use gatewayd::config::{Config, ConfigError, ServerUrl, parse_url, valid_id}; use tmp::TempDir; const OWNER: &str = "abcdefghijklmnopqrstuvwxyz"; diff --git a/crates/gatewayd/tests/mm_edit.rs b/crates/gatewayd/tests/mm_edit.rs index 47b72cb..f2d92b7 100644 --- a/crates/gatewayd/tests/mm_edit.rs +++ b/crates/gatewayd/tests/mm_edit.rs @@ -9,11 +9,11 @@ mod tls_server; use std::net::SocketAddr; use std::time::Duration; +use bxcore::secrets::Secret; use gatewayd::config::ServerUrl; use gatewayd::mm::MmError; use gatewayd::mm::rest::Client; use gatewayd::net::Connector; -use gatewayd::secrets::Secret; use http_server::{reply, serve_http}; use serde_json::json; diff --git a/crates/gatewayd/tests/mm_rest.rs b/crates/gatewayd/tests/mm_rest.rs index e2a937b..c7fe5bc 100644 --- a/crates/gatewayd/tests/mm_rest.rs +++ b/crates/gatewayd/tests/mm_rest.rs @@ -10,11 +10,11 @@ use std::net::SocketAddr; use std::sync::mpsc::Receiver; use std::time::{Duration, Instant}; +use bxcore::secrets::Secret; use gatewayd::config::ServerUrl; use gatewayd::mm::MmError; use gatewayd::mm::rest::Client; use gatewayd::net::Connector; -use gatewayd::secrets::Secret; use http_server::{Request, reply, serve_http}; use serde_json::json; use tls_server::{fixture, server_config}; diff --git a/crates/gatewayd/tests/support/gateway.rs b/crates/gatewayd/tests/support/gateway.rs index 620d987..8d25106 100644 --- a/crates/gatewayd/tests/support/gateway.rs +++ b/crates/gatewayd/tests/support/gateway.rs @@ -9,8 +9,8 @@ use std::thread::JoinHandle; use std::time::Duration; use bxcore::Turn; +use bxcore::secrets::Secret; use gatewayd::config::Config; -use gatewayd::secrets::Secret; use gatewayd::serve::{Stop, Tuning, run}; use crate::fake_loop::{done, serve_loop}; diff --git a/crates/loopd/tests/no_secrets.rs b/crates/loopd/tests/no_secrets.rs new file mode 100644 index 0000000..fbdd8bf --- /dev/null +++ b/crates/loopd/tests/no_secrets.rs @@ -0,0 +1,95 @@ +//! `loopd` holds no credentials, so no build of it enables `bxcore`'s feature `secrets` (M5b spec, +//! section 5), and its binary does not contain the code that reads them. Three manifests decide +//! that: `loopd`'s own, the workspace's entry for `bxcore`, and `bxcore`'s feature table. They are +//! parsed, so every spelling TOML allows is read. `make gate` builds the workspace with the feature +//! on (for `gatewayd`), so it cannot see a module that is not behind the feature; the last test +//! checks that line of `bxcore`'s `lib.rs`. Do not edit. + +use std::path::{Path, PathBuf}; + +fn manifest(path: &Path) -> toml::Table { + let text = std::fs::read_to_string(path) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", path.display())); + text.parse::() + .unwrap_or_else(|e| panic!("cannot parse {}: {e}", path.display())) +} + +fn crates() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")).join("..") +} + +/// Every string in `value`, at any depth, that turns the feature on: `secrets`, `bxcore/secrets` +/// or `bxcore?/secrets`. +fn enabling(value: &toml::Value, found: &mut Vec) { + match value { + toml::Value::String(s) if s == "secrets" || s.ends_with("/secrets") => { + found.push(s.clone()) + } + toml::Value::Array(items) => items.iter().for_each(|v| enabling(v, found)), + toml::Value::Table(table) => table.values().for_each(|v| enabling(v, found)), + _ => {} + } +} + +#[test] +fn loopd_never_names_the_feature() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("Cargo.toml"); + let table = manifest(&path); + assert!(table.contains_key("dependencies"), "{}", path.display()); + let mut found = Vec::new(); + enabling(&toml::Value::Table(table), &mut found); + assert!(found.is_empty(), "{} enables {found:?}", path.display()); +} + +#[test] +fn the_workspace_entry_for_bxcore_enables_nothing() { + let path = crates().join("../Cargo.toml"); + let table = manifest(&path); + let entry = table + .get("workspace") + .and_then(|w| w.get("dependencies")) + .and_then(|d| d.get("bxcore")) + .unwrap_or_else(|| panic!("{}: no [workspace.dependencies] bxcore", path.display())); + assert_eq!(entry.get("features"), None, "{entry:?}"); +} + +#[test] +fn only_the_feature_itself_turns_the_store_on() { + let path = crates().join("bxcore/Cargo.toml"); + let table = manifest(&path); + let features = table + .get("features") + .and_then(toml::Value::as_table) + .unwrap_or_else(|| panic!("{}: no [features]", path.display())); + assert!(features.contains_key("secrets"), "{features:?}"); + for (name, list) in features { + if name == "secrets" { + continue; + } + let mut found = Vec::new(); + enabling(list, &mut found); + assert!(found.is_empty(), "feature {name} turns on {found:?}"); + } + let zeroize = table + .get("dependencies") + .and_then(|d| d.get("zeroize")) + .unwrap_or_else(|| panic!("{}: no zeroize", path.display())); + assert_eq!( + zeroize.get("optional").and_then(toml::Value::as_bool), + Some(true), + "zeroize is built only with the feature" + ); +} + +#[test] +fn the_module_is_compiled_only_with_the_feature() { + let path = crates().join("bxcore/src/lib.rs"); + let text = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", path.display())); + assert!( + text.contains("#[cfg(feature = \"secrets\")]\npub mod secrets;\n"), + "{}: `pub mod secrets;` must follow `#[cfg(feature = \"secrets\")]`", + path.display() + ); + assert_eq!(text.matches("mod secrets").count(), 1, "{}", path.display()); +} diff --git a/docs/dependencies.md b/docs/dependencies.md index 82590e7..8237339 100644 --- a/docs/dependencies.md +++ b/docs/dependencies.md @@ -11,7 +11,7 @@ Every external crate has a row here. `scripts/check-dep-docs.sh` enforces it. | `emsha` | 1.0.4 | `bxcore` | SHA-256. Written by the owner; no dependencies, no `unsafe`. Checked against `sha256sum` on 3,204 inputs. | | `rustls` | 0.23.45 | `gatewayd` | TLS to Mattermost over the tailnet (M4a, owner's decision). `default-features = false` with `ring`, `std`, `tls12`. With its dependencies on Linux: `ring` (Apache-2.0 AND ISC; builds C and assembly), `rustls-webpki` and `untrusted` (ISC), `rustls-pki-types`, `subtle` (BSD-3-Clause), `zeroize`, `once_cell`, `getrandom`, `libc`, `cfg-if`. Apache-2.0 OR ISC OR MIT. | | `rustls-native-certs` | 0.8.4 | `gatewayd` | The host's trusted certificates, so a CA installed on the host is trusted too; adds `openssl-probe`. Apache-2.0 OR ISC OR MIT. | -| `zeroize` | 1.9.0 | `gatewayd` | Wipes a secret's memory when it is dropped (`Secret`). Already a dependency of `rustls`. Apache-2.0 OR MIT. | +| `zeroize` | 1.9.0 | `bxcore`, feature `secrets` (enabled by `gatewayd`; never by `loopd`) | Wipes a secret's memory when it is dropped (`Secret`). Already a dependency of `rustls`. Apache-2.0 OR MIT. | | `rusqlite` | 0.40.2 | `memoryd` from M5a | The full-text index (`index/memory.sqlite3`, FTS5), disposable and rebuilt from files. Feature `bundled`: `libsqlite3-sys` compiles SQLite from source with FTS5 on, so no system SQLite is needed. With its dependencies: `libsqlite3-sys` (MIT; build: `cc`, `pkg-config`, `vcpkg`), `hashlink`, `hashbrown`, `foldhash` (Zlib, added to `deny.toml` for it), `fallible-iterator`, `fallible-streaming-iterator`, `smallvec`, `bitflags`. MIT. | ## Ported code diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 0aab80b..de9b1f8 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -6,6 +6,7 @@ reviewer adds findings under "Reviews" once per milestone. | Task | Date | Status | Gate runs | First gate | Deviations | Notes | Model | |---|---|---|---|---|---|---|---| +| M5b/02-secrets-move | 2026-09-26 | done | 1 | pass | none | Copied the given files from `docs/plans/M5b/files/`: `bxcore/src/secrets.rs`, `bxcore/tests/{secrets,secrets_race}.rs`, `bxcore/tests/support/tmp.rs`, `loopd/tests/no_secrets.rs`, and new versions of `gatewayd/tests/{config,mm_edit,mm_rest}.rs` and `support/gateway.rs` (only an import changes in each). `bxcore/Cargo.toml`: `zeroize` becomes optional, `[features] secrets = ["dep:zeroize"]`, and two `[[test]]` entries with `required-features = ["secrets"]` so the two files run only when the feature is on; `bxcore/src/lib.rs` doc comment renamed, and `#[cfg(feature = "secrets")] pub mod secrets;` inserted between `memory` and `sha1`. `gatewayd/Cargo.toml`: `bxcore` gains `features = ["secrets"]` and its own `zeroize` dependency is dropped; `gatewayd/src/lib.rs` loses `pub mod secrets`. `gatewayd/src/config.rs` imports `SecretSpec`/`SecretSource` from `bxcore` and drops the two types plus `impl SecretSpec::source` (still called through the import via `problem` and `token_source`). `gatewayd/src/main.rs` imports `bxcore::secrets`, passes `"gatewayd"` as `role` to `load`, and uses the `let env = |k: &str| std::env::var_os(k);` closure; `gatewayd/src/mm/rest.rs` and `serve/mod.rs` import `Secret` from `bxcore`. `git rm`'d `gatewayd/src/secrets.rs` and its two secret tests. `docs/dependencies.md` lists `zeroize` under `bxcore`, feature `secrets` (enabled by gatewayd, never by loopd). Step 2 showed exactly the named E0432 failures and the two named `no_secrets` failures; after the edits `cargo test -p bxcore --features secrets` (11 in `secrets`, 5 in `secrets_race`), `cargo test -p bxcore` skips them, all four `no_secrets` pass, and `cargo test -p gatewayd` passes including `main`. Step 7 greps/trees as expected (no `zeroize`/`crate::secrets`/`gatewayd::secrets` in gatewayd, `loopd` does not enable the feature, `gatewayd` does, `loopd` builds alone, only `no_secrets.rs` untracked, nothing changed in the other crates). `cargo fmt --all`; `make gate` printed `gate: ok` with 963 tests (956 before; the 13 gatewayd secret tests moved into bxcore, plus 3 new in `bxcore/tests/secrets.rs` and 4 new in `loopd/tests/no_secrets.rs`, +7 net). `zeroize` moved from gatewayd to bxcore, so `Cargo.lock` changed. | ornith-1.5-35b-a3b | | M5b/01-invalid-arguments-detail | 2026-09-26 | done | 1 | pass | none | Copied the 13 given files from `docs/plans/M5b/files/` (two new: `bxcore/tests/fixtures/wire/tool_response_denied_detail.json` and `brokerd/tests/invalid_detail.rs`; new versions of `bxcore/tests/wire.rs` and `strict.rs`; new versions of `brokerd/tests/{admin,broker,broker_pending,courier,ledger,serve}.rs` where `broker.rs`/`serve.rs` are the `-task01` files; new versions of `loopd/tests/{broker_port,tools,turn_broker}.rs`); step 2 showed exactly the named failures (`strict::envelopes_reject_unknown_keys_at_every_depth` on the new fixture, two `E0559` in `wire`, and ten non-compiling brokerd/loopd test targets with the named error kinds). Made all the edits exactly as the task gives: `bxcore/src/wire.rs` `ToolResponse::Denied` gains `detail: Option` with the skip/serde attributes; `brokerd/src/args.rs` adds `MAX_DETAIL=300` and `ArgsError::detail` (control chars and U+2028/U+2029 -> space, cut to 300 chars); `brokerd/src/policy/mod.rs` `Denial` gains `detail`, `Denial::new` sets `None`, `decide` sets it for `InvalidArguments` only, the two `DeniedByGrant` arms set `None`; `brokerd/src/ledger/mod.rs` `Decided::Denied` now holds a `Denial` (every arm rebuilt through `Denial::new` except the `RecordPayload::Denial` arm which passes the `Denial` through); `brokerd/src/broker.rs` `handle` sends the denial's detail, the two `pending` arms send `None`; `loopd/src/tools.rs` adds `denial_message(reason, detail)` before `FakeTools` and `FakeTools::call` sends `detail: None`; `loopd/src/turn.rs` imports `denial_message` and reads `detail` into it, the `TurnEvent::ToolDenied` event keeping only the reason. Did not touch `gatewayd`, `bxctl` or `bxcore/src/audit.rs`. `cargo fmt --all`; `cargo test -p bxcore -p brokerd -p loopd` every suite passes (including the eight `invalid_detail` tests); step 6 `grep -rn "Decided::Denied(DenyReason"` prints nothing and `git diff --stat -- crates/gatewayd crates/bxctl crates/bxcore/src/audit.rs` prints nothing; `cargo test --workspace` reports 955 passed and the gate's separate loopd `end_to_end` ignored test adds 1, for 956 total. No new dependency, so `Cargo.lock` is unchanged. | ornith-1.5-35b-a3b | | M5a/22-plain-note-answers | 2026-09-25 | done | 1 | pass | none | The edits are all in `crates/memoryd/src/edit.rs` and match the task exactly. Replaced the `SAVED_AS_NOTE` const with a `saved_as_note(file)` fn ("not added to {file.name()}: this session has read untrusted content, so it was saved in notes.md as an untrusted note") plus the `NOTED` const "saved in notes.md" and `NOTED_UNTRUSTED` const "saved in notes.md, marked untrusted: this session has read untrusted content"; the `format!` string stays one line past 100 chars and `cargo fmt` left it so. Updated the comment in `apply` ("replaced by saved_as_note(file)") and the `answer:` line (`saved_as_note(file)`). In `noted` replaced the two string literals with `NOTED_UNTRUSTED`/`NOTED`. Copied the new `crates/memoryd/tests/edit.rs` and `serve.rs` from `docs/plans/M5a/files/` (not the `-task06`/`-task10` versions). Step 2 showed exactly the four named tests failing (`a_note_is_labelled_by_the_session` and `an_untrusted_session_can_take_notes_but_not_change_identity` in edit, `an_untrusted_session_writes_notes_not_identity` and `review_holds_identity_writes_but_not_notes` in serve); after the edits the step-2 command and `cargo test -p memoryd` every suite pass; `make gate` printed `gate: ok` with 944 tests, unchanged; `grep -rn SAVED_AS_NOTE crates/` prints nothing. No new dependency, so `Cargo.lock` is unchanged. | Ornith-1.5-35B-A3B | | M5a/21-session-follows-result | 2026-09-25 | done (by the design model) | many | fail | The session added and then deleted diagnostic test files (`crates/bxctl/tests/zz_diag.rs`, `zz_diag2.rs`) and ran about 80 minutes of experiments instead of stopping when a test outside the task failed, as the task says. | The session's edits match the task file exactly and the copied `ledger_label.rs` is identical to the plan's; step 2 showed the two named tests failing, and after the edits `ledger_label`, `cargo test -p brokerd` and `-p bxctl` passed. The gate then failed in `bxctl`'s `args_os` with `NotFound`. The cause was the design model's: its reference worktree for tasks 21 and 22 shared this `target/` through a symlink, so test binaries built there had the worktree's absolute paths compiled in (`CARGO_BIN_EXE_*`, `CARGO_MANIFEST_DIR`), and Cargo, whose freshness check does not include the workspace's absolute path, kept them after the worktree was deleted. After `cargo clean`, `make gate` printed `gate: ok`, 944 tests. The owner stopped the run. | Ornith-1.5-35B-A3B (the session); design model (the commit) |