store the forge webhook secret and grafana's secret key in bao, seeded from the existing files

swarm-controller/src/main.rs

@@ -8,10 +8,10 @@
 //! `startup::load_hives`. A config change means a redeploy, same as every other
 //! option this process reads.
 //!
-//! It persists `webhook-secret` under its `StateDirectory` (see `webhook`),
+//! It keeps the webhook HMAC secret in the swarm secret store (see `webhook`),
 //! because that key is handed to Forgejo at registration and so cannot be
-//! regenerated per boot, and the operator admin sync's granted set beside it
-//! (see `matrix_account::operator_admin`).
+//! regenerated per boot, and the operator admin sync's granted set under its
+//! `StateDirectory` (see `matrix_account::operator_admin`).
 //!
 //! Distinct from `hive-c0re`, which is per-hive: c0re owns the agents on
 //! one host, this owns what is true across hives.
@@ -179,7 +179,7 @@ async fn main() -> Result<()> {
     register_cert_gauges(jobq_metrics_provider.as_ref());
     held_deploys::register(jobq_metrics_provider.as_ref(), &jobq);
 
-    let webhook_secret = load_webhook_secret();
+    let webhook_secret = load_webhook_secret().await;
 
     // Diagnostic only — `put_matrix_account` does not call this yet (see its
     // own doc comment), so this cannot change any request's outcome. Logged

swarm-controller/src/startup.rs

@@ -217,8 +217,8 @@ pub(crate) fn agent_login(
 /// route; the webhook endpoint answers 503, which is the honest answer
 /// rather than a silent accept. Lifted out of `main` for
 /// `clippy::too_many_lines`.
-pub(crate) fn load_webhook_secret() -> Option<Arc<str>> {
-    match webhook::load_or_generate_secret() {
+pub(crate) async fn load_webhook_secret() -> Option<Arc<str>> {
+    match webhook::load_or_generate_secret().await {
         Ok(secret) => Some(Arc::from(secret)),
         Err(e) => {
             tracing::warn!(

swarm-controller/src/webhook.rs

@@ -27,12 +27,13 @@ use sha2::Sha256;
 
 use crate::routes::AppState;
 
-/// Where the HMAC secret is kept, relative to the daemon's state directory.
+/// The legacy home of the HMAC secret, relative to the daemon's state directory.
 ///
-/// The secret is generated on first start and handed to Forgejo when the
-/// hook is registered, so it must survive a restart — a rotated secret
-/// would make every subsequent delivery fail verification with a correctly
-/// configured hook on both sides.
+/// The secret lives in the swarm secret store; this file is where it lived
+/// before, and is read to seed the store. The secret is handed to Forgejo when
+/// the hook is registered, so it must not change — a rotated secret would make
+/// every subsequent delivery fail verification with a correctly configured
+/// hook on both sides.
 const SECRET_FILE: &str = "webhook-secret";
 
 /// Fallback state directory, used only when `STATE_DIRECTORY` is unset (a
@@ -82,44 +83,185 @@ fn state_dir_from(raw: Option<&str>) -> std::path::PathBuf {
     std::path::PathBuf::from(dir)
 }
 
-/// Load the swarm's webhook HMAC secret, generating and persisting it if the
-/// file is absent or malformed.
+/// Load the swarm's webhook HMAC secret from the store, creating it there if
+/// the store holds none.
 ///
-/// Returns a hex-encoded 32-byte secret (64 hex chars).
+/// Returns a hex-encoded 32-byte secret (64 hex chars). Resolution, first
+/// match wins:
+///
+/// 1. The store's value, whatever the legacy file holds.
+/// 2. The store holds none: the legacy file's value is written to the store and
+///    returned, or a fresh one when there is no legacy file. Nothing rotates,
+///    because hooks already registered with the current secret are never
+///    re-registered and would fail every delivery's HMAC check.
+/// 3. The store is unreachable: the legacy file's value, or an error when
+///    there is none. A secret minted while the store may hold another would
+///    rotate it.
+/// 4. The deployment has no store, or its policy does not cover the path yet
+///    (the store's host is deployed after this one): the legacy file as before,
+///    generated if absent. The next start, under the new policy, seeds the
+///    store from it.
+///
+/// The legacy file stays where it is; this never writes it except in case 4.
 ///
 /// # Errors
 ///
-/// Returns an error, leaving the file untouched, when an existing file cannot
-/// be read: hooks already registered with the stored secret are never
-/// re-registered, so replacing it would fail every delivery's HMAC check.
-/// Also errors when the secret cannot be generated (no `/dev/urandom`) or
-/// cannot be persisted. A malformed existing file is **not** an error: it
-/// is regenerated, because a secret that cannot be parsed is
-/// indistinguishable from one that was never written, and failing startup
-/// over it would strand the daemon with no path forward.
-pub(super) fn load_or_generate_secret() -> Result<String> {
-    load_or_generate_at(&secret_path())
+/// Returns an error, leaving the legacy file untouched, when an existing file
+/// cannot be read, when the store is unreachable and no legacy file exists, and
+/// when the secret cannot be generated or persisted. A malformed legacy file is
+/// **not** an error: it counts as absent, because a secret that cannot be
+/// parsed is indistinguishable from one that was never written, and failing
+/// startup over it would strand the daemon with no path forward.
+pub(super) async fn load_or_generate_secret() -> Result<String> {
+    resolve(&StoreSlot, &secret_path()).await
 }
 
-/// The path-taking half of [`load_or_generate_secret`], split out for the
-/// same reason as [`secret_path_from`]: a test can point it at a scratch
-/// file without mutating a process-global env var.
-fn load_or_generate_at(path: &std::path::Path) -> Result<String> {
+/// What the store holds at the webhook secret's path.
+pub(super) enum Stored {
+    /// The secret.
+    Value(String),
+    /// Nothing is stored there.
+    Absent,
+    /// No store is configured, or its policy for this principal does not cover
+    /// the path.
+    NoGrant,
+}
+
+/// The webhook secret's place in the store. A test stands in for the store
+/// through this trait.
+pub(super) trait SecretSlot {
+    /// What is stored now.
+    fn read(&self) -> impl Future<Output = Result<Stored>> + Send;
+
+    /// Store `value` only if nothing is stored yet.
+    fn create(&self, value: &str) -> impl Future<Output = Result<()>> + Send;
+}
+
+/// The slot at [`swarm_secret_client::forge::controller_webhook_secret_path`],
+/// reached under the controller's own store login.
+struct StoreSlot;
+
+impl StoreSlot {
+    async fn read_once() -> Result<Stored> {
+        let path = swarm_secret_client::forge::controller_webhook_secret_path()?;
+        let store = match crate::store::connect().await {
+            Ok(store) => store,
+            Err(swarm_secret_client::Error::MissingEnv(_)) => return Ok(Stored::NoGrant),
+            Err(e) => return Err(e).context("logging in to the swarm secret store"),
+        };
+        match store
+            .read_optional::<swarm_secret_client::forge::WebhookSecret>(&path)
+            .await
+        {
+            Ok(Some(stored)) if stored.value.trim().is_empty() => {
+                anyhow::bail!("{path} holds an empty value")
+            }
+            Ok(Some(stored)) => Ok(Stored::Value(stored.value.trim().to_owned())),
+            Ok(None) => Ok(Stored::Absent),
+            Err(e) if e.is_denied() => Ok(Stored::NoGrant),
+            Err(e) => Err(e).with_context(|| format!("reading {path}")),
+        }
+    }
+}
+
+impl SecretSlot for StoreSlot {
+    /// Retried through a store that is restarting alongside this daemon. A
+    /// denial is an answer, not an outage, so it is not retried.
+    async fn read(&self) -> Result<Stored> {
+        swarm_secret_client::read_with_startup_retry(
+            "reading the webhook secret from the swarm secret store",
+            Self::read_once,
+        )
+        .await
+    }
+
+    async fn create(&self, value: &str) -> Result<()> {
+        let path = swarm_secret_client::forge::controller_webhook_secret_path()?;
+        let store = crate::store::connect()
+            .await
+            .context("logging in to the swarm secret store")?;
+        store
+            .create(
+                &path,
+                &swarm_secret_client::forge::WebhookSecret {
+                    value: value.to_owned(),
+                },
+            )
+            .await
+            .with_context(|| format!("creating {path}"))
+    }
+}
+
+/// [`load_or_generate_secret`]'s resolution, against any slot and legacy file.
+async fn resolve(slot: &impl SecretSlot, legacy: &std::path::Path) -> Result<String> {
+    let stored = match slot.read().await {
+        Ok(stored) => stored,
+        Err(e) => {
+            return match read_legacy(legacy)? {
+                Some(secret) => {
+                    tracing::warn!(
+                        error = %format!("{e:#}"),
+                        path = %legacy.display(),
+                        "swarm secret store unreachable; using the webhook secret file"
+                    );
+                    Ok(secret)
+                }
+                None => Err(e.context("no webhook secret file to fall back on")),
+            };
+        }
+    };
+    match stored {
+        Stored::Value(secret) => Ok(secret),
+        Stored::Absent => seed(slot, legacy).await,
+        Stored::NoGrant => load_or_generate_at(legacy),
+    }
+}
+
+/// Store the legacy file's secret, or a new one when there is none, and return
+/// what the store then holds.
+async fn seed(slot: &impl SecretSlot, legacy: &std::path::Path) -> Result<String> {
+    let secret = if let Some(secret) = read_legacy(legacy)? {
+        tracing::info!(path = %legacy.display(), "storing the webhook secret file's secret in the store");
+        secret
+    } else {
+        tracing::info!("no webhook secret anywhere; generating one in the store");
+        generate_hex_secret()?
+    };
+    if let Err(e) = slot.create(&secret).await {
+        tracing::warn!(error = %format!("{e:#}"), "creating the webhook secret failed; reading back what the store holds");
+    }
+    match slot.read().await? {
+        Stored::Value(stored) => Ok(stored),
+        Stored::Absent | Stored::NoGrant => {
+            anyhow::bail!("the store holds no webhook secret after creating it")
+        }
+    }
+}
+
+/// The legacy file's secret, or `None` when it is absent or malformed.
+fn read_legacy(path: &std::path::Path) -> Result<Option<String>> {
     match std::fs::read_to_string(path) {
         Ok(raw) => {
             let trimmed = raw.trim().to_owned();
             if trimmed.len() == 64 && trimmed.chars().all(|c| c.is_ascii_hexdigit()) {
-                return Ok(trimmed);
+                return Ok(Some(trimmed));
             }
             tracing::warn!(
                 path = %path.display(),
-                "webhook-secret file malformed (wrong length/chars); regenerating"
+                "webhook-secret file malformed (wrong length/chars); ignoring it"
             );
+            Ok(None)
         }
-        Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
-        Err(e) => {
-            return Err(e).with_context(|| format!("read webhook secret {}", path.display()));
-        }
+        Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None),
+        Err(e) => Err(e).with_context(|| format!("read webhook secret {}", path.display())),
+    }
+}
+
+/// The file-only resolution: the secret in `path`, generated and persisted
+/// when the file is absent or malformed.
+fn load_or_generate_at(path: &std::path::Path) -> Result<String> {
+    if let Some(secret) = read_legacy(path)? {
+        return Ok(secret);
     }
     let secret = generate_hex_secret()?;
     std::fs::create_dir_all(path.parent().unwrap_or(path))
@@ -528,7 +670,10 @@ async fn announce_knowledge_change(state: &AppState) {
 
 #[cfg(test)]
 mod tests {
-    use super::{DeliveryKind, Refusal, load_or_generate_at, secret_path_from, verify};
+    use super::{
+        DeliveryKind, Refusal, SecretSlot, Stored, load_or_generate_at, resolve, secret_path_from,
+        verify,
+    };
     use axum::{body::Bytes, http::HeaderMap, http::StatusCode};
     use hmac::{Hmac, KeyInit as _, Mac as _};
     use sha2::Sha256;
@@ -798,4 +943,168 @@ mod tests {
             }
         }
     }
+
+    /// A store that records what it is asked to do.
+    struct FakeSlot {
+        held: std::sync::Mutex<Option<String>>,
+        /// What `read` answers while it holds nothing.
+        empty: fn() -> anyhow::Result<Stored>,
+        creates: std::sync::Mutex<u32>,
+    }
+
+    impl FakeSlot {
+        fn holding(secret: Option<&str>) -> Self {
+            Self {
+                held: std::sync::Mutex::new(secret.map(str::to_owned)),
+                empty: || Ok(Stored::Absent),
+                creates: std::sync::Mutex::new(0),
+            }
+        }
+
+        fn answering(empty: fn() -> anyhow::Result<Stored>) -> Self {
+            Self {
+                empty,
+                ..Self::holding(None)
+            }
+        }
+
+        fn held(&self) -> Option<String> {
+            self.held.lock().expect("unpoisoned").clone()
+        }
+
+        fn creates(&self) -> u32 {
+            *self.creates.lock().expect("unpoisoned")
+        }
+    }
+
+    impl SecretSlot for FakeSlot {
+        async fn read(&self) -> anyhow::Result<Stored> {
+            match self.held() {
+                Some(secret) => Ok(Stored::Value(secret)),
+                None => (self.empty)(),
+            }
+        }
+
+        async fn create(&self, value: &str) -> anyhow::Result<()> {
+            *self.creates.lock().expect("unpoisoned") += 1;
+            self.held
+                .lock()
+                .expect("unpoisoned")
+                .get_or_insert_with(|| value.to_owned());
+            Ok(())
+        }
+    }
+
+    const ON_DISK: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef";
+    const IN_STORE: &str = "fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210";
+
+    /// A scratch legacy file, holding `contents` when given.
+    fn legacy_file(contents: Option<&str>) -> (tempfile::TempDir, std::path::PathBuf) {
+        let dir = tempfile::tempdir().expect("tempdir");
+        let path = dir.path().join("webhook-secret");
+        if let Some(contents) = contents {
+            std::fs::write(&path, format!("{contents}\n")).expect("seed");
+        }
+        (dir, path)
+    }
+
+    #[tokio::test]
+    async fn an_empty_store_is_seeded_from_the_legacy_file_unchanged() {
+        let (_dir, path) = legacy_file(Some(ON_DISK));
+        let slot = FakeSlot::holding(None);
+
+        let secret = resolve(&slot, &path).await.expect("resolves");
+
+        assert_eq!(secret, ON_DISK, "the effective secret must not change");
+        assert_eq!(slot.held().as_deref(), Some(ON_DISK));
+        assert_eq!(
+            std::fs::read_to_string(&path).expect("file kept"),
+            format!("{ON_DISK}\n")
+        );
+    }
+
+    #[tokio::test]
+    async fn the_store_wins_when_both_hold_a_secret() {
+        let (_dir, path) = legacy_file(Some(ON_DISK));
+        let slot = FakeSlot::holding(Some(IN_STORE));
+
+        let secret = resolve(&slot, &path).await.expect("resolves");
+
+        assert_eq!(secret, IN_STORE);
+        assert_eq!(slot.creates(), 0, "a held secret is never written over");
+    }
+
+    #[tokio::test]
+    async fn neither_holding_one_generates_it_in_the_store_only() {
+        let (_dir, path) = legacy_file(None);
+        let slot = FakeSlot::holding(None);
+
+        let secret = resolve(&slot, &path).await.expect("resolves");
+
+        assert_eq!(secret.len(), 64);
+        assert_eq!(slot.held().as_deref(), Some(secret.as_str()));
+        assert!(!path.exists(), "a new secret is not mirrored to disk");
+    }
+
+    #[tokio::test]
+    async fn a_malformed_legacy_file_does_not_seed_the_store() {
+        let (_dir, path) = legacy_file(Some("not-a-hex-secret"));
+        let slot = FakeSlot::holding(None);
+
+        let secret = resolve(&slot, &path).await.expect("resolves");
+
+        assert_eq!(secret.len(), 64);
+        assert_ne!(secret, "not-a-hex-secret");
+        assert_eq!(slot.held().as_deref(), Some(secret.as_str()));
+    }
+
+    #[tokio::test]
+    async fn without_a_grant_the_legacy_file_is_used_and_never_replaced() {
+        let (_dir, path) = legacy_file(Some(ON_DISK));
+        let slot = FakeSlot::answering(|| Ok(Stored::NoGrant));
+
+        let secret = resolve(&slot, &path).await.expect("resolves");
+
+        assert_eq!(secret, ON_DISK);
+        assert_eq!(slot.creates(), 0);
+    }
+
+    #[tokio::test]
+    async fn without_a_grant_and_a_file_the_old_behaviour_generates_one_on_disk() {
+        let (_dir, path) = legacy_file(None);
+        let slot = FakeSlot::answering(|| Ok(Stored::NoGrant));
+
+        let secret = resolve(&slot, &path).await.expect("resolves");
+
+        assert_eq!(secret.len(), 64);
+        assert_eq!(
+            std::fs::read_to_string(&path).expect("persisted"),
+            format!("{secret}\n"),
+            "the next start, under a granting policy, seeds the store from it"
+        );
+        assert_eq!(slot.creates(), 0);
+    }
+
+    #[tokio::test]
+    async fn an_unreachable_store_uses_the_legacy_file() {
+        let (_dir, path) = legacy_file(Some(ON_DISK));
+        let slot = FakeSlot::answering(|| anyhow::bail!("connection refused"));
+
+        let secret = resolve(&slot, &path).await.expect("resolves");
+
+        assert_eq!(secret, ON_DISK);
+    }
+
+    #[tokio::test]
+    async fn an_unreachable_store_and_no_file_mints_nothing() {
+        let (_dir, path) = legacy_file(None);
+        let slot = FakeSlot::answering(|| anyhow::bail!("connection refused"));
+
+        resolve(&slot, &path)
+            .await
+            .expect_err("no secret may be minted while the store may hold one");
+
+        assert!(!path.exists());
+        assert_eq!(slot.creates(), 0);
+    }
 }