Replacing tls.DialWithDialer with tls.Client breaks Client Hello

Created by: linmaosong2018

Summary

The dailer changes here appears to break an initiator's TLS initiation.

Steps to reproduce

1. Save the test code below to a file. The code creates a dummy client that connects to Coinbase's public FIX sandbox.

package main

import (
    "fmt"
    "strings"

    "github.com/quickfixgo/quickfix"
)

type dummyClient struct {
    *quickfix.MessageRouter
}

func (d dummyClient) OnCreate(sessionID quickfix.SessionID) {
    return
}

func (d dummyClient) OnLogon(sessionID quickfix.SessionID) {
    fmt.Println("Logged in")
    return
}

func (d dummyClient) OnLogout(sessionID quickfix.SessionID) {
    fmt.Println("Logged out")
    return
}

func (d dummyClient) FromAdmin(msg *quickfix.Message, sessionID quickfix.SessionID) (reject quickfix.MessageRejectError) {
    return
}

func (d dummyClient) ToAdmin(msg *quickfix.Message, sessionID quickfix.SessionID) {
    fmt.Println(msg.String())
    return
}

func (d dummyClient) ToApp(msg *quickfix.Message, sessionID quickfix.SessionID) (err error) {
    fmt.Println(msg.String())
    return
}

func (d dummyClient) FromApp(msg *quickfix.Message, sessionID quickfix.SessionID) (reject quickfix.MessageRejectError) {
    return
}

func main() {
    cfg := `[SESSION]
BeginString=FIX.4.2
ReconnectInterval=5
SenderCompID=TestID
TargetCompID=Coinbase
HeartBtInt=30
ResetOnDisconnect=Y
SocketConnectPort=4198
SocketConnectHost=fix-public.sandbox.pro.coinbase.com
SocketCertificateFile=test-crt.pem
SocketPrivateKeyFile=test-key.pem
`
    appSettings, err := quickfix.ParseSettings(strings.NewReader(cfg))
    if err != nil {
        panic(err)
    }

    logFactory := quickfix.NewScreenLogFactory()
    app := &dummyClient{MessageRouter: quickfix.NewMessageRouter()}

    initiator, err := quickfix.NewInitiator(app, quickfix.NewMemoryStoreFactory(), appSettings, logFactory)

    err = initiator.Start()
    if err != nil {
        panic(err)
    }

    select {}
}

2. Create a key/cert pair under the same directory using openssl and save them as "test-crt.pem" and "test-key.pem", as shown in the config above.
3. Obtain the quickfix commit before the dialer change.

go get github.com/quickfixgo/quickfix@ce2275bf2c97f679d58b639e7b90d8b3e3e34b8b

4. Run the test code

go run .

5. The TLS connection will succeed, although it will then disconnect due to invalid user ID.

<2019-11-05 17:39:42.86836 +0000 UTC, FIX.4.2:TestID->Coinbase, event>
  (Created session)
<2019-11-05 17:39:42.868807 +0000 UTC, FIX.4.2:TestID->Coinbase, event>
  (Connecting to: fix-public.sandbox.pro.coinbase.com:4198)
...
<2019-11-05 17:39:43.314601 +0000 UTC, FIX.4.2:TestID->Coinbase, event>
  (Invalid Session State: Received Msg 8=FIX.4.29=11035=352=20191105-...
Logged out

6. Now upgrade quickfix to after the dialer change.

go get github.com/quickfixgo/quickfix@87f8869793c8c43dd3e2e912c57e7ddbf1a9fcb5

then

go run .

7. This time the error changes to

<2019-11-05 17:42:11.912862 +0000 UTC, FIX.4.2:TestID->Coinbase, event>
  (Created session)
<2019-11-05 17:42:11.913261 +0000 UTC, FIX.4.2:TestID->Coinbase, event>
  (Connecting to: fix-public.sandbox.pro.coinbase.com:4198)
...
  (Failed handshake: tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config)

Analysis

The error message in step 7 comes from here https://github.com/golang/go/blob/1b3a1db19fc68591198149540f7b3c99f56691da/src/crypto/tls/handshake_client.go#L38

The usage of TLS API is different before and after this commit:

• Before: tls.DialWithDialer
• After: tls.Client

tls.DialWithDialer, tolerates no-ServerName in config: it copies from the given address:

https://github.com/golang/go/blob/414c1d454e6d388443239209220fe0783d4dac71/src/crypto/tls/tls.go#L137-L143

As a result, before the above commit, the address obtained from SocketConnectAddress here https://github.com/quickfixgo/quickfix/blob/ce2275bf2c97f679d58b639e7b90d8b3e3e34b8b/initiator.go#L149

is implicitly copied into ServerName by tls.DialWithDialer.

After the commit, the implicit copy is gone, resulting in TLS' complaint.

Suggestions

I wonder if the above is the motivation behind this PR (although the author didn't give an explanation). As the ServerName is in the golang TLS config: https://github.com/golang/go/blob/1b3a1db19fc68591198149540f7b3c99f56691da/src/crypto/tls/common.go#L565 , would it make sense to add this item into settings?

Or, alternatively, mimic DialWithDialer's behavior by copying the address field, although this seems less ideal to me.