Bug 1844990: server/api: Disable unsafe ciphers for CVE-2016-2183

pkg/server/api.go

@@ -60,14 +60,7 @@ func NewAPIServer(a *APIHandler, p int, is bool, c, k string) *APIServer {
 
 // Serve launches the API Server.
 func (a *APIServer) Serve() {
-	mcs := &http.Server{
-		Addr:    fmt.Sprintf(":%v", a.port),
-		Handler: a.handler,
-		// We don't want to allow 1.1 as that's old.  This was flagged in a security audit.
-		TLSConfig: &tls.Config{
-			MinVersion: tls.VersionTLS12,
-		},
-	}
+	mcs := getHTTPServerCfg(fmt.Sprintf(":%v", a.port), a.handler)
 
 	glog.Infof("Launching server on %s", mcs.Addr)
 	if a.insecure {
@@ -340,3 +333,63 @@ func (h *defaultHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(http.StatusMethodNotAllowed)
 	return
 }
+
+// getHTTPServerCfg returns the basic HTTP Server
+func getHTTPServerCfg(addr string, handler http.Handler) *http.Server {
+	return &http.Server{
+		Addr:    addr,
+		Handler: handler,
+		// CVE-2016-2183: disable http/2, which by definition, requires insecure ciphers
+		// Per https://golang.org/src/net/http/doc.go this is the runtime method of disabling HTTP/2
+		TLSNextProto: make(map[string]func(*http.Server, *tls.Conn, http.Handler)),
+		// We don't want to allow 1.1 as that's old.  This was flagged in a security audit.
+		TLSConfig: &tls.Config{
+			MinVersion:   tls.VersionTLS12,
+			CipherSuites: cipherOrder(),
+		},
+	}
+
+}
+
+// Disable insecure cipher suites for CVE-2016-2183
+// cipherOrder returns an ordered list of Ciphers that are considered secure
+// Deprecated ciphers are not returned.
+func cipherOrder() []uint16 {
+	var first []uint16
+	var second []uint16
+
+	allowable := func(c *tls.CipherSuite) bool {
+		// Disallow block ciphers using straight SHA1
+		// See: https://tools.ietf.org/html/rfc7540#appendix-A
+		if strings.HasSuffix(c.Name, "CBC_SHA") {
+			return false
+		}
+		// 3DES is considered insecure
+		if strings.Contains(c.Name, "3DES") {
+			return false
+		}
+		return true
+	}
+
+	for _, c := range tls.CipherSuites() {
+		for _, v := range c.SupportedVersions {
+			if v == tls.VersionTLS13 {
+				first = append(first, c.ID)
+			}
+			if v == tls.VersionTLS12 && allowable(c) {
+				inFirst := false
+				for _, id := range first {
+					if c.ID == id {
+						inFirst = true
+						break
+					}
+				}
+				if !inFirst {
+					second = append(second, c.ID)
+				}
+			}
+		}
+	}
+
+	return append(first, second...)
+}

pkg/server/api_test.go

@@ -1,15 +1,19 @@
 package server
 
 import (
+	"crypto/tls"
 	"fmt"
 	"io/ioutil"
+	"log"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"github.com/coreos/go-semver/semver"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/net/http2"
 	"k8s.io/apimachinery/pkg/runtime"
 
 	ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
@@ -560,6 +564,143 @@ func TestAPIServer(t *testing.T) {
 	}
 }
 
+func TestAPIServerCiphers(t *testing.T) {
+	certData, err := tls.X509KeyPair(
+		[]byte(`-----BEGIN CERTIFICATE-----
+MIIBhTCCASugAwIBAgIQIRi6zePL6mKjOipn+dNuaTAKBggqhkjOPQQDAjASMRAw
+DgYDVQQKEwdBY21lIENvMB4XDTE3MTAyMDE5NDMwNloXDTE4MTAyMDE5NDMwNlow
+EjEQMA4GA1UEChMHQWNtZSBDbzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABD0d
+7VNhbWvZLWPuj/RtHFjvtJBEwOkhbN/BnnE8rnZR8+sbwnc/KhCk3FhnpHZnQz7B
+5aETbbIgmuvewdjvSBSjYzBhMA4GA1UdDwEB/wQEAwICpDATBgNVHSUEDDAKBggr
+BgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MCkGA1UdEQQiMCCCDmxvY2FsaG9zdDo1
+NDUzgg4xMjcuMC4wLjE6NTQ1MzAKBggqhkjOPQQDAgNIADBFAiEA2zpJEPQyz6/l
+Wf86aX6PepsntZv2GYlA5UpabfT2EZICICpJ5h/iI+i341gBmLiAFQOyTDT+/wQc
+6MF9+Yw1Yy0t
+-----END CERTIFICATE-----`),
+		[]byte(`-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEIIrYSSNQFaA2Hwf1duRSxKtLYX5CB04fSeQ6tF1aY/PuoAoGCCqGSM49
+AwEHoUQDQgAEPR3tU2Fta9ktY+6P9G0cWO+0kETA6SFs38GecTyudlHz6xvCdz8q
+EKTcWGekdmdDPsHloRNtsiCa697B2O9IFA==
+-----END EC PRIVATE KEY-----`),
+	)
+	if err != nil {
+		t.Fatalf("failed to create test x509 cert")
+	}
+
+	ts := getHTTPServerCfg(
+		fmt.Sprintf(":%d", 8088),
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			fmt.Fprintln(w, "Hello, client")
+		}),
+	)
+
+	// Configure the TLS testing to allow
+	ts.TLSConfig.Certificates = []tls.Certificate{certData}
+	ts.TLSConfig.InsecureSkipVerify = true
+
+	go func(t *testing.T) {
+		if err := ts.ListenAndServeTLS("", ""); err != http.ErrServerClosed {
+			log.Fatal("error creating test server")
+		}
+	}(t)
+	defer ts.Close()
+
+	type testCase struct {
+		desc      string
+		wantErr   bool
+		errString string
+		client    *http.Client
+		ciphers   []uint16
+	}
+
+	tests := []testCase{
+		{
+			desc:    "ensure base TLS configuration",
+			wantErr: false,
+			client: &http.Client{
+				Transport: &http.Transport{
+					TLSClientConfig: &tls.Config{
+						InsecureSkipVerify: true, // test server certificate is not trusted.
+					},
+				},
+			},
+		},
+		{
+			desc:      "http/2 should fail",
+			wantErr:   true,
+			errString: "http2: unexpected ALPN protocol",
+			client: &http.Client{
+				Transport: &http2.Transport{
+					TLSClientConfig: &tls.Config{
+						InsecureSkipVerify: true, // test server certificate is not trusted.
+					},
+				},
+			},
+		},
+		{
+			desc:      "TLS1.1 should fail",
+			wantErr:   true,
+			errString: "protocol version not supported",
+			client: &http.Client{
+				Transport: &http.Transport{
+					TLSClientConfig: &tls.Config{
+						MaxVersion:         tls.VersionTLS11,
+						InsecureSkipVerify: true, // test server certificate is not trusted.
+					},
+				},
+			},
+		},
+		{
+			desc:    "TLS1.2 should work",
+			wantErr: false,
+			client: &http.Client{
+				Transport: &http.Transport{
+					TLSClientConfig: &tls.Config{
+						MaxVersion:         tls.VersionTLS12,
+						InsecureSkipVerify: true, // test server certificate is not trusted.
+					},
+				},
+			},
+		},
+	}
+
+	// Test that all known insecure ciphers are straight up refused.
+	for _, c := range tls.CipherSuites() {
+		if c.Insecure || strings.HasSuffix(c.Name, "CBC_SHA") || strings.HasSuffix(c.Name, "DES") {
+			tests = append(
+				tests,
+				testCase{
+					desc:    fmt.Sprintf("refuse insecure ciphers %v", c.Name),
+					ciphers: []uint16{c.ID},
+					client: &http.Client{
+						Transport: &http.Transport{
+							TLSClientConfig: &tls.Config{
+								CipherSuites:       []uint16{c.ID},
+								InsecureSkipVerify: true, // test server certificate is not trusted.
+							},
+						},
+					},
+				})
+		}
+	}
+
+	for idx, c := range tests {
+		t.Run(fmt.Sprintf("case#%d: %s", idx, c.desc), func(t *testing.T) {
+			resp, err := c.client.Get("https://localhost:8088")
+			for _, insecure := range c.ciphers {
+				if insecure == resp.TLS.CipherSuite {
+					t.Fatalf("failed to %s:", c.desc)
+				}
+			}
+			if c.wantErr {
+				if !strings.Contains(fmt.Sprintf("%v", err), c.errString) {
+					t.Fatalf("want: %s\n got: %v", c.errString, err)
+				}
+			}
+		})
+	}
+}
+
 func checkStatus(t *testing.T, response *http.Response, expected int) {
 	if response.StatusCode != expected {
 		t.Errorf("expected response status %d, received %d", expected, response.StatusCode)