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 — quic: part 1... quic-modified openssl by jasnell · Pull Request #32377 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Mar 19, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This is one of three separate PRs that would need to be landed together. They have been split to allow easier reviewing. This PR covers the Openssl modifications necessary to support the QUIC implementation.

NOTE: This PR merges into the master branch

The other two PRs in this batch are:

  • ngtcp2 and nghttp3: quic: part 2... ngtcp2 and nghttp3 #32378
  • quic implementation: quic: part 3... initial quic implementation #32379
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 openssl Issues and PRs related to the OpenSSL dependency. label Mar 19, 2020
This was referenced Mar 19, 2020
quic: part 3... initial quic implementation #32379
Closed
quic: part 2... ngtcp2 and nghttp3 #32378
Closed
quic: initial experimental quic implementation #30943
Closed
@jasnell
jasnell requested review from addaleax and sam-github March 19, 2020 22:35
mscdex
mscdex reviewed Mar 20, 2020
View reviewed changes
Comment thread deps/openssl/config/archs/BSD-x86/asm/crypto/bf/bf-586.s Resolved

Uh oh!

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

@richardlau

richardlau commented Mar 20, 2020

Copy link
Copy Markdown
Member

I don't feel qualified to review the technical aspects of this but that first commit looks like a rather large one to float on top of openssl and I'm concerned may impair our ability to take openssl updates in a timely fashion. The in-progress 1.1.1e update, for example, has changed some of the header file locations.

The current maintaining openssl guide removes and replaces the deps/openssl/openssl directory on updates.

cc @sam-github

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell

jasnell commented Mar 20, 2020

Copy link
Copy Markdown
Member Author

@richardlau

...that first commit looks like a rather large one to float on top of openssl and I'm concerned may impair our ability to take openssl updates in a timely fashion. The in-progress 1.1.1e update, for example, has changed some of the header file locations.

Yes, that's a valid concern. The patch itself is actually quite easy to maintain. The third PR in this set (#32379) includes the entirely of the patch file and instructions for updating. I still need to update it to 1.1.1e but that should not take long at all. But yes, since the OpenSSL OMC has taken a bit of an obstinate point of view on this it's not ideal. I don't think, however, that we should block on that.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sam-github

sam-github commented Mar 20, 2020

Copy link
Copy Markdown
Contributor

I'm concerned about the size of the patch as well, but as long as how to maintain it is clear, it should be OK. Also, the feature has to remain experimental until QUIC support lands officially in OpenSSL, and someone has to commit to maintaining the patches, because if the QUIC patches don't cherry-pick clean onto a new OpenSSL release, we might have to drop them.

To avoid that outcome, can you make these updates:

  1. The commit that adds the patches should include in the commit message details of how the commit was constructed (so that it could be reconstructed by future maintainers).
  2. A follow-on commit to above should update doc/guides/maintaining-openssl.md to add instructions to cherry-pick this floating patch, and should also contain instructions on how to rebuild the floating patch for new versions of openssl when they are released (if its necessary).

The process of a maintainer updating OpenSSL should be as mechanical as necessary.

I rebuilt the config, it didn't change the whitespace comment, so that's OK.

Unfortunately, I attempted to cherrypick ce0fdeb onto #32328, it conflicted with deps/openssl/openssl/test/sslapitest.c, which was discouraging, and I didn't find a https://github.com/akamai/openssl/commits/OpenSSL_1_1_1e-quic

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell

jasnell commented Mar 20, 2020

Copy link
Copy Markdown
Member Author

I've just about got the updated version of the patch for 1.1.1e ready to go.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell

jasnell commented Mar 20, 2020

Copy link
Copy Markdown
Member Author

@sam-github ... yes absolutely. The instructions for maintaining are actually added in the third PR in the batch but I'll be moving them in here. The patch file itself will be checked in separately and will be kept up to date. The quic support is currently disabled and not built by default and would remain so for at least some period of time and the folks at akamai have indicated that they will be keeping the patches up to date as much as possible -- they need to because they have infrastructure that depends on it.

I will get the commit that adds the maintenance details in to this PR soonish.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell

jasnell commented Mar 20, 2020

Copy link
Copy Markdown
Member Author

I will get the commit that adds the maintenance details in to this PR soonish.

Done. Once the 1.1.1e update lands and I'll update the nodejs/quic repo and then update the PR

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell
jasnell force-pushed the initial-quic-part-1 branch from 718a2b7 to 00bfcd9 Compare March 24, 2020 01:15
@jasnell

jasnell commented Mar 24, 2020

Copy link
Copy Markdown
Member Author

Updated for openssl 1.1.1e with maintenance instructions included.

All reactions

