Initial SSL Connection Support #705

brandonbodnar-wk · 2019-01-31T21:40:03Z

Related issue: #521

Description

This PR adds initial support for enforcing TLS/SSL connections for the databases involved in the schema changes. In this initial pass, a gh-ost user can enforce ssl be used for connections using the --ssl flag on the command line. If the user wishes to enforce checking validity of the database's certificate, the user can also specify a CA certificate in pem format with the --ssl-ca option.

Adding these two options is sufficient for our use case right now in connecting to AWS RDS instances. While there are plenty more options that can be specified by the native mysql client regarding ssl, this can provide a good starting point for other work by those with more complex needs.

contributed code is using same conventions as original code
script/cibuild returns with no formatting errors, build errors or unit test errors.

Testing

We tested this updated code against an Aurora2 cluster running a MySQL 5.7 compatible engine.

For testing we added a user to the mysql cluster with REQUIRE SSL set.
We then attempted to run an alter table command using gh-ost against the cluster, but without the new ssl options enabled. As expected we received an access denied when attempting to connect with gh-ost.
We then attempted the same alter table command using all the same options, except also adding on the new --ssl flag, along with specifying the --ssl-ca certificate for AWS RDS. The job executed flawlessly.

- Adding a command line option for users to enforce tls/ssl connections for the applier, inspector, and binlog reader. - The user can optionally request server certificate verification through a command line option to specify the ca cert via a file path. - Fixes an existing bug appending the timeout option to the singleton applier connection.

brandonbodnar-wk · 2019-01-31T21:49:48Z

go/logic/applier.go

@@ -73,7 +73,7 @@ func (this *Applier) InitDBConnections() (err error) {
 	if this.db, _, err = mysql.GetDB(this.migrationContext.Uuid, applierUri); err != nil {
 		return err
 	}
-	singletonApplierUri := fmt.Sprintf("%s?timeout=0", applierUri)
+	singletonApplierUri := fmt.Sprintf("%s&timeout=0", applierUri)


This was an existing bug we found when adding the tls option onto the uri. Previously, this was causing the uri to take the form of gromit:penguin@tcp(myhost:3306)/test?interpolateParams=true&autocommit=true&charset=utf8mb4,utf8,latin1?timeout=0. The DSN parser for go-mysql-driver/mysql was then not parsing the timeout setting, but instead including that as part of the value for charset.

What if applierUri has no query string yet?

@johnlockwood-wf applierUri has query strings from the return value of ConnectionConfig.GetDBUri both before and after this PR.

Why not use net/url to work with urls?

I rather not refactor every place where GetDBUri is currently being used to address this bug. Because we are adding the tls param into GetDBUri, we are directly impacted by this line using the wrong symbol and thus I am including the fix here to use the correct symbol.

These are not really URLs but DSNs recognized by go-my-sql/driver. While net/url.Values may have some helpful functions, I don't think the work involved in shoehorning that in is worthwhile here where each code path that hits this spot already has params set for applierUri.

go-sql-driver's Config.FormatDSN would likely be a great improvement on the manually created DSN, but I think that is something to reserve for a separate issue or PR.

Agreed. Let's postpone use of FormatDNS to a next PR.

brandonbodnar-wk · 2019-01-31T21:53:07Z

go/cmd/gh-ost/main.go

@@ -14,6 +14,7 @@ import (

 	"github.com/github/gh-ost/go/base"
 	"github.com/github/gh-ost/go/logic"
+	_ "github.com/go-sql-driver/mysql"


Added this to make it clear that gh-ost was registering the go-sql-driver/mysql driver as its mysql driver. We had some confusion previously and thought that the driver might have been coming from the siddontang/go-mysql library referenced in the go/binlog/gomysql_reader.go. But, the driver was in fact the go-sql-driver/mysql, pulled in though an import statement located within github.com/outbrain/golib/sqlutils. Hoping that adding the import directly here instead of relying on the indirect import will make it more clear for others.

tomdeering-wf

You made that look easy, good work

go/base/context.go

go/mysql/connection.go

ericklaus-wf · 2019-01-31T22:19:14Z

go/cmd/gh-ost/main.go

@@ -14,6 +14,7 @@ import (

 	"github.com/github/gh-ost/go/base"
 	"github.com/github/gh-ost/go/logic"
+	_ "github.com/go-sql-driver/mysql"


Should we add a comment explaining why we're importing this?

I don't believe so. Not use to seeing comments within import statements, and this brings the usage much closer to the standard usage described in the go-sql-driver/mysql documentation. Potentially we could move this into the one of the classes where database/sql is being imported to exactly match the documentation, but that seems less discoverable to me than adding here in main.

brandonbodnar-wk · 2019-02-01T14:28:15Z

I've updated the description of this PR to include some of the steps we took in testing this change against some AWS RDS instances.

go/cmd/gh-ost/main.go

natewernimont-wk · 2019-02-01T16:40:39Z

go/mysql/connection.go

@@ -16,6 +22,7 @@ type ConnectionConfig struct {
 	User       string
 	Password   string
 	ImpliedKey *InstanceKey
+	tlsConfig  *tls.Config


Any reason not to make this variable public since you have a simple getter down below anyway?

Wanted to force callers consuming this struct to set up the TLS for the ConnectionConfig through the UseTLS function below if they wanted to set up the tls.Config. This prevents passing around a ConnectionConfig whose tls.Config has not be registered with the mysql driver.

natewernimont-wk · 2019-02-01T16:42:29Z

go/cmd/gh-ost/main.go

@@ -54,6 +55,9 @@ func main() {
 	flag.StringVar(&migrationContext.ConfigFile, "conf", "", "Config file")
 	askPass := flag.Bool("ask-pass", false, "prompt for MySQL password")

+	flag.BoolVar(&migrationContext.UseTLS, "ssl", false, "Enable SSL encrypted connections to MySQL")
+	flag.StringVar(&migrationContext.TLSCACertificate, "ssl-ca", "", "CA certificate in PEM format for TLS connections. Requires --ssl")


Should there be a note that these settings will apply to both the replica and master with no option to differentiate them?

Thanks. I've expanded the usage statements a little to make clear that it applies to all the MySQL hosts, while trying to keep the statements concise and general enough in case someone else needs to add a master override (similar to the username and password overrides) in the future. I imagine needing to have different CA or TLS settings for the master and replicas is probably not a super common case, but something that can definitely be added in the future.

brandonbodnar-wk · 2019-02-04T15:19:58Z

@shlomi-noach If there is anything I can add to this PR that would make review on the maintainers side easier, please let me know. Adding SSL support is a priority for us and I would like to make it as easy as possible for you.

shlomi-noach

@brandonbodnar-wk thank you! This PR is looking good. I'll run it through internal testing, though our testing will not check the introduced functionality, so more in the way of verifying nothing gets broken.

I have one further request: if you can update the docs to reflect the changes you made, we'd be most grateful.

brandonbodnar-wk · 2019-02-05T14:05:39Z

Thanks @shlomi-noach. I've added documentation for the flags into command-line-flags.md in this latest commit.

shlomi-noach · 2019-02-10T13:30:59Z

Thank you

brandonbodnar-wk mentioned this pull request Jan 31, 2019

Add support for SSL #521

Open

brandonbodnar-wk commented Jan 31, 2019

View reviewed changes

tomdeering-wf reviewed Jan 31, 2019

View reviewed changes

go/base/context.go Outdated Show resolved Hide resolved

go/mysql/connection.go Outdated Show resolved Hide resolved

go/mysql/connection.go Show resolved Hide resolved

ericklaus-wf reviewed Jan 31, 2019

View reviewed changes

brandonbodnar-wk added 3 commits January 31, 2019 17:00

Fix casing for initialism

4f9367e

Remove unnecessary branching

dc599bb

Explain default setting for TLS param in DSN

c440112

tomdeering-wf approved these changes Feb 1, 2019

View reviewed changes

natewernimont-wk reviewed Feb 1, 2019

View reviewed changes

brandonbodnar-wk added 2 commits February 1, 2019 13:16

Handle returned error

f67ec15

Expand usage statement to indicate setting applies to multiple hosts

5319157

ericklaus-wf mentioned this pull request Feb 4, 2019

Add tls support Workiva/gh-ost#1

Closed

brandonbodnar-wk mentioned this pull request Feb 4, 2019

Add tls support Workiva/gh-ost#2

Merged

matthewbelisle-wf and others added 2 commits February 4, 2019 14:46

Adding --ssl-insecure flag

79df0d1

Wireup allowing insecure ssl

5b0dfb0

shlomi-noach approved these changes Feb 5, 2019

View reviewed changes

shlomi-noach temporarily deployed to production/mysql_role=ghost_testing February 5, 2019 08:39 Inactive

Document ssl-related command line flags.

1543098

Shlomi Noach added 2 commits February 10, 2019 11:27

Merge branch 'master' into add-tls-support

9da2e21

Merge branch 'master' into add-tls-support

09ef7f4

shlomi-noach merged commit 4ff5d6a into github:master Feb 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial SSL Connection Support #705

Initial SSL Connection Support #705

brandonbodnar-wk commented Jan 31, 2019 •

edited

Loading

brandonbodnar-wk Jan 31, 2019

johnlockwood-wf Jan 31, 2019

brandonbodnar-wk Jan 31, 2019

johnlockwood-wf Jan 31, 2019

brandonbodnar-wk Jan 31, 2019 •

edited

Loading

brandonbodnar-wk Jan 31, 2019

shlomi-noach Feb 5, 2019

brandonbodnar-wk Jan 31, 2019

tomdeering-wf left a comment •

edited

Loading

ericklaus-wf Jan 31, 2019

brandonbodnar-wk Jan 31, 2019

brandonbodnar-wk commented Feb 1, 2019

natewernimont-wk Feb 1, 2019

brandonbodnar-wk Feb 1, 2019

natewernimont-wk Feb 1, 2019

brandonbodnar-wk Feb 1, 2019 •

edited

Loading

brandonbodnar-wk commented Feb 4, 2019

shlomi-noach left a comment

brandonbodnar-wk commented Feb 5, 2019

shlomi-noach commented Feb 10, 2019

Initial SSL Connection Support #705

Initial SSL Connection Support #705

Conversation

brandonbodnar-wk commented Jan 31, 2019 • edited Loading

Description

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonbodnar-wk Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdeering-wf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonbodnar-wk commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonbodnar-wk Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

brandonbodnar-wk commented Feb 4, 2019

shlomi-noach left a comment

Choose a reason for hiding this comment

brandonbodnar-wk commented Feb 5, 2019

shlomi-noach commented Feb 10, 2019

brandonbodnar-wk commented Jan 31, 2019 •

edited

Loading

brandonbodnar-wk Jan 31, 2019 •

edited

Loading

tomdeering-wf left a comment •

edited

Loading

brandonbodnar-wk Feb 1, 2019 •

edited

Loading