From 67ede8d6d284a3c5de3e70b045b5534d864ce220 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Fri, 28 Feb 2020 11:13:28 -0800
Subject: [PATCH] wgengine, magicsock: fix SetPrivateKey data race

Updates #112

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 wgengine/magicsock/magicsock.go | 57 ++++++++++++++++++++++++++-------
 wgengine/userspace.go           | 12 ++++---
 2 files changed, 54 insertions(+), 15 deletions(-)

diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go
index aa5d938d6..9596b9371 100644
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@@ -36,7 +36,6 @@ import (
 type Conn struct {
 	pconn         *RebindingUDPConn
 	pconnPort     uint16
-	privateKey    key.Private
 	stunServers   []string
 	startEpUpdate chan struct{} // send to trigger endpoint update
 	epFunc        func(endpoints []string)
@@ -68,7 +67,9 @@ type Conn struct {
 	derpRecvCh chan derpReadResult
 
 	derpMu      sync.Mutex
-	derpConn    map[int]*derphttp.Client // magic derp port (see derpmap.go) to its client
+	privateKey  key.Private
+	derpConn    map[int]*derphttp.Client   // magic derp port (see derpmap.go) to its client
+	derpCancel  map[int]context.CancelFunc // to close derp goroutines
 	derpWriteCh map[int]chan<- derpWriteRequest
 }
 
@@ -525,25 +526,33 @@ func (c *Conn) derpWriteChanOfAddr(addr *net.UDPAddr) chan<- derpWriteRequest {
 	}
 	c.derpMu.Lock()
 	defer c.derpMu.Unlock()
+	if c.privateKey.IsZero() {
+		c.logf("DERP lookup of %v with no private key; ignoring", addr.IP)
+		return nil
+	}
 	ch, ok := c.derpWriteCh[addr.Port]
 	if !ok {
 		if c.derpWriteCh == nil {
 			c.derpWriteCh = make(map[int]chan<- derpWriteRequest)
 			c.derpConn = make(map[int]*derphttp.Client)
+			c.derpCancel = make(map[int]context.CancelFunc)
 		}
 		host := derpHost(addr.Port)
 		dc, err := derphttp.NewClient(c.privateKey, "https://"+host+"/derp", log.Printf)
 		if err != nil {
-			log.Printf("derphttp.NewClient: port %d, host %q invalid? err: %v", addr.Port, host, err)
+			c.logf("derphttp.NewClient: port %d, host %q invalid? err: %v", addr.Port, host, err)
 			return nil
 		}
 
+		ctx, cancel := context.WithCancel(context.Background())
+
 		bidiCh := make(chan derpWriteRequest, bufferedDerpWritesBeforeDrop)
 		ch = bidiCh
 		c.derpConn[addr.Port] = dc
 		c.derpWriteCh[addr.Port] = ch
-		go c.runDerpReader(addr, dc)
-		go c.runDerpWriter(addr, dc, bidiCh)
+		c.derpCancel[addr.Port] = cancel
+		go c.runDerpReader(ctx, addr, dc)
+		go c.runDerpWriter(ctx, addr, dc, bidiCh)
 	}
 	return ch
 }
@@ -564,7 +573,7 @@ var logDerpVerbose, _ = strconv.ParseBool(os.Getenv("DEBUG_DERP_VERBOSE"))
 
 // runDerpReader runs in a goroutine for the life of a DERP
 // connection, handling received packets.
-func (c *Conn) runDerpReader(derpFakeAddr *net.UDPAddr, dc *derphttp.Client) {
+func (c *Conn) runDerpReader(ctx context.Context, derpFakeAddr *net.UDPAddr, dc *derphttp.Client) {
 	didCopy := make(chan struct{}, 1)
 	var buf [derp.MaxPacketSize]byte
 	var bufValid int // bytes in buf that are valid
@@ -576,13 +585,15 @@ func (c *Conn) runDerpReader(derpFakeAddr *net.UDPAddr, dc *derphttp.Client) {
 
 	for {
 		msg, err := dc.Recv(buf[:])
+		if err == derphttp.ErrClientClosed {
+			return
+		}
 		if err != nil {
-			if err == derphttp.ErrClientClosed {
-				return
-			}
 			select {
 			case <-c.donec:
 				return
+			case <-ctx.Done():
+				return
 			default:
 			}
 			log.Printf("derp.Recv: %v", err)
@@ -618,9 +629,11 @@ type derpWriteRequest struct {
 
 // runDerpWriter runs in a goroutine for the life of a DERP
 // connection, handling received packets.
-func (c *Conn) runDerpWriter(derpFakeAddr *net.UDPAddr, dc *derphttp.Client, ch <-chan derpWriteRequest) {
+func (c *Conn) runDerpWriter(ctx context.Context, derpFakeAddr *net.UDPAddr, dc *derphttp.Client, ch <-chan derpWriteRequest) {
 	for {
 		select {
+		case <-ctx.Done():
+			return
 		case <-c.donec:
 			return
 		case wr := <-ch:
@@ -740,7 +753,29 @@ func (c *Conn) ReceiveIPv6(buff []byte) (int, conn.Endpoint, *net.UDPAddr, error
 }
 
 func (c *Conn) SetPrivateKey(privateKey wgcfg.PrivateKey) error {
-	c.privateKey = key.Private(privateKey)
+	c.derpMu.Lock()
+	defer c.derpMu.Unlock()
+
+	oldKey, newKey := c.privateKey, key.Private(privateKey)
+	if newKey == oldKey {
+		return nil
+	}
+	c.privateKey = newKey
+	if oldKey.IsZero() {
+		// Initial configuration on start.
+		return nil
+	}
+
+	// Key changed. Close any DERP connections.
+	for _, c := range c.derpConn {
+		go c.Close()
+	}
+	for _, cancel := range c.derpCancel {
+		cancel()
+	}
+	c.derpConn = nil
+	c.derpCancel = nil
+	c.derpWriteCh = nil
 	return nil
 }
 
diff --git a/wgengine/userspace.go b/wgengine/userspace.go
index 1504873dc..c9c4f2838 100644
--- a/wgengine/userspace.go
+++ b/wgengine/userspace.go
@@ -325,15 +325,19 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string) error
 	e.lastReconfig = rc
 	e.lastCfg = cfg.Copy()
 
+	// Tell magicsock about the new (or initial) private key
+	// (which is needed by DERP) before wgdev gets it, as wgdev
+	// will start trying to handshake, which we want to be able to
+	// go over DERP.
+	if err := e.magicConn.SetPrivateKey(cfg.PrivateKey); err != nil {
+		e.logf("magicsock: %v\n", err)
+	}
+
 	if err := e.wgdev.Reconfig(cfg); err != nil {
 		e.logf("wgdev.Reconfig: %v\n", err)
 		return err
 	}
 
-	if err := e.magicConn.SetPrivateKey(cfg.PrivateKey); err != nil {
-		e.logf("magicsock: %v\n", err)
-	}
-
 	// TODO(apenwarr): only handling the first local address.
 	//   Currently we never use more than one anyway.
 	var cidr wgcfg.CIDR