diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 53e98e33e..798e43499 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1150,14 +1150,28 @@ func (b *LocalBackend) shouldPauseControlClientLocked(prefs ipn.PrefsView) bool return false } -// DisconnectControl shuts down control client. This can be run before node shutdown to force control to consider this ndoe -// inactive. This can be used to ensure that nodes that are HA subnet router or app connector replicas are shutting -// down, clients switch over to other replicas whilst the existing connections are kept alive for some period of time. +// DisconnectControl shuts down the control client. This can be run before +// node shutdown to force control to consider this node inactive. This can +// be used to ensure that nodes that are HA subnet router or app connector +// replicas are shutting down, clients switch over to other replicas whilst +// the existing connections are kept alive for some period of time. +// +// Shutdown of the detached client is synchronous: it cancels the client's +// in-flight requests and waits for its goroutines to exit, but it does not +// wait for pending updates to be delivered. func (b *LocalBackend) DisconnectControl() { b.mu.Lock() cc := b.resetControlClientLocked() b.mu.Unlock() + // The Shutdown call must not run while b.mu is held, per the deadlock + // history in tailscale/tailscale#18052: controlclient.Auto's + // goroutines deliver callbacks into LocalBackend through an execqueue + // whose RunSync holds the queue mutex while the callback acquires + // b.mu, and Auto.Shutdown acquires that same queue mutex, so calling + // it with b.mu held inverts the lock order. A previous attempt to + // shut the client down synchronously inside resetControlClientLocked + // with b.mu held (#18127) deadlocked and was reverted (#18149). if cc != nil { cc.Shutdown() } @@ -3004,6 +3018,23 @@ func (b *LocalBackend) controlDebugFlags() []string { func (b *LocalBackend) Start(opts ipn.Options) error { defer b.CheckDeadlocks()() + // Shut down the previous control client, if any, before starting a + // new one, so the old client can't race with the new one. Without + // this, an in-flight lite map update carrying stale Hostinfo (notably + // RequestTags) could be processed by the control plane after the new + // client's requests, which made retagging with "tailscale up + // --advertise-tags" intermittently look like an invalid tag + // transition and log the node out (tailscale/tailscale#20365). + // + // TODO(bradfitz,nickkhyl): this is still racy if Start is called + // concurrently: whichever call loses the race to reacquire b.mu + // below then detaches the winner's new control client in startLocked + // and shuts it down in a goroutine, without the ordering guarantee + // that this call provides. This is a workaround until #18052 is + // properly fixed and a control client can be shut down synchronously + // with b.mu held. + b.DisconnectControl() + b.mu.Lock() defer b.mu.Unlock() return b.startLocked(opts) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 7ce4f590c..e768c70c5 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -352,6 +352,39 @@ func (b *LocalBackend) nonInteractiveLoginForStateTest() { cc.Login(b.loginFlags | controlclient.LoginInteractive) } +// TestStartShutsDownPreviousControlClient verifies that Start waits for the +// previous control client to fully shut down before creating a new one. +// +// If the old client is still alive when the new one starts, its in-flight +// requests (carrying stale Hostinfo, notably RequestTags) can race with the +// new client's requests at the control plane. That made retagging a node +// with "tailscale up --advertise-tags" intermittently log the node out +// (tailscale/tailscale#20365): a stale RequestTags update processed after +// the tag transition looks like an invalid transition, so the control +// server expires the node key. +func TestStartShutsDownPreviousControlClient(t *testing.T) { + const enableLogging = true + var cc *mockControl + b := newLocalBackendWithTestControl(t, enableLogging, func(tb testing.TB, opts controlclient.Options) controlclient.Client { + if cc != nil { + select { + case <-cc.shutdown: + default: + t.Errorf("new control client created before the previous one was shut down") + } + } + cc = newClient(t, opts) + return cc + }) + + for i := range 3 { + t.Logf("Start %d", i+1) + if err := b.Start(ipn.Options{}); err != nil { + t.Fatalf("Start: %v", err) + } + } +} + // A very precise test of the sequence of function calls generated by // ipnlocal.Local into its controlclient instance, and the events it // produces upstream into the UI. diff --git a/tstest/integration/integration_test.go b/tstest/integration/integration_test.go index cabb50cc7..192ecdd12 100644 --- a/tstest/integration/integration_test.go +++ b/tstest/integration/integration_test.go @@ -532,6 +532,138 @@ func TestOneNodeUpAuth(t *testing.T) { } } +// TestRetagStaleMapRequestRace reproduces tailscale/tailscale#20365: a node +// tagged tag:tag1, where tag:tag1 owns tag:tag2, is retagged with "tailscale +// up --advertise-tags=tag:tag2". This should always succeed, but sometimes +// the machine is logged out instead. +// +// The cause is a race: "tailscale up" makes LocalBackend.Start shut down the +// old control client asynchronously while the new one starts, so a lite map +// update carrying the old Hostinfo.RequestTags can still be in flight when +// the new client's requests retag the node. If control processes the stale +// update after the tag transition, it looks like a request to change the +// node's tags from tag:tag2 back to tag:tag1. That fails the tag ownership +// check (tag:tag2 doesn't own tag:tag1), and control expires the node's key +// to force reauthentication, logging the machine out. +// +// The test recreates that interleaving deterministically: it triggers a lite +// map update carrying the old tags (any routine hostinfo change does that), +// holds it at the server, retags the node, and only then lets the held +// update be processed. +func TestRetagStaleMapRequestRace(t *testing.T) { + tstest.Parallel(t) + + var ( + holdStale atomic.Bool + staleHeld = make(chan struct{}, 1) + staleRelease = make(chan struct{}) + staleDone = make(chan struct{}, 1) + ) + env := NewTestEnv(t, ConfigureControl(func(control *testcontrol.Server) { + control.TagOwners = map[string][]string{ + "tag:tag1": nil, + "tag:tag2": {"tag:tag1"}, + } + control.HoldMapRequest = func(req *tailcfg.MapRequest) (done func()) { + if req.Stream || req.Hostinfo == nil || !holdStale.Load() { + return nil + } + if !slices.Equal(req.Hostinfo.RequestTags, []string{"tag:tag1"}) { + return nil + } + select { + case staleHeld <- struct{}{}: + default: + } + <-staleRelease + return func() { + select { + case staleDone <- struct{}{}: + default: + } + } + } + })) + + n1 := NewTestNode(t, env) + d1 := n1.StartDaemon() + defer d1.MustCleanShutdown(t) + n1.AwaitResponding() + n1.MustUp("--advertise-tags=tag:tag1") + n1.AwaitRunning() + + nodes := env.Control.AllNodes() + if len(nodes) != 1 { + t.Fatalf("expected 1 node, got %d", len(nodes)) + } + origKey := nodes[0].Key + if got, want := nodes[0].Tags, []string{"tag:tag1"}; !slices.Equal(got, want) { + t.Fatalf("node tags = %v; want %v", got, want) + } + + // Make the current control client send a lite map update carrying the + // old RequestTags, as any routine hostinfo change does. Control holds + // it (per HoldMapRequest above) so that it's still outstanding when + // the node is retagged below. Shutting down that control client + // cancels the request but can't unsend it; control still has it. + holdStale.Store(true) + if out, err := n1.Tailscale("set", "--hostname=retag-race-test").CombinedOutput(); err != nil { + t.Fatalf("tailscale set: %v, %s", err, out) + } + select { + case <-staleHeld: + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for the stale map update to reach control") + } + + // Retag the node while the stale update is still outstanding. This + // doesn't block on the held update: the new control client's requests + // are processed while it is held. + n1.MustUp("--advertise-tags=tag:tag2") + if err := tstest.WaitFor(10*time.Second, func() error { + n := env.Control.Node(origKey) + if n == nil { + return fmt.Errorf("node %v not found in control", origKey.ShortString()) + } + if !slices.Equal(n.Tags, []string{"tag:tag2"}) { + return fmt.Errorf("node tags = %v; want [tag:tag2]", n.Tags) + } + return nil + }); err != nil { + t.Fatal(err) + } + + // Let control process the stale update, now that the retag has been + // committed, and wait for it to finish. + close(staleRelease) + select { + case <-staleDone: + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for control to process the stale map update") + } + + // The retag should stick, and the machine should stay logged in with + // the same node key. With the bug present, control instead expires the + // node key when it processes the stale update. + n := env.Control.Node(origKey) + if n == nil { + t.Fatalf("node %v disappeared from control", origKey.ShortString()) + } + if !n.KeyExpiry.IsZero() { + t.Fatalf("control expired the node key after processing a stale map update; the machine was logged out (issue 20365)") + } + if got, want := n.Tags, []string{"tag:tag2"}; !slices.Equal(got, want) { + t.Fatalf("node tags = %v; want %v", got, want) + } + st := n1.MustStatus() + if st.BackendState != "Running" { + t.Errorf("BackendState = %q; want Running", st.BackendState) + } + if st.Self.PublicKey != origKey { + t.Errorf("node key changed from %v to %v; the machine was logged out and re-registered", origKey.ShortString(), st.Self.PublicKey.ShortString()) + } +} + // Returns true if the error returned by [exec.Run] fails with a non-zero // exit code, false otherwise. func isNonZeroExitCode(err error) bool { diff --git a/tstest/integration/testcontrol/testcontrol.go b/tstest/integration/testcontrol/testcontrol.go index 0ad335ae3..4545f23ec 100644 --- a/tstest/integration/testcontrol/testcontrol.go +++ b/tstest/integration/testcontrol/testcontrol.go @@ -70,6 +70,29 @@ type Server struct { // belong to the same user. AllNodesSameUser bool + // TagOwners, if non-nil, enables modeling of the production control + // server's tag transition handling. Map keys are tags (e.g. "tag:foo") + // and values are the tags whose nodes are allowed to assign the key's + // tag. A node registering with Hostinfo.RequestTags gets those tags + // (signup-time ownership checks are not modeled). A later non-streaming + // map request whose Hostinfo.RequestTags differ from both the node's + // stored Hostinfo's RequestTags and its current tags is treated as a + // tag transition request: if the node's current tags own each requested + // tag, the node is retagged; otherwise its node key is expired to force + // reauthentication, as the production control server does. + // + // If nil, RequestTags in map requests are ignored. + TagOwners map[string][]string + + // HoldMapRequest, if non-nil, is called with each incoming MapRequest + // before the server starts processing it. It may block to delay + // processing, letting tests control the order in which concurrent map + // requests are handled. If it returns a non-nil func, the server calls + // it when it finishes handling the request. For streaming requests + // that is when the poll ends, so returning a done func is mostly + // useful for non-streaming requests. + HoldMapRequest func(*tailcfg.MapRequest) (done func()) + // AllOnline, if true, marks every peer entry in MapResponses as // Online=true. This is a coarse stand-in for the per-node // online/offline tracking that production control servers do based @@ -1027,6 +1050,11 @@ func (s *Server) serveRegister(w http.ResponseWriter, r *http.Request, mkey key. CapMap: capMap, Capabilities: slices.Collect(maps.Keys(capMap)), } + if s.TagOwners != nil && req.Hostinfo != nil { + // Trust the requested tags at signup; ownership checks + // against the registering user are not modeled. + node.Tags = slices.Clone(req.Hostinfo.RequestTags) + } if s.MagicDNSDomain != "" { node.Name = node.Name + "." + s.MagicDNSDomain + "." } @@ -1258,6 +1286,71 @@ func (s *Server) incrInServeMap(delta int) { s.inServeMap += delta } +// handleTagTransitionLocked models the production control server's handling +// of tag changes requested via Hostinfo.RequestTags in non-streaming map +// requests (see updateTags in the control server). A request whose +// RequestTags differ from both the node's stored Hostinfo's RequestTags and +// the node's current tags is a tag transition request. If the transition is +// permitted by s.TagOwners, the node is retagged; otherwise its node key is +// expired to force reauthentication. +// +// hi is the Hostinfo from the incoming request; node.Hostinfo is the +// previously stored one. s.mu must be held. +func (s *Server) handleTagTransitionLocked(node *tailcfg.Node, hi tailcfg.HostinfoView) { + var oldReqTags []string + if node.Hostinfo.Valid() { + oldReqTags = node.Hostinfo.RequestTags().AsSlice() + } + newReqTags := hi.RequestTags().AsSlice() + if tagsEqualAnyOrder(oldReqTags, newReqTags) || tagsEqualAnyOrder(newReqTags, node.Tags) { + return + } + if s.validTagTransition(node.Tags, newReqTags) { + s.logf("testcontrol: retagging node %v: %v -> %v", node.ID, node.Tags, newReqTags) + node.Tags = newReqTags + } else { + s.logf("testcontrol: invalid tag transition %v -> %v for node %v; expiring its node key", node.Tags, newReqTags, node.ID) + node.KeyExpiry = time.Now().Add(-time.Minute) + } +} + +// validTagTransition reports whether a node currently tagged cur may retag +// itself as want, per s.TagOwners: every requested tag must be owned by one +// of the node's current tags. An untagged node may claim any defined tag, +// standing in for the production server's checks against the requesting +// user, which this server doesn't model. Removing all tags is not allowed, +// matching the production server. +func (s *Server) validTagTransition(cur, want []string) bool { + if len(want) == 0 { + return len(cur) == 0 + } + for _, tag := range want { + owners, ok := s.TagOwners[tag] + if !ok { + return false + } + if len(cur) == 0 { + continue + } + owned := slices.ContainsFunc(cur, func(c string) bool { return slices.Contains(owners, c) }) + if !owned { + return false + } + } + return true +} + +// tagsEqualAnyOrder reports whether a and b contain the same tags, ignoring order. +func tagsEqualAnyOrder(a, b []string) bool { + if len(a) != len(b) { + return false + } + as, bs := slices.Clone(a), slices.Clone(b) + slices.Sort(as) + slices.Sort(bs) + return slices.Equal(as, bs) +} + // InServeMap returns the number of clients currently in a MapRequest HTTP handler. func (s *Server) InServeMap() int { s.mu.Lock() @@ -1289,6 +1382,12 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi } s.mu.Unlock() + if s.HoldMapRequest != nil { + if done := s.HoldMapRequest(req); done != nil { + defer done() + } + } + if s.AltMapStream != nil { // The caller takes over the stream entirely; it must handle // keeping the HTTP response alive until ctx is done. @@ -1320,6 +1419,16 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi streamingNonUpdate := req.Stream && req.Version >= 68 var peersToUpdate []tailcfg.NodeID if !req.ReadOnly && !streamingNonUpdate { + if ctx.Err() != nil { + // The client canceled the request (say, its control client + // was shut down mid-request when "tailscale up" or a + // profile switch created a new one), so its contents may + // predate newer requests that were already processed. + // Don't apply its Hostinfo/endpoints; they may be stale. + s.logf("testcontrol: dropping canceled map update from %v", req.NodeKey.ShortString()) + http.Error(w, "request canceled", 400) + return + } endpoints := filterInvalidIPv6Endpoints(req.Endpoints) var hi tailcfg.HostinfoView var newDERP int @@ -1339,6 +1448,9 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi live.DiscoKey = req.DiscoKey live.Cap = req.Version if hi.Valid() { + if s.TagOwners != nil { + s.handleTagTransitionLocked(live, hi) + } live.Hostinfo = hi if newDERP != 0 { live.HomeDERP = newDERP