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