derphttp,magicsock,netcheck: add TLSConfigBypassesTLSDial opt-in flag

Today derphttp.Client.tlsConfig always passes the caller-supplied
TLSConfig through tlsdial.Config, which wraps it with a VerifyConnection
hook that runs system-root verification with a baked-in Let's Encrypt
fallback. tlsdial.Config also panics on base configs that already set
InsecureSkipVerify or VerifyConnection.

That contract works well when DERP is reachable directly over a publicly
trusted PKI, but it breaks for callers who legitimately need to perform
their own server verification — for example when DERP is fronted by a
reverse proxy that presents a non-publicly-trusted certificate, or when
authenticating with an mTLS framework that uses custom CAs / SPIFFE-style
identity / app-name verification (i.e. the standard Go pattern of
InsecureSkipVerify=true paired with a custom VerifyPeerCertificate).

This adds an opt-in TLSConfigBypassesTLSDial bool. When true (and
TLSConfig is non-nil), the supplied config is used as-is after a Clone +
ServerName fallback. tlsdial.Config is bypassed entirely. node-level
InsecureForTests is still honored; node.CertName (a tlsdial-specific
domain-fronting hook) is intentionally ignored on the bypass path —
callers bringing their own verifier are expected to encode any cert
pinning in their own VerifyPeerCertificate / VerifyConnection. The doc
comment explicitly warns that with bypass=true the supplied TLSConfig is
the sole source of server verification.

Plumbing:
  - derphttp.Client.TLSConfigBypassesTLSDial bool.
  - magicsock.Conn stores the (TLSConfig, bypass) pair as a single
    atomic.Pointer[derpTLSPair] so reconnects observe a coherent pair.
    Existing SetDERPTLSConfig(cfg) remains the default path and stores
    (cfg, false); new SetDERPTLSConfigWithBypass(cfg, bypass) stores the
    pair together for callers that need the bypass path.
  - magicsock/derp.go reads the pair atomically and propagates both onto
    the constructed derphttp.Client.
  - netcheck.Client gains DERPTLSConfigBypassesTLSDial alongside the
    existing DERPTLSConfig field, propagated to the netcheck DERP probe
    client so health checks / `coder netcheck` don't panic when
    DERPTLSConfig is a custom-verifier config.

Backward compatible: bypass=false (the default) preserves the existing
behavior and existing callers see no change. Existing SetDERPTLSConfig(cfg)
callers continue to work; their effective bypass state is false.

Test coverage:
  - derp/derphttp/derphttp_test.go: TestTLSConfigBypassesTLSDial covers
    the default path, the bypass path, ServerName behavior, the nil-config
    fallback, and node.InsecureForTests under bypass.
  - wgengine/magicsock/derp_tls_pair_test.go: TestSetDERPTLSConfigPair
    covers the legacy setter, the combined setter, and a -race-friendly
    concurrent writers/readers test that verifies the (cfg, bypass)
    atomic-pair invariant.

Author: ibdafna

derp/derphttp/derphttp_client.go

@@ -53,10 +53,42 @@ import (
 // has been called).
 type Client struct {
 	Header    http.Header
-	TLSConfig *tls.Config        // optional; nil means default
-	DNSCache  *dnscache.Resolver // optional; nil means no caching
-	MeshKey   string             // optional; for trusted clients
-	IsProber  bool               // optional; for probers to optional declare themselves as such
+	TLSConfig *tls.Config // optional; nil means default
+
+	// TLSConfigBypassesTLSDial, if true, causes TLSConfig to be used as-is
+	// (after a Clone and ServerName housekeeping) instead of being passed
+	// through tlsdial.Config. The default behavior wraps TLSConfig in
+	// Tailscale's hosted-DERP verification helper, which installs a
+	// VerifyConnection hook with a baked-in Let's Encrypt fallback and
+	// rejects (panics on) base configs that already set InsecureSkipVerify
+	// or VerifyConnection.
+	//
+	// Set this to true when the caller is responsible for server
+	// verification — e.g. when DERP is fronted by a reverse proxy that
+	// presents a non-publicly-trusted certificate, when using an mTLS
+	// framework that performs its own peer verification (custom CAs,
+	// SPIFFE-style identity), or any case in which the caller has supplied
+	// VerifyPeerCertificate / VerifyConnection on TLSConfig and does not
+	// want the bake-in fallback.
+	//
+	// SECURITY: when this flag is true, the supplied TLSConfig is the
+	// SOLE source of server verification. A TLSConfig with
+	// InsecureSkipVerify=true and no VerifyPeerCertificate /
+	// VerifyConnection callback will result in a TLS handshake that
+	// performs no server identity check at all. Callers MUST either rely
+	// on stock RootCAs + hostname matching (i.e. leave InsecureSkipVerify
+	// false), or provide a VerifyPeerCertificate / VerifyConnection that
+	// implements an equivalent check.
+	//
+	// When true, TLSConfig must be non-nil; the flag is ignored otherwise.
+	// Per-DERPNode overrides are still honored: InsecureForTests still
+	// disables verification (intentionally), but CertName (which is
+	// implemented by tlsdial) is ignored.
+	TLSConfigBypassesTLSDial bool
+
+	DNSCache *dnscache.Resolver // optional; nil means no caching
+	MeshKey  string             // optional; for trusted clients
+	IsProber bool               // optional; for probers to optional declare themselves as such
 
 	// Allow forcing WebSocket fallback for situations where proxies do not
 	// play well with `Upgrade: derp`. Turning this on will cause the client to
@@ -704,14 +736,34 @@ func (c *Client) DialRegion(ctx context.Context, reg *tailcfg.DERPRegion) (net.C
 }
 
 func (c *Client) tlsConfig(node *tailcfg.DERPNode) *tls.Config {
-	tlsConf := tlsdial.Config(c.tlsServerName(node), c.TLSConfig)
-	if node != nil {
-		if node.InsecureForTests {
+	var tlsConf *tls.Config
+	if c.TLSConfigBypassesTLSDial && c.TLSConfig != nil {
+		// Caller has opted to bring their own server verification (custom
+		// CAs / mTLS / SPIFFE-style identity / non-publicly-trusted PKI).
+		// Use the supplied config as-is, only filling in ServerName when
+		// the caller didn't pin one. node.CertName (a tlsdial-specific
+		// domain-fronting hook) is intentionally not applied here; callers
+		// using bypass are expected to encode any cert-pinning in their
+		// own VerifyPeerCertificate / VerifyConnection.
+		tlsConf = c.TLSConfig.Clone()
+		if tlsConf.ServerName == "" {
+			tlsConf.ServerName = c.tlsServerName(node)
+		}
+		if node != nil && node.InsecureForTests {
 			tlsConf.InsecureSkipVerify = true
 			tlsConf.VerifyConnection = nil
+			tlsConf.VerifyPeerCertificate = nil
 		}
-		if node.CertName != "" {
-			tlsdial.SetConfigExpectedCert(tlsConf, node.CertName)
+	} else {
+		tlsConf = tlsdial.Config(c.tlsServerName(node), c.TLSConfig)
+		if node != nil {
+			if node.InsecureForTests {
+				tlsConf.InsecureSkipVerify = true
+				tlsConf.VerifyConnection = nil
+			}
+			if node.CertName != "" {
+				tlsdial.SetConfigExpectedCert(tlsConf, node.CertName)
+			}
 		}
 	}
 	tlsConf.NextProtos = []string{"http/1.1"}

derp/derphttp/derphttp_test.go

@@ -8,6 +8,7 @@ import (
 	"bytes"
 	"context"
 	"crypto/tls"
+	"crypto/x509"
 	"net"
 	"net/http"
 	"net/http/httptest"
@@ -17,6 +18,7 @@ import (
 
 	"github.com/coder/websocket"
 	"tailscale.com/derp"
+	"tailscale.com/tailcfg"
 	"tailscale.com/types/key"
 )
 
@@ -324,3 +326,101 @@ func TestForceWebsockets(t *testing.T) {
 
 	c.Close()
 }
+
+func TestTLSConfigBypassesTLSDial(t *testing.T) {
+	node := &tailcfg.DERPNode{HostName: "derp.example.com"}
+
+	t.Run("default path wraps via tlsdial.Config", func(t *testing.T) {
+		// tlsdial.Config installs its own VerifyConnection. Confirming it
+		// runs is the easiest way to assert we did NOT bypass it. We must
+		// not pass a base config that would cause tlsdial.Config to panic.
+		c := &Client{TLSConfig: &tls.Config{RootCAs: x509.NewCertPool()}}
+		got := c.tlsConfig(node)
+		if got.VerifyConnection == nil {
+			t.Fatal("expected default path to install tlsdial's VerifyConnection")
+		}
+		if !got.InsecureSkipVerify {
+			t.Fatal("expected default path to set InsecureSkipVerify (tlsdial does its own verify)")
+		}
+		if got.ServerName != node.HostName {
+			t.Fatalf("ServerName: got %q, want %q", got.ServerName, node.HostName)
+		}
+	})
+
+	t.Run("bypass path uses caller config as-is", func(t *testing.T) {
+		// A typical "I'm bringing my own verifier" config that would make
+		// tlsdial.Config panic.
+		var verifyCalls int
+		base := &tls.Config{
+			InsecureSkipVerify: true,
+			VerifyPeerCertificate: func(_ [][]byte, _ [][]*x509.Certificate) error {
+				verifyCalls++
+				return nil
+			},
+		}
+		c := &Client{TLSConfig: base, TLSConfigBypassesTLSDial: true}
+		got := c.tlsConfig(node)
+		if got == base {
+			t.Fatal("expected tlsConfig to clone the base config")
+		}
+		if got.VerifyConnection != nil {
+			t.Fatal("bypass path must not install tlsdial's VerifyConnection")
+		}
+		if !got.InsecureSkipVerify {
+			t.Fatal("bypass path must preserve caller's InsecureSkipVerify")
+		}
+		if got.VerifyPeerCertificate == nil {
+			t.Fatal("bypass path must preserve caller's VerifyPeerCertificate")
+		}
+		if got.ServerName != node.HostName {
+			t.Fatalf("ServerName fallback: got %q, want %q", got.ServerName, node.HostName)
+		}
+		if want := []string{"http/1.1"}; len(got.NextProtos) != 1 || got.NextProtos[0] != want[0] {
+			t.Fatalf("NextProtos: got %v, want %v", got.NextProtos, want)
+		}
+	})
+
+	t.Run("bypass path preserves caller-set ServerName", func(t *testing.T) {
+		base := &tls.Config{
+			InsecureSkipVerify: true,
+			VerifyPeerCertificate: func(_ [][]byte, _ [][]*x509.Certificate) error {
+				return nil
+			},
+			ServerName: "explicit.example.com",
+		}
+		c := &Client{TLSConfig: base, TLSConfigBypassesTLSDial: true}
+		got := c.tlsConfig(node)
+		if got.ServerName != "explicit.example.com" {
+			t.Fatalf("ServerName: got %q, want explicit.example.com", got.ServerName)
+		}
+	})
+
+	t.Run("bypass flag without TLSConfig falls back to default path", func(t *testing.T) {
+		c := &Client{TLSConfigBypassesTLSDial: true}
+		got := c.tlsConfig(node)
+		if got.VerifyConnection == nil {
+			t.Fatal("expected fallback to tlsdial path when TLSConfig is nil")
+		}
+	})
+
+	t.Run("bypass path honors node.InsecureForTests", func(t *testing.T) {
+		base := &tls.Config{
+			InsecureSkipVerify: true,
+			VerifyPeerCertificate: func(_ [][]byte, _ [][]*x509.Certificate) error {
+				return nil
+			},
+		}
+		c := &Client{TLSConfig: base, TLSConfigBypassesTLSDial: true}
+		insecureNode := &tailcfg.DERPNode{HostName: "derp.example.com", InsecureForTests: true}
+		got := c.tlsConfig(insecureNode)
+		if !got.InsecureSkipVerify {
+			t.Fatal("expected InsecureForTests to keep InsecureSkipVerify=true")
+		}
+		if got.VerifyPeerCertificate != nil {
+			t.Fatal("expected InsecureForTests to clear VerifyPeerCertificate")
+		}
+		if got.VerifyConnection != nil {
+			t.Fatal("expected InsecureForTests to clear VerifyConnection")
+		}
+	})
+}

net/netcheck/netcheck.go

@@ -211,6 +211,15 @@ type Client struct {
 	// DERPTLSConfig is an optional TLS config for DERP connections.
 	DERPTLSConfig *tls.Config
 
+	// DERPTLSConfigBypassesTLSDial mirrors
+	// derphttp.Client.TLSConfigBypassesTLSDial: when true (and
+	// DERPTLSConfig is non-nil), the netcheck DERP client uses the
+	// supplied TLS config as-is instead of wrapping it via tlsdial.Config.
+	// Use this when DERPTLSConfig performs its own server verification
+	// (e.g. mTLS frameworks with custom CAs / SPIFFE-style identity), as
+	// such configs cause tlsdial.Config to panic.
+	DERPTLSConfigBypassesTLSDial bool
+
 	// For tests
 	testEnoughRegions      int
 	testCaptivePortalDelay time.Duration
@@ -1307,6 +1316,7 @@ func (c *Client) measureHTTPLatency(ctx context.Context, reg *tailcfg.DERPRegion
 	dc.Header = derpHeaders
 	if c.DERPTLSConfig != nil {
 		dc.TLSConfig = c.DERPTLSConfig
+		dc.TLSConfigBypassesTLSDial = c.DERPTLSConfigBypassesTLSDial
 	}
 	defer dc.Close()
 

wgengine/magicsock/derp.go

@@ -347,8 +347,14 @@ func (c *Conn) derpWriteChanOfAddr(addr netip.AddrPort, peer key.NodePublic) cha
 	dc.SetAddressFamilySelector(derpAddrFamSelector{c})
 	dc.SetForcedWebsocketCallback(c.derpForcedWebsocketFunc)
 	dc.DNSCache = dnscache.Get()
-	if tlsCfg := c.derpTLSConfig.Load(); tlsCfg != nil {
-		dc.TLSConfig = tlsCfg
+	// Read the (TLS config, bypass-tlsdial) pair atomically so we never
+	// observe a new custom-verifier TLS config paired with a stale
+	// bypass=false (which would panic in tlsdial.Config) nor a new
+	// publicly-trusted config paired with a stale bypass=true (which would
+	// silently skip server verification). See Conn.derpTLSConfig.
+	if pair := c.derpTLSConfig.Load(); pair != nil && pair.cfg != nil {
+		dc.TLSConfig = pair.cfg
+		dc.TLSConfigBypassesTLSDial = pair.bypassTLSDial
 	}
 	header := c.derpHeader.Load()
 	if header != nil {

wgengine/magicsock/derp_tls_pair_test.go

@@ -0,0 +1,107 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package magicsock
+
+import (
+	"crypto/tls"
+	"sync"
+	"testing"
+)
+
+// loadDERPTLSPair reads the (cfg, bypass) pair the way magicsock/derp.go
+// reads it on every reconnect. Used by tests to assert that paired writers
+// never expose an inconsistent (cfg, bypass) pairing to readers.
+func (c *Conn) loadDERPTLSPair() (cfg *tls.Config, bypass bool) {
+	if p := c.derpTLSConfig.Load(); p != nil {
+		return p.cfg, p.bypassTLSDial
+	}
+	return nil, false
+}
+
+func TestSetDERPTLSConfigPair(t *testing.T) {
+	t.Run("legacy SetDERPTLSConfig: bypass false, cfg set", func(t *testing.T) {
+		c := &Conn{}
+		want := &tls.Config{}
+		c.SetDERPTLSConfig(want)
+		got, bypass := c.loadDERPTLSPair()
+		if got != want {
+			t.Fatalf("cfg: got %p, want %p", got, want)
+		}
+		if bypass {
+			t.Fatal("bypass should default to false for SetDERPTLSConfig")
+		}
+	})
+
+	t.Run("SetDERPTLSConfigWithBypass updates pair atomically", func(t *testing.T) {
+		c := &Conn{}
+		want := &tls.Config{}
+		c.SetDERPTLSConfigWithBypass(want, true)
+		got, bypass := c.loadDERPTLSPair()
+		if got != want {
+			t.Fatalf("cfg: got %p, want %p", got, want)
+		}
+		if !bypass {
+			t.Fatal("bypass should be true")
+		}
+
+		// Switch to a different cfg with bypass=false in one call.
+		other := &tls.Config{}
+		c.SetDERPTLSConfigWithBypass(other, false)
+		got, bypass = c.loadDERPTLSPair()
+		if got != other {
+			t.Fatalf("cfg after second set: got %p, want %p", got, other)
+		}
+		if bypass {
+			t.Fatal("bypass should be false after second set")
+		}
+	})
+
+	t.Run("readers never observe inconsistent pairing under concurrent writers", func(t *testing.T) {
+		// Two writers race: one keeps flipping between (publicCfg, false)
+		// and (metatronCfg, true). A reader observes the pair atomically
+		// on every iteration. The invariant: any time we observe
+		// metatronCfg we must observe bypass=true; any time we observe
+		// publicCfg we must observe bypass=false. If the (cfg, bypass)
+		// pair were stored as two independent atomics, this test would
+		// fail with -race because we'd occasionally observe a
+		// half-updated pairing.
+		c := &Conn{}
+		publicCfg := &tls.Config{}
+		metatronCfg := &tls.Config{InsecureSkipVerify: true}
+		c.SetDERPTLSConfigWithBypass(publicCfg, false)
+
+		const iterations = 5_000
+		var wg sync.WaitGroup
+		wg.Add(2)
+		go func() {
+			defer wg.Done()
+			for i := 0; i < iterations; i++ {
+				if i%2 == 0 {
+					c.SetDERPTLSConfigWithBypass(metatronCfg, true)
+				} else {
+					c.SetDERPTLSConfigWithBypass(publicCfg, false)
+				}
+			}
+		}()
+		go func() {
+			defer wg.Done()
+			for i := 0; i < iterations; i++ {
+				cfg, bypass := c.loadDERPTLSPair()
+				switch cfg {
+				case publicCfg:
+					if bypass {
+						t.Errorf("invariant violated: publicCfg paired with bypass=true (iter %d)", i)
+						return
+					}
+				case metatronCfg:
+					if !bypass {
+						t.Errorf("invariant violated: metatronCfg paired with bypass=false (iter %d)", i)
+						return
+					}
+				}
+			}
+		}()
+		wg.Wait()
+	})
+}