Implement TLS VerifyPeerCertificate callback to skip hostname verfication#238
Implement TLS VerifyPeerCertificate callback to skip hostname verfication#238zzzming wants to merge 4 commits into
Conversation
| @@ -713,6 +713,41 @@ func (c *connection) getTLSConfig() (*tls.Config, error) { | |||
|
|
|||
| if c.tlsOptions.ValidateHostname { | |||
| tlsConfig.ServerName = c.physicalAddr.Hostname() | |||
There was a problem hiding this comment.
To support SNI, shouldn't the tlsConfig.ServerName be set in all cases?
There was a problem hiding this 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.
|
Move this change to 0.2.0 |
|
ping @EronWright PTAL |
| client.Close() | ||
| } | ||
|
|
||
| func TestTLSInsecureConnectionWithCerts(t *testing.T) { |
There was a problem hiding this 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.
| tlsConfig.ServerName = c.physicalAddr.Hostname() | ||
| tlsConfig.ServerName = c.physicalAddr.Hostname() | ||
|
|
||
| if tlsConfig.InsecureSkipVerify { |
There was a problem hiding this 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() {...}
}
| if tlsConfig.RootCAs == nil { | ||
| return nil | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this comment.
Just ran into this. As-is verification will return success when no roots are loaded.
|
ping @zzzming Can you check @EronWright comments? thanks. |
|
This PR is cool, any updates? |
8 participants
Contribution Checklist
This PR addresses a problem how to disable TLS ValidateHostname. The current implementation with empty
tlsConfig.ServerNamewould 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, golang/go#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
yeswas chosen, please highlight the changesDocumentation