ipn/store: improve FileStore.WriteState atomicity

If an error occurs with FileStore.WriteState, it should not record
the provided value as this results in an inconsistency between
what is cached in memory and what is stored on disk.

Also, update the documentation of StateStore.ReadState
to indicate that the returned value should be treated as immutable.
This property is assumed by the fact that FileStore.ReadState
returns the same slice of bytes for repeated calls to the same key.

Updates #cleanup

Signed-off-by: Joe Tsai <joetsai@digital-static.net>

clientupdate/clientupdate.go

@@ -631,24 +631,53 @@ func (up *Updater) updateMacAppStore() error {
 	return nil
 }
 
-// winMSIEnv is the environment variable that, if set, is the MSI file for the
-// update command to install. It's passed like this so we can stop the
-// tailscale.exe process from running before the msiexec process runs and tries
-// to overwrite ourselves.
-const winMSIEnv = "TS_UPDATE_WIN_MSI"
+const (
+	// winMSIEnv is the environment variable that, if set, is the MSI file for
+	// the update command to install. It's passed like this so we can stop the
+	// tailscale.exe process from running before the msiexec process runs and
+	// tries to overwrite ourselves.
+	winMSIEnv = "TS_UPDATE_WIN_MSI"
+	// winExePathEnv is the environment variable that is set along with
+	// winMSIEnv and carries the full path of the calling tailscale.exe binary.
+	// It is used to re-launch the GUI process (tailscale-ipn.exe) after
+	// install is complete.
+	winExePathEnv = "TS_UPDATE_WIN_EXE_PATH"
+)
 
 var (
-	verifyAuthenticode func(string) error // or nil on non-Windows
-	markTempFileFunc   func(string) error // or nil on non-Windows
+	verifyAuthenticode          func(string) error // or nil on non-Windows
+	markTempFileFunc            func(string) error // or nil on non-Windows
+	launchTailscaleAsWinGUIUser func(string) error // or nil on non-Windows
 )
 
 func (up *Updater) updateWindows() error {
 	if msi := os.Getenv(winMSIEnv); msi != "" {
+		// stdout/stderr from this part of the install could be lost since the
+		// parent tailscaled is replaced. Create a temp log file to have some
+		// output to debug with in case update fails.
+		close, err := up.switchOutputToFile()
+		if err != nil {
+			up.Logf("failed to create log file for installation: %v; proceeding with existing outputs", err)
+		} else {
+			defer close.Close()
+		}
+
 		up.Logf("installing %v ...", msi)
 		if err := up.installMSI(msi); err != nil {
 			up.Logf("MSI install failed: %v", err)
 			return err
 		}
+		up.Logf("relaunching tailscale-ipn.exe...")
+		exePath := os.Getenv(winExePathEnv)
+		if exePath == "" {
+			up.Logf("env var %q not passed to installer binary copy", winExePathEnv)
+			return fmt.Errorf("env var %q not passed to installer binary copy", winExePathEnv)
+		}
+		if err := launchTailscaleAsWinGUIUser(exePath); err != nil {
+			up.Logf("Failed to re-launch tailscale after update: %v", err)
+			return err
+		}
+
 		up.Logf("success.")
 		return nil
 	}
@@ -691,7 +720,7 @@ func (up *Updater) updateWindows() error {
 	up.Logf("authenticode verification succeeded")
 
 	up.Logf("making tailscale.exe copy to switch to...")
-	selfCopy, err := makeSelfCopy()
+	selfOrig, selfCopy, err := makeSelfCopy()
 	if err != nil {
 		return err
 	}
@@ -699,7 +728,7 @@ func (up *Updater) updateWindows() error {
 	up.Logf("running tailscale.exe copy for final install...")
 
 	cmd := exec.Command(selfCopy, "update")
-	cmd.Env = append(os.Environ(), winMSIEnv+"="+msiTarget)
+	cmd.Env = append(os.Environ(), winMSIEnv+"="+msiTarget, winExePathEnv+"="+selfOrig)
 	cmd.Stdout = up.Stderr
 	cmd.Stderr = up.Stderr
 	cmd.Stdin = os.Stdin
@@ -712,10 +741,35 @@ func (up *Updater) updateWindows() error {
 	panic("unreachable")
 }
 
+func (up *Updater) switchOutputToFile() (io.Closer, error) {
+	var logFilePath string
+	exePath, err := os.Executable()
+	if err != nil {
+		logFilePath = filepath.Join(os.TempDir(), "tailscale-updater.log")
+	} else {
+		logFilePath = strings.TrimSuffix(exePath, ".exe") + ".log"
+	}
+
+	up.Logf("writing update output to %q", logFilePath)
+	logFile, err := os.Create(logFilePath)
+	if err != nil {
+		return nil, err
+	}
+
+	up.Logf = func(m string, args ...any) {
+		fmt.Fprintf(logFile, m+"\n", args...)
+	}
+	up.Stdout = logFile
+	up.Stderr = logFile
+	return logFile, nil
+}
+
 func (up *Updater) installMSI(msi string) error {
 	var err error
 	for tries := 0; tries < 2; tries++ {
-		cmd := exec.Command("msiexec.exe", "/i", filepath.Base(msi), "/quiet", "/promptrestart", "/qn")
+		// TS_NOLAUNCH: don't automatically launch the app after install.
+		// We will launch it explicitly as the current GUI user afterwards.
+		cmd := exec.Command("msiexec.exe", "/i", filepath.Base(msi), "/quiet", "/promptrestart", "/qn", "TS_NOLAUNCH=true")
 		cmd.Dir = filepath.Dir(msi)
 		cmd.Stdout = up.Stdout
 		cmd.Stderr = up.Stderr
@@ -724,6 +778,7 @@ func (up *Updater) installMSI(msi string) error {
 		if err == nil {
 			break
 		}
+		up.Logf("Install attempt failed: %v", err)
 		uninstallVersion := version.Short()
 		if v := os.Getenv("TS_DEBUG_UNINSTALL_VERSION"); v != "" {
 			uninstallVersion = v
@@ -753,30 +808,30 @@ func msiUUIDForVersion(ver string) string {
 	return "{" + strings.ToUpper(uuid.NewSHA1(uuid.NameSpaceURL, []byte(msiURL)).String()) + "}"
 }
 
-func makeSelfCopy() (tmpPathExe string, err error) {
+func makeSelfCopy() (origPathExe, tmpPathExe string, err error) {
 	selfExe, err := os.Executable()
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	f, err := os.Open(selfExe)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	defer f.Close()
 	f2, err := os.CreateTemp("", "tailscale-updater-*.exe")
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 	if f := markTempFileFunc; f != nil {
 		if err := f(f2.Name()); err != nil {
-			return "", err
+			return "", "", err
 		}
 	}
 	if _, err := io.Copy(f2, f); err != nil {
 		f2.Close()
-		return "", err
+		return "", "", err
 	}
-	return f2.Name(), f2.Close()
+	return selfExe, f2.Name(), f2.Close()
 }
 
 func (up *Updater) downloadURLToFile(pathSrc, fileDst string) (ret error) {

clientupdate/clientupdate_windows.go

@@ -7,13 +7,20 @@
 package clientupdate
 
 import (
+	"os/exec"
+	"os/user"
+	"path/filepath"
+	"syscall"
+
 	"golang.org/x/sys/windows"
+	"tailscale.com/util/winutil"
 	"tailscale.com/util/winutil/authenticode"
 )
 
 func init() {
 	markTempFileFunc = markTempFileWindows
 	verifyAuthenticode = verifyTailscale
+	launchTailscaleAsWinGUIUser = launchTailscaleAsGUIUser
 }
 
 func markTempFileWindows(name string) error {
@@ -26,3 +33,25 @@ const certSubjectTailscale = "Tailscale Inc."
 func verifyTailscale(path string) error {
 	return authenticode.Verify(path, certSubjectTailscale)
 }
+
+func launchTailscaleAsGUIUser(exePath string) error {
+	exePath = filepath.Join(filepath.Dir(exePath), "tailscale-ipn.exe")
+
+	var token windows.Token
+	if u, err := user.Current(); err == nil && u.Name == "SYSTEM" {
+		sessionID := winutil.WTSGetActiveConsoleSessionId()
+		if sessionID != 0xFFFFFFFF {
+			if err := windows.WTSQueryUserToken(sessionID, &token); err != nil {
+				return err
+			}
+			defer token.Close()
+		}
+	}
+
+	cmd := exec.Command(exePath)
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		Token:      syscall.Token(token),
+		HideWindow: true,
+	}
+	return cmd.Start()
+}

cmd/k8s-operator/sts.go

@@ -9,7 +9,9 @@ import (
 	"context"
 	_ "embed"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"net/http"
 	"os"
 	"strings"
 
@@ -149,10 +151,16 @@ func (a *tailscaleSTSReconciler) Cleanup(ctx context.Context, logger *zap.Sugare
 		return false, fmt.Errorf("getting device info: %w", err)
 	}
 	if id != "" {
-		// TODO: handle case where the device is already deleted, but the secret
-		// is still around.
+		logger.Debugf("deleting device %s from control", string(id))
 		if err := a.tsClient.DeleteDevice(ctx, string(id)); err != nil {
-			return false, fmt.Errorf("deleting device: %w", err)
+			errResp := &tailscale.ErrResponse{}
+			if ok := errors.As(err, errResp); ok && errResp.Status == http.StatusNotFound {
+				logger.Debugf("device %s not found, likely because it has already been deleted from control", string(id))
+			} else {
+				return false, fmt.Errorf("deleting device: %w", err)
+			}
+		} else {
+			logger.Debugf("device %s deleted from control", string(id))
 		}
 	}
 

cmd/tailscale/cli/serve_legacy.go

@@ -164,6 +164,7 @@ type serveEnv struct {
 	tcp              string    // TCP port
 	tlsTerminatedTCP string    // a TLS terminated TCP port
 	subcmd           serveMode // subcommand
+	yes              bool      // update without prompt
 
 	lc localServeClient // localClient interface, specific to serve
 

cmd/tailscale/cli/serve_v2.go

@@ -128,7 +128,7 @@ func newServeV2Command(e *serveEnv, subcmd serveMode) *ffcli.Command {
 			fs.StringVar(&e.http, "http", "", "Expose an HTTP server at the specified port")
 			fs.StringVar(&e.tcp, "tcp", "", "Expose a TCP forwarder to forward raw TCP packets at the specified port")
 			fs.StringVar(&e.tlsTerminatedTCP, "tls-terminated-tcp", "", "Expose a TCP forwarder to forward TLS-terminated TCP packets at the specified port")
-
+			fs.BoolVar(&e.yes, "yes", false, "Update without interactive prompts")
 		}),
 		UsageFunc: usageFunc,
 		Subcommands: []*ffcli.Command{
@@ -679,13 +679,40 @@ func (e *serveEnv) removeWebServe(sc *ipn.ServeConfig, dnsName string, srvPort u
 		return errors.New("cannot remove web handler; currently serving TCP")
 	}
 
-	hp := ipn.HostPort(net.JoinHostPort(dnsName, strconv.Itoa(int(srvPort))))
-	if !sc.WebHandlerExists(hp, mount) {
+	portStr := strconv.Itoa(int(srvPort))
+	hp := ipn.HostPort(net.JoinHostPort(dnsName, portStr))
+
+	var targetExists bool
+	var mounts []string
+	// mount is deduced from e.setPath but it is ambiguous as
+	// to whether the user explicitly passed "/" or it was defaulted to.
+	if e.setPath == "" {
+		targetExists = sc.Web[hp] != nil && len(sc.Web[hp].Handlers) > 0
+		if targetExists {
+			for mount := range sc.Web[hp].Handlers {
+				mounts = append(mounts, mount)
+			}
+		}
+	} else {
+		targetExists = sc.WebHandlerExists(hp, mount)
+		mounts = []string{mount}
+	}
+
+	if !targetExists {
 		return errors.New("error: handler does not exist")
 	}
 
+	if len(mounts) > 1 {
+		msg := fmt.Sprintf("Are you sure you want to delete %d handlers under port %s?", len(mounts), portStr)
+		if !e.yes && !promptYesNo(msg) {
+			return nil
+		}
+	}
+
 	// delete existing handler, then cascade delete if empty
-	delete(sc.Web[hp].Handlers, mount)
+	for _, m := range mounts {
+		delete(sc.Web[hp].Handlers, m)
+	}
 	if len(sc.Web[hp].Handlers) == 0 {
 		delete(sc.Web, hp)
 		delete(sc.TCP, srvPort)

cmd/tailscale/cli/serve_v2_test.go

@@ -725,6 +725,40 @@ func TestServeDevConfigMutations(t *testing.T) {
 		wantErr: anyErr(),
 	})
 
+	add(step{
+		command: cmd("serve reset"),
+		want:    &ipn.ServeConfig{},
+	})
+
+	// start two handlers and turn them off in one command
+	add(step{
+		command: cmd("serve --https=4545 --set-path=/foo --bg localhost:3000"),
+		want: &ipn.ServeConfig{
+			TCP: map[uint16]*ipn.TCPPortHandler{4545: {HTTPS: true}},
+			Web: map[ipn.HostPort]*ipn.WebServerConfig{
+				"foo.test.ts.net:4545": {Handlers: map[string]*ipn.HTTPHandler{
+					"/foo": {Proxy: "http://127.0.0.1:3000"},
+				}},
+			},
+		},
+	})
+	add(step{
+		command: cmd("serve --https=4545 --set-path=/bar --bg localhost:3000"),
+		want: &ipn.ServeConfig{
+			TCP: map[uint16]*ipn.TCPPortHandler{4545: {HTTPS: true}},
+			Web: map[ipn.HostPort]*ipn.WebServerConfig{
+				"foo.test.ts.net:4545": {Handlers: map[string]*ipn.HTTPHandler{
+					"/foo": {Proxy: "http://127.0.0.1:3000"},
+					"/bar": {Proxy: "http://127.0.0.1:3000"},
+				}},
+			},
+		},
+	})
+	add(step{
+		command: cmd("serve --https=4545 --bg --yes localhost:3000 off"),
+		want:    &ipn.ServeConfig{},
+	})
+
 	lc := &fakeLocalServeClient{}
 	// And now run the steps above.
 	for i, st := range steps {

cmd/tailscale/cli/update.go

@@ -82,7 +82,14 @@ func confirmUpdate(ver string) bool {
 		return false
 	}
 
-	fmt.Printf("This will update Tailscale from %v to %v. Continue? [y/n] ", version.Short(), ver)
+	msg := fmt.Sprintf("This will update Tailscale from %v to %v. Continue?", version.Short(), ver)
+	return promptYesNo(msg)
+}
+
+// PromptYesNo takes a question and prompts the user to answer the
+// question with a yes or no. It appends a [y/n] to the message.
+func promptYesNo(msg string) bool {
+	fmt.Print(msg + " [y/n] ")
 	var resp string
 	fmt.Scanln(&resp)
 	resp = strings.ToLower(resp)

go.toolchain.rev

@@ -1 +1 @@
-d1c91593484a1db2d4de2564f2ef2669814af9c8
+56d25cd9a2efe0eee3945518fc532ec45912ccb2

ipn/store.go

@@ -68,8 +68,9 @@ func CurrentProfileKey(userID string) StateKey {
 
 // StateStore persists state, and produces it back on request.
 type StateStore interface {
-	// ReadState returns the bytes associated with ID. Returns (nil,
-	// ErrStateNotExist) if the ID doesn't have associated state.
+	// ReadState returns the bytes associated with ID.
+	// It returns (nil, ErrStateNotExist) if the ID doesn't have associated state.
+	// The returned value must not be mutated.
 	ReadState(id StateKey) ([]byte, error)
 	// WriteState saves bs as the state associated with ID.
 	//

ipn/store/stores.go

@@ -173,16 +173,22 @@ func (s *FileStore) ReadState(id ipn.StateKey) ([]byte, error) {
 }
 
 // WriteState implements the StateStore interface.
-func (s *FileStore) WriteState(id ipn.StateKey, bs []byte) error {
+func (s *FileStore) WriteState(id ipn.StateKey, bs []byte) (err error) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	if bytes.Equal(s.cache[id], bs) {
+	bs0 := s.cache[id]
+	if bytes.Equal(bs0, bs) {
 		return nil
 	}
+	defer func() {
+		if err != nil {
+			s.cache[id] = bs0
+		}
+	}()
 	s.cache[id] = bytes.Clone(bs)
-	bs, err := json.MarshalIndent(s.cache, "", "  ")
+	b, err := json.MarshalIndent(s.cache, "", "  ")
 	if err != nil {
 		return err
 	}
-	return atomicfile.WriteFile(s.path, bs, 0600)
+	return atomicfile.WriteFile(s.path, b, 0600)
 }