Sorry, something went wrong.

Uh oh!

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

@nodejs-github-bot

nodejs-github-bot commented Mar 24, 2020

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@nodejs-github-bot

nodejs-github-bot commented Mar 25, 2020

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell

jasnell commented Mar 25, 2020

Copy link
Copy Markdown
Member Author

CI is good on this.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell jasnell added review wanted PRs that need review. tsc-agenda Issues and PRs to discuss during Technical Steering Committee meetings. labels Mar 29, 2020
@jasnell

jasnell commented Mar 31, 2020

Copy link
Copy Markdown
Member Author

Putting all three of the PRs here on the tsc-agenda to queue up discussion on how we want to get these landed.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell jasnell added the quic Issues and PRs related to the QUIC transport implementation. label Mar 31, 2020
@jasnell

jasnell commented Mar 31, 2020

Copy link
Copy Markdown
Member Author

TODO: Will update this to 1.1.1f once #32583 lands

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Apr 6, 2020
Node.js Technical Steering Committee (TSC) Meeting 2020-04-08 nodejs/TSC#846
Closed
Node.js Technical Steering Committee (TSC) Meeting 2020-04-16 nodejs/TSC#848
Closed
@jasnell jasnell removed the tsc-agenda Issues and PRs to discuss during Technical Steering Committee meetings. label Apr 15, 2020
@jasnell
jasnell force-pushed the initial-quic-part-1 branch from 00bfcd9 to 7afa9a8 Compare May 7, 2020 23:44
@jasnell

jasnell commented May 7, 2020

Copy link
Copy Markdown
Member Author

nodejs/quic master was rebased and sync'd with nodejs/master earlier today. The commits in the PR have been updated accordingly.

All reactions

Sorry, something went wrong.

Uh oh!

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

@nodejs-github-bot

nodejs-github-bot commented May 14, 2020

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@BridgeAR
BridgeAR force-pushed the master branch from df4eda2 to 8ae28ff Compare May 31, 2020 12:09
@BridgeAR
BridgeAR force-pushed the master branch from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@jasnell
jasnell force-pushed the initial-quic-part-1 branch from 7afa9a8 to 846b216 Compare June 3, 2020 23:35
Qard
Qard approved these changes Jun 4, 2020
View reviewed changes

@Qard Qard left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be a ton of whitespace changes that aren't part of the patch file. Makes it very time-consuming to review this. 😬

I finally managed to get through it though and LGTM other than the nit about all that whitespace churn.

Sorry, something went wrong.

Uh oh!

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

All reactions
@jasnell

jasnell commented Jun 4, 2020

Copy link
Copy Markdown
Member Author

Seems to be a ton of whitespace changes that aren't part of the patch file. Makes it very time-consuming to review this.

Those whitespace changes are part of the openssl build process. We see exactly the same thing in the other openssl updates when configuration updates happen. Unfortunately there's not much we can do about it.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Qard

Qard commented Jun 4, 2020

Copy link
Copy Markdown
Member

Weird. Not a blocker for me, just an observation that it makes it kind of a pain to review. 😬

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell
jasnell force-pushed the initial-quic-part-1 branch from 846b216 to 0612442 Compare June 9, 2020 16:22
tmshort and others added 3 commits June 16, 2020 09:14
@tmshort @jasnell
deps: add support for BoringSSL QUIC APIs
c5a8839
Acquired from: https://github.com/akamai/openssl/tree/OpenSSL_1_1_1f-quic

Squashed:

*
akamai/openssl@2ef7c58
*
akamai/openssl@3f8eda3
*
akamai/openssl@b37f665
*
akamai/openssl@6b23589
*
akamai/openssl@3a793e0

---

This is a cherry-pick of 2a4b03a306439307e0b822b17eda3bdabddfbb68
on the master-quic-support2 branch (2019-10-07)
Which was a rebase/squash of master-quic-support:

