From 0c228aa36f93c8b3efd243cf7c72d03434b23551 Mon Sep 17 00:00:00 2001 From: atomicules Date: Fri, 21 Jan 2022 11:59:40 +0000 Subject: [PATCH 01/13] Initial work on supporting TLS in the driver 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. --- driver/driver.go | 50 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/driver/driver.go b/driver/driver.go index c2653a80d..49fbc5e2b 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -23,31 +23,71 @@ func (d driver) Open(dsn string) (sqldriver.Conn, error) { lastIndex := strings.LastIndex(dsn, "@") seps := []string{dsn[:lastIndex], dsn[lastIndex+1:]} if len(seps) != 2 { - return nil, errors.Errorf("invalid dsn, must user:password@addr[?db]") + return nil, errors.Errorf("invalid dsn, must user:password@addr[[?db[¶m=X]]") } var user string var password string var addr string var db string + var err error + var c *client.Conn if ss := strings.Split(seps[0], ":"); len(ss) == 2 { user, password = ss[0], ss[1] } else if len(ss) == 1 { user = ss[0] } else { - return nil, errors.Errorf("invalid dsn, must user:password@addr[?db]") + return nil, errors.Errorf("invalid dsn, must user:password@addr[[?db[¶m=X]]") } + params := make(map[string]string) if ss := strings.Split(seps[1], "?"); len(ss) == 2 { - addr, db = ss[0], ss[1] + // If the dsn used a `/` for the path separator this would be easier to parse + // with `url.Parse` and we could use `.Path` to get the db and then use + // `Query()` to get the parameters and values. + // But for consistency with the current way of doing things... + addr = ss[0] + dbAndParams := ss[1] + if ss := strings.Split(dbAndParams, "&"); len(ss) == 1 { + db = ss[0] + } else { + // We have to assume the first is the db + // Then need to handle possible multiple parameters / query strings + for i, p := range ss { + if i == 0 { + db = p + } else { + // Build key value pairs + kv := strings.Split(p, "=") + params[kv[0]] = kv[1] + } + } + } } else if len(ss) == 1 { addr = ss[0] } else { - return nil, errors.Errorf("invalid dsn, must user:password@addr[?db]") + return nil, errors.Errorf("invalid dsn, must user:password@addr[[?db[¶m=X]]") } - c, err := client.Connect(addr, user, password, db) + tlsConfigName, tls := params["ssl"] + if tls { + if tlsConfigName == "true" { + // This actually does insecureSkipVerify + // But not even sure if it makes sense to handle false? According to + // client_test.go it doesn't - it'd result in an error + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.UseSSL(true) }) + } else { + // TODO: This is going to be trickier. Need a way of taking the + // `tlsConfigName` which is a string type and using that to point at the actual + // `tlsConfig` which is a `NewClientTLSConfig` type. Can probably draw + // inspiration from go-sql-driver/mysql + //c, err := client.Connect(addr, user, password, db, func(c *Conn) {c.SetTLSConfig(tlsConfig)}) + return nil, errors.Errorf("Custom TLS configuration support not implemented yet") + } + } else { + c, err = client.Connect(addr, user, password, db) + } if err != nil { return nil, err } From d865626c53b3410055edd4906ba6b522dfb8b513 Mon Sep 17 00:00:00 2001 From: atomicules Date: Fri, 28 Jan 2022 13:51:07 +0000 Subject: [PATCH 02/13] Allow Cert and Key pair to be optional I.e. if we just want to pass in the CA cert. --- client/tls.go | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/client/tls.go b/client/tls.go index f65239933..1450d7c78 100644 --- a/client/tls.go +++ b/client/tls.go @@ -13,16 +13,26 @@ func NewClientTLSConfig(caPem, certPem, keyPem []byte, insecureSkipVerify bool, panic("failed to add ca PEM") } - cert, err := tls.X509KeyPair(certPem, keyPem) - if err != nil { - panic(err) - } + var config *tls.Config - config := &tls.Config{ - Certificates: []tls.Certificate{cert}, - RootCAs: pool, - InsecureSkipVerify: insecureSkipVerify, - ServerName: serverName, + if string(certPem) != "" && string(keyPem) != "" { + cert, err := tls.X509KeyPair(certPem, keyPem) + if err != nil { + panic(err) + } + config = &tls.Config{ + RootCAs: pool, + Certificates: []tls.Certificate{cert}, + InsecureSkipVerify: insecureSkipVerify, + ServerName: serverName, + } + } else { + config = &tls.Config{ + RootCAs: pool, + InsecureSkipVerify: insecureSkipVerify, + ServerName: serverName, + } } + return config } From 1aca0686f3873c3f4eb5cafda0c70f8af24b6357 Mon Sep 17 00:00:00 2001 From: atomicules Date: Fri, 28 Jan 2022 14:03:15 +0000 Subject: [PATCH 03/13] Reference commit - Custom TLS config 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. --- driver/driver.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/driver/driver.go b/driver/driver.go index 49fbc5e2b..4a953e55b 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -18,6 +18,11 @@ import ( type driver struct { } +// Testing custom tls by hard-coding something in +var CaPem = []byte(`-----BEGIN CERTIFICATE----- +MYCACERT +-----END CERTIFICATE-----`) + // Open: DSN user:password@addr[?db] func (d driver) Open(dsn string) (sqldriver.Conn, error) { lastIndex := strings.LastIndex(dsn, "@") @@ -70,6 +75,8 @@ func (d driver) Open(dsn string) (sqldriver.Conn, error) { return nil, errors.Errorf("invalid dsn, must user:password@addr[[?db[¶m=X]]") } + custom := client.NewClientTLSConfig(CaPem, make([]byte, 0), make([]byte, 0), false, "custom.host.name") + tlsConfigName, tls := params["ssl"] if tls { if tlsConfigName == "true" { @@ -82,8 +89,8 @@ func (d driver) Open(dsn string) (sqldriver.Conn, error) { // `tlsConfigName` which is a string type and using that to point at the actual // `tlsConfig` which is a `NewClientTLSConfig` type. Can probably draw // inspiration from go-sql-driver/mysql - //c, err := client.Connect(addr, user, password, db, func(c *Conn) {c.SetTLSConfig(tlsConfig)}) - return nil, errors.Errorf("Custom TLS configuration support not implemented yet") + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) {c.SetTLSConfig(custom)}) + //return nil, errors.Errorf("Custom TLS configuration support not implemented yet") } } else { c, err = client.Connect(addr, user, password, db) From 2757a0609925ba6cfa93bb259ceda83c1cecef7b Mon Sep 17 00:00:00 2001 From: atomicules Date: Tue, 1 Feb 2022 13:29:18 +0000 Subject: [PATCH 04/13] Custom TLS config support in the driver 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") [...] ``` --- client/tls.go | 3 +++ driver/driver.go | 42 +++++++++++++++++++++++------------------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/client/tls.go b/client/tls.go index 1450d7c78..c3eb87eae 100644 --- a/client/tls.go +++ b/client/tls.go @@ -15,6 +15,8 @@ func NewClientTLSConfig(caPem, certPem, keyPem []byte, insecureSkipVerify bool, var config *tls.Config + // Allow cert and key to be optional + // Send through `make([]byte, 0)` for "nil" if string(certPem) != "" && string(keyPem) != "" { cert, err := tls.X509KeyPair(certPem, keyPem) if err != nil { @@ -36,3 +38,4 @@ func NewClientTLSConfig(caPem, certPem, keyPem []byte, insecureSkipVerify bool, return config } + diff --git a/driver/driver.go b/driver/driver.go index 4a953e55b..93ff95cbc 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -3,6 +3,7 @@ package driver import ( + "crypto/tls" "database/sql" sqldriver "database/sql/driver" "fmt" @@ -15,14 +16,11 @@ import ( "github.com/siddontang/go/hack" ) +var customTLSConfig *tls.Config + type driver struct { } -// Testing custom tls by hard-coding something in -var CaPem = []byte(`-----BEGIN CERTIFICATE----- -MYCACERT ------END CERTIFICATE-----`) - // Open: DSN user:password@addr[?db] func (d driver) Open(dsn string) (sqldriver.Conn, error) { lastIndex := strings.LastIndex(dsn, "@") @@ -75,22 +73,22 @@ func (d driver) Open(dsn string) (sqldriver.Conn, error) { return nil, errors.Errorf("invalid dsn, must user:password@addr[[?db[¶m=X]]") } - custom := client.NewClientTLSConfig(CaPem, make([]byte, 0), make([]byte, 0), false, "custom.host.name") - tlsConfigName, tls := params["ssl"] if tls { - if tlsConfigName == "true" { - // This actually does insecureSkipVerify - // But not even sure if it makes sense to handle false? According to - // client_test.go it doesn't - it'd result in an error - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.UseSSL(true) }) - } else { - // TODO: This is going to be trickier. Need a way of taking the - // `tlsConfigName` which is a string type and using that to point at the actual - // `tlsConfig` which is a `NewClientTLSConfig` type. Can probably draw - // inspiration from go-sql-driver/mysql - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) {c.SetTLSConfig(custom)}) - //return nil, errors.Errorf("Custom TLS configuration support not implemented yet") + switch tlsConfigName { + case "true": + // This actually does insecureSkipVerify + // But not even sure if it makes sense to handle false? According to + // client_test.go it doesn't - it'd result in an error + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.UseSSL(true) }) + case "custom": + // I was too concerned about mimicking what go-sql-driver/mysql does which will + // allow any name for a custom tls profile and maps the query parameter value to + // that TLSConfig variable... there is no need to be that clever. We can just + // insist `"custom"` (string) == `custom` (TLSConfig) + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) {c.SetTLSConfig(customTLSConfig)}) + default: + return nil, errors.Errorf("Supported options are ssl=true or ssl=custom") } } else { c, err = client.Connect(addr, user, password, db) @@ -276,3 +274,9 @@ func (r *rows) Next(dest []sqldriver.Value) error { func init() { sql.Register("mysql", driver{}) } + +func SetCustomTLSConfig(caPem []byte, certPem []byte, keyPem []byte, insecureSkipVerify bool, serverName string) { + // Basic pass-through function so we can just import the driver + // For now there is a single shared "custom" config. I.e. any program can only have one "custom" config + customTLSConfig = client.NewClientTLSConfig(caPem, certPem, keyPem, insecureSkipVerify, serverName) +} From 0da459f10e25d6944e6f6ea55d957fa1c618287b Mon Sep 17 00:00:00 2001 From: atomicules Date: Tue, 1 Feb 2022 16:10:24 +0000 Subject: [PATCH 05/13] gofmt changes only I consider Go 100% wrong in not indenting switch/case. --- client/tls.go | 1 - driver/driver.go | 26 +++++++++++++------------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/client/tls.go b/client/tls.go index c3eb87eae..b2fe6da02 100644 --- a/client/tls.go +++ b/client/tls.go @@ -38,4 +38,3 @@ func NewClientTLSConfig(caPem, certPem, keyPem []byte, insecureSkipVerify bool, return config } - diff --git a/driver/driver.go b/driver/driver.go index 93ff95cbc..ba2c1873d 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -76,19 +76,19 @@ func (d driver) Open(dsn string) (sqldriver.Conn, error) { tlsConfigName, tls := params["ssl"] if tls { switch tlsConfigName { - case "true": - // This actually does insecureSkipVerify - // But not even sure if it makes sense to handle false? According to - // client_test.go it doesn't - it'd result in an error - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.UseSSL(true) }) - case "custom": - // I was too concerned about mimicking what go-sql-driver/mysql does which will - // allow any name for a custom tls profile and maps the query parameter value to - // that TLSConfig variable... there is no need to be that clever. We can just - // insist `"custom"` (string) == `custom` (TLSConfig) - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) {c.SetTLSConfig(customTLSConfig)}) - default: - return nil, errors.Errorf("Supported options are ssl=true or ssl=custom") + case "true": + // This actually does insecureSkipVerify + // But not even sure if it makes sense to handle false? According to + // client_test.go it doesn't - it'd result in an error + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.UseSSL(true) }) + case "custom": + // I was too concerned about mimicking what go-sql-driver/mysql does which will + // allow any name for a custom tls profile and maps the query parameter value to + // that TLSConfig variable... there is no need to be that clever. We can just + // insist `"custom"` (string) == `custom` (TLSConfig) + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.SetTLSConfig(customTLSConfig) }) + default: + return nil, errors.Errorf("Supported options are ssl=true or ssl=custom") } } else { c, err = client.Connect(addr, user, password, db) From a962bc895b00b912cd1f40614afe0a14253592cd Mon Sep 17 00:00:00 2001 From: atomicules Date: Wed, 20 Apr 2022 17:37:59 +0100 Subject: [PATCH 06/13] Allow multiple custom TLS configs in driver 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. --- driver/driver.go | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/driver/driver.go b/driver/driver.go index ba2c1873d..eb18872d3 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -8,6 +8,7 @@ import ( sqldriver "database/sql/driver" "fmt" "io" + "net/url" "strings" "github.com/go-mysql-org/go-mysql/client" @@ -16,7 +17,8 @@ import ( "github.com/siddontang/go/hack" ) -var customTLSConfig *tls.Config +// Map of dsn address (makes more sense than full dsn?) to tls Config +var customTLSConfigMap = make(map[string]*tls.Config) type driver struct { } @@ -84,9 +86,10 @@ func (d driver) Open(dsn string) (sqldriver.Conn, error) { case "custom": // I was too concerned about mimicking what go-sql-driver/mysql does which will // allow any name for a custom tls profile and maps the query parameter value to - // that TLSConfig variable... there is no need to be that clever. We can just - // insist `"custom"` (string) == `custom` (TLSConfig) - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.SetTLSConfig(customTLSConfig) }) + // that TLSConfig variable... there is no need to be that clever. + // Instead of doing that, let's store required custom TLSConfigs in a map that + // uses the DSN address as the key + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.SetTLSConfig(customTLSConfigMap[addr]) }) default: return nil, errors.Errorf("Supported options are ssl=true or ssl=custom") } @@ -275,8 +278,19 @@ func init() { sql.Register("mysql", driver{}) } -func SetCustomTLSConfig(caPem []byte, certPem []byte, keyPem []byte, insecureSkipVerify bool, serverName string) { +func SetCustomTLSConfig(dsn string, caPem []byte, certPem []byte, keyPem []byte, insecureSkipVerify bool, serverName string) { + // Extract addr from dsn + // We can hopefully extend the use of url.Parse if we switch the DSN style + parsed, err := url.Parse(dsn) + if err != nil { + errors.Errorf("Unable to parse DSN. Need to extract address to use as key for storing custom TLS config") + } + addr := parsed.Host + + // I thought about using serverName instead of addr below, but decided against that as + // having multiple CA certs for one hostname is likely when you have services running on + // different ports. + // Basic pass-through function so we can just import the driver - // For now there is a single shared "custom" config. I.e. any program can only have one "custom" config - customTLSConfig = client.NewClientTLSConfig(caPem, certPem, keyPem, insecureSkipVerify, serverName) + customTLSConfigMap[addr] = client.NewClientTLSConfig(caPem, certPem, keyPem, insecureSkipVerify, serverName) } From 53c3b91d6803bf2b7fd4dac9e11e666dfaa8b697 Mon Sep 17 00:00:00 2001 From: atomicules Date: Thu, 21 Apr 2022 10:47:50 +0000 Subject: [PATCH 07/13] Switch DSN style for driver, but still allow legacy form 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. --- driver/driver.go | 104 ++++++++++++++++++++--------------------------- 1 file changed, 45 insertions(+), 59 deletions(-) diff --git a/driver/driver.go b/driver/driver.go index eb18872d3..46119efee 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -9,7 +9,7 @@ import ( "fmt" "io" "net/url" - "strings" + "regexp" "github.com/go-mysql-org/go-mysql/client" "github.com/go-mysql-org/go-mysql/mysql" @@ -23,12 +23,22 @@ var customTLSConfigMap = make(map[string]*tls.Config) type driver struct { } -// Open: DSN user:password@addr[?db] +// Open: DSN +// Support both legacy style DSNs: user:password@addr[?db] +// And more standard DSNs: user:password@addr/db?param=value +// Optional parameters are supported in the standard DSN func (d driver) Open(dsn string) (sqldriver.Conn, error) { - lastIndex := strings.LastIndex(dsn, "@") - seps := []string{dsn[:lastIndex], dsn[lastIndex+1:]} - if len(seps) != 2 { - return nil, errors.Errorf("invalid dsn, must user:password@addr[[?db[¶m=X]]") + // If a "/" occurs after "@" and then no more "@" or "/" occur after that + standardDSN, matchErr := regexp.MatchString("@[^@]+/[^@/]+", dsn) + if matchErr != nil { + return nil, errors.Errorf("invalid dsn, must be user:password@addr[/db[?param=X]]") + } + + // Add a prefix so we can parse with url.Parse + dsn = "mysql://" + dsn + parsedDSN, parseErr := url.Parse(dsn) + if parseErr != nil { + return nil, errors.Errorf("invalid dsn, must be user:password@addr[/db[?param=X]]") } var user string @@ -38,62 +48,38 @@ func (d driver) Open(dsn string) (sqldriver.Conn, error) { var err error var c *client.Conn - if ss := strings.Split(seps[0], ":"); len(ss) == 2 { - user, password = ss[0], ss[1] - } else if len(ss) == 1 { - user = ss[0] - } else { - return nil, errors.Errorf("invalid dsn, must user:password@addr[[?db[¶m=X]]") - } - - params := make(map[string]string) - if ss := strings.Split(seps[1], "?"); len(ss) == 2 { - // If the dsn used a `/` for the path separator this would be easier to parse - // with `url.Parse` and we could use `.Path` to get the db and then use - // `Query()` to get the parameters and values. - // But for consistency with the current way of doing things... - addr = ss[0] - dbAndParams := ss[1] - if ss := strings.Split(dbAndParams, "&"); len(ss) == 1 { - db = ss[0] - } else { - // We have to assume the first is the db - // Then need to handle possible multiple parameters / query strings - for i, p := range ss { - if i == 0 { - db = p - } else { - // Build key value pairs - kv := strings.Split(p, "=") - params[kv[0]] = kv[1] - } + user = parsedDSN.User.Username() + // What does the below return for _. It's not err / bool + password, _ = parsedDSN.User.Password() + addr = parsedDSN.Host + if standardDSN { + // Remove slash + db = parsedDSN.Path[1:] + params := parsedDSN.Query() + if params["ssl"] != nil { + tlsConfigName := params.Get("ssl") + switch tlsConfigName { + case "true": + // This actually does insecureSkipVerify + // But not even sure if it makes sense to handle false? According to + // client_test.go it doesn't - it'd result in an error + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.UseSSL(true) }) + case "custom": + // I was too concerned about mimicking what go-sql-driver/mysql does which will + // allow any name for a custom tls profile and maps the query parameter value to + // that TLSConfig variable... there is no need to be that clever. + // Instead of doing that, let's store required custom TLSConfigs in a map that + // uses the DSN address as the key + c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.SetTLSConfig(customTLSConfigMap[addr]) }) + default: + return nil, errors.Errorf("Supported options are ssl=true or ssl=custom") } - } - } else if len(ss) == 1 { - addr = ss[0] - } else { - return nil, errors.Errorf("invalid dsn, must user:password@addr[[?db[¶m=X]]") - } - - tlsConfigName, tls := params["ssl"] - if tls { - switch tlsConfigName { - case "true": - // This actually does insecureSkipVerify - // But not even sure if it makes sense to handle false? According to - // client_test.go it doesn't - it'd result in an error - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.UseSSL(true) }) - case "custom": - // I was too concerned about mimicking what go-sql-driver/mysql does which will - // allow any name for a custom tls profile and maps the query parameter value to - // that TLSConfig variable... there is no need to be that clever. - // Instead of doing that, let's store required custom TLSConfigs in a map that - // uses the DSN address as the key - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.SetTLSConfig(customTLSConfigMap[addr]) }) - default: - return nil, errors.Errorf("Supported options are ssl=true or ssl=custom") + } else { + c, err = client.Connect(addr, user, password, db) } } else { + // No more processing here. Let's only support url parameters with the newer style DSN + db = parsedDSN.RawQuery c, err = client.Connect(addr, user, password, db) } if err != nil { From 09ab3510215f59477cc56624c8d9e32785b547db Mon Sep 17 00:00:00 2001 From: atomicules Date: Wed, 4 May 2022 08:20:51 +0000 Subject: [PATCH 08/13] Make SetCustomTLSConfig return an error Address linting failure: ``` Error return value of `errors.Errorf` is not checked (errcheck) ``` --- driver/driver.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/driver/driver.go b/driver/driver.go index 46119efee..5d067e47c 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -264,12 +264,12 @@ func init() { sql.Register("mysql", driver{}) } -func SetCustomTLSConfig(dsn string, caPem []byte, certPem []byte, keyPem []byte, insecureSkipVerify bool, serverName string) { +func SetCustomTLSConfig(dsn string, caPem []byte, certPem []byte, keyPem []byte, insecureSkipVerify bool, serverName string) error { // Extract addr from dsn // We can hopefully extend the use of url.Parse if we switch the DSN style parsed, err := url.Parse(dsn) if err != nil { - errors.Errorf("Unable to parse DSN. Need to extract address to use as key for storing custom TLS config") + return errors.Errorf("Unable to parse DSN. Need to extract address to use as key for storing custom TLS config") } addr := parsed.Host @@ -279,4 +279,6 @@ func SetCustomTLSConfig(dsn string, caPem []byte, certPem []byte, keyPem []byte, // Basic pass-through function so we can just import the driver customTLSConfigMap[addr] = client.NewClientTLSConfig(caPem, certPem, keyPem, insecureSkipVerify, serverName) + + return nil } From 60d94852d3ed9acbf8f6a93d815fba5858d82616 Mon Sep 17 00:00:00 2001 From: atomicules Date: Wed, 4 May 2022 09:01:27 +0000 Subject: [PATCH 09/13] Comment changes only 1. Remove comment about "extend the use of url.Parse" because that was in fact done in commit 53c3b91d6803bf2b7fd4dac9e11e666dfaa8b697 2. Add godoc style comment explaining `SetCustomTLSConfig` --- driver/driver.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/driver/driver.go b/driver/driver.go index 5d067e47c..bd92962af 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -264,9 +264,16 @@ func init() { sql.Register("mysql", driver{}) } + +// SetCustomTLSConfig sets a custom TLSConfig for the address (host:port) of the supplied DSN. +// It requires a full import of the driver (not by side-effects only). +// Example of supplying a custom CA, no client cert, no key, validating the +// certificate, and supplying a serverName for the validation: +// +// driver.SetCustomTLSConfig(CaPem, make([]byte, 0), make([]byte, 0), false, "my.domain.name") +// func SetCustomTLSConfig(dsn string, caPem []byte, certPem []byte, keyPem []byte, insecureSkipVerify bool, serverName string) error { // Extract addr from dsn - // We can hopefully extend the use of url.Parse if we switch the DSN style parsed, err := url.Parse(dsn) if err != nil { return errors.Errorf("Unable to parse DSN. Need to extract address to use as key for storing custom TLS config") From a5b62b18e3fd0f43060a0a6a3ba9cf96de6f142a Mon Sep 17 00:00:00 2001 From: atomicules Date: Wed, 4 May 2022 13:27:55 +0000 Subject: [PATCH 10/13] Split out DSN parsing into function and add unit test 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. --- driver/driver.go | 91 +++++++++++++++++++++++++++++-------------- driver/driver_test.go | 27 +++++++++++++ 2 files changed, 89 insertions(+), 29 deletions(-) diff --git a/driver/driver.go b/driver/driver.go index bd92962af..77ec71e67 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -23,64 +23,98 @@ var customTLSConfigMap = make(map[string]*tls.Config) type driver struct { } -// Open: DSN -// Support both legacy style DSNs: user:password@addr[?db] -// And more standard DSNs: user:password@addr/db?param=value -// Optional parameters are supported in the standard DSN -func (d driver) Open(dsn string) (sqldriver.Conn, error) { +type connInfo struct { + standardDSN bool + addr string + user string + password string + db string + params url.Values +} + +// ParseDSN takes a DSN string and splits it up into struct containing addr, +// user, password and db. +// It returns an error if unable to parse. +// The struct also contains a boolean indicating if the DSN is in legacy or +// standard form. +// +// Legacy form uses a `?` is used as the path separator: user:password@addr[?db] +// Standard form uses a `/`: user:password@addr/db?param=value +// +// Optional parameters are supported in the standard DSN form +func parseDSN(dsn string) (connInfo, error) { + + var matchErr error + ci := connInfo{} + // If a "/" occurs after "@" and then no more "@" or "/" occur after that - standardDSN, matchErr := regexp.MatchString("@[^@]+/[^@/]+", dsn) + ci.standardDSN, matchErr = regexp.MatchString("@[^@]+/[^@/]+", dsn) if matchErr != nil { - return nil, errors.Errorf("invalid dsn, must be user:password@addr[/db[?param=X]]") + return ci, errors.Errorf("invalid dsn, must be user:password@addr[/db[?param=X]]") } // Add a prefix so we can parse with url.Parse dsn = "mysql://" + dsn parsedDSN, parseErr := url.Parse(dsn) if parseErr != nil { - return nil, errors.Errorf("invalid dsn, must be user:password@addr[/db[?param=X]]") + return ci, errors.Errorf("invalid dsn, must be user:password@addr[/db[?param=X]]") + } + + ci.addr = parsedDSN.Host + ci.user = parsedDSN.User.Username() + // We ignore the second argument as that is just a flag for existence of a password + // If not set we get empty string anyway + ci.password, _ = parsedDSN.User.Password() + + if ci.standardDSN { + ci.db = parsedDSN.Path[1:] + ci.params = parsedDSN.Query() + } else { + ci.db = parsedDSN.RawQuery + // This is the equivalent to a "nil" list of parameters + ci.params = url.Values{} } - var user string - var password string - var addr string - var db string - var err error + return ci, nil +} + +// Open takes a supplied DSN string and opens a connection +// See ParseDSN for more information on the form of the DSN +func (d driver) Open(dsn string) (sqldriver.Conn, error) { + var c *client.Conn - user = parsedDSN.User.Username() - // What does the below return for _. It's not err / bool - password, _ = parsedDSN.User.Password() - addr = parsedDSN.Host - if standardDSN { - // Remove slash - db = parsedDSN.Path[1:] - params := parsedDSN.Query() - if params["ssl"] != nil { - tlsConfigName := params.Get("ssl") + ci, err := parseDSN(dsn) + + if err != nil { + return nil, err + } + + if ci.standardDSN { + if ci.params["ssl"] != nil { + tlsConfigName := ci.params.Get("ssl") switch tlsConfigName { case "true": // This actually does insecureSkipVerify // But not even sure if it makes sense to handle false? According to // client_test.go it doesn't - it'd result in an error - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.UseSSL(true) }) + c, err = client.Connect(ci.addr, ci.user, ci.password, ci.db, func(c *client.Conn) { c.UseSSL(true) }) case "custom": // I was too concerned about mimicking what go-sql-driver/mysql does which will // allow any name for a custom tls profile and maps the query parameter value to // that TLSConfig variable... there is no need to be that clever. // Instead of doing that, let's store required custom TLSConfigs in a map that // uses the DSN address as the key - c, err = client.Connect(addr, user, password, db, func(c *client.Conn) { c.SetTLSConfig(customTLSConfigMap[addr]) }) + c, err = client.Connect(ci.addr, ci.user, ci.password, ci.db, func(c *client.Conn) { c.SetTLSConfig(customTLSConfigMap[ci.addr]) }) default: return nil, errors.Errorf("Supported options are ssl=true or ssl=custom") } } else { - c, err = client.Connect(addr, user, password, db) + c, err = client.Connect(ci.addr, ci.user, ci.password, ci.db) } } else { // No more processing here. Let's only support url parameters with the newer style DSN - db = parsedDSN.RawQuery - c, err = client.Connect(addr, user, password, db) + c, err = client.Connect(ci.addr, ci.user, ci.password, ci.db) } if err != nil { return nil, err @@ -264,7 +298,6 @@ func init() { sql.Register("mysql", driver{}) } - // SetCustomTLSConfig sets a custom TLSConfig for the address (host:port) of the supplied DSN. // It requires a full import of the driver (not by side-effects only). // Example of supplying a custom CA, no client cert, no key, validating the diff --git a/driver/driver_test.go b/driver/driver_test.go index 5b62b9692..ba302efca 100644 --- a/driver/driver_test.go +++ b/driver/driver_test.go @@ -3,6 +3,8 @@ package driver import ( "flag" "fmt" + "net/url" + "reflect" "testing" "github.com/jmoiron/sqlx" @@ -78,3 +80,28 @@ func (s *testDriverSuite) TestTransaction(c *C) { err = tx.Commit() c.Assert(err, IsNil) } + +func TestParseDSN(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{}}, + "user@1.domain.com?db": connInfo{standardDSN: false, addr: "1.domain.com", user: "user", password: "", db: "db", params: url.Values{}}, + "user:password@2.domain.com/db": connInfo{standardDSN: true, addr: "2.domain.com", user: "user", password: "password", db: "db", params: url.Values{}}, + "user:password@3.domain.com/db?ssl=true": connInfo{standardDSN: true, addr: "3.domain.com", user: "user", password: "password", db: "db", params: url.Values{"ssl": []string{"true"}}}, + "user:password@4.domain.com/db?ssl=custom": connInfo{standardDSN: true, addr: "4.domain.com", user: "user", password: "password", db: "db", params: url.Values{"ssl": []string{"custom"}}}, + "user:password@5.domain.com/db?unused=param": connInfo{standardDSN: true, addr: "5.domain.com", user: "user", password: "password", db: "db", params: url.Values{"unused": []string{"param"}}}, + } + + for supplied, expected := range testDSNs { + actual, err := parseDSN(supplied) + if err != nil { + t.Errorf("TestParseDSN failed. Got error: %s", err) + } + // Compare that with expected + if !reflect.DeepEqual(actual, expected) { + t.Errorf("TestParseDSN failed.\nExpected:\n%#v\nGot:\n%#v", expected, actual) + } + } +} From 039dcc2b8ff02e9570fad261a7b285edac7538cf Mon Sep 17 00:00:00 2001 From: atomicules Date: Wed, 4 May 2022 14:13:17 +0000 Subject: [PATCH 11/13] golangci-lint issues (unnecessary new lines) I had trouble installing/running locally. Working now. --- driver/driver.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/driver/driver.go b/driver/driver.go index 77ec71e67..232bf0514 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -43,7 +43,6 @@ type connInfo struct { // // Optional parameters are supported in the standard DSN form func parseDSN(dsn string) (connInfo, error) { - var matchErr error ci := connInfo{} @@ -81,7 +80,6 @@ func parseDSN(dsn string) (connInfo, error) { // Open takes a supplied DSN string and opens a connection // See ParseDSN for more information on the form of the DSN func (d driver) Open(dsn string) (sqldriver.Conn, error) { - var c *client.Conn ci, err := parseDSN(dsn) From 8b16e13a4a374c6cd9cf2dfe0dd98d6bb9abf618 Mon Sep 17 00:00:00 2001 From: atomicules Date: Wed, 4 May 2022 14:32:47 +0000 Subject: [PATCH 12/13] Use sync.Mutex when updating the customTLSConfigMap I've not noticed any data race issues, but just to be a bit safer. --- driver/driver.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/driver/driver.go b/driver/driver.go index 232bf0514..9773f2ece 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -10,6 +10,7 @@ import ( "io" "net/url" "regexp" + "sync" "github.com/go-mysql-org/go-mysql/client" "github.com/go-mysql-org/go-mysql/mysql" @@ -17,6 +18,7 @@ import ( "github.com/siddontang/go/hack" ) +var customTLSMutex sync.Mutex // Map of dsn address (makes more sense than full dsn?) to tls Config var customTLSConfigMap = make(map[string]*tls.Config) @@ -315,8 +317,10 @@ func SetCustomTLSConfig(dsn string, caPem []byte, certPem []byte, keyPem []byte, // having multiple CA certs for one hostname is likely when you have services running on // different ports. + customTLSMutex.Lock() // Basic pass-through function so we can just import the driver customTLSConfigMap[addr] = client.NewClientTLSConfig(caPem, certPem, keyPem, insecureSkipVerify, serverName) + customTLSMutex.Unlock() return nil } From 587d49e8be8218e952631b09014cf205f13f8709 Mon Sep 17 00:00:00 2001 From: atomicules Date: Thu, 5 May 2022 06:38:34 +0000 Subject: [PATCH 13/13] Fix linting issue, new line after variable. --- driver/driver.go | 1 + 1 file changed, 1 insertion(+) diff --git a/driver/driver.go b/driver/driver.go index 9773f2ece..482cffafa 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -19,6 +19,7 @@ import ( ) var customTLSMutex sync.Mutex + // Map of dsn address (makes more sense than full dsn?) to tls Config var customTLSConfigMap = make(map[string]*tls.Config)