From cdacca125c4261cbd72b803acfde3d1f8f6e6b61 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Fri, 27 Aug 2021 09:33:59 +0200 Subject: [PATCH] Native restconf: SSL client certs failures are returned as http 405 errors, not fail during SSL negotiation --- CHANGELOG.md | 6 +- apps/restconf/restconf_evhtp.c | 107 ++++++++++++++++++++------- apps/restconf/restconf_main_native.c | 48 +++++++++--- apps/restconf/restconf_native.h | 3 +- apps/restconf/restconf_nghttp2.c | 80 ++++++++++++++++---- test/test_restconf_ssl_certs.sh | 9 ++- 6 files changed, 195 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1f47b26..031cb30a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,8 +57,10 @@ Users may have to change how they access the system * See changes under new feature "YANG leafref feature update" * Validation of referred node type (not referring) * Leafref required-instance must be set to make strict data-node check -* Native Restconf is now default, not fcgi/nginx - * That is, to configure with fcgi, you need to explicitly configure: `--with-restconf=fcgi` +* Native Restconf + * Native restocnf is now default, not fcgi/nginx + * That is, to configure with fcgi, you need to explicitly configure: `--with-restconf=fcgi` + * SSL client certs failures are returned as http 405 errors, not fail during SSL negotiation * New clixon-config@2021-07-11.yang revision * Added: `CLICON_RESTCONF_HTTP2_PLAIN` * Removed default of `CLICON_RESTCONF_INSTALLDIR` diff --git a/apps/restconf/restconf_evhtp.c b/apps/restconf/restconf_evhtp.c index 0c3a4fdb..8bfc4c7d 100644 --- a/apps/restconf/restconf_evhtp.c +++ b/apps/restconf/restconf_evhtp.c @@ -465,6 +465,53 @@ evhtp_upgrade_http2(clicon_handle h, } #endif /* HAVE_LIBNGHTTP2 */ +/*! Check common connection sanity checks and terminate if found before request processing + * + * These tests maybe could have done earlier, this is somewhat late since the session is + * closed and that is always good to do as early as possible. + * The following are current checks: + * 1) Check if http/2 non-tls is disabled + * 2) Check if ssl client certs ae valid + * @param[in] h Clixon handle + * @param[in] rc Restconf connection handle + * @param[in] sd Http stream + * @param[out] term Terminate session + * @retval -1 Error + * @retval 0 OK + */ +static int +restconf_evhtp_sanity(clicon_handle h, + restconf_conn *rc, + restconf_stream_data *sd) +{ + int retval = -1; + cxobj *xerr = NULL; + long code; + cbuf *cberr = NULL; + + /* 1) Check if ssl client certs are valid */ + if (rc->rc_ssl != NULL && + (code = SSL_get_verify_result(rc->rc_ssl)) != 0){ + if ((cberr = cbuf_new()) == NULL){ + clicon_err(OE_UNIX, errno, "cbuf_new"); + goto done; + } + cprintf(cberr, "HTTP cert verification failed: (code:%ld)", code); + if (netconf_operation_not_supported_xml(&xerr, "protocol", cbuf_get(cberr)) < 0) + goto done; + if (api_return_err0(sd->sd_conn->rc_h, sd, xerr, 1, YANG_DATA_JSON, 0) < 0) + goto done; + rc->rc_exit = 1; + } + retval = 0; + done: + if (cberr) + cbuf_free(cberr); + if (xerr) + xml_free(xerr); + return retval; +} + /*! Callback for each incoming http request for path / * * This are all messages except /.well-known, Registered with evhtp_set_cb @@ -518,7 +565,7 @@ restconf_path_root(evhtp_request_t *req, /* 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){ + if ((sd->sd_qvec = cvec_new(0)) == NULL){ clicon_err(OE_UNIX, errno, "cvec_new"); evhtp_internal_error(req); goto done; @@ -541,21 +588,26 @@ restconf_path_root(evhtp_request_t *req, evhtp_internal_error(req); goto done; } + /* Check sanity of session, eg ssl client cert validation, may set rc_exit */ + if (restconf_evhtp_sanity(h, rc, sd) < 0) + goto done; + if (rc->rc_exit == 0){ #ifdef HAVE_LIBNGHTTP2 - if (ret == 1){ - if ((ret = evhtp_upgrade_http2(h, sd)) < 0){ - evhtp_internal_error(req); - goto done; + if (ret == 1){ + if ((ret = evhtp_upgrade_http2(h, sd)) < 0){ + evhtp_internal_error(req); + goto done; + } + if (ret == 0) + keep_params = 1; } - if (ret == 0) - keep_params = 1; - } #endif - if (ret == 1){ - /* call generic function */ - if (api_root_restconf(h, sd, sd->sd_qvec) < 0){ - evhtp_internal_error(req); - goto done; + if (ret == 1){ + /* call generic function */ + if (api_root_restconf(h, sd, sd->sd_qvec) < 0){ + evhtp_internal_error(req); + goto done; + } } } /* Clear input request parameters from this request */ @@ -632,21 +684,26 @@ restconf_path_wellknown(evhtp_request_t *req, evhtp_internal_error(req); goto done; } + /* Check sanity of session, eg ssl client cert validation, may set rc_exit */ + if (restconf_evhtp_sanity(h, rc, sd) < 0) + goto done; + if (rc->rc_exit == 0){ #ifdef HAVE_LIBNGHTTP2 - if (ret == 1){ - if ((ret = evhtp_upgrade_http2(h, sd)) < 0){ - evhtp_internal_error(req); - goto done; + if (ret == 1){ + if ((ret = evhtp_upgrade_http2(h, sd)) < 0){ + evhtp_internal_error(req); + goto done; + } + if (ret == 0) + keep_params = 1; } - if (ret == 0) - keep_params = 1; - } #endif - if (ret == 1){ - /* call generic function */ - if (api_well_known(h, sd) < 0){ - evhtp_internal_error(req); - goto done; + if (ret == 1){ + /* call generic function */ + if (api_well_known(h, sd) < 0){ + evhtp_internal_error(req); + goto done; + } } } /* Clear input request parameters from this request */ diff --git a/apps/restconf/restconf_main_native.c b/apps/restconf/restconf_main_native.c index 68ebb5cf..213e0480 100644 --- a/apps/restconf/restconf_main_native.c +++ b/apps/restconf/restconf_main_native.c @@ -421,7 +421,13 @@ restconf_verify_certs(int preverify_ok, /* Verify the CA name */ } // h = SSL_get_app_data(ssl); - return preverify_ok; + /* Different schemes for return values if failure detected: + * - 0 (preferity_ok) the session terminates here in SSL negotiation, an http client + * will get a low level error (not http reply) + * - 1 Check if the cert is valid using SSL_get_verify_result(rc->rc_ssl) + * @see restconf_evhtp_sanity and restconf_nghttp2_sanity where this is done for http/1 and http/2 + */ + return 1; } /*! Debug print of all incoming alpn alternatives, eg h2 and http/1.1 @@ -851,6 +857,19 @@ restconf_connection(int s, "protocolmalformed-messageNo evhtp output") < 0) goto done; } + if (rc->rc_exit){ /* Server-initiated exit for http/2 */ + SSL_free(rc->rc_ssl); + rc->rc_ssl = NULL; + evconn->ssl = NULL; + if (close(rc->rc_s) < 0){ + clicon_err(OE_UNIX, errno, "close"); + goto done; + } + clixon_event_unreg_fd(rc->rc_s, restconf_connection); + clicon_debug(1, "%s evconn-free (%p) 2", __FUNCTION__, evconn); + restconf_conn_free(rc); + goto ok; + } #ifdef HAVE_LIBNGHTTP2 if (sd->sd_upgrade2){ nghttp2_error ngerr; @@ -900,16 +919,23 @@ restconf_connection(int s, #endif /* HAVE_LIBEVHTP */ #ifdef HAVE_LIBNGHTTP2 case HTTP_2: - if ((ret = http2_recv(rc, (unsigned char *)buf, n)) < 0) - goto done; - if (ret == 0){ - restconf_close_ssl_socket(rc, 1); - if (restconf_conn_free(rc) < 0) - goto done; - goto ok; + if (rc->rc_exit){ /* Server-initiated exit for http/2 */ + nghttp2_error ngerr; + if ((ngerr = nghttp2_session_terminate_session(rc->rc_ngsession, 0)) < 0) + clicon_err(OE_NGHTTP2, ngerr, "nghttp2_session_terminate_session %d", ngerr); + } + else { + if ((ret = http2_recv(rc, (unsigned char *)buf, n)) < 0) + goto done; + 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++; } - /* There may be more data frames */ - readmore++; break; #endif /* HAVE_LIBNGHTTP2 */ default: @@ -1274,9 +1300,7 @@ restconf_accept_client(int fd, * Note this _only_ works if SSL_set1_host() was set previously,... */ if (SSL_get_verify_result(rc->rc_ssl) == X509_V_OK) { /* for peer cert */ - const char *peername = SSL_get0_peername(rc->rc_ssl); - if (peername != NULL) { /* Name checks were in scope and matched the peername */ clicon_debug(1, "%s peername:%s", __FUNCTION__, peername); diff --git a/apps/restconf/restconf_native.h b/apps/restconf/restconf_native.h index 5a78017c..49aabf43 100644 --- a/apps/restconf/restconf_native.h +++ b/apps/restconf/restconf_native.h @@ -62,7 +62,7 @@ extern "C" { /* Forward */ struct restconf_conn; -/* session stream struct, mainly for http/2 but htp/1 has a single pseudo-stream with id=0 +/* session stream struct, mainly for http/2 but http/1 has a single pseudo-stream with id=0 */ typedef struct { qelem_t sd_qelem; /* List header */ @@ -95,6 +95,7 @@ typedef struct restconf_conn { clicon_handle rc_h; /* Clixon handle */ SSL *rc_ssl; /* Structure for SSL connection */ restconf_stream_data *rc_streams; /* List of http/2 session streams */ + int rc_exit; /* Set to close socket server-side (NYI) */ /* Decision to keep lib-specific data here, otherwise new struct necessary * drawback is specific includes need to go everywhere */ #ifdef HAVE_LIBEVHTP diff --git a/apps/restconf/restconf_nghttp2.c b/apps/restconf/restconf_nghttp2.c index 797026bb..6da6e395 100644 --- a/apps/restconf/restconf_nghttp2.c +++ b/apps/restconf/restconf_nghttp2.c @@ -277,6 +277,62 @@ recv_callback(nghttp2_session *session, return 0; } +/*! Check common connection sanity checks and terminate if found before request processing + * + * These tests maybe could have done earlier, this is somewhat late since the session is + * closed and that is always good to do as early as possible. + * The following are current checks: + * 1) Check if http/2 non-tls is disabled + * 2) Check if ssl client certs ae valid + * @param[in] h Clixon handle + * @param[in] rc Restconf connection handle + * @param[in] sd Http stream + * @param[out] term Terminate session + * @retval -1 Error + * @retval 0 OK + */ +static int +restconf_nghttp2_sanity(clicon_handle h, + restconf_conn *rc, + restconf_stream_data *sd) +{ + int retval = -1; + cxobj *xerr = NULL; + long code; + cbuf *cberr = NULL; + + /* 1) Check if http/2 non-tls is disabled */ + if (rc->rc_ssl == NULL && + clicon_option_bool(h, "CLICON_RESTCONF_HTTP2_PLAIN") == 0){ + if (netconf_operation_not_supported_xml(&xerr, "protocol", "Non-tls HTTP/2 is disabled") < 0) + goto done; + if (api_return_err0(h, sd, xerr, 1, YANG_DATA_JSON, 0) < 0) + goto done; + rc->rc_exit = 1; + } + /* 2) Check if ssl client certs ae valid */ + else if (rc->rc_ssl != NULL && + (code = SSL_get_verify_result(rc->rc_ssl)) != 0){ + if ((cberr = cbuf_new()) == NULL){ + clicon_err(OE_UNIX, errno, "cbuf_new"); + goto done; + } + cprintf(cberr, "HTTP cert verification failed: (code:%ld)", code); + if (netconf_operation_not_supported_xml(&xerr, "protocol", cbuf_get(cberr)) < 0) + goto done; + if (api_return_err0(sd->sd_conn->rc_h, sd, xerr, 1, YANG_DATA_JSON, 0) < 0) + goto done; + rc->rc_exit = 1; + } + retval = 0; + done: + if (cberr) + cbuf_free(cberr); + if (xerr) + xml_free(xerr); + return retval; +} + /*! Callback for each incoming http request for path / * * This are all messages except /.well-known, Registered with evhtp_set_cb @@ -298,26 +354,18 @@ restconf_nghttp2_path(restconf_stream_data *sd) char *oneline = NULL; cvec *cvv = NULL; char *cn; - + clicon_debug(1, "------------"); rc = sd->sd_conn; if ((h = rc->rc_h) == NULL){ clicon_err(OE_RESTCONF, EINVAL, "arg is NULL"); goto done; } - - if (rc->rc_ssl == NULL){ - if (clicon_option_bool(h, "CLICON_RESTCONF_HTTP2_PLAIN") == 0){ - cxobj *xerr = NULL; - - if (netconf_operation_not_supported_xml(&xerr, "protocol", "HTTP/2 plain / non-tls is not allowed") < 0) - goto done; - if (api_return_err0(h, sd, xerr, 1, YANG_DATA_JSON, 0) < 0) - goto done; - goto ok; - } - } - else { + if (restconf_nghttp2_sanity(h, rc, sd) < 0) + goto done; + if (rc->rc_exit == 1) + goto ok; + if (rc->rc_ssl != NULL){ /* Slightly awkward way of taking SSL cert subject and CN and add it to restconf parameters * instead of accessing it directly * SSL subject fields, eg CN (Common Name) , can add more here? */ @@ -339,10 +387,10 @@ restconf_nghttp2_path(restconf_stream_data *sd) } else if (api_root_restconf(h, sd, sd->sd_qvec) < 0) goto done; - ok: /* Clear (fcgi) paramaters from this request */ if (restconf_param_del_all(h) < 0) goto done; + ok: retval = 0; done: clicon_debug(1, "%s %d", __FUNCTION__, retval); @@ -894,7 +942,7 @@ http2_recv(restconf_conn *rc, { int retval = -1; nghttp2_error ngerr; - + clicon_debug(1, "%s", __FUNCTION__); if (rc->rc_ngsession == NULL){ /* http2_session_init not called */ diff --git a/test/test_restconf_ssl_certs.sh b/test/test_restconf_ssl_certs.sh index ba179825..4d7ee112 100755 --- a/test/test_restconf_ssl_certs.sh +++ b/test/test_restconf_ssl_certs.sh @@ -181,6 +181,9 @@ EOF done # invalid ca fi # XXX + # Generate random certificate + openssl req -newkey rsa:2048 -nodes -keyout $certdir/random.key -x509 -days 365 -out $certdir/random.crt -subj "/C=XX/ST=TEST/L=TEST/O=TEST/OU=TEST/CN=TEST" + fi # genkeys # Write local config @@ -286,13 +289,15 @@ EOF # code # expectpart "$(curl $CURLOPTS -X GET $RCPROTO://localhost/restconf/data/example:x 2>&1)" 0 "HTTP/$HVER 400" - new "limited invalid cert" - expectpart "$(curl $CURLOPTS --key $certdir/limited.key --cert $certdir/limited.crt -X GET $RCPROTO://localhost/restconf/data/example:x 2>&1)" "16 35 55 56" # 55 "certificate expired" + expectpart "$(curl $CURLOPTS --key $certdir/limited.key --cert $certdir/limited.crt -X GET $RCPROTO://localhost/restconf/data/example:x 2>&1)" 0 "HTTP/$HVER 405" "HTTP cert verification failed" new "too weak cert (sign w md5)" expectpart "$(curl $CURLOPTS --key $certdir/mymd5.key --cert $certdir/mymd5.crt -X GET $RCPROTO://localhost/restconf/data/example:x 2>&1)" "35 58" # "md too weak" + new "Random cert" + expectpart "$(curl $CURLOPTS --key $certdir/random.key --cert $certdir/random.crt -X GET $RCPROTO://localhost/restconf/data/example:x 2>&1)" 0 "HTTP/$HVER 405" "HTTP cert verification failed" + # Havent been able to generate "wrong CA" # new "invalid cert from wrong CA" # expectpart "$(curl $CURLOPTS --key $certdir/invalid.key --cert $certdir/invalid.crt -X GET $RCPROTO://localhost/restconf/data/example:x 2>&1)" 0 foo # 58 "unable to set private key file" # 58 unable to set private key file