ipn/ipnlocal: make EditPrefs strip private keys before returning

Signed-off-by: Maisem Ali <maisem@tailscale.com>
2 years ago · 9f39c3b10f
parent a2d15924fb
commit 9f39c3b10f
3 changed files with 88 additions and 23 deletions
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@ -492,17 +492,23 @@ func (b *LocalBackend) Shutdown() {
 	b.e.Wait()
 }

+func stripKeysFromPrefs(p ipn.PrefsView) ipn.PrefsView {
+	if !p.Valid() || p.Persist() == nil {
+		return p
+	}
+
+	p2 := p.AsStruct()
+	p2.Persist.LegacyFrontendPrivateMachineKey = key.MachinePrivate{}
+	p2.Persist.PrivateNodeKey = key.NodePrivate{}
+	p2.Persist.OldPrivateNodeKey = key.NodePrivate{}
+	return p2.View()
+}
+
 // Prefs returns a copy of b's current prefs, with any private keys removed.
-func (b *LocalBackend) Prefs() *ipn.Prefs {
+func (b *LocalBackend) Prefs() ipn.PrefsView {
 	b.mu.Lock()
 	defer b.mu.Unlock()
-	p := b.prefs.AsStruct()
-	if p != nil && p.Persist != nil {
-		p.Persist.LegacyFrontendPrivateMachineKey = key.MachinePrivate{}
-		p.Persist.PrivateNodeKey = key.NodePrivate{}
-		p.Persist.OldPrivateNodeKey = key.NodePrivate{}
-	}
-	return p
+	return stripKeysFromPrefs(b.prefs)
 }

 // Status returns the latest status of the backend and its
@ -2170,15 +2176,17 @@ func (b *LocalBackend) EditPrefs(mp *ipn.MaskedPrefs) (ipn.PrefsView, error) {
 	}
 	if p1.View().Equals(p0) {
 		b.mu.Unlock()
-		return p1.View(), nil
+		return stripKeysFromPrefs(p0), nil
 	}
 	b.logf("EditPrefs: %v", mp.Pretty())
-	b.setPrefsLockedOnEntry("EditPrefs", p1) // does a b.mu.Unlock
+	newPrefs := b.setPrefsLockedOnEntry("EditPrefs", p1) // does a b.mu.Unlock

 	// Note: don't perform any actions for the new prefs here. Not
 	// every prefs change goes through EditPrefs. Put your actions
 	// in setPrefsLocksOnEntry instead.
-	return p1.View(), nil
+
+	// This should return the public prefs, not the private ones.
+	return stripKeysFromPrefs(newPrefs), nil
 }

 // SetPrefs saves new user preferences and propagates them throughout
@ -2193,7 +2201,8 @@ func (b *LocalBackend) SetPrefs(newp *ipn.Prefs) {

 // setPrefsLockedOnEntry requires b.mu be held to call it, but it
 // unlocks b.mu when done. newp ownership passes to this function.
-func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) {
+// It returns a readonly copy of the new prefs.
+func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) ipn.PrefsView {
 	netMap := b.netMap
 	stateKey := b.stateKey
 	oldp := b.prefs
@ -2274,6 +2283,7 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) {
 	}

 	b.send(ipn.Notify{Prefs: prefs})
+	return prefs
 }

 // GetPeerAPIPort returns the port number for the peerapi server
--- a/ipn/ipnlocal/state_test.go
+++ b/ipn/ipnlocal/state_test.go
@ -46,6 +46,7 @@ func (nt *notifyThrottler) expect(count int) {

 // put adds one notification into the throttler's queue.
 func (nt *notifyThrottler) put(n ipn.Notify) {
+	nt.t.Helper()
 	nt.mu.Lock()
 	ch := nt.ch
 	nt.mu.Unlock()
@ -592,8 +593,8 @@ func TestStateMachine(t *testing.T) {
 		cc.assertCalls("unpause", "unpause")
 		c.Assert(nn[0].State, qt.IsNotNil)
 		c.Assert(ipn.NeedsLogin, qt.Equals, *nn[0].State)
-		c.Assert(b.Prefs().LoggedOut, qt.IsTrue)
-		c.Assert(b.Prefs().WantRunning, qt.IsFalse)
+		c.Assert(b.Prefs().LoggedOut(), qt.IsTrue)
+		c.Assert(b.Prefs().WantRunning(), qt.IsFalse)
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
 	}

@ -607,8 +608,8 @@ func TestStateMachine(t *testing.T) {
 		// still logged out. So it shouldn't call it again.
 		cc.assertCalls("StartLogout", "unpause")
 		cc.assertCalls()
-		c.Assert(b.Prefs().LoggedOut, qt.IsTrue)
-		c.Assert(b.Prefs().WantRunning, qt.IsFalse)
+		c.Assert(b.Prefs().LoggedOut(), qt.IsTrue)
+		c.Assert(b.Prefs().WantRunning(), qt.IsFalse)
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
 	}

@ -620,8 +621,8 @@ func TestStateMachine(t *testing.T) {
 	{
 		notifies.drain(0)
 		cc.assertCalls("unpause", "unpause")
-		c.Assert(b.Prefs().LoggedOut, qt.IsTrue)
-		c.Assert(b.Prefs().WantRunning, qt.IsFalse)
+		c.Assert(b.Prefs().LoggedOut(), qt.IsTrue)
+		c.Assert(b.Prefs().WantRunning(), qt.IsFalse)
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
 	}

@ -634,8 +635,8 @@ func TestStateMachine(t *testing.T) {
 	{
 		notifies.drain(0)
 		cc.assertCalls("Logout", "unpause")
-		c.Assert(b.Prefs().LoggedOut, qt.IsTrue)
-		c.Assert(b.Prefs().WantRunning, qt.IsFalse)
+		c.Assert(b.Prefs().LoggedOut(), qt.IsTrue)
+		c.Assert(b.Prefs().WantRunning(), qt.IsFalse)
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
 	}

@ -647,8 +648,8 @@ func TestStateMachine(t *testing.T) {
 	{
 		notifies.drain(0)
 		cc.assertCalls("unpause", "unpause")
-		c.Assert(b.Prefs().LoggedOut, qt.IsTrue)
-		c.Assert(b.Prefs().WantRunning, qt.IsFalse)
+		c.Assert(b.Prefs().LoggedOut(), qt.IsTrue)
+		c.Assert(b.Prefs().WantRunning(), qt.IsFalse)
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
 	}

@ -918,6 +919,60 @@ func TestStateMachine(t *testing.T) {
 	}
 }

+func TestEditPrefsHasNoKeys(t *testing.T) {
+	logf := t.Logf
+	store := new(testStateStorage)
+	e, err := wgengine.NewFakeUserspaceEngine(logf, 0)
+	if err != nil {
+		t.Fatalf("NewFakeUserspaceEngine: %v", err)
+	}
+	t.Cleanup(e.Close)
+
+	b, err := NewLocalBackend(logf, "logid", store, nil, e, 0)
+	if err != nil {
+		t.Fatalf("NewLocalBackend: %v", err)
+	}
+	b.hostinfo = &tailcfg.Hostinfo{OS: "testos"}
+	b.prefs = (&ipn.Prefs{
+		Persist: &persist.Persist{
+			PrivateNodeKey:    key.NewNode(),
+			OldPrivateNodeKey: key.NewNode(),
+
+			LegacyFrontendPrivateMachineKey: key.NewMachine(),
+		},
+	}).View()
+	if b.prefs.Persist().PrivateNodeKey.IsZero() {
+		t.Fatalf("PrivateNodeKey not set")
+	}
+	p, err := b.EditPrefs(&ipn.MaskedPrefs{
+		Prefs: ipn.Prefs{
+			Hostname: "foo",
+		},
+		HostnameSet: true,
+	})
+	if err != nil {
+		t.Fatalf("EditPrefs: %v", err)
+	}
+	if p.Hostname() != "foo" {
+		t.Errorf("Hostname = %q; want foo", p.Hostname())
+	}
+
+	// Test that we can't see the PrivateNodeKey.
+	if !p.Persist().PrivateNodeKey.IsZero() {
+		t.Errorf("PrivateNodeKey = %v; want zero", p.Persist().PrivateNodeKey)
+	}
+
+	// Test that we can't see the PrivateNodeKey.
+	if !p.Persist().OldPrivateNodeKey.IsZero() {
+		t.Errorf("OldPrivateNodeKey = %v; want zero", p.Persist().OldPrivateNodeKey)
+	}
+
+	// Test that we can't see the PrivateNodeKey.
+	if !p.Persist().LegacyFrontendPrivateMachineKey.IsZero() {
+		t.Errorf("LegacyFrontendPrivateMachineKey = %v; want zero", p.Persist().LegacyFrontendPrivateMachineKey)
+	}
+}
+
 type testStateStorage struct {
 	mem     mem.Store
 	written atomic.Bool
--- a/ipn/localapi/localapi.go
+++ b/ipn/localapi/localapi.go
@ -526,7 +526,7 @@ func (h *Handler) servePrefs(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 	case "GET", "HEAD":
-		prefs = h.b.Prefs().View()
+		prefs = h.b.Prefs()
 	default:
 		http.Error(w, "unsupported method", http.StatusMethodNotAllowed)
 		return