bxcore, gatewayd, loopd: the secret store moves to bxcore::secrets

Co-Authored-By: ornith-1.5-35b-a3b <agent@wntrmute.dev>

Cargo.lock

@@ -33,6 +33,7 @@ dependencies = [
  "serde",
  "serde_json",
  "toml",
+ "zeroize",
 ]
 
 [[package]]
@@ -121,7 +122,6 @@ dependencies = [
  "serde",
  "serde_json",
  "toml",
- "zeroize",
 ]
 
 [[package]]

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"]

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;

crates/bxcore/src/secrets.rs

@@ -1,18 +1,100 @@
-//! The `SecretStore`: a secret from a systemd credential, an environment variable, or an owner-only
+//! The secret store, for every role that needs one: a secret from a systemd credential, an
-//! file (M4a spec, section 4; the brief after P15). A `Secret` cannot be printed.
+//! 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.<name>]` 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<String>,
+    #[serde(default)]
+    pub env: Option<String>,
+    #[serde(default)]
+    pub file: Option<PathBuf>,
+}
+
+/// 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<SecretSource, String> {
+        // 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<String>);
 
@@ -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`,
+/// Load secret `name` from `source` for the program `role` (`"gatewayd"`), which the sentences
-/// `std::env::var_os`); tests pass their own.
+/// 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<OsString>,
 ) -> Result<Loaded, SecretError> {
-    // By source (spec section 4). Credential: read $CREDENTIALS_DIRECTORY/<name> (through `env`,
+    // By source (M4a spec, section 4). Credential: read $CREDENTIALS_DIRECTORY/<name> (through
-    // not std::env); an unset variable, or a file that cannot be read, is an error. Env: the
+    // `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`
+    // the same file handle, reads it, and a warning is set. Every value goes through `value`.
-    // to the exact text in the task. Every value goes through `value`. Every error is a
+    // Every error is a `SecretError` naming the secret, never the value.
-    // `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 {
+                None => {
-                    name: name.to_string(),
+                    return Err(SecretError {
-                    why: "CREDENTIALS_DIRECTORY is not set: gatewayd was not started by systemd with LoadCredentialEncrypted=".to_string(),
+                        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`
+/// The bytes of an owner-only regular file, read from the very file that was checked. `role`
-/// runs after the path is checked and before it is opened: `load` passes `&|| {}`; tests use it
+/// names the program in the owner check's sentence. `between` runs after the path is checked and
-/// to swap the file.
+/// before it is opened: `load` passes `&|| {}`; tests use it to swap the file.
-pub fn read_checked(path: &Path, between: &dyn Fn()) -> Result<Zeroizing<Vec<u8>>, String> {
+pub fn read_checked(
-    // 1. `path.is_absolute()`, else "<path> is not an absolute path".
+    role: &str,
-    // 2. `let named = std::fs::symlink_metadata(path)`, an Err(e) is "cannot read <path>: <e>";
+    path: &Path,
-    //    `named.file_type().is_symlink()` is "<path> is a symbolic link";
+    between: &dyn Fn(),
-    //    `!named.file_type().is_file()` is "<path> is not a regular file".
+) -> Result<Zeroizing<Vec<u8>>, String> {
-    // 3. `between();`
+    // The path is checked (absolute, not a link, a regular file), then opened, and the checks
-    // 4. `let mut file = std::fs::File::open(path)`, an Err(e) is "cannot read <path>: <e>";
+    // that matter are made on the open handle: the same file as the path named, owned by this
-    //    `let opened = file.metadata()`, the same error text.
+    // user, no group or other bits. The race it closes is M4a review, finding 2.
-    // 5. `if (opened.dev(), opened.ino()) != (named.dev(), named.ino())`:
-    //    "<path> 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()`; "<path> is not owned by the user gatewayd runs as";
-    //    "<path> has mode <mode:03o>; 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 <path>: <e>". Ok(bytes).
     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<Zeroizing<Vec<u8>
     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<u32, String> {
 
 /// The text without one trailing newline; not empty; UTF-8.
 fn value(bytes: Vec<u8>) -> Result<Secret, String> {
-    // UTF-8 (else an error), one trailing newline removed, not empty. Keep the bytes in `Zeroizing`
+    // Keep the bytes in `Zeroizing` until they are inside the `Secret`.
-    // 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);

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
+//! The secret store (M4a spec, section 4), moved from `gatewayd` to `bxcore` in M5b (spec, section
-//! changes the process's environment. Do not edit.
+//! 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 bxcore::secrets::{RUNBOOK, RUNBOOK_FILE, SecretSource, SecretSpec, load};
-use gatewayd::secrets::{RUNBOOK, RUNBOOK_FILE, 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<OsString>, 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::<SecretSpec>(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::<SecretSpec>("env = \"A\"\npath = \"/t\"").is_err(),
+        "unknown keys are refused"
+    );
+}

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}");
 }

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);
+    }
+}

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

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<PathBuf>,
 }
 
-/// 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<String>,
-    #[serde(default)]
-    pub env: Option<String>,
-    #[serde(default)]
-    pub file: Option<PathBuf>,
-}
-
-/// 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<SecretSource, String> {
-        // 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.

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;

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))`:
+    // 3. `let env = |k: &str| std::env::var_os(k);`, then
-    //    an Err(e) prints "gatewayd: {e}" (the error carries its own pointer).
+    //    `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}");

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.

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};

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";

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;
 

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};

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};

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::<toml::Table>()
+        .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<String>) {
+    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());
+}

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

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<String>` 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) |