GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tls: fix --tls-keylog option by mildsunrise · Pull Request #33366 · nodejs/node

Conversation

@mildsunrise

@mildsunrise mildsunrise commented May 12, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label May 12, 2020
@mildsunrise mildsunrise added the cli Issues and PRs related to the Node.js command-line interface. label May 12, 2020
@mildsunrise mildsunrise mentioned this pull request May 12, 2020
Optionally log master secrets for TLS connections #2363
Closed
@nodejs-github-bot

nodejs-github-bot commented May 12, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31289/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
mildsunrise force-pushed the fix-keylog-cli branch from 43d95dc to 4f271ad Compare May 12, 2020 13:12
@nodejs-github-bot

nodejs-github-bot commented May 12, 2020 •
edited by addaleax
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31290/ (:yellow_heart:)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sam-github
sam-github approved these changes May 12, 2020
View reviewed changes
@mildsunrise mildsunrise added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label May 12, 2020
jasnell
jasnell approved these changes May 12, 2020
View reviewed changes
juanarbol
juanarbol approved these changes May 13, 2020
View reviewed changes
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]>
@mildsunrise

mildsunrise commented May 14, 2020

Copy link
Copy Markdown
Contributor Author

Landed in 1dafaf0
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mildsunrise mildsunrise closed this May 14, 2020
@mildsunrise
mildsunrise deleted the fix-keylog-cli branch May 14, 2020 14:54
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 codebytere mentioned this pull request May 18, 2020
v14.3.0 proposal #33452
Merged
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]>
@codebytere codebytere mentioned this pull request Jun 9, 2020
v12.18.1 proposal #33811
Merged
@mildsunrise mildsunrise mentioned this pull request May 30, 2021
FYI: There's now a simpler way to get the keylog kolontsov/node-sslkeylog#12
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@juanarbol juanarbol juanarbol approved these changes
+1 more reviewer
@sam-github sam-github sam-github approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. cli Issues and PRs related to the Node.js command-line interface. 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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@mildsunrise @nodejs-github-bot @sam-github @jasnell @juanarbol
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.