diff --git a/apps/gateway/Cargo.lock b/apps/gateway/Cargo.lock index 052747ae..b18a2ad5 100644 --- a/apps/gateway/Cargo.lock +++ b/apps/gateway/Cargo.lock @@ -1234,6 +1234,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7cd812cc2bc1d69d4764bd80df88b4317eaef9e773c75226407d9bc0876b211c" dependencies = [ "powerfmt", + "serde_core", ] [[package]] @@ -3452,6 +3453,7 @@ dependencies = [ "sha2", "smallvec", "thiserror 2.0.18", + "time", "tokio", "tokio-stream", "tracing", @@ -3535,6 +3537,7 @@ dependencies = [ "sqlx-core", "stringprep", "thiserror 2.0.18", + "time", "tracing", "whoami", ] @@ -3572,6 +3575,7 @@ dependencies = [ "sqlx-core", "stringprep", "thiserror 2.0.18", + "time", "tracing", "whoami", ] @@ -3596,6 +3600,7 @@ dependencies = [ "serde_urlencoded", "sqlx-core", "thiserror 2.0.18", + "time", "tracing", "url", ] diff --git a/apps/gateway/Cargo.toml b/apps/gateway/Cargo.toml index def10715..4a3bdab2 100644 --- a/apps/gateway/Cargo.toml +++ b/apps/gateway/Cargo.toml @@ -83,11 +83,16 @@ futures-util = "0.3" # Error handling anyhow = "1" -# Time (for certificate validity + RFC 3339 parsing) -time = { version = "0.3", features = ["parsing"] } - -# Direct database access -sqlx = { version = "0.8", features = ["runtime-tokio", "tls-rustls", "postgres"] } +# Time (for certificate validity + RFC 3339 parsing). `serde` (Phase 5): lets +# `db::ClientHostRow.revoked_at` (an `Option`) ride the same +# JSON cache-serialization path every other cached DB row uses. +time = { version = "0.3", features = ["parsing", "serde"] } + +# Direct database access. `time` (Phase 5): maps the Postgres TIMESTAMP column +# (`client_hosts.revoked_at`, no time zone) to `time::PrimitiveDateTime` for +# `db::ClientHostRow` — sqlx maps TIMESTAMP→PrimitiveDateTime (TIMESTAMPTZ would +# be OffsetDateTime). +sqlx = { version = "0.8", features = ["runtime-tokio", "tls-rustls", "postgres", "time"] } # Agent Access (Bitwarden vault credential injection) ap-client = "0.9.0" diff --git a/apps/gateway/src/binding.rs b/apps/gateway/src/binding.rs new file mode 100644 index 00000000..cd6ad4c6 --- /dev/null +++ b/apps/gateway/src/binding.rs @@ -0,0 +1,495 @@ +//! Phase 5 (final phase of the remote-gateway hardening series): cert↔token +//! tenant binding. Where Phase 1-3 threaded an mTLS `ClientIdentity` onto +//! [`crate::gateway::ProxyContext`] and logged it, and Phase 2 minted +//! certificates carrying it, this module is the first to ever COMPARE it +//! against anything — a relay's client-certificate identity may only carry +//! agent tokens for its own tenant. +//! +//! Everything here is pure and DB-free: [`evaluate`] takes an +//! already-resolved [`crate::db::ClientHostRow`] lookup result and returns a +//! [`BindingDecision`]. The I/O (the cached DB lookup, and turning a `Deny` +//! into an actual HTTP response) lives in `gateway.rs`'s `enforce_binding`, +//! which is the ONLY caller of `evaluate` — so the CONNECT and HTTP-proxy +//! entry points can never drift from each other or from this decision table. +//! +//! # The binding rule +//! +//! A request is permitted iff the `ClientHost` resolved from the cert +//! identity's spiffe URI has `project_id == token.project_id`, is not +//! revoked, and — when it also carries an `organization_id` — that value +//! equals the token's `organization_id` too (a redundant consistency check; +//! project scoping alone is already sufficient, since a project belongs to +//! exactly one org). The `ClientHost` row IS the allowlist entry: no row for +//! this spiffe URI means no permission, full stop (deny-unless-permitted, +//! never a blocklist). +//! +//! # Modes +//! +//! `GATEWAY_BINDING_ENFORCEMENT` ([`BindingMode::from_env`]) defaults to +//! [`BindingMode::Off`] — unset, this module is a complete no-op and the +//! gateway behaves exactly as every prior phase left it. `Log` computes the +//! same decision as `Enforce` but never denies — a would-be deny becomes +//! [`BindingDecision::WouldDeny`], which the caller logs and then allows +//! anyway, so an operator can watch what enforcement WOULD do before +//! flipping the switch. `Enforce` denies for real. +//! +//! # The two subtleties this module (and its caller) must get right +//! +//! 1. Exemption is by LISTENER KIND (`on_mtls`), never by +//! `client_identity.is_some()`. An mTLS handshake whose cert has an +//! unparseable CN/SAN also yields no identity — and that case must be +//! DENIED in enforce (nothing in the cert can satisfy the allowlist), not +//! waved through as if it came in on the plain listener. +//! 2. The plain listener (web-app / loopback / all-in-one local-agent path, +//! no cert involved at all) is exempt in EVERY mode — enforcing there +//! would deny all of that traffic, which never had a certificate to bind +//! in the first place. + +use crate::db::ClientHostRow; + +// ── Mode ────────────────────────────────────────────────────────────────── + +/// Enforcement posture, read once at startup from `GATEWAY_BINDING_ENFORCEMENT`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum BindingMode { + /// No-op: `evaluate` is never even called (the caller short-circuits + /// before doing so) — zero DB/cache overhead beyond a mode check. + /// The default, so an unset var is byte-for-byte backward compatible. + Off, + /// Compute the real decision, but a would-be deny only logs — the + /// request is still allowed. Lets an operator watch what `Enforce` would + /// do before flipping to it. + Log, + /// Deny for real. + Enforce, +} + +impl BindingMode { + /// Parse an already-read `GATEWAY_BINDING_ENFORCEMENT` value. No env + /// access — this is the unit-testable core; [`Self::from_env`] is the + /// thin wrapper that reads the var and forwards here (mirroring the + /// `from_parts`/`from_env` split in `client_ca.rs`). + /// + /// Unrecognized text (including an explicitly-set empty string) is + /// treated as `Off`, but — unlike a genuinely UNSET var — with a warning: + /// an operator who set the var at all almost certainly meant something by + /// it, and a silent fall-through to "disabled" on a typo would be exactly + /// the kind of misconfiguration this feature exists to prevent going + /// unnoticed. + fn from_value(value: &str) -> Self { + match value.trim().to_ascii_lowercase().as_str() { + "off" => BindingMode::Off, + "log" => BindingMode::Log, + "enforce" => BindingMode::Enforce, + other => { + tracing::warn!( + value = %other, + "GATEWAY_BINDING_ENFORCEMENT: unrecognized value, defaulting to off" + ); + BindingMode::Off + } + } + } + + /// Read `GATEWAY_BINDING_ENFORCEMENT` from the environment. Unset (the + /// common case, and every deployment prior to Phase 5) resolves to `Off` + /// with no warning at all — that is simply the default, not a + /// misconfiguration. + pub(crate) fn from_env() -> Self { + match std::env::var("GATEWAY_BINDING_ENFORCEMENT") { + Ok(value) => Self::from_value(&value), + Err(_) => BindingMode::Off, + } + } +} + +// ── Decision ───────────────────────────────────────────────────────────── + +/// The reason string used when the host-tenant lookup itself failed (a DB or +/// cache error, as opposed to a permanent "no such host" / "wrong tenant" +/// verdict). `gateway.rs`'s `enforce_binding` matches on this exact value to +/// choose 502 (retryable) over 403 (permanent) — see its doc comment. +pub(crate) const REASON_HOST_LOOKUP_ERROR: &str = "host_lookup_error"; + +/// Outcome of [`evaluate`]. `Deny` and `WouldDeny` carry the same permanent, +/// human-readable reasons — the only difference is what the caller does with +/// them (deny the request vs. log-and-allow it). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum BindingDecision { + /// Permitted — proceed. + Allow, + /// Denied for real (mode = `Enforce`). + Deny { reason: &'static str }, + /// Would have been denied under `Enforce`, but mode = `Log`: the caller + /// logs this and allows the request anyway. + WouldDeny { reason: &'static str }, +} + +/// The pure decision core. No I/O: `host_tenant` is the ALREADY-RESOLVED +/// result of looking up the cert identity's spiffe URI against `client_hosts` +/// (`Err(())` for "the lookup itself failed", not "no matching row" — that's +/// `Ok(None)`). +/// +/// Evaluated in this order: +/// 1. `mode == Off` → `Allow`, unconditionally — this module never even looks +/// at the other arguments in that case (the caller shouldn't either; see +/// `gateway.rs`'s `enforce_binding`, which skips the lookup entirely). +/// 2. `!on_mtls` → `Allow` — the plain listener is exempt BY LISTENER KIND in +/// every mode (subtlety #2 in the module doc). Checked before `identity` +/// so a plain-listener request is never denied no matter what (there is +/// no cert at all on that path). +/// 3. `identity.is_none()` (only reachable when `on_mtls` is true) → deny — +/// an mTLS handshake that verified but whose CN/SAN was unparseable has +/// nothing to satisfy the allowlist with (subtlety #1). +/// 4. `host_tenant` is `Err(())` (the lookup failed) → deny, fail-closed — +/// an outage must never silently become an allow. +/// 5. `host_tenant` is `Ok(None)` (no `client_hosts` row for this spiffe URI) +/// → deny — deny-unless-permitted; the row IS the allowlist entry. +/// 6. The row is revoked (`revoked_at.is_some()`) → deny. +/// 7. `row.project_id != token_project` → deny. +/// 8. The row carries a non-null `organization_id` that disagrees with +/// `token_org` → deny (the redundant consistency check). +/// 9. Otherwise → `Allow`. +/// +/// In `Log` mode every deny reached via steps 3-8 becomes `WouldDeny` +/// instead of `Deny` — the caller allows the request and logs what would +/// have happened. `Enforce` returns `Deny` for the same steps. +pub(crate) fn evaluate( + mode: BindingMode, + on_mtls: bool, + identity: Option<&str>, + host_tenant: Result, ()>, + token_project: &str, + token_org: Option<&str>, +) -> BindingDecision { + if matches!(mode, BindingMode::Off) { + return BindingDecision::Allow; + } + + // Plain-listener requests are exempt BY LISTENER KIND, in every mode — + // never inferred from `identity.is_none()` (see step 3 below, and the + // module doc's subtlety #1). + if !on_mtls { + return BindingDecision::Allow; + } + + // Local closure so every deny path below picks Deny-vs-WouldDeny the + // same way, from one place — the mode check can never drift between them. + let deny = |reason: &'static str| -> BindingDecision { + if matches!(mode, BindingMode::Log) { + BindingDecision::WouldDeny { reason } + } else { + BindingDecision::Deny { reason } + } + }; + + let Some(_identity) = identity else { + // An mTLS handshake with no extractable identity (unparseable CN/SAN) + // can never satisfy the allowlist — deny, not exempt. + return deny("missing_identity"); + }; + + let host = match host_tenant { + Err(()) => return deny(REASON_HOST_LOOKUP_ERROR), + Ok(None) => return deny("unknown_host"), + Ok(Some(host)) => host, + }; + + if host.revoked_at.is_some() { + return deny("host_revoked"); + } + + if host.project_id != token_project { + return deny("project_mismatch"); + } + + if let Some(host_org) = host.organization_id.as_deref() { + if Some(host_org) != token_org { + return deny("organization_mismatch"); + } + } + + BindingDecision::Allow +} + +// ── Tests ─────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + /// A `PrimitiveDateTime` stand-in for "now" — `revoked_at` is + /// `TIMESTAMP` (no time zone; see `db::ClientHostRow`'s doc comment), and + /// `evaluate` only ever reads `.is_some()`, so the actual instant is + /// irrelevant here. + fn some_primitive_datetime() -> time::PrimitiveDateTime { + let now = time::OffsetDateTime::now_utc(); + time::PrimitiveDateTime::new(now.date(), now.time()) + } + + fn host(project_id: &str, organization_id: Option<&str>, revoked: bool) -> ClientHostRow { + ClientHostRow { + project_id: project_id.to_string(), + organization_id: organization_id.map(str::to_string), + revoked_at: revoked.then(some_primitive_datetime), + } + } + + // ── from_value / from_env ──────────────────────────────────────────── + + #[test] + fn from_value_off() { + assert_eq!(BindingMode::from_value("off"), BindingMode::Off); + assert_eq!(BindingMode::from_value("Off"), BindingMode::Off); + assert_eq!(BindingMode::from_value(" OFF "), BindingMode::Off); + } + + #[test] + fn from_value_log() { + assert_eq!(BindingMode::from_value("log"), BindingMode::Log); + assert_eq!(BindingMode::from_value("LOG"), BindingMode::Log); + } + + #[test] + fn from_value_enforce() { + assert_eq!(BindingMode::from_value("enforce"), BindingMode::Enforce); + assert_eq!(BindingMode::from_value("Enforce"), BindingMode::Enforce); + } + + #[test] + fn from_value_unknown_defaults_to_off() { + assert_eq!(BindingMode::from_value("bogus"), BindingMode::Off); + } + + #[test] + fn from_value_empty_defaults_to_off() { + assert_eq!(BindingMode::from_value(""), BindingMode::Off); + } + + // ── evaluate: mode gating ───────────────────────────────────────────── + + #[test] + fn off_mode_always_allows_no_matter_what_else_is_wrong() { + // Even a flagrant mismatch is allowed when the mode is Off — this + // module must be a total no-op in that mode. + let h = host("proj-B", None, false); + let decision = evaluate( + BindingMode::Off, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + None, + ); + assert_eq!(decision, BindingDecision::Allow); + } + + #[test] + fn plain_listener_is_exempt_regardless_of_mode_or_mismatch() { + // `on_mtls = false` must allow even under Enforce, even with a + // mismatching (or entirely absent) host tenant — subtlety #2. + for mode in [BindingMode::Log, BindingMode::Enforce] { + let decision = evaluate(mode, false, None, Ok(None), "proj-A", None); + assert_eq!(decision, BindingDecision::Allow, "mode={mode:?}"); + } + } + + // ── evaluate: identity ──────────────────────────────────────────────── + + #[test] + fn mtls_with_no_identity_is_denied_not_exempt() { + // Subtlety #1: on_mtls=true but identity=None must DENY, not be + // treated as if it came in on the plain listener. + let decision = evaluate(BindingMode::Enforce, true, None, Ok(None), "proj-A", None); + assert_eq!( + decision, + BindingDecision::Deny { + reason: "missing_identity" + } + ); + } + + // ── evaluate: lookup outcomes ───────────────────────────────────────── + + #[test] + fn db_error_denies_fail_closed() { + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Err(()), + "proj-A", + None, + ); + assert_eq!( + decision, + BindingDecision::Deny { + reason: REASON_HOST_LOOKUP_ERROR + } + ); + } + + #[test] + fn unknown_host_denies() { + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Ok(None), + "proj-A", + None, + ); + assert_eq!( + decision, + BindingDecision::Deny { + reason: "unknown_host" + } + ); + } + + #[test] + fn revoked_host_denies_even_with_matching_project() { + let h = host("proj-A", None, true); + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + None, + ); + assert_eq!( + decision, + BindingDecision::Deny { + reason: "host_revoked" + } + ); + } + + // ── evaluate: the binding rule itself ──────────────────────────────── + + #[test] + fn matching_project_no_org_on_host_allows() { + let h = host("proj-A", None, false); + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + Some("org-1"), + ); + assert_eq!(decision, BindingDecision::Allow); + } + + #[test] + fn matching_project_and_org_allows() { + let h = host("proj-A", Some("org-1"), false); + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + Some("org-1"), + ); + assert_eq!(decision, BindingDecision::Allow); + } + + #[test] + fn project_mismatch_denies() { + let h = host("proj-B", None, false); + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + None, + ); + assert_eq!( + decision, + BindingDecision::Deny { + reason: "project_mismatch" + } + ); + } + + #[test] + fn org_present_on_host_but_mismatched_denies_even_with_matching_project() { + let h = host("proj-A", Some("org-1"), false); + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + Some("org-2"), + ); + assert_eq!( + decision, + BindingDecision::Deny { + reason: "organization_mismatch" + } + ); + } + + #[test] + fn org_present_on_host_but_token_org_none_denies() { + let h = host("proj-A", Some("org-1"), false); + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + None, + ); + assert_eq!( + decision, + BindingDecision::Deny { + reason: "organization_mismatch" + } + ); + } + + // ── evaluate: per-mode behavior on an identical mismatch ───────────── + + #[test] + fn log_mode_would_deny_instead_of_deny() { + let h = host("proj-B", None, false); + let decision = evaluate( + BindingMode::Log, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + None, + ); + assert_eq!( + decision, + BindingDecision::WouldDeny { + reason: "project_mismatch" + } + ); + } + + #[test] + fn enforce_mode_denies() { + let h = host("proj-B", None, false); + let decision = evaluate( + BindingMode::Enforce, + true, + Some("spiffe://onecli/host/1"), + Ok(Some(&h)), + "proj-A", + None, + ); + assert_eq!( + decision, + BindingDecision::Deny { + reason: "project_mismatch" + } + ); + } +} diff --git a/apps/gateway/src/db.rs b/apps/gateway/src/db.rs index c99c9af8..d7c1afc8 100644 --- a/apps/gateway/src/db.rs +++ b/apps/gateway/src/db.rs @@ -80,6 +80,64 @@ pub(crate) struct OrgApiKeyRow { pub organization_id: String, } +/// A `client_hosts` row (Phase 2 mTLS enrollment; Phase 5 binding +/// enforcement) — the allowlist entry `binding::evaluate` checks a cert +/// identity's tenant against. One row per enrolled host, keyed by its unique +/// `spiffe_uri`; see the model's doc comment in `schema.prisma` for the +/// enrollment/renewal semantics. `Serialize`/`Deserialize` so it can ride the +/// same JSON cache-serialization path as `connect::ConnectResponse` +/// (`gateway.rs`'s `resolve_host_tenant` caches `Option` — +/// including the negative "no such host" result — for 60s). +#[derive(Debug, Clone, PartialEq, FromRow, serde::Serialize, serde::Deserialize)] +pub(crate) struct ClientHostRow { + pub project_id: String, + pub organization_id: Option, + /// `Some` means revoked (the timestamp itself is never read — only + /// presence/absence matters to `binding::evaluate`). + /// + /// Deliberately `PrimitiveDateTime`, NOT `OffsetDateTime`: the column is + /// `TIMESTAMP(3)` — WITHOUT time zone (see the migration in + /// `packages/db/prisma/migrations/20260730155821_add_client_hosts/migration.sql`) + /// — and sqlx-postgres maps `TIMESTAMP` to `PrimitiveDateTime` / + /// `TIMESTAMPTZ` to `OffsetDateTime`. Decoding a non-null value through + /// the wrong one of the two is a hard decode error at the `sqlx::FromRow` + /// level, not silently wrong data — `OffsetDateTime` here would only ever + /// have been exercised the first time a real row had a non-null + /// `revoked_at`, at which point every lookup for that host would fail + /// closed as a (wrongly retryable) 502 instead of a 403 `host_revoked` + /// deny. Since only `.is_some()` is ever read, the timezone this type + /// carries (none) is irrelevant to `binding::evaluate` — this is the + /// minimal fix, no schema/migration change needed. + /// + /// Serialized via `time`'s own `serde` impl (its internal representation, + /// not RFC 3339 text) — fine for a value that only ever round-trips + /// through this process's own cache and is never read by anything else. + pub revoked_at: Option, +} + +/// Look up a `client_hosts` row by its unique `spiffe_uri` — the identity +/// carried in a client certificate's URI SAN (`ClientIdentity::primary()`). +/// +/// BIND ONLY: `spiffe_uri` is attacker-controlled (it comes from a +/// certificate's SAN, which anyone holding a cert signed by the configured +/// client CA picks themselves) and is never string-interpolated into the +/// query. Looked up by the unique column directly — the `` embedded in +/// `spiffe://onecli/host/` is never parsed back out of the string; the +/// whole URI is the lookup key, matching how `ensureClientHost` (Node side) +/// stores it. +pub(crate) async fn find_client_host_by_spiffe( + pool: &PgPool, + spiffe_uri: &str, +) -> Result> { + sqlx::query_as::<_, ClientHostRow>( + r#"SELECT project_id, organization_id, revoked_at FROM client_hosts WHERE spiffe_uri = $1 LIMIT 1"#, + ) + .bind(spiffe_uri) + .fetch_optional(pool) + .await + .context("querying client_hosts by spiffe_uri") +} + /// A vault connection row from the `vault_connections` table. #[derive(Debug, FromRow)] #[allow(dead_code)] @@ -887,3 +945,75 @@ pub(crate) async fn delete_vault_connection( .context("deleting vault_connection")?; Ok(()) } + +// ── Tests ─────────────────────────────────────────────────────────────── +// +// This module has no live-Postgres test tier — every query function here +// takes a `&PgPool` and needs a real server to exercise. (The web side has +// a `*.pg.test.ts` convention for that; this crate has no Rust equivalent — +// no `#[ignore]`d live-DB tests, no `sqlx::test`, nothing gated behind a +// `DATABASE_URL`.) That means the actual `sqlx::FromRow` DECODE of a +// `client_hosts` row — the thing FIX 1 was about — is NOT exercised by +// anything below or anywhere else in this crate. What IS tested here is the +// OTHER place `ClientHostRow` has to round-trip correctly: the JSON +// cache-serialization path `gateway.rs`'s `resolve_host_tenant` uses +// (`CacheStore::set`/`get`, which go through `serde_json` under the hood — +// see `cache.rs`). If a live-DB test tier is ever added to this crate, the +// test this really needs is: insert a `client_hosts` row with a non-null +// `revoked_at`, call `find_client_host_by_spiffe`, and assert it decodes +// without error (rather than the `Err` FIX 1 fixed). +#[cfg(test)] +mod tests { + use super::*; + + /// A `PrimitiveDateTime` stand-in for "now" — see `ClientHostRow`'s doc + /// comment for why the field is `PrimitiveDateTime`, not `OffsetDateTime`. + fn some_primitive_datetime() -> time::PrimitiveDateTime { + let now = time::OffsetDateTime::now_utc(); + time::PrimitiveDateTime::new(now.date(), now.time()) + } + + /// Proves `ClientHostRow` — with a NON-NULL `revoked_at`, the exact value + /// FIX 1 was about — round-trips through `serde_json` unchanged. This is + /// the real code path `gateway.rs`'s `resolve_host_tenant` depends on + /// (`CacheStore::set`/`get`, both JSON under the hood): a positive cache + /// hit for a revoked host must deserialize back to `revoked_at: Some(_)`, + /// not silently lose it or fail to deserialize. + /// + /// This does NOT exercise the sqlx-postgres wire decode from an actual + /// `TIMESTAMP` column — see the module doc above for why that gap exists + /// and what would close it. + #[test] + fn client_host_row_json_round_trip_with_revoked_at_present() { + let row = ClientHostRow { + project_id: "proj-A".to_string(), + organization_id: Some("org-1".to_string()), + revoked_at: Some(some_primitive_datetime()), + }; + + let json = serde_json::to_string(&row).expect("serialize"); + let round_tripped: ClientHostRow = serde_json::from_str(&json).expect("deserialize"); + + assert_eq!(round_tripped, row); + assert!(round_tripped.revoked_at.is_some()); + } + + /// The negative-cache counterpart: `revoked_at: None` must also round-trip + /// — `resolve_host_tenant` caches `Option` (including the + /// "unknown host" `None` case) the same way regardless of which field is + /// null. + #[test] + fn client_host_row_json_round_trip_with_revoked_at_absent() { + let row = ClientHostRow { + project_id: "proj-A".to_string(), + organization_id: None, + revoked_at: None, + }; + + let json = serde_json::to_string(&row).expect("serialize"); + let round_tripped: ClientHostRow = serde_json::from_str(&json).expect("deserialize"); + + assert_eq!(round_tripped, row); + assert!(round_tripped.revoked_at.is_none()); + } +} diff --git a/apps/gateway/src/gateway.rs b/apps/gateway/src/gateway.rs index 3443544f..36b4a4d6 100644 --- a/apps/gateway/src/gateway.rs +++ b/apps/gateway/src/gateway.rs @@ -59,6 +59,7 @@ use tracing::{debug, info, info_span, warn, Instrument}; use crate::approval::{ApprovalDecision, ApprovalStore, APPROVAL_TIMEOUT_SECS}; use crate::auth::AuthUser; +use crate::binding::{self, BindingMode}; use crate::ca::CertificateAuthority; use crate::cache::CacheStore; use crate::client_ca::{self, ClientIdentity, MtlsConfig}; @@ -93,10 +94,14 @@ pub(crate) struct ProxyContext { pub agent_identifier: Option, pub agent_token: Option, /// Identity extracted from the client's mTLS certificate, when the - /// connection came in on the mTLS listener. Phase 1 only threads and logs - /// this (the log statements read the identity before it's moved in here) - /// — it is never compared against `agent_token`; Phase 2 is the first - /// reader of the field itself, hence the lint allowance below. + /// connection came in on the mTLS listener. Phase 5's `enforce_binding` + /// is the first to actually compare an identity against the agent + /// token's tenant — but it does so from the `client_identity` local + /// variable BEFORE this `ProxyContext` is built (see `handle_connect` / + /// `handle_http_proxy`), so this field itself is still carried onward + /// only for a future consumer (e.g. session-level logging inside + /// `mitm`/`forward`) rather than read anywhere today, hence the lint + /// allowance below. #[allow(dead_code)] pub client_identity: Option>, } @@ -134,6 +139,10 @@ pub(crate) struct GatewayState { /// CA would produce certificates nobody trusts, so `issue_client_cert` /// returns 503 rather than silently minting from the wrong CA. pub client_ca: Option>, + /// Phase 5 cert↔token tenant binding enforcement posture, read once at + /// startup from `GATEWAY_BINDING_ENFORCEMENT` (default `Off` — see + /// `binding::BindingMode`). Consulted by `enforce_binding`. + pub binding_mode: BindingMode, } // ── GatewayServer ─────────────────────────────────────────────────────── @@ -311,6 +320,21 @@ fn parse_plain_bind() -> Result { parse_plain_bind_value(std::env::var("GATEWAY_PLAIN_BIND").ok().as_deref()) } +/// Whether the plaintext listener's bind address defeats Phase 5 enforcement: +/// true when `binding_mode` is `Enforce` AND `plain_bind` is anything OTHER +/// than loopback. `enforce_binding` exempts the plain listener by design (see +/// its doc comment) — that is only safe when the plain listener itself is +/// unreachable from wherever an attacker sits. Loopback (`127.0.0.0/8` / +/// `::1`, via `IpAddr::is_loopback`) is the only address this crate can prove +/// is host-local from the bind address alone; anything else — including the +/// wildcard `0.0.0.0`/`::` AND a specific-looking but still off-host-reachable +/// address (a pod/cluster IP) — must warn, since both are equally reachable +/// from outside this process for the purpose of skipping the mTLS port +/// entirely. No env access, so this is directly unit-testable. +fn plain_bind_bypasses_binding_enforcement(binding_mode: BindingMode, plain_bind: IpAddr) -> bool { + matches!(binding_mode, BindingMode::Enforce) && !plain_bind.is_loopback() +} + /// Returns true if `host` matches any pattern in `patterns`. /// /// - `*.example.com` matches `sub.example.com` but NOT `example.com` itself. @@ -340,6 +364,7 @@ impl GatewayServer { approval_store: Arc, mtls: Option, client_ca: Option>, + binding_mode: BindingMode, ) -> Result { let global_skip = std::env::var("GATEWAY_DANGER_ACCEPT_INVALID_CERTS").is_ok(); let skip_verify_hosts = Arc::new(parse_skip_verify_hosts()); @@ -360,6 +385,26 @@ impl GatewayServer { vault/approval/cache calls, which arrive on the plaintext listener." ); } + // Phase 5: enforcement is only as strong as the listener it applies + // to. `enforce_binding` exempts the plain listener BY DESIGN (it has + // no client certificate to bind at all) — but if that listener is + // reachable from anywhere an attacker can reach, they simply skip the + // mTLS port and the binding check entirely. Broader than the mTLS + // warning above (which only fires on the literal unspecified address, + // 0.0.0.0): ANY non-loopback bind — including a specific, deliberately + // "reachable" address like a pod/cluster IP — still lets the plain + // listener see traffic from off-host, so it warns on anything that + // isn't loopback, not just the wildcard address. + if plain_bind_bypasses_binding_enforcement(binding_mode, plain_bind) { + warn!( + "GATEWAY_BINDING_ENFORCEMENT=enforce is set but the plaintext listener is \ + bound to a non-loopback address ({plain_bind}) — cert↔token binding is only \ + checked on the mTLS listener, so anyone who can reach the plaintext port \ + bypasses it entirely, same as the mTLS bypass warning above. Restrict \ + GATEWAY_PLAIN_BIND to loopback (127.0.0.1) or another trusted-network-only \ + address (same tradeoff noted above applies)." + ); + } let state = GatewayState { ca: Arc::new(ca), @@ -373,6 +418,7 @@ impl GatewayServer { vault_service, approval_store, client_ca, + binding_mode, }; Ok(Self { @@ -642,15 +688,28 @@ async fn accept_loop( .and_then(client_ca::identity_from_peer_certs) .map(Arc::new); - if let Err(e) = - handle_connection(tls_stream, peer_addr, state, router, client_identity) - .await + if let Err(e) = handle_connection( + tls_stream, + peer_addr, + state, + router, + client_identity, + // `on_mtls` is threaded as its OWN signal, separate + // from `client_identity` — an mTLS handshake whose + // cert had an unparseable CN/SAN also yields `None` + // above, and Phase 5's `enforce_binding` must still + // see "this came in on the mTLS listener" for that + // case (see `binding.rs`'s module doc, subtlety #1). + true, + ) + .await { warn!(peer = %peer_addr, error = ?e, "connection error"); } } None => { - if let Err(e) = handle_connection(stream, peer_addr, state, router, None).await + if let Err(e) = + handle_connection(stream, peer_addr, state, router, None, false).await { warn!(peer = %peer_addr, error = ?e, "connection error"); } @@ -1097,6 +1156,13 @@ async fn handle_connection( state: GatewayState, router: Router, client_identity: Option>, + // Whether this connection came in on the mTLS listener — set by + // `accept_loop` from whether `tls` was `Some`, threaded here as its OWN + // signal rather than inferred from `client_identity.is_some()`. Phase 5's + // `enforce_binding` needs to tell "mTLS handshake, cert had no usable + // identity" (deny) apart from "plain listener, no cert at all" (exempt) — + // both look identical as `client_identity: None` otherwise. + on_mtls: bool, ) -> Result<()> where S: AsyncRead + AsyncWrite + Unpin + Send + 'static, @@ -1114,9 +1180,9 @@ where let client_identity = client_identity.clone(); async move { if req.method() == Method::CONNECT { - handle_connect(req, peer_addr, state, client_identity).await + handle_connect(req, peer_addr, state, client_identity, on_mtls).await } else if is_http_proxy_request(&req) { - handle_http_proxy(req, peer_addr, state, client_identity).await + handle_http_proxy(req, peer_addr, state, client_identity, on_mtls).await } else { // Axum handles all non-proxy routes (healthz, vault API, fallback) let resp: Response = router @@ -1147,6 +1213,153 @@ where } } +// ── Phase 5: cert↔token tenant binding enforcement ────────────────────── + +/// Cache TTL for host-tenant lookups — the same 60s window `connect::resolve` +/// uses for `ConnectResponse`, so a revoked/renamed `client_hosts` row is +/// picked up on the same staleness budget as everything else CONNECT-time +/// resolution already accepts. +const BINDING_CACHE_TTL_SECS: u64 = 60; + +/// Resolve the `client_hosts` row for `spiffe` via the cache, falling back to +/// `db::find_client_host_by_spiffe` on a miss — mirroring +/// `connect::resolve`'s cache-then-DB pattern. Negative results (no matching +/// row) are cached too, exactly like a positive one: `Option` +/// round-trips through the cache either way, so an attacker hammering an +/// unknown spiffe URI doesn't turn into a sustained DB query per request. +/// +/// Returns `Err(())` — collapsing whatever the underlying error was — when +/// the lookup itself failed (DB or cache-deserialization trouble); the +/// caller (`enforce_binding`) fails closed on that, and `binding::evaluate` +/// never needs to know anything about the failure beyond "it happened". +async fn resolve_host_tenant( + state: &GatewayState, + spiffe: &str, +) -> Result, ()> { + let cache_key = format!("binding:host:{spiffe}"); + + if let Some(cached) = state + .cache + .get::>(&cache_key) + .await + { + return Ok(cached); + } + + match db::find_client_host_by_spiffe(&state.policy_engine.pool, spiffe).await { + Ok(row) => { + state + .cache + .set(&cache_key, &row, BINDING_CACHE_TTL_SECS) + .await; + Ok(row) + } + Err(e) => { + warn!(spiffe = %spiffe, error = ?e, "binding: client_hosts lookup failed"); + Err(()) + } + } +} + +/// Phase 5 enforcement gate — the ONE place `handle_connect` and +/// `handle_http_proxy` both call into, so the two entry points can never +/// diverge on what "permitted" means (see the module-level worry this whole +/// feature exists to close: a relay's cert bypassing enforcement on one path +/// while the other still checks it). Returns `Some(response)` to +/// short-circuit the request with a denial, `None` to let it proceed. +/// +/// `mode == Off` or `!on_mtls` (the plain listener is exempt BY LISTENER +/// KIND, in every mode — see `binding.rs`'s module doc) both return `None` +/// WITHOUT ever resolving the host tenant: zero DB/cache overhead beyond the +/// mode/on_mtls check itself, so a default (unset) deployment pays nothing +/// for this feature's existence. +async fn enforce_binding( + state: &GatewayState, + on_mtls: bool, + client_identity: Option<&Arc>, + agent_id: Option<&str>, + token_project: &str, + token_org: Option<&str>, +) -> Option> { + if matches!(state.binding_mode, BindingMode::Off) || !on_mtls { + return None; + } + + let identity = client_identity.and_then(|id| id.primary()); + + // Only resolve the host tenant when there's an identity to look up at + // all — `binding::evaluate` denies a missing identity before it ever + // looks at this result, so an unparseable-cert request never touches the + // cache or DB. + let host_lookup: Option, ()>> = match identity { + Some(spiffe) => Some(resolve_host_tenant(state, spiffe).await), + None => None, + }; + let host_tenant: Result, ()> = match &host_lookup { + Some(Ok(row)) => Ok(row.as_ref()), + Some(Err(())) => Err(()), + None => Ok(None), + }; + let host_project_id = match &host_lookup { + Some(Ok(Some(row))) => Some(row.project_id.as_str()), + _ => None, + }; + + let decision = binding::evaluate( + state.binding_mode, + on_mtls, + identity, + host_tenant, + token_project, + token_org, + ); + + match decision { + binding::BindingDecision::Allow => None, + binding::BindingDecision::WouldDeny { reason } => { + // Log-mode audit trail: this is what `Enforce` WOULD have denied. + // Never logs the token or any secret — spiffe/host/token ids only. + warn!( + spiffe = identity.unwrap_or("-"), + host_project_id = host_project_id.unwrap_or("-"), + token_project_id = %token_project, + token_org_id = token_org.unwrap_or("-"), + agent_id = agent_id.unwrap_or("-"), + reason, + mode = ?state.binding_mode, + on_mtls, + decision = "would_deny", + "binding: cert/token tenant mismatch (log mode — request allowed)" + ); + None + } + binding::BindingDecision::Deny { reason } => { + warn!( + spiffe = identity.unwrap_or("-"), + host_project_id = host_project_id.unwrap_or("-"), + token_project_id = %token_project, + token_org_id = token_org.unwrap_or("-"), + agent_id = agent_id.unwrap_or("-"), + reason, + mode = ?state.binding_mode, + on_mtls, + decision = "deny", + "binding: cert/token tenant mismatch — request denied" + ); + // The lookup-failed case is retryable (a transient DB/cache + // hiccup, not a permanent verdict about this identity) — 502, + // same as every other internal-error path in this file. Every + // other reason (mismatch, unknown/revoked host, missing + // identity) is a permanent denial — 403. + if reason == binding::REASON_HOST_LOOKUP_ERROR { + Some(response::bad_gateway()) + } else { + Some(response::binding_denied()) + } + } + } +} + // ── CONNECT handling ──────────────────────────────────────────────────── /// Handle a CONNECT request: authenticate, resolve policy, then MITM or tunnel. @@ -1155,6 +1368,7 @@ async fn handle_connect( peer_addr: SocketAddr, state: GatewayState, client_identity: Option>, + on_mtls: bool, ) -> Result, anyhow::Error> { let host = req .uri() @@ -1194,6 +1408,30 @@ async fn handle_connect( (false, None, None, None, None, None) }; + // Phase 5: cert↔token tenant binding. AFTER `connect::resolve` (needs the + // token's project/org), BEFORE vault/intercept/spawn — a denial here must + // short-circuit before any of that runs. Only when an agent token is + // actually present: no token, nothing to bind. + if agent_token.is_some() { + // `connect::resolve`'s success arm always sets `project_id` for a + // resolved token (`ConnectResponse { project_id: Some(agent.project_id), .. }`); + // the empty-string fallback only guards a resolver invariant this + // handler doesn't otherwise depend on. + let token_project = project_id.as_deref().unwrap_or_default(); + if let Some(denial) = enforce_binding( + &state, + on_mtls, + client_identity.as_ref(), + agent_id.as_deref(), + token_project, + organization_id.as_deref(), + ) + .await + { + return Ok(denial); + } + } + // Vault fallback: resolved at CONNECT time and passed to mitm as a frozen // fallback. Vault queries are expensive (network calls to Bitwarden), so // they're not repeated per request. DB secrets (re-resolved per request @@ -1328,6 +1566,7 @@ async fn handle_http_proxy( peer_addr: SocketAddr, state: GatewayState, client_identity: Option>, + on_mtls: bool, ) -> Result, anyhow::Error> { let authority = req .uri() @@ -1361,6 +1600,26 @@ async fn handle_http_proxy( connect::ConnectResponse::default() }; + // Phase 5: cert↔token tenant binding — AFTER `connect::resolve`, BEFORE + // app-connection resolution / vault fallback / forwarding. See the + // matching call (and its comment) in `handle_connect`; both go through + // the ONE shared `enforce_binding` helper so the two entry points can't + // drift from each other. + if agent_token.is_some() { + if let Some(denial) = enforce_binding( + &state, + on_mtls, + client_identity.as_ref(), + resolved.agent_id.as_deref(), + resolved.project_id.as_deref().unwrap_or_default(), + resolved.organization_id.as_deref(), + ) + .await + { + return Ok(denial); + } + } + // Per-request app connection disambiguation — app rules MERGE with the // secret rules (see inject::merge_injection_rules; #428). When the secret // rules already serve this request's path, app-side escalations are @@ -1714,6 +1973,7 @@ mod tests { vault_service, approval_store, client_ca: None, + binding_mode: BindingMode::Off, } } @@ -1833,7 +2093,7 @@ mod tests { .map(Arc::new); assert!(client_identity.is_some(), "identity must be extracted"); - handle_connection(tls_stream, peer_addr, state, router, client_identity).await + handle_connection(tls_stream, peer_addr, state, router, client_identity, true).await }); let client_stream = tokio::net::TcpStream::connect(addr).await.expect("connect"); @@ -1998,6 +2258,54 @@ mod tests { assert!(parse_plain_bind_value(Some("localhost")).is_err()); } + // ── plain_bind_bypasses_binding_enforcement (Phase 5, security-review FIX 2) ── + + #[test] + fn bypass_warning_fires_on_wildcard_ipv4_under_enforce() { + assert!(plain_bind_bypasses_binding_enforcement( + BindingMode::Enforce, + IpAddr::V4(Ipv4Addr::UNSPECIFIED), + )); + } + + /// FIX 2: unlike the plain `is_unspecified()` check this replaced, a + /// specific-looking but still off-host-reachable address (a pod/cluster + /// IP) must ALSO warn — it is exactly as reachable from outside this + /// process as the wildcard address is. + #[test] + fn bypass_warning_fires_on_a_specific_non_loopback_address_under_enforce() { + assert!(plain_bind_bypasses_binding_enforcement( + BindingMode::Enforce, + IpAddr::V4(Ipv4Addr::new(10, 0, 0, 5)), + )); + } + + #[test] + fn bypass_warning_silent_on_ipv4_loopback_under_enforce() { + assert!(!plain_bind_bypasses_binding_enforcement( + BindingMode::Enforce, + IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), + )); + } + + #[test] + fn bypass_warning_silent_on_ipv6_loopback_under_enforce() { + assert!(!plain_bind_bypasses_binding_enforcement( + BindingMode::Enforce, + IpAddr::V6(std::net::Ipv6Addr::LOCALHOST), + )); + } + + #[test] + fn bypass_warning_silent_when_mode_is_not_enforce_even_on_wildcard() { + for mode in [BindingMode::Off, BindingMode::Log] { + assert!( + !plain_bind_bypasses_binding_enforcement(mode, IpAddr::V4(Ipv4Addr::UNSPECIFIED)), + "mode={mode:?}" + ); + } + } + // ── is_http_proxy_request ────────────────────────────────────────── #[test] @@ -2245,4 +2553,247 @@ mod tests { .into_response(); assert_eq!(resp.status(), StatusCode::BAD_REQUEST); } + + // ── enforce_binding / resolve_host_tenant (Phase 5) ─────────────────── + // + // `test_gateway_state()`'s pool is a `connect_lazy` handle to + // `127.0.0.1` with nothing listening (see its doc comment) — no real + // Postgres runs in this test binary. That is USED deliberately below, + // not merely tolerated: + // - Cache-seeded scenarios never touch the DB at all (a cache hit + // returns before `db::find_client_host_by_spiffe` is ever called), + // so they exercise real code, not a stub. + // - The one scenario that must reach the DB (`enforce_binding` with + // nothing cached) gets a real `Err` from the doomed connection + // attempt — which IS the fail-closed path this suite needs to prove, + // obtained without hand-rolling a mock reader. + // This is the plan's documented fallback ("stub the reader" when a live + // DB isn't available in-crate) — here the "stub" is the cache, which is + // real production code on the read path `resolve_host_tenant` already + // exercises before ever reaching the DB. + // + // Both `handle_connect` and `handle_http_proxy` call the SAME + // `enforce_binding` (see the two call sites above, right after each + // one's `connect::resolve`) — there is exactly one implementation for + // this suite to exercise, which is the structural guarantee that neither + // entry point can quietly diverge from the other. + + fn binding_test_identity(spiffe: &str) -> Arc { + Arc::new(ClientIdentity { + cn: None, + uri_sans: vec![spiffe.to_string()], + serial_hex: "ab".to_string(), + not_after_unix: 0, + }) + } + + /// A cert whose CN/SAN failed extraction (or was never sanitizable) — + /// `ClientIdentity::primary()` returns `None`. Distinct from "no cert at + /// all" (`on_mtls = false`): this identity came from a verified mTLS + /// handshake, so `on_mtls = true` in every test that uses it. + fn binding_test_unparseable_identity() -> Arc { + Arc::new(ClientIdentity { + cn: None, + uri_sans: vec![], + serial_hex: "ab".to_string(), + not_after_unix: 0, + }) + } + + /// `revoked_at` is `TIMESTAMP` (no time zone — see `db::ClientHostRow`'s + /// doc comment), so the stand-in "now" value must be a + /// `PrimitiveDateTime`, not an `OffsetDateTime`. + fn binding_test_host_row( + project_id: &str, + organization_id: Option<&str>, + revoked: bool, + ) -> db::ClientHostRow { + let now = time::OffsetDateTime::now_utc(); + db::ClientHostRow { + project_id: project_id.to_string(), + organization_id: organization_id.map(str::to_string), + revoked_at: revoked.then(|| time::PrimitiveDateTime::new(now.date(), now.time())), + } + } + + async fn seed_binding_cache( + state: &GatewayState, + spiffe: &str, + row: Option<&db::ClientHostRow>, + ) { + state + .cache + .set( + &format!("binding:host:{spiffe}"), + &row, + BINDING_CACHE_TTL_SECS, + ) + .await; + } + + async fn binding_state(mode: BindingMode) -> GatewayState { + let mut state = test_gateway_state().await; + state.binding_mode = mode; + state + } + + #[tokio::test] + async fn enforce_binding_off_mode_allows_without_ever_resolving_the_host() { + let state = binding_state(BindingMode::Off).await; + // Deliberately unseeded: if `Off` resolved the host tenant anyway, it + // would hit the dead test DB and this test would hang/slow down + // rather than return immediately. + let id = binding_test_identity("spiffe://onecli/host/off-1"); + let result = + enforce_binding(&state, true, Some(&id), Some("agent-1"), "proj-A", None).await; + assert!(result.is_none(), "Off must allow unconditionally"); + } + + #[tokio::test] + async fn enforce_binding_plain_listener_exempt_even_under_enforce() { + let state = binding_state(BindingMode::Enforce).await; + let id = binding_test_identity("spiffe://onecli/host/plain-1"); + // on_mtls = false: must be exempt regardless of mode, unseeded cache, + // or anything else — the plain listener never had a cert to bind. + let result = + enforce_binding(&state, false, Some(&id), Some("agent-1"), "proj-A", None).await; + assert!( + result.is_none(), + "plain-listener requests are always exempt" + ); + } + + #[tokio::test] + async fn enforce_binding_matching_tenant_allows_under_enforce() { + let state = binding_state(BindingMode::Enforce).await; + let spiffe = "spiffe://onecli/host/match-1"; + seed_binding_cache( + &state, + spiffe, + Some(&binding_test_host_row("proj-A", Some("org-1"), false)), + ) + .await; + let id = binding_test_identity(spiffe); + let result = enforce_binding( + &state, + true, + Some(&id), + Some("agent-1"), + "proj-A", + Some("org-1"), + ) + .await; + assert!(result.is_none()); + } + + #[tokio::test] + async fn enforce_binding_mismatched_tenant_403s_under_enforce() { + let state = binding_state(BindingMode::Enforce).await; + let spiffe = "spiffe://onecli/host/mismatch-1"; + seed_binding_cache( + &state, + spiffe, + Some(&binding_test_host_row("proj-B", None, false)), + ) + .await; + let id = binding_test_identity(spiffe); + let result = enforce_binding(&state, true, Some(&id), Some("agent-1"), "proj-A", None) + .await + .expect("mismatched tenant must be denied under Enforce"); + assert_eq!(result.status(), StatusCode::FORBIDDEN); + } + + #[tokio::test] + async fn enforce_binding_mismatched_tenant_allowed_under_log_would_deny_only() { + let state = binding_state(BindingMode::Log).await; + let spiffe = "spiffe://onecli/host/mismatch-2"; + seed_binding_cache( + &state, + spiffe, + Some(&binding_test_host_row("proj-B", None, false)), + ) + .await; + let id = binding_test_identity(spiffe); + let result = + enforce_binding(&state, true, Some(&id), Some("agent-1"), "proj-A", None).await; + assert!( + result.is_none(), + "Log mode must allow (WouldDeny), never actually deny" + ); + } + + #[tokio::test] + async fn enforce_binding_off_mode_allows_even_a_cached_mismatch() { + let state = binding_state(BindingMode::Off).await; + let spiffe = "spiffe://onecli/host/off-2"; + seed_binding_cache( + &state, + spiffe, + Some(&binding_test_host_row("proj-B", None, false)), + ) + .await; + let id = binding_test_identity(spiffe); + let result = + enforce_binding(&state, true, Some(&id), Some("agent-1"), "proj-A", None).await; + assert!(result.is_none()); + } + + #[tokio::test] + async fn enforce_binding_revoked_host_403s_under_enforce() { + let state = binding_state(BindingMode::Enforce).await; + let spiffe = "spiffe://onecli/host/revoked-1"; + seed_binding_cache( + &state, + spiffe, + Some(&binding_test_host_row("proj-A", None, true)), + ) + .await; + let id = binding_test_identity(spiffe); + let result = enforce_binding(&state, true, Some(&id), Some("agent-1"), "proj-A", None) + .await + .expect("revoked host must be denied even with a matching project"); + assert_eq!(result.status(), StatusCode::FORBIDDEN); + } + + /// Subtlety #1: an mTLS handshake with no extractable identity must be + /// DENIED, not treated as an exempt plain-listener request. + #[tokio::test] + async fn enforce_binding_unparseable_identity_403s_under_enforce_not_exempt() { + let state = binding_state(BindingMode::Enforce).await; + let id = binding_test_unparseable_identity(); + let result = enforce_binding(&state, true, Some(&id), Some("agent-1"), "proj-A", None) + .await + .expect("a verified mTLS cert with no usable identity must be denied"); + assert_eq!(result.status(), StatusCode::FORBIDDEN); + } + + /// Fail-closed on a lookup failure must be 502 (retryable), never 403 + /// (permanent) — a transient DB/cache outage is not a verdict about this + /// identity. Unseeded cache + the dead test-DB pool together give a real + /// lookup failure here, not a simulated one. + #[tokio::test] + async fn enforce_binding_db_error_502s_under_enforce_not_403() { + let state = binding_state(BindingMode::Enforce).await; + let id = binding_test_identity("spiffe://onecli/host/db-error-1"); + let result = enforce_binding(&state, true, Some(&id), Some("agent-1"), "proj-A", None) + .await + .expect("a lookup failure must fail closed (deny), not silently allow"); + assert_eq!( + result.status(), + StatusCode::BAD_GATEWAY, + "a lookup failure is retryable — 502, not a permanent 403" + ); + } + + #[tokio::test] + async fn resolve_host_tenant_serves_a_cached_negative_result_without_touching_the_db() { + let state = test_gateway_state().await; + let spiffe = "spiffe://onecli/host/neg-1"; + // Cache an explicit "no such host" — proves negative results ride + // the same `Option` cache slot a positive one would, + // and that a hit (positive OR negative) never reaches the DB. + seed_binding_cache(&state, spiffe, None).await; + let result = resolve_host_tenant(&state, spiffe).await; + assert_eq!(result, Ok(None)); + } } diff --git a/apps/gateway/src/gateway/response.rs b/apps/gateway/src/gateway/response.rs index 8e1d18f3..72a2c9c9 100644 --- a/apps/gateway/src/gateway/response.rs +++ b/apps/gateway/src/gateway/response.rs @@ -116,6 +116,20 @@ pub(super) fn bad_gateway() -> Response { ) } +/// 403 Forbidden — Phase 5 cert↔token tenant binding denied the request +/// (`binding::evaluate` returned `Deny` for a reason other than a lookup +/// failure — see `gateway.rs`'s `enforce_binding`, the sole caller). No +/// specifics about WHY (mismatched project, unknown/revoked host, missing +/// identity...) ride in the body: that detail is only ever logged +/// server-side (`enforce_binding`'s structured `warn!`), never handed to the +/// caller, which is exactly who might be trying to probe the binding. +pub(super) fn binding_denied() -> Response { + with_no_retry(json_error_axum( + StatusCode::FORBIDDEN, + serde_json::json!({ "error": "identity_not_permitted" }), + )) +} + /// Build the shared JSON body for multiple-connections responses. fn multiple_connections_json( connections: &[crate::connect::ConnectionChoice], @@ -579,6 +593,34 @@ mod tests { assert_eq!(auth_header, "Basic realm=\"OneCLI Gateway\""); } + #[tokio::test] + async fn binding_denied_is_403_no_retry_and_carries_no_specifics() { + let resp = binding_denied(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + assert_eq!( + resp.headers().get("content-type").unwrap(), + "application/json" + ); + assert_eq!(resp.headers().get("x-should-retry").unwrap(), "false"); + + use http_body_util::BodyExt; + let body = resp + .into_body() + .collect() + .await + .expect("collect body") + .to_bytes(); + let json: serde_json::Value = serde_json::from_slice(&body).expect("valid JSON"); + assert_eq!(json["error"], "identity_not_permitted"); + // The body must never leak the specific reason (mismatch vs. unknown + // vs. revoked vs. missing identity) — that's server-log-only. + assert_eq!( + json.as_object().expect("object").len(), + 1, + "body must carry nothing beyond the generic error code" + ); + } + #[test] fn app_not_connected_preserves_status() { let resp: Response = diff --git a/apps/gateway/src/main.rs b/apps/gateway/src/main.rs index 747d5af5..a1454d15 100644 --- a/apps/gateway/src/main.rs +++ b/apps/gateway/src/main.rs @@ -9,6 +9,7 @@ mod auth; #[path = "ee/auth.rs"] mod auth; +mod binding; mod ca; mod client_ca; mod client_ca_authority; @@ -314,6 +315,13 @@ async fn main() -> Result<()> { None => info!("mTLS disabled (GATEWAY_MTLS_PORT not set)"), } + // Phase 5: cert↔token tenant binding enforcement posture. Defaults to + // `Off` (unset `GATEWAY_BINDING_ENFORCEMENT`) — byte-for-byte identical + // to every prior phase until an operator opts in, first to `log` (compute + // + audit, never deny) and then to `enforce`. + let binding_mode = binding::BindingMode::from_env(); + info!(mode = ?binding_mode, "cert/token tenant binding enforcement mode"); + // Connect to PostgreSQL // Support both DATABASE_URL (OSS) and individual DB_* vars (cloud ECS from Secrets Manager) let database_url = match std::env::var("DATABASE_URL") { @@ -394,6 +402,7 @@ async fn main() -> Result<()> { approval_store, mtls, client_ca, + binding_mode, )?; let result = server.run().await; diff --git a/docs/remote-gateway-deploy.md b/docs/remote-gateway-deploy.md new file mode 100644 index 00000000..98486f0f --- /dev/null +++ b/docs/remote-gateway-deploy.md @@ -0,0 +1,115 @@ +# Remote gateway deployment (CarbonoDev fork) + +How to host the OneCLI gateway on a different machine than the agents (Dokploy / +Railway), using the mutual-TLS + relay stack (gateway Phases 1–5). This is a +**fork** deployment — images publish to `ghcr.io/carbonodev/onecli`, never the +official `ghcr.io/onecli/onecli`. + +## 1. Publish the image + +The `relay` subcommand exists only in this fork's build, so remote mode needs a +fork image. The existing `.github/workflows/publish.yml` already targets the +fork's namespace (`IMAGE_NAME: ${{ github.repository }}` → `ghcr.io/carbonodev/onecli`) +and triggers on any `v*` tag — independent of the `release-please` flow on `main` +(which you do NOT run for this). + +1. Merge the stacked PRs onto your deployable line (`#2 → #3 → #4`, and `#5` for + binding enforcement). +2. Set the root `package.json` `version` to a fork-distinct pre-release, e.g. + `1.44.0-carbono.1` (the pre-release suffix avoids colliding with upstream + onecli versions, and `docker/metadata-action` then skips the moving `1.44` + tag). +3. Tag and push: + ```bash + git tag v1.44.0-carbono.1 + git push origin v1.44.0-carbono.1 + ``` + The workflow builds multi-arch (amd64 + arm64) from `docker/Dockerfile` + (an **OSS** `cargo build --release` — the broken `--features cloud` path is + not used) and pushes `ghcr.io/carbonodev/onecli:1.44.0-carbono.1` + `:latest`. + The image contains both the web app and `/usr/local/bin/onecli-gateway` + (which carries the `relay` subcommand). +4. **Make the GHCR package public** (Packages → the image → Package settings → + Change visibility → Public) so every gateway/relay host can pull without a + registry credential. (The image embeds no secrets — they're injected at + runtime via env.) + +> ARM note: the workflow's `ubuntu-24.04-arm` matrix leg needs ARM runners +> enabled for the org, or drop that leg. + +## 2. Deploy the remote gateway (Dokploy / Railway) + +Run the **full** `ghcr.io/carbonodev/onecli:1.44.0-carbono.1` image (web API for +enrollment + container-config, gateway for the mTLS listener). + +Required env for the mTLS listener (all off unless `GATEWAY_MTLS_PORT` is set): + +| Var | Value | +|-----|-------| +| `GATEWAY_MTLS_PORT` | the mTLS listen port (e.g. `10256`) | +| `GATEWAY_TLS_CERT` / `GATEWAY_TLS_KEY` | the gateway's **server** cert + key (PEM or path) | +| `GATEWAY_CLIENT_CA` | the client CA cert (Phase 2 can also generate one on disk) | +| `GATEWAY_PLAIN_BIND` | **`127.0.0.1`** — see below | +| `GATEWAY_BINDING_ENFORCEMENT` | `off` → `log` → `enforce` (roll out gradually) | + +Two deployment requirements that are easy to get wrong: + +- **Expose the mTLS port as raw TCP passthrough.** On Railway use a TCP proxy; + on Dokploy a raw TCP port — NOT the HTTP/L7 router. An L7 proxy terminates TLS + and the gateway never sees the client certificate, so the handshake fails. + The web API (`APP_URL`) can stay behind normal HTTPS/L7. +- **Restrict the plaintext listener.** The gateway keeps a plain listener (the + web app / loopback path) that is *exempt* from cert auth and binding by design. + If it's network-reachable it's a bypass of everything. Bind it to loopback + (`GATEWAY_PLAIN_BIND=127.0.0.1`) and don't publish that port. The gateway warns + at startup if enforcement is on while the plain listener is on a non-loopback + address. + +## 3. Wire nanoclaw (the relay) + +nanoclaw's default relay image already points at the fork build +(`ghcr.io/carbonodev/onecli:1.44.0-carbono.1`, kept in sync with `versions.json` +by the pin-drift test). Configure a host for remote mode via `--remote` setup, +which writes: + +- `ONECLI_URL` — the remote web API (for the SDK's container-config / approvals). +- `ONECLI_API_KEY` — a **project-scoped** `oc_` key (the relay enrolls with it). +- `NANOCLAW_EGRESS_LOCKDOWN=true` — remote mode requires lockdown (the relay IS + the egress path); nanoclaw refuses to start remote mode without it. +- `RELAY_GATEWAY_ADDR` — the remote gateway's mTLS `host:port`. +- `RELAY_GATEWAY_SERVER_CA` — the gateway's **server** CA (out-of-band; NOT the + client CA or MITM CA). Written to a file under `DATA_DIR`. +- `RELAY_GATEWAY_SERVER_NAME` — set when the mTLS cert's CN/SAN differs from the + TCP-proxy hostname. + +The relay then: generates its own keypair + CSR (private key never leaves it), +enrolls for a per-host client cert (`POST /v1/gateway/client-cert`), renews +before expiry, and blind-splices agent traffic to the gateway over mTLS. Each +agent's `aoc_` token passes through untouched, so the gateway still authenticates +the agent. + +To publish a new gateway build later, bump both `versions.json`'s +`onecli-gateway` pin and the `ONECLI_GATEWAY_IMAGE` literal in `src/config.ts` +(the drift-guard test fails if they diverge), and tag a new `v*`. + +## 4. Binding enforcement rollout + +`GATEWAY_BINDING_ENFORCEMENT` on the gateway: + +1. `off` (default) — no enforcement, byte-identical to today. +2. `log` — records every would-deny (a relay cert carrying a token for a + different project) as an audit event, but allows. Watch these to learn your + real host↔project topology. +3. `enforce` — denies mismatches (403) and fails closed on a lookup error (502). + +Binding is **project-scoped**: a relay enrolled for project X may only carry +tokens for agents in project X. If a single host must serve multiple projects, +enroll one relay/cert per project rather than loosening. + +## Known follow-ups + +- No writer yet for `ClientHost.revokedAt` — the gateway *denies* revoked hosts + within one 60s cache TTL, but nothing sets the column; add an API/UI action to + use the soft-revoke lever. +- No live-DB test tier in the gateway crate (the sqlx decode of a non-null + `revoked_at` isn't exercised end-to-end).