From 82ed02a31d2f101c233893fafdf30da661050fa7 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 7 Aug 2020 12:17:47 -0700 Subject: [PATCH 1/3] quic: fixup set_socket, fix skipped test --- lib/internal/quic/core.js | 8 ++-- src/quic/node_quic_session.cc | 44 +++++++++++-------- .../test-quic-simple-client-migrate.js | 14 ++++-- 3 files changed, 41 insertions(+), 25 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index d311852d950410..93849d0dcd8ecc 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -1803,8 +1803,10 @@ class QuicSession extends EventEmitter { }, depth, options); } - [kSetSocket](socket) { + [kSetSocket](socket, natRebinding = false) { this[kInternalState].socket = socket; + if (socket !== undefined) + this[kHandle].setSocket(socket[kHandle], natRebinding); } // Called at the completion of the TLS handshake for the local peer @@ -2432,7 +2434,7 @@ class QuicClientSession extends QuicSession { {}; } - async setSocket(socket) { + async setSocket(socket, natRebinding = false) { if (this.destroyed) throw new ERR_INVALID_STATE('QuicClientSession is already destroyed'); if (this.closing) @@ -2460,7 +2462,7 @@ class QuicClientSession extends QuicSession { this[kSetSocket](undefined); } socket[kAddSession](this); - this[kSetSocket](socket); + this[kSetSocket](socket, natRebinding); } } diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index 999dbd71fda689..e75d33e636e4e1 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -2479,7 +2479,6 @@ int QuicSession::set_session(SSL_SESSION* session) { } // A client QuicSession can be migrated to a different QuicSocket instance. -// TODO(@jasnell): This will be revisited. bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) { CHECK(!is_server()); CHECK(!is_destroyed()); @@ -2492,8 +2491,6 @@ bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) { Debug(this, "Migrating to %s", socket->diagnostic_name()); - SendSessionScope send(this); - // Ensure that we maintain a reference to keep this from being // destroyed while we are starting the migration. BaseObjectPtr ptr(this); @@ -2511,22 +2508,31 @@ bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) { // Step 4: Update ngtcp2 auto local_address = socket->local_address(); - if (nat_rebinding) { - ngtcp2_addr addr; - ngtcp2_addr_init( - &addr, - local_address.data(), - local_address.length(), - nullptr); - ngtcp2_conn_set_local_addr(connection(), &addr); - } else { + + // The nat_rebinding option here should rarely, if ever + // be used in a real application. It is intended to serve + // as a way of simulating a silent local address change, + // such as when the NAT binding changes. Currently, Node.js + // does not really have an effective way of detecting that. + // Manual user code intervention to handle the migration + // to the new QuicSocket is required, which should always + // trigger path validation using the ngtcp2_conn_initiate_migration. + if (LIKELY(!nat_rebinding)) { + SendSessionScope send(this); QuicPath path(local_address, remote_address_); - if (ngtcp2_conn_initiate_migration( - connection(), - &path, - uv_hrtime()) != 0) { - return false; - } + return ngtcp2_conn_initiate_migration( + connection(), + &path, + uv_hrtime()) == 0; + } else { + ngtcp2_addr addr; + ngtcp2_conn_set_local_addr( + connection(), + ngtcp2_addr_init( + &addr, + local_address.data(), + local_address.length(), + nullptr)); } return true; @@ -3671,7 +3677,7 @@ void QuicSessionSetSocket(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); CHECK(args[0]->IsObject()); ASSIGN_OR_RETURN_UNWRAP(&socket, args[0].As()); - args.GetReturnValue().Set(session->set_socket(socket)); + args.GetReturnValue().Set(session->set_socket(socket, args[1]->IsTrue())); } // GracefulClose flips a flag that prevents new local streams diff --git a/test/parallel/test-quic-simple-client-migrate.js b/test/parallel/test-quic-simple-client-migrate.js index d40a693ba3b93d..af15456c855fe6 100644 --- a/test/parallel/test-quic-simple-client-migrate.js +++ b/test/parallel/test-quic-simple-client-migrate.js @@ -5,10 +5,9 @@ const common = require('../common'); if (!common.hasQuic) common.skip('missing quic'); -common.skip('Not working correct yet... need to refactor'); - const assert = require('assert'); const { key, cert, ca } = require('../common/quic'); + const { once } = require('events'); const { createQuicSocket } = require('net'); const { pipeline } = require('stream'); @@ -58,14 +57,23 @@ const server = createQuicSocket({ server: options }); stream.on('end', common.mustCall(() => { assert.strictEqual(data, 'Hello from the client'); })); - stream.on('close', common.mustCall()); + stream.on('close', common.mustCall(() => { + req.close(); + })); // Send some data on one connection... stream.write('Hello '); // Wait just a bit, then migrate to a different // QuicSocket and continue sending. setTimeout(common.mustCall(async () => { + const s1 = req.socket; + const a1 = req.socket.endpoints[0].address; + await req.setSocket(client2); + + // Verify that it is using a different network endpoint + assert.notStrictEqual(s1, req.socket); + assert.notDeepStrictEqual(a1, req.socket.endpoints[0].address); client.close(); stream.end('from the client'); }), common.platformTimeout(100)); From ba73567a152708f9049dffd0f801c08b1bb00838 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 7 Aug 2020 12:25:41 -0700 Subject: [PATCH 2/3] quic: check setSocket natRebinding argument, extend test --- lib/internal/quic/core.js | 2 ++ test/parallel/test-quic-simple-client-migrate.js | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 93849d0dcd8ecc..dfbc876a84670c 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -2445,6 +2445,8 @@ class QuicClientSession extends QuicSession { throw new ERR_INVALID_STATE('QuicSocket is already destroyed'); if (socket.closing) throw new ERR_INVALID_STATE('QuicSocket is closing'); + if (typeof natRebinding !== 'boolean') + throw new ERR_INVALID_ARG_TYPE('natRebinding', 'boolean', true); await socket[kMaybeBind](); diff --git a/test/parallel/test-quic-simple-client-migrate.js b/test/parallel/test-quic-simple-client-migrate.js index af15456c855fe6..a658bfa4bbbd2f 100644 --- a/test/parallel/test-quic-simple-client-migrate.js +++ b/test/parallel/test-quic-simple-client-migrate.js @@ -69,6 +69,17 @@ const server = createQuicSocket({ server: options }); const s1 = req.socket; const a1 = req.socket.endpoints[0].address; + await Promise.all([1, {}, 'test', false, null, undefined].map((i) => { + return assert.rejects(req.setSocket(i), { + code: 'ERR_INVALID_ARG_TYPE' + }); + })); + await Promise.all([1, {}, 'test', null].map((i) => { + return assert.rejects(req.setSocket(req.socket, i), { + code: 'ERR_INVALID_ARG_TYPE' + }); + })); + await req.setSocket(client2); // Verify that it is using a different network endpoint From f2e6a0e778c28c50850992407d7fddf97c333843 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 7 Aug 2020 12:28:27 -0700 Subject: [PATCH 3/3] quic: add natRebinding argument to docs --- doc/api/quic.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/doc/api/quic.md b/doc/api/quic.md index 5595444906882d..84484357b72fb4 100644 --- a/doc/api/quic.md +++ b/doc/api/quic.md @@ -1246,12 +1246,16 @@ empty object when the key exchange is not ephemeral. The supported types are For example: `{ type: 'ECDH', name: 'prime256v1', size: 256 }`. -#### `quicclientsession.setSocket(socket])` +#### `quicclientsession.setSocket(socket[, natRebinding])` * `socket` {QuicSocket} A `QuicSocket` instance to move this session to. +* `natRebinding` {boolean} When `true`, indicates that the local address is to + be changed without triggering address validation. This will be rare and will + typically be used only to test resiliency in NAT rebind scenarios. + **Default**: `false`. * Returns: {Promise} Migrates the `QuicClientSession` to the given `QuicSocket` instance. If the new