[bug] Conn.UnderlyingConn can return partially-read connection

[ipostelnik]

Client.DialContext uses buffered reader to read from the underlying network connection. It's possible that some response data will get buffered along with the headers. We've have observed this happening on slow connection.

Calling Conn.UnderlyingConn returns connection where some data has been read into Conn.br without a way to access it. This is why http.Hijacker.Hijack() function returns bufio.ReadWriter along with the connection.

[ghost]

Are you looking for a hijack API where the application takes over the connection from the websocket.Conn? If so, that should be a new API.

See my comment in #680. UnderlyingConn is provided for access to underlying connection implementation, not hijack.

[ghost]

I took a quick look at the cloudflared issue and code. The code uses this package to:

1. Dial a net.Conn
2. Send the client opening handshake
3. Receive the server opening handshake

After completion of these steps, the Cloudflared attempts to hijack the connection by calling UnderlyingConn. Cloudflared does not use the websocket connection methods to read or write messages. It follows that Cloudlfared does not use protocol extensions (currently per message default).

Here are some possible fixes to the cloudflared bug:

• Add a Hijack API to this package. Update cloudflared to use the new API.
• Modify cloudflared to use the net/http client to execute steps 1 through 3 above. Cloudflared already has code to muck with the websocket specific headers. It's not a stretch to for Cloudflared to manage those headers in a *http.Request and *http.Response. The code for httputil.ReverseProxy can serve is inspiration.

[ipostelnik]

Thanks for suggestions. I'm actually going down both of these paths. It's pretty clear that cloudflared is abusing UnderlyingConn in a way it was never designed to be used. I will work on Hijak-like method in the style of stdlib for Gorilla. I like this approach because it most likely to be compatible with quirks of current behavior. I also have an attempt that proxies websockets without Gorilla using a bidirectional stream and a pipe, but it's currently failing some of our unit tests. This is where I'd like to go long term.

…

-Igor

On Fri, Apr 2, 2021 at 9:22 PM MonicaRJones ***@***.***> wrote: I took a quick look at the cloudflared issue and code. The code uses this package to: 1. Dial a net.Conn 2. Send the client opening handshake 3. Receive the server opening handshake After completion of these steps, the Cloudflared attempts to hijack the connection by calling UnderlyingConn. Cloudflared does not use the websocket connection methods to read or write messages. It follows that Cloudlfared does not use protocol extensions (currently per message default). Here are some possible fixes to the cloudflared bug: - Add a Hijack API to this package. Update cloudflared to use the new API. - Modify cloudflared to use the net/http client to execute steps 1 through 3 above. Cloudflared already has code to muck with the websocket specific headers. It's not a stretch to for Cloudflared to manage those headers in a *http.Request and *http.Response. The code for httputil.ReverseProxy can serve is inspiration. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#679 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACLHER7KXSVLJVZ4K5DTODTGZ3XBANCNFSM42JI7F5Q> .

[ghost]

using a bidirectional stream and a pipe

If you don't need a pipe with Gorilla (I didn't see one in the code), then you don't need a pipe without Gorilla. Here's a rough sketch of the code based on the net/http client. The bit of magic is that the net/http client sets the Response.Body to an io.ReaderWriter for a switching protocol response (Response.StatusCode == 101).

 req, err := http.NewRequest("GET",  url, nil)
 if err != nil {
     // handle error
}

// Add code to copy non-handshake headers form cloudlared initial request

req.Header.Set("Upgrade", "websocket")
req.Header.Set("Connection", "Upgrade")

// Set map directly for Sec-WebSockt-XXX to bypass CanonicalHeaderKey.
req.Header["Sec-WebSocket-Key"] = []string{challengeKey}
req.Header["Sec-WebSocket-Version"] = []string{"13"}

resp, err = http.DefaultClient.Do(req)
if err != nil {
      // handle error
}

rw, ok := resp.Body.(io.ReadWriter)
if !ok || 
    resp.StatusCode != 101  ||
    !tokenListContainsValue(resp.Header, "Upgrade", "websocket") ||
    !tokenListContainsValue(resp.Header, "Connection", "upgrade") ||
    resp.Header.Get("Sec-Websocket-Accept") != computeAcceptKey(challengeKey) {
    resp.Body.Close()
    // handshake failed, handle error 
}
// Success!  rw is the io.ReadWriter for the underlying connection.

[ipostelnik]

Yeah, TIL that "As of Go 1.12, the [Response.]Body will also implement io.Writer on a successful "101 Switching Protocols" response" . This should make things a lot easier.

…

On Fri, Apr 2, 2021 at 11:03 PM MonicaRJones ***@***.***> wrote: using a bidirectional stream and a pipe If you don't need a pipe with Gorilla (I didn't see one in the code), then you don't need a pipe without Gorilla. Here's a rough sketch of the code based on the net/http client. The bit of magic is that the net/http client sets the Response.Body to an io.ReaderWriter for a switching protocol response (Response.StatusCode == 101). req, err := http.NewRequest("GET", url, nil) if err != nil { // handle error } // Add code to copy non-handshake headers form cloudlared initial request req.Header.Set("Upgrade", "websocket") req.Header.Set("Connection", "Upgrade") // Set map directly for Sec-WebSockt-XXX to bypass CanonicalHeaderKey. req.Header["Sec-WebSocket-Key"] = []string{challengeKey} req.Header["Sec-WebSocket-Version"] = []string{"13"} resp, err = http.DefaultClient.Do(req) if err != nil { // handle error } rw, ok := resp.Body.(io.ReadWriter) if !ok || resp.StatusCode != 101 || !tokenListContainsValue(resp.Header, "Upgrade", "websocket") || !tokenListContainsValue(resp.Header, "Connection", "upgrade") || resp.Header.Get("Sec-Websocket-Accept") != computeAcceptKey(challengeKey) { resp.Body.Close() // handshake failed, handle error } // Success! rw is the io.ReadWriter for the underlying connection. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#679 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACLHEUAMCVR62PTBAE73JDTG2HPHANCNFSM42JI7F5Q> .

[ghost]

@ipostelnik It looks like you resolved the problem on your end. Can this issue be closed?

This package should not add a Hijack API unless there's a real need for one.