Deal with 24 bytes client connection preface by default

Since HTTP/2 spec requires for client to send connection preface, it
is reasonable to make this option enabled by default.  It is still a
use case to disable this, so replace this option with
nghttp2_option_set_no_recv_client_preface().

examples/libevent-server.c

@@ -537,15 +537,8 @@ static int on_stream_close_callback(nghttp2_session *session, int32_t stream_id,
 }
 
 static void initialize_nghttp2_session(http2_session_data *session_data) {
-  nghttp2_option *option;
   nghttp2_session_callbacks *callbacks;
 
-  nghttp2_option_new(&option);
-
-  /* Tells nghttp2_session object that it handles client connection
-     preface */
-  nghttp2_option_set_recv_client_preface(option, 1);
-
   nghttp2_session_callbacks_new(&callbacks);
 
   nghttp2_session_callbacks_set_send_callback(callbacks, send_callback);
@@ -562,11 +555,9 @@ static void initialize_nghttp2_session(http2_session_data *session_data) {
   nghttp2_session_callbacks_set_on_begin_headers_callback(
       callbacks, on_begin_headers_callback);
 
-  nghttp2_session_server_new2(&session_data->session, callbacks, session_data,
+  nghttp2_session_server_new(&session_data->session, callbacks, session_data);
-                              option);
 
   nghttp2_session_callbacks_del(callbacks);
-  nghttp2_option_del(option);
 }
 
 /* Send HTTP/2 client connection header, which includes 24 bytes

examples/tiny-nghttpd.c

@@ -153,7 +153,6 @@ const char *docroot;
 size_t docrootlen;
 
 nghttp2_session_callbacks *shared_callbacks;
-nghttp2_option *shared_option;
 
 static int handle_accept(io_loop *loop, uint32_t events, void *ptr);
 static int handle_connection(io_loop *loop, uint32_t events, void *ptr);
@@ -388,8 +387,7 @@ static connection *connection_new(int fd) {
 
   conn = malloc(sizeof(connection));
 
-  rv = nghttp2_session_server_new2(&conn->session, shared_callbacks, conn,
+  rv = nghttp2_session_server_new(&conn->session, shared_callbacks, conn);
-                                   shared_option);
 
   if (rv != 0) {
     goto cleanup;
@@ -1309,14 +1307,6 @@ int main(int argc, char **argv) {
   nghttp2_session_callbacks_set_send_data_callback(shared_callbacks,
                                                    send_data_callback);
 
-  rv = nghttp2_option_new(&shared_option);
-  if (rv != 0) {
-    fprintf(stderr, "nghttp2_option_new: %s", nghttp2_strerror(rv));
-    exit(EXIT_FAILURE);
-  }
-
-  nghttp2_option_set_recv_client_preface(shared_option, 1);
-
   rv = io_loop_add(&loop, serv.fd, EPOLLIN, &serv);
 
   if (rv != 0) {
@@ -1333,7 +1323,6 @@ int main(int argc, char **argv) {
 
   io_loop_run(&loop, &serv);
 
-  nghttp2_option_del(shared_option);
   nghttp2_session_callbacks_del(shared_callbacks);
 
   return 0;

lib/includes/nghttp2/nghttp2.h

@@ -1962,21 +1962,26 @@ nghttp2_option_set_peer_max_concurrent_streams(nghttp2_option *option,
 /**
  * @function
  *
- * By default, nghttp2 library only handles HTTP/2 frames and does not
+ * By default, nghttp2 library, if configured as server, requires
- * recognize first 24 bytes of client connection preface.  This design
+ * first 24 bytes of client connection preface.  In most cases, this
- * choice is done due to the fact that server may want to detect the
+ * will simplify the implementation of server.  But sometimes erver
- * application protocol based on first few bytes on clear text
+ * may want to detect the application protocol based on first few
- * communication.  But for simple servers which only speak HTTP/2, it
+ * bytes on clear text communication.
- * is easier for developers if nghttp2 library takes care of client
- * connection preface.
  *
- * If this option is used with nonzero |val|, nghttp2 library checks
+ * If this option is used with nonzero |val|, nghttp2 library does not
- * first 24 bytes client connection preface.  If it is not a valid
+ * handle first 24 bytes client connection preface.  It still checks
- * one, `nghttp2_session_recv()` and `nghttp2_session_mem_recv()` will
+ * following SETTINGS frame.  This means that applications should deal
- * return error :enum:`NGHTTP2_ERR_BAD_PREFACE`, which is fatal error.
+ * with 24 bytes connection preface by themselves.
+ *
+ * If this option is not used or used with zero value, if client
+ * connection preface does not match the one
+ * (:macro:`NGHTTP2_CLIENT_CONNECTION_PREFACE`) in the HTTP/2
+ * specification, `nghttp2_session_recv()` and
+ * `nghttp2_session_mem_recv()` will return error
+ * :enum:`NGHTTP2_ERR_BAD_PREFACE`, which is fatal error.
  */
 NGHTTP2_EXTERN void
-nghttp2_option_set_recv_client_preface(nghttp2_option *option, int val);
+nghttp2_option_set_no_recv_client_preface(nghttp2_option *option, int val);
 
 /**
  * @function
@@ -2296,7 +2301,8 @@ NGHTTP2_EXTERN ssize_t nghttp2_session_mem_send(nghttp2_session *session,
  * :enum:`NGHTTP2_ERR_BAD_PREFACE`
  *     Invalid client preface was detected.  This error only returns
  *     when |session| was configured as server and
- *     `nghttp2_option_set_recv_client_preface()` is used.
+ *     `nghttp2_option_set_no_recv_client_preface()` is not used with
+ *     nonzero value.
  */
 NGHTTP2_EXTERN int nghttp2_session_recv(nghttp2_session *session);
 
@@ -2331,7 +2337,8 @@ NGHTTP2_EXTERN int nghttp2_session_recv(nghttp2_session *session);
  * :enum:`NGHTTP2_ERR_BAD_PREFACE`
  *     Invalid client preface was detected.  This error only returns
  *     when |session| was configured as server and
- *     `nghttp2_option_set_recv_client_preface()` is used.
+ *     `nghttp2_option_set_no_recv_client_preface()` is not used with
+ *     nonzero value.
  */
 NGHTTP2_EXTERN ssize_t nghttp2_session_mem_recv(nghttp2_session *session,
                                                 const uint8_t *in,

lib/nghttp2_option.c

@@ -47,9 +47,10 @@ void nghttp2_option_set_peer_max_concurrent_streams(nghttp2_option *option,
   option->peer_max_concurrent_streams = val;
 }
 
-void nghttp2_option_set_recv_client_preface(nghttp2_option *option, int val) {
+void nghttp2_option_set_no_recv_client_preface(nghttp2_option *option,
-  option->opt_set_mask |= NGHTTP2_OPT_RECV_CLIENT_PREFACE;
+                                               int val) {
-  option->recv_client_preface = val;
+  option->opt_set_mask |= NGHTTP2_OPT_NO_RECV_CLIENT_PREFACE;
+  option->no_recv_client_preface = val;
 }
 
 void nghttp2_option_set_no_http_messaging(nghttp2_option *option, int val) {

lib/nghttp2_option.h

@@ -57,7 +57,7 @@ typedef enum {
    * SETTINGS_MAX_CONCURRENT_STREAMS from the remote endpoint.
    */
   NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS = 1 << 1,
-  NGHTTP2_OPT_RECV_CLIENT_PREFACE = 1 << 2,
+  NGHTTP2_OPT_NO_RECV_CLIENT_PREFACE = 1 << 2,
   NGHTTP2_OPT_NO_HTTP_MESSAGING = 1 << 3,
 } nghttp2_option_flag;
 
@@ -79,9 +79,9 @@ struct nghttp2_option {
    */
   uint8_t no_auto_window_update;
   /**
-   * NGHTTP2_OPT_RECV_CLIENT_PREFACE
+   * NGHTTP2_OPT_NO_RECV_CLIENT_PREFACE
    */
-  uint8_t recv_client_preface;
+  uint8_t no_recv_client_preface;
   /**
    * NGHTTP2_OPT_NO_HTTP_MESSAGING
    */

lib/nghttp2_session.c

@@ -314,7 +314,7 @@ static void active_outbound_item_reset(nghttp2_active_outbound_item *aob,
 
 /* This global variable exists for tests where we want to disable this
    check. */
-int nghttp2_enable_strict_first_settings_check = 1;
+int nghttp2_enable_strict_connection_preface_check = 1;
 
 static int session_new(nghttp2_session **session_ptr,
                        const nghttp2_session_callbacks *callbacks,
@@ -415,10 +415,10 @@ static int session_new(nghttp2_session **session_ptr,
           option->peer_max_concurrent_streams;
     }
 
-    if ((option->opt_set_mask & NGHTTP2_OPT_RECV_CLIENT_PREFACE) &&
+    if ((option->opt_set_mask & NGHTTP2_OPT_NO_RECV_CLIENT_PREFACE) &&
-        option->recv_client_preface) {
+        option->no_recv_client_preface) {
 
-      (*session_ptr)->opt_flags |= NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE;
+      (*session_ptr)->opt_flags |= NGHTTP2_OPTMASK_NO_RECV_CLIENT_PREFACE;
     }
 
     if ((option->opt_set_mask & NGHTTP2_OPT_NO_HTTP_MESSAGING) &&
@@ -433,17 +433,17 @@ static int session_new(nghttp2_session **session_ptr,
 
   session_inbound_frame_reset(*session_ptr);
 
-  if (server &&
+  if (nghttp2_enable_strict_connection_preface_check) {
-      ((*session_ptr)->opt_flags & NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE)) {
-
     nghttp2_inbound_frame *iframe = &(*session_ptr)->iframe;
 
-    iframe->state = NGHTTP2_IB_READ_CLIENT_PREFACE;
+    if (server &&
-    iframe->payloadleft = NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN;
+        ((*session_ptr)->opt_flags & NGHTTP2_OPTMASK_NO_RECV_CLIENT_PREFACE) ==
-  } else if (nghttp2_enable_strict_first_settings_check) {
+            0) {
-    nghttp2_inbound_frame *iframe = &(*session_ptr)->iframe;
+      iframe->state = NGHTTP2_IB_READ_CLIENT_PREFACE;
-
+      iframe->payloadleft = NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN;
-    iframe->state = NGHTTP2_IB_READ_FIRST_SETTINGS;
+    } else {
+      iframe->state = NGHTTP2_IB_READ_FIRST_SETTINGS;
+    }
   }
 
   return 0;

lib/nghttp2_session.h

@@ -46,7 +46,7 @@
  */
 typedef enum {
   NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE = 1 << 0,
-  NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE = 1 << 1,
+  NGHTTP2_OPTMASK_NO_RECV_CLIENT_PREFACE = 1 << 1,
   NGHTTP2_OPTMASK_NO_HTTP_MESSAGING = 1 << 2,
 } nghttp2_optmask;
 

python/nghttp2.pyx

@@ -1237,7 +1237,6 @@ if asyncio:
             logging.info('connection_made, address:%s, port:%s', address[0], address[1])
 
             self.transport = transport
-            self.connection_header = cnghttp2.NGHTTP2_CLIENT_CONNECTION_PREFACE
             sock = self.transport.get_extra_info('socket')
             sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
             ssl_ctx = self.transport.get_extra_info('sslcontext')
@@ -1247,6 +1246,16 @@ if asyncio:
                 if protocol.encode('utf-8') != \
                    cnghttp2.NGHTTP2_PROTO_VERSION_ID:
                     self.transport.abort()
+                    return
+            try:
+                self.http2 = _HTTP2SessionCore\
+                             (self.transport,
+                              self.RequestHandlerClass)
+            except Exception as err:
+                sys.stderr.write(traceback.format_exc())
+                self.transport.abort()
+                return
+
 
         def connection_lost(self, exc):
             logging.info('connection_lost')
@@ -1254,28 +1263,6 @@ if asyncio:
                 self.http2 = None
 
         def data_received(self, data):
-            nread = min(len(data), len(self.connection_header))
-
-            if self.connection_header.startswith(data[:nread]):
-                data = data[nread:]
-                self.connection_header = self.connection_header[nread:]
-                if len(self.connection_header) == 0:
-                    try:
-                        self.http2 = _HTTP2SessionCore\
-                                     (self.transport,
-                                      self.RequestHandlerClass)
-                    except Exception as err:
-                        sys.stderr.write(traceback.format_exc())
-                        self.transport.abort()
-                        return
-
-                    self.data_received = self.data_received2
-                    self.resume_writing = self.resume_writing2
-                    self.data_received(data)
-            else:
-                self.transport.abort()
-
-        def data_received2(self, data):
             try:
                 self.http2.data_received(data)
             except Exception as err:
@@ -1283,7 +1270,7 @@ if asyncio:
                 self.transport.close()
                 return
 
-        def resume_writing2(self):
+        def resume_writing(self):
             try:
                 self.http2.send_data()
             except Exception as err:

src/HttpServer.cc

@@ -92,16 +92,12 @@ template <typename Array> void append_nv(Stream *stream, const Array &nva) {
 } // namespace
 
 Config::Config()
-    : stream_read_timeout(60.), stream_write_timeout(60.),
+    : stream_read_timeout(60.), stream_write_timeout(60.), data_ptr(nullptr),
-      session_option(nullptr), data_ptr(nullptr), padding(0), num_worker(1),
+      padding(0), num_worker(1), header_table_size(-1), port(0), verbose(false),
-      header_table_size(-1), port(0), verbose(false), daemon(false),
+      daemon(false), verify_client(false), no_tls(false), error_gzip(false),
-      verify_client(false), no_tls(false), error_gzip(false),
+      early_response(false), hexdump(false) {}
-      early_response(false), hexdump(false) {
-  nghttp2_option_new(&session_option);
-  nghttp2_option_set_recv_client_preface(session_option, 1);
-}
 
-Config::~Config() { nghttp2_option_del(session_option); }
+Config::~Config() {}
 
 namespace {
 void stream_timeout_cb(struct ev_loop *loop, ev_timer *w, int revents) {
@@ -634,8 +630,7 @@ int Http2Handler::on_write() { return write_(*this); }
 int Http2Handler::connection_made() {
   int r;
 
-  r = nghttp2_session_server_new2(&session_, sessions_->get_callbacks(), this,
+  r = nghttp2_session_server_new(&session_, sessions_->get_callbacks(), this);
-                                  sessions_->get_config()->session_option);
   if (r != 0) {
     return r;
   }

src/HttpServer.h

@@ -59,7 +59,6 @@ struct Config {
   std::string address;
   ev_tstamp stream_read_timeout;
   ev_tstamp stream_write_timeout;
-  nghttp2_option *session_option;
   void *data_ptr;
   size_t padding;
   size_t num_worker;

src/asio_server_http2_handler.cc

@@ -268,17 +268,7 @@ int http2_handler::start() {
   nghttp2_session_callbacks_set_on_frame_not_send_callback(
       callbacks, on_frame_not_send_callback);
 
-  nghttp2_option *option;
+  rv = nghttp2_session_server_new(&session_, callbacks, this);
-  rv = nghttp2_option_new(&option);
-  if (rv != 0) {
-    return -1;
-  }
-
-  auto opt_del = defer(nghttp2_option_del, option);
-
-  nghttp2_option_set_recv_client_preface(option, 1);
-
-  rv = nghttp2_session_server_new2(&session_, callbacks, this, option);
   if (rv != 0) {
     return -1;
   }

src/shrpx.cc

@@ -904,6 +904,7 @@ void fill_default_config() {
 
   nghttp2_option_new(&mod_config()->http2_option);
   nghttp2_option_set_no_auto_window_update(get_config()->http2_option, 1);
+  nghttp2_option_set_no_recv_client_preface(get_config()->http2_option, 1);
 
   nghttp2_option_new(&mod_config()->http2_client_option);
   nghttp2_option_set_no_auto_window_update(get_config()->http2_client_option,

tests/main.c

@@ -41,7 +41,7 @@
 #include "nghttp2_helper_test.h"
 #include "nghttp2_buf_test.h"
 
-extern int nghttp2_enable_strict_first_settings_check;
+extern int nghttp2_enable_strict_connection_preface_check;
 
 static int init_suite1(void) { return 0; }
 
@@ -51,7 +51,7 @@ int main(int argc _U_, char *argv[] _U_) {
   CU_pSuite pSuite = NULL;
   unsigned int num_tests_failed;
 
-  nghttp2_enable_strict_first_settings_check = 0;
+  nghttp2_enable_strict_connection_preface_check = 0;
 
   /* initialize the CUnit test registry */
   if (CUE_SUCCESS != CU_initialize_registry())

tests/nghttp2_session_test.c

@@ -36,6 +36,8 @@
 #include "nghttp2_test_helper.h"
 #include "nghttp2_priority_spec.h"
 
+extern int nghttp2_enable_strict_connection_preface_check;
+
 #define OB_CTRL(ITEM) nghttp2_outbound_item_get_ctrl_frame(ITEM)
 #define OB_CTRL_TYPE(ITEM) nghttp2_outbound_item_get_ctrl_frame_type(ITEM)
 #define OB_DATA(ITEM) nghttp2_outbound_item_get_data_frame(ITEM)
@@ -6723,20 +6725,18 @@ void test_nghttp2_session_on_header_temporal_failure(void) {
 void test_nghttp2_session_recv_client_preface(void) {
   nghttp2_session *session;
   nghttp2_session_callbacks callbacks;
-  nghttp2_option *option;
   ssize_t rv;
   nghttp2_frame ping_frame;
   uint8_t buf[16];
 
+  /* enable global nghttp2_enable_strict_connection_preface_check
+     here */
+  nghttp2_enable_strict_connection_preface_check = 1;
+
   memset(&callbacks, 0, sizeof(callbacks));
 
-  nghttp2_option_new(&option);
-  nghttp2_option_set_recv_client_preface(option, 1);
-
   /* Check success case */
-  nghttp2_session_server_new2(&session, &callbacks, NULL, option);
+  nghttp2_session_server_new(&session, &callbacks, NULL);
-
-  CU_ASSERT(session->opt_flags & NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE);
 
   rv = nghttp2_session_mem_recv(
       session, (const uint8_t *)NGHTTP2_CLIENT_CONNECTION_PREFACE,
@@ -6760,7 +6760,7 @@ void test_nghttp2_session_recv_client_preface(void) {
   nghttp2_session_del(session);
 
   /* Check bad case */
-  nghttp2_session_server_new2(&session, &callbacks, NULL, option);
+  nghttp2_session_server_new(&session, &callbacks, NULL);
 
   /* Feed preface with one byte less */
   rv = nghttp2_session_mem_recv(
@@ -6777,7 +6777,9 @@ void test_nghttp2_session_recv_client_preface(void) {
 
   nghttp2_session_del(session);
 
-  nghttp2_option_del(option);
+  /* disable global nghttp2_enable_strict_connection_preface_check
+     here */
+  nghttp2_enable_strict_connection_preface_check = 0;
 }
 
 void test_nghttp2_session_delete_data_item(void) {