-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Implement support of Optional TLS #900
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
dsn.go
Outdated
@@ -560,7 +560,7 @@ func parseDSNParams(cfg *Config, params string) (err error) { | |||
} else { | |||
cfg.TLSConfig = "false" | |||
} | |||
} else if vl := strings.ToLower(value); vl == "skip-verify" { | |||
} else if vl := strings.ToLower(value); vl == "skip-verify" || vl == "optional" { |
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.
Should this really set cfg.tls = &tls.Config{InsecureSkipVerify: true}
?
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 doesn't have to. but then there should be an option to set tls=optional
and also supply a config for the CA etc.
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 option is not for security. If security is matter, they shouldn't allow unencrypted connection. In unencrypted connection, there are no verify for host.
This option is very silly behavior. It useful only for situations described in #899.
So InsecureSkipVerify: true
make sense.
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.
Please add a note to the README that using this option does not add any security.
Indeed, a Man-in-the-Middle attacker could remove TLS entirely with this option, whether InsecureSkipVerify: true
is used or not.
This also fixes the test: If a empty string was returned instead of a cipher this was not seen as an error |
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.
As discussed in #899, this option should use "preferred" instead of "optional".
Please also add a note regarding the security of this option. Other than that, this PR seems fine.
Issue: go-sql-driver#899 Add `preferred` config value to the `tls` config variable on the DSN. This results in a TLS connection when the server advertises this by the flag send in the initial packet.
Description
Add
optional
config value to thetls
config variable on the DSN.This results in a TLS connection when the server advertises this by the flag
send in the initial packet.
Closes: #899
Checklist