Resolve some of the TODO(QUIC) items

For some of the items we add FUTURE/SERVER/TESTING/MULTIPATH
designation to indicate these do not need to be resolved
in QUIC MVP release.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #21539)

doc/designs/quic-design/quic-api.md

@@ -631,10 +631,6 @@ object (i.e., a single bytestream); those components may well already call
 `SSL_shutdown` and it is not desired for such calls to affect the whole
 connection.
 
-**TBD:** Should we allow `SSL_shutdown_ex` on a QUIC stream SSL object to act as
-`SSL_stream_conclude`? Should we require this behaviour to be explicitly
-enabled?
-
 The `args->quic_error_code` and `args->reason` fields allow the application
 error code and reason string for the closure of a QUIC connection to be
 specified. If `args` or `args->reason` is `NULL`, a zero-length string is used

doc/designs/quic-design/quic-fault-injector.md

@@ -189,8 +189,8 @@ initial API will provide a basic set of listeners and helper functions in order
 to provide the basis for future work.
 
 The following outlines an illustrative set of functions that will initially be
-provided. A number of `TODO(QUIC)` comments are inserted to explain how we
-might expand the API over time:
+provided. A number of `TODO(QUIC TESTING)` comments are inserted to explain how
+we might expand the API over time:
 
 ```` C
 /* Type to represent the Fault Injector */
