Skip to content

quic: refactor ocsp handling#34498

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.

Sign up for GitHub

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

Jump to bottom
Closed
jasnell wants to merge 3 commits into from
Closed

quic: refactor ocsp handling #34498

jasnell wants to merge 3 commits into from

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented Jul 23, 2020•
edited
Loading

First two commits are cleanup.

Third commit refactors the OCSP handling, converting it from a callback-oriented event to a configuration-based async function, which makes a lot more sense given that it's only ever called once at a very specific point in the QuicSession lifecycle.

Fourth commit removes the 'clientHello' event. Use cases supporting the event are tenuous at best and do not justify the additional machinery necessary for it to work.

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

@jasnelljasnell requested a review from a teamJuly 23, 2020 20:19
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 23, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jasnell

This comment has been minimized.

Sign in to view
@jasnelljasnell marked this pull request as draft July 24, 2020 00:49
@jasnelljasnell changed the title quic: refactor ocsp handling and remove clientHello eventquic: refactor ocsp handlingJul 27, 2020
@jasnelljasnell marked this pull request as ready for review July 27, 2020 16:47
@jasnell
Copy link
MemberAuthor

jasnell commented Jul 27, 2020

Ping @nodejs/quic ... this is ready for review. Limited the PR to just the OCSP handling for now. Still working through the issues around client hello, SNI, and SecureContext selection at the start of a QuicSession... will move those changes into a separate PR

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 27, 2020•
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32516/
QUIC CI: https://ci.nodejs.org/job/node-test-commit/39867/

addaleax
addaleax approved these changes Jul 27, 2020
View reviewed changes
lib/internal/quic/core.js
// TODO(@jasnell): Proper error
state.handshakeCompletePromiseReject(
newERR_OPERATION_FAILED('Handshake failed'));
}
Copy link
Member

@addaleaxaddaleaxJul 27, 2020

Choose a reason for hiding this comment

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

Can I disagree about these TODOs being irrelevant now? ERR_OPERATION_FAILED is too generic imo, because an operation failing is just about the definition of an error :)

Copy link
MemberAuthor

@jasnelljasnellJul 27, 2020

Choose a reason for hiding this comment

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

I know we've had a conversation about this before when you mentioned that our errors are too specific in cases. Struggling to find the right balance. Do you have a more concrete suggestion?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 27, 2020

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

@jasnelljasnell added quic Issues and PRs related to the QUIC implementation / HTTP/3. dont-land-on-v14.x labels Jul 27, 2020
@jasnell
Copy link
MemberAuthor

jasnell commented Jul 27, 2020

Landed in 62198d2...1f94b89

@jasnelljasnell closed this Jul 27, 2020
jasnell added a commit that referenced this pull request Jul 27, 2020
@jasnell
quic: remove extraneous unused debug property
2fb92f4
PR-URL: #34498 Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Jul 27, 2020
@jasnell
quic: remove no-longer relevant TODO statements
0666429
PR-URL: #34498 Reviewed-By: Anna Henningsen <[email protected]>
jasnell added a commit that referenced this pull request Jul 27, 2020
@jasnell
quic: refactor ocsp to use async function rather than event/callback
1f94b89
PR-URL: #34498 Reviewed-By: Anna Henningsen <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Jul 28, 2020
Open
3 tasks
This was referenced Jul 29, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.quicIssues and PRs related to the QUIC implementation / HTTP/3.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jasnell@nodejs-github-bot@addaleax