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: expose keylog event on TLSSocket #27654

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
9 participants
@jmendeth
Copy link
Contributor

commented May 11, 2019

This exposes SSL_CTX_set_keylog_callback as a keylog event that is emitted on tls.TLSSocket and tls.Server. It enables easy debugging of TLS connections with software like Wireshark, see #2363.

SSL_CTX_set_keylog_callback is only invoked when the keylog event is actually subscribed, so this shouldn't affect performance otherwise. The implementation is pretty similar to the session event. This is my first PR, I think I'm not forgetting anything :)

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
@addaleax
Copy link
Member

left a comment

This is a really nice first contribution :)

@nodejs-github-bot

This comment has been minimized.

@jmendeth

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

Thanks! :)

Show resolved Hide resolved test/parallel/test-tls-keylog-tlsv13.js Outdated
Show resolved Hide resolved test/parallel/test-tls-keylog-tlsv13.js Outdated
Show resolved Hide resolved doc/api/tls.md Outdated
Show resolved Hide resolved doc/api/tls.md Outdated
@sam-github
Copy link
Member

left a comment

A lot of people have been asking for this feature, including me! Its great, thank you. A couple suggestions made.

Show resolved Hide resolved lib/_tls_wrap.js Outdated
Show resolved Hide resolved lib/_tls_wrap.js Outdated
Show resolved Hide resolved test/parallel/test-tls-keylog-tlsv13.js Outdated
Show resolved Hide resolved test/parallel/test-tls-keylog-tlsv13.js Outdated
Show resolved Hide resolved test/parallel/test-tls-keylog-tlsv13.js Outdated
Show resolved Hide resolved test/parallel/test-tls-keylog-tlsv13.js Outdated
@bnoordhuis
Copy link
Member

left a comment

This change conflicts with #18896 because it leaves key data in the JS heap.

I guess the C++ code could be changed to create a String::ExternalOneByteStringResource that points to mlocked/madvised memory outside the heap but that's unreliable. For example, String#slice() might copy the memory back to the JS heap.

It would be better to emit a Buffer. That way Node has complete control over where and how the memory is allocated.

Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
@nodejs-github-bot

This comment has been minimized.

Show resolved Hide resolved doc/api/tls.md Outdated
Show resolved Hide resolved doc/api/tls.md Outdated
Show resolved Hide resolved lib/_tls_wrap.js Outdated
@bnoordhuis
Copy link
Member

left a comment

Thanks, LGTM with a suggestion.

CI: https://ci.nodejs.org/job/node-test-pull-request/23105/

Show resolved Hide resolved doc/api/tls.md Outdated
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member

commented May 14, 2019

We can squash the commits as they land, or you can do it now, and force push. As you wish. What is your preference?

tls: expose keylog event on TLSSocket
Exposes SSL_CTX_set_keylog_callback in the form of a `keylog` event
that is emitted on clients and servers. This enables easy debugging
of TLS connections with i.e. Wireshark, which is a long-requested
feature.

Refs: #2363

@jmendeth jmendeth force-pushed the jmendeth:feature/tls-keylog branch from 41d81e1 to 06f1af5 May 14, 2019

@jmendeth

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I have no specific preference ^^ I've squashed if it's more convenient to you

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

Trott approved these changes May 14, 2019

@danbev

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Landed in 53bef42 🎉

@danbev danbev closed this May 15, 2019

pull bot pushed a commit to Pandinosaurus/node that referenced this pull request May 15, 2019

tls: expose keylog event on TLSSocket
Exposes SSL_CTX_set_keylog_callback in the form of a `keylog` event
that is emitted on clients and servers. This enables easy debugging
of TLS connections with i.e. Wireshark, which is a long-requested
feature.

PR-URL: nodejs#27654
Refs: nodejs#2363
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@jmendeth jmendeth deleted the jmendeth:feature/tls-keylog branch May 15, 2019

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

tls: expose keylog event on TLSSocket
Exposes SSL_CTX_set_keylog_callback in the form of a `keylog` event
that is emitted on clients and servers. This enables easy debugging
of TLS connections with i.e. Wireshark, which is a long-requested
feature.

PR-URL: #27654
Refs: #2363
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

BridgeAR added a commit that referenced this pull request May 21, 2019

2019-05-21, Version 12.3.0 (Current)
Notable changes:

* process:
  * Log errors using `util.inspect` in case of fatal exceptions
    (Ruben Bridgewater) #27243
* repl:
  * Add `process.on('uncaughtException')` support (Ruben Bridgewater)
    #27151
* stream:
  * Implemented `Readable.from` async iterator utility (Guy Bedford)
    #27660
* tls:
  * Expose built-in root certificates (Ben Noordhuis)
    #26415
  * Support `net.Server` options (Luigi Pinca)
    #27665
  * Expose `keylog` event on TLSSocket (Alba Mendez)
    #27654
* worker:
  * Added the ability to unshift messages from the `MessagePort`
    (Anna Henningsen) #27294

@BridgeAR BridgeAR referenced this pull request May 21, 2019

Merged

v12.3.0 proposal #27799

4 of 4 tasks complete

BridgeAR added a commit that referenced this pull request May 21, 2019

2019-05-21, Version 12.3.0 (Current)
Notable changes:

* esm:
  * Added the `--experimental-wasm-modules` flag to support
    WebAssembly modules (Myles Borins & Guy Bedford)
    #27659
* process:
  * Log errors using `util.inspect` in case of fatal exceptions
    (Ruben Bridgewater) #27243
* repl:
  * Add `process.on('uncaughtException')` support (Ruben Bridgewater)
    #27151
* stream:
  * Implemented `Readable.from` async iterator utility (Guy Bedford)
    #27660
* tls:
  * Expose built-in root certificates (Ben Noordhuis)
    #26415
  * Support `net.Server` options (Luigi Pinca)
    #27665
  * Expose `keylog` event on TLSSocket (Alba Mendez)
    #27654
* worker:
  * Added the ability to unshift messages from the `MessagePort`
    (Anna Henningsen) #27294

PR-URL: #27799

BridgeAR added a commit that referenced this pull request May 21, 2019

2019-05-21, Version 12.3.0 (Current)
Notable changes:

* esm:
  * Added the `--experimental-wasm-modules` flag to support
    WebAssembly modules (Myles Borins & Guy Bedford)
    #27659
* process:
  * Log errors using `util.inspect` in case of fatal exceptions
    (Ruben Bridgewater) #27243
* repl:
  * Add `process.on('uncaughtException')` support (Ruben Bridgewater)
    #27151
* stream:
  * Implemented `Readable.from` async iterator utility (Guy Bedford)
    #27660
* tls:
  * Expose built-in root certificates (Ben Noordhuis)
    #26415
  * Support `net.Server` options (Luigi Pinca)
    #27665
  * Expose `keylog` event on TLSSocket (Alba Mendez)
    #27654
* worker:
  * Added the ability to unshift messages from the `MessagePort`
    (Anna Henningsen) #27294

PR-URL: #27799
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.