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,cli: add --trace-tls command-line flag #27497

Merged
merged 3 commits into from May 2, 2019

Conversation

Projects
None yet
7 participants
@cjihrig
Copy link
Contributor

commented Apr 30, 2019

This PR adds a --trace-tls CLI flag. The purpose is to enable tracing of TLS connections without the need to modify existing application code.

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.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

I think all that's needed is the environment variable support since adedbb1 already landed?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

I think adedbb1 would only support tracing secure sockets created in a server context. tls.connect() would still require changing application code.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

I see. Maybe the description should mention this is for client sockets only?

@sam-github

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

re:

I think adedbb1 would only support tracing secure sockets created in a server context. tls.connect() would still require changing application code.

Both client and server required changing app code, but client needed tls.connect().enableTrace(), it didn't use an option property because I often found it useful to turn trace on only after handshake.

The option property makes sense, though, it makes the API a bit more consistent.

2 suggestions:

  • consider instead of using an env var, have a CLI option --enable-tls-trace (or --tls-enable-trace?), then it can be done with an env var (with NODE_OPTIONS) or by CLI, we don't have to be so opinionated as to how users will want to set it.
  • consider having the ENV var (CLI option?) set the intial value of tls.DEFAULT_ENABLE_TRACE to true, so that the property can be set globally in code with tls.DEFAULT_ENABLE_TRACE = true (this is the pattern for a number of the existing CLI/env controllable defaults)
@richardlau

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Does this need a brand new environment variable? It looks like it could be a category for the existing NODE_DEBUG. Or as @sam-github suggests it could be a command line option that could be enabled via NODE_OPTIONS.

Show resolved Hide resolved doc/api/tls.md Outdated
Show resolved Hide resolved doc/api/tls.md Outdated
@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Both client and server required changing app code

Sorry, I meant that adding a new environment variable alone was not adequate for the client.

It looks like it could be a category for the existing NODE_DEBUG

That's actually my preference, and was my first idea. However, the format of the output is different enough that I didn't think it made sense.

I'll move this to a CLI flag.

@cjihrig cjihrig force-pushed the cjihrig:tls-trace branch from b76f16a to f181020 Apr 30, 2019

@cjihrig cjihrig force-pushed the cjihrig:tls-trace branch from f181020 to 6094f0a Apr 30, 2019

@cjihrig cjihrig changed the title tls,cli: add NODE_TLS_TRACE environment variable tls,cli: add --trace-tls command-line flag Apr 30, 2019

@nodejs-github-bot

This comment has been minimized.

@Trott

Trott approved these changes May 2, 2019

@nodejs-github-bot

This comment has been minimized.

Copy link

commented May 2, 2019

cjihrig added some commits Apr 30, 2019

tls: simplify enableTrace logic
PR-URL: #27497
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
tls: support enableTrace in TLSSocket()
This commit adds the enableTrace option to the TLSSocket
constructor. It also plumbs the option through other relevant
APIs.

PR-URL: #27497
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
tls,cli: add --trace-tls command-line flag
This commit adds a --trace-tls command-line flag. The
purpose is to enable tracing of TLS connections without the
need to modify existing application code.

PR-URL: #27497
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@cjihrig cjihrig force-pushed the cjihrig:tls-trace branch from 6094f0a to 495822f May 2, 2019

@cjihrig cjihrig merged commit 495822f into nodejs:master May 2, 2019

1 of 2 checks passed

Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Passed
Details

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

tls: simplify enableTrace logic
PR-URL: #27497
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

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

tls: support enableTrace in TLSSocket()
This commit adds the enableTrace option to the TLSSocket
constructor. It also plumbs the option through other relevant
APIs.

PR-URL: #27497
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

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

tls,cli: add --trace-tls command-line flag
This commit adds a --trace-tls command-line flag. The
purpose is to enable tracing of TLS connections without the
need to modify existing application code.

PR-URL: #27497
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

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

2019-05-07, Version 12.2.0 (Current)
Notable changes:

* 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
* 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 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
* meta:
  * Added Christian Clauss (https://github.com/cclauss) to
    collaborators. #27554

PR-URL: TODO

@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 6, 2019

2019-05-07, Version 12.2.0 (Current)
Notable changes:

* 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
* 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 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
* 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

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

@cjihrig cjihrig deleted the cjihrig:tls-trace branch May 16, 2019

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.