mod_http2: Send HTTP/2 error codes in GOAWAY frames

h2_session_shutdown() passed its reason, an APR or nghttp2 status, as the
error code of the GOAWAY frame. A connection timeout was sent as 70007
(APR_TIMEUP), an inadequate TLS connection as 22 (APR_EINVAL).

Give h2_session_shutdown() the HTTP/2 error code as a separate argument.
Timeouts and graceful shutdowns send NO_ERROR, failures INTERNAL_ERROR
and inadequate TLS parameters INADEQUATE_SECURITY.

PR: 70212
GitHub: closes #799


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1939156 13f79535-47bb-0310-9956-ffa450edef68

Author: arturobernalg

changes-entries/h2-goaway-error-code.txt

@@ -0,0 +1,3 @@
+  *) mod_http2: Send only HTTP/2 error codes in GOAWAY frames. The APR status
+     of a connection timeout or error was sent instead.
+     PR 70212.  [Arturo Bernal]

modules/http2/h2_session.c

@@ -62,7 +62,16 @@ static void on_stream_state_enter(void *ctx, h2_stream *stream);
 static void on_stream_state_event(void *ctx, h2_stream *stream, h2_stream_event_t ev);
 static void on_stream_event(void *ctx, h2_stream *stream, h2_stream_event_t ev);
 static apr_status_t h2_session_shutdown(h2_session *session, int error,
-                                        const char *msg, int force_close);
+                                        uint32_t h2_error, const char *msg,
+                                        int force_close);
+
+/* The error code of a GOAWAY frame is one of RFC 9113, section 7. The
+ * reasons for a shutdown are APR or nghttp2 status values that must not
+ * end up on the wire. */
+static uint32_t h2_error_from_status(int status)
+{
+    return status? H2_ERR_INTERNAL_ERROR : H2_ERR_NO_ERROR;
+}
 
 static int h2_session_status_from_apr_status(apr_status_t rv)
 {
@@ -789,7 +798,8 @@ static apr_status_t h2_session_shutdown_notice(h2_session *session)
 }
 
 static apr_status_t h2_session_shutdown(h2_session *session, int error, 
-                                        const char *msg, int force_close)
+                                        uint32_t h2_error, const char *msg,
+                                        int force_close)
 {
     apr_status_t status = APR_SUCCESS;
 
@@ -827,14 +837,15 @@ static apr_status_t h2_session_shutdown(h2_session *session, int error,
     if (!session->c1->aborted) {
         nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, 
                               session->local.accepted_max, 
-                              error, (uint8_t*)msg, msg? strlen(msg):0);
+                              h2_error, (uint8_t*)msg, msg? strlen(msg):0);
         status = nghttp2_session_send(session->ngh2);
         if (status == APR_SUCCESS) {
             status = h2_c1_io_assure_flushed(&session->io);
         }
         ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c1,
                       H2_SSSN_LOG(APLOGNO(03069), session, 
-                                  "sent GOAWAY, err=%d, msg=%s"), error, msg? msg : "");
+                                  "sent GOAWAY, err=%u, status=%d, msg=%s"),
+                      (unsigned int)h2_error, error, msg? msg : "");
     }
     h2_session_dispatch_event(session, H2_SESSION_EV_LOCAL_GOAWAY, error, msg);
     return status;
@@ -1592,7 +1603,7 @@ static void h2_session_ev_conn_error(h2_session *session, int arg, const char *m
                           H2_SSSN_LOG(APLOGNO(03401), session, 
                           "conn error -> shutdown, remote.emitted=%d"),
                           (int)session->remote.emitted_count);
-            h2_session_shutdown(session, arg, msg, 0);
+            h2_session_shutdown(session, arg, h2_error_from_status(arg), msg, 0);
             break;
     }
 }
@@ -1603,15 +1614,15 @@ static void h2_session_ev_proto_error(h2_session *session, int arg, const char *
         ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c1,
                       H2_SSSN_LOG(APLOGNO(03402), session, 
                       "proto error -> shutdown"));
-        h2_session_shutdown(session, arg, msg, 0);
+        h2_session_shutdown(session, arg, h2_error_from_status(arg), msg, 0);
     }
 }
 
 static void h2_session_ev_conn_timeout(h2_session *session, int arg, const char *msg)
 {
     transit(session, msg, H2_SESSION_ST_DONE);
     if (!session->local.shutdown) {
-        h2_session_shutdown(session, arg, msg, 1);
+        h2_session_shutdown(session, arg, H2_ERR_NO_ERROR, msg, 1);
     }
 }
 
@@ -1646,13 +1657,13 @@ static void h2_session_ev_bad_client(h2_session *session, int arg, const char *m
 {
     transit(session, msg, H2_SESSION_ST_DONE);
     if (!session->local.shutdown) {
-        h2_session_shutdown(session, arg, msg, 1);
+        h2_session_shutdown(session, arg, (uint32_t)arg, msg, 1);
     }
 }
 
 static void h2_session_ev_pre_close(h2_session *session, int arg, const char *msg)
 {
-    h2_session_shutdown(session, arg, msg, 1);
+    h2_session_shutdown(session, arg, H2_ERR_NO_ERROR, msg, 1);
 }
 
 static void h2_session_ev_no_more_streams(h2_session *session)
@@ -1670,7 +1681,7 @@ static void h2_session_ev_no_more_streams(h2_session *session)
                 else {
                     /* We are no longer accepting new streams.
                      * Time to leave. */
-                    h2_session_shutdown(session, 0, "done", 0);
+                    h2_session_shutdown(session, 0, H2_ERR_NO_ERROR, "done", 0);
                     transit(session, "c1 done after goaway", H2_SESSION_ST_DONE);
                 }
             }
