dtls: large update to the dtls implementation - #65511
Open
jasnell wants to merge 80 commits into
Open
Conversation
The test connects to the IP literal 127.0.0.1 with rejectUnauthorized defaulting to true and no servername, so the peer identity is verified against that IP. agent1-cert.pem is CN = agent1 with no subjectAltName, so verification fails with X509_V_ERR_IP_ADDRESS_MISMATCH before the default CA set is exercised at all. Pass servername so the identity is matched against the certificate CN, keeping verification enabled while testing what the file is named for. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
The error queue is per-thread and shared with every other OpenSSL consumer in the process. DTLS spends most of its time handling unauthenticated input, so failures are routine: rejected handshakes, and DTLSv1_listen() choking on garbage datagrams. None of those entries were discarded. ERR_get_error() in Cycle() and ClearOut() popped only the first entry, and nothing cleared the queue after a failed DTLSv1_listen(), SSL_write() or SSL_shutdown(). The residue was picked up by whatever crypto operation ran next and reported as its error: after 32 junk datagrams, crypto.createPrivateKey() on malformed PEM reported "record too small" with the real DECODER error demoted into opensslErrorStack. Add MarkPopErrorOnReturn to the entry points that drive OpenSSL, so each discards whatever it queued on the way out. Route error rendering through a helper that falls back to a description of the SSL error code when the queue is empty, instead of "error:00000000:lib(0)::reason(0)". Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
OpenSSL emits one BIO_write per DTLS record, each fragmented to fit SSL_set_mtu(). enc_out_ was a byte-stream BIO, so those boundaries were lost and EncOut() drained an entire handshake flight into one datagram, defeating the MTU setting. With an agent1 chain and mtu 512, the server flight went out as 60, 2490, 266 bytes -- the 2490 being five correctly sized records concatenated into one datagram that requires IP fragmentation, which NATs and middleboxes routinely drop. SSL_OP_NO_QUERY_MTU also disables OpenSSL's black-hole recovery, so such a handshake retransmits at the same broken size until it gives up. Use BIO_s_dgram_mem() for enc_out_, which returns exactly one datagram per BIO_read. It reports "empty" as a retry and grows on write, so it needs no BIO_set_mem_eof_return(). EncOut() now sends one record per iteration instead of one flight. Loopback has a 64 KiB MTU so no existing test could see this; the new one measures datagram sizes through a relay. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
A zero length datagram is legal UDP, costs the sender nothing and can never carry a DTLS record, but OnRecv() forwarded it to ProcessDatagram() like any other. With no matching session it reached AcceptConnection(), which spent an SSL_new(), two BIO_new()s, a DTLSv1_listen() and an SSL_free() establishing there was nothing there -- before any address validation, so the source is spoofable. It also blocks moving enc_in_ to a datagram BIO: a zero length BIO_write enqueues an empty datagram, and the subsequent BIO_read returns 0, which the record layer reads as EOF rather than "try again". Reject len == 0 in ProcessDatagram(), covering both the session and accept paths. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
OpenSSL's DTLS record layer assumes a BIO read returns exactly one datagram, and clamps a read to the bytes remaining in one. enc_in_ was a byte-stream BIO, where that count means "bytes remaining in the queue", so a record header declaring a length longer than its own datagram could consume bytes belonging to the next. Not reachable today: Receive() runs Cycle() after every BIO_write, and Cycle() drains, so enc_in_ never holds more than one datagram and the clamp lands on the boundary by coincidence. The invariant is an emergent property of when Cycle() runs rather than a property of the BIO, so anything that lets two datagrams queue turns it into a silent framing desync. Use BIO_s_dgram_mem(), matching enc_out_. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
SSL_get_verify_result() was never called or exposed, so there was no way to inspect the verification result or apply an authorization policy: an application could only get an opaque "certificate verify failed". Add session.authorized and session.authorizationError, the latter carrying the short X509 code such as 'CERT_HAS_EXPIRED'. Route the lookup through ncrypto's verifyPeerCertificate() rather than SSL_get_verify_result() directly, because the latter reports X509_V_OK when the peer sent no certificate at all. ncrypto reports that as absent, while still allowing for PSK and resumption, which is mapped to UNABLE_TO_GET_ISSUER_CERT to match node:tls. These are meaningful when rejectUnauthorized is false: OpenSSL verifies the chain under SSL_VERIFY_NONE and simply does not abort. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
createContext() tested rejectUnauthorized first and requestCert only as
an else-if, so { requestCert: true, rejectUnauthorized: false } set
SSL_VERIFY_NONE. No CertificateRequest was sent and the server saw no
peer certificate even when the client offered a valid trusted one. That
combination is the node:tls idiom for "ask for a certificate and let the
application decide", so code ported from node:tls lost client
authentication silently. rejectUnauthorized also wrongly implied
requestCert.
Follow node:tls and drive the server off requestCert first:
requestCert: false -> SSL_VERIFY_NONE
requestCert, rejectUnauthorized -> PEER | FAIL_IF_NO_PEER_CERT
requestCert, !rejectUnauthorized -> PEER
and the client off rejectUnauthorized alone.
The permissive verify callback is installed in exactly one case, the
server that asked for a certificate but disabled rejection, because it is
the only combination where OpenSSL would otherwise abort a handshake the
application wants to judge.
Also validate requestCert, and CHECK the arguments to setVerifyMode
instead of Int32Value(...).FromJust() on an unchecked value.
Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
SSL_CTX_set_keylog_callback() was called unconditionally, so every handshake's CLIENT_RANDOM and master secret were formatted and copied into V8 strings whether or not the application had set onkeylog -- the JS side only gated delivery. Once a secret is a JS string it is reachable from heap snapshots, core dumps and the inspector for as long as the string lives. node:tls installs its keylog callback only when a listener is attached. Match that: add a has_keylog_listener flag to the shared session state, set it from the onkeylog setter, and return from SSLKeylogCallback before touching V8 when it is clear. Registration also moves to DTLSContext, since keylog is a per-SSL_CTX setting that was being rewritten once per session. While adding a state field, pin the session state offsets with static_asserts the way the endpoint state already does. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Every datagram arriving at a listening endpoint that did not match an existing session went straight to AcceptConnection(), which spent an SSL_new(), two BIO_new()s, a DTLSv1_listen() and an SSL_free() before concluding it was not a ClientHello. None of that is gated on anything the sender had to prove, so a spoofed-source flood bought that work at the cost of a UDP send. Screen the datagram first: handshake content type, DTLS version major, a record length that fits the datagram, and a client_hello handshake type. Deliberately structural -- parsing the ClientHello is OpenSSL's job, and getting it wrong would turn away real clients. Under a 30000 datagram flood the server absorbed all of them at 2.8us each, against roughly half of them at 6.1us each before. Add endpointStats.serverRejectedCount so this traffic is visible. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
session_count was written in five places and read in none: there was no limit on how many sessions a listening endpoint would hold. Each owns an SSL, two BIOs and a retransmit timer, so a peer willing to complete cookie exchanges could grow the table until the process ran out of memory. Cookie exchange proves a peer can receive at its claimed address, so this is not spoofable, but it does not bound what that peer may do. Add maxSessions (default 10000) and maxSessionsPerHost (default 1000), checked in AcceptConnection before anything is allocated. The per-host cap is the one that matters: without it a single peer can take the entire table. It is keyed on IP only, so a peer cannot evade it by varying source port, and erases entries at zero so it tracks live peers. A refused peer gets silence rather than an alert: it has not completed cookie exchange, so replying would make this an amplification vector. A real client retransmits and is admitted once there is room. Refusals are counted by endpointStats.serverRefusedCount. Either cap can be set to 0 to disable it. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
EncOut() and the HelloVerifyRequest path each declared a 64 KiB stack buffer to receive a datagram that is normally around 1200 bytes. EncOut() is reached from Cycle(), which can re-enter, so those frames can nest. Both were large enough to force a page-probing prologue. Now that both BIOs are datagram BIOs, BIO_pending() reports the size of the next datagram exactly, so the read can be sized to it. Use MaybeStackBuffer, which keeps the common case on the stack. Sizing from BIO_pending() also removes the possibility of a short read truncating a record, which is what a datagram BIO does when the buffer is too small. EncOut()'s frame drops from over 4 KiB with probing to 1560 bytes without. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Neither layer checked it. The JS wrapper passed the argument straight through, and the binding did Int32Value(...).FromJust() and handed the result to std::vector<uint8_t>(length), where a negative value became a huge size_t. Three ordinary-looking arguments terminated the process: session.exportKeyingMaterial(-1, label) -> core dump session.exportKeyingMaterial(4294967295, label) -> core dump session.exportKeyingMaterial(1e12, label) -> core dump Validate in JS the way node:tls does, and CHECK in the binding rather than coercing, since by then a bad value is our bug and not the caller's. Also bound the length at 65536. RFC 5705 sets no limit and node:tls does not impose one, but node:tls allocates through a BackingStore, which fails gracefully, whereas std::vector aborts. 65536 is three orders of magnitude above the largest defined exporter, DTLS-SRTP's 60 bytes. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
SocketAddress::Hash covers family, port and address. SocketAddress::Map paired it with operator==, which memcmps the whole sockaddr and so also compares sin_zero, sin6_flowinfo and sin6_scope_id. Keys that hash the same could compare unequal, putting one peer in two entries of a single bucket. The DTLS session table is the only user of Hash, and it is keyed on the peer address, so a peer whose padding differed between two datagrams would get a second session rather than matching its existing one. The kernel zeroes sin_zero on receive, so this is latent today; it stops being latent as soon as addresses reach the table from anywhere other than a recvmsg. Add SocketAddress::Equal alongside the existing IpHash/IpEqual pair and use it in the Map alias. Equal covers scope_id and Hash now folds it in: two link-local peers reachable as the same address on different interfaces are genuinely different peers. flowinfo is a QoS label and stays out of both. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
ComputeCookie() HMACed the raw sockaddr bytes. For IPv4 that spans
sin_zero, and for IPv6 sin6_flowinfo: padding the kernel is not obliged
to zero, and a QoS label that can legitimately differ between two
datagrams from one host. Either changes the cookie for an unchanged peer,
which fails the handshake, since the peer echoes the cookie it was given
and the server recomputes a different one.
Serialise {family, port, address, scope id} instead. scope id stays in
because it identifies a link-local peer. The cookie format is
process-local and lives for one time window, so changing it costs
nothing.
Also value-initialise current_cookie_peer_, so an unset value reports an
unknown family and ComputeCookie() fails closed rather than deriving a
cookie from stale bytes.
Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
The server cache mode was SSL_SESS_CACHE_SERVER | NO_AUTO_CLEAR. That pairing is only coherent alongside NO_INTERNAL, the way node:tls uses it, where there is no internal cache for the auto-clear to walk. With the internal cache enabled it meant nothing ever evicted anything: every accepted session stayed, with its master secret, for the 7200 second default timeout and beyond. Over 700 sequential handshakes from a non-ticket client, all 700 were retained. Only reachable for peers that do not offer session tickets, which excludes node's own client but not much of the CoAP/IoT population. Dropping NO_AUTO_CLEAR alone does nothing: the periodic flush only removes expired entries and only on a 255-session boundary. Use NO_INTERNAL, matching node:tls and the client branch below it. This gives up server-side session-id resumption for non-ticket clients, which nothing exercised and no API could drive; ticket resumption is stateless and unaffected. Also set a session id context, defaulting the way node:tls does from a hash of process.argv, and expose it as the sessionIdContext option. OpenSSL will not resume a session whose id context differs from the accepting SSL's, which keeps a session issued under one configuration from being resumed under another. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
The wire format is one length byte followed by that many bytes. The encoder wrote Buffer.from([buf.length]) with no range check, so a 256-byte name truncated to a zero length byte and desynchronised the rest of the list, and an empty string emitted a zero-length entry that RFC 7301 does not allow. A pre-encoded Buffer was passed through unchecked: alpn: ['a'.repeat(256)] ERR_CRYPTO_OPERATION_FAILED mid-handshake alpn: [''] negotiated, malformed list on the wire alpn: Buffer.from([0,0x68,32]) silently negotiated nothing alpn: Buffer.from([9,0x68,32]) silently negotiated nothing Range-check each name at 1..255 the way node:tls's convertProtocols does, reporting the offending index, and walk a supplied Buffer so a malformed list is refused where it is passed. Rejecting the empty name diverges from node:tls, which only checks the upper bound. It cannot be represented on the wire, so nothing valid is turned away. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
The selection callback returned SSL_TLSEXT_ERR_NOACK when the server's list and the client's offer had nothing in common. That completes the handshake with no protocol agreed, leaving both peers connected with no idea what to speak. RFC 7301 section 3.2 requires a fatal no_application_protocol alert, and node:tls made this same change. This is a behaviour change. A mismatch that used to connect now fails with "tlsv1 alert no application protocol". Only the no-overlap return changes. The earlier return for a server with no ALPN configured stays NOACK: a client offering protocols to a server that does not do ALPN is not an error, and OpenSSL only invokes the callback when the client sent the extension. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
send() returned a bare -1 both for a payload too large for a DTLS record and for a send attempted before the handshake finished. Nothing distinguished the two, -1 is not documented, and `session.send(data)` written as a statement discards the value, so the data went missing with no indication. The same method already threw for a destroyed session and for a bad argument type. Throw instead, naming the cause: before handshake ERR_INVALID_STATE > 16384 bytes ERR_OUT_OF_RANGE, giving the size and the limit SSL_write failure ERR_CRYPTO_OPERATION_FAILED closed/destroyed ERR_INVALID_STATE, unchanged The size limit is the maximum plaintext record, 2^14, not the MTU: a record larger than the path MTU is fragmented by IP, so with mtu 1200 both 1400 and 16384 byte sends succeed and arrive. This is a behaviour change for callers testing `send(x) < 0`. Also documents that a successful return means handed to the socket, not received by the peer. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
close(), destroy() and a peer-initiated close all settled `closed` and left `opened` pending. Tearing a session down before its handshake finished therefore left anything awaiting `opened` waiting forever, with no error and no timeout. They now reject with ERR_INVALID_STATE, or with the error given to destroy() so that a caller awaiting `opened` learns the same thing as one awaiting `closed`. Guarded by a flag rather than relying on a settled promise ignoring a second settle, so a handshake that already completed is not overwritten and one that failed on its own keeps its real error. Only reachable for teardown before the handshake completes. A peer that never replies is a different case: the retransmit timer runs to DTLS1_TMO_ALERT_COUNT first, and does eventually settle. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
connect() bound the local socket to '0.0.0.0' whatever the peer was. That
is an AF_INET socket, which cannot send to an AF_INET6 destination, so
connecting to an IPv6 peer could not work. Default the bind address to
'::' when the host argument is an IPv6 literal.
Only the client's hardcoded bind default was wrong. Both Bind() and
Connect() already use the auto-family SocketAddress::New(), so
listen({ host: '::1' }) was supported.
isIP() only parses, so this stays synchronous. A host name returns 0 and
keeps the IPv4 default: connect() still does not resolve names, which is
now documented rather than implied.
test-dtls-ipv6.mjs is gated on common.hasIPv6 and covers the round trip,
the defaulting, an explicit bindHost, and the IPv4 case.
Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
SendTo() treated uv_udp_try_send() as successful only when it returned exactly the buffer length, and fell through to the queued uv_udp_send() path otherwise. Any other non-negative return would put the same datagram on the wire twice. Not reachable: a datagram is sent whole or not at all, and libuv documents the non-negative return as always matching the buffer size. There is no partial send to resume. Test for >= 0 instead, and queue only on EAGAIN. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Two bits of context setup that did not describe themselves accurately. No behaviour change. SSL_OP_ALL was taken wholesale under the comment "enable all workarounds for maximum compatibility". Its membership is not stable across versions, so the macro means inheriting whatever a future OpenSSL puts in it. Name the four bits instead; both evaluate to 0x80000850 against the bundled OpenSSL 3.5.7. All four are TLS-specific and inert under DTLS 1.2, and the comment now says so per flag. They are kept rather than dropped because interop with an odd peer is not something the suite can check. SSL_OP_COOKIE_EXCHANGE was set on the temporary SSL immediately before DTLSv1_listen(), which sets it on that same SSL itself. Drop it, and explain the distinction the neighbouring comment was reaching for: the option is wrong on the context, because every session SSL would inherit it including ones whose cookie exchange has completed, and right on the individual SSL. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
has_message_listener was written by JS from the onmessage setter and never read by C++. ClearOut() copied every datagram's plaintext into a JS Buffer and dispatched it regardless, for [kSessionMessage] to find no handler and drop it. Read the flag, as has_keylog_listener already is. Reading from OpenSSL continues either way, so data is still drained rather than accumulating; only the allocation and the crossing into JS are skipped. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
BIO_new() was used without checking the result, so an allocation failure would have passed nullptr to PEM_write_bio_X509(). Use ncrypto::BIOPointer::NewMem(), which is what the rest of the tree uses, and bail if it fails. RAII also removes the manual BIO_free(). Not otherwise reachable, and there was no leak: BIO_free() ran unconditionally and tolerates nullptr. Also documents that only the leaf is returned, with no chain, and that the parsed fields node:tls exposes are unavailable, so a caller reaching for subject or fingerprint finds out from the docs rather than from an undefined property, and is pointed at session.authorized for the verification result. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
No code change; both are places where the behaviour is intentional but nothing said so. The MTU is read by DTLSSession when it builds its SSL, so setMTU() only affects sessions created afterwards, and the option is fixed for the life of the endpoint. Also corrects what the value means: it bounds the datagram, not the application payload, which is smaller once the record header and MAC are counted. LoadDefaultCAs() populates the verification store but not the client-CA list sent in a CertificateRequest. The bundled root store holds on the order of 150 certificates, and advertising all of their distinguished names would make a CertificateRequest of tens of kilobytes, which over a datagram transport has to be fragmented across many losable packets. node:tls takes the same position. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Documentation only. The cookie secret is generated once per context and not rotated. That is deliberate: rotation would need the old secret kept alive to validate cookies already in flight, which is what the 30s time window already does, and a fresh secret per context means a restart invalidates outstanding cookies anyway. The cookie is also not bound to the ClientHello, which RFC 6347 section 4.2.1 recommends. Binding it via SSL_get_client_random() does not work: the random is not populated consistently across the generate and verify callbacks during DTLSv1_listen(), so no cookie verifies. The comment records the approach that would work -- lifting the 32-byte random out of the raw ClientHello, which CouldBeClientHello() already walks. The cookie's purpose, proving the peer receives at the address it claims, does not depend on it. session.state and endpoint.sessions are marked not-public: state is a shared-memory flag view, and sessions is the live Set rather than a copy, so mutating it desynchronises the JS and C++ views of which sessions exist. Callers are pointed at session.opened/closed and endpoint.state. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
ToLocalChecked() aborts the process if the handle is empty, which happens on allocation failure or when execution is being terminated -- a worker being torn down, or process.exit() during a callback. src/crypto, the nearest comparable code, uses ToLocalChecked() twice in the whole directory; this code had seventeen. Ten of them are here: the accessors that hand a value straight back to JS and the three strings getCipher assembles. All are the last thing their function does, so returning early on failure leaves the property undefined, which is what these accessors already return when there is nothing to report. getCipher now builds its three strings before creating the object rather than inline in each Set(), so a failure part way through cannot leave a half-populated object as the return value. The remaining seven are the callback paths, which need individual care. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
The remaining seven ToLocalChecked() calls, in the paths that build arguments for a JS callback. src/dtls now has none. Unlike the accessors these are not all tail positions, and an early return is wrong in three. Each skips only the emit: Cycle(), SSL_ERROR_SSL -- owes cycle_depth_-- on the way out. Returning early leaks the increment and wedges the reentrancy guard for the rest of the session's life. Cycle(), handshake-complete -- sits mid-function. The application-data read below it and the same decrement still have to run. ClearOut()'s drain loop -- continues rather than returns. Leaving early would strand the remaining records. The other four end their block anyway, so skipping straight out matches what the code already did. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
OnRecv() took libuv's flags argument and never looked at it, so a datagram flagged UV_UDP_PARTIAL would have been passed to OpenSSL as though it were whole. A truncated datagram is not a short DTLS record, it is a corrupt one. It cannot currently fire: libuv raises UV_UDP_PARTIAL from MSG_TRUNC, which the kernel sets only when a datagram did not fit the supplied buffer, and OnAlloc always supplies 65536, above the largest possible UDP payload. The value of the check is that shrinking that buffer now degrades to dropped packets rather than corrupt records. The mmsg bits cannot fire either, since this uses plain uv_udp_init(). That is recorded where it matters rather than checked for: OnAlloc hands out one reused buffer on the assumption that datagrams arrive one at a time, and enabling recvmmsg would silently invalidate it. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Every credential failure threw ERR_CRYPTO_OPERATION_FAILED with a fixed string naming the OpenSSL function, discarding the reason OpenSSL had already put in the error queue. A malformed PEM, a key that does not match the certificate and an encrypted key with no passphrase were indistinguishable. Use crypto::ThrowCryptoError() with ERR_get_error(), which is what node:tls does at the same call sites: encrypted key, no passphrase ERR_OSSL_BAD_DECRYPT malformed key ERR_OSSL_UNSUPPORTED malformed certificate ERR_OSSL_PEM_NO_START_LINE key does not match cert ERR_OSSL_X509_KEY_VALUES_MISMATCH This matters most for the encrypted-key case, which is about to become supportable: without it, the wrong passphrase reports exactly what no passphrase reports, and neither mentions decryption. Scoped to the paths that parse caller-supplied credentials. The other sites report failures to apply settings, where the fixed string is already the whole story. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Both were compared rather than coerced -- `=== true` and `!== false` --
so a value that was not a boolean took the branch it did not look like:
createSecureContext({ isServer: 'yes' }) // a client context
connect(..., { rejectUnauthorized: 0 }) // verification stays on
Neither failed open: a client context is refused by listen(), and 0
meaning "verify" is the safe reading. But both decide something
security-relevant from a value the caller plainly meant the other way,
and said nothing.
Checked with validateBoolean where they are read, as requestCert already
was. Comparing rather than coercing stays.
Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
isConnected is documented as false once a stats object is no longer tracking anything. Nothing ever set it. kFinishClose was defined on both stats classes and imported by the module, and no caller invoked it, so the flag was true for the lifetime of the object. Reading them after a close was safe -- the AliasedStruct's backing store is a shared_ptr the ArrayBuffer keeps alive -- so there was no dangling pointer, only numbers that had stopped moving with nothing saying so. Called now on every path a session or endpoint ends by: the peer closing, close(), destroy(), and the endpoint's close callback. That snapshots the values, so the last state stays readable, and flips isConnected. node:quic, which these stats were modelled on, calls it from its close paths. The symbol and both implementations came across; the calls did not. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
A libuv failure was rethrown as ERR_INVALID_STATE carrying only uv_strerror()'s text, which dropped both the errno and the syscall: code=ERR_INVALID_STATE errno=undefined syscall=undefined code=EADDRINUSE errno=-98 syscall=bind The second is what net and dgram give for the same condition, and err.code === 'EADDRINUSE' is how this is normally handled. Against DTLS that could never pass, and ERR_INVALID_STATE is also what the module throws for a closed session, so the two were indistinguishable. Thrown with ThrowUVException instead. Rebinding a bound endpoint now reports EALREADY. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
…class rejectUnauthorized: false was documented as not verifying the certificate. It verifies it and continues, reporting authorized false with an authorizationError, which is what makes those two properties worth reading and what the prose further down the page already said. session.closed was "Resolves when the session is fully closed". It rejects when the session was destroyed with an error, or when its endpoint was. session.authorizationError was written as an escaped literal, which renders the brackets instead of linking and silences the missing- reference warning rather than answering it. The reference is defined now. Callback properties and session[Symbol.asyncDispose]() were under "Class: DTLSSession.Stats", which documents the stats object. They are members of DTLSSession and are now inside it. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
session.servername, session.endpoint, session.destroyed and endpoint.destroyed are on the prototypes and none appeared in the documentation. session.endpoint is worth stating plainly: on a server session it is the listening endpoint itself, shared with every other session on it, so a session handler holds the whole listener. connect() accepts handshakeTimeout and only listen() listed it, so the option looked server-only. servername is undefined when the client sends no name, which the entry now says rather than leaving "the SNI name" to imply otherwise. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Four calls whose failure was ignored. SSL_CTX_set_min/max_proto_version pin the context to DTLS 1.2. Refusing DTLS 1.0 is the point of setting them -- RFC 8996 deprecates it and it has no AEAD suites -- so an OpenSSL that rejected the call left a context whose floor was the version being excluded. Checked together, since either failing has that effect. BIO_write and BIO_ADDR_new in the cookie-exchange path are allocation failures. An unwritten BIO would have put DTLSv1_listen() to work on an empty buffer, and a null BIO_ADDR is not something it accepts. The datagram is dropped and the peer retransmits. uv_udp_recv_start on the connect path was ignored where Listen() checks it and unwinds. A failure there left a session in the table that no datagram could reach, reported only by its handshake timing out a minute later. It now unwinds the same way and throws the libuv error. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Returning 0 from the PSK callback tells OpenSSL there is no PSK, and it was what every failure took. A callback returning the wrong shape, a non-string identity, a key that was not a view, or a value too long for the buffer all reached the caller identically: error:0A0000DF:SSL routines::psk identity not found which names nothing the caller did and is also what a genuinely absent PSK produces. Each now reports what was wrong with what it gave back, through the same pending-error path an exception from the callback already used. An empty identity or key still returns 0 silently: that one really is "no PSK". Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
send() took a Buffer or a string and refused a Uint8Array, which is the obvious thing to send, while exportKeyingMaterial() on the same object accepted one. Bare ArrayBuffers stay refused, as they are there too. The gate was Buffer.isBuffer() in JavaScript. The binding's check was Buffer::HasInstance(), which is defined as IsArrayBufferView() and so had been accepting every view all along. It is spelled IsArrayBufferView() now, and reads the bytes through ArrayBufferViewContents, so what it takes is stated rather than inherited from what a Buffer happens to be. A view sends the bytes it covers and not the buffer behind it: a subarray, a DataView at an offset, and an Int16Array all arrive as the bytes they span. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Five options only a server can act on were handled four different ways when a client named one: sni threw, sessionIdContext was ignored, ticketKeys was applied to a client that has no tickets to issue, and requestCert was validated and then ignored. All refused now, by one rule checked before any of them is read. A client naming one has misunderstood the option, and the difference between "ignored" and "applied" was not something a caller could see. sni's own check goes away in favour of the shared one. pskIdentityHint names which key a client should pick. Given without psk there was no key to name, so it was dropped and the handshake failed for want of a PSK without mentioning the option that had been set. Each option is still accepted by a server context, so the rule is about which side may use it. ticketKeys and sni keep their own validation. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
unwrapSession folded the Buffer check in with the prefix and length checks, so all four failures reported ERR_INVALID_ARG_VALUE. Passing a string got the code that means the type was right and the contents were wrong. Split out. A Buffer that is not one of ours still reports ERR_INVALID_ARG_VALUE, which is what it is: the right type, contents that cannot be resumed. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
The binding says "Session is closed" where JavaScript says "Session is destroyed" for what looks like the same situation. The first is unreachable: JavaScript drops the handle on close and on destroy, and send() refuses a null handle before the binding is reached. That holds for a peer-initiated close too, where the close callback clears the handle before control returns to user code. The guard stays, because being unreachable today is not a reason to write into a closed SSL if that changes. The comment records why its wording is not being brought into line with a message it will never appear beside. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Bind() set UV_UDP_IPV6ONLY for every IPv6 address, unconditionally. An
endpoint on :: therefore served IPv6 only and an IPv4 peer could not
reach it, with nothing to say so and no way to ask for anything else:
listen(..., { host: '::' })
connect('127.0.0.1', port) // handshake timeout
node:dgram and node:quic both bind dual stack by default. DTLS does now
too, and ipv6Only: true selects the old behaviour.
A dual-stack socket reports IPv4 peers with mapped addresses,
::ffff:127.0.0.1 rather than 127.0.0.1, so maxSessionsPerHost and
anything else keyed on the peer address sees them in that form.
The plumbing is a setSocketOptions() binding method read by Bind().
Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
An endpoint took whatever socket the system gave it. There was no way to spread a server over several processes, and no way to give it room for bursts the default buffers drop. reusePort sets SO_REUSEPORT, where the kernel spreads datagrams between everyone bound to the port. Not SO_REUSEADDR, which libuv also offers and node:dgram exposes: on Linux that lets the last binder take the port from a running server. Without reusePort the port stays exclusive. udpReceiveBufferSize, udpSendBufferSize and udpTTL are applied once the bind succeeds, since there is no socket to set them on before that. Not naming one leaves the system default rather than substituting a number of ours. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Both blocks enumerate the options they take and neither mentioned the five added for the UDP socket. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Mentioning C++ in the dtls.md doc exposes implementation detail Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
bind() moved to a symbol key so an endpoint cannot be rebound from outside. test-permission-net-dtls.mjs still called endpoint.bind() and had been failing with: TypeError: endpoint.bind is not a function which assert.throws() reported as the wrong error rather than as a missing method, so it read like a permission-check failure. Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
The entry read "live and updated data flows through the endpoint". The session equivalent reads "updated as data flows". Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
ReportPSKError took a const char* and passed it to ToV8Value(), which already has a std::string_view overload. Every call site hands it a literal, so the length is known rather than recovered with strlen(). Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell
added
net
Collaborator
|
Review requested:
|
nodejs-github-bot
added
lib / src
Member
Author
Review guideMost of the following was AI agent generated, verified by me. 80 commits is a lot to read end to end, so here is a route through them. The commits are ordered by dependency, not by theme, so the groups below jump around the history; each commit appears in exactly one group. Every commit builds and passes the suite on its own, so anything here can be checked out and run in isolation. Numbers are positions in the branch, oldest first. Datagram framing and the BIO layerOpenSSL's DTLS record layer assumes one BIO read yields exactly one datagram. The module used byte-stream BIOs, so that assumption held only by accident. Start here: several later commits depend on both BIOs being datagram BIOs.
Denial of service and resource boundsWork an unauthenticated peer could make the server do, and limits on what an authenticated one can hold or retain.
Peer address identityThe session table is keyed on the peer address, so what counts as the same peer matters. Note that 910a8cf changes shared code and 6f27438 reverts that part -- read them together; the net effect on node_sockaddr is additive only.
Certificate verification and peer identityThere was no way to see why a handshake was rejected, and two paths where verification silently did not happen.
ALPNProtocol list encoding and what happens when nothing is shared.
New features: secure contexts, SNI, PSK, resumptionThe largest group and the bulk of the new API surface. Read in order -- the later commits fix interactions the earlier ones created.
Exception safety and OpenSSL error reportingCallbacks that run inside SSL_do_handshake() cannot report anything to JavaScript from where they stand, and OpenSSL's error queue is shared process-wide.
Session and endpoint lifecyclePromises that never settled, and ordering between a session reaching JavaScript and its handshake running.
Public surface and argument validationOptions that reached a CHECK in the binding (a caller typo aborting the process), and internals that were reachable as public API.
Sockets and addressingWhich local socket an endpoint binds, and the UDP options it exposes.
Allocation gatingTwo paths that built V8 values whether or not anything was listening.
DocumentationCorrections and additions. 7988ed8 is structural (heading levels only, anchors preserved); the rest are content.
HousekeepingTest fixes and mechanical cleanups.
Worth a closer lookBehaviour changes that could affect an existing user of the experimental module:
Security-relevant:
Notes for the reviewer
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65511 +/- ##
==========================================
+ Coverage 90.15% 90.18% +0.03%
==========================================
Files 751 751
Lines 253439 254280 +841
Branches 47740 47736 -4
==========================================
+ Hits 228484 229332 +848
+ Misses 16216 16208 -8
- Partials 8739 8740 +1
🚀 New features to boost your workflow:
|
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
2 participants
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.
node:dtlslanded with the transport working but with many gaps. This addresses those, and fills in the API surface.This is a large PR but the commits are structured logically and sequentially. I chose to keep multiple PRs rather than squashing due to the size. Each has it's own description. I recommend stepping through and reviewing commit-by-commit.
A separate review guide comment will be included.