Opened 4 months ago

Closed 4 months ago

#1981 closed defect (fixed)

no logging of HTTP/1.x requests to a plain text HTTP/2 listening socket

Reported by: Hans-Cees Owned by:
Priority: minor Milestone:
Component: nginx-core Version: 1.19.x
Keywords: http2 proxy_pass Cc:
uname -a: 4.19.0-8-amd64 #1 SMP Debian 4.19.98-1 (2020-01-26) x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.18.0

Description

Hi,
two days ago you closed my ticket without offering me further explanation or input. So to give some more input I have to open another ticket alas.

Content of complaint (now) is: nginx does not offer any feedback via logging or other means that http2 on port 80 (and without ssl) does not establish a working connection at all and so is probably unwanted.

Since you have been closing tickets for 5 years now on this issue, it seems to be very unexpected for users that nginx just does not work as expected (serving as a webserver to browsers with this config mistake.

So while you are probably right that there is a hypothetical situation where http2 and no ssl is desired, this will more often not be the case.

Perhaps it could save you some work in closing all these tickets and finding the many duplicates if you would just add a warning if http2 is configured without ssl.

Thank you very much, and stay safe!

Change History (2)

comment:1 by Maxim Dounin, 4 months ago

Status: newaccepted
Summary: please fix #808 instead of closing many tickets: port 80 and http2 is brokenno logging of HTTP/1.x requests to a plain text HTTP/2 listening socket

You were given an explanation (see ticket:1979#comment:1), and ticket:808#comment:1 further explains, with appropriate RFC links, that the configuration you've written is not going to do what you expect it to do, but rather does a different thing. Note well that to provide additional input it is enough to add comments to already existing tickets, there is no need to open additional tickets.

Given that #808 is an invalid bug report, it is not going to be fixed, because the reported behaviour is not a bug. A feature request to additionally implement simultaneous use of HTTP and HTTP/2 over plain text sockets is filed as #816 (also referred in comments to ticket #808), though it is unlikely to be implemented, since the only meaningful use case for HTTP/2 over plain text is to support SSL/TLS termination somewhere else, and this use case is already sufficiently covered.

Adding a configuration-based warning is not an option either, since the use case for HTTP/2 over plain text sockets is real and such configurations are used in practice.

But I think you are right that nginx provides no meaningful feedback here, while it should. As of now, nginx only complains about invalid connection preface in debug logs, and this isn't visible to administrator in most cases. The following patch changes invalid connection preface errors to appear at the info level, much like other client-related errors.

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1589828055 -10800
#      Mon May 18 21:54:15 2020 +0300
# Node ID 7a2c462272d17e399102daaf8c9c1526f49e8486
# Parent  3c8082c3f98ab7fdec8a598b55609701452b5254
HTTP/2: invalid connection preface logging (ticket #1981).

Previously, invalid connection preface errors were only logged at debug
level, providing no visible feedback, in particular, when a plain text
HTTP/2 listening socket is erroneously used for HTTP/1.x connections.
Now these are explicitly logged at the info level, much like other
client-related errors.

diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
--- a/src/http/v2/ngx_http_v2.c
+++ b/src/http/v2/ngx_http_v2.c
@@ -731,9 +731,8 @@ ngx_http_v2_state_preface(ngx_http_v2_co
     }
 
     if (ngx_memcmp(pos, preface, sizeof(preface) - 1) != 0) {
-        ngx_log_debug2(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
-                       "invalid http2 connection preface \"%*s\"",
-                       sizeof(preface) - 1, pos);
+        ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
+                      "invalid connection preface");
 
         return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_PROTOCOL_ERROR);
     }
@@ -754,9 +753,8 @@ ngx_http_v2_state_preface_end(ngx_http_v
     }
 
     if (ngx_memcmp(pos, preface, sizeof(preface) - 1) != 0) {
-        ngx_log_debug2(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
-                       "invalid http2 connection preface \"%*s\"",
-                       sizeof(preface) - 1, pos);
+        ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
+                      "invalid connection preface");
 
         return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_PROTOCOL_ERROR);
     }

With the patch, HTTP/1.x connections to a plain text HTTP/2 listening socket will result in the following errors in the error logs:

2020/05/18 21:45:44 [info] 85603#100101: *1 invalid connection preface while processing HTTP/2 connection, client: 127.0.0.1, server: 0.0.0.0:8080

Hopefully this will reduce confusion when accidentally configuring plain text HTTP/2 listening sockets without SSL. Please test if the patch works for you.

comment:2 by Maxim Dounin, 4 months ago

Resolution: fixed
Status: acceptedclosed

Patch committed, 7114d21bc2b1. Thanks for prodding this.

Note: See TracTickets for help on using tickets.