diff --git a/Cargo.lock b/Cargo.lock index ef0bc59d..041b08a9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4619,6 +4619,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.11.0", + "swarm-queue-client", "tokio", "tracing", "tracing-subscriber", diff --git a/swarm-nats-auth/Cargo.toml b/swarm-nats-auth/Cargo.toml index d85f10d5..60b63ba5 100644 --- a/swarm-nats-auth/Cargo.toml +++ b/swarm-nats-auth/Cargo.toml @@ -22,6 +22,11 @@ serde.workspace = true serde_json.workspace = true # The jti digest: base32hex(sha256(claims)) over every JWT this crate signs. sha2.workspace = true +# For `status::BUCKET` alone - the subjects a hive may publish to are derived +# from the bucket name, and the reader, the writer and this responder must +# name the same one. Deliberately WITHOUT the `kv` feature: this crate derives +# subject strings, it never opens the bucket. +swarm-queue-client.workspace = true tokio.workspace = true tracing.workspace = true tracing-subscriber.workspace = true diff --git a/swarm-nats-auth/src/main.rs b/swarm-nats-auth/src/main.rs index 64bd1487..eff84b8d 100644 --- a/swarm-nats-auth/src/main.rs +++ b/swarm-nats-auth/src/main.rs @@ -28,6 +28,7 @@ use clap::Parser; use futures_util::StreamExt; mod introspect; +mod policy; mod request; mod respond; @@ -73,6 +74,28 @@ struct Args { /// Path to this responder's own OIDC client secret. #[arg(long)] client_secret_file: PathBuf, + + /// Client-id prefix that marks a hive. `swarm-authelia.nix` mints one + /// machine client per roster entry as `hive-`, while the KV key is + /// the bare `` — this is the contract between the two, declared + /// rather than inferred from the shape of an id. + #[arg(long, default_value = "hive-")] + hive_client_prefix: String, + + /// Client ids allowed to read every hive's status. Repeatable. The + /// default is the swarm controller, which is the only reader that exists. + #[arg(long = "reader-client", default_values_t = [String::from("swarm-controller")])] + reader_clients: Vec, + + /// Additional subjects a hive may publish to, with `{hive}` standing for + /// its own name. Repeatable, empty by default. + /// + /// The extension point for a second stream published by the same + /// `hive-` identity — lifecycle notices, say. Without it, adding one + /// means changing this responder; with it, a deployment says so and the + /// subject still lands inside that hive's own namespace. + #[arg(long = "hive-publish-subject")] + hive_publish_subjects: Vec, } /// Read a secret file and strip surrounding whitespace. @@ -102,6 +125,17 @@ async fn main() -> anyhow::Result<()> { let args = Args::parse(); let client_secret = read_secret(&args.client_secret_file)?; + // The bucket is NOT a flag. `swarm_queue_client::status`'s own docs say + // why: reader and writer must name the same bucket, and an option is a + // way for two deployments to disagree about which one that is. This + // responder is the third end that names it, so it takes the same + // constant rather than a copy of the literal. + let policy = policy::Policy::new( + args.hive_client_prefix.clone(), + swarm_queue_client::status::BUCKET.to_owned(), + args.reader_clients.clone(), + args.hive_publish_subjects.clone(), + ); let http = reqwest::Client::new(); let issuer = nkeys::KeyPair::from_seed(&read_secret(&args.issuer_seed_file)?) .context("parse the account signing seed")?; @@ -157,12 +191,25 @@ async fn main() -> anyhow::Result<()> { }), None => None, }; + // Admission said who; the policy says what. A caller the `IdP` + // vouches for but no rule matches is denied — see `policy`'s module + // docs for why that is deny and not "connect with nothing". + let permissions = caller.as_deref().and_then(|id| policy.permissions(id)); + if let (Some(id), None) = (caller.as_deref(), permissions.as_ref()) { + // Loud, and the one case an operator has to be able to find: a + // valid credential refused by our own policy. The alternative is + // a client that authenticates fine and mysteriously cannot work. + tracing::warn!( + caller = %id, + "authenticated client matches no policy rule; denying" + ); + } // The client id is an identifier, not a credential, and it is the // only thing tying a connection in this log to a hive. tracing::info!( user_nkey = %req.user_nkey, server_id = %req.server_id.id, - granted = caller.is_some(), + granted = permissions.is_some(), caller = caller.as_deref().unwrap_or("-"), "auth request" ); @@ -175,14 +222,15 @@ async fn main() -> anyhow::Result<()> { tracing::warn!("auth request had no reply subject; dropping"); continue; }; - // The grant is still unscoped: knowing *who* connected is what makes - // scoping possible, not what performs it. Narrowing the permissions - // to the caller's own subjects is the next slice, and lands in - // `respond::grant` where the JWT is minted. - let token = if caller.is_some() { - respond::grant(&issuer, &args.account, &req.server_id.id, &req.user_nkey) - } else { - respond::deny(&issuer, &req.server_id.id, &req.user_nkey) + let token = match &permissions { + Some(permissions) => respond::grant( + &issuer, + &args.account, + &req.server_id.id, + &req.user_nkey, + permissions, + ), + None => respond::deny(&issuer, &req.server_id.id, &req.user_nkey), }; if let Err(e) = client.publish(reply_to, token.into()).await { tracing::warn!(error = ?e, "failed to publish auth response"); diff --git a/swarm-nats-auth/src/policy.rs b/swarm-nats-auth/src/policy.rs new file mode 100644 index 00000000..69d36b69 --- /dev/null +++ b/swarm-nats-auth/src/policy.rs @@ -0,0 +1,283 @@ +//! What an admitted client is allowed to do. +//! +//! [`crate::introspect`] answers *whether* to admit and *as whom*. This +//! answers *what that identity may touch* — the two are deliberately separate +//! decisions: admission is the `IdP`'s, authorisation is the swarm's. +//! +//! # Deny is the default, and that is a choice with a measurement behind it +//! +//! A client whose id matches no rule gets **no grant at all**, not an +//! unrestricted one. Every other shape here fails open: an unmatched client +//! that kept today's unscoped grant would make the whole policy advisory, and +//! the failure would be silent. Denying costs a loud refusal the first time a +//! new consumer appears, which is a config line to fix. +//! +//! # Subjects were measured, not reasoned about +//! +//! The defaults below are the *minimal* sets, established by granting a +//! candidate set and then removing each subject in turn to confirm the client +//! breaks without it (`state/attack-3297-js-api-door.sh`, +//! `state/attack-3297-reader-scope.sh` in atlas's notes). Two things that a +//! reader would otherwise reasonably assume, and that are false: +//! +//! - `$KV..` alone does **not** let a client write that key. The +//! client looks the bucket up first, so `$JS.API.STREAM.INFO.KV_` is +//! part of the minimum. +//! - `$JS.API.>` is not "the `JetStream` permission". It also covers +//! `$JS.API.STREAM.DELETE.KV_`, with which any hive can destroy the +//! whole bucket — every hive's data — while being correctly refused on a +//! neighbour's individual key. Granting it makes per-key scoping decorative. +//! +//! # Only publish is scoped +//! +//! Subscription permissions are left unrestricted. The queue's confidentiality +//! boundary is the account, and a hive reading another hive's *published* +//! status is not the problem this solves — writing it is. Scoping `sub` is a +//! separate change with its own measurement, and claiming it here without one +//! would be the same mistake as `$JS.API.>` above. + +/// The subjects an admitted client may publish to. +#[derive(Debug, PartialEq, Eq)] +pub struct Permissions { + /// Subjects allowed for publish. Never empty: an empty allow-list is a + /// grant that can do nothing, which is a denial wearing a grant's shape. + pub publish: Vec, +} + +/// Which client ids get which permissions. +/// +/// Constructed from configuration so that adding a subject a hive may publish +/// — a second event stream, say — is a deployment change rather than a change +/// to this responder. +#[derive(Debug, Clone)] +pub struct Policy { + hive_prefix: String, + bucket: String, + readers: Vec, + extra_hive_subjects: Vec, +} + +/// Placeholder replaced with the hive's own name in `extra_hive_subjects`. +const HIVE_PLACEHOLDER: &str = "{hive}"; + +impl Policy { + /// `hive_prefix` is the client-id prefix that marks a hive, `bucket` the + /// KV bucket hives report status in, `readers` the client ids allowed to + /// read every hive's key, and `extra_hive_subjects` additional subjects a + /// hive may publish to (with `{hive}` standing for its own name). + pub fn new( + hive_prefix: String, + bucket: String, + readers: Vec, + extra_hive_subjects: Vec, + ) -> Self { + Self { + hive_prefix, + bucket, + readers, + extra_hive_subjects, + } + } + + /// The permissions for `client_id`, or `None` when no rule matches. + /// + /// `None` is a denial. It is not "grant nothing and let them connect": + /// a connected client with no permissions still holds a slot and still + /// looks admitted in the logs, which is a worse answer than a refusal. + pub fn permissions(&self, client_id: &str) -> Option { + if let Some(hive) = self.hive_name(client_id) { + return Some(Permissions { + publish: self.hive_subjects(hive), + }); + } + if self.readers.iter().any(|r| r == client_id) { + return Some(Permissions { + publish: self.reader_subjects(), + }); + } + None + } + + /// The hive a client id names, when it names one. + /// + /// The prefix is configuration, not a pattern this module guesses at: the + /// authelia client is `hive-` while the KV key is the bare ``, + /// and stripping by eye is how a client id that merely *looks* + /// hive-shaped ends up with a hive's permissions. An id that is the prefix + /// and nothing else names no hive and is refused — `$KV..` is not + /// a narrower subject than `$KV..`, it is a different one. + fn hive_name<'a>(&self, client_id: &'a str) -> Option<&'a str> { + client_id + .strip_prefix(&self.hive_prefix) + .filter(|name| !name.is_empty()) + } + + fn stream(&self) -> String { + format!("KV_{}", self.bucket) + } + + /// Creating the bucket, which **both** ends need. + /// + /// `swarm_queue_client::status::open_or_create` is called by the hive that + /// writes and the controller that reads, because either may arrive first + /// on a fresh swarm — a bucket that must pre-exist turns "deployed in the + /// wrong order" into a permanent, silent absence of data. Whichever + /// connects first therefore has to be able to create it. + /// + /// Narrower than it looks: this is `CREATE` on one named stream, not + /// `UPDATE` and not the `$JS.API.>` wildcard. Creating a stream that + /// already exists with a different config is an error rather than a + /// rewrite, so a second arrival cannot reshape the bucket the first one + /// made. + fn create(&self) -> String { + format!("$JS.API.STREAM.CREATE.{}", self.stream()) + } + + fn hive_subjects(&self, hive: &str) -> Vec { + let mut subjects = vec![ + format!("$JS.API.STREAM.INFO.{}", self.stream()), + self.create(), + format!("$KV.{}.{hive}", self.bucket), + ]; + subjects.extend( + self.extra_hive_subjects + .iter() + .map(|s| s.replace(HIVE_PLACEHOLDER, hive)), + ); + subjects + } + + fn reader_subjects(&self) -> Vec { + let stream = self.stream(); + vec![ + format!("$JS.API.STREAM.INFO.{stream}"), + self.create(), + // The `.>` form specifically: the bare `$JS.API.DIRECT.GET.` + // is not the subject the client uses, and granting it was measured + // to make no difference. + format!("$JS.API.DIRECT.GET.{stream}.>"), + // `store.keys()` — the controller lists before it fetches, so a + // reader without this can get a key it already knows and discover + // nothing. + format!("$JS.API.CONSUMER.CREATE.{stream}.>"), + ] + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn policy() -> Policy { + Policy::new( + "hive-".to_owned(), + "hive-status".to_owned(), + vec!["swarm-controller".to_owned()], + Vec::new(), + ) + } + + #[test] + fn a_hive_may_publish_its_own_key_and_nothing_elses() { + let p = policy() + .permissions("hive-alpha") + .expect("a hive is admitted"); + assert!(p.publish.contains(&"$KV.hive-status.alpha".to_owned())); + assert!(!p.publish.iter().any(|s| s.contains("beta"))); + } + + #[test] + fn a_hive_grant_never_includes_the_jetstream_wildcard() { + // `$JS.API.>` also covers `$JS.API.STREAM.DELETE.KV_hive-status`, with + // which a hive refused on a neighbour's key can delete the entire + // bucket. Measured, not theorised - this test is the guard against it + // coming back as a convenience. + let p = policy() + .permissions("hive-alpha") + .expect("a hive is admitted"); + assert!(!p.publish.iter().any(|s| s.contains("$JS.API.>"))); + assert!(!p.publish.iter().any(|s| s.contains("STREAM.DELETE"))); + assert!(!p.publish.iter().any(|s| s.contains("STREAM.PURGE"))); + } + + #[test] + fn the_bucket_lookup_is_part_of_a_publishers_minimum() { + // Without it the put is refused: the client resolves the bucket before + // it writes. Dropping this line looks like tightening and is breaking. + let p = policy() + .permissions("hive-alpha") + .expect("a hive is admitted"); + assert!( + p.publish + .contains(&"$JS.API.STREAM.INFO.KV_hive-status".to_owned()) + ); + } + + #[test] + fn both_ends_may_create_the_bucket_but_not_reshape_it() { + // 🩸 The bug the subject measurements could not see: they ran against + // a bucket the *setup* had already created while unscoped, so + // "minimal" meant minimal-given-a-bucket-that-exists. `open_or_create` + // is called by both ends by design — whichever arrives first on a + // fresh swarm makes the bucket — so without CREATE a new swarm never + // gets one, and every other test still passes. + for client in ["hive-alpha", "swarm-controller"] { + let p = policy().permissions(client).expect("admitted"); + assert!( + p.publish + .contains(&"$JS.API.STREAM.CREATE.KV_hive-status".to_owned()), + "{client} cannot create the bucket on a fresh swarm" + ); + // CREATE is not UPDATE: a second arrival must not be able to + // reshape the bucket the first one made. + assert!(!p.publish.iter().any(|s| s.contains("STREAM.UPDATE"))); + } + } + + #[test] + fn a_reader_may_list_and_fetch_but_not_write() { + let p = policy() + .permissions("swarm-controller") + .expect("the reader is admitted"); + assert!( + p.publish + .contains(&"$JS.API.CONSUMER.CREATE.KV_hive-status.>".to_owned()) + ); + assert!( + p.publish + .contains(&"$JS.API.DIRECT.GET.KV_hive-status.>".to_owned()) + ); + assert!(!p.publish.iter().any(|s| s.starts_with("$KV."))); + } + + #[test] + fn an_unmatched_client_is_denied_not_granted_everything() { + // The whole policy is advisory if this returns a grant. + assert_eq!(policy().permissions("some-other-service"), None); + assert_eq!(policy().permissions(""), None); + } + + #[test] + fn the_prefix_alone_names_no_hive() { + // `hive-` would otherwise produce `$KV.hive-status.`, a subject nobody + // reviewed and that no hive owns. + assert_eq!(policy().permissions("hive-"), None); + } + + #[test] + fn extra_subjects_are_scoped_to_the_hive_that_publishes_them() { + // The extension point: a second stream (lifecycle notices, say) is + // published by the same `hive-` identity on a different subject. + // It has to land inside that hive's namespace, or one hive can write + // another's events even though its status key is scoped. + let p = Policy::new( + "hive-".to_owned(), + "hive-status".to_owned(), + Vec::new(), + vec!["$SWARM.events.{hive}.>".to_owned()], + ); + let g = p.permissions("hive-alpha").expect("a hive is admitted"); + assert!(g.publish.contains(&"$SWARM.events.alpha.>".to_owned())); + assert!(!g.publish.iter().any(|s| s.contains("{hive}"))); + } +} diff --git a/swarm-nats-auth/src/respond.rs b/swarm-nats-auth/src/respond.rs index e1473809..49402ed9 100644 --- a/swarm-nats-auth/src/respond.rs +++ b/swarm-nats-auth/src/respond.rs @@ -149,6 +149,14 @@ fn now_secs() -> i64 { struct UserNats { #[serde(rename = "type")] kind: &'static str, + /// Subjects this client may publish to. + /// + /// The server enforces what is minted here — measured, because a + /// non-operator server validates minted claims against its own config + /// rather than trusting them, and this responder's own history includes a + /// field the server rejected while the responder said `granted=true`. + #[serde(rename = "pub")] + publish: Permission, subs: i64, data: i64, payload: i64, @@ -156,12 +164,29 @@ struct UserNats { version: i64, } +/// A NATS permission block. +/// +/// Only `allow` is modelled. NATS also takes `deny`, and a struct that has it +/// is a struct someone will use: an allow-list plus a deny-list has two places +/// deciding the same question, and the interaction between them is a thing to +/// remember rather than to read. +#[derive(Serialize)] +struct Permission { + allow: Vec, +} + /// Mint the user JWT an admitted client presents. /// /// `account` is the account **name** from the server's `accounts` block (the /// module's `clientAccount`), not a public key — in config mode the server /// resolves `aud` against its own config rather than against a key. -fn user_jwt(now: i64, issuer: &KeyPair, account: &str, user_nkey: &str) -> String { +fn user_jwt( + now: i64, + issuer: &KeyPair, + account: &str, + user_nkey: &str, + publish: Vec, +) -> String { sign( Claims { iat: now, @@ -172,6 +197,7 @@ fn user_jwt(now: i64, issuer: &KeyPair, account: &str, user_nkey: &str) -> Strin aud: Some(account.to_owned()), nats: UserNats { kind: "user", + publish: Permission { allow: publish }, subs: -1, data: -1, payload: -1, @@ -183,10 +209,21 @@ fn user_jwt(now: i64, issuer: &KeyPair, account: &str, user_nkey: &str) -> Strin ) } -/// Grant: mint a user JWT placing the client in `account` and wrap it. -pub fn grant(issuer: &KeyPair, account: &str, server_id: &str, user_nkey: &str) -> String { +/// Grant: mint a user JWT placing the client in `account`, scoped to +/// `permissions`, and wrap it. +/// +/// Taking the permissions by value rather than defaulting them is the point: +/// there is no way to call this and get an unscoped grant by omission, so a +/// future caller cannot forget the argument that makes the scoping real. +pub fn grant( + issuer: &KeyPair, + account: &str, + server_id: &str, + user_nkey: &str, + permissions: &crate::policy::Permissions, +) -> String { let now = now_secs(); - let jwt = user_jwt(now, issuer, account, user_nkey); + let jwt = user_jwt(now, issuer, account, user_nkey, permissions.publish.clone()); response(now, issuer, server_id, user_nkey, Ok(jwt)) } @@ -304,10 +341,38 @@ mod tests { assert_eq!(claims["aud"], "NSERVER"); } + /// The permissions a test grant carries. Any non-empty set will do for + /// the wrapper-shape assertions; `policy.rs` owns what the real ones are. + fn perms() -> crate::policy::Permissions { + crate::policy::Permissions { + publish: vec!["$KV.hive-status.alpha".to_owned()], + } + } + + #[test] + fn the_issued_user_jwt_carries_the_permissions_it_was_given() { + // The scoping is only real if it survives into the minted token. A + // policy that computes the right subjects and a grant that drops them + // look identical from every test that stops at the policy. + let account = KeyPair::new_account(); + let wrapper = decode_claims(&grant(&account, "APP", "NSERVER", "UCLIENT", &perms())); + let user = decode_claims(wrapper["nats"]["jwt"].as_str().expect("a user jwt")); + assert_eq!(user["nats"]["pub"]["allow"][0], "$KV.hive-status.alpha"); + // An empty or absent allow-list is NATS' "everything": the one shape + // that turns this whole change into a no-op while every other + // assertion still passes. + assert!( + user["nats"]["pub"]["allow"] + .as_array() + .is_some_and(|a| !a.is_empty()), + "an empty pub.allow is an unscoped grant" + ); + } + #[test] fn a_grant_carries_a_user_jwt_and_no_error() { let account = KeyPair::new_account(); - let claims = decode_claims(&grant(&account, "APP", "NSERVER", "UCLIENT")); + let claims = decode_claims(&grant(&account, "APP", "NSERVER", "UCLIENT", &perms())); assert!(claims["nats"]["error"].is_null()); assert!( claims["nats"]["jwt"] @@ -331,7 +396,7 @@ mod tests { #[test] fn the_issued_user_jwt_names_its_account_by_aud_and_sets_no_issuer_account() { let account = KeyPair::new_account(); - let wrapper = decode_claims(&grant(&account, "APP", "NSERVER", "UCLIENT")); + let wrapper = decode_claims(&grant(&account, "APP", "NSERVER", "UCLIENT", &perms())); let user = decode_claims(wrapper["nats"]["jwt"].as_str().expect("a user jwt")); // In server-config mode the account is named by `aud`... diff --git a/swarm-queue-client/src/lib.rs b/swarm-queue-client/src/lib.rs index b5c3b193..1c03a317 100644 --- a/swarm-queue-client/src/lib.rs +++ b/swarm-queue-client/src/lib.rs @@ -144,9 +144,16 @@ pub fn chain(error: &dyn std::error::Error) -> String { } /// The hive-status KV bucket, shared by the hive that writes it and the -/// controller that reads it. Behind the `kv` feature — see the module doc -/// for why a bucket name and its config belong to neither end alone. -#[cfg(feature = "kv")] +/// controller that reads it. See the module doc for why a bucket name and +/// its config belong to neither end alone. +/// +/// The module itself is unconditional; only the parts that *open* the bucket +/// need the `kv` feature. The name is a `&str` with no dependencies, and a +/// third end names it too — the auth-callout responder, which derives the +/// subjects a hive may publish to from it without ever speaking `jetstream`. +/// Gating the name behind `kv` would have forced that consumer to choose +/// between pulling a JetStream stack it does not use and copying the literal, +/// which is the disagreement this module exists to prevent. pub mod status; /// Only the fields this needs; authelia returns several. diff --git a/swarm-queue-client/src/status.rs b/swarm-queue-client/src/status.rs index 1c8689b2..d4f96c2a 100644 --- a/swarm-queue-client/src/status.rs +++ b/swarm-queue-client/src/status.rs @@ -15,10 +15,15 @@ //! nobody chose. Sharing the constructor makes the race have one outcome //! instead of two. //! -//! Feature-gated (`kv`) so the crate's other consumer, the auth-callout -//! responder, still pulls neither `jetstream` nor `kv`: it speaks the -//! connect and nothing else. +//! The bucket *name* is unconditional; only [`open_or_create`] is behind the +//! `kv` feature. A third end names the bucket without ever opening it — the +//! auth-callout responder, which derives the subjects a hive may publish to +//! from it — and it speaks neither `jetstream` nor `kv`. Gating the name too +//! would have made that consumer choose between a JetStream stack it does not +//! use and a copied literal, and a copied literal is precisely the agreement +//! nothing checks. +#[cfg(feature = "kv")] use crate::Error; /// The KV bucket hives publish their status snapshots into, one key per @@ -39,6 +44,7 @@ pub const BUCKET: &str = "hive-status"; /// controller and the hives come up in no particular order, and a bucket /// that must pre-exist turns "the swarm was deployed in the wrong order" /// into a permanent, silent absence of data. +#[cfg(feature = "kv")] pub async fn open_or_create( client: &async_nats::Client, ) -> Result {