diff --git a/CHANGELOG.md b/CHANGELOG.md index dde41a15..c4b5c37d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,10 @@ Developers may need to change their code ### Corrected Bugs +* Native RESTCONF fixes for http/1 or http/2 only modes + * Memleak in http/1-only + * Exit if http/1 request sent to http/2-only (bad client magic) + * Hang if http/1 TLS request sent to http/2 only (alpn accepted http/1.1) * Fixed: [RESTConf GET for a specific list instance retrieves data from other submodules that have same list name and key value #244](https://github.com/clicon/clixon/issues/244) ## 5.2.0 diff --git a/apps/restconf/restconf_evhtp.c b/apps/restconf/restconf_evhtp.c index 49245afd..dfb43597 100644 --- a/apps/restconf/restconf_evhtp.c +++ b/apps/restconf/restconf_evhtp.c @@ -515,6 +515,8 @@ restconf_path_root(evhtp_request_t *req, if (clicon_debug_get()) evhtp_headers_for_each(req->headers_in, evhtp_print_header, h); /* Query vector, ie the ?a=x&b=y stuff */ + if (sd->sd_qvec) + cvec_free(sd->sd_qvec); if ((sd->sd_qvec = cvec_new(0)) ==NULL){ clicon_err(OE_UNIX, errno, "cvec_new"); evhtp_internal_error(req); diff --git a/apps/restconf/restconf_main_native.c b/apps/restconf/restconf_main_native.c index 631701c1..29e65b7d 100644 --- a/apps/restconf/restconf_main_native.c +++ b/apps/restconf/restconf_main_native.c @@ -467,14 +467,16 @@ alpn_select_proto_cb(SSL *ssl, inp++; if (clicon_debug_get()) /* debug print the protoocol */ alpn_proto_dump(__FUNCTION__, (const char*)inp, len); +#ifdef HAVE_LIBEVHTP if (pref < 10 && len == 8 && strncmp((char*)inp, "http/1.1", len) == 0){ *outlen = len; *out = inp; pref = 10; } +#endif #ifdef HAVE_LIBNGHTTP2 /* Higher pref than http/1.1 */ - else if (pref < 20 && len == 2 && strncmp((char*)inp, "h2", len) == 0){ + if (pref < 20 && len == 2 && strncmp((char*)inp, "h2", len) == 0){ *outlen = len; *out = inp; pref = 20; @@ -684,7 +686,9 @@ restconf_connection(int s, char buf[BUFSIZ]; /* from stdio.h, typically 8K XXX: reduce for test */ int readmore = 1; int sslerr; - +#ifdef HAVE_LIBNGHTTP2 + int ret; +#endif #ifdef HAVE_LIBEVHTP clicon_handle h; evhtp_connection_t *evconn = NULL; @@ -889,9 +893,14 @@ restconf_connection(int s, #endif /* HAVE_LIBEVHTP */ #ifdef HAVE_LIBNGHTTP2 case HTTP_2: - if (http2_recv(rc, (unsigned char *)buf, n) < 0) + if ((ret = http2_recv(rc, (unsigned char *)buf, n)) < 0) goto done; - //notused sd = restconf_stream_find(rc, 0); /* default stream */ + if (ret == 0){ + restconf_close_ssl_socket(rc, 1); + if (restconf_conn_free(rc) < 0) + goto done; + goto ok; + } /* There may be more data frames */ readmore++; break; @@ -1011,7 +1020,7 @@ ssl_alpn_check(clicon_handle h, } if (alpn != NULL){ cprintf(cberr, "protocolmalformed-messageALPN: protocol not recognized: %s", alpn); - clicon_log(LOG_NOTICE, "Warning: %s", cbuf_get(cberr)); + clicon_log(LOG_INFO, "%s Warning: %s", __FUNCTION__, cbuf_get(cberr)); if (send_badrequest(h, rc->rc_s, rc->rc_ssl, "application/yang-data+xml", cbuf_get(cberr)) < 0) @@ -1019,7 +1028,7 @@ ssl_alpn_check(clicon_handle h, } else{ /* XXX Sending badrequest here gives a segv in SSL_shutdown() later or a SIGPIPE here */ - clicon_log(LOG_NOTICE, "Warning: ALPN: No protocol selected"); + clicon_log(LOG_INFO, "%s Warning: ALPN: No protocol selected", __FUNCTION__); } if (rc->rc_ssl){ @@ -1029,7 +1038,7 @@ ssl_alpn_check(clicon_handle h, if ((ret = SSL_shutdown(rc->rc_ssl)) < 0){ int e = SSL_get_error(rc->rc_ssl, ret); if (e == SSL_ERROR_SYSCALL){ - clicon_log(LOG_NOTICE, "Warning: SSL_shutdown SSL_ERROR_SYSCALL"); + clicon_log(LOG_INFO, "%s Warning: SSL_shutdown SSL_ERROR_SYSCALL", __FUNCTION__); /* Continue */ } else { @@ -1577,7 +1586,7 @@ restconf_openssl_init(clicon_handle h, } int status = setrlimit(RLIMIT_CORE, &rlp); if (status != 0) { - clicon_log(LOG_NOTICE, "%s: setrlimit() failed, %s", __func__, strerror(errno)); + clicon_log(LOG_INFO, "%s: setrlimit() failed, %s", __FUNCTION__, strerror(errno)); } } diff --git a/apps/restconf/restconf_nghttp2.c b/apps/restconf/restconf_nghttp2.c index 698e1a6e..4d23e67f 100644 --- a/apps/restconf/restconf_nghttp2.c +++ b/apps/restconf/restconf_nghttp2.c @@ -865,8 +865,14 @@ error_callback2(nghttp2_session *session, } #endif -/* - * XXX see session_recv +/*! Process an HTTP/2 request received in buffer, process request and send reply + * + * @param[in] rc Restconf connection + * @param[in] buf Character buffer + * @param[in] n Lenght of buf + * @retval 1 OK + * @retval 0 Invald request + * @retval -1 Fatal error */ int http2_recv(restconf_conn *rc, @@ -884,6 +890,18 @@ http2_recv(restconf_conn *rc, } /* may make additional pending frames */ if ((ngerr = nghttp2_session_mem_recv(rc->rc_ngsession, buf, n)) < 0){ + if (ngerr == NGHTTP2_ERR_BAD_CLIENT_MAGIC){ + /* :enum:`NGHTTP2_ERR_BAD_CLIENT_MAGIC` + * Invalid client magic was detected. This error only returns + * when |session| was configured as server and + * `nghttp2_option_set_no_recv_client_magic()` is not used with + * nonzero value. */ + clicon_log(LOG_INFO, "%s Received bad client magic byte strin", __FUNCTION__); + /* unsure if this does anything, byt does not seem to hurt */ + if ((ngerr = nghttp2_session_terminate_session(rc->rc_ngsession, ngerr)) < 0) + clicon_err(OE_NGHTTP2, ngerr, "nghttp2_session_terminate_session %d", ngerr); + goto fail; + } clicon_err(OE_NGHTTP2, ngerr, "nghttp2_session_mem_recv"); goto done; } @@ -895,9 +913,13 @@ http2_recv(restconf_conn *rc, clicon_err(OE_NGHTTP2, ngerr, "nghttp2_session_send"); goto done; } - retval = 0; + retval = 1; /* OK */ done: + clicon_debug(1, "%s retval:%d", __FUNCTION__, retval); return retval; + fail: + retval = 0; + goto done; } /* Send HTTP/2 client connection header, which includes 24 bytes diff --git a/test/test_restconf.sh b/test/test_restconf.sh index 91df06ca..aea1ca80 100755 --- a/test/test_restconf.sh +++ b/test/test_restconf.sh @@ -172,37 +172,86 @@ function testrun() echo "curl $CURLOPTS -X GET $proto://$addr/.well-known/host-meta" expectpart "$(curl $CURLOPTS -X GET $proto://$addr/.well-known/host-meta)" 0 "HTTP/$HVER 200" "" "" "" - if ! ${HAVE_LIBNGHTTP2}; then # http/2 + if [ ${HAVE_LIBNGHTTP2} = true -a ${HAVE_LIBEVHTP} = false ]; then + echo "native + http/2 only" + # Important here is robustness of restconf daemon, not a meaningful reply + if [ $proto = http ]; then # see (2) https to http port in restconf_main_native.c + # http protocol mismatch can just close the socket if assumed its http/2 + # everything else would guess it is http/1 which is really wrong here + # The tr statement replaces null char to get rid of annoying message: + # ./test_restconf.sh: line 180: warning: command substitution: ignored null byte in input + new "restconf GET http/1.0 - close" + expectpart "$(curl $CURLOPTS --http1.0 -X GET $proto://$addr/.well-known/host-meta | tr '\0' '\n')" 0 "" --not-- 'HTTP' + else + new "restconf GET https/1.0 - close" + expectpart "$(curl $CURLOPTS --http1.0 -X GET $proto://$addr/.well-known/host-meta)" 52 "" --not-- 'HTTP' - if [ "${WITH_RESTCONF}" = "native" ]; then # XXX does not work with nginx - new "restconf GET http/1.0 - returns 1.0" - expectpart "$(curl $CURLOPTS --http1.0 -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.0 200 OK' "" "" "" - fi - new "restconf GET http/1.1" - expectpart "$(curl $CURLOPTS --http1.1 -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.1 200 OK' "" "" "" + fi - # Try http/2 - go back to http/1.1 - new "restconf GET http/2" - expectpart "$(curl $CURLOPTS --http2 -X GET $proto://$addr/.well-known/host-meta)" 0 "HTTP/1.1 200 OK" "" "" "" + if [ $proto = http ]; then + new "restconf GET http/1.1 - close" + expectpart "$(curl $CURLOPTS --http1.1 -X GET $proto://$addr/.well-known/host-meta | tr '\0' '\n')" 0 --not-- 'HTTP' + else + new "restconf GET https/1.1 - close" + expectpart "$(curl $CURLOPTS --http1.1 -X GET $proto://$addr/.well-known/host-meta)" 52 --not-- 'HTTP' + fi + + if [ $proto = http ]; then + new "restconf GET http/2 switch protocol" + expectpart "$(curl $CURLOPTS --http2 -X GET $proto://$addr/.well-known/host-meta | tr '\0' '\n')" 0 --not-- 'HTTP' + else + new "restconf GET https/2 alpn protocol" + expectpart "$(curl $CURLOPTS --http2 -X GET $proto://$addr/.well-known/host-meta)" 0 "HTTP/$HVER 200" "" "" "" + fi - if [ $proto = http ]; then # see (2) https to http port in restconf_main_native.c - new "restconf GET http/2 prior-knowledge (http)" - expectpart "$(curl $CURLOPTS --http2-prior-knowledge -X GET $proto://$addr/.well-known/host-meta 2>&1)" "16 52 55" # "Error in the HTTP2 framing layer" "Connection reset by peer" + # Wrong protocol http when https or vice versa + if [ $proto = http ]; then # see (2) https to http port in restconf_main_native.c + new "Wrong proto=https on http port, expect err 35 wrong version number" + expectpart "$(curl $CURLOPTS -X GET https://$addr:80/.well-known/host-meta 2>&1)" 35 #"wrong version number" # dependent on curl version + else # see (1) http to https port in restconf_main_native.c + new "Wrong proto=http on https port, expect bad request" + expectpart "$(curl $CURLOPTS -X GET http://$addr:443/.well-known/host-meta)" "16 52 55" --not-- 'HTTP' + fi else - new "restconf GET http/2 prior-knowledge(http2)" - expectpart "$(curl $CURLOPTS --http2-prior-knowledge -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.1 200 OK' "" "" "" - fi + echo "fcgi or native+http/1 or native+http/1+http/2" + if [ "${WITH_RESTCONF}" = "native" ]; then # XXX does not work with nginx + new "restconf GET http/1.0 - returns 1.0" + expectpart "$(curl $CURLOPTS --http1.0 -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.0 200 OK' "" "" "" + fi + new "restconf GET http/1.1" + expectpart "$(curl $CURLOPTS --http1.1 -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.1 200 OK' "" "" "" - # Negative test GET datastore - if [ $proto = http ]; then # see (2) https to http port in restconf_main_native.c - new "Wrong proto=https on http port, expect err 35 wrong version number" - expectpart "$(curl $CURLOPTS -X GET https://$addr:80/.well-known/host-meta 2>&1)" 35 #"wrong version number" # dependent on curl version - else # see (1) http to https port in restconf_main_native.c - new "Wrong proto=http on https port, expect bad request" - expectpart "$(curl $CURLOPTS -X GET http://$addr:443/.well-known/host-meta)" 0 "HTTP/$HVER 400" - # expectpart "$(curl $CURLOPTS -X GET http://$addr:443/.well-known/host-meta 2>&1)" 56 "Connection reset by peer" - fi + if ${HAVE_LIBNGHTTP2}; then + # http/1 + http/2 + + new "restconf GET http/2 switch protocol" + expectpart "$(curl $CURLOPTS --http2 -X GET $proto://$addr/.well-known/host-meta)" 0 "" "HTTP/2 200" "" "" "" # Only if http: HTTP/1.1 101 Switching Protocols + else + # http/1 only Try http/2 - go back to http/1.1 + new "restconf GET http/2 switch protocol" + expectpart "$(curl $CURLOPTS --http2 -X GET $proto://$addr/.well-known/host-meta)" 0 "HTTP/1.1 200 OK" "" "" "" + fi + + # http2-prior knowledge + if [ $proto = http ]; then # see (2) https to http port in restconf_main_native.c + new "restconf GET http/2 prior-knowledge (http)" + expectpart "$(curl $CURLOPTS --http2-prior-knowledge -X GET $proto://$addr/.well-known/host-meta 2>&1)" "16 52 55" # "Error in the HTTP2 framing layer" "Connection reset by peer" + else + new "restconf GET https/2 prior-knowledge" + expectpart "$(curl $CURLOPTS --http2-prior-knowledge -X GET $proto://$addr/.well-known/host-meta)" 0 "HTTP/$HVER 200" "" "" "" + fi + + # Wrong protocol http when https or vice versa + if [ $proto = http ]; then # see (2) https to http port in restconf_main_native.c + new "Wrong proto=https on http port, expect err 35 wrong version number" + expectpart "$(curl $CURLOPTS -X GET https://$addr:80/.well-known/host-meta 2>&1)" 35 #"wrong version number" # dependent on curl version + else # see (1) http to https port in restconf_main_native.c + new "Wrong proto=http on https port, expect bad request" + expectpart "$(curl $CURLOPTS -X GET http://$addr:443/.well-known/host-meta)" 0 "HTTP/" "400" + # expectpart "$(curl $CURLOPTS -X GET http://$addr:443/.well-known/host-meta 2>&1)" 56 "Connection reset by peer" + fi fi # HTTP/2 + # Exact match new "restconf get restconf resource. RFC 8040 3.3 (json)" expectpart "$(curl $CURLOPTS -X GET -H "Accept: application/yang-data+json" $proto://$addr/restconf)" 0 "HTTP/$HVER 200" '{"ietf-restconf:restconf":{"data":{},"operations":{},"yang-library-version":"2019-01-04"}}' @@ -341,7 +390,7 @@ function testrun() new "restconf Check eth/0/0 GET augmented state level 2" expectpart "$(curl $CURLOPTS -X GET -H 'Accept: application/yang-data+json' $proto://$addr/restconf/data/ietf-interfaces:interfaces/interface=eth%2f0%2f0/clixon-example:my-status)" 0 "HTTP/$HVER 200" '{"clixon-example:my-status":{"int":42,"str":"foo"}}' - new "restconf Check eth/0/0 added state XXXXXXX" + new "restconf Check eth/0/0 added state" expectpart "$(curl $CURLOPTS -X GET -H 'Accept: application/yang-data+json' $proto://$addr/restconf/data/clixon-example:state)" 0 "HTTP/$HVER 200" '{"clixon-example:state":{"op":\["41","42","43"\]}}' new "restconf Re-post eth/0/0 which should generate error" diff --git a/test/test_restconf_op.sh b/test/test_restconf_op.sh index 4ff5228d..40e58e06 100755 --- a/test/test_restconf_op.sh +++ b/test/test_restconf_op.sh @@ -204,7 +204,6 @@ new "restconf DELETE whole datastore" expectpart "$(curl $CURLOPTS -X DELETE $RCPROTO://localhost/restconf/data)" 0 "HTTP/$HVER 204" #--------------- Multiple request in single TCP tests -# XXX THIS HAS LEAKS IN HTTP/1 ONLY! expectpart "$(curl $CURLOPTS -H "Accept: application/yang-data+xml" -X GET $RCPROTO://localhost/restconf/data?content=config --next $CURLOPTS -H "Content-Type: application/yang-data+json" -X POST $RCPROTO://localhost/restconf/data -d '{"example:cont1":{"interface":{"name":"local0","type":"regular"}}}')" 0 "HTTP/$HVER 200" '' "HTTP/$HVER 201" #--------------- json type tests