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.

Sign up for GitHub

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

Jump to bottom

tls: cli option to enable TLS key logging to file #30055

Closed
wants to merge 1 commit into from
Closed

tls: cli option to enable TLS key logging to file #30055

wants to merge 1 commit into from
+99 −0

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Oct 22, 2019 •
edited
Loading

Debugging HTTPS or TLS connections from a Node.js app with (for example)
Wireshark is unreasonably difficult without the ability to get the TLS
key log. In theory, the application can be modified to use the
'keylog' event directly, but for complex apps, or apps that define
there own HTTPS Agent (like npm), this is unreasonably difficult.

Use of the option triggers a warning to be emitted so the user is
clearly notified of what is happening and its effect.

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 nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Oct 22, 2019
@addaleax addaleax added cli Issues and PRs related to the Node.js command line interface. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 22, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@sam-github
Copy link
Contributor Author

@bnoordhuis now explicitly setting the file mode to 0o600

cjihrig
cjihrig approved these changes Oct 23, 2019
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@sam-github
tls: cli option to enable TLS key logging to file
88b645b 
Debugging HTTPS or TLS connections from a Node.js app with (for example)
Wireshark is unreasonably difficult without the ability to get the TLS
key log. In theory, the application can be modified to use the
`'keylog'` event directly, but for complex apps, or apps that define
there own HTTPS Agent (like npm), this is unreasonably difficult.

Use of the option triggers a warning to be emitted so the user is
clearly notified of what is happening and its effect.
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 20, 2019
sam-github added a commit to sam-github/node that referenced this pull request Nov 20, 2019
@sam-github
tls: cli option to enable TLS key logging to file
80efb80 
Debugging HTTPS or TLS connections from a Node.js app with (for example)
Wireshark is unreasonably difficult without the ability to get the TLS
key log. In theory, the application can be modified to use the
`'keylog'` event directly, but for complex apps, or apps that define
there own HTTPS Agent (like npm), this is unreasonably difficult.

Use of the option triggers a warning to be emitted so the user is
clearly notified of what is happening and its effect.

PR-URL: nodejs#30055
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github
Copy link
Contributor Author

Landed in 80efb80

@sam-github sam-github closed this Nov 20, 2019
@sam-github sam-github deleted the tls-keylog-option branch November 20, 2019 16:01
MylesBorins pushed a commit that referenced this pull request Nov 21, 2019
@sam-github @MylesBorins
tls: cli option to enable TLS key logging to file
01fa18c 
Debugging HTTPS or TLS connections from a Node.js app with (for example)
Wireshark is unreasonably difficult without the ability to get the TLS
key log. In theory, the application can be modified to use the
`'keylog'` event directly, but for complex apps, or apps that define
there own HTTPS Agent (like npm), this is unreasonably difficult.

Use of the option triggers a warning to be emitted so the user is
clearly notified of what is happening and its effect.

PR-URL: #30055
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 21, 2019
Merged
targos pushed a commit that referenced this pull request Jan 13, 2020
@sam-github @targos
tls: cli option to enable TLS key logging to file
600c37c 
Debugging HTTPS or TLS connections from a Node.js app with (for example)
Wireshark is unreasonably difficult without the ability to get the TLS
key log. In theory, the application can be modified to use the
`'keylog'` event directly, but for complex apps, or apps that define
there own HTTPS Agent (like npm), this is unreasonably difficult.

Use of the option triggers a warning to be emitted so the user is
clearly notified of what is happening and its effect.

PR-URL: #30055
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jan 15, 2020
Closed
@mildsunrise mildsunrise mentioned this pull request Jan 30, 2020
Closed
4 tasks
@mildsunrise mildsunrise mentioned this pull request Jan 30, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@sam-github @BethGriggs
tls: cli option to enable TLS key logging to file
e80103a 
Debugging HTTPS or TLS connections from a Node.js app with (for example)
Wireshark is unreasonably difficult without the ability to get the TLS
key log. In theory, the application can be modified to use the
`'keylog'` event directly, but for complex apps, or apps that define
there own HTTPS Agent (like npm), this is unreasonably difficult.

Use of the option triggers a warning to be emitted so the user is
clearly notified of what is happening and its effect.

PR-URL: #30055
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
@mildsunrise mildsunrise mentioned this pull request May 12, 2020
Closed
4 tasks
mildsunrise added a commit to mildsunrise/node that referenced this pull request May 12, 2020
@mildsunrise
tls: fix --tls-keylog option
4f271ad 
There's a typo that causes only the first socket to be logged
(i.e. when the warning is emitted).

In addition, server sockets aren't logged because `keylog` events
are not emitted on tls.Server, not the socket. This behaviour is
counterintuitive and has caused more bugs in the past, so make all
sockets (server or client) emit 'keylog'. tls.Server will just
re-emit these events.

Refs: nodejs#30055
mildsunrise added a commit that referenced this pull request May 14, 2020
@mildsunrise
tls: fix --tls-keylog option
1dafaf0 
There's a typo that causes only the first socket to be logged
(i.e. when the warning is emitted).

In addition, server sockets aren't logged because `keylog` events
are not emitted on tls.Server, not the socket. This behaviour is
counterintuitive and has caused more bugs in the past, so make all
sockets (server or client) emit 'keylog'. tls.Server will just
re-emit these events.

Refs: #30055
PR-URL: #33366
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
codebytere pushed a commit that referenced this pull request May 16, 2020
@mildsunrise @codebytere
tls: fix --tls-keylog option
6d2aaaf 
There's a typo that causes only the first socket to be logged
(i.e. when the warning is emitted).

In addition, server sockets aren't logged because `keylog` events
are not emitted on tls.Server, not the socket. This behaviour is
counterintuitive and has caused more bugs in the past, so make all
sockets (server or client) emit 'keylog'. tls.Server will just
re-emit these events.

Refs: #30055
PR-URL: #33366
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
@mildsunrise @codebytere
tls: fix --tls-keylog option
81749bb 
There's a typo that causes only the first socket to be logged
(i.e. when the warning is emitted).

In addition, server sockets aren't logged because `keylog` events
are not emitted on tls.Server, not the socket. This behaviour is
counterintuitive and has caused more bugs in the past, so make all
sockets (server or client) emit 'keylog'. tls.Server will just
re-emit these events.

Refs: #30055
PR-URL: #33366
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
codebytere pushed a commit to codebytere/node that referenced this pull request Jun 9, 2020
@mildsunrise @codebytere
tls: fix --tls-keylog option
7315c22 
There's a typo that causes only the first socket to be logged
(i.e. when the warning is emitted).

In addition, server sockets aren't logged because `keylog` events
are not emitted on tls.Server, not the socket. This behaviour is
counterintuitive and has caused more bugs in the past, so make all
sockets (server or client) emit 'keylog'. tls.Server will just
re-emit these events.

Refs: nodejs#30055
PR-URL: nodejs#33366
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sam-github @nodejs-github-bot @bnoordhuis @danbev @jasnell @addaleax @cjihrig @Trott