8000 SSL Support by caleblloyd · Pull Request #101 · mysql-net/MySqlConnector · GitHub
[go: up one dir, main page]
Skip to content

SSL Support #101

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

Jump to bottom
Merged
merged 4 commits into from
Oct 12, 2016
Merged

SSL Support #101

merged 4 commits into from
Oct 12, 2016

Conversation

caleblloyd
Copy link
Contributor
  • Fixes Implement SSL Mode #88
  • Adds IPayloadHandler.SetByteHandler and IPacketHandler.SetByteHandler methods to their respective interfaces. Needed to switch the underlying byte handler to SSL once the TLS handshake has been completed
  • Uses TcpClient instead of Socket in MySqlSession
    • Unencrypted communication still happens at the Socket level using TcpClient.Client, SocketByteHandler, and SocketAwaitable
    • Encrypted communication happens at the NetworkStream level using TcpClient.GetStream(), SslByteHandler, and the appropriate stream methods
  • Adds a second test run to TravisCI with ssl mode=required

@bgrainger
Copy link
Member

I'm getting the following exception under dotnet test on Windows:

    SideBySide.ConnectionPool.ResetConnection(connectionReset: False, poolSize: 11, expected: 0) [FAIL]
      System.IO.IOException : The handshake failed due to an unexpected packet format.
      Stack Trace:
           at System.Net.Security.SslState.StartReadFrame(Byte[] buffer, Int32 readBytes, AsyncProtocolRequest asyncRequest)
           at System.Net.Security.SslState.PartialFrameCallback(AsyncProtocolRequest asyncRequest)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Net.Security.SslState.InternalEndProcessAuthentication(LazyAsyncResult lazyResult)
           at System.Net.Security.SslState.EndProcessAuthentication(IAsyncResult result)
           at System.Net.Security.SslStream.EndAuthenticateAsClient(IAsyncResult asyncResult)
           at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization)

(I think my local MySQL server is configured correctly because I'm able to connect with mysql.exe --ssl-mode=REQUIRED (other ssl options) -u ssltest. I'll debug this further to try to figure out what's happening.)

@caleblloyd
Copy link
Contributor Author

I'm seeing this also, running from Windows against the same MySQL database that is passing on linux, so I don't think it's your server configuration. Let me try to get some decrypted packet captures.

caleblloyd
caleblloyd commented Oct 12, 2016
View reviewed changes
src/MySqlConnector/Serialization/MySqlSession.cs
new RemoteCertificateValidationCallback(remoteCertificateCb),
new LocalCertificateSelectionCallback(localCertificateCb));
var clientCertificates = new X509CertificateCollection { certificate };
var sslProtocols = SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the error on Windows is coming from. My MySQL server only speaks TLS1.1 but the handshake is trying to initiate with TLS1.2. Need to look into how to handle this properly. Changing to var sslProtocols = SslProtocols.Tls | SslProtocols.Tls11 on Windows works, but is not the correct solution.

@caleblloyd
Copy link
Contributor Author

MySQL doesn't like the ClientHello packet sent from windows when TLSv1.2 is enabled. MySQL is responding to the ClientHello packet with "#08S01 Bad Handshake" and closing the TCP connection. Here are the packet captures:

tls-traces.zip

I don't know if this is an issue with the ClientHello that windows is sending or an issue with MySQL, so I'm not sure who to open the upstream bug to.

In the short-term, our options are to:

  1. Only enable TLS1 and TLS1.1 across all platforms
  2. Enable TLS1 and TLS1.1 on Windows and enable TLS1, TLS1.1, and TLS1.2 on all other platforms

@bgrainger
Copy link
Member
bgrainger commented Oct 12, 2016 •
edited
Loading

I'm not very familiar with the specifics of TLS negotiation, but here's what I noticed when looking at the captures (thanks for grabbing them!):

  • Linux sends a TLS 1.0 record layer containing a TLS 1.2 client hello; MySQL negotiates down to TLS 1.1 in its server hello
  • Windows sends a TLS 1.2 record layer containing a TLS 1.2 client hello; MySQL doesn't recognise this.

