A route that fails to rebuild no longer keeps its old client-certificate policy

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
 

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 '<sni>': ...`), 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 '<sni>': ...`) 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.
 

__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<ReturnType<typeof startEchoServer>>;
+	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<typeof client> = {}) => {
+		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<typeof client> = {}) =>
+		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