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: refactor clientHello #34541

Closed
wants to merge 3 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 28, 2020

First commit here is from #34533 which needs to land first.

Second commit here is the important one in this PR. This refactors the 'clientHello' event into an async function passed as an quicsocket.listen() option. This function is only ever called once at a very specific point in the QuicServerSession lifecycle, using it as an event is unnecessary. The commit changes the way it works. If appropriate to do so, user code may return a new SecureContext from the function (previously that was done in the OCSP function, which really didn't make sense).

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 requested a review from a team July 28, 2020 17:49
@nodejs-github-bot nodejs-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 28, 2020
@jasnell jasnell added dont-land-on-v14.x quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Jul 28, 2020
@jasnell jasnell changed the title quic: use OpenSSL built-in cert and hostname validation quic: refactor clientHello Jul 28, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 28, 2020

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

The actions failures are new warnings (treated as errors):

2020-07-28T17:59:21.1359496Z ../src/quic/node_quic_session.cc: In function ‘void node::quic::{anonymous}::QuicSessionOnCertDone(const v8::FunctionCallbackInfo<v8::Value>&)’:
2020-07-28T17:59:21.1455906Z ../src/quic/node_quic_session.cc:2732:16: error: unused variable ‘env’ [-Werror=unused-variable]
2020-07-28T17:59:21.1456744Z    Environment* env = Environment::GetCurrent(args);
2020-07-28T17:59:21.1457123Z                 ^~~
2020-07-28T17:59:21.9743160Z ../src/quic/node_quic_session.cc: In member function ‘virtual void node::quic::JSQuicSessionListener::OnClientHello(const char*, const char*)’:
2020-07-28T17:59:21.9744621Z ../src/quic/node_quic_session.cc:399:55: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::Array; T = v8::Array]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9745989Z    session()->crypto_context()->hello_ciphers().ToLocal(&ciphers);
2020-07-28T17:59:21.9746893Z    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
2020-07-28T17:59:21.9748732Z ../src/quic/node_quic_session.cc:402:54: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::String; T = v8::String]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9749778Z      String::NewFromUtf8(env->isolate(), alpn).ToLocal(&alpn_string);
2020-07-28T17:59:21.9749960Z      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
2020-07-28T17:59:21.9750767Z ../src/quic/node_quic_session.cc:405:61: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::String; T = v8::String]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9751274Z      String::NewFromUtf8(env->isolate(), server_name).ToLocal(&servername);
2020-07-28T17:59:21.9751434Z      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
2020-07-28T17:59:21.9807567Z ../src/quic/node_quic_session.cc: In member function ‘virtual void node::quic::JSQuicSessionListener::OnHandshakeCompleted()’:
2020-07-28T17:59:21.9808402Z ../src/quic/node_quic_session.cc:581:55: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::Value; T = v8::Value]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9808617Z      crypto::GetValidationErrorReason(env, err).ToLocal(&validationErrorReason);
2020-07-28T17:59:21.9808753Z      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
2020-07-28T17:59:21.9809720Z ../src/quic/node_quic_session.cc:582:53: error: ignoring return value of ‘bool v8::MaybeLocal<T>::ToLocal(v8::Local<S>*) const [with S = v8::Value; T = v8::Value]’, declared with attribute warn_unused_result [-Werror=unused-result]
2020-07-28T17:59:21.9810060Z      crypto::GetValidationErrorCode(env, err).ToLocal(&validationErrorCode);
2020-07-28T17:59:21.9810235Z      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
2020-07-28T17:59:26.2727155Z cc1plus: all warnings being treated as errors

@jasnell
Copy link
Member Author

jasnell commented Jul 28, 2020

CI is good on this!

src/quic/node_quic_session.cc Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Jul 29, 2020

@addaleax, please take another look when you have the opportunity to do so. I've added a new QuicCallbackScope modeled after CallbackScope but it ensures that the QuicSession is destroyed if an error is thrown, allowing an attempt to be made to send a final CONNECTION_CLOSE to the peer.

lib/internal/quic/core.js Outdated Show resolved Hide resolved
test/parallel/test-quic-client-server.js Show resolved Hide resolved
Refactor the `'clientHello'` event into a `clientHelloHandler`
configuration option and async function. Remove the addContext
API as it's not needed.
Alternative to `CallbackScope` that handles destroying the
`QuicSession` in the try_catch cleanup.
@jasnell
Copy link
Member Author

jasnell commented Jul 31, 2020

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 31, 2020
@jasnell
Copy link
Member Author

jasnell commented Jul 31, 2020

Regular CI and QUIC CI are good to go on this

gengjiawen pushed a commit that referenced this pull request Aug 3, 2020
Refactor the `'clientHello'` event into a `clientHelloHandler`
configuration option and async function. Remove the addContext
API as it's not needed.

PR-URL: #34541
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
gengjiawen pushed a commit that referenced this pull request Aug 3, 2020
Alternative to `CallbackScope` that handles destroying the
`QuicSession` in the try_catch cleanup.

PR-URL: #34541
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
gengjiawen pushed a commit that referenced this pull request Aug 3, 2020
PR-URL: #34541
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@gengjiawen
Copy link
Member

Landed in e5dacc2...6e65f26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants