Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 20 additions & 26 deletions src/js/node/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -383,16 +383,15 @@ const SocketHandlers: SocketHandler = {
// will be handled in onConnectEnd
return;
}
// The second argument is "authorized" (handshake + verification +
// hostname), matching the public Bun.connect handshake callback. node:tls
// decides what to do with verification results in JS via the
// rejectUnauthorized / checkServerIdentity handling below, so a
// verification-class result (an X509 code such as
// UNABLE_TO_VERIFY_LEAF_SIGNATURE, or the native hostname verdict) still
// means the TLS session itself was established. Only a fatal TLS protocol
// failure tears the socket down here: those arrive as EPROTO carrying the
// OpenSSL "error:...:SSL routines:..." reason (or an already decomposed
// ERR_SSL_* / ERR_OSSL_* code).
// The second argument is "authorized" (handshake + chain verification),
// matching the public Bun.connect handshake callback. node:tls decides
// what to do with verification results in JS via the rejectUnauthorized /
// checkServerIdentity handling below, so a verification-class result (an
// X509 code such as UNABLE_TO_VERIFY_LEAF_SIGNATURE) still means the TLS
// session itself was established. Only a fatal TLS protocol failure tears
// the socket down here: those arrive as EPROTO carrying the OpenSSL
// "error:...:SSL routines:..." reason (or an already decomposed ERR_SSL_*
// / ERR_OSSL_* code).
const isProtocolFailure =
!success &&
verifyError?.code != null &&
Expand All @@ -407,9 +406,7 @@ const SocketHandlers: SocketHandler = {
self._securePending = false;
self.secureConnecting = false;
// ECONNRESET and protocol-level failures returned above, so reaching here
// means the TLS session itself was established - even when `success`
// (authorized) is false purely because of the native hostname verdict,
// which arrives with no error object.
// means the TLS session itself was established.
Comment thread
robobun marked this conversation as resolved.
Resolved
self._secureEstablished = true;

self.emit("secure", self);
Expand Down Expand Up @@ -1119,16 +1116,15 @@ const SocketHandlers2: SocketHandler<NonNullable<import("node:net").Socket["_han
// will be handled in onConnectEnd
return;
}
// The second argument is "authorized" (handshake + verification +
// hostname), matching the public Bun.connect handshake callback. node:tls
// decides what to do with verification results in JS via the
// rejectUnauthorized / checkServerIdentity handling below, so a
// verification-class result (an X509 code such as
// UNABLE_TO_VERIFY_LEAF_SIGNATURE, or the native hostname verdict) still
// means the TLS session itself was established. Only a fatal TLS protocol
// failure tears the socket down here: those arrive as EPROTO carrying the
// OpenSSL "error:...:SSL routines:..." reason (or an already decomposed
// ERR_SSL_* / ERR_OSSL_* code).
// The second argument is "authorized" (handshake + chain verification),
// matching the public Bun.connect handshake callback. node:tls decides
// what to do with verification results in JS via the rejectUnauthorized /
// checkServerIdentity handling below, so a verification-class result (an
// X509 code such as UNABLE_TO_VERIFY_LEAF_SIGNATURE) still means the TLS
// session itself was established. Only a fatal TLS protocol failure tears
// the socket down here: those arrive as EPROTO carrying the OpenSSL
// "error:...:SSL routines:..." reason (or an already decomposed ERR_SSL_*
// / ERR_OSSL_* code).
const isProtocolFailure =
!success &&
verifyError?.code != null &&
Expand All @@ -1143,9 +1139,7 @@ const SocketHandlers2: SocketHandler<NonNullable<import("node:net").Socket["_han
self._securePending = false;
self.secureConnecting = false;
// ECONNRESET and protocol-level failures returned above, so reaching here
// means the TLS session itself was established - even when `success`
// (authorized) is false purely because of the native hostname verdict,
// which arrives with no error object.
// means the TLS session itself was established.
self._secureEstablished = true;

self.emit("secure", self);
Expand Down
45 changes: 2 additions & 43 deletions src/runtime/socket/socket_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1627,36 +1627,9 @@ impl<const SSL: bool> NewSocket<SSL> {
success
);

let mut authorized = success == 1;
let mut hostname_mismatch = false;
let authorized = success == 1;

if SSL && authorized && !handlers.mode.is_server() {
if let Some(ssl_ptr) = this.socket.get().ssl() {
let hostname: &[u8] = if let Some(server_name) = this.server_name.get() {
&server_name[..]
} else if let Some(super::listener::UnixOrHost::Host { host, .. }) =
this.connection.get()
{
&host[..]
} else {
b""
};
if !hostname.is_empty()
&& !bun_boringssl::check_server_identity(
boringssl_sys::SSL::opaque_mut(ssl_ptr),
hostname,
)
{
authorized = false;
hostname_mismatch = true;
}
}
}

this.update_flags(|f| {
f.set(Flags::AUTHORIZED, authorized);
f.set(Flags::HOSTNAME_MISMATCH, hostname_mismatch);
});
this.update_flags(|f| f.set(Flags::AUTHORIZED, authorized));

let mut callback = handlers.on_handshake();
let mut is_open = false;
Expand Down Expand Up @@ -2083,19 +2056,6 @@ impl<const SSL: bool> NewSocket<SSL> {
// is very usefull to have this feature depending on the user workflow
let ssl_error = this.socket.get().get_verify_error();
if ssl_error.error_no == 0 {
if this.flags.get().contains(Flags::HOSTNAME_MISMATCH) {
let mismatch = SystemError {
errno: 0,
code: BunString::clone_utf8(b"HOSTNAME_MISMATCH"),
message: BunString::clone_utf8(b"Hostname mismatch"),
path: BunString::EMPTY,
syscall: BunString::EMPTY,
hostname: BunString::EMPTY,
fd: c_int::MIN,
dest: BunString::EMPTY,
};
return Ok(mismatch.to_error_instance(global));
}
return Ok(JSValue::NULL);
}

Expand Down Expand Up @@ -3831,7 +3791,6 @@ bitflags::bitflags! {
/// `us_socket_raw_write` (bypassing the SSL layer) so node:net can pipe
/// pre-handshake bytes / read the underlying TCP stream.
const BYPASS_TLS = 1 << 9;
const HOSTNAME_MISMATCH = 1 << 11;
}
}

Expand Down
118 changes: 73 additions & 45 deletions test/js/node/tls/node-tls-connect-hostname-verification.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,16 +103,60 @@ describe("tls.connect hostname verification without explicit servername", () =>
assert.strictEqual(await promise, "localhost");
});
});

