-
-
Notifications
You must be signed in to change notification settings - Fork 178
add basic support for SSL connections with CA cert validation #179
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
Conversation
0531daa
to
2dff491
Compare
roots := x509.NewCertPool() | ||
ok := roots.AppendCertsFromPEM([]byte(c.opts.CaCert)) | ||
if !ok { | ||
panic("failed to parse root certificate") |
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 think it would make more sense for an error to be returned here instead of panicking, what do you think?
This looks pretty great thanks! I have put a couple of comments mostly regarding code style however I was wondering why you have chosen to manually pass the certificate as a string instead of passing the path to a certificate? |
Thanks for the great feedback. I've made the suggested changes and let me know if there's anything else you'd like to see adjusted. re: passing certificate as a string, I figured it's best for the driver to not force where the certificate is located (i.e. path to file) to allow for applications to load the certificate from an environment variable or other service. If it's what you'd like to see (file path instead of string), I'll make the change. |
Thanks for making those changes. Regarding how the certificate is passed I am not convinced that either option is the best but I think we should figure out what option provides the most flexibility. I have had a look at other libraries to see what they do and have quite a few different methods. For example:
What do you think about doing something like the mysql driver by adding the TLSConfig to the connection options (something like this https://gist.github.com/dancannon/140ae1e08a6e6ac2535e). This would give full control to anybody using the driver. The issue with this approach is that setting up a gorethink session using TLS would require significantly more code. PS. as mentioned earlier I dont think there is anything wrong with your approach I would just like to discuss all options :) |
After looking at the mysql driver implementation and also the MongoDB one mgo, I agree it would be best to abstract the TLSConfig out more. This will effectively let people configure the driver for all forms of SSL communication and the driver can just use whatever it's given. I like what you have proposed in the gist so I'll pull that over to my branch and push up the changes shortly. |
Thanks @jipperinbham, ill merge this in the next release (v0.7.2) |
👍 |
add basic support for SSL connections with CA cert validation
adds an ssl flag to the connectopts and a cacerts option. In the connection class, it now checks for the SSL flag and build the necessary cert info and calls tls.DialWithDialer.
For now, the SSL support is only good for encrypting traffic and verifying the CA certification to prevent MITM attacks.