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.

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

Update PostgreSQL TLS config #853

Merged
merged 4 commits into from
Oct 14, 2020
Merged

Conversation

wxing1292
Copy link
Contributor

@wxing1292 wxing1292 commented Oct 14, 2020

What changed?

  • Allow set CA or key / cert seperately for PostgreSQL
  • Add support of host name verification & fix existing issue

Why?
See above

How did you test it?
Install schema with CA or CA & key & cert

Potential risks
N/A

* Allow set CA or key / cert seperately for PostgreSQL
* Add support of host name verification
@wxing1292 wxing1292 requested review from underrun, sergeybykov and a team October 14, 2020 04:28
Comment on lines -45 to -51
host, _, err := net.SplitHostPort(cfg.ConnectAddr)
if err != nil {
return fmt.Errorf("error in host port from ConnectAddr: %v", err)
}

// TODO: create a way to set MinVersion and CipherSuites via cfg.
tlsConfig := auth.NewTLSConfigForServer(host)
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 is a bug on MySQL

Copy link
Contributor

Choose a reason for hiding this comment

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

because NewTLSConfigForServer is "for connecting as a TLS client to a server with a name we know and may want to verify"? because when i read the function name it makes me think its a config for a server listening on TLS. but this is not an issue with this PR and warrants a bigger discussion about how we are talking about TLS connections.

Comment on lines -45 to -51
host, _, err := net.SplitHostPort(cfg.ConnectAddr)
if err != nil {
return fmt.Errorf("error in host port from ConnectAddr: %v", err)
}

// TODO: create a way to set MinVersion and CipherSuites via cfg.
tlsConfig := auth.NewTLSConfigForServer(host)
Copy link
Contributor

Choose a reason for hiding this comment

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

because NewTLSConfigForServer is "for connecting as a TLS client to a server with a name we know and may want to verify"? because when i read the function name it makes me think its a config for a server listening on TLS. but this is not an issue with this PR and warrants a bigger discussion about how we are talking about TLS connections.

@wxing1292
Copy link
Contributor Author

wxing1292 commented Oct 14, 2020

because NewTLSConfigForServer is "for connecting as a TLS client to a server with a name we know and may want to verify"?

@underrun
nop, from what i understand ServerName and connection address can be different, e.g. IP vs DNS
plus the config EnableHostVerification is not used..
did not pay too much attention with the function name
ref:

cluster.SslOpts = &gocql.SslOptions{
CertPath: cfg.TLS.CertFile,
KeyPath: cfg.TLS.KeyFile,
CaPath: cfg.TLS.CaFile,
EnableHostVerification: cfg.TLS.EnableHostVerification,
Config: auth.NewTLSConfigForServer(cfg.TLS.ServerName),
}

@wxing1292 wxing1292 merged commit d2d09b6 into temporalio:master Oct 14, 2020
@wxing1292 wxing1292 deleted the postgresql-tls branch October 14, 2020 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants