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: Dial to server with invalid client certificates succeeds and allows writes on tip, but not in 1.12 #32202

Closed
zikaeroh opened this issue May 23, 2019 · 5 comments
Assignees
Milestone

Comments

@zikaeroh
Copy link

@zikaeroh zikaeroh commented May 23, 2019

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

$ go version
go version devel +4fbb4e7 Thu May 23 13:55:55 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No. (1.12.5)

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jake/nobackup/gotip_home/cache/go-build"
GOENV="/home/jake/nobackup/gotip_home/config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jake/nobackup/gotip_home/gopath"
GOPROXY="direct"
GOROOT="/home/jake/sdk/gotip"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/home/jake/sdk/gotip/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jake/repos/tlsbug/go.mod"
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-build626324268=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Break a TLS server by setting ClientCAs to nil (making a self signed cert invalid).

https://play.golang.org/p/w0WCFqxoNhw

What did you expect to see?

Both sides of the connection fail, specifically the client during the dial.

What did you see instead?

The server side of the connection fails, but the client side of the connection is able to write data without an error. In 1.12, this write fails, which I noticed when testing a project with gotip.

$ go run .; gotip run .
2019/05/23 07:09:23 go1.12.5
2019/05/23 07:09:23 reader: got=, err=tls: failed to verify client's certificate: x509: certificate signed by unknown authority
2019/05/23 07:09:23 dial: remote error: tls: bad certificate
exit status 1
2019/05/23 07:09:24 devel +4fbb4e7 Thu May 23 13:55:55 2019 +0000
2019/05/23 07:09:24 writer: n=9, err=<nil>
2019/05/23 07:09:24 reader: got=, err=tls: failed to verify client's certificate: x509: certificate signed by unknown authority

Swapping the roles of the client/server (make the server do the write) makes both sides fail correctly.

Feel free to retitle this if crypto/tls is not the right place.

@FiloSottile FiloSottile added this to the Go1.13 milestone May 23, 2019
@FiloSottile FiloSottile self-assigned this May 23, 2019
@FiloSottile FiloSottile changed the title crypto/tls: Dial to server with invalid certificates succeeds and allows writes on tip, but not in 1.12 crypto/tls: Dial to server with invalid client certificates succeeds and allows writes on tip, but not in 1.12 May 23, 2019
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented May 23, 2019

This is probably a consequence of turning on TLS 1.3. Can you check that setting MaxVersion to VersionTLS12 restores the expected behavior?

Unfortunately, this is due to TLS 1.3's protocol architecture: the client sends the client certificates, and then is immediately ready to send data to the server, without waiting for a reply. If no reads are done, the client will never consume the alert letting it know the handshake actually failed.

We might add a method to wait for the server finished if this turns out to be a common issue, but we can't make it the default behavior as it would add a round-trip and invalidate a lot of TLS 1.3's performance progress.

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented May 23, 2019

Sure, I'll give it a try (though I won't have access to my machine until later today). It being a 1.3 thing makes sense given its design, though it might be a bit unfortunate for connections which don't do any sort of special handshake.

My use case is IRC where I'm writing PASS/NICK to authenticate before I'd ever read anything back, so it's easy to consider the connection "ok" if nothing went wrong up until that point, then start handling incoming messages somewhere else.

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented May 24, 2019

That is indeed the case; setting the max version to 1.2 on either or both of the sides makes the behaviors match.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@gottwald

This comment has been minimized.

Copy link

@gottwald gottwald commented Jul 22, 2019

FYI, just discovered that this breaks traefik integration tests (https://github.com/containous/traefik/tree/v1.7
Branch: 1.7 (current stable)). Not sure how much it's related to the test itself. Haven't looked into it enough.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jul 22, 2019

This is indeed an issue with how the test interacts with TLS 1.3: there is no way for the client Handshake to return an error for a rejected client certificate. The error must either be checked on the first Read, or on the server side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.