Keep TLS session resumption alive across config reloads - #42
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements TLS session resumption across configuration reloads by threading a long-lived TlsConfigCache through the route table build process, ensuring that unchanged certificates retain their ServerConfig allocations (and thus their session caches and ticket keys). It also introduces a mark-and-sweep mechanism to retire rotated-out certificates and adds comprehensive tests. The review feedback correctly identifies a potential issue where aborted or failed configuration builds can leave stale keys in the cache's used set, leading to temporary memory leaks on subsequent successful reloads. It is recommended to implement the suggested clear_used method and invoke it at the start of each route table build to prevent this.
| /// Drop every entry not requested since the previous sweep, retiring rotated-out certs. | ||
| /// Callers run this once they *commit* the table they just built — see `build_route_table`. | ||
| pub fn retain_used(&mut self) { | ||
| let used = std::mem::take(&mut self.used); | ||
| self.cache.retain(|k, _| used.contains(k)); | ||
| } |
There was a problem hiding this comment.
If a configuration reload fails validation (e.g., during the protection config validation phase after the route table is built), the TlsConfigCache is not swept via retain_used(). This causes any new ServerConfig keys marked in self.used during the failed build to persist in the used set. On the next successful reload, these aborted configs will be incorrectly retained in the cache, causing a temporary memory leak until a second successful reload occurs.
To prevent this, we should expose a clear_used method on TlsConfigCache so that the used set can be reset at the start of any new route table build.
| /// Drop every entry not requested since the previous sweep, retiring rotated-out certs. | |
| /// Callers run this once they *commit* the table they just built — see `build_route_table`. | |
| pub fn retain_used(&mut self) { | |
| let used = std::mem::take(&mut self.used); | |
| self.cache.retain(|k, _| used.contains(k)); | |
| } | |
| /// Drop every entry not requested since the previous sweep, retiring rotated-out certs. | |
| /// Callers run this once they *commit* the table they just built — see `build_route_table`. | |
| pub fn retain_used(&mut self) { | |
| let used = std::mem::take(&mut self.used); | |
| self.cache.retain(|k, _| used.contains(k)); | |
| } | |
| /// Clear the marked keys, resetting state if a build is aborted or before starting a new one. | |
| pub fn clear_used(&mut self) { | |
| self.used.clear(); | |
| } |
| ) -> crate::error::Result<RouteTable> { | ||
| let mut cache = TlsConfigCache::new(); | ||
| let mut exact: HashMap<Arc<str>, Route> = HashMap::new(); | ||
| let mut wildcard: Vec<(Arc<str>, Route)> = Vec::new(); | ||
| let mut monitored_balancers: Vec<Arc<UdsBalancer>> = Vec::new(); |
There was a problem hiding this comment.
To ensure that any leftover marked keys from a previously failed or aborted route table build do not pollute the TlsConfigCache's used set, we should clear the marked keys at the start of build_route_table.
| ) -> crate::error::Result<RouteTable> { | |
| let mut cache = TlsConfigCache::new(); | |
| let mut exact: HashMap<Arc<str>, Route> = HashMap::new(); | |
| let mut wildcard: Vec<(Arc<str>, Route)> = Vec::new(); | |
| let mut monitored_balancers: Vec<Arc<UdsBalancer>> = Vec::new(); | |
| ) -> crate::error::Result<RouteTable> { | |
| cache.clear_used(); | |
| let mut exact: HashMap<Arc<str>, Route> = HashMap::new(); | |
| let mut wildcard: Vec<(Arc<str>, Route)> = Vec::new(); | |
| let mut monitored_balancers: Vec<Arc<UdsBalancer>> = Vec::new(); |
08c421f to
aff6593
Compare
symphony already configures rustls for session resumption (a session cache for TLS 1.2 IDs, a ticketer for TLS 1.3 tickets), and it works — but only until the first config reload. Both of those live on the rustls ServerConfig, and build_route_table created a TlsConfigCache per call. Every route-table rebuild therefore minted a brand-new ServerConfig for every route, including ones whose cert had not changed at all, which threw away the session cache and generated fresh random ticket keys. Every ticket already handed to a client became undecryptable. Nothing errors — clients just quietly go back to full handshakes. That matters because reloads are routine: adding or removing a route, or an on-disk cert renewal picked up by the file watcher, rebuilds the whole table. Measured against the current build: a TLS 1.3 client resumes on its second connection, then fails to resume after an updateConfig() that changes nothing. Fix: the proxy owns the TlsConfigCache and threads it through every build_route_table, so an unchanged cert maps to the same Arc<ServerConfig> and keeps its session state. Keying on the cert bytes gives the right lifetime for free — sessions live as long as the cert they were issued under, and a rotation retires them. Two details worth keeping: - The sweep (retain_used, mark-and-sweep over keys touched during a build) is the caller's and runs at the commit point in proxy.rs, never inside build_route_table. updateConfig is all-or-nothing: a route build can succeed and then be discarded when the protection half fails validation, and sweeping against a table that never went live would retire configs the still-running table is serving from. Over-retaining for one generation is the safe direction. - Ticket keys stay per-config, not process-global. A shared ticketer would let a ticket minted under one tenant's cert resume against another tenant's route. Tests: unit tests for config identity across rebuilds (with the per-build cache as an explicit control) and for the sweep; an end-to-end spec covering TLS 1.2 and 1.3 resumption, survival across a reload, and correct non-resumption across a cert rotation. The reload assertions fail on the previous behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aff6593 to
6348b46
Compare
What this is
symphony already supports TLS session resumption, and it works — but only until the first config reload. This makes it survive one.
The problem
src/tls.rssets both halves of resumption on every terminatingServerConfig(rustls defaults to neither):session_storage = ServerSessionMemoryCache::new(1024)— TLS 1.2 session IDsticketer = ring::Ticketer::new()— TLS 1.3 ticketsBoth of those live on the
ServerConfig, andbuild_route_tablecreated its ownTlsConfigCacheper call. So every route-table rebuild minted a brand-newServerConfigfor every route — including routes whose cert had not changed at all — which discarded the session cache and generated fresh random ticket keys. Every ticket already handed to a client became undecryptable.Nothing errors when this happens. Clients just quietly go back to full handshakes.
That matters because reloads are routine: adding or removing a route, or an on-disk cert renewal picked up by the file watcher, rebuilds the whole table. In a multi-tenant deployment, resumption rarely survived long enough to do anything.
Measured against
mainwith a Node client, identical routes and an identical cert on both sides of the reload:The fix
The proxy owns the
TlsConfigCacheand threads it through everybuild_route_table, so an unchanged cert maps to the sameArc<ServerConfig>and keeps its session state across hot-swaps. Keying on the cert bytes gives the right lifetime for free: session state lives exactly as long as the cert it was issued under, and rotating a cert retires it.Two details are load-bearing and worth a close look:
retain_used()(mark-and-sweep over the keys touched during a build) runs inproxy.rsafter the new table is swapped in — never insidebuild_route_table.updateConfigis all-or-nothing: a route build can succeed and then be discarded when the protection half fails validation, and sweeping against a table that never went live would retire configs the still-running table is serving from. Over-retaining for one generation is the safe direction; under-retaining costs live session state.Not addressed (both documented in
CLAUDE.md)Ticketerrotates its keys on its own ~6h schedule, independent of reloads.Both degrade to a full handshake, never to an error.
Tests
router.rs—unchanged_cert_keeps_its_server_config_across_rebuildsasserts config identity across a rebuild, and carries the old per-build cache as an explicit control so the test states what the bug was.sweep_retires_configs_no_route_referencescovers the eviction side.__test__/session-resumption.spec.ts— end-to-end over both TLS 1.2 and 1.3: a session is issued and resumes, it survives anupdateConfig()that doesn't change the cert, and it correctly does not resume across a cert rotation.I confirmed the reload assertions fail on the previous behaviour before keeping them — reverting just the shared-cache change turns those two green tests red and leaves the rest passing.
Full suite: 110/110 JS, 107/107 Rust, clippy clean.
Note on the diff:
cargo fmtandprettier --writereformat this repo wholesale (it isn't formatter-clean today), so I reverted that churn and hand-matched the surrounding style. The diff is only the change.🤖 Generated with Claude Code