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: Exporting h2 connection setup #12737

Closed
kennylevinsen opened this issue Sep 24, 2015 · 2 comments

Comments

Projects
None yet
5 participants
@kennylevinsen
Copy link

commented Sep 24, 2015

Issue from old http2 issue tracker: bradfitz/http2#69

Some initial context

soheilhy have written cmux (https://github.com/soheilhy/cmux), and I have written serve2/serve2d (https://github.com/joushou/serve2d). These "protocol multiplexers" read some data from a connection, identifies it with some preprovided detection mechanisms, and feeds the result as a buffered net.Conn to a handler that should support the protocol. In case of serve2d, TLS is also handled by that machinery, leaving only a buffered net.Conn implementation, and the ability to inquire about transport information, such as ALPN information from TLS, but *tls.Conn's are never exposed, as I need to feed the data I read back when the protocol is about to be handled.

Our issue

Currently, the http lib(s) only provide a single way to handle HTTP connections, and that's through serving a net.Listener. This means that, to feed http/1.1 connections, one must have "fake" net.Listener stuffing connections into the http packages own server loop. The HTTP server logic then directly checks if the receives conn is a *tls.Conn, in which case it performs some setup, checks if there are hooks for the ALPN (called NPN in the source), and calls them if necessary. This means that if you are not feeding it a *tls.Conn, either due to using a different implementation, or due just doing something weird and different (like me), things won't work as intended. For HTTP/1.1, this only results in http.Request's TLS field not being set, but that is only a minor inconvenience, as the information can be retrieved through other means.

With h2, however, it is not possible to start the protocol without the http package knowing about the TLS wrapping, effectively making it impossible to start a h2 session using x/net/http2 when you're not using crypto/tls's tls.Conn. For serve2d, the result is that, while I can easily forward traffic to a real h2 server by just wrapping the traffic in a TLS client and requesting h2 as NextProto, the builtin web server (a convenience feature) is only capable of serving HTTP/1.1.

A solution: Expose HandleConn

Solheilhy suggested in the old http2 issue tracker to expose Server.handleConn. This would allow one to provide an arbitrary net.Conn to start h2 upon, without http2 worrying about the TLS part (all TLS checks are only run if the provided net.Conn is actually a *tls.Conn). To copy his example from the old bug report, a server loop could then be made manually like this:

s := &http.Server{
    Handler: &myHandler{},
}
s2 := &http2.Server{}
for {
    c, err := l.Accept()
    if err != nil {
        if terr, ok := err.(net.Error); ok && terr.Temporary() {
            continue
        }
        return
    }
    go s2.HandleConn(s, c, s.Handler)
}

Another solution: Introduce a "NewH2Conn"

Personally, I would also like this changed for the old http package, allowing you to start both http/1.1 and h2 given only a net.Conn. To do this, I would change Server.handleConn to Server.NewH2Conn, which instead of calling Server.serve, would return the serverConn (most likely also exposed as ServerConn), upon which one could call Serve to start the loop manually. This API could also be used as the "advanced" method, where h2c could potentially be exposed (previous discussions seem to indicate that h2c is not wanted by default). The effective result would be that one could manually start a h2 connection on a connection with TLS handled without being a *tls.Conn like so:

s := &http.Server{
    Handler: &myHandler{},
}
s2 := &http2.Server{}
for {
    c, err := l.Accept()
    if err != nil {
        if terr, ok := err.(net.Error); ok && terr.Temporary() {
            continue
        }
        return
    }
    h2 := s2.NewH2Conn(s, c, s.Handler)
    go h2.Serve()
}

One could then also expose a NewH1Conn or similar from the net/http package, exposing the serve() server loop there as Serve as well, making things nicely uniform:

s := &http.Server{
    Handler: &myHandler{},
}
for {
    c, err := l.Accept()
    if err != nil {
        if terr, ok := err.(net.Error); ok && terr.Temporary() {
            continue
        }
        return
    }
    h1 := s.NewH1Conn(s, c, s.Handler)
    go h1.Serve()
}

My fork of http2 (https://github.com/joushou/http2) implements NewH2Conn, and have been working flawlessly at serving HTTP2 from my own servers. The exposeserve branch implements my solution, the exposehandleconn branch implements solheilhy's solution, and master includes both exposeserve and an import adjustment commit for easy usage.

A third solution: Use an interface instead of asserting *tls.Conn

One could change instead of asserting a *tls.Conn, assert an interface that could be implemented easily externally.

Final words

I realize that this request might only fit me and solheilhy's isolated use case, but I do not see any issues in exposing this functionality. Right now, a lot of http functionality seems "closed off", this specific issue being my biggest obstacle. One of the things I love about the Go standard and x libs, is how much it allows you to poke around, and implement things easily that would have taken unfeasible amounts of time and research without those libraries. I'm hoping that the HTTP packages could also allow for this kind of tinkering in the future.

I'd love to help if there's anything I can do.

@rakyll rakyll added this to the Unreleased milestone Sep 28, 2015

@tamird

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2015

This is not just an http2 problem; as pointed out in the cmux readme's limitations section:

TLS: Since cmux sits in between the actual listener and the mux'ed listeners, TLS handshake is not handled inside the actual servers. Because of that, you can serve HTTPS using cmux but http.Request.TLS would not be set in your handlers.

This is really due to the type assertion used to identify TLS connections, which sadly breaks the goodness of net.Conn and net.Listener being interfaces.

I'd like to see the third option implemented, where the type assertion to tls.Conn is replaced with a type assertion to an exported interface.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 3, 2016

CL https://golang.org/cl/19176 mentions this issue.

@golang golang locked and limited conversation to collaborators Feb 3, 2017

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

http2: export Server.ServeConn
Fixes golang/go#12737
Updates golang/go#14141

Change-Id: I552b603b63a7c87d7fcdb4eb09f96ab9fd0ec0aa
Reviewed-on: https://go-review.googlesource.com/19176
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.