-
Notifications
You must be signed in to change notification settings - Fork 338
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
Add CA Certificate validation #280
Conversation
caleblloyd
commented
Jun 22, 2017
- Fixes Specify Acceptable Server CA / Expose SslStream? #278
- Please review for technical issues. I took most of the implementation from the two SO answers here. If there is a better way to validate certificate chains, comment away.
- I've hand tested; need to write Unit tests for this before it is ready to be merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert the MySqlConnectionStringBuilder API changes.
The certificate checking looks OK, but I've never used those APIs before so it's hard to know if I'm overlooking anything.
@@ -54,16 +54,22 @@ public MySqlSslMode SslMode | |||
set => MySqlConnectionStringOption.SslMode.SetValue(this, value); | |||
} | |||
|
|||
public string CertificateFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though the old names are bad and confusing, we should keep them for compatibility with Connector/Net.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yea, forgot this class was Public. I'll change it back.
AddOption(CertificateFile = new MySqlConnectionStringOption<string>( | ||
keys: new[] { "CertificateFile", "Certificate File" }, | ||
AddOption(ClientCertificate = new MySqlConnectionStringOption<string>( | ||
keys: new[] { "ClientCertificate", "Client Certificate", "CertificateFile", "Certificate File" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it's not a breaking change to add ClientCertificate
as an alias, I'm not sure there's real value in doing so. Since we need to keep CertificateFile
as the C# property name, might as well just be consistent with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got 2 things named Certificate
now, a Client Certificate
and a CA Certificate
. I think an alias could help users qualify which one they're dealing with.
public string CertificatePassword { get; } | ||
public string ClientCertificate { get; } | ||
public string ClientCertificatePassword { get; } | ||
public string CACertificate { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with renaming the properties here (on our internal class) to make it clear what each one stores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I wonder if ClientCertificateFilePath
would be better for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, adding FilePath
makes sense
if (cs.SslMode == MySqlSslMode.Preferred || cs.SslMode == MySqlSslMode.Required) | ||
return true; | ||
|
||
if ((rcbPolicyErrors & SslPolicyErrors.RemoteCertificateChainErrors) > 0 && cs.CACertificate != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I would write != 0
for testing a bitflag. (> 0
first made me think it was a count of errors, not testing a flag.)
if (!File.Exists(cs.CACertificate)) | ||
throw new MySqlException("Cannot find CA Certificate File", ex); | ||
throw new MySqlException("The CA Certificate File is invalid", ex); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if constructing caCertificate
should be done outside the callback. I also wonder if any MySqlException
that is thrown gets passed through, or would it end up being wrapped by, say, a CryptographicException
? (I haven't tested to see.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably should. There's an extremely rare case where the CA was already validated and this construction gets skipped completely, but in that case they wouldn't need to supply a CA certificate at all. I'll move it out.
I reverted all of the naming changes and am simply calling the new option |
@@ -526,47 +526,81 @@ private void VerifyConnected() | |||
|
|||
private async Task InitSslAsync(ProtocolCapabilities serverCapabilities, ConnectionSettings cs, IOBehavior ioBehavior, CancellationToken cancellationToken) | |||
{ | |||
X509CertificateCollection clientCertificates = null; | |||
X509CertificateCollection CertificateFiles = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still only contains (a single) client certificate, correct? If so, I think the old name of clientCertificates
is clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang find/replace got me here
if (cs.CertificatePassword == null) | ||
certificate = new X509Certificate2(cs.CertificateFile); | ||
else | ||
certificate = new X509Certificate2(cs.CertificateFile, cs.CertificatePassword); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this if
/else
necessary? It looks like the old code worked to pass a potentially-null cs.CertificatePassword
as the second argument to the constructor, and I don't see any indication in the documentation that it can't be null
. In corefx, the single-argument constructor just seems to pass null
when it delegates to an overload.
So it feels like this could be simplified back to the old code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the CoreFX link. I wanted to make sure importing PEM encoded certs would work too and usually those aren't password protected. I'll change it back though since it runs through the same codepath.
} | ||
} | ||
|
||
Func<object, string, X509CertificateCollection, X509Certificate, string[], X509Certificate> localCertificateCb = | ||
(lcbSender, lcbTargetHost, lcbLocalCertificates, lcbRemoteCertificate, lcbAcceptableIssuers) => lcbLocalCertificates[0]; | ||
X509Certificate LocalCertificateCb(object lcbSender, string lcbTargetHost, X509CertificateCollection lcbLocalCertificates, X509Certificate lcbRemoteCertificate, string[] lcbAcceptableIssuers) => lcbLocalCertificates[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local function 👍
|
||
SslStream sslStream; | ||
if (clientCertificates == null) | ||
if (CertificateFiles == null || CertificateFiles.Count == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how CertificateFiles.Count == 0
would happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This always-false condition should be removed, or line 641 should be updated with equivalent logic if it can happen (I lean towards the former).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, shouldn't happen. It just doesn't feel right referring to the element [0] without checking the count, but it has worked fine without this check so far. I'll take it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New code LGTM. As noted, I think there are some unnecessary changes from the current code that could be reverted before this is merged.
I'll fix the things highlighted, thanks. I also need to add a handful of unit tests to test the CA validation and also check operation with PEM encoded certs. |
b2e3bb0
to
1c4c534
Compare
@bgrainger I've made the changes and added tests. This should be ready. |
Also I ran the SSL tests from Windows to a SSL Enabled MySQL server (since Appveyor doesn't have SSL enabled). They all passed, both our tests and the |
Shipped in 0.22.0. |