-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
[v10.x] tls: expose keylog event on TLSSocket #31582
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @nodejs/lts would have a better idea whether renaming file makes things better, or is just another source of merge conflicts. I'm OK with leaving the file name as-is.
wait, I see there's |
It's okay to disable the functionality with |
15c9ce9
to
1ff0c68
Compare
Okay! I've suppressed the |
@mildsunrise Do you want me to nominate you for collaborator status? I.e., get your commit bit? You've done enough high-impact work to qualify, IMO. |
1ff0c68
to
72c1fe1
Compare
Oh, I forgot to skip the test when needed... it should be complete now
It'd be a pleasure! :) |
#32014 :) |
This comment has been minimized.
This comment has been minimized.
72c1fe1
to
81f31fc
Compare
In the most recent CI, the Windows failures are flakes, but the failure on ubuntu1804_sharedlibs_openssl110_x64 seems real - https://ci.nodejs.org/job/node-test-commit-linux-containered/18601/nodes=ubuntu1804_sharedlibs_openssl110_x64/testReport/junit/(root)/test/parallel_test_tls_keylog_tlsv13/ |
Keylog support isn't available on that openssl version, but the regex that was added to check had a small typo. Passed for me locally after (by skipping the test). |
Yes, please. |
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>
250618c
to
f82b4ae
Compare
Totally right, that was a typo. I've renamed the file and squashed, hopefully everything is in order now (sorry for taking so many attempts to get it right 😅) |
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 Backport-PR-URL: #31582 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>
Landed in a2b0e9e 🎉 |
Backport of #27654.
I had to modify
test-tls-keylog-tlsv13.js
to use TLS 1.2 connections as (AFAIK) v10 is not going to support TLS 1.3. I also modified the count from 5 to 1, because TLS 1.2 handshakes only emit 1 keylog event. Should I rename the file totest-tls-keylog-tlsv12.js
?