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: support bidirectional stream for CONNECT method #17227

Closed
ayanamist opened this issue Sep 25, 2016 · 26 comments

Comments

Projects
None yet
5 participants
@ayanamist
Copy link

commented Sep 25, 2016

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7.1 windows/amd64

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

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\ayanamist\Documents\Projects\golang\MEOW
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1

What did you do?

Running any proxy (i use fiddler) listen on 127.0.0.1:8888, and run this test

func TestConnect(t *testing.T) {
    tr := &http.Transport{
        Dial: func(network, address string) (net.Conn, error) {
            return net.Dial("tcp", "127.0.0.1:8888")
        },
    }
    pr, pw := io.Pipe()
    req, err := http.NewRequest("CONNECT", "http://www.baidu.com/", pr)
    if err != nil {
        t.Fatal(err)
    }
    resp, err := tr.RoundTrip(req)
    if err != nil {
        t.Fatal(err)
    }
    log.Printf("resp: %v", resp)
    if resp.StatusCode != http.StatusOK {
        t.Fatalf("want 200 OK, got %s", resp.Status)
    }
    req, _ = netHttp.NewRequest("GET", "http://www.baidu.com/", nil)
    if err = req.Write(pw); err != nil {
        t.Fatal(err)
    }
    io.Copy(os.Stdout, resp.Body)
}

What did you expect to see?

output http header and some html

What did you see instead?

hangs and got 408 timeout

@ayanamist

This comment has been minimized.

Copy link
Author

commented Sep 25, 2016

It seems missing something like AllowResponseBeforeBody in #13444 for HTTP 1.1
And if i modify a little to emulate to normal CONNECT proxy behavior, it also hangs.

package main

import (
        "fmt"
        "io"
        "io/ioutil"
        "log"
        "net/http"
        "os"
        "time"
)

func main() {
        pr, pw := io.Pipe()
        req, err := http.NewRequest("PUT", "https://http2.golang.org/ECHO", ioutil.NopCloser(pr))
        if err != nil {
                log.Fatal(err)
        }
        res, err := http.DefaultClient.Do(req)
        if err != nil {
                log.Fatal(err)
        }
        log.Printf("Got: %#v", res)
        go func() {
            for {   
                    time.Sleep(1 * time.Second)
                    fmt.Fprintf(pw, "It is now %v\n", time.Now())
            }
        }()
        n, err := io.Copy(os.Stdout, res.Body)
        log.Fatalf("copied %d, %v", n, err)
}

It seems that, after something write to req.Body, the request actually sent out; if nothing write to req.Body, nothing sent out to proxy server. However, nearly all proxy client (including Chromium) will wait for 200 Connection Established and then send real request, so it happens to be a deadlock.

@minux

This comment has been minimized.

Copy link
Member

commented Sep 25, 2016

@minux minux closed this Sep 25, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 25, 2016

@minux, the net/http client-side Transport doesn't let you Hijack the connections. Only the server side lets you do that.

So, this bug is kinda valid. But for HTTP/1.1 connects, people usually just net.Dial directly and then http.Request.Write + ReadResponse the negotiation, and then have the net.Conn already from that.

@bradfitz bradfitz reopened this Sep 25, 2016

@minux

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

@ayanamist

This comment has been minimized.

Copy link
Author

commented Sep 26, 2016

@minux For HTTP 2.0, it is impossible to hijack as an connection.

@minux

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

@minux, you're mixing several issues here.

We don't support client-side Hijack for either HTTP/1 or HTTP/2. I think there is an ancient bug for that, or maybe we closed as it as WontFix.

We also explicitly do not support Hijack server-side for HTTP/2. Giving out the raw conn doesn't make sense, and giving "the stream" (as in: the HTTP/2) stream back as the net.Conn is kinda useless if the http.Handler can do the same already with Reading from Request.Body and writing to ResponseWriter.

@ayanamist

This comment has been minimized.

Copy link
Author

commented Sep 27, 2016

@bradfitz It seems #13717 should be reopened since it is not solved?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

@ayanamist, I replied there again.

@ayanamist

This comment has been minimized.

Copy link
Author

commented Sep 27, 2016

@bradfitz However, the reporter of that issue is me the same........
Sorry but i did not verify your solution in that issue but just forward it to my friend.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

Yes, so let's keep it there. No need to split the conversation there and distract this thread.

@ayanamist

This comment has been minimized.

Copy link
Author

commented Sep 27, 2016

OK. So can this issue be planned? I think there will be a lot of modifications.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

