You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by sh...@apache.org on 2018/09/20 15:25:36 UTC
[trafficserver] branch master updated: Improve the logging messages
on server certificate verification failure.
This is an automated email from the ASF dual-hosted git repository.
shinrich pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 6389199 Improve the logging messages on server certificate verification failure.
6389199 is described below
commit 638919960c3f0f713514e33bb1bcd7b0fa3df925
Author: Susan Hinrichs <sh...@apache.org>
AuthorDate: Fri Sep 7 13:20:59 2018 -0500
Improve the logging messages on server certificate verification failure.
---
iocore/net/I_NetVConnection.h | 31 +++++++++++++++++-
iocore/net/P_UnixNetVConnection.h | 1 +
iocore/net/SSLClientUtils.cc | 68 ++++++++++++++++++---------------------
proxy/http/HttpSM.cc | 3 ++
4 files changed, 66 insertions(+), 37 deletions(-)
diff --git a/iocore/net/I_NetVConnection.h b/iocore/net/I_NetVConnection.h
index d3900f6..3cfa6a2 100644
--- a/iocore/net/I_NetVConnection.h
+++ b/iocore/net/I_NetVConnection.h
@@ -177,6 +177,10 @@ struct NetVCOptions {
/** Server name to use for SNI data on an outbound connection.
*/
ats_scoped_str sni_servername;
+ /** FQDN used to connect to the origin. May be different
+ * than sni_servername if pristine host headers are used
+ */
+ ats_scoped_str ssl_servername;
/**
* Client certificate to use in response to OS's certificate request
@@ -209,6 +213,16 @@ struct NetVCOptions {
return *this;
}
self &
+ set_ssl_servername(const char *name)
+ {
+ if (name) {
+ ssl_servername = ats_strdup(name);
+ } else {
+ ssl_servername = nullptr;
+ }
+ return *this;
+ }
+ self &
set_client_certname(const char *name)
{
clientCertificate = ats_strdup(name);
@@ -220,15 +234,30 @@ struct NetVCOptions {
operator=(self const &that)
{
if (&that != this) {
+ /*
+ * It is odd but necessary to null the scoped string pointer here
+ * and then explicitly call release on them in the string assignements
+ * below.
+ * We a memcpy from that to this. This will put that's string pointers into
+ * this's memory. Therefore we must first explicitly null out
+ * this's original version of the string. The release after the
+ * memcpy removes the extra reference to that's copy of the string
+ * Removing the release will eventualy cause a double free crash
+ */
sni_servername = nullptr; // release any current name.
+ ssl_servername = nullptr;
clientCertificate = nullptr;
memcpy(static_cast<void *>(this), &that, sizeof(self));
if (that.sni_servername) {
sni_servername.release(); // otherwise we'll free the source string.
this->sni_servername = ats_strdup(that.sni_servername);
}
+ if (that.ssl_servername) {
+ ssl_servername.release(); // otherwise we'll free the source string.
+ this->ssl_servername = ats_strdup(that.ssl_servername);
+ }
if (that.clientCertificate) {
- clientCertificate.release();
+ clientCertificate.release(); // otherwise we'll free the source string.
this->clientCertificate = ats_strdup(that.clientCertificate);
}
}
diff --git a/iocore/net/P_UnixNetVConnection.h b/iocore/net/P_UnixNetVConnection.h
index 6bc3765..90ce23c 100644
--- a/iocore/net/P_UnixNetVConnection.h
+++ b/iocore/net/P_UnixNetVConnection.h
@@ -67,6 +67,7 @@ NetVCOptions::reset()
etype = ET_NET;
sni_servername = nullptr;
+ ssl_servername = nullptr;
clientCertificate = nullptr;
}
diff --git a/iocore/net/SSLClientUtils.cc b/iocore/net/SSLClientUtils.cc
index ea07881..4aefe6a 100644
--- a/iocore/net/SSLClientUtils.cc
+++ b/iocore/net/SSLClientUtils.cc
@@ -25,6 +25,7 @@
#include "tscore/X509HostnameValidator.h"
#include "P_Net.h"
#include "P_SSLClientUtils.h"
+#include "YamlSNIConfig.h"
#include <openssl/err.h>
#include <openssl/pem.h>
@@ -54,21 +55,23 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
*/
ssl = static_cast<SSL *>(X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()));
SSLNetVConnection *netvc = SSLNetVCAccess(ssl);
- if (!preverify_ok) {
+ bool enforce_mode = (netvc && netvc->options.clientVerificationFlag == static_cast<uint8_t>(YamlSNIConfig::Level::STRICT));
+ if (!preverify_ok && netvc != nullptr) {
// Don't bother to check the hostname if we failed openssl's verification
SSLDebug("verify error:num=%d:%s:depth=%d", err, X509_verify_cert_error_string(err), depth);
- if (netvc && netvc->options.clientVerificationFlag == 2) {
- if (netvc->options.sni_servername) {
- Warning("Hostname verification failed for (%s) but still continuing with the connection establishment",
- netvc->options.sni_servername.get());
- } else {
- char buff[INET6_ADDRSTRLEN];
- ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN);
- Warning("Server certificate verification failed for %s but still continuing with the connection establishment", buff);
- }
- return 1;
+ const char *sni_name;
+ char buff[INET6_ADDRSTRLEN];
+ ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN);
+ if (netvc->options.sni_servername) {
+ sni_name = netvc->options.sni_servername.get();
+ } else {
+ sni_name = buff;
}
- return preverify_ok;
+ Warning("Core server certificate verification failed for (%s). Action=%s Error=%s server=%s(%s) depth=%d", sni_name,
+ enforce_mode ? "Terminate" : "Continue", X509_verify_cert_error_string(err), netvc->options.ssl_servername.get(), buff,
+ depth);
+ // If not enforcing ignore the error, just log warning
+ return enforce_mode ? preverify_ok : 1;
}
if (depth != 0) {
// Not server cert....
@@ -77,35 +80,28 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
if (netvc != nullptr) {
netvc->callHooks(TS_EVENT_SSL_SERVER_VERIFY_HOOK);
- // Match SNI if present
+ char *matched_name = nullptr;
+ unsigned char *sni_name;
+ char buff[INET6_ADDRSTRLEN];
if (netvc->options.sni_servername) {
- char *matched_name = nullptr;
- if (validate_hostname(cert, reinterpret_cast<unsigned char *>(netvc->options.sni_servername.get()), false, &matched_name)) {
- SSLDebug("Hostname %s verified OK, matched %s", netvc->options.sni_servername.get(), matched_name);
- ats_free(matched_name);
- return preverify_ok;
- }
- Warning("Hostname verification failed for (%s)", netvc->options.sni_servername.get());
- }
- // Otherwise match by IP
- else {
- char buff[INET6_ADDRSTRLEN];
+ sni_name = reinterpret_cast<unsigned char *>(netvc->options.sni_servername.get());
+ } else {
+ sni_name = reinterpret_cast<unsigned char *>(buff);
ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN);
- if (validate_hostname(cert, reinterpret_cast<unsigned char *>(buff), true, nullptr)) {
- SSLDebug("IP %s verified OK", buff);
- return preverify_ok;
- }
- Warning("IP verification failed for (%s)", buff);
}
-
- if (netvc->options.clientVerificationFlag == 2) {
- char buff[INET6_ADDRSTRLEN];
- ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN);
- Warning("Server certificate verification failed but continuing with the connection establishment:%s:%s",
- netvc->options.sni_servername.get(), buff);
+ if (validate_hostname(cert, sni_name, false, &matched_name)) {
+ SSLDebug("Hostname %s verified OK, matched %s", netvc->options.sni_servername.get(), matched_name);
+ ats_free(matched_name);
return preverify_ok;
}
- return 0;
+ // Get the server address if we did't already compute it
+ if (netvc->options.sni_servername) {
+ ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN);
+ }
+ // If we got here the verification failed
+ Warning("SNI (%s) not in certificate. Action=%s server=%s(%s)", sni_name, enforce_mode ? "Terminate" : "Continue",
+ netvc->options.ssl_servername.get(), buff);
+ return enforce_mode ? 0 : preverify_ok;
}
return preverify_ok;
}
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index cc03aec..7a974f9 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -5001,6 +5001,9 @@ HttpSM::do_http_server_open(bool raw)
if (host && len > 0) {
opt.set_sni_servername(host, len);
}
+ if (t_state.server_info.name) {
+ opt.set_ssl_servername(t_state.server_info.name);
+ }
SSLConfig::scoped_config params;
// check if the overridden client cert filename is already attached to an existing ssl context