From 2445b82d961a88617ab53749982dbdba1a221ce5 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Sat, 11 Feb 2017 00:35:01 +0000 Subject: [PATCH] Need dedicated TS_SSL_SERVERNAME_HOOK If you have a plugin that needs to trigger on every TLS client hello, this won't work the TS_SSL_SNI_HOOK with openssl-1.0.2. In that version TS_SSL_SNI_HOOK is shared with the TS_SSL_CERT_HOOK which does not trigger on session reuse. I propose adding a dedicated TS_SSL_SERVERNAME_HOOK and deprecate the TS_SSL_SNI_HOOK to drop on the next major release --- .../api/functions/TSVConnTunnel.en.rst | 5 +--- .../api/types/TSHttpHookID.en.rst | 4 +++ iocore/net/SSLNetVConnection.cc | 18 ++++++++---- iocore/net/SSLUtils.cc | 28 +++++++++++++------ lib/ts/apidefs.h.in | 6 ++-- proxy/InkAPIInternal.h | 1 + proxy/InkAPITest.cc | 3 +- proxy/http/HttpDebugNames.cc | 2 ++ 8 files changed, 46 insertions(+), 21 deletions(-) diff --git a/doc/developer-guide/api/functions/TSVConnTunnel.en.rst b/doc/developer-guide/api/functions/TSVConnTunnel.en.rst index 0669b9a4759..85fa1842b51 100644 --- a/doc/developer-guide/api/functions/TSVConnTunnel.en.rst +++ b/doc/developer-guide/api/functions/TSVConnTunnel.en.rst @@ -32,8 +32,5 @@ Description =========== Set the SSL connection :arg:`svc` to convert to a blind tunnel. Can be called -from :member:`TS_VCONN_PRE_ACCEPT_HOOK` or :member:`TS_SSL_SNI_HOOK` / :member:`TS_SSL_CERT_HOOK`. +from :member:`TS_VCONN_PRE_ACCEPT_HOOK`, :member:`TS_SSL_SERVERNAME_HOOK`, or :member:`TS_SSL_SNI_HOOK` / :member:`TS_SSL_CERT_HOOK`. -For this to work from the :member:`TS_SSL_SNI_HOOK` or :member:`TS_SSL_CERT_HOOK`, -either the server must be running OpenSSL 1.0.2 or a version of OpenSSL 1.0.1 -with the appropriate patch installed. diff --git a/doc/developer-guide/api/types/TSHttpHookID.en.rst b/doc/developer-guide/api/types/TSHttpHookID.en.rst index 6ebdcae23c2..0520931fbf0 100644 --- a/doc/developer-guide/api/types/TSHttpHookID.en.rst +++ b/doc/developer-guide/api/types/TSHttpHookID.en.rst @@ -74,6 +74,8 @@ Enumeration Members .. c:member:: TSHttpHookID TS_SSL_CERT_HOOK +.. c:member:: TSHttpHookID TS_SSL_SERVERNAME_HOOK + .. c:member:: TSHttpHookID TS_SSL_LAST_HOOK .. c:member:: TSHttpHookID TS_HTTP_LAST_HOOK @@ -81,3 +83,5 @@ Enumeration Members Description =========== +Note that :member:`TS_SSL_CERT_HOOK` and :member:`TS_SSL_SNI_HOOK` hook the same openssl callbacks. +In openssl 1.0.2 and beyond :member:`TS_SSL_SERVERNAME_HOOK` is involved only for the openssl servername callback. :member:`TS_SSL_SNI_HOOK` and :member:`TS_SSL_CERT_HOOK` are called for the openssl certificate callback. diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc index 5ad895b9eb0..50f4bdcc4af 100644 --- a/iocore/net/SSLNetVConnection.cc +++ b/iocore/net/SSLNetVConnection.cc @@ -1438,17 +1438,22 @@ bool SSLNetVConnection::callHooks(TSEvent eventId) { // Only dealing with the SNI/CERT hook so far. - ink_assert(eventId == TS_EVENT_SSL_CERT); + ink_assert(eventId == TS_EVENT_SSL_CERT || eventId == TS_EVENT_SSL_SERVERNAME); Debug("ssl", "callHooks sslHandshakeHookState=%d", this->sslHandshakeHookState); // First time through, set the type of the hook that is currently being invoked - if (HANDSHAKE_HOOKS_PRE == sslHandshakeHookState) { + if ((this->sslHandshakeHookState == HANDSHAKE_HOOKS_PRE || this->sslHandshakeHookState == HANDSHAKE_HOOKS_DONE) && + eventId == TS_EVENT_SSL_CERT) { // the previous hook should be DONE and set curHook to nullptr before trigger the sni hook. ink_assert(curHook == nullptr); // set to HOOKS_CERT means CERT/SNI hooks has called by SSL_accept() this->sslHandshakeHookState = HANDSHAKE_HOOKS_CERT; // get Hooks curHook = ssl_hooks->get(TS_SSL_CERT_INTERNAL_HOOK); + } else if (eventId == TS_EVENT_SSL_SERVERNAME) { + if (!curHook) { + curHook = ssl_hooks->get(TS_SSL_SERVERNAME_INTERNAL_HOOK); + } } else { // Not in the right state // reenable and continue @@ -1457,14 +1462,17 @@ SSLNetVConnection::callHooks(TSEvent eventId) bool reenabled = true; if (curHook != nullptr) { - // Otherwise, we have plugin hooks to run this->sslHandshakeHookState = HANDSHAKE_HOOKS_INVOKE; curHook->invoke(eventId, this); - reenabled = (this->sslHandshakeHookState != HANDSHAKE_HOOKS_INVOKE); + reenabled = eventId != TS_EVENT_SSL_CERT || (this->sslHandshakeHookState != HANDSHAKE_HOOKS_INVOKE); } else { // no SNI-Hooks set, set state to HOOKS_DONE // no plugins registered for this hook, return (reenabled == true) - sslHandshakeHookState = HANDSHAKE_HOOKS_DONE; + if (eventId == TS_EVENT_SSL_CERT) { + sslHandshakeHookState = HANDSHAKE_HOOKS_DONE; + } else if (eventId == TS_EVENT_SSL_SERVERNAME) { + sslHandshakeHookState = HANDSHAKE_HOOKS_PRE; + } } return reenabled; } diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index 99a4e434ca7..363bf79da15 100644 --- a/iocore/net/SSLUtils.cc +++ b/iocore/net/SSLUtils.cc @@ -402,9 +402,21 @@ ssl_cert_callback(SSL *ssl, void * /*arg*/) // Return 1 for success, 0 for error, or -1 to pause return retval; } + +/* + * Cannot stop this callback. Always reeneabled + */ +static int +ssl_servername_only_callback(SSL *ssl, int * /* ad */, void * /*arg*/) +{ + SSLNetVConnection *netvc = (SSLNetVConnection *)SSL_get_app_data(ssl); + netvc->callHooks(TS_EVENT_SSL_SERVERNAME); + return SSL_TLSEXT_ERR_OK; +} + #else static int -ssl_servername_callback(SSL *ssl, int * /* ad */, void * /*arg*/) +ssl_servername_and_cert_callback(SSL *ssl, int * /* ad */, void * /*arg*/) { SSLNetVConnection *netvc = SSLNetVCAccess(ssl); bool reenabled; @@ -566,16 +578,13 @@ ssl_context_enable_tickets(SSL_CTX *ctx, const char *ticket_key_path) // so that we don't leave a ticket_key pointer attached if it fails. if (SSL_CTX_set_tlsext_ticket_key_cb(ctx, ssl_callback_session_ticket) == 0) { Error("failed to set session ticket callback"); - goto fail; + ticket_block_free(keyblock); + return nullptr; } SSL_CTX_clear_options(ctx, SSL_OP_NO_TICKET); return keyblock; -fail: - ticket_block_free(keyblock); - return nullptr; - #else /* !HAVE_OPENSSL_SESSION_TICKETS */ (void)ticket_key_path; return nullptr; @@ -1188,7 +1197,7 @@ SSLDiagnostic(const SourceLocation &loc, bool debug, SSLNetVConnection *vc, cons while ((l = ERR_get_error_line_data(&file, &line, &data, &flags)) != 0) { if (debug) { if (unlikely(diags->on())) { - diags->log("ssl", DL_Debug, &loc, "SSL::%lu:%s:%s:%d%s%s%s%s", es, ERR_error_string(l, buf), file, line, + diags->log("ssl-diag", DL_Debug, &loc, "SSL::%lu:%s:%s:%d%s%s%s%s", es, ERR_error_string(l, buf), file, line, (flags & ERR_TXT_STRING) ? ":" : "", (flags & ERR_TXT_STRING) ? data : "", vc ? ": peer address is " : "", ip_buf); } @@ -1212,7 +1221,7 @@ SSLDiagnostic(const SourceLocation &loc, bool debug, SSLNetVConnection *vc, cons va_start(ap, fmt); if (debug) { - diags->log_va("ssl", DL_Debug, &loc, fmt, ap); + diags->log_va("ssl-diag", DL_Debug, &loc, fmt, ap); } else { diags->error_va(DL_Error, &loc, fmt, ap); } @@ -1453,8 +1462,9 @@ ssl_set_handshake_callbacks(SSL_CTX *ctx) // Make sure the callbacks are set #if TS_USE_CERT_CB SSL_CTX_set_cert_cb(ctx, ssl_cert_callback, nullptr); + SSL_CTX_set_tlsext_servername_callback(ctx, ssl_servername_only_callback); #else - SSL_CTX_set_tlsext_servername_callback(ctx, ssl_servername_callback); + SSL_CTX_set_tlsext_servername_callback(ctx, ssl_servername_and_cert_callback); #endif #endif } diff --git a/lib/ts/apidefs.h.in b/lib/ts/apidefs.h.in index dcf6d0e83d7..513141e9f75 100644 --- a/lib/ts/apidefs.h.in +++ b/lib/ts/apidefs.h.in @@ -288,7 +288,8 @@ typedef enum { TS_VCONN_PRE_ACCEPT_HOOK = TS_SSL_FIRST_HOOK, TS_SSL_SNI_HOOK, TS_SSL_CERT_HOOK = TS_SSL_SNI_HOOK, - TS_SSL_LAST_HOOK = TS_SSL_CERT_HOOK, + TS_SSL_SERVERNAME_HOOK, + TS_SSL_LAST_HOOK = TS_SSL_SERVERNAME_HOOK, TS_HTTP_LAST_HOOK } TSHttpHookID; @@ -448,7 +449,8 @@ typedef enum { TS_EVENT_INTERNAL_60200 = 60200, TS_EVENT_INTERNAL_60201 = 60201, TS_EVENT_INTERNAL_60202 = 60202, - TS_EVENT_SSL_CERT = 60203 + TS_EVENT_SSL_CERT = 60203, + TS_EVENT_SSL_SERVERNAME = 60204 } TSEvent; #define TS_EVENT_HTTP_READ_REQUEST_PRE_REMAP TS_EVENT_HTTP_PRE_REMAP /* backwards compat */ diff --git a/proxy/InkAPIInternal.h b/proxy/InkAPIInternal.h index a3248b8aad6..5e364a9b7b4 100644 --- a/proxy/InkAPIInternal.h +++ b/proxy/InkAPIInternal.h @@ -285,6 +285,7 @@ typedef enum { TS_SSL_INTERNAL_FIRST_HOOK, TS_VCONN_PRE_ACCEPT_INTERNAL_HOOK = TS_SSL_INTERNAL_FIRST_HOOK, TS_SSL_CERT_INTERNAL_HOOK, + TS_SSL_SERVERNAME_INTERNAL_HOOK, TS_SSL_INTERNAL_LAST_HOOK } TSSslHookInternalID; diff --git a/proxy/InkAPITest.cc b/proxy/InkAPITest.cc index 6978c05d59d..eac3df9c243 100644 --- a/proxy/InkAPITest.cc +++ b/proxy/InkAPITest.cc @@ -5550,7 +5550,8 @@ typedef enum { ORIG_TS_SSL_FIRST_HOOK, ORIG_TS_VCONN_PRE_ACCEPT_HOOK = ORIG_TS_SSL_FIRST_HOOK, ORIG_TS_SSL_SNI_HOOK, - ORIG_TS_SSL_LAST_HOOK = ORIG_TS_SSL_SNI_HOOK, + ORIG_TS_SSL_SERVERNAME_HOOK, + ORIG_TS_SSL_LAST_HOOK = TS_SSL_SERVERNAME_HOOK, ORIG_TS_HTTP_LAST_HOOK } ORIG_TSHttpHookID; diff --git a/proxy/http/HttpDebugNames.cc b/proxy/http/HttpDebugNames.cc index 7285787aa7b..b9e526337fc 100644 --- a/proxy/http/HttpDebugNames.cc +++ b/proxy/http/HttpDebugNames.cc @@ -482,6 +482,8 @@ HttpDebugNames::get_api_hook_name(TSHttpHookID t) return "TS_VCONN_PRE_ACCEPT_HOOK"; case TS_SSL_CERT_HOOK: return "TS_SSL_CERT_HOOK"; + case TS_SSL_SERVERNAME_HOOK: + return "TS_SSL_SERVERNAME_HOOK"; } return "unknown hook";