http2: make ConfigureServer set PreferServerCipherSuites, return an errors

Fixes golang/go#12895

Change-Id: I7cf6e63b1bbdf1f4e8974c00bdaed69b74f6db49
Reviewed-on: https://go-review.googlesource.com/15860Reviewed-by: Adam Langley <agl@golang.org>

http2/server.go

@@ -130,12 +130,33 @@ func (s *Server) maxConcurrentStreams() uint32 {
 // The configuration conf may be nil.
 //
 // ConfigureServer must be called before s begins serving.
-func ConfigureServer(s *http.Server, conf *Server) {
+func ConfigureServer(s *http.Server, conf *Server) error {
 	if conf == nil {
 		conf = new(Server)
 	}
+
 	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 isBadCipher(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")
+		}
 	}
 
 	// Note: not setting MinVersion to tls.VersionTLS12,
@@ -145,22 +166,7 @@ func ConfigureServer(s *http.Server, conf *Server) {
 	// during next-proto selection, but using TLS <1.2 with
 	// HTTP/2 is still the client's bug.
 
-	// Be sure we advertise tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
-	// at least.
-	// TODO: enable PreferServerCipherSuites?
-	if s.TLSConfig.CipherSuites != nil {
-		const requiredCipher = tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
-		haveRequired := false
-		for _, v := range s.TLSConfig.CipherSuites {
-			if v == requiredCipher {
-				haveRequired = true
-				break
-			}
-		}
-		if !haveRequired {
-			s.TLSConfig.CipherSuites = append(s.TLSConfig.CipherSuites, requiredCipher)
-		}
-	}
+	s.TLSConfig.PreferServerCipherSuites = true
 
 	haveNPN := false
 	for _, p := range s.TLSConfig.NextProtos {
@@ -187,6 +193,7 @@ func ConfigureServer(s *http.Server, conf *Server) {
 	}
 	s.TLSNextProto[NextProtoTLS] = protoHandler
 	s.TLSNextProto["h2-14"] = protoHandler // temporary; see above.
+	return nil
 }
 
 func (srv *Server) handleConn(hs *http.Server, c net.Conn, h http.Handler) {

http2/server_test.go

@@ -2522,3 +2522,67 @@ func (c *issue53Conn) RemoteAddr() net.Addr               { return &net.TCPAddr{
 func (c *issue53Conn) SetDeadline(t time.Time) error      { return nil }
 func (c *issue53Conn) SetReadDeadline(t time.Time) error  { return nil }
 func (c *issue53Conn) SetWriteDeadline(t time.Time) error { return nil }
+
+// golang.org/issue/12895
+func TestConfigureServer(t *testing.T) {
+	tests := []struct {
+		name    string
+		in      http.Server
+		wantErr string
+	}{
+		{
+			name: "empty server",
+			in:   http.Server{},
+		},
+		{
+			name: "just the required cipher suite",
+			in: http.Server{
+				TLSConfig: &tls.Config{
+					CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+				},
+			},
+		},
+		{
+			name: "missing required cipher suite",
+			in: http.Server{
+				TLSConfig: &tls.Config{
+					CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384},
+				},
+			},
+			wantErr: "is missing HTTP/2-required TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
+		},
+		{
+			name: "required after bad",
+			in: http.Server{
+				TLSConfig: &tls.Config{
+					CipherSuites: []uint16{tls.TLS_RSA_WITH_RC4_128_SHA, tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256},
+				},
+			},
+			wantErr: "contains an HTTP/2-approved cipher suite (0xc02f), but it comes after",
+		},
+		{
+			name: "bad after required",
+			in: http.Server{
+				TLSConfig: &tls.Config{
+					CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, tls.TLS_RSA_WITH_RC4_128_SHA},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		err := ConfigureServer(&tt.in, nil)
+		if (err != nil) != (tt.wantErr != "") {
+			if tt.wantErr != "" {
+				t.Errorf("%s: success, but want error", tt.name)
+			} else {
+				t.Errorf("%s: unexpected error: %v", tt.name, err)
+			}
+		}
+		if err != nil && tt.wantErr != "" && !strings.Contains(err.Error(), tt.wantErr) {
+			t.Errorf("%s: err = %v; want substring %q", tt.name, err, tt.wantErr)
+		}
+		if err == nil && !tt.in.TLSConfig.PreferServerCipherSuites {
+			t.Error("%s: PreferServerCipherSuite is false; want true", tt.name)
+		}
+	}
+}