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

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

Merged
merged 7 commits into from
Nov 28, 2022

Conversation

lance6716
Copy link
Contributor

@lance6716 lance6716 commented Nov 11, 2022

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

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 11, 2022

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
Copy link
Contributor

lgtm-com bot commented Nov 11, 2022

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

new alerts:

  • 1 for Disabled TLS certificate check

@lance6716
Copy link
Contributor Author

PTAL at your convenience @methane @dveeden

@methane
Copy link
Member

methane commented Nov 13, 2022

I don't like adding new registry.

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

@lance6716
Copy link
Contributor Author

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

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 13, 2022

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
Copy link
Contributor

lgtm-com bot commented Nov 13, 2022

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

new alerts:

  • 1 for Disabled TLS certificate check

@lance6716 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
@lance6716
Copy link
Contributor Author

PR description is also updated 😄

@dveeden
Copy link
Contributor

dveeden commented Nov 13, 2022

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:

  1. Merge this PR and handle this in the go-sql-driver for MySQL (with the calling code optionally enabling this)
  2. Add complexity to the code that uses go-sql-driver, e.g. to reconnect without TLS if there is an TLS related error
  3. 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
Copy link
Contributor Author

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

packets.go Outdated
Comment on lines 225 to 227
if mc.flags&clientSSL == 0 && mc.cfg.TLS != nil {
if mc.cfg.AllowFallbackToNoTLS {
mc.cfg.TLS = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why delete TLSConfig == "preferred"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I got it.

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

LGTM.

@zhangyangyu
Copy link
Contributor

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.
image

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

methane commented Nov 14, 2022

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?

dsn.go Outdated
@@ -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":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lance6716 lance6716 Nov 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lance6716 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
Copy link

@D3Hunter D3Hunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Co-authored-by: D3Hunter <[email protected]>
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Nov 16, 2022

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
Copy link
Contributor Author

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

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
@shogo82148
Copy link
Contributor

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
Copy link
Contributor

lgtm-com bot commented Nov 22, 2022

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.

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.
@lance6716
Copy link
Contributor Author

ptal @methane

@lance6716
Copy link
Contributor Author

PTAL @methane this PR has LGT1 now

@methane methane merged commit 41dd159 into go-sql-driver:master Nov 28, 2022
xuanyu66 added a commit to pingcap/go-tpc that referenced this pull request Jan 17, 2024
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants