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.

Sign up for GitHub

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

Jump to bottom

Add support for SSL #521

Open
igroene opened this issue Nov 13, 2017 · 11 comments
Open

Add support for SSL #521

igroene opened this issue Nov 13, 2017 · 11 comments
Labels
enhancement

Comments

@igroene
Copy link

igroene commented Nov 13, 2017

I came across some use cases where strict compliance rules enforce that all connections to the db's need to use SSL. It would be nice if gh-ost supported that option.
It looks like go-mysql-driver already has support (go-sql-driver/mysql#89)

Also for reference: https://dev.mysql.com/doc/refman/5.7/en/using-encrypted-connections.html
@zmoazeni
Copy link
Contributor

We have been looking into this as well. One option Percona mentioned to me was to define the SSL options in the my.cnf and set the user you use to require SSL.

I haven't had a chance to test this yet but it has been on my radar. You may also need to setup the existing replication user the same way.

For what it's worth, I've also been thinking about this in the context of orchestrator (failing over replication using SSL when that happens)

@igroene
Copy link
Author

igroene commented Nov 14, 2017

We have been looking into this as well. One option Percona mentioned to me was to define the SSL options in the my.cnf and set the user you use to require SSL.

That won't work for gh-ost as it doesn't use the standard mysql client to connect

@zmoazeni
Copy link
Contributor

Ah that makes sense. That might only work for orchestrator issuing CHANGE MASTER TO... without supplying the SSL options. (Since it's mysqld connecting to mysqld). Bummer.

@shlomi-noach
Copy link
Contributor

shlomi-noach commented Nov 14, 2017 •
edited
Loading

Well, orchestrator uses the exact same client to connect to the servers via SSL. It just supports SSL code-wise, as per https://github.com/github/orchestrator/blob/master/docs/ssl-and-tls.md , and we'd need to apply roughly the same to gh-ost.

@wesleyk
Copy link

wesleyk commented Jul 18, 2018

Any update on this request @shlomi-noach? If no one has started on this, is there perhaps a sample PR from orchestrator that you could link here that we might use to add SSL support in?

Thanks!

@shlomi-noach
Copy link
Contributor

@wesleyk I have no immediate plan to work on this. Relevant orchestrator PRs:

@wesleyk
Copy link

wesleyk commented Jul 19, 2018

Sounds good, thanks for the PRs!

@brandonbodnar-wk brandonbodnar-wk mentioned this issue Jan 31, 2019
Merged
2 tasks
@brandonbodnar-wk
Copy link
Contributor

We experienced a recent need for SSL support while using the gh-ost tool on AWS RDS instances. I've submitted #705 as an initial pass of what I think is the minimum support we need for our use case. While it doesn't cover every ssl setting that users might want to select, it can provide at least a starting point to adding in some level of SSL/TLS support into gh-ost.

@RafeKettler
Copy link

@shlomi-noach I am also interested in SSL support. I tried @brandonbodnar-wk's patch last week, and had to make some further changes to get SSL to work in my use case. The specific issue I have found is it does not seem like SSL options passed to the CLI are properly used in all places we connect to MySQL; this was causing errors while gh-ost was trying to walk from the replica up to the master:

2019-02-13 00:26:57 INFO starting gh-ost
2019-02-13 00:26:57 INFO Migrating `rafetest_staging`.`foo`
2019-02-13 00:26:57 INFO connection validated on host2
2019-02-13 00:26:57 INFO User has ALL privileges
2019-02-13 00:26:57 INFO binary logs validated on host2
2019-02-13 00:26:57 INFO Restarting replication on host2 to make sure binlog settings apply to replication thread
2019-02-13 00:26:58 INFO Inspector initiated on host2, version 5.6.42-84.2-log
2019-02-13 00:26:58 INFO Table found. Engine=InnoDB
2019-02-13 00:26:58 INFO Estimated number of rows via EXPLAIN: 3
2019-02-13 00:26:58 INFO Recursively searching for replication master
2019-02-13 00:26:58 ERROR invalid value / unknown config name: host1
2019-02-13 00:26:58 INFO Tearing down inspector
2019-02-13 00:26:58 FATAL invalid value / unknown config name: host1

Full disclosure, I edited the output above to remove actual hostnames and replace them with host1 and host2. Error is coming from attempting to use a TLS config called "host1" when mysql.RegisterTLSConfig was never called to register a config "host1". I am working on tests and plan on opening the PR in the next week.

@brandonbodnar-wk
Copy link
Contributor

Interesting @RafeKettler. I'll admit, I have not tested the patch for ssl support on mysql setups other than with AWS RDS, which I know acts differently than other setups in gh-ost.

RafeKettler pushed a commit to RafeKettler/gh-ost that referenced this issue Feb 22, 2019
Improve SSL support
b4233df 
Related issue: github#521

 - Add --ssl-cert and --ssl-key options to specify SSL public/private
   key files
 - Allow combining --ssl-allow-insecure with other --ssl* flags.
   `mysql.RegisterTLSConfig` allows combining the corresponding
   parameters in the `tls.Config` it receives, so gh-ost should
   allow this. I found being able to pass --ssl-allow-insecure along
   with --ssl-ca, --ssl-cert, and --ssl-key useful in testing.
 - Use the same TLS config everywhere. Since the CLI only supports
   a single set of --ssl* configuration parameters, this should be
   fine -- `mysql.RegisterTLSConfig` documentation indicates the
   TLS config given will not be modified, so it can safely be used
   in many goroutines provided we also do not modify it. The previous
   implementation did not work when the TLS config was duplicated,
   which happens when gh-ost walks up the replication chain trying
   to find the master. This is because, when the config is duplicated,
   we must call `RegisterTLSConfig` again with the new config. This
   config is exactly the same, so it's easiest to side-step the issue
   by registering the TLS config once and using it everywhere.
RafeKettler pushed a commit to RafeKettler/gh-ost that referenced this issue Feb 22, 2019
Improve SSL support
f0867ad 
Related issue: github#521

 - Add --ssl-cert and --ssl-key options to specify SSL public/private
   key files
 - Allow combining --ssl-allow-insecure with other --ssl* flags.
   `mysql.RegisterTLSConfig` allows combining the corresponding
   parameters in the `tls.Config` it receives, so gh-ost should
   allow this. I found being able to pass --ssl-allow-insecure along
   with --ssl-ca, --ssl-cert, and --ssl-key useful in testing.
 - Use the same TLS config everywhere. Since the CLI only supports
   a single set of --ssl* configuration parameters, this should be
   fine -- `mysql.RegisterTLSConfig` documentation indicates the
   TLS config given will not be modified, so it can safely be used
   in many goroutines provided we also do not modify it. The previous
   implementation did not work when the TLS config was duplicated,
   which happens when gh-ost walks up the replication chain trying
   to find the master. This is because, when the config is duplicated,
   we must call `RegisterTLSConfig` again with the new config. This
   config is exactly the same, so it's easiest to side-step the issue
   by registering the TLS config once and using it everywhere.
RafeKettler pushed a commit to RafeKettler/gh-ost that referenced this issue Feb 22, 2019
Improve SSL support
b4da7cf 
Related issue: github#521

 - Add --ssl-cert and --ssl-key options to specify SSL public/private
   key files
 - Allow combining --ssl-allow-insecure with other --ssl* flags.
   `mysql.RegisterTLSConfig` allows combining the corresponding
   parameters in the `tls.Config` it receives, so gh-ost should
   allow this. I found being able to pass --ssl-allow-insecure along
   with --ssl-ca, --ssl-cert, and --ssl-key useful in testing.
 - Use the same TLS config everywhere. Since the CLI only supports
   a single set of --ssl* configuration parameters, this should be
   fine -- `mysql.RegisterTLSConfig` documentation indicates the
   TLS config given will not be modified, so it can safely be used
   in many goroutines provided we also do not modify it. The previous
   implementation did not work when the TLS config was duplicated,
   which happens when gh-ost walks up the replication chain trying
   to find the master. This is because, when the config is duplicated,
   we must call `RegisterTLSConfig` again with the new config. This
   config is exactly the same, so it's easiest to side-step the issue
   by registering the TLS config once and using it everywhere.
@RafeKettler RafeKettler mentioned this issue Feb 22, 2019
Merged
@RafeKettler
Copy link

RafeKettler commented Feb 22, 2019 •
edited by shlomi-noach
Loading

I opened PR #716 to address the issues I mentioned. @shlomi-noach mind taking a look when you get a chance? @brandonbodnar-wk would also love your feedback, especially on whether this breaks your usage of gh-ost on RDS :)

@dbussink dbussink mentioned this issue Jun 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@zmoazeni @RafeKettler @wesleyk @shlomi-noach @timvaillancourt @igroene @brandonbodnar-wk