Add option to allow non-standard Common Name (CN) in SSL certificate verification. #705
Conversation
msdsn/conn_str.go
Outdated
| } | ||
| if insecureSkipVerify { | ||
| config.InsecureSkipVerify = true | ||
| if !allowNonstandardHostname { |
There was a problem hiding this comment.
The core issue is that Go TLS does not consider a hostname with ":" to be valid. I generally agree, but here we are.
In this case, remove all code and documentation regarding the new option. rather, do a check in config.ServerName if it contains a ":". If that is the case, then do the alternative check.
msdsn/conn_str.go
Outdated
| // replacing. This does not disable VerifyConnection. | ||
| config.InsecureSkipVerify = true | ||
| config.VerifyConnection = func(cs tls.ConnectionState) error { | ||
| commonName := cs.PeerCertificates[0].Subject.CommonName |
There was a problem hiding this comment.
This verify Connection function doesn't open the "insecure Skip Verify" bool. If insecure skip verify is true, then do an early return.
msdsn/conn_str.go
Outdated
| var config tls.Config | ||
| config.ServerName = hostInCertificate |
There was a problem hiding this comment.
I don't like how SetupTLS is not structured now. Please do the following:
func SetupTLS() {
config := tls.Config {
ServerName: hostInCertificate,
// comment
DynamicRecordSizingDisabled: true,
}
if len(certificate) == 0 {
return &config, nil
}
ReadFile(certificate)
if strings.Contains(config.ServerName, ":") {
// new code
return &config, nil
}
// orig code
return &config, nil
}
|
Okay, so this LGTM, with one change: it needs to be for only go1.15+ only. I coded this up, but couldn't make it commit well, so I'm going to merge and push a follow up. |
Fixes #704