Skip to content

Initial SSL Connection Support #705

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

Merged
merged 11 commits into from
Feb 10, 2019
Merged

Initial SSL Connection Support #705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
23617f2
Add initial support for ssl encryption connections to database servers.
brandonbodnar-wk Jan 31, 2019
4f9367e
Fix casing for initialism
brandonbodnar-wk Jan 31, 2019
dc599bb
Remove unnecessary branching
brandonbodnar-wk Jan 31, 2019
c440112
Explain default setting for TLS param in DSN
brandonbodnar-wk Jan 31, 2019
f67ec15
Handle returned error
brandonbodnar-wk Feb 1, 2019
5319157
Expand usage statement to indicate setting applies to multiple hosts
brandonbodnar-wk Feb 1, 2019
79df0d1
Adding --ssl-insecure flag
matthewbelisle-wf Feb 4, 2019
5b0dfb0
Wireup allowing insecure ssl
brandonbodnar-wk Feb 4, 2019
1543098
Document ssl-related command line flags.
brandonbodnar-wk Feb 5, 2019
9da2e21
Merge branch 'master' into add-tls-support
Feb 10, 2019
09ef7f4
Merge branch 'master' into add-tls-support
Feb 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions doc/command-line-flags.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,18 @@ By default `gh-ost` verifies no foreign keys exist on the migrated table. On ser

