Skip to content

Extend TLS/SSL functionality with SNI and using system roots #358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Extend TLS/SSL functionality with SNI and using system roots #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f66f0ae
Add the TLS SNI extension when connecting over TLS to a server
dbussink Aug 11, 2021
5655185
Use system CA roots for client if no option provided
dbussink Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/violite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ const char *sslGetErrString(enum enum_ssl_init_error err);

struct st_VioSSLFd {
SSL_CTX *ssl_context;
const char *hostname;
};

int sslaccept(struct st_VioSSLFd *, MYSQL_VIO, long timeout,
Expand All @@ -264,7 +265,7 @@ struct st_VioSSLFd *new_VioSSLConnectorFd(
const char *key_file, const char *cert_file, const char *ca_file,
const char *ca_path, const char *cipher, const char *ciphersuites,
enum enum_ssl_init_error *error, const char *crl_file, const char *crl_path,
const long ssl_ctx_flags, const char *server_host);
const long ssl_ctx_flags, const char *server_host, int verify, bool verify_identity);

long process_tls_version(const char *tls_version);

Expand Down
4 changes: 3 additions & 1 deletion plugin/x/client/xconnection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,8 +675,10 @@ XError Connection_impl::activate_tls() {
auto ssl_ctx_flags = process_tls_version(
details::null_when_empty(m_context->m_ssl_config.m_tls_version));

const int verify = m_context->m_ssl_config.m_mode >= Ssl_config::Mode::Ssl_verify_ca ? SSL_VERIFY_PEER : SSL_VERIFY_NONE;
const bool verify_identity =
Ssl_config::Mode::Ssl_verify_identity == m_context->m_ssl_config.m_mode;

m_vioSslFd = new_VioSSLConnectorFd(
details::null_when_empty(m_context->m_ssl_config.m_key),
details::null_when_empty(m_context->m_ssl_config.m_cert),
Expand All @@ -686,7 +688,7 @@ XError Connection_impl::activate_tls() {
&m_ssl_init_error,
details::null_when_empty(m_context->m_ssl_config.m_crl),
details::null_when_empty(m_context->m_ssl_config.m_crl_path),
ssl_ctx_flags, verify_identity ? m_hostname.c_str() : nullptr);
ssl_ctx_flags, m_hostname.c_str(), verify, verify_identity);

if (nullptr == m_vioSslFd) return get_ssl_init_error(m_ssl_init_error);

Expand Down
93 changes: 43 additions & 50 deletions sql-common/client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3352,9 +3352,12 @@ const char *STDCALL mysql_get_ssl_cipher(MYSQL *mysql MY_ATTRIBUTE((unused))) {
return nullptr;
}

#if OPENSSL_VERSION_NUMBER < 0x10002000L
/*
Check the server's (subject) Common Name against the
hostname we connected to
hostname we connected to. This function is only used
when OpenSSL is older than 1.0.2 since versions since
1.0.2 provide an implementation for hostname validation.

SYNOPSIS
ssl_verify_server_cert()
Expand All @@ -3375,13 +3378,11 @@ static int ssl_verify_server_cert(Vio *vio, const char *server_hostname,
X509 *server_cert = nullptr;
int ret_validation = 1;

#if !(OPENSSL_VERSION_NUMBER >= 0x10002000L)
int cn_loc = -1;
char *cn = NULL;
ASN1_STRING *cn_asn1 = NULL;
X509_NAME_ENTRY *cn_entry = NULL;
X509_NAME *subject = NULL;
#endif

DBUG_TRACE;
DBUG_PRINT("enter", ("server_hostname: %s", server_hostname));
Expand Down Expand Up @@ -3411,16 +3412,6 @@ static int ssl_verify_server_cert(Vio *vio, const char *server_hostname,
are what we expect.
*/

/* Use OpenSSL certificate matching functions instead of our own if we
have OpenSSL. The X509_check_* functions return 1 on success.
*/
#if OPENSSL_VERSION_NUMBER >= 0x10002000L
/*
For OpenSSL 1.0.2 and up we already set certificate verification
parameters in the new_VioSSLFd() to perform automatic checks.
*/
ret_validation = 0;
#else /* OPENSSL_VERSION_NUMBER < 0x10002000L */
/*
OpenSSL prior to 1.0.2 do not support X509_check_host() function.
Use deprecated X509_get_subject_name() instead.
Expand Down Expand Up @@ -3460,14 +3451,14 @@ static int ssl_verify_server_cert(Vio *vio, const char *server_hostname,
/* Success */
ret_validation = 0;
}
#endif /* OPENSSL_VERSION_NUMBER >= 0x10002000L */

*errptr = "SSL certificate validation success";

error:
if (server_cert != nullptr) X509_free(server_cert);
return ret_validation;
}
#endif /* OPENSSL_VERSION_NUMBER < 0x10002000L */

/*
Note that the mysql argument must be initialized with mysql_init()
Expand Down Expand Up @@ -4161,20 +4152,6 @@ static int cli_establish_ssl(MYSQL *mysql) {
goto error;
}

/*
If the ssl_mode is VERIFY_CA or VERIFY_IDENTITY, make sure that the
connection doesn't succeed without providing the CA certificate.
*/
if (mysql->options.extension &&
mysql->options.extension->ssl_mode > SSL_MODE_REQUIRED &&
!(mysql->options.ssl_ca || mysql->options.ssl_capath)) {
set_mysql_extended_error(mysql, CR_SSL_CONNECTION_ERROR, unknown_sqlstate,
ER_CLIENT(CR_SSL_CONNECTION_ERROR),
"CA certificate is required if ssl-mode "
"is VERIFY_CA or VERIFY_IDENTITY");
goto error;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself already supports fallback to the system roots if nothing is given. So this check can be removed on its own. The reason for all the other changes is that providing the ca path or file was also used to infer the SSL verification mode, which is now made all explicit.

The fallback logic exists here:

mysql-server/vio/viosslfactories.cc

Lines 696 to 700 in beb865a

/* otherwise go use the defaults */
if (SSL_CTX_set_default_verify_paths(ssl_fd->ssl_context) == 0) {
*error = SSL_INITERR_BAD_PATHS;
goto error;
}

/*
Attempt SSL connection if ssl_mode != SSL_MODE_DISABLED and the
server supports SSL. Fallback on unencrypted connection otherwise.
Expand All @@ -4186,11 +4163,21 @@ static int cli_establish_ssl(MYSQL *mysql) {
struct st_mysql_options *options = &mysql->options;
struct st_VioSSLFd *ssl_fd;
enum enum_ssl_init_error ssl_init_error = SSL_INITERR_NOERROR;
#if OPENSSL_VERSION_NUMBER < 0x10002000L
const char *cert_error;
#endif
unsigned long ssl_error;
char buff[33], *end;
const bool verify_identity =
mysql->client_flag & CLIENT_SSL_VERIFY_SERVER_CERT;
int verify;
bool verify_identity;

if (options->extension && options->extension->ssl_mode >= SSL_MODE_VERIFY_CA) {
verify = SSL_VERIFY_PEER;
verify_identity = options->extension->ssl_mode == SSL_MODE_VERIFY_IDENTITY;
} else {
verify = SSL_VERIFY_NONE;
verify_identity = false;
}

/* check if server supports compression else turn off client capability */
if (!(mysql->server_capabilities & CLIENT_ZSTD_COMPRESSION_ALGORITHM))
Expand Down Expand Up @@ -4227,7 +4214,7 @@ static int cli_establish_ssl(MYSQL *mysql) {
options->extension ? options->extension->ssl_crl : nullptr,
options->extension ? options->extension->ssl_crlpath : nullptr,
options->extension ? options->extension->ssl_ctx_flags : 0,
verify_identity ? mysql->host : nullptr))) {
mysql->host, verify, verify_identity))) {
set_mysql_extended_error(mysql, CR_SSL_CONNECTION_ERROR, unknown_sqlstate,
ER_CLIENT(CR_SSL_CONNECTION_ERROR),
sslGetErrString(ssl_init_error));
Expand All @@ -4249,13 +4236,19 @@ static int cli_establish_ssl(MYSQL *mysql) {
}
DBUG_PRINT("info", ("IO layer change done!"));

/* OpenSSL 1.0.2 and up will use their own verification function.
* This is set up in new_VioSSLConnectorFd and adding here it here
* again is superfluous.
*/
#if OPENSSL_VERSION_NUMBER < 0x10002000L
/* Verify server cert */
if (verify_identity &&
ssl_verify_server_cert(net->vio, mysql->host, &cert_error)) {
set_mysql_extended_error(mysql, CR_SSL_CONNECTION_ERROR, unknown_sqlstate,
ER_CLIENT(CR_SSL_CONNECTION_ERROR), cert_error);
goto error;
}
#endif

MYSQL_TRACE(SSL_CONNECTED, mysql, ());
MYSQL_TRACE_STAGE(mysql, AUTHENTICATE);
Expand Down Expand Up @@ -4305,21 +4298,6 @@ static net_async_status cli_establish_ssl_nonblocking(MYSQL *mysql, int *res) {
goto error;
}

/*
If the ssl_mode is VERIFY_CA or VERIFY_IDENTITY, make sure
that the connection doesn't succeed without providing the
CA certificate.
*/
if (mysql->options.extension &&
mysql->options.extension->ssl_mode > SSL_MODE_REQUIRED &&
!(mysql->options.ssl_ca || mysql->options.ssl_capath)) {
set_mysql_extended_error(mysql, CR_SSL_CONNECTION_ERROR, unknown_sqlstate,
ER_CLIENT(CR_SSL_CONNECTION_ERROR),
"CA certificate is required if ssl-mode "
"is VERIFY_CA or VERIFY_IDENTITY");
goto error;
}

/*
Attempt SSL connection if ssl_mode != SSL_MODE_DISABLED and
the server supports SSL. Fallback on unencrypted
Expand Down Expand Up @@ -4366,11 +4344,21 @@ static net_async_status cli_establish_ssl_nonblocking(MYSQL *mysql, int *res) {
struct st_mysql_options *options = &mysql->options;
struct st_VioSSLFd *ssl_fd;
enum enum_ssl_init_error ssl_init_error;
#if OPENSSL_VERSION_NUMBER < 0x10002000L
const char *cert_error;
#endif
unsigned long ssl_error;
size_t ret;
const bool verify_identity =
mysql->client_flag & CLIENT_SSL_VERIFY_SERVER_CERT;
int verify;
bool verify_identity;

if (options->extension && options->extension->ssl_mode >= SSL_MODE_VERIFY_CA) {
verify = SSL_VERIFY_PEER;
verify_identity = options->extension->ssl_mode == SSL_MODE_VERIFY_IDENTITY;
} else {
verify = SSL_VERIFY_NONE;
verify_identity = false;
}

MYSQL_TRACE_STAGE(mysql, SSL_NEGOTIATION);

Expand All @@ -4385,7 +4373,7 @@ static net_async_status cli_establish_ssl_nonblocking(MYSQL *mysql, int *res) {
options->extension ? options->extension->ssl_crl : nullptr,
options->extension ? options->extension->ssl_crlpath : nullptr,
options->extension ? options->extension->ssl_ctx_flags : 0,
verify_identity ? mysql->host : nullptr))) {
mysql->host, verify, verify_identity))) {
set_mysql_extended_error(mysql, CR_SSL_CONNECTION_ERROR,
unknown_sqlstate,
ER_CLIENT(CR_SSL_CONNECTION_ERROR),
Expand Down Expand Up @@ -4427,14 +4415,19 @@ static net_async_status cli_establish_ssl_nonblocking(MYSQL *mysql, int *res) {
/* sslconnect creates a new vio, so update it. */
vio_set_blocking_flag(net->vio, !ctx->non_blocking);

/* OpenSSL 1.0.2 and up will use their own verification function.
* This is set up in new_VioSSLConnectorFd and adding here it here
* again is superfluous.
*/
#if OPENSSL_VERSION_NUMBER < 0x10002000L
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On OpenSSL 1.0.2 and newer, the additional identity check here is not needed since we already set it up inside new_VioSSLConnectorFd using the OpenSSL provided validation function. This means the check here was superfluous and would result in double checking the certificate.

If anything, it makes it more brittle because there could be implementation differences between these validation functions (such as honoring DNS SANs, using the deprecated CN or not etc).

/* Verify server cert */
if (verify_identity &&
ssl_verify_server_cert(net->vio, mysql->host, &cert_error)) {
set_mysql_extended_error(mysql, CR_SSL_CONNECTION_ERROR, unknown_sqlstate,
ER_CLIENT(CR_SSL_CONNECTION_ERROR), cert_error);
goto error;
}

#endif
MYSQL_TRACE(SSL_CONNECTED, mysql, ());
MYSQL_TRACE_STAGE(mysql, AUTHENTICATE);
}
Expand Down
3 changes: 2 additions & 1 deletion vio/test-ssl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ int main(int argc, char **argv) {
ssl_acceptor =
new_VioSSLAcceptorFd(server_key, server_cert, ca_file, ca_path, cipher);
ssl_connector = new_VioSSLConnectorFd(client_key, client_cert, ca_file,
ca_path, cipher, &ssl_init_error);
ca_path, cipher, NULL, &ssl_init_error,
NULL, NULL, 0, NULL, SSL_VERIFY_NONE, false);

client_vio = (Vio *)my_malloc(sizeof(Vio), MYF(0));
client_vio->sd = sv[0];
Expand Down
4 changes: 3 additions & 1 deletion vio/test-sslclient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ int main(int argc MY_ATTRIBUTE((unused)), char **argv) {
if (ca_path != 0) printf("CApath : %s\n", ca_path);

ssl_connector = new_VioSSLConnectorFd(client_key, client_cert, ca_file,
ca_path, cipher, &ssl_init_error);
ca_path, cipher, NULL, &ssl_init_error,
NULL, NULL, 0, NULL, SSL_VERIFY_NONE, false);

if (!ssl_connector) {
fatal_error("client:new_VioSSLConnectorFd failed");
}
Expand Down
5 changes: 5 additions & 0 deletions vio/viossl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,11 @@ static int ssl_do(struct st_VioSSLFd *ptr, Vio *vio, long timeout,
DBUG_PRINT("info", ("ssl: %p timeout: %ld", ssl, timeout));
SSL_clear(ssl);
SSL_SESSION_set_timeout(SSL_get_session(ssl), timeout);
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
if (ptr->hostname) {
SSL_set_tlsext_host_name(ssl, const_cast<char *>(ptr->hostname));
}
#endif
SSL_set_fd(ssl, sd);
#if defined(SSL_OP_NO_COMPRESSION)
SSL_set_options(ssl, SSL_OP_NO_COMPRESSION); /* OpenSSL >= 1.0 only */
Expand Down
24 changes: 12 additions & 12 deletions vio/viosslfactories.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,8 @@ static struct st_VioSSLFd *new_VioSSLFd(
const char *ca_path, const char *cipher,
const char *ciphersuites MY_ATTRIBUTE((unused)), bool is_client,
enum enum_ssl_init_error *error, const char *crl_file, const char *crl_path,
const long ssl_ctx_flags, const char *server_host MY_ATTRIBUTE((unused))) {
const long ssl_ctx_flags, const char *server_host,
bool verify_identity MY_ATTRIBUTE((unused))) {
DH *dh;
struct st_VioSSLFd *ssl_fd;
long ssl_ctx_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3;
Expand Down Expand Up @@ -633,6 +634,10 @@ static struct st_VioSSLFd *new_VioSSLFd(
return nullptr;
}

if (server_host) {
ssl_fd->hostname = my_strdup(PSI_NOT_INSTRUMENTED, server_host, MYF(0));
}

#ifdef HAVE_TLSv13
/*
Set OpenSSL TLS v1.3 ciphersuites.
Expand Down Expand Up @@ -758,7 +763,7 @@ static struct st_VioSSLFd *new_VioSSLFd(
If server_host parameter is set it contains either IP address or
server's hostname. Pass it to the lib to perform automatic checks.
*/
if (server_host) {
if (verify_identity && server_host) {
X509_VERIFY_PARAM *param = SSL_CTX_get0_param(ssl_fd->ssl_context);
assert(is_client);
/*
Expand All @@ -784,6 +789,7 @@ static struct st_VioSSLFd *new_VioSSLFd(
error:
DBUG_PRINT("error", ("%s", sslGetErrString(*error)));
report_errors();
my_free(const_cast<char *>(ssl_fd->hostname));
SSL_CTX_free(ssl_fd->ssl_context);
my_free(ssl_fd);
return nullptr;
Expand All @@ -795,19 +801,12 @@ struct st_VioSSLFd *new_VioSSLConnectorFd(
const char *key_file, const char *cert_file, const char *ca_file,
const char *ca_path, const char *cipher, const char *ciphersuites,
enum enum_ssl_init_error *error, const char *crl_file, const char *crl_path,
const long ssl_ctx_flags, const char *server_host) {
const long ssl_ctx_flags, const char *server_host, int verify, bool verify_identity) {
struct st_VioSSLFd *ssl_fd;
int verify = SSL_VERIFY_PEER;

/*
Turn off verification of servers certificate if both
ca_file and ca_path is set to NULL
*/
if (ca_file == nullptr && ca_path == nullptr) verify = SSL_VERIFY_NONE;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer infer this value, but it gets passed in explicitly by the caller based on the SSL mode that is selected. This means the SSL mode flag is leading and we fall back already automatically to the system roots.


if (!(ssl_fd = new_VioSSLFd(key_file, cert_file, ca_file, ca_path, cipher,
ciphersuites, true, error, crl_file, crl_path,
ssl_ctx_flags, server_host))) {
ssl_ctx_flags, server_host, verify_identity))) {
return nullptr;
}

Expand All @@ -828,7 +827,7 @@ struct st_VioSSLFd *new_VioSSLAcceptorFd(
int verify = SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE;
if (!(ssl_fd = new_VioSSLFd(key_file, cert_file, ca_file, ca_path, cipher,
ciphersuites, false, error, crl_file, crl_path,
ssl_ctx_flags, nullptr))) {
ssl_ctx_flags, nullptr, false))) {
return nullptr;
}
/* Init the the VioSSLFd as a "acceptor" ie. the server side */
Expand All @@ -849,6 +848,7 @@ struct st_VioSSLFd *new_VioSSLAcceptorFd(
}

void free_vio_ssl_acceptor_fd(struct st_VioSSLFd *fd) {
my_free(const_cast<char *>(fd->hostname));
SSL_CTX_free(fd->ssl_context);
my_free(fd);
}
3 changes: 2 additions & 1 deletion vio/viotest-ssl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ int main(int argc, char **argv) {
ssl_acceptor =
new_VioSSLAcceptorFd(server_key, server_cert, ca_file, ca_path);
ssl_connector = new_VioSSLConnectorFd(client_key, client_cert, ca_file,
ca_path, &ssl_init_error);
ca_path, NULL, &ssl_init_error, NULL,
NULL, 0, SSL_VERIFY_NONE, false);

client_vio = (Vio *)my_malloc(sizeof(Vio), MYF(0));
client_vio->sd = sv[0];
Expand Down