Skip to content
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

Implement support of Optional TLS #900

Merged
merged 1 commit into from Dec 2, 2018

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented Dec 1, 2018

Description

Add optional 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.

Closes: #899

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@julienschmidt julienschmidt added this to the v1.5.0 milestone Dec 1, 2018
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" {
Copy link
Member

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} ?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

@dveeden
Copy link
Contributor Author

dveeden commented Dec 1, 2018

This also fixes the test: If a empty string was returned instead of a cipher this was not seen as an error

Copy link
Member

@julienschmidt julienschmidt left a 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.
@julienschmidt julienschmidt merged commit 60d456a into go-sql-driver:master Dec 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants