Allow TLS connections in the driver #673

atomicules · 2022-02-01T15:55:57Z

Opening this for discussion and feedback. I'm not expecting this to be merged in as-is.

This mostly exposes what is already elsewhere in this codebase to the driver
Allows for an optional ssl= URL parameter (query string) of values true and custom. Database must be specified for this to work
true just does c.UseSSL(true) on the connection
custom uses a package level variable of customTLSConfig to do c.SetTLSConfig(customTLSConfig)
That variable is set via a new function, SetCustomTLSConfig
To use a custom config you'd do a full import of the driver (not for side-effects only) and then do something like:
```
  driver.SetCustomTLSConfig(CaPem, make([]byte, 0), make([]byte, 0), false, "my.domain.name")
```

For what I need the basic TLS support is enough (ssl=true and c.UseSSL(true)) so if need be I could reduce this PR down to the first commit; However, since we are basically exposing what is already supported elsewhere in this codebase to the driver I thought I should get the custom TLS config support is a usable state (this PR).

Things that I could do with feedback/opinions on:

Since the DSN style for this driver uses a ? as a separator/indicator for the database the ssl parameter only works and makes sense if a database is specified. E.g:
```
  `?mydatabase&ssl=true`
```
vs
```
  `?ssl=true`
```
Which would try to connect to a database called ssl=true.

I am ok with this limitation/requirement.
I have also tried to keep the current style for parsing the URL parameters as opposed to re-working everything to use url.Parse. I assume there is a reason why that's not used and why ? is used instead of / for the database separator/indicator; At the very least I understand that switching to / would be a breaking change for many folk
I've allowed the Cert and Key to be optional in NewClientTLSConfig. I think that is a good idea, but I don't know if I've gone about it the best way
I'm just allowing for a single custom TLSConfig. This means that if a program uses the driver then only one custom config can be set - which might not be desirable if that driver is connecting to multiple endpoints. For me, right now, that's fine. But I'm willing to look at this some more
SetCustomTLSConfig is a very simple pass-through function. I welcome suggestions on how to improve. But it means I can just import the driver and still take advantage of NewClientTLSConfig

It's supported elsewhere in the codebase, but currently the driver has no way to flag/indicate TLS through the DSN. This uses additional parameters / query strings to indicate ssl. I'm not sure _at all_ at the moment how to do the more complex custom tlsConfig, but as an inital commit we can try doing basic TLS. I could be being naive, but to me using `url.Parse` rather than this custom parsing code would be easier. But perhaps there a reason why it's done this way and why `?` is used instead of `/` for the database. I've done my changes to fit in with the current way of doing things.

I.e. if we just want to pass in the CA cert.

This was just so I could test something locally. I hardcoded the CA cert and domain I needed in and then had a custom TLS config which used the CA cert and domain only. The `make([]byte, 0)` are effectively Nils and a way to set the cert/key pair to Nil. Ideally what I'd like to do is finish this off properly, but I don't actually need this change. The basic TLS changes are all I need.

This is pretty rudimentary, but good enough to start a PR and review/discussion for how it should end up. - There is just a single custom config called `custom` - A program using the driver can only have one custom config. That "works for me", but for people using the driver in a single program to communicate with different endpoints that probably isn't going to work - Adds a basic "wrapper" function `SetCustomTLSConfig` which passes through to `NewClientTLSConfig`. This means just the driver can be imported. To use this you'd do: ``` import ( "database/sql" "net/url" "github.com/go-mysql-org/go-mysql/driver" // full import required ) [...] var ( CaPem = []byte(`-----BEGIN CERTIFICATE----- [...] -----END CERTIFICATE-----`) ) [...] // CA and domain, no Cert and Key driver.SetCustomTLSConfig(CaPem, make([]byte, 0), make([]byte, 0), false, "my.domain.com") [...] ```

I consider Go 100% wrong in not indenting switch/case.

lance6716 · 2022-02-06T03:09:06Z

Do you have time to take a look @skoef ? seems you are an active user of it 😄

skoef · 2022-02-06T18:35:13Z

I consider myself an active (or perhaps heavy?) user of go-mysql indeed, but haven't touched the driver package or TLS at all. So as far as TLS change, couldn't tell if that's a good idea or bad.

