crypto/tls: don't close tcp-session if TLS-handshake fails

[eglinux]

What version of Go are you using (go version)?

$ go version
go version go1.16.5 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/root/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/src/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/src/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.5"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1581134579=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Use crypto/tls package
https://github.com/golang/go/blob/master/src/crypto/tls/tls.go#L157-L159

What did you expect to see?

Not nil if tls-handshake fails.

I use blackbox_exporter (https://github.com/prometheus/blackbox_exporter) for the ssl-certificates monitoring. Due to the current implementation it is not possible to check hosts with having 2 way SSL enabled. Because tls-handshake always fails for those and conn structure is nil - the calling code cannot properly intercept this case.

The one way to fix it I can see is to add extra optional boolean parameter to the function which will let Rawconn structure to be returned to the caller and don't close tcp-connection in this case.

It should be possible to split TLS and TCP layers as their states should be independent from each other. In this case caller might be aware about TLS-layer errors (via err: ), but still be able to use TCP-connection on its own. In this case for instance the client will be able to grab ssl-certificate of the server even if tls-handshake fails.

What did you see instead?

Function returns nil and closes tcp-connection.

[seankhliao]

cc @FiloSottile

[networkimprov]

cc @ianlancetaylor re possible proposal

[antong]

@eglinux, You say it should be possible to split the TCP and TLS layers. I believe it is. You do not have to use tls.Dial. You can do the TCP part yourself and pass the net.Conn to tcp.Client. It doesn't even have to be TCP, it can be any net.Conn.

When you do use Dial and the handshake fails, you say you want it to return an error but a non-nil tls.Conn. I think this would be a bad idea. A tls.Conn is a net.Conn, and you should be able to Read and Write to it etc. What does that mean if the TLS handshake has not succeeded? The TLS connection represents the encrypted stream, and it doesn't exist until the handshake is successfully completed. I think nil is the appropriate return value here.

You mention that the use case is to access the server certificates when the server requires client authentication and you don't have any client certificate. This is possible even using tls.Dial. You can do it by supplying a tls.Config with appropriate verification hooks, e.g., by setting InsecureSkipVerify and VerifyPeerCertificate or VerifyConnection.

[eglinux]

Hi @antong. Thank you for you reply!

I came across to this issue from blackbox_exporter and their implementation of TLS-check. In a while I found alternative project which is called ssl_exporter - https://github.com/ribbybibby/ssl_exporter . There is no such issue in that one and I think is due to the different implementation of TLS-check. Basically the difference affecting me is here:
https://github.com/ribbybibby/ssl_exporter/blob/master/prober/tcp.go#L17-L45

Instead of calling tls.Dial they do create TCP-socket and after it they call tls.Client function from the crypto/tls package.
I am not that experienced in go, but if I look on the implementation of tls.Dial it does more-less the same thing. Basically it is also calling Client in the end.

So, to me it looks like Dial is the high-level method of standard library which is supposed to be called and which has to take care of underlying layers (like TCP and TLS in this case) and return a valid connection object. However it doesn't work all the times like caller code expects and thus other methods are used with similar semantics. Obviously those are specific corner cases - like 2-ways SSL and certificate check in this case.

It can be treated both, either as a limitation or as not enough flexibility of tls.Dial. Maybe worth couple of words in the comments.

Anyway thanks for looking into it!

[antong]

@eglinux, Great! As I suggested, it is actually possible to check certificates using tls.Dial (last paragraph in #46735 (comment)). I don't know why the other program you mention (ssl_exporter) manually makes a TCP socket and then uses the tls.Client. One reason might be that then you can collect TCP level metrics, such as whether TCP connect succeeds and how long the TCP handshake takes independently of TLS.

[eglinux]

@antong, could you, please, give some hints/examples of code of how to use those verification hooks you mentioned? Maybe I can test and make PR to blackbox_exporter project?

[antong]

Here is an example that prints server certificate info even when the handshake fails because of no client cert: https://play.golang.org/p/Ys3xWhT_Ow3

Please note that unlike many other open source projects, the Go project uses Github issues only for tracking bugs and proposals, and not for general discussion or questions. So unless we can make a bug or proposal out of this, let's take the discussion to a more appropriate forum. See https://github.com/golang/go/wiki/Questions for options. Thanks!

[eglinux]

Many thanks! Indeed it doesn't look like a bug and I cannot make a constructive proposal at this stage, so will close the issue.