Enforce cipher suites.

server.go

@@ -135,6 +135,31 @@ func ConfigureServer(s *http.Server, conf *Server) {
 	if s.TLSConfig == nil {
 		s.TLSConfig = new(tls.Config)
 	}
+
+	// Note: not setting MinVersion to tls.VersionTLS12,
+	// as we don't want to interfere with HTTP/1.1 traffic
+	// on the user's server. We enforce TLS 1.2 later once
+	// we accept a connection. Ideally this should be done
+	// 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)
+		}
+	}
+
 	haveNPN := false
 	for _, p := range s.TLSConfig.NextProtos {
 		if p == NextProtoTLS {
@@ -212,7 +237,20 @@ func (srv *Server) handleConn(hs *http.Server, c net.Conn, h http.Handler) {
 			return
 		}
 
-		// TODO: verify cipher suites. (9.2.1, 9.2.2)
+		if isBadCipher(sc.tlsState.CipherSuite) {
+			// "Endpoints MAY choose to generate a connection error
+			// (Section 5.4.1) of type INADEQUATE_SECURITY if one of
+			// the prohibited cipher suites are negotiated."
+			//
+			// We choose that. In my opinion, the spec is weak
+			// here. It also says both parties must support at least
+			// TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 so there's no
+			// excuses here. If we really must, we could allow an
+			// "AllowInsecureWeakCiphers" option on the server later.
+			// Let's see how it plays out first.
+			sc.rejectConn(ErrCodeInadequateSecurity, "Prohibited TLS 1.2 Cipher Suite")
+			return
+		}
 	}
 
 	if hook := testHookGetServerConn; hook != nil {
@@ -221,6 +259,30 @@ func (srv *Server) handleConn(hs *http.Server, c net.Conn, h http.Handler) {
 	sc.serve()
 }
 
+// isBadCipher reports whether the cipher is blacklisted by the HTTP/2 spec.
+func isBadCipher(cipher uint16) bool {
+	switch cipher {
+	case tls.TLS_RSA_WITH_RC4_128_SHA,
+		tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
+		tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_ECDHE_RSA_WITH_RC4_128_SHA,
+		tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA:
+		// Reject cipher suites from Appendix A.
+		// "This list includes those cipher suites that do not
+		// offer an ephemeral key exchange and those that are
+		// based on the TLS null, stream or block cipher type"
+		return true
+	default:
+		return false
+	}
+}
+
 func (sc *serverConn) rejectConn(err ErrCode, debug string) {
 	// ignoring errors. hanging up anyway.
 	sc.framer.WriteGoAway(0, err, []byte(debug))

server_test.go

@@ -65,21 +65,25 @@ func newServerTester(t testing.TB, handler http.HandlerFunc, opts ...interface{}
 
 	logBuf := new(bytes.Buffer)
 	ts := httptest.NewUnstartedServer(handler)
-	ConfigureServer(ts.Config, &Server{})
 
 	tlsConfig := &tls.Config{
 		InsecureSkipVerify: true,
 		NextProtos:         []string{NextProtoTLS},
 	}
+
 	for _, opt := range opts {
 		switch v := opt.(type) {
-		case func(c *tls.Config):
+		case func(*tls.Config):
 			v(tlsConfig)
+		case func(*httptest.Server):
+			v(ts)
 		default:
 			t.Fatalf("unknown newServerTester option type %T", v)
 		}
 	}
 
+	ConfigureServer(ts.Config, &Server{})
+
 	st := &serverTester{
 		t:      t,
 		ts:     ts,
@@ -1974,6 +1978,48 @@ func testRejectTLS(t *testing.T, max uint16) {
 	}
 }
 
+func TestServer_Rejects_TLSBadCipher(t *testing.T) {
+	st := newServerTester(t, nil, func(c *tls.Config) {
+		// Only list bad ones:
+		c.CipherSuites = []uint16{
+			tls.TLS_RSA_WITH_RC4_128_SHA,
+			tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
+			tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+			tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+			tls.TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,
+			tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+			tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
+			tls.TLS_ECDHE_RSA_WITH_RC4_128_SHA,
+			tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
+			tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+			tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
+		}
+	})
+	defer st.Close()
+	gf := st.wantGoAway()
+	if got, want := gf.ErrCode, ErrCodeInadequateSecurity; got != want {
+		t.Errorf("Got error code %v; want %v", got, want)
+	}
+}
+
+func TestServer_Advertises_Common_Cipher(t *testing.T) {
+	const requiredSuite = tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
+	st := newServerTester(t, nil, func(c *tls.Config) {
+		// Have the client only support the one required by the spec.
+		c.CipherSuites = []uint16{requiredSuite}
+	}, func(ts *httptest.Server) {
+		var srv *http.Server = ts.Config
+		// Have the server configured with one specific cipher suite
+		// which is banned. This tests that ConfigureServer ends up
+		// adding the good one to this list.
+		srv.TLSConfig = &tls.Config{
+			CipherSuites: []uint16{tls.TLS_RSA_WITH_AES_128_CBC_SHA}, // just a banned one
+		}
+	})
+	defer st.Close()
+	st.greet()
+}
+
 // TODO: move this onto *serverTester, and re-use the same hpack
 // decoding context throughout.  We're just getting lucky here with
 // creating a new decoder each time.