From e66084195b5b7ec7d34bddbcac7c2e279d09bf99 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 21 Oct 2026 19:45:39 +0000 Subject: [PATCH] db: succeed on a repeated revoke of an auth key A repeated v2 DELETE of an auth key now answers 200, as Tailscale does. --- hscontrol/apiv2_keys_test.go | 6 ++++-- hscontrol/db/preauth_keys.go | 8 ++++---- hscontrol/servertest/lifecycle_compat_test.go | 8 +++----- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/hscontrol/apiv2_keys_test.go b/hscontrol/apiv2_keys_test.go index 5a296b2df..2c46741e9 100644 --- a/hscontrol/apiv2_keys_test.go +++ b/hscontrol/apiv2_keys_test.go @@ -297,8 +297,9 @@ func TestAPIv2Key_Delete(t *testing.T) { require.NotNil(t, pak.Revoked) require.Error(t, pak.Validate(), "revoked key is invalid") - // Delete again -> 404 (already revoked). - assert.Equal(t, http.StatusNotFound, api.Delete("/api/v2/tailnet/-/keys/"+created.ID).Code) + // Delete again -> 200, as Tailscale answers; the first revoke time stays. + assert.Equal(t, http.StatusOK, api.Delete("/api/v2/tailnet/-/keys/"+created.ID).Code) + assert.Equal(t, pak.Revoked, srvKey(t, app, created.ID).Revoked) // The collector hard-deletes keys revoked before the cutoff. reaped, err := app.state.DestroyRevokedPreAuthKeysBefore(time.Now().Add(time.Minute)) @@ -306,6 +307,7 @@ func TestAPIv2Key_Delete(t *testing.T) { assert.GreaterOrEqual(t, reaped, 1) assert.False(t, containsKeyID(listKeys(t, api), created.ID), "collector removed the revoked key") assert.Equal(t, http.StatusNotFound, api.Get("/api/v2/tailnet/-/keys/"+created.ID).Code) + assert.Equal(t, http.StatusNotFound, api.Delete("/api/v2/tailnet/-/keys/"+created.ID).Code) } func TestAPIv2Key_Create_NoTags_NoOwner_400(t *testing.T) { diff --git a/hscontrol/db/preauth_keys.go b/hscontrol/db/preauth_keys.go index 89a2795aa..77d71129e 100644 --- a/hscontrol/db/preauth_keys.go +++ b/hscontrol/db/preauth_keys.go @@ -401,12 +401,12 @@ func (hsdb *HSDatabase) RevokePreAuthKey(id uint64) error { // RevokePreAuthKey soft-revokes a key (the v2 API's DELETE): the row is kept and // stays retrievable with its invalid flag set, but the key can no longer // authorize nodes. The background collector hard-deletes it after the retention -// window. An already-revoked or unknown id returns [ErrPreAuthKeyNotFound], so a -// repeated DELETE is a clean 404. +// window. Revoking a revoked key keeps its first revoke time and succeeds, as a +// repeated DELETE does on Tailscale; an unknown id returns [ErrPreAuthKeyNotFound]. func RevokePreAuthKey(tx *gorm.DB, id uint64) error { res := tx.Model(&types.Credential{}). - Where("kind = ? AND id = ? AND revoked IS NULL", types.CredentialPreAuthKey, id). - Update("revoked", time.Now()) + Where("kind = ? AND id = ?", types.CredentialPreAuthKey, id). + Update("revoked", gorm.Expr("COALESCE(revoked, ?)", time.Now())) if res.Error != nil { return res.Error } diff --git a/hscontrol/servertest/lifecycle_compat_test.go b/hscontrol/servertest/lifecycle_compat_test.go index a54d26015..2ad439789 100644 --- a/hscontrol/servertest/lifecycle_compat_test.go +++ b/hscontrol/servertest/lifecycle_compat_test.go @@ -36,14 +36,12 @@ import ( // the step of what killed nodes leave behind. A listed check that matches // fails, so a fix must delete its entry. var lifecycleSkipReasons = map[string]string{ - "ephemeral-lifecycle/E3-key-delete/2/keys": deletedKeyListed, - "ephemeral-lifecycle/E3-key-delete/3/keys": deletedKeyListed, - "ephemeral-lifecycle/E3-key-delete/4/status": deletedKeyDeleteNotFound, + "ephemeral-lifecycle/E3-key-delete/2/keys": deletedKeyListed, + "ephemeral-lifecycle/E3-key-delete/3/keys": deletedKeyListed, } const ( - deletedKeyListed = "DELETED_KEY_LISTED: headscale lists a deleted auth key; SaaS leaves it out of both key lists, while a read by ID still answers" - deletedKeyDeleteNotFound = "DELETED_KEY_DELETE_NOT_FOUND: headscale answers 404 to a second delete of an auth key; SaaS answers 200" + deletedKeyListed = "DELETED_KEY_LISTED: headscale lists a deleted auth key; SaaS leaves it out of both key lists, while a read by ID still answers" ) // TestRouteApprovalCompat replays a user-owned router through admin