Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
eglinux opened this issue Jun 14, 2021 · 8 comments
Closed

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

eglinux opened this issue Jun 14, 2021 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@eglinux
Copy link

eglinux commented Jun 14, 2021

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 seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 14, 2021
@seankhliao
Copy link
Member

cc @FiloSottile

@networkimprov
Copy link

cc @ianlancetaylor re possible proposal

@antong
Copy link
Contributor

antong commented Aug 20, 2021

@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
Copy link
Author

eglinux commented Aug 24, 2021

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
Copy link
Contributor

antong commented Aug 24, 2021

@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
Copy link
Author

eglinux commented Aug 24, 2021

@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
Copy link
Contributor

antong commented Aug 24, 2021

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
Copy link
Author

eglinux commented Aug 25, 2021

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.

@eglinux eglinux closed this as completed Aug 25, 2021
@golang golang locked and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants