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 <kari@tailscale.com>

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()
 }
 

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)
+	}
+}

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)
+	}
+}

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

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()

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)
+				}
 			}
 		})
 	}

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
 }
 

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

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)
+	}
 }