QUIC: add error code for handshake failed.

[Sergey Kandaurov]

> On 17 Feb 2023, at 09:17, Jiuzhou Cui <jiuzhoucui at 163.com> wrote:
> 
> Maybe NGX_QUIC_ERR_CRYPTO(sslerr) is not suitable here.
> 
> I think error and error_reason should be set together.
> 
> My scenes is error_reason is "handshake failed", but error is 8 set by parse TRANSPORT_PARAMETER failed before. My actual problem is parse transport parameter failed, so maybe error_reason "failed to process transport parameters" is useful for me to positioning problem. What do you think about this?
> 

Actually, both error and error_reason are usually set together.
The problem is in overwriting error_reason that was previously set
specifically for transport parameters parsing error, during handshake,
with a generic "handshake failed" phrase.  Please see the patch.

# HG changeset patch
# User Sergey Kandaurov <pluknet at nginx.com>
# Date 1676638271 -14400
#      Fri Feb 17 16:51:11 2023 +0400
# Branch quic
# Node ID 09c7414487aa82b40d4ab33738dc578363fe3273
# Parent  2fcd590d85da9c3a0205a18cb295ec316c03f18e
QUIC: using most specific handshake error reasons.

A QUIC handshake may fail for a variety of reasons.  Some of them arise
from our own checks and have a knownledge to set a suitable reason phrase.
Previously, it was overridden with a generic "handshake failed" phrase.
The latter is used for handshake errors generated in the SSL library and
reported with the send_alert callback.  Now a specific phrase is preserved.

Reported by Jiuzhou Cui.

diff --git a/src/event/quic/ngx_event_quic_ssl.c b/src/event/quic/ngx_event_quic_ssl.c
--- a/src/event/quic/ngx_event_quic_ssl.c
+++ b/src/event/quic/ngx_event_quic_ssl.c
@@ -423,7 +423,11 @@ ngx_quic_crypto_input(ngx_connection_t *
 
         if (sslerr != SSL_ERROR_WANT_READ) {
             ngx_ssl_error(NGX_LOG_ERR, c->log, 0, "SSL_do_handshake() failed");
-            qc->error_reason = "handshake failed";
+
+            if (!qc->error_reason) {
+                qc->error_reason = "handshake failed";
+            }
+
             return NGX_ERROR;
         }
     }

> 
> At 2023-02-16 22:42:27, "Sergey Kandaurov" <pluknet at nginx.com> wrote:
> >
> >> On 16 Feb 2023, at 17:36, Jiuzhou Cui <jiuzhoucui at 163.com> wrote:
> >> 
> >> Hello!
> >> 
> >> # HG changeset patch
> >> # User Jiuzhou Cui <cuijiuzhou at alibaba-inc.com>
> >> # Date 1676554419 -28800
> >> #      Thu Feb 16 21:33:39 2023 +0800
> >> # Branch quic
> >> # Node ID 13396c3ad10bdc8c1ac6969e965ceac510dc162f
> >> # Parent  b87a0dbc1150f415def5bc1e1f00d02b33519026
> >> QUIC: add error code for handshake failed.
> >> 
> >> diff -r b87a0dbc1150 -r 13396c3ad10b src/event/quic/ngx_event_quic_ssl.c
> >> --- a/src/event/quic/ngx_event_quic_ssl.c       Tue Oct 25 12:52:09 2022 +0400
> >> +++ b/src/event/quic/ngx_event_quic_ssl.c       Thu Feb 16 21:33:39 2023 +0800
> >> @@ -202,7 +202,7 @@
> >>          SSL_get0_alpn_selected(ssl_conn, &alpn_data, &alpn_len);
> >> 
> >>          if (alpn_len == 0) {
> >> -            qc->error = 0x100 + SSL_AD_NO_APPLICATION_PROTOCOL;
> >> +            qc->error = NGX_QUIC_ERR_CRYPTO(SSL_AD_NO_APPLICATION_PROTOCOL);
> >>              qc->error_reason = "unsupported protocol in ALPN extension";
> >> 
> >>              ngx_log_error(NGX_LOG_INFO, c->log, 0,
> >> @@ -452,6 +452,7 @@
> >> 
> >>          if (sslerr != SSL_ERROR_WANT_READ) {
> >>              ngx_ssl_error(NGX_LOG_ERR, c->log, 0, "SSL_do_handshake() failed");
> >> +            qc->error = NGX_QUIC_ERR_CRYPTO(sslerr);
> >>              qc->error_reason = "handshake failed";
> >>              return NGX_ERROR;
> >>          }
> >
> >Thank you for the patch.
> >
> >Applying to TLS handshake, qc->error is used to keep CRYPTO_ERROR,
> >a value based on the TLS alert.  You are trying to set there
> >something different, this is not going to work.
> >
> >More, qc->error is usually set in the send_alert callback as passed from
> >TLS, so no need to deal with it here.  Other places are QUIC protocol-
> >specific additions to negotiate ALPN and to carry transport parameters,
> >they are managed elsewhere.
> >
> >The ALPN part is ok.  Looks like it was missed from the 97adb87f149b
> >change, which went soon after ALPN checks added in a2c34e77cfc1.
> >
> ># HG changeset patch
> ># User Sergey Kandaurov <pluknet at nginx.com>
> ># Date 1676558213 -14400
> >#      Thu Feb 16 18:36:53 2023 +0400
> ># Branch quic
> ># Node ID 2fcd590d85da9c3a0205a18cb295ec316c03f18e
> ># Parent  12b756caaf167d2239fd3bd7a75b270ca89ca26b
> >QUIC: using NGX_QUIC_ERR_CRYPTO macro in ALPN checks.
> >
> >Patch by Jiuzhou Cui.
> >
> >diff --git a/src/event/quic/ngx_event_quic_ssl.c b/src/event/quic/ngx_event_quic_ssl.c
> >--- a/src/event/quic/ngx_event_quic_ssl.c
> >+++ b/src/event/quic/ngx_event_quic_ssl.c
> >@@ -190,7 +190,7 @@ ngx_quic_add_handshake_data(ngx_ssl_conn
> >         SSL_get0_alpn_selected(ssl_conn, &alpn_data, &alpn_len);
> > 
> >         if (alpn_len == 0) {
> >-            qc->error = 0x100 + SSL_AD_NO_APPLICATION_PROTOCOL;
> >+            qc->error = NGX_QUIC_ERR_CRYPTO(SSL_AD_NO_APPLICATION_PROTOCOL);
> >             qc->error_reason = "unsupported protocol in ALPN extension";
> > 
> >             ngx_log_error(NGX_LOG_INFO, c->log, 0,
> >
> >-- 
> >Sergey Kandaurov
> >_______________________________________________
> >nginx-devel mailing list
> >nginx-devel at nginx.org
> >https://mailman.nginx.org/mailman/listinfo/nginx-devel
> 
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

-- 
Sergey Kandaurov