add --pemfile option to support TLS connection on mysql
#180
Conversation
|
@VojtechVitek This is just a friendly reminder that I'm waiting for your review. How about it? Is there any problem with it? |
cmd/goose/main.go
Outdated
| @@ -15,6 +15,7 @@ var ( | |||
| verbose = flags.Bool("v", false, "enable verbose mode") | |||
| help = flags.Bool("h", false, "print help") | |||
| version = flags.Bool("version", false, "print version") | |||
| sslCA = flags.String("ssl-ca", "", "file path to root CA's certificates in pem format (only support on mysql)") | |||
There was a problem hiding this comment.
can we call this "pemfile" or "certfile"?
cmd/goose/main.go
Outdated
| @@ -35,6 +36,13 @@ func main() { | |||
| return | |||
| } | |||
|
|
|||
| tls := *sslCA != "" | |||
| if tls { | |||
There was a problem hiding this comment.
if *sslCA != "" {
is imho more readable
| return config.FormatDSN(), nil | ||
| } | ||
|
|
||
| func registerTLSConfig(pemfile string) error { |
There was a problem hiding this comment.
Can we make normalizeMySQLDSN() and registerTLSConfig() a single function, since it's MySQL specific?
There was a problem hiding this comment.
I didn't combine the functions, but made registerTLSConfig defined only in driver_mysql.go.
There was a problem hiding this comment.
I think that might fail a non-mysql builds
|
Thanks for the review! I revised the issues. @VojtechVitek |
cmd/goose/main.go
Outdated
| verbose = flags.Bool("v", false, "enable verbose mode") | ||
| help = flags.Bool("h", false, "print help") | ||
| version = flags.Bool("version", false, "print version") | ||
| certfile = flags.String("ssl-ca", "", "file path to root CA's certificates in pem format (only support on mysql)") |
There was a problem hiding this comment.
Sorry, I meant the CLI arg to be named "certfile" too
There was a problem hiding this comment.
Oh, I didn't realize your intention. But, the mysql command itself has a --ssl-ca option and I take the name from it. The mysql has a some SSL related options, so I think it seems better to match names.
ref. https://dev.mysql.com/doc/refman/8.0/en/connection-options.html
There was a problem hiding this comment.
How about it? @VojtechVitek
There was a problem hiding this comment.
let's use "certfile" please
eventually, we might use this flag for non-MySQL DBs too, ie. Postgres psql has
--set=sslmode=verify-full --set=sslrootcert=/some-cert.pem
There was a problem hiding this comment.
renamed. Thanks for reviewing.
Songmu
changed the title
-ssl-ca option to support TLS connection on mysql--certfile option to support TLS connection on mysql
|
I fixed you pointed, and this feature has worked fine in my production environment. It would be beneficial if you could merge it. @VojtechVitek |
VojtechVitek
changed the title
--certfile option to support TLS connection on mysql--pemfile option to support TLS connection on mysql
This feature is very useful especially for Amazon RDS.
I Added the
-ssl-caoption to specify a CA certificate to support TLS connection. The naming of this option follows same option name of the mysql command.Using as follows.
rds-combined-ca-bundle.pem is taken from https://s3.amazonaws.com/rds-downloads/rds-combined-ca-bundle.pem.
ref. https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/CHAP_MySQL.html#MySQL.Concepts.SSLSupport