Add API to express like a `--ssl-mode=PREFERRED` MySQL client #1370

lance6716 · 2022-11-11T13:02:35Z

Signed-off-by: lance6716 [email protected]

Description

Problem

We use "preferred" tls mode to encrypt MySQL connection when it's possible. After golang 1.18 the default minimum TLS version is changed to v1.2, so it's no longer a valid value to connect to old MySQL servers, and according to the doc the only way to come back to old behaviour is setting tls.Config.MinVersion to VersionTLS10

New feature

In this PR I want to introduce(or update)

one new configuration:

AllowFallbackToNoTLS which is a bool to indicate the TLS is preferred rather than a must. Its behaviour is same as the old "preferred" tls mode

one (optional) configuration field change:

TLS which is a *tls.Config in Config. It's OK that we don't have this change and use old-style register + name reference, but I hope we can get rid of boring works (unique naming, register, deregister)

behaviour of configuration item combinations

(some of them are mentioned in the comments of this PR)

--tls-version=TLSv1.1 + --ssl-mode=PREFERRED as mysql client: If we set the tls.Config.MinVersion to v1.1 and use AllowFallbackToNoTLS, it should be the same as that
tlsConfig=preferred&tlsVersion=1.0 as an alternative: I think using *tls.Config and AllowFallbackToNoTLS are more flexible, for example, in *tls.Config I can set the minimun and maximun TLS version.
tlsConfig=skip-verify&tlsVersion=1.0 as an alternative: same as above, because in *tls.Config we can control InsecureSkipVerify. In addition to that, we can use VerifyPeerCertificate to perform more verification like --ssl-mode=VERIFY_CA/VERIFY_IDENTITY

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

Signed-off-by: lance6716 <[email protected]>

lgtm-com · 2022-11-11T13:04:10Z

This pull request introduces 1 alert when merging ad744fc into fa1e4ed - view on LGTM.com

new alerts:

1 for Disabled TLS certificate check

Signed-off-by: lance6716 <[email protected]>

lgtm-com · 2022-11-11T13:07:35Z

This pull request introduces 1 alert when merging 636b5b0 into fa1e4ed - view on LGTM.com

new alerts:

1 for Disabled TLS certificate check

lance6716 · 2022-11-13T02:16:52Z

PTAL at your convenience @methane @dveeden

methane · 2022-11-13T03:01:05Z

I don't like adding new registry.

Existing registries were added when Go didn't have Connector API.

lance6716 · 2022-11-13T03:10:54Z

OK, to use Connector we can pass everything needed to Config, but now the TLS part of Config is a string key to look up tls.Config in the inner map. Would you mind me add two fields that can directly pass *tls.Config and a bool indicating preferred? @methane

This reverts commit ad744fc.

Signed-off-by: lance6716 <[email protected]>

lgtm-com · 2022-11-13T04:16:53Z

This pull request introduces 1 alert when merging 372d17b into fa1e4ed - view on LGTM.com

new alerts:

1 for Disabled TLS certificate check

Signed-off-by: lance6716 <[email protected]>

lgtm-com · 2022-11-13T04:23:43Z

This pull request introduces 1 alert when merging 9e2588b into fa1e4ed - view on LGTM.com

new alerts:

1 for Disabled TLS certificate check

lance6716 · 2022-11-13T04:32:22Z

PR description is also updated 😄

dveeden · 2022-11-13T09:23:31Z

So the problem is that tls=preferred will use TLS if the server advertises this, but with the recent changes in Go this means that only TLSv1.2 or newer is supported. Older MySQL servers (before 5.7.28, excluding MySQL Enterprise) are build with YaSSL, which supports TLSv1.0 and TLSv1.1, but not TLSv1.2 or newer. This leads to connection failures.

I'm not sure how MySQL Client behaves in this situation. Looks like this PR might be more like setting --tls-version=TLSv1.1 than setting --ssl-mode=PREFERRED.

