Updated Branches: refs/heads/sphinx-docs d9f943ed7 -> f032bbcd1
TS-1850: improved SSL error reporting Many SSL certificate management errors don't report the full OpenSSL error stack, making the error difficult to diagnose. We should log the full error stack in those cases. Add varargs logging macros to Diags.h. Add const to Diags members where possible. Improve SSL certificate error reporting Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/bb2fcaad Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/bb2fcaad Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/bb2fcaad Branch: refs/heads/sphinx-docs Commit: bb2fcaad3ad568e9aa64c2bcffbd9992249f72f7 Parents: 95a28c7 Author: James Peach <[email protected]> Authored: Wed Apr 24 16:01:44 2013 -0700 Committer: James Peach <[email protected]> Committed: Wed Apr 24 16:04:40 2013 -0700 ---------------------------------------------------------------------- CHANGES | 2 + iocore/net/P_SSLUtils.h | 2 +- iocore/net/SSLUtils.cc | 248 +++++++++++++++++++----------------------- 3 files changed, 114 insertions(+), 138 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb2fcaad/CHANGES ---------------------------------------------------------------------- diff --git a/CHANGES b/CHANGES index 0b43956..d75f00d 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,8 @@ Changes with Apache Traffic Server 3.3.3 + *) [TS-1850] Improve SSL certificate error reporting. + *) [TS-1848] Fix MIMEHdr::field_value_set_int64() wrapper. Author: Yunkai Zhang <[email protected]> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb2fcaad/iocore/net/P_SSLUtils.h ---------------------------------------------------------------------- diff --git a/iocore/net/P_SSLUtils.h b/iocore/net/P_SSLUtils.h index d336706..5f6dc01 100644 --- a/iocore/net/P_SSLUtils.h +++ b/iocore/net/P_SSLUtils.h @@ -53,7 +53,7 @@ SSL_CTX * SSLInitClientContext(const SSLConfigParams * param); void SSLInitializeLibrary(); // Log an SSL error. -void SSLError(const char *errStr, bool critical = true); +void SSLError(const char * fmt, ...) TS_PRINTFLIKE(1, 2); // Log a SSL network buffer. void SSLDebugBufferPrint(const char * tag, const char * buffer, unsigned buflen, const char * message); http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb2fcaad/iocore/net/SSLUtils.cc ---------------------------------------------------------------------- diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index 9827815..2cbcf77 100644 --- a/iocore/net/SSLUtils.cc +++ b/iocore/net/SSLUtils.cc @@ -49,6 +49,44 @@ typedef SSL_METHOD * ink_ssl_method_t; static ProxyMutex ** sslMutexArray; static bool open_ssl_initialized = false; +struct ats_x509_certificate +{ + explicit ats_x509_certificate(X509 * x) : x509(x) {} + ~ats_x509_certificate() { if (x509) X509_free(x509); } + + operator bool() const { + return x509 != NULL; + } + + X509 * x509; + +private: + ats_x509_certificate(const ats_x509_certificate&); + ats_x509_certificate& operator=(const ats_x509_certificate&); +}; + +struct ats_file_bio +{ + ats_file_bio(const char * path, const char * mode) + : bio(BIO_new_file(path, mode)) { + } + + ~ats_file_bio() { + (void)BIO_set_close(bio, BIO_CLOSE); + BIO_free(bio); + } + + operator bool() const { + return bio != NULL; + } + + BIO * bio; + +private: + ats_file_bio(const ats_file_bio&); + ats_file_bio& operator=(const ats_file_bio&); +}; + static unsigned long SSL_pthreads_thread_id() { @@ -74,45 +112,29 @@ SSL_locking_callback(int mode, int type, const char * file, int line) } } -static int -SSL_CTX_add_extra_chain_cert_file(SSL_CTX * ctx, const char *file) +static bool +SSL_CTX_add_extra_chain_cert_file(SSL_CTX * ctx, const char * chainfile) { - BIO *in; - int ret = 0; - X509 *x = NULL; + ats_file_bio bio(chainfile, "r"); - in = BIO_new(BIO_s_file_internal()); - if (in == NULL) { - SSLerr(SSL_F_SSL_USE_CERTIFICATE_FILE, ERR_R_BUF_LIB); - goto end; + if (!bio) { + return false; } - if (BIO_read_filename(in, file) <= 0) { - SSLerr(SSL_F_SSL_USE_CERTIFICATE_FILE, ERR_R_SYS_LIB); - goto end; - } + for (;;) { + ats_x509_certificate certificate(PEM_read_bio_X509_AUX(bio.bio, NULL, NULL, NULL)); - // j = ERR_R_PEM_LIB; - while ((x = PEM_read_bio_X509(in, NULL, ctx->default_passwd_callback, ctx->default_passwd_callback_userdata)) != NULL) { - ret = SSL_CTX_add_extra_chain_cert(ctx, x); - if (!ret) { - X509_free(x); - BIO_free(in); - return -1; - } + if (!certificate) { + // No more the certificates in this file. + break; } -/* x = PEM_read_bio_X509(in, NULL, ctx->default_passwd_callback, ctx->default_passwd_callback_userdata); - if (x == NULL) { - SSLerr(SSL_F_SSL_USE_CERTIFICATE_FILE, j); - goto end; - } - - ret = SSL_CTX_add_extra_chain_cert(ctx, x);*/ -end: - // if (x != NULL) X509_free(x); - if (in != NULL) - BIO_free(in); - return (ret); + + if (!SSL_CTX_add_extra_chain_cert(ctx, certificate.x509)) { + return false; + } + } + + return true; } #if TS_USE_TLS_SNI @@ -202,7 +224,7 @@ SSLInitializeLibrary() } void -SSLError(const char *errStr, bool critical) +SSLError(const char * fmt, ...) { unsigned long l; char buf[256]; @@ -210,28 +232,14 @@ SSLError(const char *errStr, bool critical) int line, flags; unsigned long es; - if (!critical) { - if (errStr) { - Debug("ssl_error", "SSL ERROR: %s", errStr); - } else { - Debug("ssl_error", "SSL ERROR"); - } - } else { - if (errStr) { - Error("SSL ERROR: %s", errStr); - } else { - Error("SSL ERROR"); - } - } + va_list ap; + va_start(ap, fmt); + ErrorV(fmt, ap); + va_end(ap); es = CRYPTO_thread_id(); while ((l = ERR_get_error_line_data(&file, &line, &data, &flags)) != 0) { - if (!critical) { - Debug("ssl_error", "SSL::%lu:%s:%s:%d:%s", es, - ERR_error_string(l, buf), file, line, (flags & ERR_TXT_STRING) ? data : ""); - } else { - Error("SSL::%lu:%s:%s:%d:%s", es, ERR_error_string(l, buf), file, line, (flags & ERR_TXT_STRING) ? data : ""); - } + Error("SSL::%lu:%s:%s:%d:%s", es, ERR_error_string(l, buf), file, line, (flags & ERR_TXT_STRING) ? data : ""); } } @@ -285,50 +293,51 @@ SSLInitServerContext( SSL_CTX_set_quiet_shutdown(ctx, 1); + // XXX OpenSSL recommends that we should use SSL_CTX_use_certificate_chain_file() here. That API + // also loads only the first certificate, but it allows the intermediate CA certificate chain to + // be in the same file. SSL_CTX_use_certificate_chain_file() was added in OpenSSL 0.9.3. completeServerCertPath = Layout::relative_to(params->serverCertPathOnly, serverCertPtr); - - if (SSL_CTX_use_certificate_file(ctx, completeServerCertPath, SSL_FILETYPE_PEM) <= 0) { - Error ("SSL ERROR: Cannot use server certificate file: %s", (const char *)completeServerCertPath); + if (!SSL_CTX_use_certificate_file(ctx, completeServerCertPath, SSL_FILETYPE_PEM)) { + SSLError("failed to load certificate from %s", (const char *)completeServerCertPath); goto fail; } + // First, load any CA chains from the global chain file. + if (params->serverCertChainPath) { + xptr<char> completeServerCaCertPath(Layout::relative_to(params->serverCACertPath, params->serverCertChainPath)); + if (!SSL_CTX_add_extra_chain_cert_file(ctx, completeServerCaCertPath)) { + SSLError("failed to load global certificate chain from %s", (const char *)completeServerCaCertPath); + goto fail; + } + } + + // Now, load any additional certificate chains specified in this entry. if (serverCaCertPtr) { xptr<char> completeServerCaCertPath(Layout::relative_to(params->serverCACertPath, serverCaCertPtr)); - if (SSL_CTX_add_extra_chain_cert_file(ctx, completeServerCaCertPath) <= 0) { - Error ("SSL ERROR: Cannot use server certificate chain file: %s", (const char *)completeServerCaCertPath); + if (!SSL_CTX_add_extra_chain_cert_file(ctx, completeServerCaCertPath)) { + SSLError("failed to load certificate chain from %s", (const char *)completeServerCaCertPath); goto fail; } } if (serverKeyPtr == NULL) { // assume private key is contained in cert obtained from multicert file. - if (SSL_CTX_use_PrivateKey_file(ctx, completeServerCertPath, SSL_FILETYPE_PEM) <= 0) { - Error("SSL ERROR: Cannot use server private key file: %s", (const char *)completeServerCertPath); + if (!SSL_CTX_use_PrivateKey_file(ctx, completeServerCertPath, SSL_FILETYPE_PEM)) { + SSLError("failed to load server private key from %s", (const char *)completeServerCertPath); goto fail; } - } else { - if (params->serverKeyPathOnly != NULL) { - xptr<char> completeServerKeyPath(Layout::get()->relative_to(params->serverKeyPathOnly, serverKeyPtr)); - if (SSL_CTX_use_PrivateKey_file(ctx, completeServerKeyPath, SSL_FILETYPE_PEM) <= 0) { - Error("SSL ERROR: Cannot use server private key file: %s", (const char *)completeServerKeyPath); - goto fail; - } - } else { - SSLError("Empty ssl private key path in records.config."); - } - - } - - if (params->serverCertChainPath) { - xptr<char> completeServerCaCertPath(Layout::relative_to(params->serverCACertPath, params->serverCertChainPath)); - if (SSL_CTX_add_extra_chain_cert_file(ctx, params->serverCertChainPath) <= 0) { - Error ("SSL ERROR: Cannot use server certificate chain file: %s", (const char *)completeServerCaCertPath); + } else if (params->serverKeyPathOnly != NULL) { + xptr<char> completeServerKeyPath(Layout::get()->relative_to(params->serverKeyPathOnly, serverKeyPtr)); + if (!SSL_CTX_use_PrivateKey_file(ctx, completeServerKeyPath, SSL_FILETYPE_PEM)) { + SSLError("failed to load server private key from %s", (const char *)completeServerKeyPath); goto fail; } + } else { + SSLError("empty SSL private key path in records.config"); } if (!SSL_CTX_check_private_key(ctx)) { - SSLError("Server private key does not match the certificate public key"); + SSLError("server private key does not match the certificate public key"); goto fail; } @@ -349,21 +358,23 @@ SSLInitServerContext( } else { // disable client cert support server_verify_client = SSL_VERIFY_NONE; - Error("Illegal Client Certification Level in records.config"); + Error("illegal client certification level %d in records.config", server_verify_client); } + // XXX I really don't think that this is a good idea. We should be setting this a some finer granularity, + // possibly per SSL CTX. httpd uses md5(host:port), which seems reasonable. session_id_context = 1; + SSL_CTX_set_session_id_context(ctx, (const unsigned char *) &session_id_context, sizeof(session_id_context)); SSL_CTX_set_verify(ctx, server_verify_client, NULL); SSL_CTX_set_verify_depth(ctx, params->verify_depth); // might want to make configurable at some point. - SSL_CTX_set_session_id_context(ctx, (const unsigned char *) &session_id_context, sizeof session_id_context); SSL_CTX_set_client_CA_list(ctx, SSL_load_client_CA_file(params->serverCACertFilename)); } if (params->cipherSuite != NULL) { if (!SSL_CTX_set_cipher_list(ctx, params->cipherSuite)) { - SSLError("Invalid Cipher Suite in records.config"); + SSLError("invalid cipher suite in records.config"); goto fail; } } @@ -392,7 +403,7 @@ SSLInitClientContext(const SSLConfigParams * params) // disable selected protocols SSL_CTX_set_options(client_ctx, params->ssl_ctx_options); if (!client_ctx) { - SSLError("Cannot create new client context"); + SSLError("cannot create new client context"); return NULL; } @@ -404,22 +415,20 @@ SSLInitClientContext(const SSLConfigParams * params) } if (params->clientCertPath != 0) { - if (SSL_CTX_use_certificate_file(client_ctx, params->clientCertPath, SSL_FILETYPE_PEM) <= 0) { - Error ("SSL Error: Cannot use client certificate file: %s", params->clientCertPath); - SSL_CTX_free(client_ctx); - return NULL; + if (!SSL_CTX_use_certificate_file(client_ctx, params->clientCertPath, SSL_FILETYPE_PEM)) { + SSLError("failed to load client certificate from %s", params->clientCertPath); + goto fail; } - if (SSL_CTX_use_PrivateKey_file(client_ctx, clientKeyPtr, SSL_FILETYPE_PEM) <= 0) { - Error ("SSL ERROR: Cannot use client private key file: %s", clientKeyPtr); - SSL_CTX_free(client_ctx); - return NULL; + if (!SSL_CTX_use_PrivateKey_file(client_ctx, clientKeyPtr, SSL_FILETYPE_PEM)) { + SSLError("failed to load client private key file from %s", clientKeyPtr); + goto fail; } if (!SSL_CTX_check_private_key(client_ctx)) { - Error("SSL ERROR: Client private key (%s) does not match the certificate public key (%s)", clientKeyPtr, params->clientCertPath); - SSL_CTX_free(client_ctx); - return NULL; + SSLError("client private key (%s) does not match the certificate public key (%s)", + clientKeyPtr, params->clientCertPath); + goto fail; } } @@ -431,56 +440,21 @@ SSLInitClientContext(const SSLConfigParams * params) SSL_CTX_set_verify_depth(client_ctx, params->client_verify_depth); if (params->clientCACertFilename != NULL && params->clientCACertPath != NULL) { - if ((!SSL_CTX_load_verify_locations(client_ctx, params->clientCACertFilename, - params->clientCACertPath)) || + if ((!SSL_CTX_load_verify_locations(client_ctx, params->clientCACertFilename, params->clientCACertPath)) || (!SSL_CTX_set_default_verify_paths(client_ctx))) { - Error("SSL ERROR: Client CA Certificate file (%s) or CA Certificate path (%s) invalid", params->clientCACertFilename, params->clientCACertPath); - SSL_CTX_free(client_ctx); - return NULL; + SSLError("invalid client CA Certificate file (%s) or CA Certificate path (%s)", + params->clientCACertFilename, params->clientCACertPath); + goto fail; } } } return client_ctx; -} - -struct ats_x509_certificate -{ - explicit ats_x509_certificate(X509 * x) : x509(x) {} - ~ats_x509_certificate() { if (x509) X509_free(x509); } - - operator bool() const { - return x509 != NULL; - } - - X509 * x509; -private: - ats_x509_certificate(const ats_x509_certificate&); - ats_x509_certificate& operator=(const ats_x509_certificate&); -}; - -struct ats_file_bio -{ - ats_file_bio(const char * path, const char * mode) - : bio(BIO_new_file(path, mode)) { - } - - ~ats_file_bio() { - (void)BIO_set_close(bio, BIO_CLOSE); - BIO_free(bio); - } - - operator bool() const { - return bio != NULL; - } - - BIO * bio; - -private: - ats_file_bio(const ats_file_bio&); - ats_file_bio& operator=(const ats_file_bio&); -}; +fail: + SSL_CTX_free(client_ctx); + return NULL; +} static char * asn1_strdup(ASN1_STRING * s) @@ -657,7 +631,7 @@ SSLParseCertificateConfiguration( } if (!file_buf) { - Error("%s: failed to read SSL certificate configuration from %s", __func__, params->configFilePath); + Error("failed to read SSL certificate configuration from %s", params->configFilePath); return false; }
