You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/05/03 19:52:01 UTC
[GitHub] [pulsar-client-go] zzzming opened a new pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
zzzming opened a new pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238
### Contribution Checklist
This PR addresses a problem how to disable TLS ValidateHostname. The current implementation with empty `tlsConfig.ServerName` would not work, because the connected server host will be inferred in absence of tlsConfig.ServerName by Go Tls library. One of the use cases is when DNS name in the server certificate returned does not match the broker host name that the client connects to. The client can be deployed within the same Pulsar kubernetes cluster if the client connects to the internal proxy or broker host directly instead of the public fqdn. This problem may also rise for self-signed cert.
This specific problem and solution are described by this issue report, https://github.com/golang/go/issues/21971
This PR implements a TLS VerifyPeerCertificate callback to skip host name validation if any client chooses to disable TLSValidateHostname in ClientOption.
I understand TLSValidateHostname is false by default because Go initializes bool as `false`. There is an existing issue #171 that is tracking the problem. I think it will open up a discussion how to support backward compatibility that might require consensus from the community. Therefore, altering the current default is beyond the scope of this PR.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (yes) Yes the current implementation of skip hostname check is broken, this is a fix.
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
### Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (not applicable)
----------------------------------------------------------------
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
[GitHub] [pulsar-client-go] zzzming commented on a change in pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by GitBox <gi...@apache.org>.
zzzming commented on a change in pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#discussion_r430440967
##########
File path: pulsar/internal/connection.go
##########
@@ -713,6 +713,41 @@ func (c *connection) getTLSConfig() (*tls.Config, error) {
if c.tlsOptions.ValidateHostname {
tlsConfig.ServerName = c.physicalAddr.Hostname()
Review comment:
@EronWright you are right that ServerName should be set as suggested by https://github.com/golang/go/blob/62a3f2e27c7732656bb3ae8f14047b74a9956e77/src/crypto/tls/common.go#L542
I think the problem is the default value of TLSValidateHostname is `false`. It should have been called `DisabledTLSValidateHostname`. Do you have any suggestion how to handle backward compatibility of TLSValidateHostname?
I made an update to add ServerName but only skip verification if InsecureSkipVerify is true.
----------------------------------------------------------------
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
[GitHub] [pulsar-client-go] snowcrumble removed a comment on pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by GitBox <gi...@apache.org>.
snowcrumble removed a comment on pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#issuecomment-667936528
Why do not add an option let user set an arbitrary `tls.Config`
----------------------------------------------------------------
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
[GitHub] [pulsar-client-go] EronWright commented on a change in pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by 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
[GitHub] [pulsar-client-go] EronWright commented on a change in pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#discussion_r426759484
##########
File path: pulsar/internal/connection.go
##########
@@ -713,6 +713,41 @@ func (c *connection) getTLSConfig() (*tls.Config, error) {
if c.tlsOptions.ValidateHostname {
tlsConfig.ServerName = c.physicalAddr.Hostname()
Review comment:
To support SNI, shouldn't the `tlsConfig.ServerName` be set in all cases?
----------------------------------------------------------------
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
[GitHub] [pulsar-client-go] snowcrumble commented on pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by GitBox <gi...@apache.org>.
snowcrumble commented on pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#issuecomment-667936528
Why do not add an option let user set an arbitrary `tls.Config`
----------------------------------------------------------------
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
[GitHub] [pulsar-client-go] EronWright commented on a change in pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by GitBox <gi...@apache.org>.
EronWright commented on a change in pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#discussion_r426759183
##########
File path: pulsar/internal/connection.go
##########
@@ -713,6 +713,41 @@ func (c *connection) getTLSConfig() (*tls.Config, error) {
if c.tlsOptions.ValidateHostname {
tlsConfig.ServerName = c.physicalAddr.Hostname()
Review comment:
To support SNI, shouldn't the `tlsConfig.ServerName` be set in all cases?
----------------------------------------------------------------
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
[GitHub] [pulsar-client-go] wolfstudy commented on pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#issuecomment-640972047
----------------------------------------------------------------
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
[GitHub] [pulsar-client-go] wolfstudy commented on pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#issuecomment-674624087
ping @zzzming Can you check @EronWright comments? thanks.
----------------------------------------------------------------
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
[GitHub] [pulsar-client-go] jdkuki commented on a change in pull request #238: Implement TLS VerifyPeerCertificate callback to skip hostname verfication
Posted by GitBox <gi...@apache.org>.
jdkuki commented on a change in pull request #238:
URL: https://github.com/apache/pulsar-client-go/pull/238#discussion_r696050924
##########
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:
Just ran into this. As-is verification will return success when no roots are loaded.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org