image

My assumption would be that if your protocols are SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12, it should negotiate with the lowest-common-denominator, which doesn't appear to be happening. That makes me think the Windows ClientHello is the problem (but I'd really have to read the specs to confirm).

One other option would be to catch this exception and then renegotiate with just Tls11. However, there doesn't seem to be a lot of point to doing that unless we find a some combination of client and server that works; there's no value in adding a failing SSL handshake to every single connection.

Enable TLS1 and TLS1.1 on Windows and enable TLS1, TLS1.1, and TLS1.2 on all other platforms

This seems like the best approach. TLS 1.2 wasn't available in MySQL Server until 5.7.10 (and isn't in Community Server) so it may not even be widely deployed. (This might become an issue if cloud MySQL providers started requiring TLS 1.2, but I haven't found any indication that this might happen after a quick search.)

@bgrainger
Copy link
Member

From RFC 5246 Appendix E:

Earlier versions of the TLS specification were not fully clear on what the record layer version number (TLSPlaintext.version) should contain when sending ClientHello (i.e., before it is known which version of the protocol will be employed). Thus, TLS servers compliant with this specification MUST accept any value {03,XX} as the record layer version number for ClientHello.

TLS clients that wish to negotiate with older servers MAY send any value {03,XX} as the record layer version number. Typical values would be {03,00}, the lowest version number supported by the client, and the value of ClientHello.client_version. No single value will guarantee interoperability with all old servers, but this is a complex topic beyond the scope of this document.

It appears that the client code is permitted to send "TLS 1.2" as its record layer version number (as Windows does). However, it's also noted that this doesn't "guarantee interoperability".

Certainly it appears that MySQL Server is not "compliant with this specification" because it doesn't accept any value as the record layer version number.

@bgrainger
Copy link
Member

Neither the Windows .NET client nor MySQL Server are following these recommendations; this combination is breaking protocol negotiation.

@bgrainger
Copy link
Member

Here's someone reporting essentially the same issue in Schannel (the native Windows SSL provider), but the problem doesn't seem to have been understood on the Microsoft side and no resolution was posted.

@caleblloyd
Copy link
Contributor Author

Very nice find. While the Schannel side isn't a bug, it's still not following a best practice. I'm not sure if it's worth trying to send them a bug report. The MySQL side is definitely a bug and should be reported upstream. I can report it or you can - let me know.

Working on disabling TLS 1.2 on windows now. Getting the runtime information is turning out to be a pretty difficult task, this is the first language I've dealt with where it's not a one-liner.

@bgrainger
Copy link
Member

This potentially problematic behaviour in Schannel was reported five years ago; I'm not sure we're going to see it changed any time soon...

I wouldn't be surprised if there would be significant interoperability problems caused by changing it; presumably Microsoft has good reasons for keeping it the way it is.

@bgrainger
Copy link
Member

Getting the runtime information is turning out to be a pretty difficult task

I did not know (until now) that they took away System.Environment.OSVersion in .NET Core. The workarounds seem unnecessarily complicated...

@caleblloyd
Copy link
Contributor Author

I ended up importing System.Runtime.InteropServices.RuntimeInformation in both Netstandard 1.3 and Net45 so we'd have a common runtime information API

Tests are passing for me from Windows after the latest commit

@bgrainger bgrainger merged commit 8477ec6 into mysql-net:master Oct 12, 2016
@caleblloyd
Copy link
Contributor Author

TLS 1.2 bug opened upstream to MySQL at http://bugs.mysql.com/bug.php?id=83347

@caleblloyd caleblloyd deleted the f_ssl2 branch November 13, 2016 16:20
@bgrainger bgrainger mentioned this pull request Jan 24, 2018
Closed
@caleblloyd caleblloyd mentioned this pull request Mar 13, 2018
Closed
@bgrainger bgrainger mentioned this pull request Mar 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@caleblloyd @bgrainger
0