> On 22 Aug 2025, at 11:49, Vladimir Homutov <[email protected]> wrote: > > On Thu, Aug 21, 2025 at 06:25:25PM +0400, Sergey Kandaurov wrote: >> On Fri, Aug 15, 2025 at 10:56:40AM +0300, Vladimir Homutov wrote: >>> Hello, >>> >>> Commits 7468a10b62276be4adee0fcd6aaf6244270984ab >>> "QUIC: adjusted handling of callback errors." >>> >>> and 47f96993f669543c6cb4979dd3f680ad01314ee5 >>> "QUIC: logging of SSL library errors." >>> >>> lead to the situation when you may get spurious >>> "ignoring stale global SSL error" errors in unrelated connections. >>> >>> This happens due to the fact that openssl error queue is (thread) global, >>> and quic handshake handler does not read out the error after the failed >>> handshake and relies on qc->error set by callback. >>> >>> So the error stays in queue and may show itself in unrelated quic >>> connection, >>> typically on SSL shutdown. >>> >>> config below may be used to reproduce the issue: >>> >>>>>> >>> daemon off; >>> error_log logs/error.log debug; >>> events { >>> } >>> >>> http { >>> ssl_certificate_key localhost.key; >>> ssl_certificate localhost.crt; >>> server { >>> error_log logs/good.log debug; >>> listen 127.0.0.1:8080 quic; >>> location / { return 200 OK; } >>> } >>> server { >>> error_log logs/reject.log debug; >>> listen 127.0.0.1:8081 quic; >>> ssl_reject_handshake on; >>> location / { return 200 OK; } >>> } >>> } >>> <<< >>> >>> start the server and run: >>> >>> $ curl -k --http3 https://127.0.0.1:8080/ >>> $ curl -k --http3 https://127.0.0.1:8081/ >>> >>> The result is alert in good.log: >>> >>> 2025/08/15 09:58:19 [alert] 1154786#1154786: *1 ignoring stale global SSL >>> error (SSL: error:10000084:SSL routines:OPENSSL_internal:CLIENTHELLO_TLSEXT >>> error:100000be:SSL routines:OPENSSL_internal:PARSE_TLSEXT) while preparing >>> ack, client: 127.0.0.1, server: 127.0.0.1:8080 >>> >>> caused by failed handshake in server 2; >> >> Thanks for reporting. >> I was able to reproduce this with Test::Nginx reliably: >> >> my $s = Test::Nginx::HTTP3->new(8980); >> my $s2 = Test::Nginx::HTTP3->new(8981, probe => 1); >> >> select undef, undef, undef, 0.1; >> $s = undef; >> >> select undef, undef, undef, 0.2; >> >> $t->stop(); >> >> So the error queue remains non-empty until the 1st connection SSL shutdown. >> The following happens as seen in debug (filtered): >> >> 2025/08/21 15:52:02 [debug] 73543#1490328: *1 quic run >> 2025/08/21 15:52:02 [debug] 73543#1490328: *1 SSL_do_handshake: -1 >> 2025/08/21 15:52:02 [debug] 73543#1490328: *1 SSL_do_handshake: 1 >> 2025/08/21 15:52:02 [debug] 73543#1490328: *6 quic run >> 2025/08/21 15:52:02 [debug] 73543#1490328: *6 quic ngx_quic_send_alert() >> level:0 alert:112 >> 2025/08/21 15:52:02 [debug] 73543#1490328: *6 SSL_do_handshake: -1 >> 2025/08/21 15:52:02 [alert] 73543#1490328: *1 ignoring stale global SSL >> error (SSL: error:10000084:SSL routines:OPENSSL_internal:CLIENTHELLO_TLSEXT >> error:100000be:SSL routines:OPENSSL_internal:PARSE_TLSEXT) while preparing >> ack, client: 127.0.0.1, server: 127.0.0.1:8980 >> 2025/08/21 15:52:02 [debug] 73543#1490328: *1 quic ngx_quic_send_alert() >> level:3 alert:0 >> 2025/08/21 15:52:02 [debug] 73543#1490328: *1 SSL_shutdown: 1 >> >> The above is using BoringSSL to match the report. >> This is equivalently broken with all other libraries >> with a slightly different error message: >> >> 2025/08/21 16:03:40 [alert] 78666#1507465: *1 ignoring stale global SSL >> error (SSL: error:0A0000EA:SSL routines::callback failed) while preparing >> ack, client: 127.0.0.1, server: 127.0.0.1:8980 >> >> I don't think 47f96993f669543c6cb4979dd3f680ad01314ee5 is related. >> Although it formally changes reproduction surface by ngx_ssl_error(), >> this touches errors, which should not normally happen in practice. >> 7468a10b62276be4adee0fcd6aaf6244270984ab is a real culprit. >> >>> >>> when the ngx_http_ssl_servername() callback returns error, >>> the ngx_quic_send_alert() sets qc->error, and the ngx_quic_ssl_handshake() >>> checks qc->error after SSL_do_handshake() and closes connection. >>> But the error stays, and manifests itself later in another connection >>> in the same process. >>> >>> The quic fix is probably something like this: >>> >>> diff --git a/src/event/quic/ngx_event_quic_ssl.c >>> b/src/event/quic/ngx_event_quic_ssl.c >>> index e961c80cd..dc0a030ff 100644 >>> --- a/src/event/quic/ngx_event_quic_ssl.c >>> +++ b/src/event/quic/ngx_event_quic_ssl.c >>> @@ -696,6 +696,7 @@ ngx_quic_handshake(ngx_connection_t *c) >>> ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0, "SSL_do_handshake: %d", >>> n); >>> >>> if (qc->error) { >>> + ERR_clear_error(); >>> return NGX_ERROR; >>> } >>> >>> Or may be it makes sense to clear error in the moment of setting qc->error; >>> and do not clear everything, but try to log using ngx_ssl_error() with some >>> non-alert level. >> >> What happens here is the added early check for qc->error broke the >> check for handshake_rejected, resulting in a missing diagnostics >> for the SNI-rejected handshake, and in a missing ERR_clear_error(). >> >> This can be fixed by moving the qc->error check to later such that >> handshake_rejected is checked first to make the error queue cleared. >> Although not practicably visible as needed, this can be also accompanied >> by clearing the error queue under the qc->error case as well, to be safe. >> >> Fixing this is complicated by different codes returned from SSL_get_error() >> for handshake errors happened in QUIC callbacks in various SSL libraries: >> >> # define SSL_ERROR_SSL 1 >> # define SSL_ERROR_WANT_READ 2 >> # define SSL_ERROR_WANT_WRITE 3 >> >> compat 3.5 bssl qtls lssl >> tp 2 3* 2 2 2 >> sni 1 1 1 1 1 >> >> In this regard, checking c->ssl->handshake_rejected is moved out of >> "sslerr != SSL_ERROR_WANT_READ". It is a set only flag, so this should >> be safe to do. This also somewhat simplifies and streamlines code. >> >> Patch also reconstructs a missing "SSL_do_handshake() failed" diagnostics >> for the qc->error case. It is made this way to avoid logging at the crit >> log level because qc->error is expected to have an empty error queue. >> >> Please test if it works for you. >> >> diff --git a/src/event/quic/ngx_event_quic_ssl.c >> b/src/event/quic/ngx_event_quic_ssl.c >> index e961c80cd..1de896291 100644 >> --- a/src/event/quic/ngx_event_quic_ssl.c >> +++ b/src/event/quic/ngx_event_quic_ssl.c >> @@ -695,25 +695,25 @@ ngx_quic_handshake(ngx_connection_t *c) >> >> ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0, "SSL_do_handshake: %d", >> n); >> >> - if (qc->error) { >> - return NGX_ERROR; >> - } >> - >> if (n <= 0) { >> sslerr = SSL_get_error(ssl_conn, n); >> >> ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0, "SSL_get_error: %d", >> sslerr); >> >> - if (sslerr != SSL_ERROR_WANT_READ) { >> - >> - if (c->ssl->handshake_rejected) { >> - ngx_connection_error(c, 0, "handshake rejected"); >> - ERR_clear_error(); >> + if (c->ssl->handshake_rejected) { >> + ngx_connection_error(c, 0, "handshake rejected"); >> + ERR_clear_error(); >> + return NGX_ERROR; >> + } >> >> - return NGX_ERROR; >> - } >> + if (qc->error) { >> + ngx_connection_error(c, 0, "SSL_do_handshake() failed"); >> + ERR_clear_error(); >> + return NGX_ERROR; >> + } >> >> + if (sslerr != SSL_ERROR_WANT_READ) { >> ngx_ssl_connection_error(c, sslerr, 0, "SSL_do_handshake() >> failed"); >> return NGX_ERROR; >> } > > yes, this works and totally makes sense. > > thank you!
Thanks for testing! Just for the record, submitted as https://github.com/nginx/nginx/pull/889 -- Sergey Kandaurov _______________________________________________ nginx-devel mailing list [email protected] https://mailman.nginx.org/mailman/listinfo/nginx-devel
