You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by jp...@apache.org on 2013/04/25 01:05:30 UTC
[2/2] git commit: TS-1850: improved SSL error reporting
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/master
Commit: bb2fcaad3ad568e9aa64c2bcffbd9992249f72f7
Parents: 95a28c7
Author: James Peach <jp...@apache.org>
Authored: Wed Apr 24 16:01:44 2013 -0700
Committer: James Peach <jp...@apache.org>
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 <qi...@taobao.com>
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;
}