I don't really think it's worth the effort or additional API.

CONNECT is almost always the first request on the TCP connection so you can just net.Dial it yourself, write the HTTP request, use http.ReadResponse, and then you have the net.Conn to read+write as needed.

Is the issue that you don't know whether the proxy supports http1 vs http2 and you want the net/http to auto-negotiate for you? Even so, the workaround is easy enough. Just dial with NextProto set to "h2" and if you get "h2", then use x/net/http2.Transport with that TLS conn. Otherwise do the HTTP/1.1 *Request.Write + ReadResponse thing.

@bradfitz bradfitz added this to the Unplanned milestone Sep 27, 2016

@ayanamist

This comment has been minimized.

Copy link
Author

commented Sep 27, 2016

Thanks for your advice, i will give it a try.

@ayanamist

This comment has been minimized.

Copy link
Author

commented Sep 28, 2016

After a little modification for my test, it still not work.
golang.org/x/net using f09c4662a0bd6bd8943ac7b4931e185df9471da4
Using nghttp2 with fiddler to support h2c (which is handy to see detail in wireshark)

log-level=INFO
backend=127.0.0.1,8888
frontend=127.0.0.1,8125;no-tls
http2-proxy=yes

Use this test code

package main

import (
    "crypto/tls"
    "io"
    "log"
    "net"
    "net/http"
    "os"

    "golang.org/x/net/http2"
)

func main() {
    tr := &http2.Transport{
        DialTLS: func(network, addr string, cfg *tls.Config) (net.Conn, error) {
            return net.Dial("tcp", "127.0.0.1:8125")
        },
        AllowHTTP: true,
    }
    pr, pw := io.Pipe()
    req, err := http.NewRequest("CONNECT", "http://www.baidu.com/", pr)
    if err != nil {
        log.Fatal(err)
    }
    resp, err := tr.RoundTrip(req)
    if err != nil {
        log.Fatal(err)
    }
    log.Printf("resp: %v", resp)
    if resp.StatusCode != http.StatusOK {
        log.Fatalf("want 200 OK, got %s", resp.Status)
    }
    req, _ = http.NewRequest("GET", "http://www.baidu.com/", nil)
    if err = req.Write(pw); err != nil {
        log.Fatal(err)
    }
    io.Copy(os.Stdout, resp.Body)
}

After running go run, it still hangs. nghttpx log shows SETTINGS frame timeout, and wireshark shows no concrete request is sent out to nghttpx from golang program.

@ayanamist

This comment has been minimized.

Copy link
Author

commented Sep 28, 2016

@bradfitz I have found the problem.
https://github.com/golang/net/blob/f09c4662a0bd6bd8943ac7b4931e185df9471da4/http2/transport.go#L630
It is trying to read the body for 1 byte to detect if there any data, but here body is a pipe and no data is writing, so it hangs forever.
I just change to add

    if req.Method == http.MethodConnect {
        return req.Body, -1
    }

And it works now

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

Thanks for debugging. You can also just set the http.Request.ContentLength to -1 too, and then you don't need to modify the net/http package.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

For Go 1.11, we can special-case CONNECT for HTTP/1 proxies in *http.Transport: if the user sends an explicit CONNECT request, the Transport will (should) know now to ever reuse that connection, and then the *http.Response's Body can just be set to the underlying net.Conn. The caller can then do the TLS handshake on it if appropriate.

/cc @tombergan

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

then the *http.Response's Body can just be set to the underlying net.Conn. The caller can then do the TLS handshake on it if appropriate.

If we do this, then I think we should automatically handle the TLS handshake if the CONNECT URL's scheme is "https".

This feels kind of sneaky, though. We can't ask all RoundTrip implementations to support CONNECT in this way because that would be a backwards incompatible API change. It seems simpler to add a Connect method to http.Transport. This also gives us a nice place to document any peculiarities of the request:

// Requires: tr.Proxy is non-nil, req.Method is "CONNECT", and the Host is set
// in req or req.URL.
// req.URL.{Path,Query,Userinfo} are ignored.
//
// Semantics: A CONNECT request (req) is sent to tr.Proxy(req). We return the response
// to that request. The response always has an empty body. If the response is 200,
// we also return a net.Conn which represents the CONNECT tunnel. If req.URL.Scheme
// is "https", we automatically do the TLS handshake after getting 200, otherwise we
// return the raw Conn.
func (tr *Transport) Connect(req *http.Request) (*http.Response, net.Conn, error)