See [`approve-renamed-columns`](#approve-renamed-columns)

### ssl

By default `gh-ost` does not use ssl/tls connections to the database servers when performing migrations. This flag instructs `gh-ost` to use encrypted connections. If enabled, `gh-ost` will use the system's ca certificate pool for server certificate verification. If a different certificate is needed for server verification, see `--ssl-ca`. If you wish to skip server verification, but still use encrypted connections, use with `--ssl-allow-insecure`.

### ssl-allow-insecure

Allows `gh-ost` to connect to the MySQL servers using encrypted connections, but without verifying the validity of the certificate provided by the server during the connection. Requires `--ssl`.

### ssl-ca

`--ssl-ca=/path/to/ca-cert.pem`: ca certificate file (in PEM format) to use for server certificate verification. If specified, the default system ca cert pool will not be used for verification, only the ca cert provided here. Requires `--ssl`.

### test-on-replica

Issue the migration on a replica; do not modify data on master. Useful for validating, testing and benchmarking. See [`testing-on-replica`](testing-on-replica.md)
Expand Down
10 changes: 10 additions & 0 deletions go/base/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ type MigrationContext struct {
ConfigFile string
CliUser string
CliPassword string
UseTLS bool
TLSAllowInsecure bool
TLSCACertificate string
CliMasterUser string
CliMasterPassword string

Expand Down Expand Up @@ -697,6 +700,13 @@ func (this *MigrationContext) ApplyCredentials() {
}
}

func (this *MigrationContext) SetupTLS() error {
if this.UseTLS {
return this.InspectorConnectionConfig.UseTLS(this.TLSCACertificate, this.TLSAllowInsecure)
}
return nil
}

// ReadConfigFile attempts to read the config file, if it exists
func (this *MigrationContext) ReadConfigFile() error {
this.configMutex.Lock()
Expand Down
1 change: 1 addition & 0 deletions go/binlog/gomysql_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func NewGoMySQLReader(migrationContext *base.MigrationContext) (binlogReader *Go
Port: uint16(binlogReader.connectionConfig.Key.Port),
User: binlogReader.connectionConfig.User,
Password: binlogReader.connectionConfig.Password,
TLSConfig: binlogReader.connectionConfig.TLSConfig(),
UseDecimal: true,
}
binlogReader.binlogSyncer = replication.NewBinlogSyncer(binlogSyncerConfig)
Expand Down
14 changes: 14 additions & 0 deletions go/cmd/gh-ost/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"github.com/outbrain/golib/log"

"golang.org/x/crypto/ssh/terminal"
Expand Down Expand Up @@ -54,6 +55,10 @@ 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 hosts")
flag.StringVar(&migrationContext.TLSCACertificate, "ssl-ca", "", "CA certificate in PEM format for TLS connections to MySQL hosts. Requires --ssl")
flag.BoolVar(&migrationContext.TLSAllowInsecure, "ssl-allow-insecure", false, "Skips verification of MySQL hosts' certificate chain and host name. Requires --ssl")

flag.StringVar(&migrationContext.DatabaseName, "database", "", "database name (mandatory)")
flag.StringVar(&migrationContext.OriginalTableName, "table", "", "table name (mandatory)")
flag.StringVar(&migrationContext.AlterStatement, "alter", "", "alter statement (mandatory)")
Expand Down Expand Up @@ -196,6 +201,12 @@ func main() {
if migrationContext.CliMasterPassword != "" && migrationContext.AssumeMasterHostname == "" {
log.Fatalf("--master-password requires --assume-master-host")
}
if migrationContext.TLSCACertificate != "" && !migrationContext.UseTLS {
log.Fatalf("--ssl-ca requires --ssl")
}
if migrationContext.TLSAllowInsecure && !migrationContext.UseTLS {
log.Fatalf("--ssl-allow-insecure requires --ssl")
}
if *replicationLagQuery != "" {
log.Warningf("--replication-lag-query is deprecated")
}
Expand Down Expand Up @@ -240,6 +251,9 @@ func main() {
migrationContext.SetThrottleHTTP(*throttleHTTP)
migrationContext.SetDefaultNumRetries(*defaultRetries)
migrationContext.ApplyCredentials()
if err := migrationContext.SetupTLS(); err != nil {
log.Fatale(err)
}
if err := migrationContext.SetCutOverLockTimeoutSeconds(*cutOverLockTimeoutSeconds); err != nil {
log.Errore(err)
}
Expand Down
2 changes: 1 addition & 1 deletion go/logic/applier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if applierUri has no query string yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use net/url to work with urls?

Copy link
Contributor Author

@brandonbodnar-wk brandonbodnar-wk Jan 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if this.singletonDB, _, err = mysql.GetDB(this.migrationContext.Uuid, singletonApplierUri); err != nil {
return err
}
Expand Down
58 changes: 53 additions & 5 deletions go/mysql/connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,14 @@
package mysql

import (
"crypto/tls"
"crypto/x509"
"errors"
"fmt"
"io/ioutil"
"net"

"github.com/go-sql-driver/mysql"
)

// ConnectionConfig is the minimal configuration required to connect to a MySQL server
Expand All @@ -16,6 +22,7 @@ type ConnectionConfig struct {
User string
Password string
ImpliedKey *InstanceKey
tlsConfig *tls.Config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func NewConnectionConfig() *ConnectionConfig {
Expand All @@ -29,9 +36,10 @@ func NewConnectionConfig() *ConnectionConfig {
// DuplicateCredentials creates a new connection config with given key and with same credentials as this config
func (this *ConnectionConfig) DuplicateCredentials(key InstanceKey) *ConnectionConfig {
config := &ConnectionConfig{
Key: key,
User: this.User,
Password: this.Password,
Key: key,
User: this.User,
Password: this.Password,
tlsConfig: this.tlsConfig,
}
config.ImpliedKey = &config.Key
return config
Expand All @@ -42,13 +50,47 @@ func (this *ConnectionConfig) Duplicate() *ConnectionConfig {
}

func (this *ConnectionConfig) String() string {
return fmt.Sprintf("%s, user=%s", this.Key.DisplayString(), this.User)
return fmt.Sprintf("%s, user=%s, usingTLS=%t", this.Key.DisplayString(), this.User, this.tlsConfig != nil)
}

func (this *ConnectionConfig) Equals(other *ConnectionConfig) bool {
return this.Key.Equals(&other.Key) || this.ImpliedKey.Equals(other.ImpliedKey)
}

func (this *ConnectionConfig) UseTLS(caCertificatePath string, allowInsecure bool) error {
var rootCertPool *x509.CertPool
var err error

if !allowInsecure {
if caCertificatePath == "" {
rootCertPool, err = x509.SystemCertPool()
if err != nil {
return err
}
} else {
rootCertPool = x509.NewCertPool()
pem, err := ioutil.ReadFile(caCertificatePath)
if err != nil {
return err
}
if ok := rootCertPool.AppendCertsFromPEM(pem); !ok {
return errors.New("could not add ca certificate to cert pool")
}
}
}

this.tlsConfig = &tls.Config{
RootCAs: rootCertPool,
InsecureSkipVerify: allowInsecure,
}

return mysql.RegisterTLSConfig(this.Key.StringCode(), this.tlsConfig)
}

func (this *ConnectionConfig) TLSConfig() *tls.Config {
return this.tlsConfig
}

func (this *ConnectionConfig) GetDBUri(databaseName string) string {
hostname := this.Key.Hostname
var ip = net.ParseIP(hostname)
Expand All @@ -57,5 +99,11 @@ func (this *ConnectionConfig) GetDBUri(databaseName string) string {
hostname = fmt.Sprintf("[%s]", hostname)
}
interpolateParams := true
return fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?interpolateParams=%t&autocommit=true&charset=utf8mb4,utf8,latin1", this.User, this.Password, hostname, this.Key.Port, databaseName, interpolateParams)
// go-mysql-driver defaults to false if tls param is not provided; explicitly setting here to
// simplify construction of the DSN below.
tlsOption := "false"
if this.tlsConfig != nil {
tlsOption = this.Key.StringCode()
}
return fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?interpolateParams=%t&autocommit=true&charset=utf8mb4,utf8,latin1&tls=%s", this.User, this.Password, hostname, this.Key.Port, databaseName, interpolateParams, tlsOption)
}
19 changes: 18 additions & 1 deletion go/mysql/connection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package mysql

import (
"crypto/tls"
"testing"

"github.com/outbrain/golib/log"
Expand All @@ -31,6 +32,10 @@ func TestDuplicateCredentials(t *testing.T) {
c.Key = InstanceKey{Hostname: "myhost", Port: 3306}
c.User = "gromit"
c.Password = "penguin"
c.tlsConfig = &tls.Config{
InsecureSkipVerify: true,
ServerName: "feathers",
}

dup := c.DuplicateCredentials(InstanceKey{Hostname: "otherhost", Port: 3310})
test.S(t).ExpectEquals(dup.Key.Hostname, "otherhost")
Expand All @@ -39,6 +44,7 @@ func TestDuplicateCredentials(t *testing.T) {
test.S(t).ExpectEquals(dup.ImpliedKey.Port, 3310)
test.S(t).ExpectEquals(dup.User, "gromit")
test.S(t).ExpectEquals(dup.Password, "penguin")
test.S(t).ExpectEquals(dup.tlsConfig, c.tlsConfig)
}

func TestDuplicate(t *testing.T) {
Expand All @@ -63,5 +69,16 @@ func TestGetDBUri(t *testing.T) {
c.Password = "penguin"

uri := c.GetDBUri("test")
test.S(t).ExpectEquals(uri, "gromit:penguin@tcp(myhost:3306)/test?interpolateParams=true&autocommit=true&charset=utf8mb4,utf8,latin1")
test.S(t).ExpectEquals(uri, "gromit:penguin@tcp(myhost:3306)/test?interpolateParams=true&autocommit=true&charset=utf8mb4,utf8,latin1&tls=false")
}

func TestGetDBUriWithTLSSetup(t *testing.T) {
c := NewConnectionConfig()
c.Key = InstanceKey{Hostname: "myhost", Port: 3306}
c.User = "gromit"
c.Password = "penguin"
c.tlsConfig = &tls.Config{}

uri := c.GetDBUri("test")
test.S(t).ExpectEquals(uri, "gromit:penguin@tcp(myhost:3306)/test?interpolateParams=true&autocommit=true&charset=utf8mb4,utf8,latin1&tls=myhost:3306")
}