Fix data race between key rotation and registry signer

pkg/daemon/daemon.go

@@ -934,9 +934,15 @@ func (d *Daemon) Start() error {
 	// "registry: signature verification failed" on every heartbeat.
 	if d.identity != nil {
 		rc.SetSigner(func(challenge string) string {
+			// Hold identityMu.RLock across Sign(): RotateKey zeros the old
+			// PrivateKey buffer in place under identityMu.Lock(). Releasing
+			// the lock before Sign() would let an in-flight signer read the
+			// very bytes RotateKey is concurrently zeroing (use-after-zero
+			// on signing material). RLock and Lock are mutually exclusive,
+			// so signing and zeroing can never overlap.
 			d.identityMu.RLock()
+			defer d.identityMu.RUnlock()
 			cur := d.identity
-			d.identityMu.RUnlock()
 			if cur == nil {
 				return ""
 			}
@@ -2311,27 +2317,33 @@ func (d *Daemon) RotateKey() (map[string]interface{}, error) {
 		return nil, fmt.Errorf("rotate_key: registry: %w", err)
 	}
 
+	// Swap the identity AND zero the old private key under the SAME write
+	// Lock. Signer closures read the private key under identityMu.RLock()
+	// for the full duration of Sign(); RLock and Lock are mutually
+	// exclusive, so zeroing here can never overlap an in-flight Sign() of
+	// the old key. Doing the zero outside the Lock (the old behavior) raced
+	// with a signer that had captured the old identity before the swap.
+	//
+	// We zero the old key so it doesn't linger on the heap until GC — a
+	// long-lived daemon can keep it alive for hours. ed25519.PrivateKey is
+	// a []byte (seed || public).
 	d.identityMu.Lock()
 	d.identity = newID
-	d.identityMu.Unlock()
-
-	// Zero the old private key so it doesn't linger on the heap
-	// until GC — a long-lived daemon can keep it alive for hours.
-	// ed25519.PrivateKey is a []byte (seed || public).
 	for i := range current.PrivateKey {
 		current.PrivateKey[i] = 0
 	}
+	d.identityMu.Unlock()
 
 	d.tunnels.SetIdentity(newID)
 	// The signer installed in Start() reads d.identity under d.identityMu
 	// on every call, so this SetSigner re-bind is no longer load-bearing —
 	// kept for symmetry with the original RotateKey contract. The closure
-	// here also goes through the mutex to stay consistent if d.identity
-	// ever rotates again.
+	// here holds the RLock across Sign() (same invariant as Start's signer)
+	// so it is safe against a future rotation's in-place key zeroing.
 	d.regConn.SetSigner(func(c string) string {
 		d.identityMu.RLock()
+		defer d.identityMu.RUnlock()
 		cur := d.identity
-		d.identityMu.RUnlock()
 		if cur == nil {
 			return ""
 		}

pkg/daemon/zz_rotate_key_sign_race_test.go

@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package daemon
+
+import (
+	"crypto/ed25519"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+)
+
+// TestConcurrentRotateKeyAndSign is a regression test for the rotate-key /
+// signer data race: RotateKey swaps d.identity and then zeros the OLD
+// PrivateKey buffer in place, while the registry signer closure reads the
+// private key to produce an Ed25519 signature. If the signer releases
+// identityMu before calling Sign(), an in-flight signer that captured the
+// old identity reads the very bytes RotateKey is concurrently zeroing
+// (use-after-zero on signing material). Under -race this manifests as a
+// WRITE/READ race on the PrivateKey slice.
+//
+// The fix holds identityMu.RLock() across the whole Sign() in every signer
+// closure, and performs the old-key zeroing under identityMu.Lock(). RLock
+// and Lock are mutually exclusive, so signing and zeroing can never overlap.
+//
+// This test spins N goroutines hammering the production-shaped signer closure
+// while another goroutine repeatedly rotates the identity. It asserts every
+// signature is valid for either the old or the new public key (never a
+// corrupt/partially-zeroed key), and -race must report no data races.
+func TestConcurrentRotateKeyAndSign(t *testing.T) {
+	t.Parallel()
+
+	reg, rc := startTestRegistry(t)
+	t.Cleanup(func() { reg.Close() })
+	t.Cleanup(func() { rc.Close() })
+
+	d := New(Config{})
+	d.regConn = rc
+	registerSelfOnRegistry(t, d)
+
+	// The exact signer closure the daemon installs in Start() / RotateKey:
+	// hold identityMu.RLock() across Sign() so the read can never overlap
+	// RotateKey's in-place zeroing of the old key (done under Lock()).
+	sign := func(challenge string) (sig []byte, pub ed25519.PublicKey) {
+		d.identityMu.RLock()
+		defer d.identityMu.RUnlock()
+		cur := d.identity
+		if cur == nil {
+			return nil, nil
+		}
+		// Snapshot the public key under the same lock so the verify target
+		// matches the key that produced the signature.
+		pub = append(ed25519.PublicKey(nil), cur.PublicKey...)
+		return cur.Sign([]byte(challenge)), pub
+	}
+
+	const (
+		signers     = 8
+		rotations   = 12
+		challengeFm = "rotate:race:challenge"
+	)
+
+	var (
+		wg          sync.WaitGroup
+		stop        atomic.Bool
+		corruptSigs atomic.Int64
+		signCount   atomic.Int64
+	)
+
+	// Signer goroutines: hammer the signer closure and verify each signature
+	// against the public key snapshotted under the SAME RLock that produced
+	// it. A correct signer returns a (sig, pub) pair where pub verifies sig.
+	// A use-after-zero — the signer reading PrivateKey bytes that RotateKey
+	// is concurrently zeroing — corrupts the signature so ed25519.Verify
+	// fails (and trips the race detector). Snapshotting the pubkey under the
+	// same lock means the verify target always matches the signing key, so a
+	// verify failure can only come from torn signing material, not from a
+	// benign mid-rotation key change.
+	for i := 0; i < signers; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for !stop.Load() {
+				sig, pub := sign(challengeFm)
+				if sig == nil {
+					continue
+				}
+				signCount.Add(1)
+				if !ed25519.Verify(pub, []byte(challengeFm), sig) {
+					corruptSigs.Add(1)
+				}
+			}
+		}()
+	}
+
+	// Rotation goroutine: the real production write+zero path. Each rotation
+	// generates a fresh keypair, swaps it in under Lock, and zeros the old
+	// PrivateKey buffer under Lock.
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		for i := 0; i < rotations; i++ {
+			if _, err := d.RotateKey(); err != nil {
+				t.Errorf("RotateKey: %v", err)
+				break
+			}
+		}
+		stop.Store(true)
+	}()
+
+	// Safety valve so a wedged rotation can't hang the suite.
+	done := make(chan struct{})
+	go func() { wg.Wait(); close(done) }()
+	select {
+	case <-done:
+	case <-time.After(30 * time.Second):
+		stop.Store(true)
+		t.Fatal("concurrent rotate/sign did not finish within 30s")
+	}
+
+	if c := corruptSigs.Load(); c != 0 {
+		t.Fatalf("got %d corrupt signatures (use-after-zero on signing key)", c)
+	}
+	if signCount.Load() == 0 {
+		t.Fatal("no signatures were produced; test did not exercise the signer")
+	}
+}