cmd/*, lib/tlsutil: Refactor TLS stuff (fixes #5256) (#5276)

This changes the TLS and certificate handling in a few ways:

- We always use TLS 1.2, both for sync connections (as previously) and
  the GUI/REST/discovery stuff. This is a tightening of the requirements
  on the GUI. AS far as I can tell from caniusethis.com every browser from
  2013 and forward supports TLS 1.2, so I think we should be fine.

- We always greate ECDSA certificates. Previously we'd create
  ECDSA-with-RSA certificates for sync connections and pure RSA
  certificates for the web stuff. The new default is more modern and the
  same everywhere. These certificates are OK in TLS 1.2.

- We use the Go CPU detection stuff to choose the cipher suites to use,
  indirectly. The TLS package uses CPU capabilities probing to select
  either AES-GCM (fast if we have AES-NI) or ChaCha20 (faster if we
  don't). These CPU detection things aren't exported though, so the tlsutil
  package now does a quick TLS handshake with itself as part of init().
  If the chosen cipher suite was AES-GCM we prioritize that, otherwise we
  prefer ChaCha20. Some might call this ugly. I think it's awesome.

cmd/stdiscosrv/main.go

@@ -121,7 +121,7 @@ func main() {
 	cert, err := tls.LoadX509KeyPair(certFile, keyFile)
 	if err != nil {
 		log.Println("Failed to load keypair. Generating one, this might take a while...")
-		cert, err = tlsutil.NewCertificate(certFile, keyFile, "stdiscosrv", 0)
+		cert, err = tlsutil.NewCertificate(certFile, keyFile, "stdiscosrv")
 		if err != nil {
 			log.Fatalln("Failed to generate X509 key pair:", err)
 		}

cmd/strelaypoolsrv/main.go

@@ -636,7 +636,7 @@ func createTestCertificate() tls.Certificate {
 	}
 
 	certFile, keyFile := filepath.Join(tmpDir, "cert.pem"), filepath.Join(tmpDir, "key.pem")
-	cert, err := tlsutil.NewCertificate(certFile, keyFile, "relaypoolsrv", 3072)
+	cert, err := tlsutil.NewCertificate(certFile, keyFile, "relaypoolsrv")
 	if err != nil {
 		log.Fatalln("Failed to create test X509 key pair:", err)
 	}

cmd/strelaysrv/main.go

@@ -166,7 +166,7 @@ func main() {
 	cert, err := tls.LoadX509KeyPair(certFile, keyFile)
 	if err != nil {
 		log.Println("Failed to load keypair. Generating one, this might take a while...")
-		cert, err = tlsutil.NewCertificate(certFile, keyFile, "strelaysrv", 3072)
+		cert, err = tlsutil.NewCertificate(certFile, keyFile, "strelaysrv")
 		if err != nil {
 			log.Fatalln("Failed to generate X509 key pair:", err)
 		}

cmd/syncthing/gui.go

@@ -185,28 +185,13 @@ func (s *apiService) getListener(guiCfg config.GUIConfiguration) (net.Listener,
 			name = tlsDefaultCommonName
 		}
 
-		cert, err = tlsutil.NewCertificate(s.httpsCertFile, s.httpsKeyFile, name, httpsRSABits)
+		cert, err = tlsutil.NewCertificate(s.httpsCertFile, s.httpsKeyFile, name)
 	}
 	if err != nil {
 		return nil, err
 	}
-	tlsCfg := &tls.Config{
-		Certificates: []tls.Certificate{cert},
-		MinVersion:   tls.VersionTLS10, // No SSLv3
-		CipherSuites: []uint16{
-			// No RC4
-			tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
-			tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
-			tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
-			tls.TLS_RSA_WITH_AES_128_CBC_SHA,
-			tls.TLS_RSA_WITH_AES_256_CBC_SHA,
-			tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
-			tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
-		},
-	}
+	tlsCfg := tlsutil.SecureDefault()
+	tlsCfg.Certificates = []tls.Certificate{cert}
 
 	if guiCfg.Network() == "unix" {
 		// When listening on a UNIX socket we should unlink before bind,

cmd/syncthing/main.go

@@ -78,8 +78,6 @@ const (
 const (
 	bepProtocolName      = "bep/1.0"
 	tlsDefaultCommonName = "syncthing"
-	httpsRSABits         = 2048
-	bepRSABits           = 0 // 384 bit ECDSA used instead
 	defaultEventTimeout  = time.Minute
 	maxSystemErrors      = 5
 	initialSystemLog     = 10
@@ -471,7 +469,7 @@ func generate(generateDir string) {
 		l.Warnln("Key exists; will not overwrite.")
 		l.Infoln("Device ID:", protocol.NewDeviceID(cert.Certificate[0]))
 	} else {
-		cert, err = tlsutil.NewCertificate(certFile, keyFile, tlsDefaultCommonName, bepRSABits)
+		cert, err = tlsutil.NewCertificate(certFile, keyFile, tlsDefaultCommonName)
 		if err != nil {
 			l.Fatalln("Create certificate:", err)
 		}
@@ -639,7 +637,7 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 	cert, err := tls.LoadX509KeyPair(locations[locCertFile], locations[locKeyFile])
 	if err != nil {
 		l.Infof("Generating ECDSA key and certificate for %s...", tlsDefaultCommonName)
-		cert, err = tlsutil.NewCertificate(locations[locCertFile], locations[locKeyFile], tlsDefaultCommonName, bepRSABits)
+		cert, err = tlsutil.NewCertificate(locations[locCertFile], locations[locKeyFile], tlsDefaultCommonName)
 		if err != nil {
 			l.Fatalln(err)
 		}
@@ -680,30 +678,6 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 		}()
 	}
 
-	// The TLS configuration is used for both the listening socket and outgoing
-	// connections.
-
-	tlsCfg := &tls.Config{
-		Certificates:           []tls.Certificate{cert},
-		NextProtos:             []string{bepProtocolName},
-		ClientAuth:             tls.RequestClientCert,
-		SessionTicketsDisabled: true,
-		InsecureSkipVerify:     true,
-		MinVersion:             tls.VersionTLS12,
-		CipherSuites: []uint16{
-			tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
-			tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
-			tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
-			tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
-			tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
-			tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
-		},
-	}
-
 	perf := cpuBench(3, 150*time.Millisecond, true)
 	l.Infof("Hashing performance is %.02f MB/s", perf)
 
@@ -794,6 +768,16 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 	cachedDiscovery := discover.NewCachingMux()
 	mainService.Add(cachedDiscovery)
 
+	// The TLS configuration is used for both the listening socket and outgoing
+	// connections.
+
+	tlsCfg := tlsutil.SecureDefault()
+	tlsCfg.Certificates = []tls.Certificate{cert}
+	tlsCfg.NextProtos = []string{bepProtocolName}
+	tlsCfg.ClientAuth = tls.RequestClientCert
+	tlsCfg.SessionTicketsDisabled = true
+	tlsCfg.InsecureSkipVerify = true
+
 	// Start connection management
 
 	connectionsService := connections.NewService(cfg, myID, m, tlsCfg, cachedDiscovery, bepProtocolName, tlsDefaultCommonName)

lib/discover/global_test.go

@@ -110,9 +110,8 @@ func TestGlobalOverHTTPS(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Generate a server certificate, using fewer bits than usual to hurry the
-	// process along a bit.
-	cert, err := tlsutil.NewCertificate(dir+"/cert.pem", dir+"/key.pem", "syncthing", 1024)
+	// Generate a server certificate.
+	cert, err := tlsutil.NewCertificate(dir+"/cert.pem", dir+"/key.pem", "syncthing")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -176,9 +175,8 @@ func TestGlobalAnnounce(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Generate a server certificate, using fewer bits than usual to hurry the
-	// process along a bit.
-	cert, err := tlsutil.NewCertificate(dir+"/cert.pem", dir+"/key.pem", "syncthing", 1024)
+	// Generate a server certificate.
+	cert, err := tlsutil.NewCertificate(dir+"/cert.pem", dir+"/key.pem", "syncthing")
 	if err != nil {
 		t.Fatal(err)
 	}

lib/tlsutil/tlsutil.go

@@ -27,17 +27,74 @@ var (
 	ErrIdentificationFailed = fmt.Errorf("failed to identify socket type")
 )
 
-// NewCertificate generates and returns a new TLS certificate. If tlsRSABits
-// is greater than zero we generate an RSA certificate with the specified
-// number of bits. Otherwise we create a 384 bit ECDSA certificate.
-func NewCertificate(certFile, keyFile, tlsDefaultCommonName string, tlsRSABits int) (tls.Certificate, error) {
-	var priv interface{}
-	var err error
-	if tlsRSABits > 0 {
-		priv, err = rsa.GenerateKey(rand.Reader, tlsRSABits)
-	} else {
-		priv, err = ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
+var (
+	// The list of cipher suites we will use / suggest for TLS connections.
+	// This is built based on the component slices below, depending on what
+	// the hardware prefers.
+	cipherSuites []uint16
+
+	// Suites that are good and fast on hardware with AES-NI. These are
+	// reordered from the Go default to put the 256 bit ciphers above the
+	// 128 bit ones - because that looks cooler, even though there is
+	// probably no relevant difference in strength yet.
+	gcmSuites = []uint16{
+		tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
 	}
+
+	// Suites that are good and fast on hardware *without* AES-NI.
+	chaChaSuites = []uint16{
+		tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
+		tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
+	}
+
+	// The rest of the suites, minus DES stuff.
+	otherSuites = []uint16{
+		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_RSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_RSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_RSA_WITH_AES_128_CBC_SHA256,
+		tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+	}
+)
+
+func init() {
+	// Creates the list of ciper suites that SecureDefault uses.
+	cipherSuites = buildCipherSuites()
+}
+
+// SecureDefault returns a tls.Config with reasonable, secure defaults set.
+func SecureDefault() *tls.Config {
+	// paranoia
+	cs := make([]uint16, len(cipherSuites))
+	copy(cs, cipherSuites)
+
+	return &tls.Config{
+		// TLS 1.2 is the minimum we accept
+		MinVersion: tls.VersionTLS12,
+		// We want the longer curves at the front, because that's more
+		// secure (so the web tells me, don't ask me to explain the
+		// details).
+		CurvePreferences: []tls.CurveID{tls.CurveP521, tls.CurveP384, tls.CurveP256},
+		// The cipher suite lists built above.
+		CipherSuites: cs,
+		// We've put some thought into this choice and would like it to
+		// matter.
+		PreferServerCipherSuites: true,
+	}
+}
+
+// NewCertificate generates and returns a new TLS certificate.
+func NewCertificate(certFile, keyFile, commonName string) (tls.Certificate, error) {
+	priv, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
 	if err != nil {
 		return tls.Certificate{}, fmt.Errorf("generate key: %s", err)
 	}
@@ -48,11 +105,11 @@ func NewCertificate(certFile, keyFile, tlsDefaultCommonName string, tlsRSABits i
 	template := x509.Certificate{
 		SerialNumber: new(big.Int).SetInt64(rand.Int63()),
 		Subject: pkix.Name{
-			CommonName: tlsDefaultCommonName,
+			CommonName: commonName,
 		},
-		NotBefore: notBefore,
-		NotAfter:  notAfter,
-
+		NotBefore:             notBefore,
+		NotAfter:              notAfter,
+		SignatureAlgorithm:    x509.ECDSAWithSHA256,
 		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
 		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
 		BasicConstraintsValid: true,
@@ -185,3 +242,79 @@ func pemBlockForKey(priv interface{}) (*pem.Block, error) {
 		return nil, fmt.Errorf("unknown key type")
 	}
 }
+
+// buildCipherSuites returns a list of cipher suites with either AES-GCM or
+// ChaCha20 at the top. This takes advantage of the CPU detection that the
+// TLS package does to create an optimal cipher suite list for the current
+// hardware.
+func buildCipherSuites() []uint16 {
+	pref := preferredCipherSuite()
+	for _, suite := range gcmSuites {
+		if suite == pref {
+			// Go preferred an AES-GCM suite. Use those first.
+			return append(gcmSuites, append(chaChaSuites, otherSuites...)...)
+		}
+	}
+	// Use ChaCha20 at the top, then AES-GCM etc.
+	return append(chaChaSuites, append(gcmSuites, otherSuites...)...)
+}
+
+// preferredCipherSuite returns the cipher suite that is selected for a TLS
+// connection made with the Go defaults to ourselves. This is (currently,
+// probably) either a ChaCha20 suite or an AES-GCM suite, depending on what
+// the CPU detection has decided is fastest on this hardware.
+//
+// The function will return zero if something odd happens, and there's no
+// guarantee what cipher suite would be chosen anyway, so the return value
+// should be taken with a grain of salt.
+func preferredCipherSuite() uint16 {
+	// This is one of our certs from NewCertificate above, to avoid having
+	// to generate one at init time just for this function.
+	crtBs := []byte(`-----BEGIN CERTIFICATE-----
+MIIBXDCCAQOgAwIBAgIIQUODl2/bE4owCgYIKoZIzj0EAwIwFDESMBAGA1UEAxMJ
+c3luY3RoaW5nMB4XDTE4MTAxNDA2MjU0M1oXDTQ5MTIzMTIzNTk1OVowFDESMBAG
+A1UEAxMJc3luY3RoaW5nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEMqP+1lL4
+0s/xtI3ygExzYc/GvLHr0qetpBrUVHaDwS/cR1yXDsYaJpJcUNtrf1XK49IlpWW1
+Ds8seQsSg7/9BaM/MD0wDgYDVR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUF
+BwMBBggrBgEFBQcDAjAMBgNVHRMBAf8EAjAAMAoGCCqGSM49BAMCA0cAMEQCIFxY
+MDBA92FKqZYSZjmfdIbT1OI6S9CnAFvL/pJZJwNuAiAV7osre2NiCHtXABOvsGrH
+vKWqDvXcHr6Tlo+LmTAdyg==
+-----END CERTIFICATE-----
+	`)
+	keyBs := []byte(`-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEIHtPxVHlj6Bhi9RgSR2/lAtIQ7APM9wmpaJAcds6TD2CoAoGCCqGSM49
+AwEHoUQDQgAEMqP+1lL40s/xtI3ygExzYc/GvLHr0qetpBrUVHaDwS/cR1yXDsYa
+JpJcUNtrf1XK49IlpWW1Ds8seQsSg7/9BQ==
+-----END EC PRIVATE KEY-----
+	`)
+
+	cert, err := tls.X509KeyPair(crtBs, keyBs)
+	if err != nil {
+		return 0
+	}
+
+	serverCfg := &tls.Config{
+		MinVersion:               tls.VersionTLS12,
+		PreferServerCipherSuites: true,
+		Certificates:             []tls.Certificate{cert},
+	}
+
+	clientCfg := &tls.Config{
+		MinVersion:         tls.VersionTLS12,
+		InsecureSkipVerify: true,
+	}
+
+	c0, c1 := net.Pipe()
+
+	c := tls.Client(c0, clientCfg)
+	go func() {
+		c.Handshake()
+	}()
+
+	s := tls.Server(c1, serverCfg)
+	if err := s.Handshake(); err != nil {
+		return 0
+	}
+
+	return c.ConnectionState().CipherSuite
+}

lib/tlsutil/tlsutil_test.go

@@ -11,6 +11,7 @@ package tlsutil
 
 import (
 	"bytes"
+	"crypto/tls"
 	"io"
 	"net"
 	"testing"
@@ -74,6 +75,53 @@ func TestUnionedConnection(t *testing.T) {
 	}
 }
 
+func TestCheckCipherSuites(t *testing.T) {
+	// This is the set of cipher suites we expect - only the order should
+	// differ.
+	allSuites := []uint16{
+		tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
+		tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_RSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_RSA_WITH_AES_256_GCM_SHA384,
+		tls.TLS_RSA_WITH_AES_128_CBC_SHA256,
+		tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+	}
+
+	suites := buildCipherSuites()
+
+	if len(suites) != len(allSuites) {
+		t.Fatal("should get a list representing all suites")
+	}
+
+	// Check that the returned list of suites doesn't contain anything
+	// unexpecteds and is free from duplicates.
+	seen := make(map[uint16]struct{})
+nextSuite:
+	for _, s0 := range suites {
+		if _, ok := seen[s0]; ok {
+			t.Fatal("duplicate suite", s0)
+		}
+		for _, s1 := range allSuites {
+			if s0 == s1 {
+				seen[s0] = struct{}{}
+				continue nextSuite
+			}
+		}
+		t.Fatal("got unknown suite", s0)
+	}
+}
+
 type fakeAccepter struct {
 	data []byte
 }