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

net/http/httputil: ReverseProxy doesn't support TCP half-close when HTTP is upgraded #35892

Open
sngchlko opened this issue Nov 28, 2019 · 4 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@sngchlko
Copy link

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

$ go version
go version go1.13.4 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="/home/scko/.cache/go-build"
GOENV="/home/scko/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/scko/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
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-build357268893=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I made the simple program to show the problem that I have.
This program has three servers: a frontend, a reverse proxy, and a backend.
After the frontend's HTTP upgrade request is accepted, the backend immediately closes the output stream. But the backend can't read the input stream because of the socket is closed by the reverse proxy.

I think httputil.ReverseProxy doesn't support TCP half-close.

package main

import (
	"bufio"
	"fmt"
	"io"
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
	"net/http/httputil"
	"net/url"
	"strings"

	"golang.org/x/net/http/httpguts"
)

func upgradeType(h http.Header) string {
	if !httpguts.HeaderValuesContainsToken(h["Connection"], "Upgrade") {
		return ""
	}
	return strings.ToLower(h.Get("Upgrade"))
}

func main() {
	reqDone := make(chan struct{})
	backendServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		defer close(reqDone)
		if upgradeType(r.Header) != "websocket" {
			http.Error(w, "unexpected request", 400)
			return
		}
		c, _, err := w.(http.Hijacker).Hijack()
		if err != nil {
			http.Error(w, fmt.Sprintf("%v", err), 500)
			return
		}

		io.WriteString(c, "HTTP/1.1 101 Switching Protocols\r\nConnection: upgrade\r\nUpgrade: WebSocket\r\n\r\n")

		if tcpc, ok := c.(interface {
			CloseWrite() error
		}); ok {
			tcpc.CloseWrite()
		} else if closer, ok := c.(io.Closer); ok {
			closer.Close()
			return
		}

		bs := bufio.NewScanner(c)
		if !bs.Scan() {
			fmt.Printf("backend failed to read line from client: %v", bs.Err())
			return
		}

		got := bs.Text()
		want := "Hello"
		if got != want {
			panic(fmt.Sprintf("got %#q, want %#q", got, want))
		}
		fmt.Println("backend got", got)
	}))
	defer backendServer.Close()

	backURL, _ := url.Parse(backendServer.URL)
	rproxy := httputil.NewSingleHostReverseProxy(backURL)
	rproxy.ErrorLog = log.New(ioutil.Discard, "", 0) // quiet for tests

	frontendProxy := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
		rproxy.ServeHTTP(rw, req)
	}))

	defer frontendProxy.Close()

	req, _ := http.NewRequest("GET", frontendProxy.URL, nil)
	req.Header.Set("Connection", "Upgrade")
	req.Header.Set("Upgrade", "websocket")

	c := frontendProxy.Client()
	res, err := c.Do(req)
	if err != nil {
		panic(err)
	}
	if res.StatusCode != 101 {
		panic(fmt.Sprintf("status = %v; want 101", res.Status))
	}

	if upgradeType(res.Header) != "websocket" {
		panic(fmt.Sprintf("not websocket upgrade; got %#v", res.Header))
	}
	rwc, ok := res.Body.(io.ReadWriteCloser)
	if !ok {
		panic(fmt.Sprintf("response body is of type %T; does not implement ReadWriteCloser", res.Body))
	}
	defer rwc.Close()

	ioutil.ReadAll(rwc)
	io.WriteString(rwc, "Hello\n")
	<-reqDone
}

What did you expect to see?

The backend can read the input stream even though the output stream is closed.

backend got Hello

What did you see instead?

But the input stream was closed as well.

backend failed to read line from client: <nil>
sngchlko added a commit to sngchlko/go that referenced this issue Nov 28, 2019
In order to support TCP half-close in ReverseProxy,
ReverseProxy.handleUpgradeResponse should not close all sockets
immediately when the one side gets an EOF. But we should close the
write stream on the socket to inform the transfer is complete.

Fixes golang#35892
@gopherbot
Copy link

Change https://golang.org/cl/209357 mentions this issue: net: support TCP half-close when HTTP is upgraded in ReverseProxy

@odeke-em
Copy link
Member

Thank you for this bug report as well as CL and welcome to the Go project @sngchlko!

I've added some feedback to your CL and we are currently in a code freeze until perhaps February 2020, but I look forward to encountering you more here and on the code review list and thank you again!

@odeke-em odeke-em added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 29, 2019
@odeke-em odeke-em added this to the Backlog milestone Nov 29, 2019
@gopherbot
Copy link

Change https://go.dev/cl/564375 mentions this issue: net/http: support TCP half-close when HTTP is upgraded in ReverseProxy

@callthingsoff
Copy link
Contributor

Hi @odeke-em!
https://golang.org/cl/209357 is abandoned, shall we move this forward?
Could you please take a look at https://go.dev/cl/564375? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
4 participants