From b92fa36673d08dff0ea43dfbfc1ec183b4520421 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 7 Aug 2020 13:05:46 -0700 Subject: [PATCH 1/6] quic: fixup session ticket app data todo comments --- src/quic/node_quic_session.h | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index c41d58125b9f70..7fd9d853bbcfbc 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -616,20 +616,17 @@ class QuicApplication : public MemoryRetainer, virtual void ResumeStream(int64_t stream_id) {} - virtual void SetSessionTicketAppData(const SessionTicketAppData& app_data) { - // TODO(@jasnell): Different QUIC applications may wish to set some - // application data in the session ticket (e.g. http/3 would set - // server settings in the application data). For now, doing nothing - // as I'm just adding the basic mechanism. - } - + // Different QUIC applications may set some application data in + // the session ticket (e.g. http/3 would set server settings in the + // application data). By default, there's nothing to set. + virtual void SetSessionTicketAppData(const SessionTicketAppData& app_data) {} + + // Different QUIC applications may set some application data in + // the session ticket (e.g. http/3 would set server settings in the + // application data). By default, there's nothing to get. virtual SessionTicketAppData::Status GetSessionTicketAppData( const SessionTicketAppData& app_data, SessionTicketAppData::Flag flag) { - // TODO(@jasnell): Different QUIC application may wish to set some - // application data in the session ticket (e.g. http/3 would set - // server settings in the application data). For now, doing nothing - // as I'm just adding the basic mechanism. return flag == SessionTicketAppData::Flag::STATUS_RENEW ? SessionTicketAppData::Status::TICKET_USE_RENEW : SessionTicketAppData::Status::TICKET_USE; From 717c6564e89810ee4259b16b06a9a73fed77b33a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 7 Aug 2020 13:07:25 -0700 Subject: [PATCH 2/6] quic: resolve InitializeSecureContext TODO comment Using a static label is sufficient. --- src/quic/node_quic_crypto.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/quic/node_quic_crypto.cc b/src/quic/node_quic_crypto.cc index 8f1f433525ecc3..b073ca9c381487 100644 --- a/src/quic/node_quic_crypto.cc +++ b/src/quic/node_quic_crypto.cc @@ -602,8 +602,6 @@ void InitializeSecureContext( BaseObjectPtr sc, bool early_data, ngtcp2_crypto_side side) { - // TODO(@jasnell): Using a static value for this at the moment but - // we need to determine if a non-static or per-session value is better. constexpr static unsigned char session_id_ctx[] = "node.js quic server"; switch (side) { case NGTCP2_CRYPTO_SIDE_SERVER: From ff2159acd3a1afe30be3a79c0ef97059c90f18ab Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 10 Aug 2020 13:21:31 -0700 Subject: [PATCH 3/6] quic: clarify TODO statements --- src/quic/node_quic_session.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index e75d33e636e4e1..b1748c8f728001 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -263,6 +263,16 @@ void QuicSessionConfig::Set( // TODO(@jasnell): QUIC allows both IPv4 and IPv6 addresses to be // specified. Here we're specifying one or the other. Need to // determine if that's what we want or should we support both. + // + // TODO(@jasnell): Currently, this is specified as a single value + // that is used for all connections. In the future, it may be + // necessary to determine the preferred address based on the + // remote address. The trick, however, is that the preferred + // address must be selected before the QuicSession is created, + // before the handshake can be started. That is, it may need + // to be an optional callback on QuicSocket. That would incur + // a performance penalty so we'd really have to be sure of the + // utility. if (preferred_addr != nullptr) { transport_params.preferred_address_present = 1; switch (preferred_addr->sa_family) { From d3feff4d4755f0b9aba8dea245fd69366fc30037 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 10 Aug 2020 14:28:34 -0700 Subject: [PATCH 4/6] quic: consolidate stats collecting in QuicSession --- src/quic/node_quic_http3_application.cc | 9 ++++----- src/quic/node_quic_session.cc | 21 +++++++-------------- src/quic/node_quic_session.h | 7 ++++--- 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/src/quic/node_quic_http3_application.cc b/src/quic/node_quic_http3_application.cc index e05ad316341c66..82cd677d1ce558 100644 --- a/src/quic/node_quic_http3_application.cc +++ b/src/quic/node_quic_http3_application.cc @@ -79,11 +79,10 @@ Http3Application::Http3Application( // Push streams in HTTP/3 are a bit complicated. // First, it's important to know that only an HTTP/3 server can // create a push stream. -// Second, it's important to recognize that a push stream is -// essentially an *assumed* request. For instance, if a client -// requests a webpage that has links to css and js files, and -// the server expects the client to send subsequent requests -// for those css and js files, the server can shortcut the +// Second, a push stream is essentially an *assumed* request. For +// instance, if a client requests a webpage that has links to css +// and js files, and the server expects the client to send subsequent +// requests for those css and js files, the server can shortcut the // process by opening a push stream for each additional resource // it assumes the client to make. // Third, a push stream can only be opened within the context diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index b1748c8f728001..59f80c77057598 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -2098,7 +2098,6 @@ bool QuicSession::Receive( UpdateIdleTimer(); SendPendingData(); - UpdateRecoveryStats(); Debug(this, "Successfully processed received packet"); return true; } @@ -2893,19 +2892,6 @@ void QuicSessionOnCertDone(const FunctionCallbackInfo& args) { } } // namespace -// Recovery stats are used to allow user code to keep track of -// important round-trip timing statistics that are updated through -// the lifetime of a connection. Effectively, these communicate how -// much time (from the perspective of the local peer) is being taken -// to exchange data reliably with the remote peer. -// TODO(@jasnell): Revisit -void QuicSession::UpdateRecoveryStats() { - ngtcp2_conn_stat stat; - ngtcp2_conn_get_conn_stat(connection(), &stat); - SetStat(&QuicSessionStats::min_rtt, stat.min_rtt); - SetStat(&QuicSessionStats::latest_rtt, stat.latest_rtt); - SetStat(&QuicSessionStats::smoothed_rtt, stat.smoothed_rtt); -} // Data stats are used to allow user code to keep track of important // statistics such as amount of data in flight through the lifetime @@ -2918,6 +2904,13 @@ void QuicSession::UpdateDataStats() { ngtcp2_conn_stat stat; ngtcp2_conn_get_conn_stat(connection(), &stat); + SetStat(&QuicSessionStats::latest_rtt, stat.latest_rtt); + SetStat(&QuicSessionStats::min_rtt, stat.min_rtt); + SetStat(&QuicSessionStats::smoothed_rtt, stat.smoothed_rtt); + SetStat(&QuicSessionStats::receive_rate, stat.recv_rate_sec); + SetStat(&QuicSessionStats::send_rate, stat.delivery_rate_sec); + SetStat(&QuicSessionStats::cwnd, stat.cwnd); + state_->bytes_in_flight = stat.bytes_in_flight; // The max_bytes_in_flight is a highwater mark that can be used // in performance analysis operations. diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index 7fd9d853bbcfbc..05c7606e8b54f7 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -204,7 +204,10 @@ enum QuicSessionStateFields { V(BLOCK_COUNT, block_count, "Block Count") \ V(MIN_RTT, min_rtt, "Minimum RTT") \ V(LATEST_RTT, latest_rtt, "Latest RTT") \ - V(SMOOTHED_RTT, smoothed_rtt, "Smoothed RTT") + V(SMOOTHED_RTT, smoothed_rtt, "Smoothed RTT") \ + V(CWND, cwnd, "Cwnd") \ + V(RECEIVE_RATE, receive_rate, "Receive Rate / Sec") \ + V(SEND_RATE, send_rate, "Send Rate Sec") #define V(name, _, __) IDX_QUIC_SESSION_STATS_##name, enum QuicSessionStatsIdx : int { @@ -1272,8 +1275,6 @@ class QuicSession final : public AsyncWrap, bool WritePackets(const char* diagnostic_label = nullptr); - void UpdateRecoveryStats(); - void UpdateConnectionID( int type, const QuicCID& cid, From cc5c4560c4d19b51d6b4bd9f450bf3c6caa205da Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 11 Aug 2020 15:45:26 -0700 Subject: [PATCH 5/6] src: allow instances of net.BlockList to be created internally Initial PR had it so that user code would create BlockList instances. This sets it up so that instances can be created internally by Node.js --- lib/internal/blocklist.js | 9 +++++++-- src/env.h | 2 ++ src/node_sockaddr.cc | 14 ++++++++++++++ src/node_sockaddr.h | 1 + tools/doc/type-parser.js | 1 + 5 files changed, 25 insertions(+), 2 deletions(-) diff --git a/lib/internal/blocklist.js b/lib/internal/blocklist.js index d4074ab41c2150..0ad58373b746b2 100644 --- a/lib/internal/blocklist.js +++ b/lib/internal/blocklist.js @@ -26,8 +26,13 @@ const { } = require('internal/errors').codes; class BlockList { - constructor() { - this[kHandle] = new BlockListHandle(); + constructor(handle = new BlockListHandle()) { + // The handle argument is an intentionally undocumented + // internal API. User code will not be able to create + // a BlockListHandle object directly. + if (!(handle instanceof BlockListHandle)) + throw new ERR_INVALID_ARG_TYPE('handle', 'BlockListHandle', handle); + this[kHandle] = handle; this[kHandle][owner_symbol] = this; } diff --git a/src/env.h b/src/env.h index d99bb63db6fb29..b1367c2660f4fa 100644 --- a/src/env.h +++ b/src/env.h @@ -182,6 +182,7 @@ constexpr size_t kFsStatsBufferLength = V(asn1curve_string, "asn1Curve") \ V(async_ids_stack_string, "async_ids_stack") \ V(bits_string, "bits") \ + V(block_list_string, "blockList") \ V(buffer_string, "buffer") \ V(bytes_parsed_string, "bytesParsed") \ V(bytes_read_string, "bytesRead") \ @@ -423,6 +424,7 @@ constexpr size_t kFsStatsBufferLength = V(async_wrap_object_ctor_template, v8::FunctionTemplate) \ V(base_object_ctor_template, v8::FunctionTemplate) \ V(binding_data_ctor_template, v8::FunctionTemplate) \ + V(blocklist_instance_template, v8::ObjectTemplate) \ V(compiled_fn_entry_template, v8::ObjectTemplate) \ V(dir_instance_template, v8::ObjectTemplate) \ V(fd_constructor_template, v8::ObjectTemplate) \ diff --git a/src/node_sockaddr.cc b/src/node_sockaddr.cc index 8ba82ff68535a6..58d03eefa6e6e6 100644 --- a/src/node_sockaddr.cc +++ b/src/node_sockaddr.cc @@ -524,6 +524,19 @@ SocketAddressBlockListWrap::SocketAddressBlockListWrap( MakeWeak(); } +BaseObjectPtr SocketAddressBlockListWrap::New( + Environment* env) { + Local obj; + if (!env->blocklist_instance_template() + ->NewInstance(env->context()).ToLocal(&obj)) { + return {}; + } + BaseObjectPtr wrap = + MakeDetachedBaseObject(env, obj); + CHECK(wrap); + return wrap; +} + void SocketAddressBlockListWrap::New( const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); @@ -673,6 +686,7 @@ void SocketAddressBlockListWrap::Initialize( env->SetProtoMethod(t, "check", SocketAddressBlockListWrap::Check); env->SetProtoMethod(t, "getRules", SocketAddressBlockListWrap::GetRules); + env->set_blocklist_instance_template(t->InstanceTemplate()); target->Set(env->context(), name, t->GetFunction(env->context()).ToLocalChecked()).FromJust(); diff --git a/src/node_sockaddr.h b/src/node_sockaddr.h index f539cf6555f798..69a370afa32e81 100644 --- a/src/node_sockaddr.h +++ b/src/node_sockaddr.h @@ -280,6 +280,7 @@ class SocketAddressBlockListWrap : v8::Local context, void* priv); + static BaseObjectPtr New(Environment* env); static void New(const v8::FunctionCallbackInfo& args); static void AddAddress(const v8::FunctionCallbackInfo& args); static void AddRange(const v8::FunctionCallbackInfo& args); diff --git a/tools/doc/type-parser.js b/tools/doc/type-parser.js index 6441b5eef70fda..9e54e92aeee687 100644 --- a/tools/doc/type-parser.js +++ b/tools/doc/type-parser.js @@ -122,6 +122,7 @@ const customTypesMap = { 'require': 'modules.html#modules_require_id', 'Handle': 'net.html#net_server_listen_handle_backlog_callback', + 'net.BlockList': 'net.html#net_class_net_blocklist', 'net.Server': 'net.html#net_class_net_server', 'net.Socket': 'net.html#net_class_net_socket', From 40e2ca37908bf6b7a511f93f3ba1a9d8c50f4479 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 11 Aug 2020 15:46:25 -0700 Subject: [PATCH 6/6] quic: use net.BlockList for limiting access to a QuicSocket --- doc/api/quic.md | 18 ++++++++++ lib/internal/quic/core.js | 11 ++++++ src/quic/node_quic_session.h | 2 +- src/quic/node_quic_socket.cc | 13 +++++++ src/quic/node_quic_socket.h | 1 + test/parallel/test-quic-blocklist.js | 52 ++++++++++++++++++++++++++++ 6 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-quic-blocklist.js diff --git a/doc/api/quic.md b/doc/api/quic.md index 84484357b72fb4..486c8ee6d6848e 100644 --- a/doc/api/quic.md +++ b/doc/api/quic.md @@ -1445,6 +1445,24 @@ error will be thrown if `quicsock.addEndpoint()` is called either after the `QuicSocket` has already started binding to the local ports, or after the `QuicSocket` has been destroyed. +#### `quicsocket.blockList` + + +* Type: {net.BlockList} + +A {net.BlockList} instance used to define rules for remote IPv4 or IPv6 +addresses that this `QuicSocket` is not permitted to interact with. The +rules can be specified as either specific individual addresses, ranges +of addresses, or CIDR subnet ranges. + +When listening as a server, if a packet is received from a blocked address, +the packet will be ignored. + +When connecting as a client, if the remote IP address is blocked, the +connection attempt will be rejected. + #### `quicsocket.bound`