[GitHub] [pulsar-client-go] EronWright commented on a change in pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org