mirror/tailscale
1
0
Fork 0
mirror of https://github.com/tailscale/tailscale.git synced 2026-06-26 08:45:40 -04:00
Code Issues Packages Projects Releases Wiki Activity

Revert "control/controlclient: continue map poll during key expiry to receive extensions" (#20257)

Browse Source 
* Revert "control/controlclient: continue map poll during key expiry to receive extensions"

This reverts commit 6a822dcc36. This commit
has caused test failures in the corp repo by unexpected changing the login
behaviour when nodes have a valid node key.

Updates tailscale/corp#43705
Updates #19326

Signed-off-by: Alex Chan <alexc@tailscale.com>

* Revert "tsnet: test key extension after server restart"

This reverts commit 317201375f. This test
relies on changes in 317201375f, which is
also being reverted because it causes test failures in corp.

Updates tailscale/corp#43705
Updates #19326

Signed-off-by: Alex Chan <alexc@tailscale.com>

---------

Signed-off-by: Alex Chan <alexc@tailscale.com>
This commit is contained in:
Alex Chan
2026-06-25 23:24:12 +01:00
committed by GitHub
parent 6e1de5b651
commit 9169b206be
4 changed files with 12 additions and 772 deletions
Download Patch File Download Diff File Expand all files Collapse all files

54
control/controlclient/auto.go
View File

@@ -472,15 +472,16 @@ func (mrs mapRoutineState) UpdateFullNetmap(nm *netmap.NetworkMap) {
c.mu.Lock()
c.inMapPoll = true
c.expiry = nm.SelfKeyExpiry()
c.logf("[v1] mapRoutine: netmap received: loggedIn=%v inMapPoll=true", c.loggedIn)
stillAuthed := c.loggedIn
c.logf("[v1] mapRoutine: netmap received: loggedIn=%v inMapPoll=true", stillAuthed)
// Reset the backoff timer if we got a netmap.
mrs.bo.Reset()
c.mu.Unlock()
// Always send status - sendStatus will check if we should forward the netmap
// based on loggedIn, hasNodeKey, and inMapPoll.
c.sendStatus("mapRoutine-got-netmap", nil, "", nm)
if stillAuthed {
c.sendStatus("mapRoutine-got-netmap", nil, "", nm)
}
}
func (mrs mapRoutineState) UpdateNetmapDelta(muts []netmap.NodeMutation) bool {
@@ -613,16 +614,10 @@ func (c *Auto) mapRoutine() {
c.mu.Lock()
loggedIn := c.loggedIn
c.logf("[v1] mapRoutine: loggedIn=%v", loggedIn)
ctx := c.mapCtx
c.mu.Unlock()
// Check if we have a valid node key that could receive updates.
// Even if !loggedIn (e.g., key expired, waiting for interactive auth),
// we should still poll if we have credentials, because the server
// might send us a key extension notification.
_, hasNodeKey := c.direct.GetPersist().PublicNodeKeyOK()
c.logf("[v1] mapRoutine: loggedIn=%v hasNodeKey=%v", loggedIn, hasNodeKey)
report := func(err error, msg string) {
c.logf("[v1] %s: %v", msg, err)
err = fmt.Errorf("%s: %w", msg, err)
@@ -633,8 +628,8 @@ func (c *Auto) mapRoutine() {
}
}
if !loggedIn && !hasNodeKey {
// No credentials at all, wait for auth to complete.
if !loggedIn {
// Wait for something interesting to happen
c.mu.Lock()
c.inMapPoll = false
c.mu.Unlock()
@@ -725,17 +720,14 @@ func (c *Auto) sendStatus(who string, err error, url string, nm *netmap.NetworkM
loginGoal := c.loginGoal
c.mu.Unlock()
// Check if we have a valid node key - if so, we should forward the netmap
// even if !loggedIn, to allow the backend to see key expiry changes.
_, hasNodeKey := c.direct.GetPersist().PublicNodeKeyOK()
c.logf("[v1] sendStatus: %s: loggedIn=%v inMapPoll=%v hasNodeKey=%v", who, loggedIn, inMapPoll, hasNodeKey)
c.logf("[v1] sendStatus: %s: loggedIn=%v inMapPoll=%v", who, loggedIn, inMapPoll)
var p persist.PersistView
if nm != nil && (loggedIn || hasNodeKey) && inMapPoll {
if nm != nil && loggedIn && inMapPoll {
p = c.direct.GetPersist()
} else {
// don't send netmap status, as it's misleading when we're
// not logged in and have no credentials.
// not logged in.
nm = nil
}
newSt := &Status{
@@ -850,29 +842,7 @@ func (c *Auto) Login(flags LoginFlags) {
c.loginGoal = &LoginGoal{
flags: flags,
}
// If we have valid credentials (loggedIn=true) or a valid node key,
// don't cancel the map poll. This allows the client to continue receiving
// key extension notifications from the server while the auth flow proceeds
// in parallel.
//
// This is important for the "Extend key" feature: if the admin extends a
// key while the user has clicked "Login", we want the map poll to receive
// that notification and recover without requiring the user to complete the
// auth flow.
//
// The hasNodeKey check handles the case where a tsnet server restarts with
// an expired key: loggedIn is false (server returned AuthURL), but we have
// a valid node key that can still receive map updates including key extensions.
//
// "First successful flow wins": if a key extension arrives via map poll,
// the client recovers. If the auth flow completes first, that works too.
var hasNodeKey bool
if c.direct != nil {
_, hasNodeKey = c.direct.GetPersist().PublicNodeKeyOK()
}
if !c.loggedIn && !hasNodeKey {
c.cancelMapCtxLocked()
}
c.cancelMapCtxLocked()
c.cancelAuthCtxLocked()
}

152
control/controlclient/key_expiry_test.go
View File

@@ -1,152 +0,0 @@
// Copyright (c) Tailscale Inc & contributors
// SPDX-License-Identifier: BSD-3-Clause
package controlclient
import (
"context"
"testing"
"tailscale.com/types/key"
"tailscale.com/types/persist"
)
// TestLoginPreservesMapPollWhenLoggedIn tests the fix for the key extension bug.
//
// When a client has valid credentials (loggedIn=true) but needs to re-authenticate
// due to key expiry, calling Login() should NOT cancel the map poll. This allows
// the client to continue receiving key extension notifications from the server
// while the auth flow proceeds in parallel.
func TestLoginPreservesMapPollWhenLoggedIn(t *testing.T) {
// Create an Auto client that is already logged in
// This simulates a client with valid credentials but expired key
auto := &Auto{
logf: t.Logf,
loggedIn: true, // Already authenticated (key expired, but creds valid)
closed: false,
}
auto.mapCtx, auto.mapCancel = context.WithCancel(context.Background())
t.Cleanup(auto.mapCancel)
auto.authCtx, auto.authCancel = context.WithCancel(context.Background())
t.Cleanup(auto.authCancel)
originalMapCtx := auto.mapCtx
// Call Login() - this is what happens when user clicks "Login" after key expiry
auto.Login(LoginInteractive)
// The fix: when loggedIn=true, mapCtx should NOT be cancelled
// This allows the map poll to continue receiving key extension notifications
select {
case <-originalMapCtx.Done():
t.Error("Login() cancelled mapCtx even though loggedIn=true; key extension notifications would be lost")
default:
// Good - map context still active
}
// Verify loginGoal was set (auth flow can proceed in parallel)
auto.mu.Lock()
hasLoginGoal := auto.loginGoal != nil
auto.mu.Unlock()
if !hasLoginGoal {
t.Error("loginGoal should be set even though mapCtx wasn't cancelled")
}
}
// TestLoginPreservesMapPollWithNodeKey tests the tsnet restart scenario.
//
// When a tsnet server restarts with an expired key:
// 1. The server has a valid node key stored in persist
// 2. Control returns an AuthURL (for interactive login)
// 3. loggedIn is false (because TryLogin returned a URL, not success)
// 4. But we should NOT cancel the map poll, because the server might send
// a key extension notification via the existing node key
//
// This test verifies that Login() preserves the map poll when we have a
// valid node key, even if loggedIn=false.
func TestLoginPreservesMapPollWithNodeKey(t *testing.T) {
// Create persist data with a valid node key (simulating stored credentials)
nodeKey := key.NewNode()
p := &persist.Persist{
PrivateNodeKey: nodeKey,
}
// Create a Direct client with the persist data
direct := &Direct{
persist: p.View(),
}
// Create an Auto client that is NOT logged in but HAS a valid node key
// This simulates a tsnet server restart with expired key:
// - loggedIn=false because control returned an AuthURL
// - but we have a valid node key that can receive map updates
auto := &Auto{
logf: t.Logf,
loggedIn: false, // Control returned AuthURL, so not "logged in" yet
closed: false,
direct: direct,
}
auto.mapCtx, auto.mapCancel = context.WithCancel(context.Background())
t.Cleanup(auto.mapCancel)
auto.authCtx, auto.authCancel = context.WithCancel(context.Background())
t.Cleanup(auto.authCancel)
originalMapCtx := auto.mapCtx
// Call Login() - this is what tsnet's StartLoginInteractive does
auto.Login(LoginInteractive)
// The fix: even though loggedIn=false, we have a valid node key,
// so mapCtx should NOT be cancelled. This allows us to receive
// key extension notifications from the server.
select {
case <-originalMapCtx.Done():
t.Error("Login() cancelled mapCtx even though we have a valid node key; " +
"key extension notifications would be lost in tsnet restart scenario")
default:
// Good - map context still active, can receive key extensions
}
// Verify loginGoal was set (auth flow can proceed in parallel)
auto.mu.Lock()
hasLoginGoal := auto.loginGoal != nil
auto.mu.Unlock()
if !hasLoginGoal {
t.Error("loginGoal should be set for the auth flow to proceed")
}
}
// TestLoginCancelsMapPollWhenNoNodeKey verifies that when there's no node key
// at all (fresh install, never authenticated), Login() should cancel the map poll.
func TestLoginCancelsMapPollWhenNoNodeKey(t *testing.T) {
// Create a Direct client with empty persist (no node key)
direct := &Direct{
persist: new(persist.Persist).View(),
}
auto := &Auto{
logf: t.Logf,
loggedIn: false,
closed: false,
direct: direct,
}
auto.mapCtx, auto.mapCancel = context.WithCancel(context.Background())
t.Cleanup(auto.mapCancel)
auto.authCtx, auto.authCancel = context.WithCancel(context.Background())
t.Cleanup(auto.authCancel)
originalMapCtx := auto.mapCtx
// Call Login()
auto.Login(LoginInteractive)
// When loggedIn=false AND no node key, mapCtx SHOULD be cancelled
select {
case <-originalMapCtx.Done():
// Good - cancelled as expected for fresh login with no credentials
default:
t.Error("mapCtx should be cancelled when loggedIn=false and no node key")
}
}