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

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
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@dmitshur
Copy link
Contributor

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 13, 2018
@dmitshur dmitshur added this to the Unreleased milestone Nov 13, 2018
@dmitshur
Copy link
Contributor Author

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 x/net/http2: TestServer_Rejects_TLSBadCipher failing on tip x/net/http2: TestServer_Rejects_TLSBadCipher failing on Go tip with TLS 1.3 Nov 13, 2018
@dmitshur
Copy link
Contributor Author

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

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

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

@bradfitz
Copy link
Contributor

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

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

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

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

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 13, 2018
@jhump
Copy link

jhump commented Nov 13, 2018

@FiloSottile, done: #28779

@gopherbot
Copy link

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

mariash added a commit to cloudfoundry/diego-release that referenced this issue Mar 22, 2019
[finishes #164248944](https://www.pivotaltracker.com/story/show/164248944)

Submodule src/golang.org/x/crypto 56440b844..e84da0312:
  > hkdf: add Extract and Expand
  > hkdf: improve example
  > openpgp: don't generate PubKeyAlgoRSASignOnly keys
  > acme/autocert: include rejected hostname in TLS handshake error when host not configured
  > ssh/testdata: correct typo
  > ssh: fix typo in error message
  > openpgp: split up tests and keys
  > openpgp: allow RSA/ECDSA signers to return a pointer
  > openpgp: test subkeys with sub-optimal signature packet ordering
  > openpgp: use latest subkey binding signature
  > openpgp: move addUserID outside of ReadEntity
  > openpgp: don't treat extra subkey selfsigs as uid sigs
  > ssh: don't panic if a key is too short.
  > acme/autocert: clarify that multiple names are allowed
  > chacha20poly1305: use x/sys/cpu feature variables directly
  > openpgp/clearsign: add ability to sign with more than one key.
  > chacha20poly1305: add some more XChaCha20-Poly1305 test vectors.
  > acme/autocert: fix race in test.
  > ssh: RFC5208 support PKCS#8 key
  > chacha20poly1305: add example for NewX
  > chacha20poly1305: add XChaCha20-Poly1305
Submodule src/golang.org/x/net a46301530...65e2d4e15:
  > http2/h2demo: fix the HTTP/1-vs-HTTP/2 demo after HSTS breakage
  > html: update inSelectIM and inSelectInTableIM for the latest spec
  > html: make (*nodeStack)contains distinguish namespace
  > http2: Revert a closed stream cannot receive data
  > trace: tighten the check for duplicate registration
  > route: update package docs
  > route: fix TestRouteMessage
  > proxy: support socks5h scheme in proxy URL
  > ipv4, ipv6: directly initialize ctlOpts and sockOpts
  > ipv4, ipv6: drop support for macOS 10.7 and earlier
  > route: add support for freebsd/arm64
  > internal/nettest: drop causesIPv6Crash
  > publicsuffix: hold icann-ness until wildcards fully match
  > publicsuffix: add comments on the icann return value
  > internal/socket: use correct cmsg alignment for openbsd/arm
  < [release-branch.go1.11] http2: don't leak streams on broken body
  > http2/hpack: track the beginning of a header block
  > http2/h2c: Add missing error check
  > http2: revert Transport's strict interpretation of MAX_CONCURRENT_STREAMS
  > http2: confirm the test fix for golang/go#28762 was correct
  > internal/socket: use libc calls on darwin
  > route: use libc calls on Darwin
  > context/ctxhttp: remove Go 1.6 support, use std context
  > http2: disable TLS 1.3 in failing test temporarily(?)
  > internal/socket: simplify nested if-blocks
  > webdav: remove Go 1.6 support, use std context
  > trace: remove Go 1.6 support
  > http2: don't leak streams on broken body
  > internal/socket: update zoneCache on cache misses to cover appearing interfaces
  > icmp, ipv4, ipv6: re-adjust build constraints
  > http2/h2i: re-adjust build constraints
  > icmp: fix InterfaceIdent.Index handling
  > Revert "icmp: fix ExtendedEchoRequest extension object"
  > icmp: fix ExtendedEchoRequest extension object
  > http2: remove support for Go 1.8 and earlier
  > http2: fix typos
  > internal/nettest: add AIX operating system
  > publicsuffix: update table to latest list from publicsuffix.org
  > bpf: support JumpIf on RegX instead of K
  > route: don't run NET_RT_IFLIST vs. NET_RT_IFLISTL test in 386 emulation (again)
  > route: re-adjust routing message alignment for FreeBSD 386 emulation
  > html: remove unnecessary namespace checking
  > all: fix typos in comments
  > html: add missing special elements for <svg> and <math>
  > html: don't set im if <template> doesn't have HTML namespace
  > html: avoid panic even if unconsidered <isindex> and <template> combination
  > internal/socks: fix socket descriptor leakage in Dialer.Dial
  > html: add more comments to Parse and ParseFragment
  > html: fix wrong comparison in foster parenting algorithm
  > html: copy passed tests from chromium
  > html: handle end-of-file cases correctly
  > http2: reduce init-time work & allocations
  > html: remove special procedure for <template> in frameset im
Submodule src/golang.org/x/sys 98c5dad5d..3b5209105:
  > unix: add ClockGettime on darwin/amd64
  > unix: update zsysnum_darwin_amd64 for macOS 10.14
  > unix: require GOOS be set in mksyscall
  > unix/linux: update Dockerfile to glibc 2.29 and Go 1.12beta2
  > windows: Implement WaitForMultipleObjects
  > unix: set 100644 mode on mksyscall_aix_ppc64.go
  > unix: add RND* ioctl consts on Linux
  > cpu: detect RDRAND and RDSEED instructions on x86
  > unix: add ioctl wrappers to get and set RTC time on Linux
  > unix: fix misspellings
  > unix: regenerate sysnum files for freebsd/arm64 and netbsd/arm64
  > unix: use glibc release branch in Dockerfile
  > unix: replace "mksyscall_aix_ppc64.pl" script with a Go program
  > unix: add socket error queue constants and types from linux/errqueue.h
  > unix: add netbsd/arm64 files.
  > unix: use int8 for RawSockaddrUnix.Path on linux/riscv64
  > unix: use Renameat2 to implement Renameat on linux/riscv64
  > unix: add Renameat on dragonfly
  > unix: add Signalfd on linux
  > unix: add ClockNanosleep and TIMER_ABSTIME
  > unix: get *BSD syscalls.master files using HTTPS
  > unix: replace "mksyscall_aix_ppc.pl" script with a Go program
  > cpu: add linux/ppc64x
  > plan9: replace "mksyscall.pl" with Go program
  > unix: make Fcntl* routines use libSystem on Darwin
  > plan9: remove "use" function and calls from generated code.
  > unix/linux: update Dockerfile to Linux Kernel 4.20
  > unix: add support for freebsd/arm64
  > unix: fix Fstatat by using fillStat_t on linux/mips64x
  > unix: mksysnum: correct +build restriction
  > unix: replace mksysnum_*.pl scripts with Go program
  > unix: generate linux/sparc64 go files using Docker
  > unix: fix unused import in syscall_darwin_386.go
  > unix: add space to generated code
  > unix: reverse default for 1.11/1.12 split
  > unix: add Sendfile test
  > unix/linux: set 100644 mode on mksysnum.go
  > unix: remove raw syscall from Sendfile
  > unix: remove raw syscall from getAttrList
  > unix: remove raw syscall from getattrlistTimes
  > unix: remove raw syscall from Getfsstat
  > unix: remove Getdirentries on iOS
  > unix: avoid "64"-postfixed libSystem calls on iOS
  > unix: convert Darwin syscalls from raw to libSystem
  > unix: update Dockerfile to Go 1.12beta1
  > windows/svc: use wevtutil.exe instead of powershell for windows/arm
  > windows: use netevent.dll in TestFormatMessage for windows/arm
  > unix: remove Flock syscall for aix
  > unix: add SyncFileRange on linux/arm
  > unix: evaluate cmsg alignment in cmsgAlignOf at compile time
  > unix: use correct cmsg alignment for openbsd/arm
  > unix: regenerate ztypes for openbsd/arm
  > unix: always use fstatat(AT_SYMLINK_NOFOLLOW) over lstat on AMD64
  > unix: always use inotify_init1 over inotify_init on AMD64
  > unix: avoid index out of range in Vmsplice with empty iovs
  > cpu: add linux/arm64
  > unix/linux: replace "mksysnum.pl" script with a Go program
  > unix: replace "mksyscall.pl" script with a Go program
  > unix: rework TestGetwd to handle test dirs whose names contain symlinks
  > unix: add IoctlSetPointerInt
  > unix: add ioctl definitions for PPP interfaces.
  > unix: add sockaddr and defines for PPPoE sockets.
  > unix: export FdSet bits field on freebsd
  > unix: add IoctlGetPtmget on netbsd
  > unix: test UtimesNanoAt on darwin
  > unix: don't use deprecated sys/capability.h header on FreeBSD
  > unix: fix errors in syscalls when using -linkshared on ppc64x
  > unix: avoid index out of range in *setxattr on BSD
  > unix: add remaining *at functions on dragonfly
  > unix: add remaining *at functions on netbsd
  > unix: add remaining *at functions on openbsd
  > unix: add Readlinkat on openbsd
  > unix: add Openat on dragonfly
  > unix: add Openat on netbsd
  > unix: allow to pass a NULL pointer to the pledge syscall
  > unix: add kernel module load/unload syscalls on Linux
  > unix: add support for OpenBSD unveil(2)
  > unix: use ppoll to implement Pause on linux/{arm64,riscv64}
  > unix: add Openat on openbsd
  > unix: add Ppoll on openbsd
  > unix: update Dockerfile to Linux 4.19
  > unix: don't use local syscall number in Pledge()
  > unix: FreeBSD 12 ino64 support
  > unix: update syscall numbers to OpenBSD 6.4
  > unix: add SOF_TIMESTAMPING_* flags on Linux
  > unix: add support for aix/ppc64, gc implementation
  > unix: add marker comments to zsyscall_solaris_amd64.go
  > unix: convert errorList and signalList to new format for linux/sparc64
  > unix: add KexecFileLoad on linux/{amd64,ppc64x,s390x}
  > unix: export sizeof consts
  > unix: use correctly aligned result buffer in SysctlClockinfo
  > unix: add SysctlUvmexp on OpenBSD
  > unix: re-add vm.* sysctl strings on openbsd/amd64
  > unix: add NCSI generic netlink constants
  > unix: add ClockGetres on Linux
  > unix: add SysctlClockinfo on NetBSD
  > unix: fix TestSchedSetaffinity for smt settings
  > unix: add additional rlimit resource constants on NetBSD
  > unix: add MNT_* flags on OpenBSD
  > windows: add support for windows/arm
  > unix: add MNT_* flags on DragonflyBSD
  > unix: add MNT_* flags on NetBSD
  > unix: implement Linux AF_XDP sockets
  > unix: add more RTA_* constants on Linux
  > unix: add Termio, Winsize, Statfs_t, TC[GS]ET, Mkdir and Mknod for AIX
  > unix: change MemfdCreate flags parameter to int for all GOARCHes
  > unix: add MemfdCreate on Linux
  > unix: correct misspelling in test output
  > windows: correct misspelling in comment
  > unix: add remaining RTA_* constants for use with rtnetlink on Linux
  > unix: add support for linux/riscv64
  > unix: add SYNC_FILE_RANGE_* constants on Linux
  > unix: correct argument order for SyncFileRange syscall on linux/ppc64{,le}
  > Revert "unix: implement SyncFileRange for ARM"
  > unix: implement SyncFileRange for ARM
  > windows: add support for AF_UNIX sockets
  > unix: update Dockerfile to Go 1.11
  > unix: code for AIX ppc and ppc64, for gccgo
  > unix: add UBI ioctl constants on Linux
  > unix: use pipe2 syscall on FreeBSD instead of pipe
  > unix: add additional file mode bit constants from sys/stat.h on Solaris
  > unix: add example for Flock
  > unix: add NOSTD syscall numbers on FreeBSD
  > unix: add additional file mode bit constants from sys/stat.h on FreeBSD
  > unix: add additional file mode bit constants from sys/stat.h on DragonflyBSD
  > windows: add Wincrypt.h Cert related values
  > unix: update Dockerfile to Linux 4.18, glibc 2.28 and Go 1.11beta3
Submodule src/golang.org/x/text cb6730876..e6919f657:
  > internal/export/unicode: copy unicode/maketables.go
  > message/pipeline: fix type switch case order
  > unicode/norm: bug fix in Iter
  > unicode/norm: correct use of ErrShortDst
  > language: improve docs for Parent
  > all: fix typos
  > x/text: add go.mod and go.sum files
  > message/catalog: fix usage of plural in docs
  > message/pipeline: don't panic if message not imported
  > unicode/norm: move sync.Once check out of loop
  > cmd/gotext: fix "go format failed: invalid argument"
  > encoding: regenerate tables
  > unicode/norm: lazy load recompMap from string
Submodule src/golang.org/x/tools b4b6fe2cb..49d818b07:
  > [release-branch.go1.12] cmd/godoc: fix -url flag, add tests
  > internal/lsp: fix crash caused by lack of TypeSizes from go/packages.Load
  > internal/lsp: type-check packages from source
  > internal/lsp: add support for analyzers with dependencies on other analyzers
  > go/packages: fix broken tests to handle changes to packagestest
  > internal/lsp: connect basic analysis functionality
  > go/packages: use strings.EqualFold in sameFile to handle case-insensitive file systems
  > internal/lsp/cmd: fix a nil pointer and some minor clean-up
  > internal/lsp, go/packages/packagestest: support testing test files
  > internal/lsp: add additional logging for go/packages failures
  > internal/lsp: handle escaping URIs for files in the module cache
  > internal/lsp: use interface for diagnostics code
  > godoc: remove leading tabs while formatting text
  > cmd/stringer: type check using export data
  > cmd/guru: remove Go 1.9 handling, since we no longer support 1.8
  > internal/lsp: do not return nil identifiers without errors
  > internal/lsp/diff: fix sources listed for diff implementation
  > internal/lsp: rank types higher when completing in return of a function
  > Revert "go/analysis/passes/atomicalign: handle pointers to struct"
  > internal/lsp: implement diff for computing text edits
  > cmd/tip: delete
  > internal/lsp/cmd: add the definition mode
  > go/analysis/passes/atomicalign: handle pointers to struct
  > godoc/static: update static.go
  > go/internal/gcimporter: write export data for go/types
  > godoc/static: inherit textarea color to avoid illegible text
  > imports: drop anything after a non identifier rune in package names
  > godoc/short: point to new tracking issue for shortlink creation
  > imports: update zstdlib to Go 1.12
  > go/packages: rename name= query and "disable" it
  > go/vcs: remove go.googlesource.com vcsPath entry
  > cmd/heapview: delete
  > cmd/godoc: add x/xerrors redirect
  > internal/lsp: refactor definition and hover to share functionality
  > go/packages: stop parsing files if the context is canceled
  > go/analysis/passes/composite: add an example to the doc
  > cmd/gopls/integration: git ignore temporary vscode files
  > godoc/dl: add julieqiu to validUser
  > cmd/present: fix CSS when printing slides
  > cmd/tip: also fetch x/net repository before building cmd/godoc
  > imports: stop using go/packages for modules
  > internal/lsp: do not complete inside comments in functions
  > go/analysis/internal/checker: correct comment grammar
  > go/analysis/passes/atomicalign: add atomicalign ckecker
  > imports: remove globals, stop using build.Default
  > imports: don't eagerly guess package names

Co-authored-by: Nick Wei <nwei@pivotal.io>
@golang golang locked and limited conversation to collaborators Nov 29, 2019
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
Fixes golang/go#28762

Change-Id: I8d8b74cd8836bbed3116b334f6595225a8f0a36e
Reviewed-on: https://go-review.googlesource.com/c/151619
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants