[PATCH] QUIC: refined sending CONNECTION_CLOSE in various packet types

[Roman Arutyunyan]

Hi,

On Thu, Aug 31, 2023 at 06:59:46PM +0400, Sergey Kandaurov wrote:
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1693493736 -14400
> #      Thu Aug 31 18:55:36 2023 +0400
> # Node ID 358c657a4a7afef502a00b9a41bddbe08f6859ae
> # Parent  015353ca1be7acc176f6369ed92ec6c49975ee6a
> QUIC: refined sending CONNECTION_CLOSE in various packet types.
> 
> As per RFC 9000, section 10.2.3, to ensure that peer successfully removed
> packet protection, CONNECTION_CLOSE can be sent in multiple packets using
> different packet protection levels.
> 
> Specifically, new logic is added to more strictly follow these rules:
> 
> - by default, the highest available level of packet protection is used;
> - unless handshake is confirmed, but we have got application keys available,
>   that means the client may have or may have not application keys to remove

may *not* have ?

>   1-RTT packet protection; in such case, send both 1-RTT and HS packets;
> - additionally, if we still have initial protection keys not yet discarded,
>   which happens if the path was not yet validated by successfully removing
>   Handshake packet protection, that means the client may not have handshake
>   keys; in such case, send an Initial packet too.
> 
> This roughly resembles the following paragraph:
> 
> * Prior to confirming the handshake, a peer might be unable to process 1-RTT
>   packets, so an endpoint SHOULD send a CONNECTION_CLOSE frame in both Handshake
>   and 1-RTT packets.  A server SHOULD also send a CONNECTION_CLOSE frame in an
>   Initial packet.
> 
> In practice, this change allows to avoid sending Initial packet when we know
> the client has handshake keys, and fixes sending CONNECTION_CLOSE when using
> QuicTLS with old QUIC API, where TLS stack releases application read keys
> before handshake confirmation, sending it additionally in a Handshake packet.
> 
> diff --git a/src/event/quic/ngx_event_quic.c b/src/event/quic/ngx_event_quic.c
> --- a/src/event/quic/ngx_event_quic.c
> +++ b/src/event/quic/ngx_event_quic.c
> @@ -540,8 +540,20 @@ ngx_quic_close_connection(ngx_connection
>  
>              (void) ngx_quic_send_cc(c);
>  
> -            if (qc->error_level == ssl_encryption_handshake) {
> -                /* for clients that might not have handshake keys */
> +            if (qc->error_level == ssl_encryption_application
> +                && ngx_quic_keys_available(qc->keys,
> +                                           ssl_encryption_handshake))
> +            {
> +                /* handshake not confirmed, client may not have app keys */
> +                qc->error_level = ssl_encryption_handshake;
> +                (void) ngx_quic_send_cc(c);
> +            }
> +
> +            if (qc->error_level == ssl_encryption_handshake
> +                && ngx_quic_keys_available(qc->keys,
> +                                           ssl_encryption_initial))
> +            {
> +                /* path not validated, client may not have hs keys */
>                  qc->error_level = ssl_encryption_initial;
>                  (void) ngx_quic_send_cc(c);
>              }

I have a feeling that we can just send CC for all levels for which keys are
available:

for (i = 0; i < NGX_QUIC_SEND_CTX_LAST; i++) {                       
	ctx = &qc->send_ctx[i];                                          
																	 
	if (!ngx_quic_keys_available(qc->keys, ctx->level)) {            
		continue;                                                    
	}                                                                
																	 
	qc->error_level = ctx->level;                                    
	(void) ngx_quic_send_cc(c);                                      
																	 
	if (rc == NGX_OK && !qc->close.timer_set) {                      
		ngx_add_timer(&qc->close, 3 * ngx_quic_pto(c, ctx));         
	}                                                                
}  

--
Roman Arutyunyan