// The native handshake reports only the cert-chain result; hostname
// verification is checkServerIdentity's job. A user-supplied override that
// accepts the mismatch must result in an authorized, secure-established
// socket, matching Node.
test("user checkServerIdentity that accepts a mismatch yields _secureEstablished=true, authorized=true", async () => {
await withServer(async port => {
const { promise, resolve, reject } = Promise.withResolvers<{
_secureEstablished: boolean;
authorized: boolean;
authorizationError: unknown;
}>();
const socket = tls.connect({
host: "localhost",
port,
ca,
rejectUnauthorized: true,
checkServerIdentity: () => undefined,
});
socket.on("secureConnect", () => {
resolve({
_secureEstablished: socket._secureEstablished,
authorized: socket.authorized,
authorizationError: socket.authorizationError,
});
socket.destroy();
});
socket.on("error", err => {
socket.destroy();
reject(err);
});
const result = await promise;
assert.deepStrictEqual(
{ _secureEstablished: result._secureEstablished, authorized: result.authorized },
{ _secureEstablished: true, authorized: true },
);
assert.ok(
result.authorizationError == null,
`authorizationError should be unset, got ${result.authorizationError}`,
);
});
});
});

describe("Bun.connect TLS hostname verification", () => {
describe("Bun.connect TLS handshake reports chain verification only", () => {
// The server presents the agent1 cert (CN=agent1, no SAN) signed by ca1.
// A client that trusts ca1 and connects to "localhost" passes chain
// validation, but the certificate is not valid for "localhost", so the
// socket must not be reported as authorized.
test("reports authorized=false when a CA-trusted cert does not match the connected hostname", async () => {
// The native handshake result reflects the BoringSSL cert-chain verify
// only; hostname verification is left to the caller (node:tls does it via
// checkServerIdentity). So a CA-trusted cert whose CN/SAN does not match
// the connected hostname still reports success=true / authorized=true, and
// getAuthorizationError() is null.
test("hostname mismatch with a CA-trusted cert still reports success=true, authorized=true", async () => {
const listener = Bun.listen({
hostname: "localhost",
hostname: "127.0.0.1",
port: 0,
tls: { key: serverKey, cert: serverCert },
socket: {
Expand All @@ -124,16 +168,26 @@ describe("Bun.connect TLS hostname verification", () => {
},
});
try {
// Mismatch: connect host "localhost" vs cert CN "agent1".
const mismatch = Promise.withResolvers<{ flag: boolean; arg: boolean; error: NodeJS.ErrnoException | null }>();
const badSocket = await Bun.connect({
hostname: "localhost",
// Mismatch: serverName "localhost" vs cert CN "agent1".
const mismatch = Promise.withResolvers<{
success: boolean;
authorized: boolean;
authorizationError: NodeJS.ErrnoException | null;
verifyError: unknown;
}>();
const socket = await Bun.connect({
hostname: "127.0.0.1",
port: listener.port,
tls: { ca },
tls: { ca, serverName: "localhost" },
socket: {
open() {},
handshake(s, success) {
mismatch.resolve({ flag: s.authorized, arg: success, error: s.getAuthorizationError() });
handshake(s, success, verifyError) {
mismatch.resolve({
success,
authorized: s.authorized,
authorizationError: s.getAuthorizationError(),
verifyError,
});
s.end();
},
data() {},
Expand All @@ -148,39 +202,13 @@ describe("Bun.connect TLS hostname verification", () => {
},
});
const result = await mismatch.promise;
badSocket.end();
assert.strictEqual(result.arg, false, "handshake callback must not report success for a hostname mismatch");
assert.strictEqual(result.flag, false, "socket.authorized must be false for a hostname mismatch");
assert.ok(result.error, "getAuthorizationError() must report why the socket is not authorized");
assert.strictEqual(result.error.code, "HOSTNAME_MISMATCH");

// Legitimate case: same cert, but the client asks for server name
// "agent1", which matches the certificate. Must remain authorized.
const match = Promise.withResolvers<boolean>();
const goodSocket = await Bun.connect({
hostname: "localhost",
port: listener.port,
tls: { ca, serverName: "agent1" },
socket: {
open() {},
handshake(s) {
match.resolve(s.authorized);
s.end();
},
data() {},
drain() {},
close() {},
error(_s, err) {
match.reject(err);
},
connectError(_s, err) {
match.reject(err);
},
},
socket.end();
assert.deepStrictEqual(result, {
success: true,
authorized: true,
authorizationError: null,
verifyError: null,
});
const okAuthorized = await match.promise;
goodSocket.end();
assert.strictEqual(okAuthorized, true, "a certificate matching the requested server name must stay authorized");
} finally {
listener.stop(true);
}
Expand Down
Loading