For the DSN part, I think @atomicules raises a fair question: why is the database formatted as a query parameter where this is "always" the path parameter in the DSN. Not only would that make switching between drivers a lot easier but also parsing the DSN could be handled pretty much if not completely by url.Parse. So IMHO this was a poor choice back then. Perhaps we could refactor parsing the DSN a bit that it throws an error when [insert valid regex] matches, so we can move towards using url.Parse instead.

lance6716 · 2022-02-07T13:34:25Z

I'm just allowing for a single custom TLSConfig

I mostly argue on this decision, I think it's not much work to use a map to support multiple certificates. But I'm also ok if you want to change it in following pr.

My desire API for certificates is

a certID for DSN
the certID is returned by a registering function. User can use empty string to let the function generate a valid certID. User can provide the preferred certID as argument, and a bool/enum flag as another argument to choose "error on certID already exists" or "adjust the certID if already exists" (or always "error on certID already exists" for non-empty certID argument)

atomicules · 2022-02-10T17:42:45Z

I think it's not much work to use a map to support multiple certificates

I will look at doing this.

Perhaps we could refactor parsing the DSN

I'll have a think about this as well to see if there is a nice way to not break existing use cases, but also allow a / and use of url.Parse.

Thanks for the comments so far.

Using changes per here: go-mysql-org/go-mysql#673

Stored in a map using the address (hostname:port) as the key. Initial work at addressing review feedback. Possibly/probably there is mutex and locking stuff we want to do. Maybe overkill though? Using the address vs the full DSN makes sense to me as the key. It's a little cleaner and I can't see a situation where a slight difference in the DSN (i.e. different database) would require a different certificate or TLS config. This is actually pretty neat as we just use `&ssl=custom` and it looks up the TLS Config (that people will have to pre-register/store) based on the address in the DSN.

lance6716

@skoef do you want to give an advice?

driver/driver.go

Address feedback from review. The old style of DSN uses a `?` as path separator. This made parsing the DSN more complicated than necessary especially when adding in parsing of url parameters. By switching to the more standard `/` as a path separator we can use `url.Parse` to more easily split up the DSN and pull out the query strings. However... for compatibility we should still allow the old style. So what this commit does is: 1. Use a regex to identify whether a legacy or standard DSN style. We are assuming that if a "/" occurs after an "@" and no more "@" or "/" occur after that then it's the new/standard DSN style 2. Prefix the DSN with a scheme so we can use `url.Parse` 3. Only supports the url parameters (`ssl=X`) for the newer style DSN I.e. this shouldn't break anything for existing use cases, but does also allow people to start using TLS in the driver. There are probably edge cases in parsing of DSNs, but there would have been in the old way of doing it as well.

skoef · 2022-04-21T10:52:05Z

If we introduce changes to the way the URL can be parsed, what I would do before I start: abstract the code currently parsing the URL to a separate function and create a bunch of unit tests for it. This way you can know for sure you didn't mess up the original way of parsing whilst introducing new features.

atomicules · 2022-04-21T10:55:14Z

Hey, sorry for the long delay in getting back to this.

I've just pushed two commits to address previous feedback (and I see you've already started leaving feedback).

Allows multiple custom TLS configs. We still use ?ssl=custom to indicate instead of a specific named value, but we look up the TLS config based on the address (host and port) of the DSN. We'd register/store it up front like so:
```
 driver.SetCustomTLSConfig(dsn, caPem, make([]byte, 0), make([]byte, 0), insecureSkipVerify, serverName)
```
Re-jigs the DSN to use a / as the path separator instead of ? and uses url.Parse to split out the DSN. It still allows the old style DSN to be used so won't break things for existing users. If people want to use the url parameters (currently only ssl=X) then you need to use the new style DSN.

atomicules · 2022-04-21T10:56:56Z

I'll try to address the latest feedback a bit more quickly.

driver/driver.go

Address linting failure: ``` Error return value of `errors.Errorf` is not checked (errcheck) ```

1. Remove comment about "extend the use of url.Parse" because that was in fact done in commit 53c3b91 2. Add godoc style comment explaining `SetCustomTLSConfig`

Review feedback. Needed to split out to add a unit test and that naturally resulted in adding a struct/type for the connection info. Ideally I'd go back in time and split out the original code and use this unit test for that - and maybe that'll happen if deemed necessary with some rebasing. But if not we should be able to satisfy ourselves that the changes to the driver parsing code are ok with this test.

atomicules · 2022-05-04T14:00:59Z

