Skip to content

Logout: revoke the node even when the server is already stopped#53

Closed
danReynolds wants to merge 1 commit into
mainfrom
logout-revokes-stopped-node
Closed

Logout: revoke the node even when the server is already stopped#53
danReynolds wants to merge 1 commit into
mainfrom
logout-revokes-stopped-node

Conversation

@danReynolds

Copy link
Copy Markdown
Owner

Problem

Logout(stateDir) only revokes the node key at the control plane when a server is currently running:

s := srv
if s != nil { revokeNodeKey(s) }   // revokeNodeKey needs a live LocalClient
Stop()
os.RemoveAll(stateDir)

revokeNodeKey issues the control-plane logout via srv.LocalClient().Logout(ctx), which requires a live backend. So a caller that stops/down()s the server before calling Logout (e.g. a disconnect()-then-logout() sequence) gets a local-only wipe: the node is never told to log out, and stays registered in the tailnet until node-key expiry. Worse, any surviving copy of the (now-wiped) state remains a valid credential for that window.

A Logout that silently leaves the node registered just because the engine was already stopped is the wrong contract — logging out should clear the node regardless of connection state.

Fix

  • Cache the most recent successful Start's hostname + controlURL in package state.
  • When Logout finds no live server but persisted state exists, briefly bring the node back up from that state (revokeStoppedNode) — reusing the persisted node key, no auth key — so the LocalAPI logout can reach the control plane, then tear the transient server down and wipe.
  • Best-effort and timeout-bounded (20s): if the control plane is unreachable the transient bring-up fails and we still wipe, exactly as before. With no cached control URL we skip straight to the wipe (no hang).

Net: a logout after a down()/Stop() now still revokes the node at the control plane, instead of only clearing local credentials.

Verification

  • go test ./... passes, including a new TestLogout_NoCachedControlURLWipesWithoutHang guard (persisted state + no cached control URL → wipes without attempting a hanging transient revoke). Existing TestLogout_RemovesDir / HasState tests unchanged.
  • go build -buildmode=c-shared ./cmd/dylib builds cleanly; DuneLogout export intact.
  • Not yet covered here: an end-to-end assertion that the stopped-node revoke actually expires the node at the control plane. The transient bring-up uses the standard tsnet "start from persisted state" path, but this should be confirmed against the headscale e2e (test/e2e/run_e2e.sh) before merge — happy to add a down() → logout() → node revoked case there.

🤖 Generated with Claude Code

Logout only revoked the node key at the control plane when a server was running — revokeNodeKey needs a live LocalClient. A caller that stops/downs the server before logging out (e.g. disconnect-then-logout) therefore got a local-only wipe: the node stayed registered in the tailnet until key expiry, and any surviving copy of the wiped state remained a valid credential.

Cache the last Start's hostname + control URL; when Logout finds no live server but persisted state exists, briefly bring the node back up from that state (reusing its node key, no auth key) to issue the control-plane logout, then wipe. Best-effort and timeout-bounded — the local wipe always proceeds.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@danReynolds

Copy link
Copy Markdown
Owner Author

Closing this after re-checking the original concern.

The machine-key/node-ID rotation case is already covered on main: Logout(stateDir) always removes the persisted state directory, even when the embedded server was already stopped, so the next up() cannot reuse the old local machine key and should register as a fresh node.

This PR only adds a best-effort online remote-revocation path for the narrower down()/Stop() then logout() case. It still cannot revoke when the control plane/network is unavailable, and that stopped-node revocation behavior is not proven by the current tests. Given the motivating question was offline/local rotation, this is extra complexity rather than a necessary fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant