From 250ea53e4b968bfc2a3dd0e301a1878fdfa41efb Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 23 Mar 2015 00:12:27 +0900 Subject: [PATCH] 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 | 11 +--------- examples/tiny-nghttpd.c | 13 +----------- lib/includes/nghttp2/nghttp2.h | 35 +++++++++++++++++++------------- lib/nghttp2_option.c | 7 ++++--- lib/nghttp2_option.h | 6 +++--- lib/nghttp2_session.c | 26 ++++++++++++------------ lib/nghttp2_session.h | 2 +- python/nghttp2.pyx | 35 ++++++++++---------------------- src/HttpServer.cc | 17 ++++++---------- src/HttpServer.h | 1 - src/asio_server_http2_handler.cc | 12 +---------- src/shrpx.cc | 1 + tests/main.c | 4 ++-- tests/nghttp2_session_test.c | 20 ++++++++++-------- 14 files changed, 76 insertions(+), 114 deletions(-) diff --git a/examples/libevent-server.c b/examples/libevent-server.c index 9746f234..ef4c2f7d 100644 --- a/examples/libevent-server.c +++ b/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, - option); + nghttp2_session_server_new(&session_data->session, callbacks, session_data); nghttp2_session_callbacks_del(callbacks); - nghttp2_option_del(option); } /* Send HTTP/2 client connection header, which includes 24 bytes diff --git a/examples/tiny-nghttpd.c b/examples/tiny-nghttpd.c index b25293fe..c586adfb 100644 --- a/examples/tiny-nghttpd.c +++ b/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, - shared_option); + rv = nghttp2_session_server_new(&conn->session, shared_callbacks, conn); 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; diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 98c1d4f0..4b5bfb9b 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/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 - * recognize first 24 bytes of client connection preface. This design - * choice is done due to the fact that server may want to detect the - * application protocol based on first few bytes on clear text - * communication. But for simple servers which only speak HTTP/2, it - * is easier for developers if nghttp2 library takes care of client - * connection preface. + * By default, nghttp2 library, if configured as server, requires + * first 24 bytes of client connection preface. In most cases, this + * will simplify the implementation of server. But sometimes erver + * may want to detect the application protocol based on first few + * bytes on clear text communication. * - * If this option is used with nonzero |val|, nghttp2 library checks - * first 24 bytes client connection preface. If it is not a valid - * one, `nghttp2_session_recv()` and `nghttp2_session_mem_recv()` will - * return error :enum:`NGHTTP2_ERR_BAD_PREFACE`, which is fatal error. + * If this option is used with nonzero |val|, nghttp2 library does not + * handle first 24 bytes client connection preface. It still checks + * following SETTINGS frame. This means that applications should deal + * 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, diff --git a/lib/nghttp2_option.c b/lib/nghttp2_option.c index db2a27aa..cf9bae02 100644 --- a/lib/nghttp2_option.c +++ b/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) { - option->opt_set_mask |= NGHTTP2_OPT_RECV_CLIENT_PREFACE; - option->recv_client_preface = val; +void nghttp2_option_set_no_recv_client_preface(nghttp2_option *option, + int 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) { diff --git a/lib/nghttp2_option.h b/lib/nghttp2_option.h index db94be55..f35339ab 100644 --- a/lib/nghttp2_option.h +++ b/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 */ diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 4af67f30..ba3aa7b0 100644 --- a/lib/nghttp2_session.c +++ b/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) && - option->recv_client_preface) { + if ((option->opt_set_mask & NGHTTP2_OPT_NO_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 && - ((*session_ptr)->opt_flags & NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE)) { - + if (nghttp2_enable_strict_connection_preface_check) { nghttp2_inbound_frame *iframe = &(*session_ptr)->iframe; - iframe->state = NGHTTP2_IB_READ_CLIENT_PREFACE; - iframe->payloadleft = NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN; - } else if (nghttp2_enable_strict_first_settings_check) { - nghttp2_inbound_frame *iframe = &(*session_ptr)->iframe; - - iframe->state = NGHTTP2_IB_READ_FIRST_SETTINGS; + if (server && + ((*session_ptr)->opt_flags & NGHTTP2_OPTMASK_NO_RECV_CLIENT_PREFACE) == + 0) { + iframe->state = NGHTTP2_IB_READ_CLIENT_PREFACE; + iframe->payloadleft = NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN; + } else { + iframe->state = NGHTTP2_IB_READ_FIRST_SETTINGS; + } } return 0; diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index 94f990cc..ea532262 100644 --- a/lib/nghttp2_session.h +++ b/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; diff --git a/python/nghttp2.pyx b/python/nghttp2.pyx index ad1ebb82..10b12759 100644 --- a/python/nghttp2.pyx +++ b/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: diff --git a/src/HttpServer.cc b/src/HttpServer.cc index e8377a6e..1aa2aea7 100644 --- a/src/HttpServer.cc +++ b/src/HttpServer.cc @@ -92,16 +92,12 @@ template void append_nv(Stream *stream, const Array &nva) { } // namespace Config::Config() - : stream_read_timeout(60.), stream_write_timeout(60.), - session_option(nullptr), data_ptr(nullptr), padding(0), num_worker(1), - header_table_size(-1), port(0), verbose(false), daemon(false), - verify_client(false), no_tls(false), error_gzip(false), - early_response(false), hexdump(false) { - nghttp2_option_new(&session_option); - nghttp2_option_set_recv_client_preface(session_option, 1); -} + : stream_read_timeout(60.), stream_write_timeout(60.), data_ptr(nullptr), + padding(0), num_worker(1), header_table_size(-1), port(0), verbose(false), + daemon(false), verify_client(false), no_tls(false), error_gzip(false), + early_response(false), hexdump(false) {} -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, - sessions_->get_config()->session_option); + r = nghttp2_session_server_new(&session_, sessions_->get_callbacks(), this); if (r != 0) { return r; } diff --git a/src/HttpServer.h b/src/HttpServer.h index 62d50a5d..f28a5282 100644 --- a/src/HttpServer.h +++ b/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; diff --git a/src/asio_server_http2_handler.cc b/src/asio_server_http2_handler.cc index 7e2304ae..c55cb329 100644 --- a/src/asio_server_http2_handler.cc +++ b/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_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); + rv = nghttp2_session_server_new(&session_, callbacks, this); if (rv != 0) { return -1; } diff --git a/src/shrpx.cc b/src/shrpx.cc index 86e4a637..4b64ce39 100644 --- a/src/shrpx.cc +++ b/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, diff --git a/tests/main.c b/tests/main.c index 09a92124..9b9e8e63 100644 --- a/tests/main.c +++ b/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()) diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 1a4589c9..ef94e3d7 100644 --- a/tests/nghttp2_session_test.c +++ b/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); - - CU_ASSERT(session->opt_flags & NGHTTP2_OPTMASK_RECV_CLIENT_PREFACE); + nghttp2_session_server_new(&session, &callbacks, NULL); 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) {