-
Notifications
You must be signed in to change notification settings - Fork 3
ING-662: Add support for client cert authentication #35
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
routingconn.go
Outdated
|
|
||
| if opts.RootCAs != nil || opts.InsecureSkipVerify { | ||
| creds := credentials.NewTLS(&tls.Config{InsecureSkipVerify: opts.InsecureSkipVerify, RootCAs: opts.RootCAs}) | ||
| var certificates []tls.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.
Does cert auth actually require TLS enabled? This seems like it should be below where the username and password check is. Should probably also error if certificate and username/password are both set. We're still at 0.x version so we could actually introduce an Authenticator type and remove username/password and certificate - which prevents both from being set.
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.
Does cert auth actually require TLS enabled?
Yes because it's during the TLS handshake that the client and server exchange the certs.
Should probably also error if certificate and username/password are both set
When connecting to the cluster it's impossible to define a passwordAuthenticator and a certificateAuthenticator so unless there will be other users of gocbcoreps isn't another check for this redundant?
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.
When connecting to the cluster it's impossible to define a passwordAuthenticator and a certificateAuthenticator so unless there will be other users of gocbcoreps isn't another check for this redundant?
It's entirely possible that there will be in the future.
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.
At the moment if the user specifies a Certificate and no TLS options then they're possibly going to get some weird errors because we just won't set any authentication at all. That should probably also be an error case. We might also want to think about adding a invalid args error type so that callers can detect that.
I also wonder if we should simplify this by just not accepting Certificate, RootCAs and InsecureSkipVerify. Instead just accepting a tls.Config.
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.
Good point, I've changed it so that the cert is always added to the TLS creds, so that a client cert being passed with no TLS options will work if the user has added their CA cert to their system pool.
7325003 to
2717a06
Compare
2717a06 to
03e4482
Compare
ING-662: Simplify building of TLS credentials
03e4482 to
36e1378
Compare
No description provided.