* 5aa62ce Add support for more secrets - Todd Short/Todd Short (master-quic-support)
* 58e0643 Tweeks to quic_change_cipher_state() - Todd Short/Todd Short
* 8169702 Move QUIC code out of tls13_change_cipher_state() - Todd Short/Todd Short
* a08cfe6 Correctly disable middlebox compat - Todd Short/Todd Short
* 3a9eabf Add OPENSSL_NO_QUIC wrapper - Todd Short/Todd Short
* f550eca Add client early traffic secret storage - Todd Short/Todd Short
* 1b787ae Quick fix: s2c to c2s for early secret - Todd Short/Todd Short
* f97e6a9 Don't process an incomplete message - Todd Short/Todd Short
* 81f0ce2 Reset init state in SSL_process_quic_post_handshake() - Todd Short/Todd Short
* 5d59cf9 Fix quic_transport constructors/parsers - Todd Short/Todd Short
* 5e5f91c Fix INSTALL nit. - Todd Short/Todd Short
* bd290ab Fix duplicate word in docs - Todd Short/Todd Short
* 699590b fixup! Handle partial handshake messages - Todd Short/Todd Short
* a472a8d Handle partial handshake messages - Todd Short/Todd Short
* 363cf3d fixup! Use proper secrets for handshake - Todd Short/Todd Short
* b03fee6 Use proper secrets for handshake - Todd Short/Todd Short
* 2ab1aa0 Move QUIC transport params to encrypted extensions - Todd Short/Todd Short
* 0d16af9 Make temp secret names less confusing - Todd Short/Todd Short
* abb6f39 New method to get QUIC secret length - Todd Short/Todd Short
* 05fdae9 Add support for BoringSSL QUIC APIs - Todd Short/Todd Short

This adds a compatible API for BoringSSL's QUIC support, based
on the current |draft-ietf-quic-tls|.

Based on BoringSSL commit 3c034b2cf386b3131f75520705491871a2e0cafe
Based on BoringSSL commit c8e0f90f83b9ec38ea833deb86b5a41360b62b6a
Based on BoringSSL commit 3cbb0299a28a8bd0136257251a78b91a96c5eec8
Based on BoringSSL commit cc9d935256539af2d3b7f831abf57c0d685ffd81
Based on BoringSSL commit e6eef1ca16a022e476bbaedffef044597cfc8f4b
Based on BoringSSL commit 6f733791148cf8a076bf0e95498235aadbe5926d
Based on BoringSSL commit 384d0eaf1930af1ebc47eda751f0c78dfcba1c03
Based on BoringSSL commit a0373182eb5cc7b81d49f434596b473c7801c942
Based on BoringSSL commit b1b76aee3cb43ce11889403c5334283d951ebd37

New method to get QUIC secret length

Make temp secret names less confusing

Move QUIC transport params to encrypted extensions

Use proper secrets for handshake

fixup! Use proper secrets for handshake

Handle partial handshake messages

fixup! Handle partial handshake messages

Fix duplicate word in docs

Fix INSTALL nit.

Fix quic_transport constructors/parsers

Reset init state in SSL_process_quic_post_handshake()

Don't process an incomplete message

Quick fix: s2c to c2s for early secret

Add client early traffic secret storage

Add OPENSSL_NO_QUIC wrapper

Correctly disable middlebox compat

Move QUIC code out of tls13_change_cipher_state()

Create quic_change_cipher_state() that does the minimal required
to generate the QUIC secrets. (e.g. encryption contexts are not
initialized).

Tweeks to quic_change_cipher_state()

Add support for more secrets

Fix resumption secret

(cherry picked from commit 16fafdf4e0ec6cddd5705f407e5dca26cb30914d)

QUIC: Handle EndOfEarlyData and MaxEarlyData

QUIC: Increase HKDF_MAXBUF to 2048

Fall-through for 0RTT
@jasnell
deps: update archs files for OpenSSL-1.1.0
d232a80
After an OpenSSL source update, all the config files need to be
regenerated and committed by:
  $ make -C deps/openssl/config
  $ git add deps/openssl/config/archs
  $ git add deps/openssl/openssl/include/crypto/bn_conf.h
  $ git add deps/openssl/openssl/include/crypto/dso_conf.h
  $ git add deps/openssl/openssl/include/openssl/opensslconf.h
  $ git commit
@jasnell
deps: details for updating openssl quic support
e96bea6
Signed-off-by: James M Snell <[email protected]>
@jasnell
jasnell force-pushed the initial-quic-part-1 branch from 0612442 to e96bea6 Compare June 16, 2020 16:14
@jasnell jasnell closed this Jun 16, 2020
@richardlau richardlau mentioned this pull request Jun 18, 2020
CI jobs with --experimental-quic nodejs/build#2353
Closed
@codebytere codebytere added backport-requested-v14.x and removed backport-requested-v14.x labels Jun 18, 2020
@devsnek
devsnek deleted the initial-quic-part-1 branch August 31, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Qard Qard Qard approved these changes
@sam-github sam-github Awaiting requested review from sam-github
@addaleax addaleax Awaiting requested review from addaleax
+1 more reviewer
@mscdex mscdex mscdex left review comments
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

openssl Issues and PRs related to the OpenSSL dependency. quic Issues and PRs related to the QUIC transport implementation. review wanted PRs that need review.

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.

8 participants

@jasnell @richardlau @sam-github @nodejs-github-bot @Qard @mscdex @codebytere @tmshort
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.