From 8009ad74a3b1e0c532a21375266f42336cd22aa3 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 19 May 2025 08:39:55 -0700 Subject: [PATCH] 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 --- cmd/derper/cert_test.go | 1 + cmd/derper/depaware.txt | 1 + cmd/k8s-operator/depaware.txt | 1 + cmd/tailscale/depaware.txt | 1 + cmd/tailscaled/depaware.txt | 1 + derp/derp_server.go | 23 ++++++++++++- derp/derp_test.go | 3 +- derp/derpconst/derpconst.go | 11 ++++++ derp/derphttp/derphttp_client.go | 3 +- net/tlsdial/tlsdial.go | 58 +++++++++++++++++++------------- tsnet/depaware.txt | 1 + 11 files changed, 77 insertions(+), 27 deletions(-) create mode 100644 derp/derpconst/derpconst.go diff --git a/cmd/derper/cert_test.go b/cmd/derper/cert_test.go index 2ec7b756e..31fd4ea44 100644 --- a/cmd/derper/cert_test.go +++ b/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 diff --git a/cmd/derper/depaware.txt b/cmd/derper/depaware.txt index f22b4873f..ca7723530 100644 --- a/cmd/derper/depaware.txt +++ b/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+ diff --git a/cmd/k8s-operator/depaware.txt b/cmd/k8s-operator/depaware.txt index bbbaebc19..12fb5cf2e 100644 --- a/cmd/k8s-operator/depaware.txt +++ b/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 diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 85bf64e4a..03bf2f94c 100644 --- a/cmd/tailscale/depaware.txt +++ b/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+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 823d639c9..6de0ddc39 100644 --- a/cmd/tailscaled/depaware.txt +++ b/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 diff --git a/derp/derp_server.go b/derp/derp_server.go index c330572d2..abda9da73 100644 --- a/derp/derp_server.go +++ b/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 diff --git a/derp/derp_test.go b/derp/derp_test.go index f0fc52fe7..c5a92bafa 100644 --- a/derp/derp_test.go +++ b/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 { diff --git a/derp/derpconst/derpconst.go b/derp/derpconst/derpconst.go new file mode 100644 index 000000000..74ca09ccb --- /dev/null +++ b/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" diff --git a/derp/derphttp/derphttp_client.go b/derp/derphttp/derphttp_client.go index 21ee4a671..faa218ca2 100644 --- a/derp/derphttp/derphttp_client.go +++ b/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 diff --git a/net/tlsdial/tlsdial.go b/net/tlsdial/tlsdial.go index 4d22383ef..1bd2450aa 100644 --- a/net/tlsdial/tlsdial.go +++ b/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 -// to require that exactly 1 cert is presented, and that the hex of its SHA256 hash -// is equal to wantFullCertSHA256Hex and that it's a valid cert for c.ServerName. +// SetConfigExpectedCertHash configures c's VerifyPeerCertificate function to +// require that exactly 1 cert is presented (not counting any present MetaCert), +// 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 { - return errors.New("no certs presented") + var sawGoodCert bool + 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 { - return errors.New("unexpected multiple certs 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) + if !sawGoodCert { + return errors.New("expected cert not presented") } return nil } diff --git a/tsnet/depaware.txt b/tsnet/depaware.txt index f5cd1232d..662752554 100644 --- a/tsnet/depaware.txt +++ b/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