Add support for 'prefer' TLS mode and make it default #179
Conversation
|
|
|
@DMickens I'd like to use the name |
connection.go
Outdated
| // Set default to tlsModeServerPrefer | ||
| sslFlag = tlsModeServerPrefer | ||
| // Set default to tlsModePrefer | ||
| sslFlag = tlsModeNone |
There was a problem hiding this comment.
I changed this back to 'tlsModeNone' as it was initially which is an appropriate logic. This change made all the workflow tests pass.
connection.go
Outdated
| @@ -676,6 +677,9 @@ func (v *connection) initializeSSL(sslFlag string) error { | |||
| case tlsModeServer: | |||
| connectionLogger.Info("enabling SSL/TLS server mode") | |||
| v.conn = tls.Client(v.conn, &tls.Config{InsecureSkipVerify: true}) | |||
| case tlsModePrefer: | |||
| connectionLogger.Info("enabling SSL/TLS prefer mode") | |||
| v.conn = tls.Client(v.conn, &tls.Config{ServerName: v.connURL.Hostname(), InsecureSkipVerify: true}) | |||
There was a problem hiding this comment.
What is the reason we need to set the ServerName in the tls Config?
There was a problem hiding this comment.
I don't expect this will work when the server is not configured for TLS. Has this been tested in cases where the server IS configured as well as IS NOT configured for TLS?
It looks to me like initializeSSL() returns an error every time if the server's response is that SSL is not supported ('N'). This error return value then causes the calling function to error out early as well. Previously there was no option for the driver to proceed when a TLS mode was provided. Now with 'prefer', we want to proceed even if TLS is not supported, so if the server responds with 'N', we don't want to return an error if the tls mode is prefer. But if the tls mode is server or server-strict, then it would be an error.
|
I think this looks good now, lets give Siting a chance to review now that she is back |
| @@ -201,6 +201,10 @@ func TestTLSConfiguration(t *testing.T) { | |||
| switch *tlsMode { | |||
| case "none": | |||
| assertEqual(t, sslState, "None") | |||
| case "prefer": | |||
There was a problem hiding this comment.
There is no new test adding to CI, have you manually tested these two cases?
- tlsmode=prefer, TLS is not configured on server
- tlsmode=prefer, TLS is enabled on server
There was a problem hiding this comment.
Yes, I have tested them manually and also verified them with @DMickens.
sitingren
changed the title
No description provided.