@@ -1895,7 +1906,8 @@ apr_status_t h2_session_process(h2_session *session, int async,
         if (!h2_protocol_is_acceptable_c1(c, session->r, 1)) {
             const char *msg = nghttp2_strerror(NGHTTP2_INADEQUATE_SECURITY);
             update_child_status(session, SERVER_BUSY_READ, msg, NULL);
-            h2_session_shutdown(session, APR_EINVAL, msg, 1);
+            h2_session_shutdown(session, APR_EINVAL, H2_ERR_INADEQUATE_SECURITY,
+                                msg, 1);
         }
         else {
             update_child_status(session, SERVER_BUSY_READ, "init", NULL);
@@ -1999,7 +2011,7 @@ apr_status_t h2_session_process(h2_session *session, int async,
                  * (Checked before the want_read assert below: after receiving a
                  * GOAWAY nghttp2 may no longer want to read.) */
                 if (!session->local.shutdown) {
-                    h2_session_shutdown(session, 0, "done", 0);
+                    h2_session_shutdown(session, 0, H2_ERR_NO_ERROR, "done", 0);
                 }
                 transit(session, "remote goaway, streams drained",
                         H2_SESSION_ST_DONE);

test/modules/http2/test_108_goaway.py

@@ -0,0 +1,97 @@
+#
+# mod-h2 test suite
+# check the error code of GOAWAY frames sent by the server
+#
+import socket
+import ssl
+import struct
+
+import pytest
+
+from .env import H2Conf, H2TestEnv
+
+
+@pytest.mark.skipif(condition=H2TestEnv.is_unsupported(), reason="mod_http2 not supported here")
+class TestGoaway:
+
+    PREFACE = b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'
+    FRAME_SETTINGS = 0x04
+    FRAME_GOAWAY = 0x07
+    # error codes of RFC 9113, section 7
+    ERR_NO_ERROR = 0x00
+    ERR_INADEQUATE_SECURITY = 0x0c
+    ERR_HTTP_1_1_REQUIRED = 0x0d
+
+    @pytest.fixture(autouse=True, scope='class')
+    def _class_scope(self, env):
+        conf = H2Conf(env, extras={
+            'base': [
+                "AcceptFilter http none",
+                "Timeout 1",
+                "KeepAliveTimeout 1",
+                "SSLCipherSuite HIGH:!aNULL",
+            ]
+        })
+        conf.add_vhost_cgi()
+        conf.install()
+        assert env.apache_restart() == 0
+
+    @staticmethod
+    def _frame(ftype, payload=b''):
+        # length, type, flags and the stream id 0
+        return len(payload).to_bytes(3, 'big') + bytes([ftype, 0]) + bytes(4) + payload
+
+    @staticmethod
+    def _recv_exact(sock, n):
+        data = b''
+        while len(data) < n:
+            chunk = sock.recv(n - len(data))
+            if not chunk:
+                break
+            data += chunk
+        return data
+
+    def _goaway_error(self, env, ctx):
+        """Open a connection, send the client preface and return the error
+        code of the first GOAWAY frame the server sends, None if there is none."""
+        with socket.create_connection(('localhost', int(env.https_port)), timeout=10) as raw:
+            with ctx.wrap_socket(raw, server_hostname='localhost') as sock:
+                assert sock.selected_alpn_protocol() == 'h2'
+                sock.sendall(self.PREFACE + self._frame(self.FRAME_SETTINGS))
+                while True:
+                    head = self._recv_exact(sock, 9)
+                    if len(head) < 9:
+                        return None
+                    length = int.from_bytes(head[0:3], 'big')
+                    payload = self._recv_exact(sock, length)
+                    if head[3] == self.FRAME_GOAWAY:
+                        return struct.unpack('>II', payload[:8])[1]
+
+    @staticmethod
+    def _tls_context(ciphers=None, tls12_only=False):
+        ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
+        ctx.check_hostname = False
+        ctx.verify_mode = ssl.CERT_NONE
+        ctx.set_alpn_protocols(['h2'])
+        if tls12_only:
+            ctx.maximum_version = ssl.TLSVersion.TLSv1_2
+        if ciphers:
+            ctx.set_ciphers(ciphers)
+        return ctx
+
+    # An idle connection that runs into the server timeout is closed
+    # gracefully. The GOAWAY must carry NO_ERROR and not the APR status
+    # of the timeout.
+    def test_h2_108_01_idle_timeout(self, env):
+        error = self._goaway_error(env, self._tls_context())
+        assert error == self.ERR_NO_ERROR, f"GOAWAY error code {error}"
+
+    # A TLS connection that is not acceptable for HTTP/2 is refused with
+    # INADEQUATE_SECURITY (RFC 9113, section 9.2.1).
+    def test_h2_108_02_inadequate_security(self, env):
+        try:
+            ctx = self._tls_context(ciphers='AES128-SHA256', tls12_only=True)
+            error = self._goaway_error(env, ctx)
+        except ssl.SSLError as ex:
+            pytest.skip(f"cipher not available: {ex}")
+        assert error == self.ERR_INADEQUATE_SECURITY, f"GOAWAY error code {error}"