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: recent TLS 1.3 changes appear to break cases where client cert should be rejected #28779

Closed
jhump opened this issue Nov 13, 2018 · 5 comments
Assignees
Milestone

Comments

@jhump
Copy link

@jhump jhump commented Nov 13, 2018

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

$ go version
go version devel +45e9c5538b Tue Nov 13 17:16:48 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

Nope, it's okay in release. It only repros with tip.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/travis/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/travis/gopath"
GOPROXY=""
GORACE=""
GOROOT="/home/travis/.gimme/versions/go"
GOTMPDIR=""
GOTOOLDIR="/home/travis/.gimme/versions/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build882726140=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I have a test case that is verifying that a TLS client that uses a bad cert (either no cert, when server requires one, or an expired cert or one issued by untrusted CA) gets rejected by the server during a TLS handshake.

I've distilled it down to this test: fullstorydev/grpcurl#68

What did you expect to see?

Server should have rejected the client due to bad certificate.

This is what happens in every other Go version that I tested: https://travis-ci.org/fullstorydev/grpcurl/builds/454615565

What did you see instead?

Server accepted the client.

If I set MaxVersion on the server's tls.Config to tls.VersionTLS12 then things work as expected. So this appears to be related to the new TLS 1.3 work that landed yesterday.

FYI: @FiloSottile

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 29, 2018

The server is in fact rejecting the certificate here, but that error is being thrown out after being printed by your test. You can see the error logged in Travis.

https://github.com/fullstorydev/grpcurl/blob/a90ea9e114ebe5e570fea4fcf44f9351b58b98a5/tls13_bug_test.go#L111-L113

Bad handshake: tls: failed to verify client's certificate: x509: certificate has expired or is not yet valid
Bad handshake: tls: client didn't provide a certificate
--- FAIL: TestTLS13RejectsBadCerts (0.07s)
    tls13_bug_test.go:37: Expecting failure due to use of expired cert!
    tls13_bug_test.go:45: Expecting failure due to use of untrusted cert!
    tls13_bug_test.go:53: Expecting failure due to missing cert!
Bad handshake: tls: client didn't provide a certificate

Your test is checking the error on the client side instead of the server side. In TLS 1.2, there was a whole round-trip of handshake after sending the client cert, so the remote error was making it to the client in the form of an alert before Handshake had to return. In TLS 1.3, the client certificate is sent in the second and last client flight, meaning the client sends the client cert and then returns before hearing back from the server, so it can't return an error in Handshake.

The error will show up on the client first read. This is unavoidable, because the protocol is designed that way, but I can see it causing trouble. Hopefully relatively few users call Handshake directly. We should definitely mention it in the release notes.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 29, 2018

@jhump BTW, thank you for that reproduction PR, it was extremely handy.

@jhump

This comment has been minimized.

Copy link
Author

@jhump jhump commented Nov 29, 2018

@FiloSottile, will the server simply close the connection after it sees the cert is bad? If so, does that mean there is no way to distinguish, in the client, between the server rejecting the client's credentials and the server simply being partitioned from the client between the handshake and first read?

I'm trying to figure out how to work around this: for command-line tools (such as grpcurl), it's nice to provide meaningful/useful error messages related to client identity in mutually-authenticated TLS. But it sounds like that may not be feasible with TLS 1.3. I understand the desire to reduce round trips, particularly for TLS over mobile networks, but this is an unfortunate side effect for debugging issues on the client.

One example: it means that using grpc.WithBlock, to dial a gRPC server, won't be useful if there are unrecoverable errors due to TLS certs. The gRPC client will block until the socket is established, the handshake complete, and think the channel is ready for use (just as in my code example). But then every RPC on it will fail due to the transport channel getting closed. IMO, that kind of defeats the purpose of grpc.WithBlock. With gRPC, I guess we can work-around it by sending an HTTP/2 PING frame immediately if we want to confirm the channel is good and the server accepted the cert. But that won't work, for example, with HTTP 1.1 communications.

Anyhow. Sorry for the long-winded comment. I guess, summing up, I'm wondering if you have recommendations for providing better client-side error messages with TLS 1.3 in these situations.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 29, 2018

I do agree that it’s unfortunate, but I don’t have useful advice beyond what you already suggested.

Note though that the server won’t just close the connection but send an alert, so the first client read will fail with the same meaningful error as the Handshake used to.

@jhump

This comment has been minimized.

Copy link
Author

@jhump jhump commented Nov 29, 2018

so the first client read will fail with the same meaningful error as the Handshake used to.

Ok, that's useful actually. Thanks for the info.

alainjobart added a commit to alainjobart/vitess that referenced this issue Jan 12, 2019
TLS 1.3 pushes detection of some client-side handshake errors to the time of
the first read. For more details, see:
golang/go#28779 (comment)

Signed-off-by: Alain Jobart <alainjobart@google.com>
derekperkins added a commit to nozzle/vitess that referenced this issue Jan 20, 2019
TLS 1.3 pushes detection of some client-side handshake errors to the time of
the first read. For more details, see:
golang/go#28779 (comment)

Signed-off-by: Alain Jobart <alainjobart@google.com>
@golang golang locked and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.