Updated Branches: refs/heads/3.2.x fd0fc8624 -> e640fd9fa
[TS-1484] SSL-crashed every now and then with 3.2.0 + SNI-fixes Trunk: d6d07d8c43c084d5683b45c1efe9ed2bf9ef8635 3.2.x Patch: https://issues.apache.org/jira/secure/attachment/12546611/TS-1484-3.2.x.patch review/test: jpeach, igalic, zwoop backport: igalic Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/e640fd9f Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/e640fd9f Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/e640fd9f Branch: refs/heads/3.2.x Commit: e640fd9fab76737546c7bd2c38ebc5b647d2d2c3 Parents: fb6cf1c Author: Igor GaliÄ <[email protected]> Authored: Tue Oct 9 21:42:58 2012 +0200 Committer: Igor GaliÄ <[email protected]> Committed: Tue Oct 9 21:42:58 2012 +0200 ---------------------------------------------------------------------- CHANGES | 2 + STATUS | 6 -- iocore/net/P_SSLCertLookup.h | 8 ++-- iocore/net/SSLCertLookup.cc | 98 ++++++++++++++++++++++++++------- iocore/net/SSLNetVConnection.cc | 55 ------------------- lib/ts/Trie.h | 7 +-- 6 files changed, 84 insertions(+), 92 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e640fd9f/CHANGES ---------------------------------------------------------------------- diff --git a/CHANGES b/CHANGES index 82f0953..ee1240d 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ -*- coding: utf-8 -*- Changes with Apache Traffic Server 3.2.3 + *) [TS-1484] SSL-crashed every now and then with 3.2.0 + SNI-fixes + *) [TS-1514] Fix collation in custom logging. Author: bettydramit http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e640fd9f/STATUS ---------------------------------------------------------------------- diff --git a/STATUS b/STATUS index 97ce02e..5d101fd 100644 --- a/STATUS +++ b/STATUS @@ -41,12 +41,6 @@ A list of all bugs open for the next development release can be found at PATCHES ACCEPTED TO BACKPORT FROM TRUNK: - *) SSL-crashed every now and then with 3.2.0 + SNI-fixes - Trunk: d6d07d8c43c084d5683b45c1efe9ed2bf9ef8635 - 3.2.x Patch: https://issues.apache.org/jira/secure/attachment/12546611/TS-1484-3.2.x.patch - Jira: https://issues.apache.org/jira/browse/TS-1484 - +1: jpeach, igalic, zwoop - PATCHES PROPOSED TO BACKPORT FROM TRUNK: [ New proposals should be added at the end of the list ] http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e640fd9f/iocore/net/P_SSLCertLookup.h ---------------------------------------------------------------------- diff --git a/iocore/net/P_SSLCertLookup.h b/iocore/net/P_SSLCertLookup.h index da18345..ae65fd8 100644 --- a/iocore/net/P_SSLCertLookup.h +++ b/iocore/net/P_SSLCertLookup.h @@ -36,12 +36,12 @@ class SSLCertLookup bool addInfoToHash( const char *strAddr, const char *cert, const char *ca, const char *serverPrivateKey); - char config_file_path[PATH_NAME_MAX]; - SslConfigParams *param; - bool multipleCerts; + char config_file_path[PATH_NAME_MAX]; + SslConfigParams * param; + bool multipleCerts; SSLContextStorage * ssl_storage; - SSL_CTX * ssl_default; + SSL_CTX * ssl_default; public: bool hasMultipleCerts() const { return multipleCerts; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e640fd9f/iocore/net/SSLCertLookup.cc ---------------------------------------------------------------------- diff --git a/iocore/net/SSLCertLookup.cc b/iocore/net/SSLCertLookup.cc index 8438ef2..bce1124 100644 --- a/iocore/net/SSLCertLookup.cc +++ b/iocore/net/SSLCertLookup.cc @@ -45,6 +45,64 @@ typedef const SSL_METHOD * ink_ssl_method_t; typedef SSL_METHOD * ink_ssl_method_t; #endif +#if TS_USE_TLS_SNI + +static int +ssl_servername_callback(SSL * ssl, int * ad, void * arg) +{ + SSL_CTX * ctx = NULL; + SSLCertLookup * lookup = (SSLCertLookup *) arg; + const char * servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); + + Debug("ssl", "ssl=%p ad=%d lookup=%p server=%s", ssl, *ad, lookup, servername); + + if (likely(servername)) { + ctx = lookup->findInfoInHash((char *)servername); + } + + if (ctx == NULL) { + ctx = lookup->defaultContext(); + } + + if (ctx != NULL) { + SSL_set_SSL_CTX(ssl, ctx); + } + + // At this point, we might have updated ctx based on the SNI lookup, or we might still have the + // original SSL context that we set when we accepted the connection. + ctx = SSL_get_SSL_CTX(ssl); + Debug("ssl", "found SSL context %p for requested name '%s'", ctx, servername); + + if (ctx == NULL) { + return SSL_TLSEXT_ERR_NOACK; + } + + // We need to return one of the SSL_TLSEXT_ERR constants. If we return an + // error, we can fill in *ad with an alert code to propgate to the + // client, see SSL_AD_*. + return SSL_TLSEXT_ERR_OK; +} + +#endif /* TS_USE_TLS_SNI */ + +static SSL_CTX * +make_ssl_context(void * arg) +{ + SSL_CTX * ctx = NULL; + ink_ssl_method_t meth = NULL; + + meth = SSLv23_server_method(); + ctx = SSL_CTX_new(meth); + +#if TS_USE_TLS_SNI + Debug("ssl", "setting SNI callbacks with for ctx %p", ctx); + SSL_CTX_set_tlsext_servername_callback(ctx, ssl_servername_callback); + SSL_CTX_set_tlsext_servername_arg(ctx, arg); +#endif /* TS_USE_TLS_SNI */ + + return ctx; +} + class SSLContextStorage { @@ -52,7 +110,7 @@ class SSLContextStorage { explicit SslEntry(SSL_CTX * c) : ctx(c) {} - void Print() const { printf("%p/%p", this, ctx); } + void Print() const { Debug("ssl", "SslEntry=%p SSL_CTX=%p", this, ctx); } SSL_CTX * ctx; LINK(SslEntry, link); @@ -112,7 +170,14 @@ void SSLCertLookup::init(SslConfigParams * p) { param = p; - multipleCerts = buildTable(); + + this->multipleCerts = buildTable(); + + // We *must* have a default context even if it can't possibly work. The default context is used to bootstrap the SSL + // handshake so that we can subsequently do the SNI lookup to switch to the real context. + if (this->ssl_default == NULL) { + this->ssl_default = make_ssl_context(this); + } } bool @@ -192,17 +257,6 @@ SSLCertLookup::buildTable() line = tokLine(NULL, &tok_state); } // while(line != NULL) -/* if(num_el == 0) - { - Warning("%s No entries in %s. Using default server cert for all connections", - moduleName, configFilePath); - } - - if(is_debug_tag_set("ssl")) - { - Print(); - } -*/ ats_free(file_buf); return ret; } @@ -272,10 +326,9 @@ SSLCertLookup::addInfoToHash( const char *strAddr, const char *cert, const char *caCert, const char *serverPrivateKey) { - ink_ssl_method_t meth = NULL; + SSL_CTX * ctx; - meth = SSLv23_server_method(); - SSL_CTX *ctx = SSL_CTX_new(meth); + ctx = make_ssl_context(this); if (!ctx) { SSLNetProcessor::logSSLError("Cannot create new server contex."); return (false); @@ -493,7 +546,7 @@ SSLContextStorage::insert(SSL_CTX * ctx, const char * name) return false; } - Debug("indexed wildcard certificate for '%s' as '%s'", name, reversed); + Debug("ssl", "indexed wildcard certificate for '%s' as '%s' with SSL_CTX %p", name, reversed, ctx); return this->wildcards.Insert(reversed, new SslEntry(ctx), 0 /* rank */, -1 /* keylen */); } else { ink_hash_table_insert(this->hostnames, name, (void *)ctx); @@ -522,6 +575,7 @@ SSLContextStorage::lookup(const char * name) const return NULL; } + Debug("ssl", "attempting wildcard match for %s", reversed); entry = this->wildcards.Search(reversed); if (entry) { return entry->ctx; @@ -537,17 +591,18 @@ REGRESSION_TEST(SslHostLookup)(RegressionTest* t, int atype, int * pstatus) { TestBox tb(t, pstatus); SSLContextStorage storage; - ink_ssl_method_t methods = SSLv23_server_method(); - SSL_CTX * wild = SSL_CTX_new(methods); - SSL_CTX * notwild = SSL_CTX_new(methods); - SSL_CTX * foo = SSL_CTX_new(methods); + SSL_CTX * wild = make_ssl_context(NULL); + SSL_CTX * notwild = make_ssl_context(NULL); + SSL_CTX * b_notwild = make_ssl_context(NULL); + SSL_CTX * foo = make_ssl_context(NULL); *pstatus = REGRESSION_TEST_PASSED; tb.check(storage.insert(foo, "www.foo.com"), "insert host context"); tb.check(storage.insert(wild, "*.wild.com"), "insert wildcard context"); tb.check(storage.insert(notwild, "*.notwild.com"), "insert wildcard context"); + tb.check(storage.insert(b_notwild, "*.b.notwild.com"), "insert wildcard context"); // Basic wildcard cases. tb.check(storage.lookup("a.wild.com") == wild, "wildcard lookup for a.wild.com"); @@ -557,6 +612,7 @@ REGRESSION_TEST(SslHostLookup)(RegressionTest* t, int atype, int * pstatus) // Varify that wildcard does longest match. tb.check(storage.lookup("a.notwild.com") == notwild, "wildcard lookup for a.notwild.com"); tb.check(storage.lookup("notwild.com") == notwild, "wildcard lookup for notwild.com"); + tb.check(storage.lookup("c.b.notwild.com") == b_notwild, "wildcard lookup for c.b.notwild.com"); // Basic hostname cases. tb.check(storage.lookup("www.foo.com") == foo, "host lookup for www.foo.com"); http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e640fd9f/iocore/net/SSLNetVConnection.cc ---------------------------------------------------------------------- diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc index fd89cba..1df458e 100644 --- a/iocore/net/SSLNetVConnection.cc +++ b/iocore/net/SSLNetVConnection.cc @@ -24,10 +24,6 @@ #include "P_Net.h" #include "P_SSLNextProtocolSet.h" -#if HAVE_OPENSSL_TLS1_H -#include <openssl/tls1.h> -#endif - #define SSL_READ_ERROR_NONE 0 #define SSL_READ_ERROR 1 #define SSL_READ_READY 2 @@ -46,48 +42,6 @@ ClassAllocator<SSLNetVConnection> sslNetVCAllocator("sslNetVCAllocator"); // Private // -static SSL_CTX * ssl_default = SSL_CTX_new(SSLv23_server_method()); - -#if TS_USE_TLS_SNI - -static int -ssl_servername_callback(SSL * ssl, int * ad, void * arg) -{ - SSL_CTX * ctx = NULL; - SSLCertLookup * lookup = (SSLCertLookup *) arg; - const char * servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); - - Debug("ssl", "ssl=%p ad=%d lookup=%p server=%s", ssl, *ad, lookup, servername); - - if (likely(servername)) { - ctx = lookup->findInfoInHash((char *)servername); - } - - if (ctx == NULL) { - ctx = lookup->defaultContext(); - } - - if (ctx != NULL) { - SSL_set_SSL_CTX(ssl, ctx); - } - - // At this point, we might have updated ctx based on the SNI lookup, or we might still have the - // original SSL context that we set when we accepted the connection. - ctx = SSL_get_SSL_CTX(ssl); - Debug("ssl", "found SSL context %p for requested name '%s'", ctx, servername); - - if (ctx == NULL) { - return SSL_TLSEXT_ERR_NOACK; - } - - // We need to return one of the SSL_TLSEXT_ERR constants. If we return an - // error, we can fill in *ad with an alert code to propgate to the - // client, see SSL_AD_*. - return SSL_TLSEXT_ERR_OK; -} - -#endif /* TS_USE_TLS_SNI */ - static SSL * make_ssl_connection(SSL_CTX * ctx, SSLNetVConnection * netvc) { @@ -503,15 +457,6 @@ SSLNetVConnection::sslStartHandShake(int event, int &err) if (ctx == NULL) { ctx = sslCertLookup.defaultContext(); } - if (ctx == NULL) { - ctx = ssl_default; - } - -#if TS_USE_TLS_SNI - Debug("ssl", "setting SNI callbacks with initial ctx %p", ctx); - SSL_CTX_set_tlsext_servername_callback(ctx, ssl_servername_callback); - SSL_CTX_set_tlsext_servername_arg(ctx, &sslCertLookup); -#endif /* TS_USE_TLS_SNI */ this->ssl = make_ssl_connection(ctx, this); if (this->ssl == NULL) { http://git-wip-us.apache.org/repos/asf/trafficserver/blob/e640fd9f/lib/ts/Trie.h ---------------------------------------------------------------------- diff --git a/lib/ts/Trie.h b/lib/ts/Trie.h index eed9207..eb5363b 100644 --- a/lib/ts/Trie.h +++ b/lib/ts/Trie.h @@ -170,14 +170,9 @@ Trie<T>::Search(const char *key, int key_len /* = -1 */) const curr_node->Print("Trie::Search"); } if (curr_node->occupied) { - if (!found_node) { + if (!found_node || curr_node->rank <= found_node->rank) { found_node = curr_node; } - else { - if (curr_node->rank < found_node->rank) { - found_node = curr_node; - } - } } if (i == key_len) { break;