This is basically equivalent to your suggestion, except that the net.Conn is returned explicitly
rather than being squirreled away in Response.Body when Response.StatusCode is 200.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

If we do this, then I think we should automatically handle the TLS handshake if the CONNECT URL's scheme is "https".

I disagree. That is a layering violation, and we have no knowledge of what protocol is being run over the CONNECT-ed connection. You can have an http:// or an https:// CONNECT server proxying any types of connections (TLS or not TLS) over any port numbers. You can't use the port numbers to determine whether it's TLS or not either.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

It's the scheme not the port. Examples:

connectA = Request{URL: "https://www.foo.com/"}            # the tunnel is TLS
connectB = Request{URL: "http://www.foo.com/"}             # the tunnel is not TLS
connectC = Request{URL.Scheme = "custom", Host="foo.com"}  # the tunnel is still not TLS ("custom" protocol)
connectC = Request{URL.Scheme = "", Host="foo.com"}        # the tunnel is still not TLS (unspecified protocol)

Any of the above requests could be sent to any of the below proxies:

Transport.Proxy = "http://www.proxy.com/"   # proxy is HTTP
Transport.Proxy = "https://www.proxy.com/"  # proxy is HTTPS

That is: tr.Proxy specifies the CONNECT server that will receive the CONNECT request. tr.Proxy.Scheme specifies the protocol we use to speak to the CONNECT server.

req.Host (or req.URL.Host) specifies the origin server that we will CONNECT to. req.URL.Scheme specifies the protocol that will be used when talking to the origin server. If that protocol is TLS, we can auto handshake.

Maybe this is too complex, though.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

Oh, yeah, that's some gnarly magic. I'd rather avoid that, at least in the first phase.

I think the scheme for CONNECT requests should be the empty string.

Other misc thoughts.

Currently the Request.URL field is documented like https://golang.org/pkg/net/http/#Request

        // For client requests, the URL's Host specifies the server to
        // connect to, while the Request's Host field optionally
        // specifies the Host header value to send in the HTTP
        // request.
        URL *url.URL

Current demo: https://play.golang.org/p/3izjmelGx8

Writes:

CONNECT backend:443 HTTP/1.1
Host: backend:443
User-Agent: Go-http-client/1.1

We might want a way to let an *http.Request contain a stand-alone CONNECT request without relying on the Transport.Proxy field. Currently we can't even make the CONNECT argument be different from the Host header, but they need to be for many servers. (that's what motivated Transport.ProxyConnectHeader in #15027). Maybe ProxyConnectHeader is enough, but relying on that seems gross if we're going to do first-class CONNECT support. I'd like the Request to stand alone and be usable with http.DefaultClient.Do.

Hah--- there is a current way to do it, but it's gross: https://play.golang.org/p/0Y4nNcvvwx

Note that the URL.Path must be non-empty.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

This is getting kind of crazy :)

I'd like the Request to stand alone and be usable with http.DefaultClient.Do

I don't think this is possible without adding another field to *http.Request, for the reasons you outlined above.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 4, 2017

I don't think it's that crazy. Consider this minimal example:

https://play.golang.org/p/67B7cqIxUB

So if that Path field weren't there, it'd just be:

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

func main() {
	req := &http.Request{
		Method: "CONNECT",
		URL: &url.URL{
			Scheme: "https", // of proxy.com
			Host:   "proxy.com",
			Opaque: "backend:443",
		},
	}
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		log.Fatal(err)
	}
	if res.StatusCode%100 != 2 {
		log.Fatal(res.Status)
	}
	conn := res.Body.(net.Conn) // to backend:443
	_ = conn
}

We'd only need to document that Request.URL.Opaque is the host:port target for CONNECT, and that upon a successful connect, the res.Body is the underlying net.Conn.

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jul 10, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Jul 10, 2018

Change https://golang.org/cl/123156 mentions this issue: net/http: flesh out HTTP/1's CONNECT+bidi support to match HTTP/2

@gopherbot

This comment has been minimized.

Copy link

commented Aug 24, 2018

Change https://golang.org/cl/131279 mentions this issue: net/http: make Transport return Writable Response.Body on protocol switch

gopherbot pushed a commit that referenced this issue Aug 25, 2018

net/http: make Transport return Writable Response.Body on protocol sw…
…itch

Updates #26937
Updates #17227

Change-Id: I79865938b05c219e1947822e60e4f52bb2604b70
Reviewed-on: https://go-review.googlesource.com/131279
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@gopherbot gopherbot closed this in da6c168 Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.