Repository navigation
[ADR40] Plaintext INFO before TLS: serves the attempt only, wait for the INFO after the first PONG on 2.12, bytes after INFO fail the handshake - #1666
Conversation
… after the first PONG on 2.12, bytes after INFO fail the handshake - With TLS after INFO, a context without tlsFirst and not websocket, the server's first INFO arrives in plaintext. Its fields serve the attempt and the connection it establishes. Its connect_urls and lame duck flag are not applied to the server pool and raise no event, whether or not the handshake succeeds. Plaintext connections, tlsFirst and websocket use the first INFO as before. - After the first PONG, when the server is 2.12.0 or later and discovered servers are not ignored, the attempt waits up to the connection timeout for the INFO the server sends inside TLS, so the pool is populated when connect returns. The wait counts against the attempt deadline, and losing the connection during the wait fails the attempt. - Bytes after the INFO line and before the handshake fail the attempt at once with "TLS handshake failed, the server sent data after INFO and before the handshake." and the server's first line quoted by quoteUntrusted, cut at 256 bytes with control characters escaped. On a plaintext connection or inside tlsFirst the bytes are protocol: NatsConnectionReader.preload parses them first, so a server's -ERR after INFO reaches the error listener instead of "Read past initial info message." - README paragraph under the TLS table and docs/TLS_CONFIGURATION.md section 9, including the consequence for servers before 2.12 and the two ways around it. - Tests: TLSConnectTests for the plaintext INFO not used, the post-PONG wait, lame duck ignored and data before the handshake; InfoHandlerTests for -ERR after INFO on plaintext and quoteUntrusted. SniTestServer reports 2.12.0 and sends its connect_urls INFO after the first PONG inside TLS, as nats-server 2.12 does, since the plaintext INFO no longer feeds the pool.
|
Claude finished @scottf's task in 3m 18s —— View job Review complete
This is a well-scoped follow-up to #1665 implementing ADR-40's three behavior changes (plaintext INFO scoped to the attempt only, waiting for the post-PONG INFO on 2.12+, and failing the handshake on bytes received after INFO). The mechanics — One concrete issue flagged inline on Connection-timeout double counting ( Everything else — the |
![synadia-claude-reviewer[bot]](https://avatars.githubusercontent.com/u/132956691?s=60&v=4){kind=small}
| if (waitingFor != null) { | ||
| long waitStart = NatsSystemClock.nanoTime(); | ||
| try { | ||
| waitingFor.get(options.getConnectionTimeout().toNanos(), TimeUnit.NANOSECONDS); |
There was a problem hiding this comment.
The wait for the post-PONG INFO uses a fresh full options.getConnectionTimeout() rather than the remaining budget of the current attempt (the pattern used everywhere else in this method via timeCheck(end, ...)). Combined with end += elapsed in the finally below (which erases this wait from the deadline for later timeCheck calls), a single tryToConnect attempt against a 2.12+ server can now take up to roughly 2x the configured connection timeout: once for the normal connect/TLS/ping sequence, and again for this wait. That also contradicts the PR description's "The wait counts against the attempt deadline" — the code does the opposite, re-basing end so the wait is excluded from the deadline. Since tryToConnect is shared by both connect and reconnect (lines 285/536), this extra latency/timeout risk applies to every reconnect against such a server too, not just the initial connect.
Consider bounding this wait by the remaining time in the attempt (e.g. reuse timeoutNanos/timeCheck(end, ...) like the rest of the method) instead of a brand-new full timeout.
There was a problem hiding this comment.
The code is the intended behavior. ADR-40 puts the wait for the INFO that follows the first PONG outside the attempt budget: it has a timer of its own that starts at the first PONG and lasts until that INFO arrives or the connection timeout passes. The end += in the finally keeps the rest of the attempt on its own budget, and docs/TLS_CONFIGURATION.md section 9 states the rule as "on a timer of its own". So one attempt against a 2.12 server can take up to the connection timeout twice, on connect and on reconnect, as you describe.
The PR description sentence "The wait counts against the attempt deadline" was wrong and is corrected. No code change.
Follows #1665, now merged. Three changes to how the client treats the server's first INFO when the TLS handshake follows it, the usual flow for
tls://andsecure()withouttlsFirst()and not websocket.The plaintext INFO serves the attempt only. Its nonce, limits, version and TLS flags are used for that attempt and the connection it establishes, as before. Its
connect_urlsand lame duck flag are no longer applied to the server pool and raise no discovered-servers or lame duck event, whether or not the handshake then succeeds. Plaintext connections,tlsFirst()and websocket use the first INFO exactly as before.The INFO after the first PONG is waited for. nats-server 2.12 and later send an INFO inside TLS right after the first PONG. When the server reports 2.12.0 or later and discovered servers are not ignored, the attempt waits for that INFO, up to the connection timeout, so the pool is populated when
connectreturns. The wait is outside the attempt budget, on a timer of its own equal to the connection timeout, so one attempt against such a server can take up to the connection timeout twice. Losing the connection during the wait fails the attempt. Against a server before 2.12 the pool now holds only the configured servers until the server sends an INFO for a cluster change; listing more servers in the options or usingtlsFirst()avoids that, anddocs/TLS_CONFIGURATION.mdsection 9 says so.Bytes after the INFO line and before the handshake fail the attempt. The message is "TLS handshake failed, the server sent data after INFO and before the handshake." followed by the server's first line as untrusted text, cut at 256 bytes with quotes, backslashes and control characters escaped. On a plaintext connection or inside
tlsFirst()those bytes are protocol: the reader parses them first, so a full server's-ERR 'maximum connections exceeded'sent right after INFO now reaches the error listener instead of failing as "Read past initial info message."Docs: README paragraph under the "TLS client versus server checks" table;
docs/TLS_CONFIGURATION.mdsection 9.Tests,
TLSConnectTests: a proxy that serves a plaintext INFO with otherconnect_urlsand then fails the handshake leaves the pool unchanged; the INFO after the first PONG is in the pool whenconnectreturns; a plaintext INFO withldmraises no lame duck event; a proxy that writes a line after INFO fails with the handshake message quoting that line.InfoHandlerTests:-ERRafter INFO on plaintext reaches the error listener;quoteUntrustedcases.SniTestServerreports version 2.12.0 and sends itsconnect_urlsINFO after the first PONG inside TLS, as nats-server 2.12 does, sinceTlsHostnameTestsrelied on the plaintext INFO feeding the pool. Four of the new tests fail on main; the post-PONG wait test passes there and guards the behavior.