Add a client parameter to specify SSL version(s). #450
Conversation
Prior to this patch, TLS1.2 was always disabled on Windows client, and enabled elsewhere, but this was not correct. The ability to use TLS1.2 depends entirely on the way how *server* was built. If server was built with YASSL, TLS1.2 negotation would fail due to YaSSL bug described here https://jira.mariadb.org/browse/MDEV-12190 (no graceful version downgrade) If the server was built with OpenSSL, TLS1.2 negotiation would succeed, or gracefully downgrades for older versions of OpenSSL. FIX: This patch allows users to specify the SSL version(s) in the connection string with SslProtocols parameter. Default value for it, which works with all versions of server, is TLS|TLS11. User can overwrite it with e.g TLS12, if required.
| @@ -347,6 +355,10 @@ static MySqlConnectionStringOption() | |||
| keys: new[] { "SSL Mode", "SslMode" }, | |||
| defaultValue: MySqlSslMode.Preferred)); | |||
|
|
|||
| AddOption(SslProtocols = new MySqlConnectionStringOption<SslProtocols>( | |||
| keys: new[] { "SSL Protocols", "SslProtocols" }, | |||
| defaultValue: System.Security.Authentication.SslProtocols.Tls | System.Security.Authentication.SslProtocols.Tls11)); | |||
There was a problem hiding this comment.
This should be as secure as possible by default, and include Tls12 on non-Windows
There was a problem hiding this comment.
Windows, or OS where client runs, is not important. How server was compiled is important.
There was a problem hiding this comment.
why o
8000
n non-Windows? Client OS is not important, it is not where the bug comes from.
Server OS is somewhat-important . On Linux, distributions always use openssl, but the tarballs usually use Yassl. Windows was traditionally compiled with unpatched Yassl containing the bug, although MySQL Enterprise Windows uses openssl, recent MariaDB fixes the yassl negotiation bug, and not-yet-released MySQL 8.0 ditched Yassl in its packaging.
|
Some historical reference here: GitHub discussion: #101 (comment) and associated discussion in a MySQL bug The problem does lie within YaSSL and that is typically what MySQL Community Edition is compiled with. The connection issue is only a problem on windows because of the way that Schannel does TLS negotiation, however. The default for this new option should be as secure where possible but also account for the majority of use cases. I believe the default should be |
|
I agree that supporting TLS 1.2 by default is important, particularly now that MySQL Server 8.0 is built with OpenSSL in all configurations. (Our previous fix was based on the assumption that TLS 1.2 wasn't widespread: #101 (comment).) I'm not sure that forcing users to opt in/out of a TLS version is the right approach. (It's either going to be broken by default on Windows, which will confuse/frustrate users, or insecure by default on Linux/with MySQL 8/MariaDB.) I'd rather take an approach where the user doesn't have to configure the library's behaviour; I think we could address this by trying TLS 1.2 first, then falling back to try TLS 1.1 if a) we're on Windows; and b) the server version is 5.7.x. (Or maybe we just check ServerVersion first and fall back to TLS 1.1 if we think it isn't going to work.) Is there a minimum MariaDB version where TLS 1.2 is negotiated correctly, that we could test |
|
A hacky example of what I'm proposing: bgrainger@f22cf53 (Needs to remove the |
|
MariaDB fixes YASSL in 10.2.6, MySQL gets rid of YASSL in 8.0. I'm not sure this library is able to parse MariaDB version correctly, I have not checked (it appears to be prefixed by 5.5.5- prefix, long story why, but this will be kept). I do not mind if this MySqlConnector changes defaults from the one I proposed, as long as it is able to handle them correctly and gracefully ( but correctly means to me not derivíng defaults from client OS, this only makes sense when connecting to localhost). The default I propose (1.0 and 1.1) is the one that is also found elsewhere, e.g in MariaDB JDBC driver. It is important to make SslProtocols configurable, and allow previously disabled TLS1.2 on Windows client. |
|
@caleblloyd , Schannel is not guilty here. Yassl is, we have debugged this, it does not downgrade from 1.2 to 1.1 as per spec. Schannel has some other bugs,sporadic failures related to ticket caching, something fixed back in 2016 in Windows. but then again it is not related to TLS1.2 . |
|
@vaintroub the issue that we saw was tested against the Linux Docker Container for MySQL 5.7 from a Linux Client and a Windows Client, see #101 (comment) The Windows Client fails to negotiate down to TLS 1.1 with YaSSL when The Linux Client successfully negotiates down to TLS 1.1 with YaSSL when That is the reason we initially made the decision to disable |
According to #101 (comment) Linux SSL implementation is able to negotiate TLS, even if server is on Yassl.
|
On a Windows client, Schannel uses |
|
Clicked "Close" by mistake. |
|
@caleblloyd : interesting, thanks for the link. We knew that Schannel does not work with Yassl, but also Java's SSL would break. Apparently there are implementations that work better. |
|
Any comments on 4ba5dae ? this addresses @caleblloyd concern. As for try-and-fails a la bgrainger@f22cf53 , or checking server versions, to me it seems like it adds more complication. If the future is OS independend TLS implementation, then all hacks will still have to be rewritten, or removed. |
Can you explain why? (I want to make sure I'm not misunderstanding.)
Yes it adds more complication to the implementation, but it makes the library easier to use for clients. I'm extremely reluctant to push the burden of understanding which TLS protocols to use onto library consumers. It should "just work". Making (or allowing) users to specify |
|
First of all, some customer, especially in security sensitive environments, want to have more control over TLS version. They might not want a TLS1.0 or TLS1.1 fallback, but specifically would like to force TLS1.2 with a switch, Why it is hard to add TLS1.3 in the future? Whenever .NET supports it , it can be easily add to (currently non-Windows) default. Right now, there is no need to change connection strings, and the behavior with the new variable is exactly the same as before introduction of this variable, the only difference that it brings is that TLS1.2 now possible on Windows, and it was not before Try-and-fail is not everyone's choice. Exactly in the environments where security (and thus TLS) matters ,failed connection attempts are likely to be logged into audit or error log, and a large number of those will annoy DBAs. |
If this is important, it should be configured on the server, not by changing client connection strings.
Once people start adding
I'm sympathetic to this, but not convinced yet that auditing of failed TLS handshakes (it doesn't even really get far enough to be a failed connection attempt) is a serious problem. (I don't even know if a failed TLS handshake is logged.) I'm still optimistic that the library can do the right thing (the first time, and not have to "try and fail") by examining the |
|
I doubt you can solve it with ServerVersion, because it is not really solvable. You can restrict Windows client on TLS1.0/TLS1.2 , based on version heuristics. But, unless you're connecting to localhost, you do not know whether the peer is using OpenSSL, or YASSL. You get MySQL 5.7, or MariaDB 10.1 version as peer. If you pessimistically assume YASSL, and the server appears to use OpenSSL, which is on overwhelming majority of Linux deployments. The client will have to use downlevel TLS version, and there is no way to say "but I want TLS1.2". |
This is a fair point; the server version does not tell how the server was compiled. |
|
Microsoft just published guidance today where they recommend that you "Do not specify the TLS version." https://docs.microsoft.com/en-us/dotnet/framework/network-programming/tls This furthers my objection to making the TLS protocol configurable through the connection string (and also makes me realise that we should start with |
|
Right, but you existing code specifies TLS version by hardcoding it, and making it impossible to overwrite the hardcoded allowed versions. So, we do have a Windows user now, who needs TLS1.2, uses Linux server-side, does use Windows client-sie, and does use MariaDB 10.0. Connector/NET is not something I can recommend, it sticks with TLS1.0. MySqlConnector is not something that would do either. MS recommendations do not help them much either. |
The existing code is wrong and needs to be fixed; no disagreement there. |
|
I'm hoping to ship an update that improves the situation soon. (I'm willing to make the "Windows client connecting to yaSSL-based MySQL" scenario worse in order to improve the "Linux or Windows client connecting to MariaDB or OpenSSL-based MySQL" scenario better.) |
What would the fix be for a Windows Client that is currently connecting over TLS to a YaSSL-based MySQL Server? They would either need to be able to
I'm also not a fan of implementing (1) for the reasons that Microsoft is recommending, so I guess that would leave (2) |
|
I see three options:
|
|
I've pushed my proposal for fixing this problem: #458. |
|
Closing this in favour of #458. |
Prior to this patch, TLS1.2 was always disabled on Windows client, and
enabled elsewhere, but this was not correct.
The ability to use TLS1.2 depends entirely on the way how server was
built.
If server was built with YASSL, TLS1.2 negotation would fail due to YaSSL
bug described here https://jira.mariadb.org/browse/MDEV-12190
(no graceful version downgrade)
If the server was built with OpenSSL, TLS1.2 negotiation would succeed,
or gracefully downgrades for older versions of OpenSSL.
FIX:
This patch allows users to specify the SSL version(s) in the connection
string with SslProtocols parameter. Default value for it,
which works with all versions of server, is TLS|TLS11.
User can overwrite it with e.g TLS12, if required.