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

quic: update deps for quic draft 29 #34033

Closed
wants to merge 13 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 23, 2020

Update dependencies (ngtcp2, nghttp3, and akamai openssl+quic) for latest 29... update impl accordingly. There are likely additional bits that needs to be changed.

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 and others added 6 commits June 22, 2020 18:49
…a4e2e9ea568

Original Commit Message:

  Some cleanup for the main QUIC changes

  Try to reduce unneeded whitespace changes and wrap new code to 80 columns.
  Reword documentation to attempt to improve clarity.
  Add some more sanity checks and clarifying comments to the code.
  Update referenced I-D versions.
…2f2487bf8b3

Original Commit Message:

  Prevent KeyUpdate for QUIC

  QUIC does not use the TLS KeyUpdate message/mechanism, and indeed
  it is an error to generate or receive such a message.  Add the
  necessary checks (noting that the check for receipt should be
  redundant since SSL_provide_quic_data() is the only way to provide
  input to the TLS layer for a QUIC connection).
…f482e392456

Original Commit Message:

  Test KeyUpdate rejection

  For now, just test that we don't generate any, since we don't really
  expose the mechanics for encrypting one and the QUIC API is not
  integrated into the TLSProxy setup.
…31d18d7bda9

Original Commit Message:

  Fix out-of-bounds read when TLS msg is split up into multiple chunks

  Previously, SSL_provide_quic_data tried to handle this kind of
  situation, but it failed when the length of input data is less than
  SSL3_HM_HEADER_LENGTH.  If that happens, the code might get wrong
  message length by reading value from out-of-bounds region.
@nodejs-github-bot nodejs-github-bot added the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Jun 23, 2020
@jasnell jasnell marked this pull request as draft June 23, 2020 18:56
The ngtcp2 update uses a gcc builtin that is not available under
_MSC_VER. This floats a patch to fix it.

Upstream PR: ngtcp2/ngtcp2#247
@jasnell jasnell marked this pull request as ready for review June 24, 2020 00:02
@jasnell
Copy link
Member Author

jasnell commented Jun 24, 2020

Still a couple of failures in test-quic-ipv6only to investigate but overall moving in the right direction... https://ci.nodejs.org/job/node-test-commit/39233/

@nodejs-github-bot

This comment has been minimized.

@gengjiawen
Copy link
Member

Still a couple of failures in test-quic-ipv6only to investigate but overall moving in the right direction... ci.nodejs.org/job/node-test-commit/39233

Build looks failed.

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member Author

jasnell commented Jun 24, 2020

Build looks failed

Yep, fixing the regular without --experimental-quic build now. For the CI with --experimental-quic, there are definitely going to be a handful of failures in CI that we're still working through.

@nodejs-github-bot
Copy link
Collaborator

deps/ngtcp2/ngtcp2.gyp Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the update-quic-deps-20200622 branch 2 times, most recently from f49d13b to 8448815 Compare June 24, 2020 04:10
jasnell added a commit that referenced this pull request Jun 24, 2020
PR-URL: #34033
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
jasnell added a commit that referenced this pull request Jun 24, 2020
The dual stack support is currently broken as the QuicSocket endpoints
are not properly accounting for all cases. Needs to be investigated
further.

PR-URL: #34033
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
jasnell added a commit that referenced this pull request Jun 24, 2020
PR-URL: #34033
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jun 24, 2020

Landed in a2b1b92...7816e5f

@jasnell jasnell closed this Jun 24, 2020
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Dec 15, 2020
PR-URL: nodejs#34033
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@github-actions github-actions bot mentioned this pull request Dec 17, 2020
MylesBorins pushed a commit that referenced this pull request Dec 17, 2020
PR-URL: #34033
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>

PR-URL: #36520
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
targos pushed a commit that referenced this pull request Dec 21, 2020
PR-URL: #34033
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>

PR-URL: #36520
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants