From 7355116c0543a53ed7e70a62ec485ef03331e29a Mon Sep 17 00:00:00 2001 From: kari-ts <135075563+kari-ts@users.noreply.github.com> Date: Fri, 29 May 2026 11:22:14 -0700 Subject: [PATCH] ipn/store: make WriteState(id, nil) delete key instead of adding nil entry (#19920) All StateStore implementations store a nil value in the cache map when WriteState is called with a nil byte slice instead of deleting the key. This causes ReadState to return (nil, nil) instead of (nil, ErrStateNotExist), since the key is still present in the map. This breaks reset-auth in Windows, Linux, and Android, and the node can't log back in without manually editing the state file. (macOS uses a different state store) DeleteProfile, DeleteAllProfilesForUser, setUnattendedModeAsConfigured are impacted but don't seem to break because the deleted keys are not reread. This deletes the key from the cache instead. Fixes tailscale/corp#42477 Signed-off-by: kari-ts --- feature/tpm/tpm.go | 7 ++-- ipn/ipnlocal/local_test.go | 30 +++++++++++++++++ ipn/ipnlocal/profiles_test.go | 44 +++++++++++++++++++++++++ ipn/store.go | 4 ++- ipn/store/kubestore/store_kube.go | 26 +++++++++++++++ ipn/store/kubestore/store_kube_test.go | 45 +++++++++++++++++++++++--- ipn/store/mem/store_mem.go | 6 +++- ipn/store/stores.go | 6 +++- ipn/store/stores_test.go | 36 +++++++++++++++++++++ 9 files changed, 194 insertions(+), 10 deletions(-) diff --git a/feature/tpm/tpm.go b/feature/tpm/tpm.go index e257aa7bc..a30bb1134 100644 --- a/feature/tpm/tpm.go +++ b/feature/tpm/tpm.go @@ -243,8 +243,11 @@ func (s *tpmStore) WriteState(k ipn.StateKey, bs []byte) error { if bytes.Equal(s.cache[k], bs) { return nil } - s.cache[k] = bytes.Clone(bs) - + if bs == nil { + delete(s.cache, k) + } else { + s.cache[k] = bytes.Clone(bs) + } return s.writeSealed() } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index e8d7e0743..390996bd7 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -8683,3 +8683,33 @@ func TestPeerRoutesCGNATCollapse(t *testing.T) { t.Errorf("with subnet: got %v; want %v", got, want) } } + +func TestResetAuthClearsMachineKey(t *testing.T) { + store := new(mem.Store) + + // Write a machine key to the store. + machineKey := key.NewMachine() + keyText, _ := machineKey.MarshalText() + if err := store.WriteState(ipn.MachineKeyStateKey, keyText); err != nil { + t.Fatal(err) + } + + // Verify key is readable. + if bs, err := store.ReadState(ipn.MachineKeyStateKey); err != nil { + t.Fatalf("ReadState before clear: %v", err) + } else if len(bs) == 0 { + t.Fatal("machine key is empty before clear") + } + + // Clear the key the same way ResetAuth does. + if err := ipn.WriteState(store, ipn.MachineKeyStateKey, nil); err != nil { + t.Fatal(err) + } + + // Verify key is gone. It should return ErrStateNotExist, + // not nil/nil which would cause initMachineKeyLocked to + // fail with "invalid key ... doesn't have expected type prefix". + if _, err := store.ReadState(ipn.MachineKeyStateKey); err != ipn.ErrStateNotExist { + t.Fatalf("ReadState after clear: got err %v, want ErrStateNotExist", err) + } +} diff --git a/ipn/ipnlocal/profiles_test.go b/ipn/ipnlocal/profiles_test.go index ec92673e5..7b6ce1cf5 100644 --- a/ipn/ipnlocal/profiles_test.go +++ b/ipn/ipnlocal/profiles_test.go @@ -1185,3 +1185,47 @@ func (k *failingHardwareAttestationKey) UnmarshalJSON([]byte) error { k.unmarshalCalled = true return errors.New("failed to unmarshal attestation key!") } + +func TestDeleteProfileClearsState(t *testing.T) { + store := new(mem.Store) + + pm, err := newProfileManagerWithGOOS(store, logger.Discard, health.NewTracker(eventbustest.NewBus(t)), "linux") + if err != nil { + t.Fatal(err) + } + + pm.SetCurrentUserID("user1") + pm.SwitchToNewProfile() + p := pm.CurrentPrefs().AsStruct() + p.Persist = &persist.Persist{ + NodeID: "node1", + PrivateNodeKey: key.NewNode(), + UserProfile: tailcfg.UserProfile{ + ID: 1, + LoginName: "user@example.com", + }, + } + if err := pm.SetPrefs(p.View(), ipn.NetworkProfile{}); err != nil { + t.Fatal(err) + } + + profileKey := pm.currentProfile.Key() + if profileKey == "" { + t.Fatal("profile key is empty") + } + + // Verify profile state exists in store. + if _, err := store.ReadState(profileKey); err != nil { + t.Fatalf("ReadState before delete: %v", err) + } + + profileID := pm.currentProfile.ID() + if err := pm.DeleteProfile(profileID); err != nil { + t.Fatal(err) + } + + // Verify profile state is deleted from store. + if _, err := store.ReadState(profileKey); err != ipn.ErrStateNotExist { + t.Fatalf("ReadState after delete: got err %v, want ErrStateNotExist", err) + } +} diff --git a/ipn/store.go b/ipn/store.go index 1bd3e5a3b..49007c14e 100644 --- a/ipn/store.go +++ b/ipn/store.go @@ -92,7 +92,9 @@ type StateStore interface { // ReadState returns the bytes associated with ID. Returns (nil, // ErrStateNotExist) if the ID doesn't have associated state. ReadState(id StateKey) ([]byte, error) - // WriteState saves bs as the state associated with ID. + // WriteState saves bs as the state associated with ID. If bs is nil, + // the state associated with ID is deleted, and a subsequent ReadState + // for ID will return ErrStateNotExist. // // Callers should generally use the ipn.WriteState wrapper func // instead, which only writes if the value is different from what's diff --git a/ipn/store/kubestore/store_kube.go b/ipn/store/kubestore/store_kube.go index f7d1b90cd..50ea1e287 100644 --- a/ipn/store/kubestore/store_kube.go +++ b/ipn/store/kubestore/store_kube.go @@ -147,6 +147,9 @@ func (s *Store) WriteState(id ipn.StateKey, bs []byte) (err error) { s.memory.WriteState(ipn.StateKey(sanitizeKey(id)), bs) } }() + if bs == nil { + return s.removeSecretField(string(id), s.secretName) + } return s.updateSecret(map[string][]byte{string(id): bs}, s.secretName) } @@ -339,6 +342,29 @@ func (s *Store) updateSecret(data map[string][]byte, secretName string) (err err return nil } +func (s *Store) removeSecretField(key, secretName string) error { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + if s.canPatchSecret(secretName) { + return s.client.JSONPatchResource(ctx, secretName, kubeclient.TypeSecrets, []kubeclient.JSONPatch{ + { + Op: "remove", + Path: "/data/" + sanitizeKey(ipn.StateKey(key)), + }, + }) + } + // No patch permissions, use UPDATE: get the secret, delete the key, update. + secret, err := s.client.GetSecret(ctx, secretName) + if err != nil { + return fmt.Errorf("error getting Secret %s: %w", secretName, err) + } + delete(secret.Data, sanitizeKey(ipn.StateKey(key))) + if err := s.client.UpdateSecret(ctx, secret); err != nil { + return fmt.Errorf("error updating Secret %s: %w", secretName, err) + } + return nil +} + func (s *Store) loadState() (err error) { ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() diff --git a/ipn/store/kubestore/store_kube_test.go b/ipn/store/kubestore/store_kube_test.go index 1e6f711d6..5f65116bc 100644 --- a/ipn/store/kubestore/store_kube_test.go +++ b/ipn/store/kubestore/store_kube_test.go @@ -172,6 +172,32 @@ func TestWriteState(t *testing.T) { }, allowPatch: true, }, + { + name: "delete_with_patch", + initial: map[string][]byte{ + "foo": []byte("bar"), + "baz": []byte("quux"), + }, + key: "foo", + value: nil, + wantData: map[string][]byte{ + "baz": []byte("quux"), + }, + allowPatch: true, + }, + { + name: "delete_with_update", + initial: map[string][]byte{ + "foo": []byte("bar"), + "baz": []byte("quux"), + }, + key: "foo", + value: nil, + wantData: map[string][]byte{ + "baz": []byte("quux"), + }, + allowPatch: false, + }, } for _, tt := range tests { @@ -208,6 +234,9 @@ func TestWriteState(t *testing.T) { } else if p.Op == "add" && strings.HasPrefix(p.Path, "/data/") { key := strings.TrimPrefix(p.Path, "/data/") secret[key] = p.Value.([]byte) + } else if p.Op == "remove" && strings.HasPrefix(p.Path, "/data/") { + key := strings.TrimPrefix(p.Path, "/data/") + delete(secret, key) } } return nil @@ -234,11 +263,17 @@ func TestWriteState(t *testing.T) { // Verify memory store was updated got, err := s.memory.ReadState(ipn.StateKey(sanitizeKey(string(tt.key)))) - if err != nil { - t.Errorf("reading from memory store: %v", err) - } - if !cmp.Equal(got, tt.value) { - t.Errorf("memory store key %q = %v, want %v", tt.key, got, tt.value) + if tt.value == nil { + if err != ipn.ErrStateNotExist { + t.Errorf("reading deleted key from memory store: got err %v, want ErrStateNotExist", err) + } + } else { + if err != nil { + t.Errorf("reading from memory store: %v", err) + } + if !cmp.Equal(got, tt.value) { + t.Errorf("memory store key %q = %v, want %v", tt.key, got, tt.value) + } } }) } diff --git a/ipn/store/mem/store_mem.go b/ipn/store/mem/store_mem.go index 247714c9a..eafa429c7 100644 --- a/ipn/store/mem/store_mem.go +++ b/ipn/store/mem/store_mem.go @@ -49,7 +49,11 @@ func (s *Store) WriteState(id ipn.StateKey, bs []byte) error { if s.cache == nil { s.cache = map[ipn.StateKey][]byte{} } - s.cache[id] = bytes.Clone(bs) + if bs == nil { + delete(s.cache, id) + } else { + s.cache[id] = bytes.Clone(bs) + } return nil } diff --git a/ipn/store/stores.go b/ipn/store/stores.go index fd51f8c38..31a055322 100644 --- a/ipn/store/stores.go +++ b/ipn/store/stores.go @@ -214,7 +214,11 @@ func (s *FileStore) WriteState(id ipn.StateKey, bs []byte) error { if bytes.Equal(s.cache[id], bs) { return nil } - s.cache[id] = bytes.Clone(bs) + if bs == nil { + delete(s.cache, id) + } else { + s.cache[id] = bytes.Clone(bs) + } bs, err := json.MarshalIndent(s.cache, "", " ") if err != nil { return err diff --git a/ipn/store/stores_test.go b/ipn/store/stores_test.go index 345b1c103..c9f4e3348 100644 --- a/ipn/store/stores_test.go +++ b/ipn/store/stores_test.go @@ -135,11 +135,41 @@ func testStoreSemantics(t *testing.T, store ipn.StateStore) { } } +func testStoreDeleteSemantics(t *testing.T, store ipn.StateStore) { + t.Helper() + + // Write a key, verify it exists. + if err := store.WriteState("delme", []byte("val")); err != nil { + t.Fatalf("WriteState: %v", err) + } + if bs, err := store.ReadState("delme"); err != nil { + t.Fatalf("ReadState after write: %v", err) + } else if string(bs) != "val" { + t.Fatalf("ReadState after write: got %q, want %q", bs, "val") + } + + // Delete by writing nil. + if err := store.WriteState("delme", nil); err != nil { + t.Fatalf("WriteState(nil): %v", err) + } + + // Read should return ErrStateNotExist. + if _, err := store.ReadState("delme"); err != ipn.ErrStateNotExist { + t.Fatalf("ReadState after delete: got err %v, want ErrStateNotExist", err) + } + + // Delete of a non-existent key should not error. + if err := store.WriteState("never-existed", nil); err != nil { + t.Fatalf("WriteState(nil) on non-existent key: %v", err) + } +} + func TestMemoryStore(t *testing.T) { tstest.PanicOnLog() store := new(mem.Store) testStoreSemantics(t, store) + testStoreDeleteSemantics(t, store) } func TestFileStore(t *testing.T) { @@ -154,6 +184,7 @@ func TestFileStore(t *testing.T) { } testStoreSemantics(t, store) + testStoreDeleteSemantics(t, store) // Build a brand new file store and check that both IDs written // above are still there. @@ -176,4 +207,9 @@ func TestFileStore(t *testing.T) { t.Errorf("reading %q (2nd store): got %q, want %q", key, bs, want) } } + + // Verify deleted key is still gone after reload. + if _, err := store.ReadState("delme"); err != ipn.ErrStateNotExist { + t.Fatalf("reading deleted key from reloaded store: got err %v, want ErrStateNotExist", err) + } }