Note that TLSv1.1 is supported by YaSSL, so it should not be needed to downgrade all the way to TLSv1.0 for MySQL 5.7. For MySQL 5.6 and earlier TLSv1.0 was hardcoded (see https://bugs.mysql.com/bug.php?id=75239) and downgrading all the way might be needed.

So I think we have these options:

Merge this PR and handle this in the go-sql-driver for MySQL (with the calling code optionally enabling this)
Add complexity to the code that uses go-sql-driver, e.g. to reconnect without TLS if there is an TLS related error
Add complexity to the code that uses go-sql-driver, e.g. to connect without TLS and reconnect if the error says TLS is required.

I think merging this PR (or other changes, with the similar behavior) would be good to make it easier for applications to handle this.

Using older TLS protocols should not be done by default as that would be bad for security, but for people to migrate to newer database servers that support new TLS features this PR would help.

LGTM

lance6716 · 2022-11-14T02:59:45Z

@methane please give me some advice, we need the auto-TLS feature

Daemonxiao · 2022-11-14T03:02:06Z

packets.go

+	if mc.flags&clientSSL == 0 && mc.cfg.TLS != nil {
+		if mc.cfg.AllowFallbackToNoTLS {
+			mc.cfg.TLS = nil


Why delete TLSConfig == "preferred"?

the case TLSConfig == "preferred" is moved into dsn.go

case "preferred": cfg.TLS = &tls.Config{InsecureSkipVerify: true} cfg.AllowFallbackToNoTLS = true

I want to only use one variable AllowFallbackToNoTLS to control this behaviour

Ok. I got it.

Merging this PR is beneficial for some users with old versions of MySQL. 🎉

LGTM.

zhangyangyu · 2022-11-14T03:44:08Z

How about just allowing users to mysql.registerTLSConfig("preferred", &tls.Config{}). Use the registered one or use the current if there is none.

One thing to note is how to treat insecureSkipVerify when preferred. MySQL CLI seems to always ignore certs when preferred.

zhangyangyu · 2022-11-14T03:45:22Z

dsn.go

@@ -46,13 +46,14 @@ type Config struct {
 	ServerPubKey     string            // Server public key name
 	pubKey           *rsa.PublicKey    // Server public key
 	TLSConfig        string            // TLS configuration name
-	tls              *tls.Config       // TLS configuration
+	TLS              *tls.Config       // TLS configuration, its priority is higher than TLSConfig


In this version, does it still need to be public?

Yes it does not need to be public, we can still use the old way to register a TLS config and use key name to find it.

Let the maintainer to decide if we can add a short path 😄

methane · 2022-11-14T04:04:17Z

Almost LGTM.

We need to reorganize Config obj and DSN in v2, but now is not a time for it.

So I am considering about only naming for now:

TLS vs TLSConfigObj, or do not make it public in v1.
AllowFallbackToNoTLS vs AllowUnencrypted, InsecureAllowUnencrypted, InsecureFallbackToUnencrypted, etc...

@shogo82148 How do you think?

zhangyangyu · 2022-11-14T04:37:08Z

dsn.go

@@ -391,6 +401,14 @@ func parseDSNParams(cfg *Config, params string) (err error) {
 				return errors.New("invalid bool value: " + value)
 			}

+		// Allow fallback to unencrypted connection if server does not support TLS
+		case "allowFallbackToNoTLS":


What if setting tls=true or verify CA and hostname with allowFallbackToNoTLS at the same time? What's the meaning then? Looks like invalid combination to me.

OK, I should give more detailed description on "when will the driver fallback to no TLS", does it happen on TLS identity verify error, not supported TLS version or server not enable TLS?

I want a fallback for the last cases, and maybe the name should contain "TLSHandshake" or something. Will learn the TLS concepts to find a good name later. Welcome to give me some advice

also cc @dveeden

D3Hunter

maybe add a param tlsVersion, so we can use preferred to connect to old MySQL using tlsConfig=preferred&tlsVersion=1.0

and this way for tlsConfig = true or skip-verify, we can combined it with tlsVersion

README.md

Co-authored-by: D3Hunter <[email protected]>

lgtm-com · 2022-11-16T11:43:19Z

This pull request introduces 1 alert when merging a58b468 into fa1e4ed - view on LGTM.com

new alerts:

1 for Disabled TLS certificate check

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

lance6716 · 2022-11-21T07:38:42Z

I updated the PR description to give a overview of the comments.

@shogo82148 please give a review, especially about the naming/API stype as @methane requested

dsn.go

+		case "true":
+			cfg.TLS = &tls.Config{}
+		case "skip-verify":
+			cfg.TLS = &tls.Config{InsecureSkipVerify: true}


dsn.go

shogo82148 · 2022-11-21T19:23:16Z

TLS vs TLSConfigObj, or do not make it public in v1.

TLS sounds good to me.

TLSConfigObj is a little verbose for me, and I can't think of any other good names.
There is no problem with publishing in v1.

AllowFallbackToNoTLS vs AllowUnencrypted, InsecureAllowUnencrypted, InsecureFallbackToUnencrypted, etc...

How about AllowFallbackToPlaintext ?
We should use the word fallback because the default configure doesn't encrypt.
Negative sentences are a little confusing.

Signed-off-by: lance6716 <[email protected]>

lgtm-com · 2022-11-22T08:47:39Z

This pull request introduces 1 alert when merging 4ced115 into fa1e4ed - view on LGTM.com

new alerts:

1 for Disabled TLS certificate check

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

dsn.go

+		case "skip-verify":
+			cfg.TLS = &tls.Config{InsecureSkipVerify: true}
+		case "preferred":
+			cfg.TLS = &tls.Config{InsecureSkipVerify: true}


lance6716 · 2022-11-23T02:17:31Z

ptal @methane

lance6716 · 2022-11-28T02:04:42Z

PTAL @methane this PR has LGT1 now

- Using tls=preferred, the server accepts a TLS connection from the client but is also okay if the client does not switch to encryption. - upgrade go-sql-driver > 1.7.0 to use this feature for older version MySQL server go-sql-driver/mysql#1370.

A way to register preferred TLS config

ad744fc

Signed-off-by: lance6716 <[email protected]>

add myself to AUTHORS

636b5b0

Signed-off-by: lance6716 <[email protected]>

lance6716 added 2 commits November 13, 2022 12:00

Revert "A way to register preferred TLS config"

2a5ac9b

This reverts commit ad744fc.

change to another impl

372d17b

Signed-off-by: lance6716 <[email protected]>

add doc

9e2588b

Signed-off-by: lance6716 <[email protected]>

lance6716 changed the title ~~A way to register preferred TLS config~~ Add API to express like a --ssl-mode=mode=PREFERRED MySQL client Nov 13, 2022

Daemonxiao reviewed Nov 14, 2022

View reviewed changes

zhangyangyu reviewed Nov 14, 2022

View reviewed changes

lance6716 changed the title ~~Add API to express like a --ssl-mode=mode=PREFERRED MySQL client~~ Add API to express like a --ssl-mode=PREFERRED MySQL client Nov 16, 2022

D3Hunter reviewed Nov 16, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

a58b468

Co-authored-by: D3Hunter <[email protected]>

github-advanced-security bot found potential problems Nov 21, 2022

View reviewed changes

dsn.go

case "true":

cfg.TLS = &tls.Config{}

case "skip-verify":

cfg.TLS = &tls.Config{InsecureSkipVerify: true}

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.

dsn.go Fixed Show fixed Hide fixed

address comment

4ced115

Signed-off-by: lance6716 <[email protected]>

github-advanced-security bot found potential problems Nov 22, 2022

View reviewed changes

dsn.go

case "skip-verify":

cfg.TLS = &tls.Config{InsecureSkipVerify: true}

case "preferred":

cfg.TLS = &tls.Config{InsecureSkipVerify: true}

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.

shogo82148 approved these changes Nov 22, 2022

View reviewed changes

methane merged commit 41dd159 into go-sql-driver:master Nov 28, 2022

shogo82148 mentioned this pull request Nov 29, 2022

Add a new "tls-mode=preferred" DSN parameter #928

Closed

5 tasks

xuanyu66 mentioned this pull request Jan 17, 2024

Support connect to both tls and non-tls server pingcap/go-tpc#174

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API to express like a `--ssl-mode=PREFERRED` MySQL client #1370

Add API to express like a `--ssl-mode=PREFERRED` MySQL client #1370

lance6716 commented Nov 11, 2022 •

edited

Loading

lgtm-com bot commented Nov 11, 2022

lgtm-com bot commented Nov 11, 2022

lance6716 commented Nov 13, 2022

methane commented Nov 13, 2022

lance6716 commented Nov 13, 2022

lgtm-com bot commented Nov 13, 2022

lgtm-com bot commented Nov 13, 2022

lance6716 commented Nov 13, 2022

dveeden commented Nov 13, 2022

lance6716 commented Nov 14, 2022

Daemonxiao Nov 14, 2022

lance6716 Nov 14, 2022

Daemonxiao Nov 14, 2022

zhangyangyu commented Nov 14, 2022

zhangyangyu Nov 14, 2022

lance6716 Nov 14, 2022

methane commented Nov 14, 2022

zhangyangyu Nov 14, 2022

lance6716 Nov 14, 2022 •

edited

Loading

D3Hunter left a comment

lgtm-com bot commented Nov 16, 2022

lance6716 commented Nov 21, 2022

shogo82148 commented Nov 21, 2022

lgtm-com bot commented Nov 22, 2022

lance6716 commented Nov 23, 2022

lance6716 commented Nov 28, 2022

Add API to express like a --ssl-mode=PREFERRED MySQL client #1370

Add API to express like a --ssl-mode=PREFERRED MySQL client #1370

Conversation

lance6716 commented Nov 11, 2022 • edited Loading

Description

Problem

New feature

behaviour of configuration item combinations

Checklist

lgtm-com bot commented Nov 11, 2022

lgtm-com bot commented Nov 11, 2022

lance6716 commented Nov 13, 2022

methane commented Nov 13, 2022

lance6716 commented Nov 13, 2022

lgtm-com bot commented Nov 13, 2022

lgtm-com bot commented Nov 13, 2022

lance6716 commented Nov 13, 2022

dveeden commented Nov 13, 2022

lance6716 commented Nov 14, 2022

Daemonxiao Nov 14, 2022

Choose a reason for hiding this comment

lance6716 Nov 14, 2022

Choose a reason for hiding this comment

Daemonxiao Nov 14, 2022

Choose a reason for hiding this comment

zhangyangyu commented Nov 14, 2022

zhangyangyu Nov 14, 2022

Choose a reason for hiding this comment

lance6716 Nov 14, 2022

Choose a reason for hiding this comment

methane commented Nov 14, 2022

zhangyangyu Nov 14, 2022

Choose a reason for hiding this comment

lance6716 Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 16, 2022

lance6716 commented Nov 21, 2022

shogo82148 commented Nov 21, 2022

lgtm-com bot commented Nov 22, 2022

lance6716 commented Nov 23, 2022

lance6716 commented Nov 28, 2022

Add API to express like a `--ssl-mode=PREFERRED` MySQL client #1370

Add API to express like a `--ssl-mode=PREFERRED` MySQL client #1370

lance6716 commented Nov 11, 2022 •

edited

Loading

lance6716 Nov 14, 2022 •

edited

Loading