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: add debugging to native TLS code #26843

Closed
wants to merge 3 commits into from

Conversation

@addaleax
Copy link
Member

commented Mar 21, 2019

This is an attempt to make investigating the TLS state machine a bit more feasible.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
addaleax added 2 commits Mar 21, 2019

@addaleax addaleax requested a review from sam-github Mar 21, 2019

@sam-github
Copy link
Member

left a comment

LGTM

I suggest you add to every Debug() message whether its client or server. If you are running standalone node code, you know, but almost all the code I debugged had both a client and server in the same process (all our unit tests do), so having intermixed debug output and not knowing whether it was from client or server state made the debug output not very useful. I added client/server indications to both js debug statements, and the C++ fprintfs I was using.

Would be useful to add some info on how to enable the debug messages. I could't figure it out in 2 minutes of grepping and tag searching. I'm sure its only a couple more minutes till I find out how its enabled, probably I can git blame and find the original PRs, but a few comments in debug_utils.[h,cc] about how its supposed to use would be very much appreciated.

// Ignore cycling data if ClientHello wasn't yet parsed
if (!hello_parser_.IsEnded())
if (!hello_parser_.IsEnded()) {
Debug(this, "Bailing out of ClearIn(), hello_parser_ active");

This comment has been minimized.

Copy link
@sam-github

sam-github Mar 21, 2019

Member

I think "Not yet ready for " would be more clear than "Bailing out". The debug is mostly useful while reading the source, but Bailing sounds to me like a permanent state, but its just temporary.

This comment has been minimized.

Copy link
@addaleax

addaleax Mar 21, 2019

Author Member

I replaced all occurrences of Bailing out of with Returning from, that seems like the most accurate statement

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

I suggest you add to every Debug() message whether its client or server. If you are running standalone node code, you know, but almost all the code I debugged had both a client and server in the same process (all our unit tests do), so having intermixed debug output and not knowing whether it was from client or server state made the debug output not very useful. I added client/server indications to both js debug statements, and the C++ fprintfs I was using.

Done 👍

Would be useful to add some info on how to enable the debug messages. I could't figure it out in 2 minutes of grepping and tag searching. I'm sure its only a couple more minutes till I find out how its enabled, probably I can git blame and find the original PRs, but a few comments in debug_utils.[h,cc] about how its supposed to use would be very much appreciated.

Setting NODE_DEBUG_NATIVE=tls should do the trick – it’s mentioned in all of the CLI docs, fwiw. It should probably be mentioned in something like #26665 as well?

@danbev

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

Landed in f87b3a7, 2d5387e

@addaleax addaleax closed this Mar 26, 2019

@addaleax addaleax deleted the addaleax:tls-x branch Mar 26, 2019

addaleax added a commit that referenced this pull request Mar 26, 2019
tls: add CHECK for impossible condition
PR-URL: #26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit that referenced this pull request Mar 26, 2019
tls: add debugging to native TLS code
PR-URL: #26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Doesn't land cleanly on v11.x-staging. Does it depend on #26951 ?

@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

I'll try to cherry-pick it onto #26951 and see if it land clean there.

sam-github added a commit to sam-github/node that referenced this pull request Mar 29, 2019
tls: add CHECK for impossible condition
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Mar 29, 2019
tls: add debugging to native TLS code
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@sam-github sam-github referenced this pull request Mar 29, 2019
3 of 4 tasks complete
@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

yeah, cherry picks clean after TLS1.3

sam-github added a commit to sam-github/node that referenced this pull request Mar 29, 2019
tls: add CHECK for impossible condition
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Apr 9, 2019
tls: add CHECK for impossible condition
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Apr 9, 2019
tls: add debugging to native TLS code
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Apr 9, 2019
tls: add CHECK for impossible condition
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Apr 9, 2019
tls: add debugging to native TLS code
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Apr 11, 2019
tls: add CHECK for impossible condition
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
sam-github added a commit to sam-github/node that referenced this pull request Apr 11, 2019
tls: add debugging to native TLS code
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs added a commit that referenced this pull request Apr 15, 2019
tls: add CHECK for impossible condition
Backport-PR-URL: #26951
PR-URL: #26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs added a commit that referenced this pull request Apr 15, 2019
tls: add debugging to native TLS code
Backport-PR-URL: #26951
PR-URL: #26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere referenced this pull request Apr 19, 2019
@addaleax addaleax referenced this pull request May 29, 2019
2 of 4 tasks complete
addaleax added a commit to addaleax/node that referenced this pull request May 30, 2019
tls: add CHECK for impossible condition
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit to addaleax/node that referenced this pull request May 30, 2019
tls: add debugging to native TLS code
PR-URL: nodejs#26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs added a commit that referenced this pull request Jul 16, 2019
tls: add CHECK for impossible condition
Backport-PR-URL: #27967
PR-URL: #26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs added a commit that referenced this pull request Jul 16, 2019
tls: add debugging to native TLS code
Backport-PR-URL: #27967
PR-URL: #26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs added a commit that referenced this pull request Jul 17, 2019
tls: add CHECK for impossible condition
Backport-PR-URL: #27967
PR-URL: #26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs added a commit that referenced this pull request Jul 17, 2019
tls: add debugging to native TLS code
Backport-PR-URL: #27967
PR-URL: #26843
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BethGriggs BethGriggs referenced this pull request Jul 17, 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.