tls: allow enabling the TLS debug trace #27376
Conversation
nodejs-github-bot
added
the
openssl
sam-github
added
semver-minor
|
The PR is unfortunately large, because I had to regenerate the openssl configuration. The last commit is quite small. |
@nodejs/platform-aix Any idea why https://github.com/sam-github/node/blob/tls-trace/src/tls_wrap.cc#L915-L931 or something in https://github.com/sam-github/node/blob/tls-trace/deps/openssl/openssl/ssl/t1_trce.c would pulling in an undefined C++ iostreams symbol? |
|
The 1.1.1 shared failure is because the tests expect SSL_trace to be available, but the openssl linked against doesn't have it. The code handles it, the tests don't. I'll fix that. |
|
All green. @mscdex @richardlau PTAL /cc @nodejs/crypto |
src/node_constants.cc
Outdated
| #else | ||
| # define OPENSSL_NO_SSL_TRACE 0 | ||
| #endif | ||
| NODE_DEFINE_CONSTANT(target, OPENSSL_NO_SSL_TRACE); |
There was a problem hiding this comment.
Is it even worth exposing OPENSSL_NO_SSL_TRACE? We don't expose any of the other OPENSSL_NO_* defines either.
I'd remove it if it's just for feature detection in the test. You could make TLSWrap::EnableTrace() return a boolean or throw an exception and use that for feature detection.
There was a problem hiding this comment.
Its annoyingly more complex than it might seem. The return value of enableTrace isn't seen on the server side (its called before the user code gets the TLSSocket), and since the tests have to call them in a sub-process to catch stderr, it makes a simple test skip more fragile than needed. I'll move these to the internal binding so they aren't visible in our API.
|
@bnoordhuis Do you have any idea why all the whitespace changes are ocurring? I see it even on master when I do a straight regen of the config: sam-github@e8e97c1 The timestamp changes are expected, but the whitespace in the asm files is not. I am the one who generated them last time, on the same laptop, though perhaps perl, or binutils, or something, has gotten a routine update, but I haven't figure out what changed yet. |
|
I changed the PR base to https://github.com/nodejs/node/tree/configure-openssl-trace, it makes it much easier to review. I'll redo the constants tomorrow. |
Enable the same trace output that the OpenSSL s_client and s_server support with their `-trace` option. This is invaluable when debugging reports of TLS bugs as well as when debugging the internal TLS implementation. See: - nodejs#25383 - nodejs#17936 - postmanlabs/postman-app-support#5918 (comment)
|
Landed in 1d4a880...adedbb1 |
OpenSSL doesn't compile `SSL_trace()` by default for historical reasons. Add the enable-ssl-trace option to the OpenSSL Configure arguments. PR-URL: #27376 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Ran: $ cd deps/openssl/config; make to update deps/openssl/config/archs/ with current configuration. PR-URL: #27376 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Enable the same trace output that the OpenSSL s_client and s_server support with their `-trace` option. This is invaluable when debugging reports of TLS bugs as well as when debugging the internal TLS implementation. See: - #25383 - #17936 - postmanlabs/postman-app-support#5918 (comment) PR-URL: #27376 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
OpenSSL doesn't compile `SSL_trace()` by default for historical reasons. Add the enable-ssl-trace option to the OpenSSL Configure arguments. PR-URL: #27376 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Ran: $ cd deps/openssl/config; make to update deps/openssl/config/archs/ with current configuration. PR-URL: #27376 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Enable the same trace output that the OpenSSL s_client and s_server support with their `-trace` option. This is invaluable when debugging reports of TLS bugs as well as when debugging the internal TLS implementation. See: - #25383 - #17936 - postmanlabs/postman-app-support#5918 (comment) PR-URL: #27376 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Notable changes:
* deps:
* Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
parser refuse any request URL that contained the "|" (vertical bar)
character. #27595
* tls:
* Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
option to `tls.createServer()`. When enabled, TSL packet trace
information is written to `stderr`. This can be used to debug TLS
connection problems. #27497
#27376
* cli:
* Added a `--trace-tls` command-line flag that enables tracing of TLS
connections without the need to modify existing application code.
#27497
* Added a `--cpu-prof-interval` command-line flag. It can be used to
specify the sampling interval for the CPU profiles generated by
`--cpu-prof`. #27535
* module:
* Added the `createRequire()` method. It allows to create a require
function from a file URL object, a file URL string or an absolute
path string. The existing `createRequireFromPath()` method is now
deprecated #27405.
* Throw on `require('./path.mjs')`. This is technically a breaking
change that should have landed with Node.js 12.0.0. It is necessary
to have this to keep the possibility for a future minor version to
load ES Modules with the require function.
#27417
* repl:
* The REPL now supports multi-line statements using `BigInt` literals
as well as public and private class fields and methods.
#27400
* The REPL now supports tab autocompletion of file paths with `fs`
methods. #26648
* meta:
* Added Christian Clauss (https://github.com/cclauss) to
collaborators. #27554
PR-URL: #27578
Notable changes:
* deps:
* Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
parser refuse any request URL that contained the "|" (vertical bar)
character. #27595
* tls:
* Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
option to `tls.createServer()`. When enabled, TSL packet trace
information is written to `stderr`. This can be used to debug TLS
connection problems. #27497
#27376
* cli:
* Added a `--trace-tls` command-line flag that enables tracing of TLS
connections without the need to modify existing application code.
#27497
* Added a `--cpu-prof-interval` command-line flag. It can be used to
specify the sampling interval for the CPU profiles generated by
`--cpu-prof`. #27535
* module:
* Added the `createRequire()` method. It allows to create a require
function from a file URL object, a file URL string or an absolute
path string. The existing `createRequireFromPath()` method is now
deprecated #27405.
* Throw on `require('./path.mjs')`. This is technically a breaking
change that should have landed with Node.js 12.0.0. It is necessary
to have this to keep the possibility for a future minor version to
load ES Modules with the require function.
#27417
* repl:
* The REPL now supports multi-line statements using `BigInt` literals
as well as public and private class fields and methods.
#27400
* The REPL now supports tab autocompletion of file paths with `fs`
methods. #26648
* meta:
* Added Christian Clauss (https://github.com/cclauss) to
collaborators. #27554
PR-URL: #27578
|
@sam-github , should this land on v10.x? Please add the |
|
It would be useful on v10.x, but I've no idea how hard it will be to backport. |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes