Skip to content
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.

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

Use the best TLS version by default #458

Merged
merged 1 commit into from
Mar 20, 2018
Merged

Conversation

bgrainger
Copy link
Member

Specifying SslProtocols.None allows the .NET Framework to choose the best TLS version supported by the OS (on .NET 4.7 and later).

However, negotiating TLS 1.2 with a Windows Schannel client against a yaSSL-based MySQL Server will fail (see #101); automatically try with a lower TLS version if it's possible we're in this scenario.

This replaces #450, which moved specifying the TLS version into a connection string setting. In this PR, the library tries to do the best thing possible, at the cost (for Windows clients only) of extra TLS connection time for MySQL < 8.0 and/or .NET Framework < 4.7.


// MySQL Server 8.0 and MariaDB are compiled with OpenSSL, which negotiates TLS 1.2 correctly; a failure should
// not cause an automatic retry with an older/weaker TLS version.
var serverHasOpenSsl = shouldRetrySsl && (ServerVersion.Version.Major >= 8 || ServerVersion.OriginalString.IndexOf("MariaDB", StringComparison.OrdinalIgnoreCase) != -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

MariaDB is not compiled with openssl on Windows, not yet at least. MariaDB (currently only in 10.2.6+ , and in the future in other versions) fixes YASSL's TLS version negotiation, so that it downgrades to TLS1.2 that schannel sends, to TLS 1.1 without fatal error.

Copy link
Contributor

Choose a reason for hiding this comment

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

MySQL is compiled with OpenSSL almost everywhere, too. But not on Windows prior to 8.0, not in tar.gz packages, and maybe in 1-2 other obscure cases.

Copy link
Member Author

@bgrainger bgrainger Mar 16, 2018

Choose a reason for hiding this comment

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

Thanks for the correction; you already described the MariaDB/yaSSL situation in #450 but I didn't follow it carefully enough. I need to fix the name/description of this variable at a minimum, or maybe rework this section.

RE: MySQL

MySQL Community Edition binary distributions are compiled using OpenSSL. (Prior to MySQL 8.0.4, MySQL Community Edition binary distributions are compiled using yaSSL.)

https://dev.mysql.com/doc/refman/8.0/en/openssl-versus-yassl.html

I don't know if it's "obscure" but the Docker Hub mysql:5.7 image appears to be built with yaSSL. Edit: mysql:5.7 images on Docker appear to only support TLS 1.1, which makes me think they use yaSSL (because 5.7.10 should be able to support TLS 1.2 if compiled with OpenSSL), but I don't know for sure how they're compiled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me clarify the OpenSSL statement. Oracle MySQL, or MariaDB, both provide tar.gz or zip/MSI for downloads, yet outside Windows, those packages are not used much.

Instead, Linux/BSD distribution packagers would build MySQL or MariaDB the way they think is correct, and this normally means openssl, since packagers prefer better known libraries over less known, and shared libraries over compiled-in static ones.

As for docker, you can make a good guess how it was compiled by using "Show session status like 'Ssl_cipher_list' " as described in https://dev.mysql.com/doc/refman/5.7/en/encrypted-connection-protocols-ciphers.html . Yassl list is short, openssl is much larger.

Pool.SslProtocols = sslProtocols;
}
#endif
catch (Exception ex) when (shouldRetrySsl && serverHasOpenSsl && ((ex is MySqlException && ex.InnerException is IOException) || ex is IOException))
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic is reversed; it was supposed to be !serverHasOpenSsl.

This code fails when tested against a local instance of MySQL Server 5.7.21 for Windows (but apparently not on Appveyor?).

Specifying SslProtocols.None allows the .NET Framework to choose the best TLS version supported by the OS (on .NET 4.7 and later).

However, negotiating TLS 1.2 with a Windows Schannel client against a yaSSL-based MySQL Server will fail; automatically try with a lower TLS version if it's possible we're in this scenario.
@bgrainger
Copy link
Member Author

I'm reverting the ServerVersion-based check for now. It was intended to avoid a TLS downgrade (and extra server roundtrip) for servers that the client believes should negotiate TLS 1.2 correctly without an exception. However, it's not clear that the check covers enough situations to be useful. Additionally, it seems like an attacker who can modify traffic to force a TLS downgrade could modify the server's initial handshake to report a lower ServerVersion and bypass this check.

@bgrainger bgrainger merged commit 0f50925 into mysql-net:master Mar 20, 2018
@bgrainger bgrainger deleted the tls branch March 20, 2018 05:55
@bgrainger
Copy link
Member Author

Added in 0.37.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants