cmd/derper, net/tlsdial: fix client's self-signed cert validation

This fixes the implementation and test from #15208 which apparently
never worked.

Ignore the metacert when counting the number of expected certs
presented.

And fix the test, pulling out the TLSConfig setup code into something
shared between the real cmd/derper and the test.

Fixes #15579

Change-Id: I90526e38e59f89b480629b415f00587b107de10a
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/derper/cert_test.go

@@ -140,6 +140,7 @@ func TestPinnedCertRawIP(t *testing.T) {
 	var hs http.Server
 	hs.Handler = mux
 	hs.TLSConfig = cp.TLSConfig()
+	ds.ModifyTLSConfigToAddMetaCert(hs.TLSConfig)
 	go hs.ServeTLS(ln, "", "")
 
 	lnPort := ln.Addr().(*net.TCPAddr).Port

cmd/derper/depaware.txt

@@ -92,6 +92,7 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa
         tailscale.com/client/tailscale                               from tailscale.com/derp
         tailscale.com/client/tailscale/apitype                       from tailscale.com/client/tailscale+
         tailscale.com/derp                                           from tailscale.com/cmd/derper+
+        tailscale.com/derp/derpconst                                 from tailscale.com/derp+
         tailscale.com/derp/derphttp                                  from tailscale.com/cmd/derper
         tailscale.com/disco                                          from tailscale.com/derp
         tailscale.com/drive                                          from tailscale.com/client/local+

cmd/k8s-operator/depaware.txt

@@ -786,6 +786,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/
         tailscale.com/control/controlhttp/controlhttpcommon          from tailscale.com/control/controlhttp
         tailscale.com/control/controlknobs                           from tailscale.com/control/controlclient+
         tailscale.com/derp                                           from tailscale.com/derp/derphttp+
+        tailscale.com/derp/derpconst                                 from tailscale.com/derp+
         tailscale.com/derp/derphttp                                  from tailscale.com/ipn/localapi+
         tailscale.com/disco                                          from tailscale.com/derp+
         tailscale.com/doctor                                         from tailscale.com/ipn/ipnlocal

cmd/tailscale/depaware.txt

@@ -88,6 +88,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         tailscale.com/control/controlhttp/controlhttpcommon          from tailscale.com/control/controlhttp
         tailscale.com/control/controlknobs                           from tailscale.com/net/portmapper
         tailscale.com/derp                                           from tailscale.com/derp/derphttp
+        tailscale.com/derp/derpconst                                 from tailscale.com/derp+
         tailscale.com/derp/derphttp                                  from tailscale.com/net/netcheck
         tailscale.com/disco                                          from tailscale.com/derp
         tailscale.com/drive                                          from tailscale.com/client/local+

cmd/tailscaled/depaware.txt

@@ -260,6 +260,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         tailscale.com/control/controlhttp/controlhttpcommon          from tailscale.com/control/controlhttp
         tailscale.com/control/controlknobs                           from tailscale.com/control/controlclient+
         tailscale.com/derp                                           from tailscale.com/derp/derphttp+
+        tailscale.com/derp/derpconst                                 from tailscale.com/derp+
         tailscale.com/derp/derphttp                                  from tailscale.com/cmd/tailscaled+
         tailscale.com/disco                                          from tailscale.com/derp+
         tailscale.com/doctor                                         from tailscale.com/ipn/ipnlocal

derp/derp_server.go

@@ -11,6 +11,7 @@ import (
 	"context"
 	"crypto/ed25519"
 	crand "crypto/rand"
+	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/binary"
@@ -38,6 +39,7 @@ import (
 	"golang.org/x/sync/errgroup"
 	"tailscale.com/client/local"
 	"tailscale.com/client/tailscale"
+	"tailscale.com/derp/derpconst"
 	"tailscale.com/disco"
 	"tailscale.com/envknob"
 	"tailscale.com/metrics"
@@ -616,7 +618,7 @@ func (s *Server) initMetacert() {
 	tmpl := &x509.Certificate{
 		SerialNumber: big.NewInt(ProtocolVersion),
 		Subject: pkix.Name{
-			CommonName: fmt.Sprintf("derpkey%s", s.publicKey.UntypedHexString()),
+			CommonName: derpconst.MetaCertCommonNamePrefix + s.publicKey.UntypedHexString(),
 		},
 		// Windows requires NotAfter and NotBefore set:
 		NotAfter:  s.clock.Now().Add(30 * 24 * time.Hour),
@@ -636,6 +638,25 @@ func (s *Server) initMetacert() {
 // TLS server to let the client skip a round trip during start-up.
 func (s *Server) MetaCert() []byte { return s.metaCert }
 
+// ModifyTLSConfigToAddMetaCert modifies c.GetCertificate to make
+// it append s.MetaCert to the returned certificates.
+//
+// It panics if c or c.GetCertificate is nil.
+func (s *Server) ModifyTLSConfigToAddMetaCert(c *tls.Config) {
+	getCert := c.GetCertificate
+	if getCert == nil {
+		panic("c.GetCertificate is nil")
+	}
+	c.GetCertificate = func(hi *tls.ClientHelloInfo) (*tls.Certificate, error) {
+		cert, err := getCert(hi)
+		if err != nil {
+			return nil, err
+		}
+		cert.Certificate = append(cert.Certificate, s.MetaCert())
+		return cert, nil
+	}
+}
+
 // registerClient notes that client c is now authenticated and ready for packets.
 //
 // If c.key is connected more than once, the earlier connection(s) are

derp/derp_test.go

@@ -27,6 +27,7 @@ import (
 	qt "github.com/frankban/quicktest"
 	"go4.org/mem"
 	"golang.org/x/time/rate"
+	"tailscale.com/derp/derpconst"
 	"tailscale.com/disco"
 	"tailscale.com/net/memnet"
 	"tailscale.com/tstest"
@@ -930,7 +931,7 @@ func TestMetaCert(t *testing.T) {
 	if fmt.Sprint(cert.SerialNumber) != fmt.Sprint(ProtocolVersion) {
 		t.Errorf("serial = %v; want %v", cert.SerialNumber, ProtocolVersion)
 	}
-	if g, w := cert.Subject.CommonName, fmt.Sprintf("derpkey%s", pub.UntypedHexString()); g != w {
+	if g, w := cert.Subject.CommonName, derpconst.MetaCertCommonNamePrefix+pub.UntypedHexString(); g != w {
 		t.Errorf("CommonName = %q; want %q", g, w)
 	}
 	if n := len(cert.Extensions); n != 1 {

derp/derpconst/derpconst.go

@@ -0,0 +1,11 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+// Package derpconst contains constants used by the DERP client and server.
+package derpconst
+
+// MetaCertCommonNamePrefix is the prefix that the DERP server
+// puts on for the common name of its "metacert". The suffix of
+// the common name after "derpkey" is the hex key.NodePublic
+// of the DERP server.
+const MetaCertCommonNamePrefix = "derpkey"

derp/derphttp/derphttp_client.go

@@ -30,6 +30,7 @@ import (
 
 	"go4.org/mem"
 	"tailscale.com/derp"
+	"tailscale.com/derp/derpconst"
 	"tailscale.com/envknob"
 	"tailscale.com/health"
 	"tailscale.com/net/dnscache"
@@ -1152,7 +1153,7 @@ var ErrClientClosed = errors.New("derphttp.Client closed")
 func parseMetaCert(certs []*x509.Certificate) (serverPub key.NodePublic, serverProtoVersion int) {
 	for _, cert := range certs {
 		// Look for derpkey prefix added by initMetacert() on the server side.
-		if pubHex, ok := strings.CutPrefix(cert.Subject.CommonName, "derpkey"); ok {
+		if pubHex, ok := strings.CutPrefix(cert.Subject.CommonName, derpconst.MetaCertCommonNamePrefix); ok {
 			var err error
 			serverPub, err = key.ParseNodePublicUntyped(mem.S(pubHex))
 			if err == nil && cert.SerialNumber.BitLen() <= 8 { // supports up to version 255

net/tlsdial/tlsdial.go

@@ -21,10 +21,12 @@ import (
 	"net"
 	"net/http"
 	"os"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"time"
 
+	"tailscale.com/derp/derpconst"
 	"tailscale.com/envknob"
 	"tailscale.com/health"
 	"tailscale.com/hostinfo"
@@ -247,9 +249,10 @@ func SetConfigExpectedCert(c *tls.Config, certDNSName string) {
 	}
 }
 
-// SetConfigExpectedCertHash configures c's VerifyPeerCertificate function
+// SetConfigExpectedCertHash configures c's VerifyPeerCertificate function to
-// to require that exactly 1 cert is presented, and that the hex of its SHA256 hash
+// require that exactly 1 cert is presented (not counting any present MetaCert),
-// is equal to wantFullCertSHA256Hex and that it's a valid cert for c.ServerName.
+// and that the hex of its SHA256 hash is equal to wantFullCertSHA256Hex and
+// that it's a valid cert for c.ServerName.
 func SetConfigExpectedCertHash(c *tls.Config, wantFullCertSHA256Hex string) {
 	if c.VerifyPeerCertificate != nil {
 		panic("refusing to override tls.Config.VerifyPeerCertificate")
@@ -260,28 +263,35 @@ func SetConfigExpectedCertHash(c *tls.Config, wantFullCertSHA256Hex string) {
 	c.InsecureSkipVerify = true
 	c.VerifyConnection = nil
 	c.VerifyPeerCertificate = func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
-		if len(rawCerts) == 0 {
+		var sawGoodCert bool
-			return errors.New("no certs presented")
+		for _, rawCert := range rawCerts {
+			cert, err := x509.ParseCertificate(rawCert)
+			if err != nil {
+				return fmt.Errorf("ParseCertificate: %w", err)
+			}
+			if strings.HasPrefix(cert.Subject.CommonName, derpconst.MetaCertCommonNamePrefix) {
+				continue
+			}
+			if sawGoodCert {
+				return errors.New("unexpected multiple certs presented")
+			}
+			if fmt.Sprintf("%02x", sha256.Sum256(rawCert)) != wantFullCertSHA256Hex {
+				return fmt.Errorf("cert hash does not match expected cert hash")
+			}
+			if err := cert.VerifyHostname(c.ServerName); err != nil {
+				return fmt.Errorf("cert does not match server name %q: %w", c.ServerName, err)
+			}
+			now := time.Now()
+			if now.After(cert.NotAfter) {
+				return fmt.Errorf("cert expired %v", cert.NotAfter)
+			}
+			if now.Before(cert.NotBefore) {
+				return fmt.Errorf("cert not yet valid until %v; is your clock correct?", cert.NotBefore)
+			}
+			sawGoodCert = true
 		}
-		if len(rawCerts) > 1 {
+		if !sawGoodCert {
-			return errors.New("unexpected multiple certs presented")
+			return errors.New("expected cert not presented")
-		}
-		if fmt.Sprintf("%02x", sha256.Sum256(rawCerts[0])) != wantFullCertSHA256Hex {
-			return fmt.Errorf("cert hash does not match expected cert hash")
-		}
-		cert, err := x509.ParseCertificate(rawCerts[0])
-		if err != nil {
-			return fmt.Errorf("ParseCertificate: %w", err)
-		}
-		if err := cert.VerifyHostname(c.ServerName); err != nil {
-			return fmt.Errorf("cert does not match server name %q: %w", c.ServerName, err)
-		}
-		now := time.Now()
-		if now.After(cert.NotAfter) {
-			return fmt.Errorf("cert expired %v", cert.NotAfter)
-		}
-		if now.Before(cert.NotBefore) {
-			return fmt.Errorf("cert not yet valid until %v; is your clock correct?", cert.NotBefore)
 		}
 		return nil
 	}

tsnet/depaware.txt

@@ -227,6 +227,7 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware)
         tailscale.com/control/controlhttp/controlhttpcommon          from tailscale.com/control/controlhttp
         tailscale.com/control/controlknobs                           from tailscale.com/control/controlclient+
         tailscale.com/derp                                           from tailscale.com/derp/derphttp+
+        tailscale.com/derp/derpconst                                 from tailscale.com/derp+
         tailscale.com/derp/derphttp                                  from tailscale.com/ipn/localapi+
         tailscale.com/disco                                          from tailscale.com/derp+
         tailscale.com/doctor                                         from tailscale.com/ipn/ipnlocal