diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 3fe6e0153..f9fda9cd9 100644 --- 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 diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 9dab83299..983a61bb4 100644 --- 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 diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index 913d4dd6f..114eb1956 100644 --- 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