Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

quic: update ngtcp2, openssl APIs, and use ngtcp2-crypto #138

Closed
wants to merge 7 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 27, 2019

Adopts the BoringSSL APIs based on openssl/openssl#8797. We may need to update the license accordingly. These will need to be tracked over time.

Update the implementation to use the new ngtcp2_crypto helper.

Note: because of the need to use the BoringSSL APIs, QUIC will not be available when using shared_openssl. Still need to make sure that's disabled in the build.

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

@jasnell jasnell force-pushed the ngtcp2-crypto2 branch 4 times, most recently from f8c519a to 7294c65 Compare September 27, 2019 19:47
@jasnell

This comment has been minimized.

@jasnell

This comment has been minimized.

@jasnell jasnell force-pushed the ngtcp2-crypto2 branch 3 times, most recently from cd20336 to 6204b14 Compare September 30, 2019 20:58
@jasnell

This comment has been minimized.

@jasnell

This comment has been minimized.

@jasnell
Copy link
Member Author

jasnell commented Sep 30, 2019

Ok, I've narrowed it down. ngtcp2 is buffering the handshake data and exiting because the handshake keys on the client side are not installed at the right time. ngtcp2 made a change that assumes that both the rx and tx handshake keys are available at the same time, which is based on the assumption that the boringssl quic APIs are being used. The timing of the KeyCB approach we currently have is different so we're running into an issue. Very happy that I finally narrowed that down but now need to find a fix for it...

@jasnell
Copy link
Member Author

jasnell commented Oct 1, 2019

Ah, well, this is going to be a fun one... Digging into it... tatsuhiro-san's initial modification to openssl to support quic included adding the key callback we use the get the keying material. That version of the quic modifications would invoke the key callback once per generated secret by patching into openssl's existing tls13_change_cipher_state function. The BoringSSL api approach is to actually branch to an entirely different quic_change_cipher_state function that generates the client and server secrets for each cipher level at the same time. This is a cleaner and more correct approach but to get it working for us since we don't have the boringssl APIs yet, we're going to have to patch on top of the original patch. 🎉 💯 The moral of the story is that as soon as we can move to the boringssl APIs, the better. I'll start working up the additional openssl patch tomorrow

@jasnell jasnell force-pushed the master branch 2 times, most recently from 89ce0dd to a529ed5 Compare October 3, 2019 00:06
@jasnell

This comment has been minimized.

@jasnell
Copy link
Member Author

jasnell commented Oct 3, 2019

Ok, progress! With the two additional commits here we get a lot further along in the TLS handshake but we're still not all the way yet. Still digging...

image

@jasnell
Copy link
Member Author

jasnell commented Oct 3, 2019

Testing is confirming that we are now completing the client side of the TLS handshake! The server side, however, is still not completing. I'll track that down tomorrow.

@jasnell
Copy link
Member Author

jasnell commented Oct 9, 2019

PROGRESS! Ok, with a couple of exceptions, things should be working. There's still work to do in this PR to get things cleaned up. The one thing that's not working is accessing the client certificate on the server side. That's what I'll be chasing down tomorrow. But, progress has been made.

@jasnell jasnell marked this pull request as ready for review October 9, 2019 16:32
@jasnell
Copy link
Member Author

jasnell commented Oct 9, 2019

WOO! Ok, this should be ready to go. There are still a few pieces that need to be revisited but we need some fixes in ngtcp2 to resolve those (see ngtcp2/ngtcp2#156)

@jasnell jasnell changed the title [WIP] quic: update ngtcp2 quic: update ngtcp2, openssl APIs, and use ngtcp2-crypto Oct 9, 2019
@jasnell
Copy link
Member Author

jasnell commented Oct 9, 2019

There's one commit in here that needs to be upstreamed... ngtcp2/ngtcp2#157

@jasnell jasnell closed this Oct 9, 2019
addaleax pushed a commit that referenced this pull request Dec 11, 2019
This reverts commit 170c5d0.

PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 11, 2019
This reverts commit ecda77c.

PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 11, 2019
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 11, 2019
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 11, 2019
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 11, 2019
This commit will need to be submitted upstream then backed out
once it lands and we can update

PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Dec 11, 2019
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Dec 13, 2019
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Dec 13, 2019
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
sam-github pushed a commit to sam-github/node that referenced this pull request Dec 13, 2019
sam-github pushed a commit to sam-github/node that referenced this pull request Dec 13, 2019
PR-URL: nodejs/quic#138
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
This commit will need to be submitted upstream then backed out
once it lands and we can update

PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
This commit will need to be submitted upstream then backed out
once it lands and we can update

PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit to juanarbol/quic that referenced this pull request Dec 17, 2019
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit to jasnell/quic that referenced this pull request Feb 3, 2020
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit to jasnell/quic that referenced this pull request Feb 3, 2020
PR-URL: nodejs#138
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Feb 13, 2020
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Feb 13, 2020
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Feb 13, 2020
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Feb 13, 2020
PR-URL: #138
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants