This partually reverts commit 4cdb0e89d8.
A nullptr check in TSLWrap::IsAlive() and the added test were left.
PR-URL: https://github.com/nodejs/node/pull/11947
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
TLSWrap object keeps a pointer reference to the underlying
TCPWrap object. This TCPWrap object could be closed and deleted
by the event-loop which leaves us with a dangling pointer.
So the TLSWrap object needs to track the "close" event on the
TCPWrap object.
PR-URL: https://github.com/nodejs/node/pull/11776
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
The `exports` parameter is unnecessary.
PR-URL: https://github.com/nodejs/node/pull/10834
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <sam@strongloop.com>
`TLSSocket` should not have a hard dependency on `tls.Server`, since it
may be running without it in cases like `STARTTLS`.
Fix: #10704
PR-URL: https://github.com/nodejs/node/pull/10706
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Either the options or the listener argument to tls.createServer() was
optional, but not both. This makes no sense, so align the argument
checking and documentation with net.createServer(), which accepts the
same option sequence, and which tls.createServer() is modelled on.
PR-URL: https://github.com/nodejs/node/pull/9800
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Minor fix to favor strict equality in http_server.js and tls_wrap.js
to ensure accurate comparisons without type coercion.
PR-URL: https://github.com/nodejs/node/pull/9849
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The `.` character does not need to be escaped when it appears inside a
regular expression character class. This removes instances of
unnecessary escapes of the `.` character.
This also removes a few unnecessary escapes of the `(` and `)`
characters within character classes too.
PR-URL: https://github.com/nodejs/node/pull/9449
Ref: https://github.com/nodejs/node/pull/9749
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Writing data to TLSWrap instance during handshake will result in it
being queued in `write_item_queue_`. This queue won't get cleared up
until the end of the handshake.
Technically, it gets cleared on `~TLSWrap` invocation, however this
won't ever happen because every `WriteWrap` holds a reference to the
`TLSWrap` through JS object, meaning that they are doomed to be alive
for eternity.
To breach this dreadful contract a knight shall embark from the
`close` function to kill the dragon of memory leak with his magic
spear of `destroySSL`.
`destroySSL` cleans up `write_item_queue_` and frees `SSL` structure,
both are good for memory usage.
PR-URL: https://github.com/nodejs/node/pull/9586
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
In most cases, named functions match the variable or property to which
they are being assigned. That also seems to be the practice in a series
of PRs currently being evaluated that name currently-anonymous
functions.
This change applies that rule to instances in the code base that don't
comply with that practice.
This will be enforceable with a lint rule once we upgrade to ESLint
3.8.0.
PR-URL: https://github.com/nodejs/node/pull/9113
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Removes branch that would make TLSSocket emit '_tlsError' event if error
occured on handshake and control was not released, as it was never happening.
Addedd test for tls.Server to ensure it still emits 'tlsClientError' as expected.
Fixes: https://github.com/nodejs/node/issues/8803
PR-URL: https://github.com/nodejs/node/pull/8805
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Previously `TLSSocket#_emitTLSError` was used as an `error` event
handler. However that function can emit `error` event itself, so it is
not suitable for such use. Luckily the event can be emitted only when
the control is released, so this looping-error can't happen.
Replace the error handler for clarity and correctness.
PR-URL: https://github.com/nodejs/node/pull/8889
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
When node began using the OneByte API (f150d56) it also switched to
officially supporting ISO-8859-1. Though at the time no new encoding
string was introduced.
Introduce the new encoding string 'latin1' to be more explicit. The
previous 'binary' and documented as an alias to 'latin1'. While many
tests have switched to use 'latin1', there are still plenty that do both
'binary' and 'latin1' checks side-by-side to ensure there is no
regression.
PR-URL: https://github.com/nodejs/node/pull/7111
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
There is no official way to figure out if the socket that you have on
hand is still connecting to the remote host. Introduce
`Socket#connecting`, which is essentially an unprefixed `_connecting`
property that we already had.
PR-URL: https://github.com/nodejs/node/pull/6404
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Introduce a new MACRO to check if the data is a String object and
update existing MACROs to include the actual object description to
be printed in case of an error.
PR-URL: https://github.com/nodejs/node/pull/3100
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fail early in require('crypto'), require('tls'),
require('https'), etc when crypto is not available
(rather than depending on an internal try/catch).
Add documentation for detecting when crypto is not available.
PR-URL: https://github.com/nodejs/node/pull/5611
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Several changes:
* Soft-Deprecate Buffer() constructors
* Add `Buffer.from()`, `Buffer.alloc()`, and `Buffer.allocUnsafe()`
* Add `--zero-fill-buffers` command line option
* Add byteOffset and length to `new Buffer(arrayBuffer)` constructor
* buffer.fill('') previously had no effect, now zero-fills
* Update the docs
PR-URL: https://github.com/nodejs/node/pull/4682
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Instead of leaking the arguments object by passing it as an
argument to a function, copy it's contents to a new array,
then pass the array. This allows V8 to optimize the function
that contains this code, improving performance.
PR-URL: https://github.com/nodejs/node/pull/4361
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
The role of `this.server` is now split between `this._server` and
`this.server`. Where the first one is used for counting active
connections of `net.Server`, and the latter one is just a public API for
users' consumption.
The reasoning for this is simple, `TLSSocket` instances wrap
`net.Socket` instances, thus both refer to the `net.Server` through the
`this.server` property. However, only one of them should be used for
`net.Server` connection count book-keeping, otherwise double-decrement
will happen on socket destruction.
Fix: #5083
PR-URL: https://github.com/nodejs/node/pull/5262
Reviewed-By: James M Snell <jasnell@gmail.com>
This is an intermediate fix for an issue of accessing `TLSWrap` fields
after the parent handle was destroyed. While `close` listener cleans up
this field automatically, it can be done even earlier at the
`TLSWrap.close` call.
Proper fix is going to be submitted and landed after this one.
Fix: #5108
PR-URL: https://github.com/nodejs/node/pull/5168
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
This commit adds a new method for TLS sockets that returns the
negotiated protocol version.
PR-URL: https://github.com/nodejs/node/pull/4995
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
With an indentation style of two spaces, it is not possible to indent
multiline variable declarations by four spaces. Instead, the var keyword
is used on every new line.
Use const instead of var where applicable for changed lines.
PR-URL: https://github.com/nodejs/io.js/pull/2286
Reviewed-By: Roman Reiss <me@silverwind.io>
`clientError` will have `http.Server`-specific behavior, and we don't
want to shadow it in `tls.Server`.
PR-URL: https://github.com/nodejs/node/pull/4557
Reviewed-By: Brian White <mscdex@mscdex.net>
use `arrow functions` instead of `bind(this)` in order to improve
performance through optimizations.
PR-URL: https://github.com/nodejs/node/pull/3622
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Add `secureContext` option to `tls.connect`. It is useful for caching
client certificates, key, and CA certificates.
PR-URL: https://github.com/nodejs/node/pull/4246
Reviewed-By: James M Snell <jasnell@gmail.com>
FIPS 140-2 disallows use of MD5, which is used to derive the
default sessionIdContext for tls.createServer().
PR-URL: https://github.com/nodejs/node/pull/3866
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
FIPS 140-2 disallows use of MD5, which is used to derive the
default sessionIdContext for tls.createServer().
PR-URL: https://github.com/nodejs/node/pull/3755
Reviewed-By: Fedor Indutny <fedor@indutny.com>
This commit fixes some error messages that are not consistent with
some general rules which most of the error messages follow.
PR-URL: https://github.com/nodejs/node/pull/3374
Reviewed-By: Roman Reiss <me@silverwind.io>
Check that tls.connect() fails in the expected way when passing in
invalid minDHSize options.
PR-URL: https://github.com/nodejs/node/pull/3629
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
ALPN is added to tls according to RFC7301, which supersedes NPN.
When the server receives both NPN and ALPN extensions from the client,
ALPN takes precedence over NPN and the server does not send NPN
extension to the client. alpnProtocol in TLSSocket always returns
false when no selected protocol exists by ALPN.
In https server, http/1.1 token is always set when no
options.ALPNProtocols exists.
PR-URL: https://github.com/nodejs/node/pull/2564
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Upon creating a TLSSocket object, set the default isServer option to false
Updated tls docs and added test-tls-socket-default-options
PR-URL: https://github.com/nodejs/node/pull/2614
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Add a new option to specifiy a minimum size of an ephemeral DH
parameter to accept a tls connection. Default is 1024 bit.
PR-URL: https://github.com/nodejs/node/pull/1831
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Returns an object representing a type, name and size of an ephemeral
key exchange in a client connection. Currently only DHE and ECHE are
supported.
This api only works on on a client connection. When it is called on a
server connection, null is returned. When its key exchange is not
ephemeral, an empty object is returned.
PR-URL: https://github.com/nodejs/node/pull/1831
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
When closing the child TLSWrap handle - wait for the proper parent's
handle close callback invocation. `uv_close_cb` may be invoked much
later than the next libuv tick, depending on the platform.
The only platform that currently seem to defer `uv_close_cb` is Windows
XP. This behavior was not observed on other Windows systems, and is not
possible on Unixes.
Fix: https://github.com/nodejs/node/issues/2979
PR-URL: https://github.com/nodejs/node/pull/2991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Expose and use in TLSWrap an `v8::External` wrap of the
`StreamBase*` pointer instead of guessing the ancestor C++ class in
`node_wrap.h`.
Make use of `StreamBase::Callback` structure for storing/passing both
callback and context in a single object.
Introduce `GetObject()` for future user-land usage, when a child class
is not going to be inherited from AsyncWrap.
PR-URL: https://github.com/nodejs/node/pull/2351
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
As per the discussion in #734, this patch deprecates the usage of
`EventEmitter.listenerCount` static function in the docs, and introduces
the `listenerCount` function in the prototype of `EventEmitter` itself.
PR-URL: https://github.com/nodejs/node/pull/2349
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
When TLS Session Ticket is renewed by server - no Certificate record is
to the client. We are prepared for empty certificate in this case, but
this relies on the session reuse check, which was implemented
incorrectly and was returning false when the TLS Session Ticket was
renewed.
Use session reuse check provided by OpenSSL instead.
Fix: https://github.com/nodejs/io.js/issues/2304
PR-URL: https://github.com/nodejs/io.js/pull/2312
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Queued write requests should be invoked on handle close, otherwise the
"consumer" might be already destroyed when the write callbacks of the
"consumed" handle will be invoked. Same applies to the shutdown
requests.
Make sure to "move" away socket from server to not break the
`connections` counter in `net.js`. Otherwise it might not call `close`
callback, or call it too early.
Fix: https://github.com/iojs/io.js/issues/1696
PR-URL: https://github.com/nodejs/io.js/pull/1910
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Defer reading until user-land has a chance to add listeners. This
allows the TLS wrapper to listen for _tlsError and trigger a
clientError event if the socket already has data that could trigger.
Fixes: https://github.com/nodejs/io.js/issues/1114
PR-URL: https://github.com/nodejs/io.js/pull/1496
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
5795e835a1 introduced unintentional
copy-paste bug. `cb` is not actually present in those functions and
should not be called, the socket should be destroy instead.
PR-URL: https://github.com/nodejs/io.js/pull/1951
Reviewed-By: Roman Reiss <me@silverwind.io>
Trevor Norris
jBarz
Jackson Tian
Fedor Indutny
Sam Roberts
Walter Beller-Morales
Rich Trott
Mariusz 'koder' Chwalba
Teddy Katz
Ben Noordhuis
Sakthipriyan Vairamani
James M Snell
Nathan Woltman
Brian White
Michaël Zasso
Minwoo Jung
Stefan Budeanu
micnic
Shigeki Ohtsu
Yuval Brik
James Hartig
Roman Reiss
Malte-Thorben Bruns