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

x/net/http2: TestServer_Rejects_TLSBadCipher failing on Go tip with TLS 1.3 #28762

Closed
dmitshur opened this Issue Nov 13, 2018 · 10 comments

Comments

Projects
None yet
5 participants
@dmitshur
Member

dmitshur commented Nov 13, 2018

The http2 tests have recently started to fail on tip (they're still passing on Go 1.11.x and 1.10.x):

--- FAIL: TestServer_Rejects_TLSBadCipher (0.00s)
    server_test.go:550: got a *http2.SettingsFrame; want *GoAwayFrame
    server_test.go:236: Framer read log:
        2018-11-12 21:11:21.8639308 Framer 0xc0003ec9a0: read SETTINGS len=24, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896, INITIAL_WINDOW_SIZE=1048576
FAIL
FAIL	golang.org/x/net/http2	12.388s

See https://build.golang.org/?repo=golang.org%2fx%2fnet.

It looks like https://go-review.googlesource.com/c/go/+/147160 might be the culprit CL, judging from the dashboard, but I'm not completely sure of that. /cc @bradfitz @aclements

Edit: It's more likely there was another CL (perhaps related to TLS 1.3) that went in at the same time and didn't show up on the dashboard. Bisecting now. Edit 2: Yep.

@dmitshur dmitshur added this to the Unreleased milestone Nov 13, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 13, 2018

Bisected this test failure to CL 147638 (crypto/tls: enable TLS 1.3 and update tests), which makes a lot of sense. /cc @FiloSottile

@dmitshur dmitshur changed the title from x/net/http2: TestServer_Rejects_TLSBadCipher failing on tip to x/net/http2: TestServer_Rejects_TLSBadCipher failing on Go tip with TLS 1.3 Nov 13, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 13, 2018

FWIW, explicitly setting the maximum acceptable TLS version to 1.2 in that test makes it pass:

diff --git a/http2/server_test.go b/http2/server_test.go
index e5be1a6..3cd398f 100644
--- a/http2/server_test.go
+++ b/http2/server_test.go
@@ -2416,6 +2416,7 @@ func testRejectTLS(t *testing.T, max uint16) {
 
 func TestServer_Rejects_TLSBadCipher(t *testing.T) {
 	st := newServerTester(t, nil, func(c *tls.Config) {
+		c.MaxVersion = tls.VersionTLS12
 		// Only list bad ones:
 		c.CipherSuites = []uint16{
 			tls.TLS_RSA_WITH_RC4_128_SHA,

But I don't know if that's the best fix. Maybe it's better to update the test to work with TLS 1.3 if available (without breaking it for Go 1.10.x and 1.11.x that will not have TLS 1.3). Leaving to @FiloSottile.

@jhump

This comment has been minimized.

jhump commented Nov 13, 2018

I have a project (grpcurl) set to run against Go "tip", and I'm seeing similar problems.

My project includes tests to make sure TLS settings are correctly configured based on command-line flags. And, starting last night, it now consistently fails with Go "tip", accepting TLS connections in cases where they should not be.

These cases configure the client to use (a) an expired cert, (b) an untrusted cert, and (c) no cert (where server is configured to require a client cert). In all three cases, the server is allowing the connection instead of rejecting it. Just like described here, if I set the MaxVersion of tls.Config to tls.VersionTLS12, the problem goes away.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 13, 2018

@jhump, thanks for testing tip. Welcome to life on the edge. :-)

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 13, 2018

Let's get the quick workaround fix in to make the x/net tree usable, and we'll mark this as a release blocker to figure out before the release.

@gopherbot

This comment has been minimized.

gopherbot commented Nov 13, 2018

Change https://golang.org/cl/149357 mentions this issue: http2: disable TLS 1.3 in failing test temporarily(?)

gopherbot pushed a commit to golang/net that referenced this issue Nov 13, 2018

http2: disable TLS 1.3 in failing test temporarily(?)
Updates golang/go#28762

Change-Id: If73b292f28e553646431af995942169ce58d43f5
Reviewed-on: https://go-review.googlesource.com/c/149357
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Nov 13, 2018

That’s the right fix, because TLS 1.3 has no bad ciphers to test. If you want, you can add a test to make sure TLS 1.3 ciphers are considered good, but that’s probably covered by the rest of the tests defaulting to TLS 1.3.

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Nov 13, 2018

@jhump your issue is different, though. Can you open a separate issue and tag me please?

@jhump

This comment has been minimized.

jhump commented Nov 13, 2018

@FiloSottile, done: #28779

@gopherbot

This comment has been minimized.

gopherbot commented Nov 29, 2018

Change https://golang.org/cl/151619 mentions this issue: http2: confirm the test fix for golang/go#28762 was correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment