quic: use the SocketAddressLRU to track validation status

PR-URL: #34618 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Aug 10, 2020 · 5b6cd6f · 5b6cd6f
1 parent f75e69a
commit 5b6cd6f
Show file tree

Hide file tree

Showing 7 changed files with 19 additions and 37 deletions.
diff --git a/doc/api/quic.md b/doc/api/quic.md
@@ -288,11 +288,6 @@ added: REPLACEME
   * `validateAddress` {boolean} When `true`, the `QuicSocket` will use explicit
     address validation using a QUIC `RETRY` frame when listening for new server
     sessions. Default: `false`.
-  * `validateAddressLRU` {boolean} When `true`, validation will be skipped if
-    the address has been recently validated. Currently, only the 10 most
-    recently validated addresses are remembered. Setting `validateAddressLRU`
-    to `true`, will enable the `validateAddress` option as well. Default:
-    `false`.
 
 The `net.createQuicSocket()` function is used to create new `QuicSocket`
 instances associated with a local UDP address.

diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js
@@ -179,7 +179,6 @@ const {
     QUICCLIENTSESSION_OPTION_REQUEST_OCSP,
     QUICCLIENTSESSION_OPTION_VERIFY_HOSTNAME_IDENTITY,
     QUICSOCKET_OPTIONS_VALIDATE_ADDRESS,
-    QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU,
     QUICSTREAM_HEADERS_KIND_NONE,
     QUICSTREAM_HEADERS_KIND_INFORMATIONAL,
     QUICSTREAM_HEADERS_KIND_INITIAL,
@@ -941,9 +940,6 @@ class QuicSocket extends EventEmitter {
       // True if address verification should be used.
       validateAddress,
 
-      // True if an LRU should be used for add validation
-      validateAddressLRU,
-
       // Whether qlog should be enabled for sessions
       qlog,
 
@@ -964,8 +960,6 @@ class QuicSocket extends EventEmitter {
     let socketOptions = 0;
     if (validateAddress)
       socketOptions |= (1 << QUICSOCKET_OPTIONS_VALIDATE_ADDRESS);
-    if (validateAddressLRU)
-      socketOptions |= (1 << QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU);
 
     this[kSetHandle](
       new QuicSocketHandle(

diff --git a/lib/internal/quic/util.js b/lib/internal/quic/util.js
@@ -539,7 +539,6 @@ function validateQuicSocketOptions(options = {}) {
     retryTokenTimeout = DEFAULT_RETRYTOKEN_EXPIRATION,
     server = {},
     statelessResetSecret,
-    validateAddressLRU = false,
     validateAddress = false,
   } = options;
 
@@ -548,7 +547,6 @@ function validateQuicSocketOptions(options = {}) {
   validateObject(server, 'options.server');
   validateLookup(lookup);
   validateBoolean(validateAddress, 'options.validateAddress');
-  validateBoolean(validateAddressLRU, 'options.validateAddressLRU');
   validateBoolean(qlog, 'options.qlog');
   validateBoolean(disableStatelessReset, 'options.disableStatelessReset');
 
@@ -597,8 +595,7 @@ function validateQuicSocketOptions(options = {}) {
     retryTokenTimeout,
     server,
     type,
-    validateAddress: validateAddress || validateAddressLRU,
-    validateAddressLRU,
+    validateAddress,
     qlog,
     statelessResetSecret,
     disableStatelessReset,

diff --git a/src/quic/node_quic.cc b/src/quic/node_quic.cc
@@ -189,7 +189,6 @@ void Initialize(Local<Object> target,
   V(QUICSERVERSESSION_OPTION_REJECT_UNAUTHORIZED)                              \
   V(QUICSERVERSESSION_OPTION_REQUEST_CERT)                                     \
   V(QUICSOCKET_OPTIONS_VALIDATE_ADDRESS)                                       \
-  V(QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU)                                   \
   V(QUICSTREAM_HEADER_FLAGS_NONE)                                              \
   V(QUICSTREAM_HEADER_FLAGS_TERMINAL)                                          \
   V(QUICSTREAM_HEADERS_KIND_NONE)                                              \

diff --git a/src/quic/node_quic_socket.cc b/src/quic/node_quic_socket.cc
@@ -675,23 +675,27 @@ bool QuicSocket::SendStatelessReset(
 // peer must termination it's initial attempt to
 // establish a connection and start a new attempt.
 //
-// TODO(@jasnell): Retry packets will only ever be
-// generated by QUIC servers, and only if the QuicSocket
-// is configured for explicit path validation. There is
-// no way for a client to force a retry packet to be created.
-// However, once a client determines that explicit
-// path validation is enabled, it could attempt to
-// DOS by sending a large number of malicious
-// initial packets to intentionally ellicit retry
-// packets (It can do so by intentionally sending
-// initial packets that ignore the retry token).
-// To help mitigate that risk, we should limit the number
-// of retries we send to a given remote endpoint.
+// Retry packets will only ever be generated by QUIC servers,
+// and only if the QuicSocket is configured for explicit path
+// validation. There is no way for a client to force a retry
+// packet to be created. However, once a client determines that
+// explicit path validation is enabled, it could attempt to
+// DOS by sending a large number of malicious initial packets
+// to intentionally ellicit retry packets (It can do so by
+// intentionally sending initial packets that ignore the retry
+// token). To help mitigate that risk, we limit the number of
+// retries we send to a given remote endpoint.
 bool QuicSocket::SendRetry(
     const QuicCID& dcid,
     const QuicCID& scid,
     const SocketAddress& local_addr,
     const SocketAddress& remote_addr) {
+  auto info = addrLRU_.Upsert(remote_addr);
+  // Do not send a retry if the retry count is greater
+  // than the retry limit.
+  // TODO(@jasnell): Make the retry limit configurable.
+  if (++(info->retry_count) > DEFAULT_MAX_RETRY_LIMIT)
+    return true;
   std::unique_ptr<QuicPacket> packet =
       GenerateRetryPacket(token_secret_, dcid, scid, local_addr, remote_addr);
   return packet ?

diff --git a/src/quic/node_quic_socket.h b/src/quic/node_quic_socket.h
@@ -36,10 +36,10 @@ class QuicSocket;
 class QuicEndpoint;
 
 constexpr size_t DEFAULT_MAX_SOCKETADDRESS_LRU_SIZE = 1000;
+constexpr size_t DEFAULT_MAX_RETRY_LIMIT = 10;
 
 #define QUICSOCKET_OPTIONS(V)                                                  \
-    V(VALIDATE_ADDRESS, validate_address)                                      \
-    V(VALIDATE_ADDRESS_LRU, validate_address_lru)
+    V(VALIDATE_ADDRESS, validate_address)
 
 #define V(id, _) QUICSOCKET_OPTIONS_##id,
 enum QuicSocketOptions : uint32_t {

diff --git a/test/parallel/test-quic-errors-quicsocket-create.js b/test/parallel/test-quic-errors-quicsocket-create.js
@@ -68,13 +68,6 @@ const { createQuicSocket } = require('net');
   });
 });
 
-// Test invalid QuicSocket validateAddressLRU argument option
-[1, NaN, 1n, null, {}, []].forEach((validateAddressLRU) => {
-  assert.throws(() => createQuicSocket({ validateAddressLRU }), {
-    code: 'ERR_INVALID_ARG_TYPE'
-  });
-});
-
 // Test invalid QuicSocket qlog argument option
 [1, NaN, 1n, null, {}, []].forEach((qlog) => {
   assert.throws(() => createQuicSocket({ qlog }), {