Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: allow enabling the TLS debug trace #27376

Closed

Conversation

Projects
None yet
6 participants
@sam-github
Copy link
Member

commented Apr 24, 2019

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:
- https://github.com/nodejs/node/issues/25383
- https://github.com/nodejs/node/issues/17936
- https://github.com/postmanlabs/postman-app-support/issues/5918#issuecomment-465311423
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

The PR is unfortunately large, because I had to regenerate the openssl configuration. The last commit is quite small.

Show resolved Hide resolved lib/_tls_wrap.js Outdated
@nodejs-github-bot

This comment has been minimized.

@sam-github sam-github force-pushed the sam-github:tls-trace branch from 37903e2 to 84a3026 Apr 24, 2019

@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

not ok 1932 parallel/test-tls-enable-trace
  ---
  duration_ms: 0.370
  severity: fail
  exitcode: 1
  stack: |-
    Could not load program node:
    rtld: 0712-001 Symbol _ZTTNSt7__cxx1119basic_ostringstreamIcSt11char_traitsIcESaIcEEE was referenced
          from module node(), but a runtime definition
    	    of the symbol was not found.

@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?

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

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.

@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

All green. @mscdex @richardlau PTAL

/cc @nodejs/crypto

Show resolved Hide resolved test/parallel/test-tls-enable-trace.js Outdated
Show resolved Hide resolved src/node_constants.cc Outdated
Show resolved Hide resolved src/tls_wrap.cc Outdated
@nodejs-github-bot

This comment has been minimized.

Show resolved Hide resolved lib/_tls_wrap.js Outdated
@bnoordhuis
Copy link
Member

left a comment

If you strip the whitespace changes from the first commit, it slims down to a more manageable +252 −652.

You could trim it down even further by dropping the mostly irrelevant buildinf.h changes but that's arguably falsifying history. :-)

Show resolved Hide resolved doc/api/tls.md Outdated
#else
# define OPENSSL_NO_SSL_TRACE 0
#endif
NODE_DEFINE_CONSTANT(target, OPENSSL_NO_SSL_TRACE);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 25, 2019

Member

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.

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 25, 2019

Author Member

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.

Show resolved Hide resolved src/tls_wrap.cc Outdated

@sam-github sam-github force-pushed the sam-github:tls-trace branch from 6c0b0ab to bf86076 Apr 25, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@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.

@sam-github sam-github force-pushed the sam-github:tls-trace branch from bf86076 to a4efe28 Apr 25, 2019

@sam-github sam-github changed the base branch from master to configure-openssl-trace Apr 25, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

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.

tls: allow enabling the TLS debug trace
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)

@sam-github sam-github force-pushed the sam-github:tls-trace branch from a4efe28 to d481424 Apr 26, 2019

@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

Landed​ in 1d4a880...adedbb1

@sam-github sam-github closed this Apr 29, 2019

@sam-github sam-github deleted the sam-github:tls-trace branch Apr 29, 2019

sam-github added a commit that referenced this pull request Apr 29, 2019

deps: configure OpenSSL's SSL_trace to be built
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>

sam-github added a commit that referenced this pull request Apr 29, 2019

deps: update archs files for OpenSSL-1.1.1b
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>

sam-github added a commit that referenced this pull request Apr 29, 2019

tls: allow enabling the TLS debug trace
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>

targos added a commit that referenced this pull request Apr 30, 2019

deps: configure OpenSSL's SSL_trace to be built
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>

targos added a commit that referenced this pull request Apr 30, 2019

deps: update archs files for OpenSSL-1.1.1b
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>

targos added a commit that referenced this pull request Apr 30, 2019

tls: allow enabling the TLS debug trace
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>

@targos targos referenced this pull request May 6, 2019

Merged

v12.2.0 proposal #27578

targos added a commit that referenced this pull request May 7, 2019

2019-05-07, Version 12.2.0 (Current)
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

targos added a commit that referenced this pull request May 7, 2019

2019-05-07, Version 12.2.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.