diff --git a/docs/trust-boundary/security.md b/docs/trust-boundary/security.md index c6fa8982..790aadd6 100644 --- a/docs/trust-boundary/security.md +++ b/docs/trust-boundary/security.md @@ -45,6 +45,28 @@ matrix **identities** — the public handles (`name`, `user_id` `@user:server`, peers can find and address one another on a shared matrix instance. Only the public handle crosses that boundary; the token never does. +### The swarm secret store is not a boundary between hives + +A hive reads its agents' credentials out of the swarm's secret store with its +own certificate. **Every hive's policy grants read on every agent's +credentials**, not only on the agents it hosts — so a compromised hive can read +the matrix token of an agent running on a different hive. + +That is deliberate and it is the interim state, not the intent. An agent's +credential path does not name the hive hosting it (agents move between hives), +so a per-hive grant has to be an enumeration the controller re-emits whenever +the roster changes — and an enumeration that can drift or land out of order +advertises a boundary it does not actually hold. A wide grant that says what it +is beats a narrow one that only looks narrow. + +What still holds: the grant is **read-only** (a hive cannot write an agent's +credential, so it cannot hand itself an agent's identity), and it is scoped to +the agent-credential prefix — nothing else in the store is reachable with it. + +Narrowing it is tracked in **#4137**, with the two candidate directions: scope +the grant per hive (and pay for the re-emission), or give each agent container +its own store identity so credentials never pass through a hive at all. + ### Threat model: prompt injection → confused deputy The realistic adversary **never needs to breach the container**. They supply diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index 3d7212d4..00454f5b 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -731,32 +731,7 @@ fn render(declaration: &swarm_queue_client::wanted::HiveWanted) -> Vec>) -> Option> { - status.map(|s| { - Arc::new(wanted::WantedWriter::new( - s.queue_client(), - read_policy_sink(), - )) - }) -} - -/// The sink that keeps a hive's read grant in step with its declaration, or -/// `None` on a deployment that has no store identity to write one with. -/// -/// Decided once here rather than per write: the `BAO_*` environment is a -/// systemd unit's, so a variable that is absent at start is absent for the -/// life of the process. The absent case is logged because it is otherwise -/// indistinguishable from a grant that is being published and ignored. -fn read_policy_sink() -> Option> { - match swarm_secret_client::client::Settings::from_env() { - Ok(_) => Some(Arc::new(read_policy::StoreSink)), - Err(e) => { - tracing::info!( - reason = %e, - "no secret-store identity: hives get no read grant, and no credential delivery either" - ); - None - } - } + status.map(|s| Arc::new(wanted::WantedWriter::new(s.queue_client()))) } /// The per-agent status reader, sharing the status reader's connection and @@ -1647,8 +1622,15 @@ async fn main() -> Result<()> { let config_prs = forge_client.clone().map(config_pr::spawn); let state_forge = keep_forge_for_state(forge_client, webhook_secret.clone()); + let hives = load_hives(); + // Before serving, because a hive whose policy does not exist cannot read + // anything this daemon writes for it. Same "log and carry on" shape as + // every connect above. + read_policy::ensure_hive_policies(&hives.iter().map(|h| h.name.clone()).collect::>()) + .await; + let state = AppState { - hives: Arc::new(load_hives()), + hives: Arc::new(hives), links: Arc::new(load_links()), wanted: wanted_writer(status.as_ref()), agent_status: agent_status_reader(status.as_ref()), diff --git a/swarm-controller/src/read_policy.rs b/swarm-controller/src/read_policy.rs index b39514f4..f00297e4 100644 --- a/swarm-controller/src/read_policy.rs +++ b/swarm-controller/src/read_policy.rs @@ -1,47 +1,48 @@ -//! Publishing the grant that lets a hive read its own agents' credentials. +//! Writing the policy a hive's own certificate logs in with. //! //! The controller writes a credential; the hive fetches it with its own //! certificate. Nothing said which paths that certificate may read, so the //! read half of every delivery answers 403. //! -//! The grant is derived state: it is re-rendered from the swarm's declaration -//! whenever that declaration changes ([`crate::wanted`]). A missed re-emission -//! is that same untraceable 403, which is why the sink is a trait — "did it -//! emit, and with what" is then a test rather than care. +//! A startup pass, not a hook: the document is the same for every hive and +//! does not depend on which agents exist ([`swarm_secret_client::policy`] +//! explains why it is that wide), so there is nothing to keep in step with +//! anything. Per-hive failures are reported and skipped — one unwritable +//! policy should not take down a daemon that serves everything else, and the +//! next start retries it. use anyhow::{Context, Result}; -use async_trait::async_trait; -use swarm_secret_client::policy; +use swarm_secret_client::{SecretStore, policy}; -/// Where a rendered read grant goes. -#[async_trait] -pub trait ReadPolicySink: Send + Sync { - /// Grant `hive` read on exactly `agents`, replacing whatever it had. - /// - /// # Errors - /// Whatever the implementation cannot do — for the store-backed one, a - /// name the controller may not write or a store it cannot reach. - async fn publish(&self, hive: &str, agents: &[&str]) -> Result<()>; -} - -/// The real sink: renders the policy and writes it into the secret store. +/// Give every hive in `hives` the read policy its certificate will carry. /// -/// Connects per call, like `matrix_account`'s writer does: the controller -/// declares an agent's state rarely, and a handle held across a token's -/// lifetime is a renewal problem in exchange for nothing. -pub struct StoreSink; - -#[async_trait] -impl ReadPolicySink for StoreSink { - async fn publish(&self, hive: &str, agents: &[&str]) -> Result<()> { - let name = policy::hive_object_name(hive)?; - let document = policy::render(hive, agents)?; - crate::store::connect() - .await - .context("connecting to the swarm secret store")? - .write_policy(&name, &document) - .await - .with_context(|| format!("writing the read policy {name}"))?; - Ok(()) +/// Does nothing when this deployment has no store identity, which is the +/// shape a controller without a secret store runs in. +pub async fn ensure_hive_policies(hives: &[String]) { + if hives.is_empty() { + return; + } + let store = match crate::store::connect().await { + Ok(store) => store, + Err(e) => { + tracing::info!(reason = %e, "no secret store reachable; hive read policies are not managed here"); + return; + } + }; + for hive in hives { + if let Err(e) = write_one(&store, hive).await { + tracing::warn!(hive, error = %format!("{e:#}"), "writing this hive's read policy failed"); + } } } + +/// One hive's policy: its own name, the shared document. +async fn write_one(store: &SecretStore, hive: &str) -> Result<()> { + let name = policy::hive_object_name(hive)?; + store + .write_policy(&name, &policy::render()) + .await + .with_context(|| format!("writing the read policy {name}"))?; + tracing::info!(hive, policy = %name, "hive read policy in place"); + Ok(()) +} diff --git a/swarm-controller/src/wanted.rs b/swarm-controller/src/wanted.rs index ac543b04..77e0ca04 100644 --- a/swarm-controller/src/wanted.rs +++ b/swarm-controller/src/wanted.rs @@ -13,14 +13,12 @@ //! declaration to reconcile against, because the current value can be read //! back from the queue whenever it is needed. -use std::{fmt, sync::Arc}; +use std::fmt; use anyhow::{Context, Result}; use async_nats::jetstream::kv::{CreateErrorKind, UpdateErrorKind}; use swarm_queue_client::wanted::{AgentState, AgentWanted, HiveWanted}; -use crate::read_policy::ReadPolicySink; - /// The one failure out of [`apply`] that is the caller's mistake, not a /// server fault — worth a distinct type so `set_agent_state` can tell it /// apart from every other error this module raises (an undecodable @@ -103,60 +101,15 @@ fn apply(current: Option<&[u8]>, agent: &str, state: AgentState) -> Result<(Hive Ok((declaration, encoded)) } -/// Which of a declaration's agents a hive may read credentials for. -/// -/// Not all of them: the declared set is the hive's whole history, destroyed -/// entries included — they stay so that redeclaring one back to `Up` can be -/// refused as the terminal transition it is, rather than silently -/// un-destroying an agent. A torn-down agent keeping read access on its -/// credentials is what this filter exists to stop. -fn readable_agents(declaration: &HiveWanted) -> Vec<&str> { - declaration - .agents - .iter() - .filter(|(_, wanted)| wanted.state != AgentState::Destroyed) - .map(|(agent, _)| agent.as_str()) - .collect() -} - -/// Re-render `hive`'s read grant from the declaration about to be published. -/// -/// Separate from [`WantedWriter::set`] so the trigger is assertable: `set` -/// needs a live queue, this needs nothing, and "was the grant emitted, over -/// exactly which agents" is the half that fails as a 403 nobody can trace. -async fn publish_read_grant( - sink: Option<&Arc>, - hive: &str, - declaration: &HiveWanted, -) -> Result<()> { - let Some(sink) = sink else { - return Ok(()); - }; - let agents = readable_agents(declaration); - sink.publish(hive, &agents) - .await - .with_context(|| format!("publishing the read grant for {hive}")) -} - /// Writes the wanted-state bucket, and reads it back. pub struct WantedWriter { client: async_nats::Client, - /// Where a hive's read grant is published, or `None` on a deployment - /// with no store identity — see [`WantedWriter::new`]. - read_policy: Option>, } impl WantedWriter { - /// `read_policy` is `None` when this deployment has no secret store to - /// authenticate to. A declaration still publishes; there is simply no - /// grant to keep in step with it, and the credential delivery it would - /// have served does not exist either. #[must_use] - pub fn new(client: async_nats::Client, read_policy: Option>) -> Self { - Self { - client, - read_policy, - } + pub fn new(client: async_nats::Client) -> Self { + Self { client } } /// One hive's bucket handle, created on first use if nothing has made it @@ -217,14 +170,6 @@ impl WantedWriter { let (declaration, encoded) = apply(entry.as_ref().map(|e| e.value.as_ref()), agent, state)?; - // Before the write, not after: a grant that lands late is a 403 - // on the agent's first credential fetch, while - // a grant that shrinks a moment early only affects an agent - // already being torn down. If the write below then fails, the - // grant is a harmless superset and the next declaration - // re-renders it. - publish_read_grant(self.read_policy.as_ref(), hive, &declaration).await?; - // `update` and `create` have separate error types, and only one // variant of each means "someone else got there first". Every // other failure returns immediately: retrying a disconnect or a @@ -264,114 +209,8 @@ impl WantedWriter { #[cfg(test)] mod tests { - use super::{apply, publish_read_grant, readable_agents}; - use std::sync::{Arc, Mutex}; - use swarm_queue_client::wanted::{AgentState, HiveWanted}; - - /// Records what a caller published instead of talking to a store. - #[derive(Default)] - struct Recorder { - published: Mutex)>>, - } - - #[async_trait::async_trait] - impl super::ReadPolicySink for Recorder { - async fn publish(&self, hive: &str, agents: &[&str]) -> anyhow::Result<()> { - self.published.lock().unwrap().push(( - hive.to_owned(), - agents.iter().map(|a| (*a).to_owned()).collect(), - )); - Ok(()) - } - } - - /// A sink that always refuses, to pin that a failed grant is not swallowed. - struct Refuses; - - #[async_trait::async_trait] - impl super::ReadPolicySink for Refuses { - async fn publish(&self, _hive: &str, _agents: &[&str]) -> anyhow::Result<()> { - anyhow::bail!("the store said no") - } - } - - fn declaration(pairs: &[(&str, AgentState)]) -> HiveWanted { - let mut declaration = HiveWanted::default(); - for (agent, state) in pairs { - let (d, _) = apply( - Some(&serde_json::to_vec(&declaration).unwrap()), - agent, - *state, - ) - .unwrap(); - declaration = d; - } - declaration - } - - #[test] - fn a_destroyed_agent_is_not_granted_read_while_the_live_ones_are() { - let d = declaration(&[ - ("iris", AgentState::Up), - ("argus", AgentState::Offline), - ("atlas", AgentState::Destroyed), - ]); - let mut granted = readable_agents(&d); - granted.sort_unstable(); - assert_eq!( - granted, - ["argus", "iris"], - "destroyed keeps its declaration" - ); - } - - /// The control on the filter above: without it every assertion there - /// could pass because `readable_agents` returns nothing at all. - #[test] - fn a_declaration_with_no_destroyed_agent_grants_every_one_of_them() { - let d = declaration(&[("iris", AgentState::Up), ("argus", AgentState::Offline)]); - assert_eq!(readable_agents(&d).len(), 2); - } - - #[tokio::test] - async fn declaring_an_agent_publishes_the_grant_for_exactly_the_live_set() { - let recorder = Arc::new(Recorder::default()); - let sink: Arc = recorder.clone(); - let d = declaration(&[("iris", AgentState::Up), ("atlas", AgentState::Destroyed)]); - - publish_read_grant(Some(&sink), "pr1ma", &d).await.unwrap(); - - let published = recorder.published.lock().unwrap().clone(); - assert_eq!( - published, - vec![("pr1ma".to_owned(), vec!["iris".to_owned()])] - ); - } - - /// A deployment with no store still declares agent state — there is just - /// no grant to keep in step with it. - #[tokio::test] - async fn no_sink_is_a_no_op_rather_than_a_failure() { - let d = declaration(&[("iris", AgentState::Up)]); - publish_read_grant(None, "pr1ma", &d) - .await - .expect("a deployment with no store is not an error"); - } - - /// The failure this ordering exists for: a refused grant must stop the - /// declaration, not publish one whose reader will 403. - #[tokio::test] - async fn a_refused_grant_is_an_error_the_caller_sees() { - let sink: Arc = Arc::new(Refuses); - let d = declaration(&[("iris", AgentState::Up)]); - let err = publish_read_grant(Some(&sink), "pr1ma", &d) - .await - .expect_err("the sink refused"); - assert!( - format!("{err:#}").contains("the store said no"), - "the underlying refusal should survive the context: {err:#}" - ); - } + use super::apply; + use swarm_queue_client::wanted::AgentState; #[test] fn declaring_one_agent_preserves_every_other() { diff --git a/swarm-secret-client/src/policy.rs b/swarm-secret-client/src/policy.rs index 7d6af585..de45771e 100644 --- a/swarm-secret-client/src/policy.rs +++ b/swarm-secret-client/src/policy.rs @@ -4,11 +4,16 @@ //! this one says who is allowed to read it, and the two have to agree on the //! same path or a delivery fails with a 403 that names nothing. //! -//! Rendering is separate from writing on purpose: the text is a pure function -//! of a hive name and its agent set, so the shape that matters can be asserted -//! without a store to talk to. - -use std::fmt::Write as _; +//! ⚠️ Every hive gets the same document, and it grants read on **every** +//! agent's credentials rather than on the ones that hive hosts. That is a +//! decision, not an oversight: an agent's path does not name its hive, so a +//! per-hive grant has to be enumerated and re-emitted, and an enumeration that +//! can silently drift advertises a boundary it does not hold. A wide grant that +//! says so beats a narrow one that only looks narrow. The narrower shapes, and +//! what they would cost, are in `docs/trust-boundary/security.md`. +//! +//! Rendering stays separate from writing so the text can be asserted with no +//! store to talk to. use crate::{ Error, @@ -32,30 +37,17 @@ pub fn hive_object_name(hive: &str) -> Result { Ok(format!("{HIVE_PREFIX}{hive}")) } -/// Render the policy granting `hive` read on exactly the agents it hosts. +/// Render the document every hive's policy holds: read on every agent's +/// credentials. /// -/// One stanza per agent rather than a prefix grant: an agent's credential path -/// does not name the hive hosting it (agents migrate), so "this hive's agents" -/// has no prefix expression and has to be enumerated. +/// Takes no arguments because it depends on nothing — same text for every +/// hive, unchanged by which agents exist. That is what makes it a deploy-time +/// object rather than derived state with a re-emission to get wrong. /// -/// An empty `agents` renders an empty policy, which grants nothing. That is the -/// correct reading of a hive with no agents, and it fails closed. -/// -/// # Errors -/// [`Error::PathSegment`] when `hive` or any agent name holds anything but -/// `[A-Za-z0-9_-]` — which is what stops a name from closing the stanza and -/// opening a wider one. -pub fn render(hive: &str, agents: &[&str]) -> Result { - checked_segment("hive", hive)?; - let mut out = String::new(); - for agent in agents { - checked_segment("agent", agent)?; - let _ = writeln!( - out, - "path \"{MOUNT}/data/{AGENT_PREFIX}/{agent}/*\" {{\n capabilities = [\"read\"]\n}}" - ); - } - Ok(out) +/// Read-only: the controller mints these and never reads one back. +#[must_use] +pub fn render() -> String { + format!("path \"{MOUNT}/data/{AGENT_PREFIX}/*\" {{\n capabilities = [\"read\"]\n}}\n") } #[cfg(test)] @@ -63,75 +55,52 @@ mod tests { use super::*; #[test] - fn one_agent_renders_one_read_stanza_under_the_agent_prefix() { - let p = render("pr1ma", &["atlas"]).expect("both segments are legal"); + fn the_document_grants_read_over_the_whole_agent_prefix() { assert_eq!( - p, - "path \"secret/data/swarm/agents/atlas/*\" {\n capabilities = [\"read\"]\n}\n" + render(), + "path \"secret/data/swarm/agents/*\" {\n capabilities = [\"read\"]\n}\n" ); } #[test] - fn every_hosted_agent_gets_its_own_stanza_and_nothing_else_does() { - let p = render("pr1ma", &["atlas", "iris"]).expect("legal"); - assert_eq!(p.matches("path \"").count(), 2, "one stanza per agent"); - assert!(p.contains("/atlas/*")); - assert!(p.contains("/iris/*")); - assert!(!p.contains("argus"), "an agent not passed is not granted"); - } - - #[test] - fn the_grant_is_read_only_and_never_a_prefix_over_all_agents() { - // Both halves of what makes this a least-privilege policy rather than - // the broad grant that was considered and rejected. - let p = render("pr1ma", &["atlas"]).expect("legal"); + fn the_grant_is_read_only() { + // The half of the old policy that survives the widening: a hive reads + // credentials, and a hive that could write one could hand itself an + // agent's identity. + let p = render(); assert!(!p.contains("create")); assert!(!p.contains("update")); assert!(!p.contains("delete")); - assert!( - !p.contains(&format!("{MOUNT}/data/{AGENT_PREFIX}/*")), - "a wildcard directly under the agent prefix would grant every agent" - ); + assert!(!p.contains("list")); } #[test] - fn a_name_cannot_close_the_stanza_and_open_a_wider_one() { - // The reason `checked_segment` runs before the name reaches HCL: these - // are policy injection, not path traversal, and a `contains("..")` - // check catches none of them. - for bad in [ - "atlas/*\" { capabilities = [\"root\"] }\npath \"secret/data", - "*", - "../argus", - "a b", - "", - ] { - assert!( - render("pr1ma", &[bad]).is_err(), - "agent name {bad:?} must be refused" - ); - assert!( - render(bad, &["atlas"]).is_err(), - "hive name {bad:?} must be refused" - ); - } + fn no_name_reaches_the_document_at_all() { + // Why the injection cases that used to live here are gone rather than + // relaxed: nothing interpolates into the text any more, so there is no + // stanza for a name to close. `hive_object_name` still validates, + // because a name does reach the policy's *identifier*. + let p = render(); + assert!(!p.contains("pr1ma")); + assert_eq!(p.matches("path \"").count(), 1, "one stanza, no per-agent"); + assert!( + hive_object_name("atlas/*\" { capabilities = [\"root\"] }").is_err(), + "the object NAME is still a place a name can do damage" + ); } #[test] fn the_legal_charset_is_actually_reachable() { - // The control for the case above: if every name were refused, that test - // would pass while proving nothing. - assert!(render("a-b_C9", &["d-e_F0"]).is_ok()); + // The control for the case above: if every name were refused, that + // assertion would pass while proving nothing. assert!(hive_object_name("a-b_C9").is_ok()); } #[test] - fn a_hive_with_no_agents_grants_nothing() { - let p = render("pr1ma", &[]).expect("a hive may legitimately host none"); - assert!( - p.is_empty(), - "no stanza means no capability, which is closed" - ); + fn every_hive_gets_a_byte_identical_document() { + // The property the deploy-time write depends on: nothing about a hive + // or its agents changes the text, so there is nothing to re-emit. + assert_eq!(render(), render()); } #[test]