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: `http.Server.Serve` doesn't serve http2 traffic #14374

Closed
jingweno opened this issue Feb 18, 2016 · 9 comments
Closed

x/net/http2: `http.Server.Serve` doesn't serve http2 traffic #14374

jingweno opened this issue Feb 18, 2016 · 9 comments

Comments

@jingweno
Copy link

@jingweno jingweno commented Feb 18, 2016

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

1.6

  • What operating system and processor architecture are you using?

Darwin, AMD64

  • What did you do?

I'm trying to pass a tls net.Listener to http.Server.Serve but the server didn't run on http2.

See below code:

package main

import (
    "crypto/tls"
    "fmt"
    "log"
    "net"
    "net/http"
    "time"
)

func main() {
    crt := "YOUR_CRT"
    key := "YOUR_KEY"

    mux := http.NewServeMux()
    mux.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
        fmt.Fprintf(w, "hello\n")
    })

    ss := &http.Server{
        Addr:    ":8080",
        Handler: mux,
    }

    err := listenAndServeTLS(ss, []byte(crt), []byte(key))
    if err != nil {
        log.Fatal(err)
    }
}

// listenAndServeTLS is equivalent to http.Server.ListenAndServeTLS
// but loads cert and key as []byte instead of files
func listenAndServeTLS(srv *http.Server, cert, key []byte) error {
    addr := srv.Addr
    if addr == "" {
        addr = ":https"
    }

    config := cloneTLSConfig(srv.TLSConfig)
    if !strSliceContains(config.NextProtos, "http/1.1") {
        config.NextProtos = append(config.NextProtos, "http/1.1")
    }

    var err error
    config.Certificates = make([]tls.Certificate, 1)
    config.Certificates[0], err = tls.X509KeyPair(cert, key)
    if err != nil {
        return err
    }

    ln, err := net.Listen("tcp", addr)
    if err != nil {
        return err
    }

    tlsListener := tls.NewListener(tcpKeepAliveListener{ln.(*net.TCPListener)}, config)
    return srv.Serve(tlsListener)
}

// tcpKeepAliveListener sets TCP keep-alive timeouts on accepted
// connections. It's used by ListenAndServe and ListenAndServeTLS so
// dead TCP connections (e.g. closing laptop mid-download) eventually
// go away.
type tcpKeepAliveListener struct {
    *net.TCPListener
}

func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) {
    tc, err := ln.AcceptTCP()
    if err != nil {
        return
    }
    tc.SetKeepAlive(true)
    tc.SetKeepAlivePeriod(3 * time.Minute)
    return tc, nil
}

func strSliceContains(ss []string, s string) bool {
    for _, v := range ss {
        if v == s {
            return true
        }
    }
    return false
}

// cloneTLSConfig returns a shallow clone of the exported
// fields of cfg, ignoring the unexported sync.Once, which
// contains a mutex and must not be copied.
//
// The cfg must not be in active use by tls.Server, or else
// there can still be a race with tls.Server updating SessionTicketKey
// and our copying it, and also a race with the server setting
// SessionTicketsDisabled=false on failure to set the random
// ticket key.
//
// If cfg is nil, a new zero tls.Config is returned.
func cloneTLSConfig(cfg *tls.Config) *tls.Config {
    if cfg == nil {
        return &tls.Config{}
    }
    return &tls.Config{
        Rand:                     cfg.Rand,
        Time:                     cfg.Time,
        Certificates:             cfg.Certificates,
        NameToCertificate:        cfg.NameToCertificate,
        GetCertificate:           cfg.GetCertificate,
        RootCAs:                  cfg.RootCAs,
        NextProtos:               cfg.NextProtos,
        ServerName:               cfg.ServerName,
        ClientAuth:               cfg.ClientAuth,
        ClientCAs:                cfg.ClientCAs,
        InsecureSkipVerify:       cfg.InsecureSkipVerify,
        CipherSuites:             cfg.CipherSuites,
        PreferServerCipherSuites: cfg.PreferServerCipherSuites,
        SessionTicketsDisabled:   cfg.SessionTicketsDisabled,
        SessionTicketKey:         cfg.SessionTicketKey,
        ClientSessionCache:       cfg.ClientSessionCache,
        MinVersion:               cfg.MinVersion,
        MaxVersion:               cfg.MaxVersion,
        CurvePreferences:         cfg.CurvePreferences,
    }
}

When I curled it:

$ curl https://localhost:8080 -k -v --http2
* Rebuilt URL to: https://localhost:8080/
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8080 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* successfully set certificate verify locations:
*   CAfile: /opt/boxen/homebrew/etc/openssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS header, Certificate Status (22):
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Client hello (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES256-GCM-SHA384
* ALPN, server accepted to use http/1.1
* Server certificate:
*        subject: C=US; ST=California; L=San Francisco; O=Heroku; OU=Heroku API; CN=Midgard; emailAddress=api@heroku.com
*        start date: Oct 30 17:30:09 2015 GMT
*        expire date: Mar 13 17:30:09 2017 GMT
*        issuer: C=US; ST=California; L=San Francisco; O=Heroku; OU=Heroku API; CN=Midgard; emailAddress=api@heroku.com
*        SSL certificate verify result: self signed certificate (18), continuing anyway.
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.47.1
> Accept: */*
>
< HTTP/1.1 200 OK
< Date: Thu, 18 Feb 2016 02:09:44 GMT
< Content-Length: 5
< Content-Type: text/plain; charset=utf-8
<
* Connection #0 to host localhost left intact
hello%

I compared line by line my implementation of listenAndServeTLS with http.Sever.ListenAndServeTLS. The difference is I didn't call srv.setupHTTP2(). But it should be called in http.Server.Serve again. However, if I changed the code to http.Server.ListenAndServeTLS, the server ran on http2.

  • What did you expect to see?

I expected the server to run on http2 with http.Sever.Serve.

  • What did you see instead?

The server ran on http1/1

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 18, 2016

Per the docs, Go doesn't enable http2 for you automatically if you've set your Server's TLSConfig.NextProtos at all. If it's non-nil, it steps out of the way and assumes you want to handle it all. And in your code, you're setting it to "http/1.1".

@bradfitz bradfitz closed this Feb 18, 2016
@jingweno

This comment has been minimized.

Copy link
Author

@jingweno jingweno commented Feb 23, 2016

Even if I removed the following line:

 if !strSliceContains(config.NextProtos, "http/1.1") {
        config.NextProtos = append(config.NextProtos, "http/1.1")
    }

it's still not serving http2 traffic...

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 23, 2016

Set srv.TLSConfig = config before your call to to srv.Serve(tlsListener) and it will work.

The http.Server's code to auto-enable http2 needs to have a reference to the *tls.Config in order to modify it to add the NextProtos.

@jingweno

This comment has been minimized.

Copy link
Author

@jingweno jingweno commented Feb 23, 2016

Set srv.TLSConfig = config before your call to to srv.Serve(tlsListener) and it will work.

That works! But why would http.Server.ListenAndServeTLS work? I follow every single line of code except https://github.com/golang/go/blob/release-branch.go1.6/src/net/http/server.go#L2245-L2279.

@jingweno

This comment has been minimized.

Copy link
Author

@jingweno jingweno commented Feb 23, 2016

Oh, that's because of

go/src/net/http/h2_bundle.go

Lines 2554 to 2576 in 58ec583

if s.TLSConfig == nil {
s.TLSConfig = new(tls.Config)
} else if s.TLSConfig.CipherSuites != nil {
// If they already provided a CipherSuite list, return
// an error if it has a bad order or is missing
// ECDHE_RSA_WITH_AES_128_GCM_SHA256.
const requiredCipher = tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
haveRequired := false
sawBad := false
for i, cs := range s.TLSConfig.CipherSuites {
if cs == requiredCipher {
haveRequired = true
}
if http2isBadCipher(cs) {
sawBad = true
} else if sawBad {
return fmt.Errorf("http2: TLSConfig.CipherSuites index %d contains an HTTP/2-approved cipher suite (%#04x), but it comes after unapproved cipher suites. With this configuration, clients that don't support previous, approved cipher suites may be given an unapproved one and reject the connection.", i, cs)
}
}
if !haveRequired {
return fmt.Errorf("http2: TLSConfig.CipherSuites is missing HTTP/2-required TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256")
}
}
. Should the doc be updated to clarify this? Or maybe the implementation could adjust a bit?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 23, 2016

Which docs need to be clarified?

What implementation needs to be adjusted a bit?

@jingweno

This comment has been minimized.

Copy link
Author

@jingweno jingweno commented Feb 23, 2016

Here:

        // TLSNextProto optionally specifies a function to take over
        // ownership of the provided TLS connection when an NPN
        // protocol upgrade has occurred.  The map key is the protocol
        // name negotiated. The Handler argument should be used to
        // handle HTTP requests and will initialize the Request's TLS
        // and RemoteAddr if not already set.  The connection is
        // automatically closed when the function returns.
        // If TLSNextProto is nil, HTTP/2 support is enabled automatically.
        TLSNextProto map[string]func(*Server, *tls.Conn, Handler)

It makes it sound like TLSNextProto is the only one that determines enabling/disabling http2. In fact, if we are passing a TLSConfig, TLSNextProto won't matter anymore. And the doc for TLSConfig just says it's used by ListenAndServeTLS:

TLSConfig      *tls.Config   // optional TLS config, used by ListenAndServeTLS

and the doc for TLSConfig.NextProtos:

// NextProtos is a list of supported, application level protocols.
NextProtos []string

We should mention TLSConfig.NextProtos should be nil if we're to enable http2. Also I find having two ways to control enabling/disabling http2 confusing: Server.TLSNextProto and TLSConfig.NextProtos. It seems like they're talking about the same thing but they're in two different places: http.Server and TLSConfig.

Does it make sense?

dkumor added a commit to dkumor/acmewrapper that referenced this issue Mar 19, 2016
uncreativemynameis on reddit pointed out that the examples don't have http2 support since they don't pass in tlsconfig to the http server.
https://www.reddit.com/r/golang/comments/4axi8q/acmewrapper_add_lets_encrypt_support_to_your_go/d15fwi3

This is the go issue:
golang/go#14374

The example and readme were changed to have http2 enabled.
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented May 31, 2016

Agreed that this is way too magic not to be documented everywhere.

For example both Serve and TLSConfig should warn you that you won't get HTTP/2 with Serve unless you set Server.TLSConfig or tls.Config.NextProtos.

A colleague just tripped into this, took a while to realize it.

Also because #15908 would make you assume otherwise.

Should i open a documentation issue?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jun 29, 2016

@FiloSottile, closed issues aren't tracked and usually comments on closed issues aren't seen.

This is more documented as of b5f0aff

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.