If we introduce changes to the way the URL can be parsed, what I would do before I start: abstract the code currently parsing the URL to a separate function and create a bunch of unit tests for it. This way you can know for sure you didn't mess up the original way of parsing whilst introducing new features.

@skoef Good point, but since I'd already started I've split out the new parsing code to a separate function and added a unit test after the fact. If deemed necessary I could rebase things and alter the git history, etc so that I split out the existing/original code and add a unit test for that.

Alternatively we could just add DSN examples into the tests for the new parsing code.

I had trouble installing/running locally. Working now.

atomicules · 2022-05-04T14:18:19Z

Added some more commits:

(Finally) fixed linting issues
Return an error from the SetCustomTLSConfig function
Split out the DSN parsing into a separate function and added a unit test

From my point of view just ~~two~~ one open issues:

Whether we want to rebase, etc so that I split out the parsing and add a unit test for the original parsing code, before going on to alter that parsing code?
~~I want to spend a little time thinking about the race condition on the customTLSConfigMap. I haven't hit an issue yet during my testing, but I feel like I should fix that.~~ Added a commit to use sync.Mutex: 8b16e13

I've not noticed any data race issues, but just to be a bit safer.

lance6716

rest lgtm.

yes I hope we can add an unit test for original parsing code to remind us about the compatibility.

@skoef Do you have other comments?

lance6716 · 2022-05-05T01:14:23Z

driver/driver.go


 	"github.com/go-mysql-org/go-mysql/client"
 	"github.com/go-mysql-org/go-mysql/mysql"
 	"github.com/pingcap/errors"
 	"github.com/siddontang/go/hack"
 )

+var customTLSMutex sync.Mutex


Suggested change

var customTLSMutex sync.Mutex

var customTLSMutex sync.Mutex

Fixed this.

skoef · 2022-05-05T07:52:07Z

As long as we're backwards compatible ánd moving towards primarily using url.Parse, I couldn't be happier 👍

lance6716 · 2022-05-05T08:32:28Z

If deemed necessary I could rebase things and alter the git history, etc so that I split out the existing/original code and add a unit test for that.

Sorry I missed this comment and the word "rebase" in later comment. I think we do not need it. Will merge this PR if you have nothing else to do. @atomicules

atomicules · 2022-05-05T09:08:37Z

Thanks for the approval. I'll wait for comments from @skoef before merging

yes I hope we can add an unit test for original parsing code to remind us about the compatibility.

I've not pushed up a commit, but what I could do is add code to driver_test.go like this:

func TestOriginalParseDSN(t *testing.T) {
	// List of DSNs to test and expected results
	// Use different numbered domains to more readily see what has failed - since we
	// test in a loop we get the same line number on error
	testDSNs := map[string]connInfo{
		"user:password@localhost?db":                 connInfo{standardDSN: false, addr: "localhost", user: "user", password: "password", db: "db", params: url.Values(nil)},
		"[email protected]?db":                       connInfo{standardDSN: false, addr: "1.domain.com", user: "user", password: "", db: "db", params: url.Values(nil)},
	}

	for supplied, expected := range testDSNs {
		actual, err := originalParseDSN(supplied)
		if err != nil {
			t.Errorf("TestOriginalParseDSN failed. Got error: %s", err)
		}
		// Compare that with expected
		if !reflect.DeepEqual(actual, expected) {
			t.Errorf("TestOriginalParseDSN failed.\nExpected:\n%#v\nGot:\n%#v", expected, actual)
		}
	}
}

// originalParseDSN is from: https://github.com/go-mysql-org/go-mysql/blob/7153c75fcadc4083dcff2e772c4d4c906eccdeb3/driver/driver.go
func originalParseDSN(dsn string) (connInfo, error) {
	ci := connInfo{}

	lastIndex := strings.LastIndex(dsn, "@")
	seps := []string{dsn[:lastIndex], dsn[lastIndex+1:]}
	if len(seps) != 2 {
		return ci, errors.Errorf("invalid dsn, must user:password@addr[?db]")
	}

	if ss := strings.Split(seps[0], ":"); len(ss) == 2 {
		ci.user, ci.password = ss[0], ss[1]
	} else if len(ss) == 1 {
		ci.user = ss[0]
	} else {
		return ci, errors.Errorf("invalid dsn, must user:password@addr[?db]")
	}

	if ss := strings.Split(seps[1], "?"); len(ss) == 2 {
		ci.addr, ci.db = ss[0], ss[1]
	} else if len(ss) == 1 {
		ci.addr = ss[0]
	} else {
		return ci, errors.Errorf("invalid dsn, must user:password@addr[?db]")
	}

	return ci, nil
}

If we wanted something in there for reference?

That would be instead of re-writing git history to add in before the changes.

atomicules · 2022-05-05T09:10:16Z

Sorry I missed this comment and the word "rebase" in later comment. I think we do not need it. Will merge this PR if you have nothing else to do. @atomicules

😄 I just wrote the comment above before seeing your comment.

I am happy for it to be merged as is.

atomicules · 2022-05-05T09:45:33Z

Thank you!

atomicules added 5 commits January 21, 2022 18:27

Allow Cert and Key pair to be optional

d865626

I.e. if we just want to pass in the CA cert.

gofmt changes only

0da459f

I consider Go 100% wrong in not indenting switch/case.

atomicules added a commit to compose/transporter that referenced this pull request Feb 15, 2022

Reference commit for setting custom TLS config

fc5842a

Using changes per here: go-mysql-org/go-mysql#673

atercattus and others added 2 commits March 24, 2022 18:21

Merge branch 'master' into tls-in-driver

43aa9b9

lance6716 reviewed Apr 21, 2022

View reviewed changes

lance6716 reviewed Apr 24, 2022

View reviewed changes

driver/driver.go Outdated Show resolved Hide resolved

atomicules added 3 commits May 4, 2022 14:29

Make SetCustomTLSConfig return an error

09ab351

Address linting failure: ``` Error return value of `errors.Errorf` is not checked (errcheck) ```

Comment changes only

60d9485

1. Remove comment about "extend the use of url.Parse" because that was in fact done in commit 53c3b91 2. Add godoc style comment explaining `SetCustomTLSConfig`

golangci-lint issues (unnecessary new lines)

039dcc2

I had trouble installing/running locally. Working now.

Use sync.Mutex when updating the customTLSConfigMap

8b16e13

I've not noticed any data race issues, but just to be a bit safer.

lance6716 approved these changes May 5, 2022

View reviewed changes

Fix linting issue, new line after variable.

587d49e

Merge branch 'master' into tls-in-driver

b490dc8

lance6716 merged commit 145f684 into go-mysql-org:master May 5, 2022

atomicules deleted the tls-in-driver branch May 5, 2022 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow TLS connections in the driver #673

Allow TLS connections in the driver #673

atomicules commented Feb 1, 2022

lance6716 commented Feb 6, 2022

skoef commented Feb 6, 2022 •

edited

Loading

lance6716 commented Feb 7, 2022 •

edited

Loading

atomicules commented Feb 10, 2022

lance6716 left a comment

skoef commented Apr 21, 2022

atomicules commented Apr 21, 2022

atomicules commented Apr 21, 2022

atomicules commented May 4, 2022

atomicules commented May 4, 2022 •

edited

Loading

lance6716 left a comment

lance6716 May 5, 2022

atomicules May 5, 2022

skoef commented May 5, 2022

lance6716 commented May 5, 2022

atomicules commented May 5, 2022

atomicules commented May 5, 2022

atomicules commented May 5, 2022

Allow TLS connections in the driver #673

Allow TLS connections in the driver #673

Conversation

atomicules commented Feb 1, 2022

lance6716 commented Feb 6, 2022

skoef commented Feb 6, 2022 • edited Loading

lance6716 commented Feb 7, 2022 • edited Loading

atomicules commented Feb 10, 2022

lance6716 left a comment

Choose a reason for hiding this comment

skoef commented Apr 21, 2022

atomicules commented Apr 21, 2022

atomicules commented Apr 21, 2022

atomicules commented May 4, 2022

atomicules commented May 4, 2022 • edited Loading

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 May 5, 2022

Choose a reason for hiding this comment

atomicules May 5, 2022

Choose a reason for hiding this comment

skoef commented May 5, 2022

lance6716 commented May 5, 2022

atomicules commented May 5, 2022

atomicules commented May 5, 2022

atomicules commented May 5, 2022

skoef commented Feb 6, 2022 •

edited

Loading

lance6716 commented Feb 7, 2022 •

edited

Loading

atomicules commented May 4, 2022 •

edited

Loading