Repository navigation
Conversation
SSLConfig::from_generated returns None when no member of the options
object differs from its default. SocketConfig::from_generated stored
that None. So Bun.connect({ tls: {} }) opened a socket without TLS and
sent the first write in cleartext.
On a client, the object arm now stores the tls: true defaults when the
parser returns None. A tls option that is absent, undefined, null or
false keeps its meaning. Bun.listen does not change.
Collaborator
Author
|
Status: draft. It waits for a maintainer's choice between the three answers in the description (TLS, a throw, or no change). How I reproduced it: a plain Bun.connect({ hostname: "127.0.0.1", port, tls: {}, socket: { open(s) { s.write("SECRET"); }, data() {} } });
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Bun.connect({ tls: {} })opens a socket without TLS. The first write goes out in cleartext, with no error.src/runtime/socket/Handlers.rs:511. It stores theNonethatSSLConfig::from_generatedreturns when no member is set.Fix
Bun.connect, atlsobject that sets no option now gets thetls: trueconfig (tls_true_defaults,rejectUnauthorizedon). The other answers are a throw on the same line, or no change.tlsabsent,undefined,nullorfalsekeeps its meaning.Bun.listendoes not change.test/js/bun/net/socket.test.ts, 41 new cases, 29 fail without the fix. Also ran 49 more socket and TLS test files.Background
tls: {}and needs the plain socket.tls: {}as a request for TLS.Downsides
tls: {}breaks with no error:datanever runs. BunSmtp then hits its own 5-minute timer..textgrows 256 bytes.Notes
The question. The body of #44618 says: "One open question:
tls: {}(an object that names no TLS option) is plain TCP onBun.listen/Bun.connect, here and onmain. It is the same trap astls: [], but changing it changes a Bun default, so it is left alone." This PR is one tested answer for theBun.connecthalf. There is no urgency. The three answers:tls: truedefaults (this PR). Callers that meant TLS work with no release of their own. A caller that meant plaintext stalls with no event.tls: []. It fails closed for each shape and tells the author at once. It can become TLS in a later minor, because an error turns into a working call.socket.upgradeTLSandRedisClientalready throw for the same object.If the answer is 2, I rework the same arm. If it is 3, I close this.
Shapes that move. Each of these dialed plaintext on
Bun.connectand now sends a ClientHello:{},{ rejectUnauthorized: undefined },{ requestCert: false },{ ca: [] },{ ca: undefined, cert: undefined, key: undefined },{ ecdhCurve: "auto" },{ lowMemoryMode: false },{ secureOptions: 0 },{ checkServerIdentity() {} },{ unknownKey: 1 },Object.create(null). The first byte on the wire goes from 83 (Sof the application'sSECRET) to 22 (TLS handshake record), in the hostname, unix and fd forms.What a caller that meant plaintext sees. The handshake runs against its plain peer. With no
handshakehandler and a peer that speaks first: the promise resolves,openruns withauthorized === falseandgetAuthorizationError().code === "EPROTO", thencloseruns.data,errorandenddo not run. With a peer that waits, no handler runs. This is whattls: truedoes today. devmoamal/BunSmtp (src/relay.ts:83) dials an SMTP relay withtls: {}, reads the plaintext 220 greeting indata, and upgrades with STARTTLS. It registersdata,errorandendonly, so after this change its own 5-minute timer is the one signal.Why
Bun.listenis not in this PR. The first version changed both modes. slidesk 2.19.0 starts its telnet server withtls: { key, cert, passphrase }, and each member isundefinedwhen no certificate is configured. On main that is a plain listener. With the change it is a TLS listener with no certificate:listen()does not throw and the client gets nothing. No caller was found that needs a listener to refuse. A client can do TLS with no option, and a server cannot. So the arm has the guard!mode.is_server().Bun.serve({ tls: {} })also stays HTTP. The client arm alone stops the cleartext when both ends passtls: {}: the listener receives a ClientHello.Other effects of the arm.
handshakehandler,openruns after the handshake and not at TCP connect, as for each TLS socket.Bun.connect({ tls: { secureContext } })alone now reaches the code that adopts the context (Listener.rs:1184-1200).upgradeTLSon a socket fromBun.connect({ tls: {} })returnsundefined, because the socket is already TLS.Not changed, same parser result.
Bun.listen: atlsobject that sets no option is a listener without TLS.socket.upgradeTLS({ tls: {} })andupgradeDuplexToTLSthrowExpected "tls" option.test/js/bun/net/socket.test.ts:756-761pins the throw.new Bun.RedisClient(url, { tls: {} })throwsExpected tls to be a object for 'tls'.Bun.serve({ tls: {} })andtls: [{}]give anhttp:server.Bun.servealso parses its top-level options object with the same parser (ServerConfig.rs:1271), so the parser'sNonemust stay.{ requestCert: false }is not a reason for this PR. It stopped counting as a TLS option in 1.3.0 at each entry point, and that needs its own change in the parser.Why this line.
SocketConfig::from_jshas two callers,Listener::listen(SocketMode::Server) andListener::connect_inner(SocketMode::Client). This match is the only reader of the object arm of thetlsunion. Later frames see onlyssl: Option<SSLConfig>. node:net and node:tls pass through the same line and never send such an object: the client setsrequestCert: true.Bun.SQL.
src/js/internal/sql/shared.ts:2168-2174: "Explicit tls/ssl options request an encrypted connection: if the server declines TLS, the connection is aborted instead of continuing in plaintext."test/js/sql/adapter-env-var-precedence.test.ts:422-426pins the literaltls: {}tosslMode2. The rule is for a client only.Callers found.
bunqueue2.9.11 (7,597),@fuman/bun0.0.21 (4,961),bun-routeros1.1.0 (16),irc-client-ts0.23.1 (14),@ninots/mail0.1.2 (10),bun-kafka0.1.0 (10). Each builds the object from optional settings when its user asks for TLS, for exampletls: options.tls ? {} : undefined. I read the six call sites.Bun.connectorBun.listen. I did not check the other 136 classifications again. slidesk was not in it, so the survey is not complete.Bun.connect, slidesk forBun.listen. I read both sources.Measurements. Release builds of main 3f1765a with and without the patch, linux x64.
SocketConfig::from_jsown instructions, second call, gdbstepiwith calls stepped over (base → PR):tlsabsent 147 → 146,false151 → 149,true168 → 164,{ serverName }175 → 175,{ requestCert: true, rejectUnauthorized: true }175 → 175. Conditional branches for a configured object 16 → 17.memcpycalls 4/4/5/5/5 on both.match ..? { None => .., configured => configured }adds one 171-bytememcpyand 11 instructions to a configured object. A check after the outer match adds 13 instructions to each call. So the arm is one match on the parser'sResult.tls: true377 → 377,{ serverName }405 → 405.tls: {}with the patch: 401.tls: {}dials with the patch make the same calls as 100tls: truedials.Bun.connect14 (1 oftls) and plainBun.listen12 (1 oftls) on both builds.tls: {}onBun.connectgoes from 14/1 to 15/2, the same astls: true.SSL_CTX_newcalls for 1tls: truedial and 1,000tls: {}dials in one process: 1..text58,536,037 → 58,536,293 bytes.SocketConfig::from_js2,270 → 2,322 bytes. The stripped binary has the same size. No host function and no codegen entry is added.Tests. 41 new cases pass on the debug build of this branch. On a release build of main 3f1765a, 29 fail (26 by assertion, the 3 untrusted-certificate cases by timeout) and the 12 controls pass. The cases:
Bun.listenand aBun.connectthat both passtls: {}(1)tls: true(6)handshakehandler, in a child process withNODE_EXTRA_CA_CERTS(1)On Windows the unix transport is a named pipe, and one row is added to the Windows-only table that counts the
SSL_CTXof a named-pipe connect. I could not run Windows. I read the named-pipe arms (Listener.rs:1202-1442) and did not run them.The 50 files are
test/js/bun/net(10),test/js/node/tls(28),test/js/node/net(10),test/regression/issue/21792.test.tsandtest/js/bun/http/bun-connect-x509.test.ts. On the debug build, 19 other tests in them fail on my machine, and this change causes none. Eight need the public internet. Ten run past a timeout: eight of those also time out on a debug build of main, and two pass when run alone. One TLS reset test innode-tls-server.test.tsfails in a whole-file run on main too.Self-review. The review asked for these and they are done: the listener half is out, the body says that no issue exists and names the callers that lose, the
requestCertregression is out of the reasons, the Bun.SQL rule is cited, the landing order is stated, a test shows an accepted handshake with nohandshakehandler, a test pins what a plaintext peer gives, and the Windows named-pipe arm has a case. In part: the review also ran each of the six packages against a TLS-only port. I did not repeat that run, so its numbers are not here.#44618.
git merge-treeagainst its head 1c0cb80 gives one conflict hunk, insrc/runtime/socket/Handlers.rs. The resolution passes that branch'smode.is_server()tofrom_generatedandtls_true_defaults.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts