Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Initial SSL Connection Support #705
Conversation
- 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.
| @@ -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) | |||
brandonbodnar-wk
Jan 31, 2019
Author
Contributor
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.
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.
johnlockwood-wf
Jan 31, 2019
What if applierUri has no query string yet?
What if applierUri has no query string yet?
brandonbodnar-wk
Jan 31, 2019
Author
Contributor
@johnlockwood-wf applierUri has query strings from the return value of ConnectionConfig.GetDBUri both before and after this PR.
@johnlockwood-wf applierUri has query strings from the return value of ConnectionConfig.GetDBUri both before and after this PR.
johnlockwood-wf
Jan 31, 2019
Why not use net/url to work with urls?
Why not use net/url to work with urls?
brandonbodnar-wk
Jan 31, 2019
•
Author
Contributor
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.
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.
brandonbodnar-wk
Jan 31, 2019
Author
Contributor
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.
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.
shlomi-noach
Feb 5, 2019
Contributor
Agreed. Let's postpone use of FormatDNS to a next PR.
Agreed. Let's postpone use of FormatDNS to a next PR.
| @@ -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" | |||
brandonbodnar-wk
Jan 31, 2019
Author
Contributor
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.
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.
|
You made that look easy, good work |
| @@ -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" | |||
ericklaus-wf
Jan 31, 2019
Should we add a comment explaining why we're importing this?
Should we add a comment explaining why we're importing this?
brandonbodnar-wk
Jan 31, 2019
Author
Contributor
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.
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.
|
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. |
| @@ -16,6 +22,7 @@ type ConnectionConfig struct { | |||
| User string | |||
| Password string | |||
| ImpliedKey *InstanceKey | |||
| tlsConfig *tls.Config | |||
natewernimont-wk
Feb 1, 2019
Any reason not to make this variable public since you have a simple getter down below anyway?
Any reason not to make this variable public since you have a simple getter down below anyway?
brandonbodnar-wk
Feb 1, 2019
Author
Contributor
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.
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.
| @@ -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") | |||
natewernimont-wk
Feb 1, 2019
Should there be a note that these settings will apply to both the replica and master with no option to differentiate them?
Should there be a note that these settings will apply to both the replica and master with no option to differentiate them?
brandonbodnar-wk
Feb 1, 2019
•
Author
Contributor
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.
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.
|
@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. |
|
@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. |
|
Thanks @shlomi-noach. I've added documentation for the flags into command-line-flags.md in this latest commit. |
|
Thank you |
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
--sslflag 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-caoption.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.
script/cibuildreturns 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.
REQUIRE SSLset.gh-ostagainst the cluster, but without the new ssl options enabled. As expected we received an access denied when attempting to connect withgh-ost.--sslflag, along with specifying the--ssl-cacertificate for AWS RDS. The job executed flawlessly.