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

Implement SSL Mode #88

Closed
caleblloyd opened this issue Oct 5, 2016 · 9 comments
Closed

Implement SSL Mode #88

caleblloyd opened this issue Oct 5, 2016 · 9 comments
Assignees
Milestone

Comments

@caleblloyd
Copy link
Contributor

From the connection string reference:

SSL Mode, SslMode

None - do not use SSL.
Preferred - use SSL if the server supports it, but allow connection in all cases.
Required - Always use SSL. Deny connection if server does not support SSL.
VerifyCA - Always use SSL. Validate the CA but tolerate name mismatch.
VerifyFull - Always use SSL. Fail if the host name is not correct.

I think this is a needed feature for 1.0 since many companies require SSL/TLS. Also, some cloud MySQL providers such as Google Cloud SQL run over the public network so they pretty much require SSL/TLS.

A solution would most likely use the SslStream library

Certificate validation based off the SSL Mode options would be performed in the RemoteCertificateValidationCallback, checking the SslPolicyErrors

I don't fully understand why someone would want to use the SSL Mode=Preferred option. It seems to me like this would mask a server misconfiguration. This may be an option that was added to support the legacy Encrypt, UseSSL connection string options. I think we should consider not implementing SSL Mode=Preferred.

@caleblloyd
Copy link
Contributor Author

caleblloyd commented Oct 11, 2016

Working branch at caleblloyd@5d5c0e9

How should we test this? Just a small test file that connects and runs a query? Or something more?

@bgrainger
Copy link
Member

My hope would be that we could re-run the full test suite with every combination of SslMode=Required/None, UseCompression=true/false (maybe ForceSynchronous=true/false). I'm not sure yet if xUnit has a way to re-run an entire test fixture with different settings; maybe CI would have to run dotnet test multiple times with different environment variables? I haven't dug into this yet.

@caleblloyd
Copy link
Contributor Author

caleblloyd commented Oct 11, 2016

every combination of SslMode=Required/None, UseCompression=true/false (maybe ForceSynchronous=true/false)

I agree, we should test all 3.

I'll take a shot at converting the test to use a config.json connection string (that is gitignored, like in MySqlConnector.Performance tests). This will have configurable user variables:

  • Host
  • Port
  • Normal User and Password
  • SSL User and Password (if this is not set, SSL Tests will not run, such as on AppVeyor)
  • Passwordless User
  • Schema? (see below)

What do you think about using a single schema for the tests? And drop the schema on test startup? This would allow somebody with an existing instillation of MySQL to add our tests to their instillation pretty easily.

The last part will be to write a test.sh and test.ps1 script that cycles through the combinations of SSL, Compression, and ForceSynchronous and runs dotnet test.

@bgrainger
Copy link
Member

I'll take a shot at converting the test to use a config.json connection string

👍

Another benefit would be being more easily able to test MariaDB 10.1 or MySQL Server 5.6 (I believe you've mentioned that in the past).

What do you think about using a single schema for the tests?

Sounds good; are you thinking of creating a .sql file that could be piped to mysql before each test run?

If it were schemas, we could load core.sql and mysql-5.7.sql for JSON tests against MySQL 5.7, just core.sql for MariaDB 10.1, etc.

@caleblloyd
Copy link
Contributor Author

SSL WIP is passing with the refactored tests

@bgrainger
Copy link
Member

👍

I want to land the refactored serialization PR soon. (Spent too much time trying to understand the compression protocol, make sure compression would work in the new API (before finalizing it) and ended up reporting bugs in MySql.Data...)

@caleblloyd
Copy link
Contributor Author

How do you want me to handle the SSL Implementation PR? Do you want a PR to the refactor-serialization branch? Or wait for #93 to land, then open a PR to master?

@bgrainger
Copy link
Member

I was just thinking... the new code's not perfect but it's better than what's already on master. I'll push it, so you're unblocked. (If I refactor the interfaces later, that won't affect very much of the new SSL code.)

@caleblloyd caleblloyd mentioned this issue Oct 12, 2016
bgrainger added a commit that referenced this issue Oct 12, 2016
@bgrainger
Copy link
Member

Shipped in 0.3.0.

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

No branches or pull requests

2 participants