Repository navigation
feat: encrypt the login packet when encrypt is false - #1825
arthurschreiber wants to merge 2 commits into
Conversation
When `encrypt` is `false`, tedious now advertises `ENCRYPT_OFF` instead of `ENCRYPT_NOT_SUP` in PRELOGIN. If the server supports encryption and responds with `ENCRYPT_OFF`, the first packet of the LOGIN7 message is sent over TLS, and the rest of the connection stays unencrypted, matching the login-only encryption behavior of other SQL Server drivers. The server certificate is not validated for login-only encryption. If the TLS handshake fails, the connection fails with an error explaining that the login packet could not be encrypted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tion Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4b2d82e to
43992ca
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1825 +/- ##
==========================================
+ Coverage 84.76% 84.84% +0.07%
==========================================
Files 96 96
Lines 4720 4751 +31
Branches 884 889 +5
==========================================
+ Hits 4001 4031 +30
Misses 455 455
- Partials 264 265 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cheenamalhotra
left a comment
There was a problem hiding this comment.
Looks good overall. A few hardening suggestions inline.
Follow-up idea (not blocking): when we send ENCRYPT_OFF and the server replies ENCRYPT_REQ, SqlClient upgrades to full encryption, while we still throw EENCRYPT (Server requires encryption...). Could be a separate PR.
| this.outgoingMessageStream.pause(); | ||
|
|
||
| securePair.encrypted.on('data', forwardEncrypted); | ||
| securePair.cleartext.write(packet, (err) => { |
There was a problem hiding this comment.
This relies on the encrypted bytes reaching forwardEncrypted before the write callback runs. If they arrive later, the listener is already removed and the session destroyed, so the login packet is lost and the connection hangs. Tests pass, but it depends on Node/native-duplexpair timing.
Safer: drain what is left before destroying, e.g.
let chunk;
while ((chunk = securePair.encrypted.read()) !== null) {
this.socket.write(chunk);
}| }; | ||
|
|
||
| this.outgoingMessageStream.unpipe(this.socket); | ||
| this.outgoingMessageStream.once('data', (packet: Buffer) => { |
There was a problem hiding this comment.
If the connection is aborted or times out after the handshake but before LOGIN7 is sent, this once('data') listener and the TLS session are never cleaned up. Suggest tearing them down on socket close as well.
| private encryptNextPacket(securePair: { cleartext: tls.TLSSocket, encrypted: Duplex }, onSecureError: (err: Error) => void) { | ||
| // The TLS record size must not be capped below the packet size, as the | ||
| // server expects the encrypted packet to arrive in a single TLS record. | ||
| securePair.cleartext.setMaxSendFragment(MessageIO.MAX_TLS_SEND_FRAGMENT_SIZE); |
There was a problem hiding this comment.
packetSize can be up to 32767, but a TLS record is max 16 KB. If the first LOGIN7 packet is larger than that, it is split into two records, which the server rejects. Maybe cap the first packet or document the limit.
| done(); | ||
| }); | ||
|
|
||
| request.on('row', function(columns) { |
There was a problem hiding this comment.
Asserts inside the row handler throw outside done, so a failure shows up as an uncaught error. Suggest collecting the values here and asserting them in the request callback.
Closes #1822.
When
encryptisfalse, tedious advertisedENCRYPT_NOT_SUPin PRELOGIN, so the LOGIN7 packet (including the SQL authentication password, which is only obfuscated) was always sent unencrypted, even when the server supports TLS.This changes tedious to match the login-only encryption behavior of SqlClient, JDBC and ODBC:
encrypt: falsenow advertisesENCRYPT_OFF.ENCRYPT_OFF, a TLS handshake is performed and the first packet of the LOGIN7 message is sent over TLS. The TLS session is then discarded and the connection continues unencrypted. Encrypting only the first packet is what SqlClient does (TdsParserStateObject.cs), and SQL Server rejects or hangs on any other split.ENCRYPT_NOT_SUP,ENCRYPT_ON/ENCRYPT_REQandencrypt: 'strict'behave as before.Behavior notes
These are documented on the
encryptandtrustServerCertificateoptions:trustServerCertificate. This protects credentials against passive eavesdropping, not against an active man-in-the-middle.packetSizeof4096(default) or more, this always covers the SQL authentication password. Access tokens and other authentication data are not covered.Failed to connect to <server> - TLS handshake to encrypt the login packet failed: <reason>(ESOCKET). Servers that only support old TLS versions can be allowed viacryptoCredentialsDetails(e.g.{ minVersion: 'TLSv1' }).secureevent is not emitted for login-only encryption.Performance
No change for the query path;
sendMessageand the post-login socket piping are untouched.Connection setup for
encrypt: falsenow includes a TLS handshake. Against SQL Server 2025 on Linux (local Docker), mean connect time went from 74 ms to 100 ms, the same asencrypt: true, which is unchanged. Tracing shows almost all of this is the server's response time to the TLS ClientHello (~25 ms); the client-side overhead is below 1 ms.Tests
packetSize: 512.encrypt: falsewithpacketSize4096and512, verifying login-only TLS was negotiated andencrypt_optionisFALSE.🤖 Generated with Claude Code