@@ -295,15 +295,15 @@ int ossl_quic_fault_set_handshake_listener(OSSL_QUIC_FAULT *fault,
 int ossl_quic_fault_resize_handshake(OSSL_QUIC_FAULT *fault, size_t newlen);
 
 /*
- * TODO(QUIC): Add listeners for specific types of frame here. E.g. we might
- * expect to see an "ACK" frame listener which will be passed pre-parsed ack
- * data that can be modified as required.
+ * TODO(QUIC TESTING): Add listeners for specific types of frame here. E.g.
+ * we might expect to see an "ACK" frame listener which will be passed
+ * pre-parsed ack data that can be modified as required.
  */
 
 /*
  * Handshake message specific listeners. Unlike the general handshake message
  * listener these messages are pre-parsed and supplied with message specific
- * data and exclude the handshake header
+ * data and exclude the handshake header.
  */
 typedef int (*ossl_quic_fault_on_enc_ext_cb)(OSSL_QUIC_FAULT *fault,
                                              OSSL_QF_ENCRYPTED_EXTENSIONS *ee,
@@ -314,7 +314,7 @@ int ossl_quic_fault_set_hand_enc_ext_listener(OSSL_QUIC_FAULT *fault,
                                               ossl_quic_fault_on_enc_ext_cb encextcb,
                                               void *encextcbarg);
 
-/* TODO(QUIC): Add listeners for other types of handshake message here */
+/* TODO(QUIC TESTING): Add listeners for other types of handshake message here */
 
 
 /*
@@ -338,9 +338,9 @@ int ossl_quic_fault_delete_extension(OSSL_QUIC_FAULT *fault,
                                      size_t *extlen);
 
 /*
- * TODO(QUIC): Add additional helper functions for querying extensions here (e.g.
- * finding or adding them). We could also provide a "listener" API for listening
- * for specific extension types
+ * TODO(QUIC TESTING): Add additional helper functions for querying extensions
+ * here (e.g. finding or adding them). We could also provide a "listener" API
+ * for listening for specific extension types.
  */
 
 /*
@@ -465,19 +465,9 @@ static int test_unknown_frame(void)
     if (!TEST_int_eq(SSL_get_error(cssl, ret), SSL_ERROR_SSL))
         goto err;
 
-#if 0
-    /*
-     * TODO(QUIC): We should expect an error on the queue after this - but we
-     * don't have it yet.
-     * Note, just raising the error in the obvious place causes SSL_tick() to
-     * succeed, but leave a spurious error on the stack. We need to either
-     * allow SSL_tick() to fail, or somehow delay the raising of the error
-     * until the SSL_read() call.
-     */
     if (!TEST_int_eq(ERR_GET_REASON(ERR_peek_error()),
                      SSL_R_UNKNOWN_FRAME_TYPE_RECEIVED))
         goto err;
-#endif
 
     if (!TEST_true(qtest_check_server_protocol_err(qtserv)))
         goto err;

doc/man3/SSL_accept_stream.pod

@@ -45,7 +45,8 @@ TODO(QUIC): Revise in MSMT PR to mention threading considerations.
 
 Depending on whether default stream functionality is being used, it may be
 necessary to explicitly configure the incoming stream policy before streams can
-be accepted; see L<SSL_set_incoming_stream_policy(3)>.
+be accepted; see L<SSL_set_incoming_stream_policy(3)>. See also
+L<openssl-quic(7)/MODES OF OPERATION>.
 
 =begin comment
 
@@ -67,7 +68,8 @@ a QUIC connection SSL object.
 
 =head1 SEE ALSO
 
-L<SSL_new_stream(3)>, L<SSL_set_blocking_mode(3)>, L<SSL_free(3)>
+L<openssl-quic(7)/MODES OF OPERATION>, L<SSL_new_stream(3)>,
+L<SSL_set_blocking_mode(3)>, L<SSL_free(3)>
 
 =head1 HISTORY
 

doc/man3/SSL_get_version.pod

@@ -21,14 +21,16 @@ SSL_version - get the protocol information of a connection
 
 =head1 DESCRIPTION
 
-SSL_client_version() returns the numeric protocol version advertised by the
-client in the legacy_version field of the ClientHello when initiating the
-connection. Note that, for TLS, this value will never indicate a version greater
-than TLSv1.2 even if TLSv1.3 is subsequently negotiated. SSL_get_version()
-returns the name of the protocol used for the connection. SSL_version() returns
-the numeric protocol version used for the connection. They should only be called
-after the initial handshake has been completed. Prior to that the results
-returned from these functions may be unreliable.
+For SSL, TLS and DTLS protocols SSL_client_version() returns the numeric
+protocol version advertised by the client in the legacy_version field of the
+ClientHello when initiating the connection. Note that, for TLS, this value
+will never indicate a version greater than TLSv1.2 even if TLSv1.3 is
+subsequently negotiated. For QUIC connections it returns OSSL_QUIC1_VERSION.
+
+SSL_get_version() returns the name of the protocol used for the connection.
+SSL_version() returns the numeric protocol version used for the connection.
+They should only be called after the initial handshake has been completed.
+Prior to that the results returned from these functions may be unreliable.
 
 SSL_is_dtls() returns 1 if the connection is using DTLS or 0 if not.
 
@@ -125,8 +127,7 @@ The connection uses the DTLSv1.2 protocol
 
 =item OSSL_QUIC1_VERSION
 
-The connection uses the QUICv1 protocol (never returned for
-SSL_client_version()).
+The connection uses the QUICv1 protocol.
 
 =back
 

doc/man3/SSL_handle_events.pod

@@ -64,11 +64,9 @@ will be made to the object for a substantial period of time. So long as at least
 one call to the SSL object is blocking, no such call is needed. However,
 SSL_handle_events() may optionally be used on a QUIC connection object if desired.
 
-=begin comment
-
-TODO(QUIC): Update the above paragraph once we support thread assisted mode.
-
-=end comment
+With the thread-assisted mode of operation L<OSSL_QUIC_client_thread_method(3)>
+it is unnecessary to call SSL_handle_events() as the assist thread handles the QUIC
+connection events.
 
 =back
 

doc/man3/SSL_shutdown.pod

@@ -105,12 +105,6 @@ zero-initialised B<SSL_SHUTDOWN_EX_ARGS> structure. When used with a non-QUIC
 SSL object, the arguments are ignored and the call functions identically to
 SSL_shutdown().
 
-=begin comment
-
-TODO(QUIC): Once streams are implemented, revise this text
-
-=end comment
-
 When used with a QUIC connection SSL object, SSL_shutdown_ex() initiates a QUIC
 immediate close. The I<quic_error_code> field can be used to specify a 62-bit
 application error code to be signalled via QUIC. The value specified must be in
@@ -263,6 +257,8 @@ BIOs.
 
 It can also occur when not all data was read using SSL_read().
 
+This value is also returned when called on QUIC stream SSL objects.
+
 =back
 
 =head1 SEE ALSO

doc/man3/SSL_stream_conclude.pod

@@ -13,8 +13,8 @@ SSL_stream_conclude - conclude the sending part of a QUIC stream
 =head1 DESCRIPTION
 
 SSL_stream_conclude() signals the normal end-of-stream condition for the send
-part of a QUIC stream. If called on a QUIC connection SSL object, it signals the
-end of the single stream to the peer.
+part of a QUIC stream. If called on a QUIC connection SSL object with an
+associated default stream, it signals the end of the single stream to the peer.
 
 Any data already queued for transmission via a call to SSL_write() will still be
 written in a reliable manner before the end-of-stream is signalled, assuming the
@@ -32,23 +32,17 @@ I<flags> is reserved and should be set to 0.
 Only the first call to this function has any effect for a given stream;
 subsequent calls are no-ops. This is considered a success case.
 
-=begin comment
-
-TODO(QUIC): Once streams are implemented, revise this text
-
-=end comment
-
-This function is not supported on non-QUIC SSL objects.
+This function is not supported on an object other than a QUIC stream SSL object.
 
 =head1 RETURN VALUES
 
 Returns 1 on success and 0 on failure.
 
-Returns 0 if called on a non-QUIC SSL object.
+Returns 0 if called on an SSL object not representing a QUIC stream.
 
 =head1 SEE ALSO
 
-L<ssl(7)>, L<SSL_shutdown_ex(3)>
+L<openssl-quic(7)>, L<ssl(7)>, L<SSL_shutdown_ex(3)>
 
 =head1 HISTORY
 

include/internal/quic_stream_map.h

@@ -297,8 +297,8 @@ struct quic_stream_st {
      *            reasonably certain no benefit would be gained by sending
      *            STOP_SENDING.]
      *
-     *            TODO(QUIC): Implement the latter case (currently we just
-     *                        always do STOP_SENDING).
+     *            TODO(QUIC FUTURE): Implement the latter case (currently we
+                                     just always do STOP_SENDING).
      *
      *         and;
      *

ssl/quic/cc_newreno.c

@@ -36,7 +36,7 @@ typedef struct ossl_cc_newreno_st {
 
 #define MIN_MAX_INIT_WND_SIZE    14720  /* RFC 9002 s. 7.2 */
 
-/* TODO(QUIC): Pacing support. */
+/* TODO(QUIC FUTURE): Pacing support. */
 
 static void newreno_set_max_dgram_size(OSSL_CC_NEWRENO *nr,
                                        size_t max_dgram_size);

ssl/quic/quic_ackm.c

@@ -927,7 +927,7 @@ static int ackm_set_loss_detection_timer(OSSL_ACKM *ackm)
 static int ackm_in_persistent_congestion(OSSL_ACKM *ackm,
                                          const OSSL_ACKM_TX_PKT *lpkt)
 {
-    /* TODO(QUIC): Persistent congestion not currently implemented. */
+    /* TODO(QUIC FUTURE): Persistent congestion not currently implemented. */
     return 0;
 }
 
@@ -1284,7 +1284,8 @@ static void ackm_queue_probe_anti_deadlock_initial(OSSL_ACKM *ackm)
 static void ackm_queue_probe(OSSL_ACKM *ackm, int pkt_space)
 {
     /*
-     * TODO(QUIC): We are allowed to send either one or two probe packets here.
+     * TODO(QUIC FUTURE): We are allowed to send either one or two probe
+     * packets here.
      * Determine a strategy for when we should send two probe packets.
      */
     ++ackm->pending_probe.pto[pkt_space];

ssl/quic/quic_channel.c

@@ -21,8 +21,8 @@
  * not suitable for network use. In particular, it does not implement address
  * validation, anti-amplification or retry logic.
  *
- * TODO(QUIC): Implement address validation and anti-amplification
- * TODO(QUIC): Implement retry logic
+ * TODO(QUIC SERVER): Implement address validation and anti-amplification
+ * TODO(QUIC SERVER): Implement retry logic
  */
 
 #define INIT_DCID_LEN           8
@@ -1380,7 +1380,7 @@ static int ch_on_transport_params(const unsigned char *params,
 
         case QUIC_TPARAM_PREFERRED_ADDR:
             {
-                /* TODO(QUIC): Handle preferred address. */
+                /* TODO(QUIC FUTURE): Handle preferred address. */
                 QUIC_PREFERRED_ADDR pfa;
 
                 /*
@@ -1684,7 +1684,7 @@ static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags)
              * again because packets that were not previously processable and
              * were deferred might now be processable.
              *
-             * TODO(QUIC): Consider handling this in the yield_secret callback.
+             * TODO(QUIC FUTURE): Consider handling this in the yield_secret callback.
              */
         } while (ch->have_new_rx_secret);
     }
@@ -1985,7 +1985,7 @@ static void ch_rx_handle_packet(QUIC_CHANNEL *ch)
             return;
 
         /*
-         * TODO(QUIC): Theoretically this should probably be in the QRX.
+         * TODO(QUIC FUTURE): Theoretically this should probably be in the QRX.
          * However because validation is dependent on context (namely the
          * client's initial DCID) we can't do this cleanly. In the future we
          * should probably add a callback to the QRX to let it call us (via
@@ -2011,8 +2011,8 @@ static void ch_rx_handle_packet(QUIC_CHANNEL *ch)
             return;
 
         /*
-         * TODO(QUIC): Implement 0-RTT on the server side. We currently do
-         * not need to implement this as a client can only do 0-RTT if we
+         * TODO(QUIC 0RTT): Implement 0-RTT on the server side. We currently
+         * do not need to implement this as a client can only do 0-RTT if we
          * have given it permission to in a previous session.
          */
         break;
@@ -2124,7 +2124,7 @@ static void ch_default_packet_handler(QUIC_URXE *e, void *arg)
         case QUIC_VERSION_NONE:
         default:
             /* Unknown version or proactive version negotiation request, bail. */
-            /* TODO(QUIC): Handle version negotiation on server side */
+            /* TODO(QUIC SERVER): Handle version negotiation on server side */
             goto undesirable;
     }
 

ssl/quic/quic_impl.c

@@ -375,7 +375,7 @@ SSL *ossl_quic_new(SSL_CTX *ctx)
         = (ssl_base->method == OSSL_QUIC_client_thread_method());
 #endif
 
-    qc->as_server       = 0; /* TODO(QUIC): server support */
+    qc->as_server       = 0; /* TODO(QUIC SERVER): add server support */
     qc->as_server_state = qc->as_server;
 
     qc->default_stream_mode     = SSL_DEFAULT_STREAM_MODE_AUTO_BIDI;
@@ -547,7 +547,7 @@ int ossl_quic_clear(SSL *s)
     if (!expect_quic(s, &ctx))
         return 0;
 
-    /* TODO(QUIC): Currently a no-op. */
+    /* TODO(QUIC FUTURE): Currently a no-op. */
     return 1;
 }
 
@@ -1172,7 +1172,6 @@ int ossl_quic_conn_shutdown(SSL *s, uint64_t flags,
         return -1;
 
     if (ctx.is_stream)
-        /* TODO(QUIC): Semantics currently undefined for QSSOs */
         return -1;
 
     quic_lock(ctx.qc);
@@ -1412,7 +1411,6 @@ static int quic_do_handshake(QCTX *ctx)
     }
 
     if (qc->as_server != qc->as_server_state) {
-        /* TODO(QUIC): Must match the method used to create the QCSO */
         QUIC_RAISE_NON_NORMAL_ERROR(ctx, ERR_R_PASSED_INVALID_ARGUMENT, NULL);
         return -1; /* Non-protocol error */
     }
@@ -1800,8 +1798,8 @@ static void quic_post_write(QUIC_XSO *xso, int did_append, int do_tick)
     /*
      * Try and send.
      *
-     * TODO(QUIC): It is probably inefficient to try and do this immediately,
-     * plus we should eventually consider Nagle's algorithm.
+     * TODO(QUIC FUTURE): It is probably inefficient to try and do this
+     * immediately, plus we should eventually consider Nagle's algorithm.
      */
     if (do_tick)
         ossl_quic_reactor_tick(ossl_quic_channel_get_reactor(xso->conn->ch), 0);

ssl/quic/quic_local.h

@@ -293,7 +293,7 @@ const SSL_METHOD *func_name(void)  \
                 ossl_quic_free, \
                 ossl_quic_reset, \
                 ossl_quic_init, \
-                ossl_quic_clear, \
+                NULL /* clear */, \
                 ossl_quic_deinit, \
                 q_accept, \
                 q_connect, \

ssl/quic/quic_reactor.c

@@ -342,14 +342,14 @@ int ossl_quic_reactor_block_until_pred(QUIC_REACTOR *rtor,
              * things again. If poll_two_fds returns 0, this is some other
              * non-timeout failure and we should stop here.
              *
-             * TODO(QUIC): In the future we could avoid unnecessary syscalls by
-             * not retrying network I/O that isn't ready based on the result of
-             * the poll call. However this might be difficult because it
-             * requires we do the call to poll(2) or equivalent syscall
-             * ourselves, whereas in the general case the application does the
-             * polling and just calls SSL_handle_events(). Implementing this
-             * optimisation in the future will probably therefore require API
-             * changes.
+             * TODO(QUIC FUTURE): In the future we could avoid unnecessary
+             * syscalls by not retrying network I/O that isn't ready based
+             * on the result of the poll call. However this might be difficult
+             * because it requires we do the call to poll(2) or equivalent
+             * syscall ourselves, whereas in the general case the application
+             * does the polling and just calls SSL_handle_events().
+             * Implementing this optimisation in the future will probably
+             * therefore require API changes.
              */
             return 0;
     }

ssl/quic/quic_rstream.c

@@ -280,7 +280,6 @@ int ossl_quic_rstream_move_to_rbuf(QUIC_RSTREAM *qrs)
 
 int ossl_quic_rstream_resize_rbuf(QUIC_RSTREAM *qrs, size_t rbuf_size)
 {
-    /* TODO(QUIC): Do we need to distinguish different error conditions ? */
     if (ossl_sframe_list_is_head_locked(&qrs->fl))
         return 0;