diff --git a/CLAUDE.md b/CLAUDE.md index 7883aa4..0d749ec 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -26,7 +26,7 @@ Three properties are load-bearing and easy to regress: Prometheus shape: blocked/error counts are emitted **only** under their `reason` label (they sum to the unlabeled total, so a separate total would be a second representation of the same number), and the proxy-wide active gauge is `sum without(listener)`. `renderPrometheus` is exported from the package for embedded consumers. -The server also **watches the cert/key files referenced by the config** (grouped by parent dir, deduped, re-derived on every reconcile so watchers don't leak) → a debounced `reconcile()` on change, so an on-disk cert renewal is picked up live without a `config.json` write or restart. Two details make a listener-level cert rotation actually apply: the per-proxy `listenerSig` is computed over the *resolved* listeners (cert contents included), so a rotated `defaultCert`/mTLS file changes the signature and forces a recreate rather than a route-only hot-swap against the frozen `default_listener_tls`. Basename-filtered dir watching handles in-place / rename rotation (what host-manager does); k8s projected-volume `..data` symlink swaps are not yet covered. Cert-failure resilience lives in `router.rs::build_route_table`: a route whose cert can't be built (e.g. rustls `KeyMismatch` from a rotated key vs a stale inlined chain) is isolated — one bad tenant cert never aborts the whole table; on a hot-swap the last-good route is carried forward for that SNI (mid-rotation the old cert is still valid), and on initial build the SNI is simply dropped. +The server also **watches the cert/key files referenced by the config** (grouped by parent dir, deduped, re-derived on every reconcile so watchers don't leak) → a debounced `reconcile()` on change, so an on-disk cert renewal is picked up live without a `config.json` write or restart. Two details make a listener-level cert rotation actually apply: the per-proxy `listenerSig` is computed over the *resolved* listeners (cert contents included), so a rotated `defaultCert`/mTLS file changes the signature and forces a recreate rather than a route-only hot-swap against the frozen `default_listener_tls`. Basename-filtered dir watching handles in-place / rename rotation (what host-manager does); k8s projected-volume `..data` symlink swaps are not yet covered. Cert-failure resilience lives in `router.rs::build_route_table`: a route whose cert can't be built (e.g. rustls `KeyMismatch` from a rotated key vs a stale inlined chain) is isolated — one bad tenant cert never aborts the whole table; on a hot-swap the last-good route is carried forward for that SNI (mid-rotation the old cert is still valid), and on initial build the SNI is simply dropped. Carry-forward is conditional on the client-certificate policy: the last-good `ServerConfig` *is* its client verifier, so it is kept only while the new spec asks for the same `ClientAuth` (passthrough / no client cert / SHA-256 of the effective CA bundle + `requireClientCert`) — otherwise the SNI is tombstoned, or a route newly requiring mTLS would keep admitting certless clients until a later reload built (symphony#62). The policy lives in `RouteTable.client_auth`, not on `Route`, because every connection clones `Route` into its task. For the comparison to see the requested policy, `server.ts::resolveRoute` keeps the resolved `mtls` on a cert-file read failure, and substitutes an empty CA bundle (which can never build, so never matches) when the CA file itself is unreadable; a passthrough route ignores `mtls`, so this only drops terminating routes. One spec decides each configured SNI even when it is rejected (the last exact duplicate, the first wildcard), so a duplicate can't serve a tombstoned SNI under another policy. ### Data flow diff --git a/README.md b/README.md index 1fd2a82..a11bff2 100644 --- a/README.md +++ b/README.md @@ -106,7 +106,7 @@ above `TCP_KEEPCNT`'s ceiling of 127. | Field | Type | Default | Description | |---|---|---|---| -| `sni` | `string` | required | Hostname for exact match, or `'*.suffix'` for wildcard, or `''` for default (maximum 255 UTF-8 bytes; no control characters; exact duplicates remain last-wins, later wildcard duplicates are skipped) | +| `sni` | `string` | required | Hostname for exact match, or `'*.suffix'` for wildcard, or `''` for default (maximum 255 UTF-8 bytes; no control characters; exact duplicates remain last-wins, later wildcard duplicates are skipped, even when the winning entry is rejected) | | `metricsGroup` | `string` | — | Stable grouping key for aggregating several domain routes as one tenant (maximum 128 UTF-8 bytes; no control characters) | | `upstreams` | `Upstream[]` | required | Destination(s); multiple UDS upstreams are load-balanced | | `terminateTls` | `boolean` | required | `true` = decrypt TLS; `false` = TCP passthrough | @@ -396,7 +396,7 @@ Use `sourceAddressHeader` on a route to control how the real client IP is commun This declaration only helps when a header could actually be injected in the first place. A **passthrough** route (`terminateTls: false`) never decrypts the stream, so a header-carried mode has no carrier at all regardless of `protocol` — declaring `'http'` on a passthrough route wouldn't make it work, and is rejected with a distinct "no carrier" error instead of steering you toward a declaration that can't help. Use `sourceAddressHeader: 'proxyProtocolV2'` there instead (it works on any byte stream, passthrough included). -Like a cert that fails to build, a route rejected for either reason is isolated to just that route: it's dropped (its SNI resolves to nothing) rather than failing `new SymphonyProxy()` or `updateConfig()` for every other route on the same port-set — one tenant's config mistake doesn't take the rest down with it. The rejection is still loud: it's logged (`symphony: skipping route '': ...`), and on a hot-swap the route's last-good version (if any) keeps serving until the config is fixed. +Like a cert that fails to build, a route rejected for either reason is isolated to just that route: it's dropped (its SNI resolves to nothing) rather than failing `new SymphonyProxy()` or `updateConfig()` for every other route on the same port-set — one tenant's config mistake doesn't take the rest down with it. The rejection is still loud: it's logged (`symphony: skipping route '': ...`) and counted in `failingRoutes`. A rejected declaration is dropped on a hot-swap too, since it won't heal until the config is edited. A cert that fails to build on a hot-swap (typically a cert and key rotating as two separate files) instead keeps the route's last-good version serving until the pair is consistent again, but only while the route still asks for the client-certificate policy that version enforces (passthrough, no `mtls`, or the same `clientCaCert` bundle and `requireClientCert`). A route whose policy changed is dropped, so an old verifier never outlives the edit that replaced it. This is a breaking change for a hand-written route that already uses `sourceAddressHeader: 'xForwardedFor'` (or a header-carried `forwardFingerprint`) without `protocol: 'http'` — add the declaration when upgrading, or that route will stop serving traffic (silently, aside from the log line) rather than failing loudly at startup. diff --git a/__test__/server.spec.ts b/__test__/server.spec.ts index 4fb15bc..ec7e648 100644 --- a/__test__/server.spec.ts +++ b/__test__/server.spec.ts @@ -14,7 +14,7 @@ import { spawn, ChildProcess } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; -import { generateSelfSignedCert, getFreePort, startEchoServer, tlsRoundTrip, sleep } from './util.js'; +import { generateClientCa, generateSelfSignedCert, getFreePort, startEchoServer, tlsRoundTrip, sleep } from './util.js'; // server.js sits next to this compiled spec's sibling ts/ dir: dist-test/ts/server.js const SERVER_JS = path.join(__dirname, '..', 'ts', 'server.js'); @@ -534,6 +534,130 @@ describe('symphony-server (route cert-file read failure is isolated)', () => { }); }); +describe('symphony-server (a route that fails to rebuild never keeps an old client-cert policy)', () => { + // symphony#62 + const serverCert = generateSelfSignedCert('tenant.local'); + const clientCa = generateClientCa(); + const client = { clientCert: clientCa.clientCert, clientKey: clientCa.clientKey }; + let dir: string; + let configPath: string; + let statusPath: string; + let caFile: string; + let proxyPort: number; + let echo: Awaited>; + let server: RunningServer; + + // Each route reads its own copy of the cert/key, so deleting one route's file breaks only that route. + const certFiles = (sni: string) => { + const certChainFile = path.join(dir, `${sni}.crt`); + const privateKeyFile = path.join(dir, `${sni}.key`); + return { certChainFile, privateKeyFile }; + }; + const route = (sni: string, mtls?: object) => ({ + sni, + upstreams: [{ kind: 'tcp', host: '127.0.0.1', port: echo.port }], + terminateTls: true, + cert: certFiles(sni), + mtls, + }); + const publish = (open: object) => + writeConfigAtomic(configPath, { + version: 1, + proxies: [ + { + listeners: [{ host: '127.0.0.1', port: proxyPort }], + routes: [ + open, + route('inline-ca.local', { clientCaCert: clientCa.caCert, requireClientCert: true }), + route('file-ca.local', { clientCaCertFile: caFile, requireClientCert: true }), + // No client auth: a dropped route under it must not fall through to it. + route('*.local'), + ], + }, + ], + }); + const connects = async (servername: string, presented: Partial = {}) => { + try { + const reply = await tlsRoundTrip({ port: proxyPort, servername, data: Buffer.from('ping'), ...presented }); + return reply.toString() === 'ping'; + } catch { + return false; + } + }; + const reloadedAt = (): string => { + try { + return JSON.parse(fs.readFileSync(statusPath, 'utf8')).reloadedAt ?? ''; + } catch { + return ''; + } + }; + const becomesRefused = (servername: string, presented: Partial = {}) => + waitFor(async () => !(await connects(servername, presented)), 8000, 150).then( + () => true, + () => false + ); + + before(async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'symphony-clientauth-')); + configPath = path.join(dir, 'config.json'); + statusPath = path.join(dir, 'status.json'); + caFile = path.join(dir, 'client-ca.pem'); + fs.writeFileSync(caFile, clientCa.caCert); + for (const sni of ['open.local', 'inline-ca.local', 'file-ca.local', '*.local']) { + fs.writeFileSync(certFiles(sni).certChainFile, serverCert.cert); + fs.writeFileSync(certFiles(sni).privateKeyFile, serverCert.key); + } + + echo = await startEchoServer(); + proxyPort = await getFreePort(); + publish(route('open.local')); + server = spawnServer(configPath, statusPath); + await waitFor(() => fs.existsSync(statusPath)); + }); + + after(async () => { + await killServer(server); + await echo.close().catch(() => {}); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('drops a route republished to require client certs under a CA no verifier can use', async () => { + await waitFor(() => connects('open.local')); + publish(route('open.local', { clientCaCert: '', requireClientCert: true })); + assert.ok( + await becomesRefused('open.local'), + 'a certless client still reaches the route reloaded to require certs' + ); + assert.match(server.getStderr(), /route 'open\.local'.*not retained/); + assert.equal(server.child.exitCode, null); + assert.equal(await connects('other.local'), true, 'the wildcard keeps serving its own subdomains'); + }); + + it('drops a route whose client CA file becomes unreadable', async () => { + await waitFor(() => connects('file-ca.local', client)); + fs.rmSync(caFile); + assert.ok(await becomesRefused('file-ca.local', client), 'the route kept trusting a CA it can no longer read'); + assert.match(server.getStderr(), /route 'file-ca\.local'.*not retained/); + assert.equal(await connects('inline-ca.local', client), true, 'a co-tenant keeps serving'); + + fs.writeFileSync(caFile, clientCa.caCert); + await waitFor(() => connects('file-ca.local', client), 8000, 150); + assert.equal(await connects('file-ca.local'), false, 'the restored route requires a client cert again'); + }); + + it('keeps the last-good route of an mTLS route whose cert file goes missing', async () => { + await waitFor(() => connects('inline-ca.local', client)); + fs.rmSync(certFiles('inline-ca.local').privateKeyFile); + // status.json's reloadedAt is written once that reconcile has published its table. + const unreadable = /^(\S+) \[symphony-server\] failed to resolve cert material for route 'inline-ca\.local'/m; + await waitFor(() => unreadable.test(server.getStderr()), 8000, 100); + const loggedAt = server.getStderr().match(unreadable)![1]; + await waitFor(() => reloadedAt() >= loggedAt, 8000, 50); + assert.equal(await connects('inline-ca.local', client), true, `stderr:\n${server.getStderr()}`); + assert.equal(await connects('inline-ca.local'), false, 'the retained route still requires a client cert'); + }); +}); + describe('symphony-server (protection hot-swap via config file)', () => { // Verifies that adding a protection block to a previously-unprotected listener forces // a seamless recreate (fix 1: hasProtection in listenerSig), and that removing it diff --git a/src/router.rs b/src/router.rs index e10f0bf..7e18c6e 100644 --- a/src/router.rs +++ b/src/router.rs @@ -1,6 +1,6 @@ use crate::balancer::{UdsBalancer, UdsSlotSpec}; use crate::metrics::RouteMetrics; -use crate::tls::{CertSpec, MtlsSpec, TlsConfigCache}; +use crate::tls::{sha256, CertSpec, MtlsSpec, TlsConfigCache}; use arc_swap::ArcSwap; use rustls::ServerConfig; use std::collections::{HashMap, HashSet}; @@ -253,6 +253,10 @@ pub struct RouteTable { /// fail-closed reasoning as `dropped_exact`, checked against incoming SNIs with the same /// suffix-matching rule as `wildcard` itself. dropped_wildcard: Vec>, + /// The client-certificate policy each installed route enforces, keyed by configured SNI + /// (`*.`-prefixed for a wildcard). Kept here rather than on `Route`, which every connection + /// clones into its task. + client_auth: HashMap, ClientAuth>, } impl RouteTable { @@ -398,12 +402,68 @@ impl ListenerTlsSpec { } } +fn effective_mtls<'a>( + spec: &'a RouteSpec, + listener_tls: &'a ListenerTlsSpec, +) -> Option<(&'a [u8], bool)> { + match spec.mtls_ca_pem.as_deref() { + Some(ca) => Some((ca, spec.require_client_cert)), + None => listener_tls + .mtls_ca_pem + .as_deref() + .map(|ca| (ca, listener_tls.require_client_cert)), + } +} + +/// The client-certificate policy a route spec asks for, as `build_route` would enforce it. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum ClientAuth { + /// symphony never terminates, so any client authentication is the upstream's own. Distinct + /// from `NoClientCert`: a terminating route must not stand in for a passthrough whose upstream + /// requires client certificates. + Passthrough, + NoClientCert, + ClientCert { + ca_sha256: [u8; 32], + required: bool, + }, +} + +impl ClientAuth { + fn of(spec: &RouteSpec, listener_tls: &ListenerTlsSpec) -> Self { + if !spec.terminate_tls { + return Self::Passthrough; + } + match effective_mtls(spec, listener_tls) { + None => Self::NoClientCert, + Some((ca, required)) => Self::ClientCert { + ca_sha256: sha256(ca), + required, + }, + } + } +} + +fn tombstone( + sni: &str, + dropped_exact: &mut HashSet>, + dropped_wildcard: &mut Vec>, +) { + if let Some(suffix) = sni.strip_prefix("*.") { + dropped_wildcard.push(Arc::from(suffix)); + } else { + dropped_exact.insert(Arc::from(sni)); + } +} + /// Build a RouteTable from a list of route specs and a listener-level fallback. /// /// `previous` is the currently-live table on a hot-swap (None on initial construction). When /// a route's cert fails to build, its last-good route is carried forward from `previous` if -/// present, so a transient rotation mismatch keeps serving the old (still-valid) cert instead -/// of dropping the SNI. +/// present and it enforces the client-certificate policy the new spec asks for, so a transient +/// rotation mismatch keeps serving the old (still-valid) cert instead of dropping the SNI. A +/// route whose requested policy changed is dropped instead: its old verifier would otherwise +/// outlive the edit that replaced it. /// /// `cache` is the proxy's long-lived `TlsConfigCache`, deliberately *not* created here: an /// unchanged cert must map to the same `Arc` across rebuilds so its TLS session @@ -427,13 +487,27 @@ pub fn build_route_table( let mut configured_wildcards = HashSet::with_capacity(specs.len()); let mut dropped_exact: HashSet> = HashSet::new(); let mut dropped_wildcard: Vec> = Vec::new(); + let mut client_auth: HashMap, ClientAuth> = HashMap::new(); + let last_exact: HashMap<&str, usize> = specs + .iter() + .enumerate() + .filter(|(_, spec)| !spec.sni.starts_with("*.")) + .map(|(i, spec)| (spec.sni.as_str(), i)) + .collect(); - for spec in specs { - // Exact duplicates already collapse last-wins in `exact`. Wildcards live in a Vec and - // resolve first-wins, so isolate later live duplicates to keep that routing behavior while - // ensuring the metric table cannot expose two identical configured route labels. + for (i, spec) in specs.iter().enumerate() { + // One spec decides each configured SNI, even when it is rejected: otherwise a duplicate could + // serve the SNI under a policy the deciding spec didn't ask for. Exact duplicates collapse + // last-wins. Wildcards live in a Vec and resolve first-wins, so later duplicates are isolated, + // which also keeps the metric table from exposing two identical configured route labels. + if last_exact + .get(spec.sni.as_str()) + .is_some_and(|&last| last != i) + { + continue; + } let wildcard_suffix = spec.sni.strip_prefix("*."); - if wildcard_suffix.is_some_and(|suffix| configured_wildcards.contains(suffix)) { + if wildcard_suffix.is_some_and(|suffix| !configured_wildcards.insert(suffix)) { let newly_failing = previous.is_none_or(|p| !p.failing_snis.contains(spec.sni.as_str())); failing_snis.insert(Arc::from(spec.sni.as_str())); @@ -462,14 +536,12 @@ pub fn build_route_table( if newly_failing { eprintln!("symphony: skipping route '{}': {}", spec.sni, e); } - if let Some(suffix) = spec.sni.strip_prefix("*.") { - dropped_wildcard.push(Arc::from(suffix)); - } else { - dropped_exact.insert(Arc::from(spec.sni.as_str())); - } + tombstone(&spec.sni, &mut dropped_exact, &mut dropped_wildcard); continue; } + let requested_auth = ClientAuth::of(spec, listener_tls); + // Isolate per-route failures: a single route whose cert can't be built (e.g. a // rotated key no longer matching an inlined chain → rustls KeyMismatch) must not // abort the whole table and take every other tenant on the port down with it. @@ -483,14 +555,21 @@ pub fn build_route_table( previous.is_none_or(|p| !p.failing_snis.contains(spec.sni.as_str())); failing_snis.insert(Arc::from(spec.sni.as_str())); - match previous.and_then(|p| p.get_for_spec_sni(&spec.sni)) { + let last_good = previous.and_then(|p| { + let route = p.get_for_spec_sni(&spec.sni)?; + Some(( + route, + p.client_auth.get(spec.sni.as_str()) == Some(&requested_auth), + )) + }); + match last_good { // On a hot-swap, a cert and key rotate as two non-atomic files, so a // transient mismatch mid-rotation is normal. Carry the previous route // forward *whole* (cert and upstreams) — intentional: its cert is still // valid, and the route heals on the next reconcile when the pair is // consistent. If the route's upstreams also changed in the same reconcile, // they too persist until then; that's the accepted cost of last-good. - Some(prev) => { + Some((prev, true)) => { if newly_failing { eprintln!( "symphony: route '{}' failed to rebuild ({}); retaining last-good route", @@ -499,6 +578,17 @@ pub fn build_route_table( } prev.clone() } + // The previous route's `tls_config` holds its client verifier, so retaining it + // would keep enforcing the old policy. Always logged: the previous table was + // serving this SNI, so this is a transition even if it was already failing. + Some((_, false)) => { + eprintln!( + "symphony: skipping route '{}': {}; its last-good route enforces a different client-certificate policy, so it is not retained", + spec.sni, e + ); + tombstone(&spec.sni, &mut dropped_exact, &mut dropped_wildcard); + continue; + } // No prior route (initial build, or a newly-added route): drop this SNI. It must // resolve to nothing, not silently fall through to a broader wildcard or the // default route — this SNI *was* configured, just rejected, which is a different @@ -509,11 +599,7 @@ pub fn build_route_table( if newly_failing { eprintln!("symphony: skipping route '{}': {}", spec.sni, e); } - if let Some(suffix) = spec.sni.strip_prefix("*.") { - dropped_wildcard.push(Arc::from(suffix)); - } else { - dropped_exact.insert(Arc::from(spec.sni.as_str())); - } + tombstone(&spec.sni, &mut dropped_exact, &mut dropped_wildcard); continue; } } @@ -527,10 +613,6 @@ pub fn build_route_table( route.metric_identity = previous_route.metric_identity.clone(); } } - if let Some(suffix) = wildcard_suffix { - configured_wildcards.insert(suffix); - } - // Collected from the route actually installed, which may be a carried-forward one. for dest in std::iter::once(&route.destination).chain(route.destination_h2.iter()) { if let Destination::UdsSet(ref bal) = dest { @@ -543,6 +625,7 @@ pub fn build_route_table( } } + client_auth.insert(Arc::from(spec.sni.as_str()), requested_auth); if spec.sni.starts_with("*.") { let suffix: Arc = Arc::from(&spec.sni[2..]); wildcard.push((suffix, route)); @@ -572,6 +655,7 @@ pub fn build_route_table( failing_snis, dropped_exact, dropped_wildcard, + client_auth, }) } @@ -633,18 +717,11 @@ fn build_route( private_key_pem: key_pem.to_vec().into(), }; - let mtls_ca = spec - .mtls_ca_pem - .as_deref() - .or(listener_tls.mtls_ca_pem.as_deref()); - let mtls_spec = mtls_ca.map(|ca| MtlsSpec { - client_ca_pem: ca.to_vec(), - require_client_cert: if spec.mtls_ca_pem.is_some() { - spec.require_client_cert - } else { - listener_tls.require_client_cert - }, - }); + let mtls_spec = + effective_mtls(spec, listener_tls).map(|(ca, require_client_cert)| MtlsSpec { + client_ca_pem: ca.to_vec(), + require_client_cert, + }); Some(cache.get_or_build(&cert_spec, mtls_spec.as_ref(), spec.http2)?) } else { @@ -1245,6 +1322,200 @@ UlqL1DcgX6Szi9w/p7B4BZO9iA== ); } + fn with_client_ca(mut spec: RouteSpec, ca: &[u8], required: bool) -> RouteSpec { + spec.mtls_ca_pem = Some(ca.to_vec()); + spec.require_client_cert = required; + spec + } + + fn build( + specs: &[RouteSpec], + listener: &ListenerTlsSpec, + live: Option<&RouteTable>, + ) -> RouteTable { + build_route_table(specs, listener, live, &mut TlsConfigCache::new()).expect("build") + } + + /// A passthrough spec whose build fails: every upstream is h2-marked, so it has no default one. + fn failing_passthrough(sni: &str) -> RouteSpec { + let mut spec = tls_route(sni, CERT_A, KEY_A); + spec.terminate_tls = false; + spec.upstreams = vec![UpstreamSpec::Uds { + paths: vec!["/nonexistent/h2.sock".into()], + pids: vec![None], + tids: vec![None], + ip_affinity: false, + affinity_ttl_ms: 0, + protocol: Some("h2".into()), + }]; + spec + } + + // symphony#62 + #[test] + fn failed_rebuild_under_a_new_client_cert_policy_is_dropped() { + let listener = ListenerTlsSpec::empty(); + let wildcard = tls_route("*.example.com", CERT_A, KEY_A); + let live = build( + &[ + tls_route("tenant.example.com", CERT_A, KEY_A), + wildcard.clone(), + ], + &listener, + None, + ); + assert!(live.resolve(Some("tenant.example.com")).is_some()); + + let required_with_no_trust_anchors = + with_client_ca(tls_route("tenant.example.com", CERT_A, KEY_A), b"", true); + let failing = [required_with_no_trust_anchors, wildcard.clone()]; + let swapped = build(&failing, &listener, Some(&live)); + assert!( + swapped.resolve(Some("tenant.example.com")).is_none(), + "the old route admits certless clients the new policy refuses, so it must not be retained, and the SNI must not fall through to the wildcard" + ); + assert!(swapped.failing_snis.contains("tenant.example.com")); + assert!( + swapped.resolve(Some("other.example.com")).is_some(), + "the healthy wildcard keeps serving its own subdomains" + ); + + let again = build(&failing, &listener, Some(&swapped)); + assert!(again.resolve(Some("tenant.example.com")).is_none()); + + let buildable = + with_client_ca(tls_route("tenant.example.com", CERT_A, KEY_A), CERT_A, true); + let healed = build(&[buildable, wildcard], &listener, Some(&again)); + assert!(healed.resolve(Some("tenant.example.com")).is_some()); + assert_eq!(healed.failing_route_count(), 0); + } + + #[test] + fn a_rejected_duplicate_is_not_replaced_by_its_sibling() { + let listener = ListenerTlsSpec::empty(); + for (sni, probe) in [ + ("tenant.example.com", "tenant.example.com"), + ("*.example.com", "app.example.com"), + ] { + let certless = tls_route(sni, CERT_A, KEY_A); + let rejected = with_client_ca(certless.clone(), b"", true); + let live = build(std::slice::from_ref(&certless), &listener, None); + let deciding_rejected = if sni.starts_with("*.") { + [rejected.clone(), certless.clone()] + } else { + [certless.clone(), rejected.clone()] + }; + for previous in [None, Some(&live)] { + let table = build(&deciding_rejected, &listener, previous); + assert!( + table.resolve(Some(probe)).is_none(), + "{sni}: a certless duplicate must not serve the SNI its deciding spec requires certs for" + ); + } + + let mut deciding_buildable = deciding_rejected; + deciding_buildable.reverse(); + let table = build(&deciding_buildable, &listener, Some(&live)); + assert!( + table.resolve(Some(probe)).is_some(), + "{sni}: a buildable deciding duplicate serves" + ); + } + } + + #[test] + fn cert_only_failure_keeps_last_good_while_the_client_cert_policy_is_unchanged() { + let listener = ListenerTlsSpec::empty(); + let specs = |key: &[u8], required: bool| { + ["tenant.example.com", "*.example.com"] + .map(|sni| with_client_ca(tls_route(sni, CERT_A, key), CERT_A, required)) + }; + let live = build(&specs(KEY_A, true), &listener, None); + + let carried = build(&specs(KEY_B, true), &listener, Some(&live)); + for sni in ["tenant.example.com", "other.example.com"] { + assert_eq!( + config_ptr(&carried, sni), + config_ptr(&live, sni), + "{sni}: a mismatched key under an unchanged policy keeps the last-good route" + ); + } + + let dropped = build(&specs(KEY_B, false), &listener, Some(&carried)); + for sni in ["tenant.example.com", "other.example.com"] { + assert!( + dropped.resolve(Some(sni)).is_none(), + "{sni}: a carried-forward route must not outlive a client-cert policy change" + ); + } + } + + #[test] + fn every_client_auth_change_drops_a_failing_route() { + let listener = ListenerTlsSpec::empty(); + let sni = "tenant.example.com"; + let terminated = tls_route(sni, CERT_A, KEY_A); + let required = with_client_ca(terminated.clone(), CERT_A, true); + let mut other_bundle = CERT_A.to_vec(); + other_bundle.extend_from_slice(b"# a different bundle\n"); + let mismatched = |spec: &RouteSpec| RouteSpec { + key_pem: Some(KEY_B.to_vec()), + ..spec.clone() + }; + + let cases = [ + ("client CA removed", &required, mismatched(&terminated)), + ( + "client cert made optional", + &required, + mismatched(&with_client_ca(terminated.clone(), CERT_A, false)), + ), + ( + "client CA bundle changed", + &required, + mismatched(&with_client_ca(terminated.clone(), &other_bundle, true)), + ), + ( + "client CA added", + &terminated, + mismatched(&with_client_ca(terminated.clone(), CERT_A, true)), + ), + ( + "terminating route switched to passthrough", + &terminated, + failing_passthrough(sni), + ), + ]; + for (change, previous, failing) in cases { + let live = build(std::slice::from_ref(previous), &listener, None); + assert!(live.resolve(Some(sni)).is_some(), "{change}: live route"); + let swapped = build(&[failing], &listener, Some(&live)); + assert!( + swapped.resolve(Some(sni)).is_none(), + "{change}: the last-good route must not be retained" + ); + } + } + + #[test] + fn client_auth_inherited_from_the_listener_compares_by_effect() { + let listener = ListenerTlsSpec { + mtls_ca_pem: Some(CERT_A.to_vec()), + require_client_cert: true, + ..ListenerTlsSpec::empty() + }; + let sni = "tenant.example.com"; + let live = build(&[tls_route(sni, CERT_A, KEY_A)], &listener, None); + + let same_effect = with_client_ca(tls_route(sni, CERT_A, KEY_B), CERT_A, true); + let carried = build(&[same_effect], &listener, Some(&live)); + assert_eq!(config_ptr(&carried, sni), config_ptr(&live, sni)); + + let optional = with_client_ca(tls_route(sni, CERT_A, KEY_B), CERT_A, false); + let dropped = build(&[optional], &listener, Some(&live)); + assert!(dropped.resolve(Some(sni)).is_none()); + } + #[test] fn route_metric_identity_survives_reload_until_group_changes() { let mut initial = tls_route("tenant.example.com", CERT_A, KEY_A); diff --git a/src/tls.rs b/src/tls.rs index dad5a07..5af4997 100644 --- a/src/tls.rs +++ b/src/tls.rs @@ -230,7 +230,7 @@ fn build_server_config( Ok(Arc::new(cfg)) } -fn sha256(data: &[u8]) -> [u8; 32] { +pub(crate) fn sha256(data: &[u8]) -> [u8; 32] { let digest = ring::digest::digest(&ring::digest::SHA256, data); let mut out = [0u8; 32]; out.copy_from_slice(digest.as_ref()); diff --git a/ts/server.ts b/ts/server.ts index 3eec7df..e18c6a0 100644 --- a/ts/server.ts +++ b/ts/server.ts @@ -100,22 +100,30 @@ function resolveMtls(m: FileMtlsConfig, baseDir: string): MtlsConfig { // A cert the Rust layer is guaranteed to reject at build time (empty PEM → "no // certificates"). Substituted for a route whose cert/key file can't be read, so the failure // is isolated inside build_route_table (which carries the route's last-good forward on a -// hot-swap, or drops just that SNI on an initial build) instead of throwing out of +// hot-swap while its client-cert policy is unchanged, or drops just that SNI) instead of throwing out of // toProxyConfig and aborting the update for every route on the port-set. const UNRESOLVABLE_CERT: CertConfig = { certChain: '', privateKey: Buffer.alloc(0) }; +// Stands in for a client CA that can't be read: the requested policy is unknown. No verifier builds +// from an empty bundle, so on a terminating route it never matches a serving route's policy and the +// route is dropped rather than kept under its old one. A passthrough route ignores mtls. +const UNRESOLVABLE_MTLS: MtlsConfig = { clientCaCert: '', requireClientCert: true }; + // Resolve one route's cert/mTLS from disk, isolating a file-read failure (e.g. ENOENT while a // cert rotates) to this route rather than letting it abort the whole port-set's reconcile. function resolveRoute(r: FileRouteConfig, baseDir: string): RouteConfig { + let mtls: MtlsConfig | undefined; try { - return { - ...r, - cert: r.cert ? resolveCert(r.cert, baseDir) : undefined, - mtls: r.mtls ? resolveMtls(r.mtls, baseDir) : undefined, - }; + mtls = r.mtls ? resolveMtls(r.mtls, baseDir) : undefined; + } catch (err) { + logErr(`failed to resolve the client CA for route '${r.sni}' — isolating route:`, (err as Error).message); + return { ...r, cert: UNRESOLVABLE_CERT, mtls: UNRESOLVABLE_MTLS }; + } + try { + return { ...r, cert: r.cert ? resolveCert(r.cert, baseDir) : undefined, mtls }; } catch (err) { logErr(`failed to resolve cert material for route '${r.sni}' — isolating route:`, (err as Error).message); - return { ...r, cert: UNRESOLVABLE_CERT, mtls: undefined }; + return { ...r, cert: UNRESOLVABLE_CERT, mtls }; } }