EronWright commented on a change in pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#discussion_r438992095
##########
File path: pulsar/client_impl_test.go
##########
@@ -67,6 +67,24 @@ func TestTLSInsecureConnection(t *testing.T) {
client.Close()
}
+func TestTLSInsecureConnectionWithCerts(t *testing.T) {
Review comment:
This test doesn't seem to do anything substantive. I'd expect it to
exercise the resultant `tls.Config` in some way, maybe by making an actual TLS
connection.
##########
File path: pulsar/internal/connection.go
##########
@@ -711,8 +711,46 @@ func (c *connection) getTLSConfig() (*tls.Config, error) {
}
}
- if c.tlsOptions.ValidateHostname {
- tlsConfig.ServerName = c.physicalAddr.Hostname()
+ tlsConfig.ServerName = c.physicalAddr.Hostname()
+
+ if tlsConfig.InsecureSkipVerify {
Review comment:
Why isn't `c.tlsOptions.ValidateHostname` used anywhere? I was thinking:
```
tlsConfig := &tls.Config{
// disable built-in verification if hostname verification is to be
skipped
InsecureSkipVerify: c.tlsOptions.AllowInsecureConnection ||
!c.tlsOptions.ValidateHostname
}
if !c.tlsOptions.AllowInsecureConnection && tlsConfig.InsecureSkipVerify {
// implement a limited verification routine that checks the certificate
but skips hostname verification
tlsConfig.VerifyPeerCertificate = func() {...}
}
```
##########
File path: pulsar/internal/connection.go
##########
@@ -711,8 +711,46 @@ func (c *connection) getTLSConfig() (*tls.Config, error) {
}
}
- if c.tlsOptions.ValidateHostname {
- tlsConfig.ServerName = c.physicalAddr.Hostname()
+ tlsConfig.ServerName = c.physicalAddr.Hostname()
+
+ if tlsConfig.InsecureSkipVerify {
+ // Solution is credited to
https://github.com/golang/go/issues/21971
+ // Code is adapted from the original implementation of
handshake_client.go at
+ //
https://github.com/golang/go/blob/master/src/crypto/tls/handshake_client.go#L804
+ // disable the default verification; use customized
VerifyPeerCertificate
+ tlsConfig.VerifyPeerCertificate = func(rawCerts [][]byte,
certChain [][]*x509.Certificate) error {
+ // If this is the first handshake on a connection,
process and
+ // (optionally) verify the server's certificates.
+ certs := make([]*x509.Certificate, len(rawCerts))
+ for i, asn1Data := range rawCerts {
+ cert, err := x509.ParseCertificate(asn1Data)
+ if err != nil {
+ return fmt.Errorf("tls: failed to parse
server certificate error: %s", err.Error())
+ }
+ certs[i] = cert
+ }
+
+ if tlsConfig.RootCAs == nil {
+ return nil
+ }
Review comment:
How would this deal with the common scenario where the client is using
the system's bundle? I am unsure of whether `RootCAs` is `nil` in that
scenario.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
