Set node key expiration in the far future #3069
Conversation
…eauth keys to avoid Tailscale spinlock
…s will correctly return early without being routed into handleLogout and having their expiry corrupted.
| // Note: node.Expiry().Valid() was too strict — tagged nodes have nil expiry, | ||
| // which caused zero-time requests to fall through to handleLogout and | ||
| // incorrectly set their expiry to 0001-01-01 (Go zero time). | ||
| if req.Expiry.IsZero() && !node.IsExpired() { |
There was a problem hiding this comment.
This is unrelated to the issue above, but is a nice bugfix nonetheless:
Issue:
Tagged devices when first added display Key expiry "Never" on headplane, but then sometimes they change to Key expiry 12/31/1, 7:03:58 PM.
Resolution:
The change was from:
if req.Expiry.IsZero() && node.Expiry().Valid() && !node.IsExpired() { to:
if req.Expiry.IsZero() && !node.IsExpired() { This guard only runs when req.Auth == nil AND the node exists. Here are all the scenarios:
| Node state | IsExpired() |
Old guard | New guard | Correct? |
|---|---|---|---|---|
| Valid future expiry | false | return early | return early | Same |
| Valid past expiry (expired) | true | fall through → handleLogout → force re-auth | fall through → same | Same |
| Nil expiry (tagged node) | false | fall through → handleLogout → corrupts expiry | return early | Fixed |
| Zero-time expiry (already corrupted) | false | fall through → handleLogout → writes zero again | return early (no-op) | Safe |
The only case that changes behavior is the tagged-node case, which is the bug. All other cases are identical.
The key safety argument: legitimate logouts send a non-zero past expiry (caught at auth.go:35, before this guard ever runs). A zero expiry with
Auth=nil is exclusively a tailscaled restart signal — the comment at line 73 says exactly this. So returning early for any non-expired node with
zero expiry is always correct.
The fix is safe.
1 participant
I noticed that when I connected a Tailscale client using a preauthorized key it would start burning quite a few CPU cycles on the client device. Turns out the Tailscale client was spinlocking due to a bug where it sees an expiration of 0 and considers that it "must refresh immediately" triggering expensive key loads and checks in an infinite loop.
I'm not sure if this is the correct fix for the issue, would love to get some visibility from the Tailscale team to help determine what is the correct way to ensure the client knows a node's key has no expiration date.
According to the official documentation tagged devices should have no expiry, so this is still wrong, but at least it doesn't spinlock clients: https://tailscale.com/docs/features/tags#key-expiry