From 2917ea8d0e1b816ea80b4237d2adb25295984d87 Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Fri, 19 Dec 2025 12:22:19 -0600 Subject: [PATCH] ipn/ipnauth, safesocket: defer named pipe client's token retrieval until ipnserver needs it An error returned by net.Listener.Accept() causes the owning http.Server to shut down. With the deprecation of net.Error.Temporary(), there's no way for the http.Server to test whether the returned error is temporary / retryable or not (see golang/go#66252). Because of that, errors returned by (*safesocket.winIOPipeListener).Accept() cause the LocalAPI server (aka ipnserver.Server) to shut down, and tailscaled process to exit. While this might be acceptable in the case of non-recoverable errors, such as programmer errors, we shouldn't shut down the entire tailscaled process for client- or connection-specific errors, such as when we couldn't obtain the client's access token because the client attempts to connect at the Anonymous impersonation level. Instead, the LocalAPI server should gracefully handle these errors by denying access and returning a 401 Unauthorized to the client. In tailscale/tscert#15, we fixed a known bug where Caddy and other apps using tscert would attempt to connect at the Anonymous impersonation level and fail. However, we should also fix this on the tailscaled side to prevent a potential DoS, where a local app could deliberately open the Tailscale LocalAPI named pipe at the Anonymous impersonation level and cause tailscaled to exit. In this PR, we defer token retrieval until (*WindowsClientConn).Token() is called and propagate the returned token or error via ipnauth.GetConnIdentity() to ipnserver, which handles it the same way as other ipnauth-related errors. Fixes #18212 Fixes tailscale/tscert#13 Signed-off-by: Nick Khyl --- ipn/ipnauth/ipnauth_windows.go | 29 +++++-------- safesocket/pipe_windows.go | 74 ++++++++++++++++++++++++++------- safesocket/pipe_windows_test.go | 7 +++- 3 files changed, 76 insertions(+), 34 deletions(-) diff --git a/ipn/ipnauth/ipnauth_windows.go b/ipn/ipnauth/ipnauth_windows.go index 1138bc23d..e3ea448a8 100644 --- a/ipn/ipnauth/ipnauth_windows.go +++ b/ipn/ipnauth/ipnauth_windows.go @@ -25,6 +25,12 @@ func GetConnIdentity(logf logger.Logf, c net.Conn) (ci *ConnIdentity, err error) if !ok { return nil, fmt.Errorf("not a WindowsClientConn: %T", c) } + if err := wcc.CheckToken(); err != nil { + // Failure to obtain a token means the client cannot be authenticated. + // We don't care about the exact error, but it typically means the client + // attempted to connect at the Anonymous impersonation level. + return nil, fmt.Errorf("authentication failed: %w", err) + } ci.pid, err = wcc.ClientPID() if err != nil { return nil, err @@ -169,26 +175,13 @@ func (t *token) IsUID(uid ipn.WindowsUserID) bool { // WindowsToken returns the WindowsToken representing the security context // of the connection's client. func (ci *ConnIdentity) WindowsToken() (WindowsToken, error) { - var wcc *safesocket.WindowsClientConn - var ok bool - if wcc, ok = ci.conn.(*safesocket.WindowsClientConn); !ok { + wcc, ok := ci.conn.(*safesocket.WindowsClientConn) + if !ok { return nil, fmt.Errorf("not a WindowsClientConn: %T", ci.conn) } - - // We duplicate the token's handle so that the WindowsToken we return may have - // a lifetime independent from the original connection. - var h windows.Handle - if err := windows.DuplicateHandle( - windows.CurrentProcess(), - windows.Handle(wcc.Token()), - windows.CurrentProcess(), - &h, - 0, - false, - windows.DUPLICATE_SAME_ACCESS, - ); err != nil { + token, err := wcc.Token() + if err != nil { return nil, err } - - return newToken(windows.Token(h)), nil + return newToken(token), nil } diff --git a/safesocket/pipe_windows.go b/safesocket/pipe_windows.go index 582834165..2968542f2 100644 --- a/safesocket/pipe_windows.go +++ b/safesocket/pipe_windows.go @@ -10,6 +10,7 @@ "fmt" "net" "runtime" + "sync" "time" "github.com/tailscale/go-winio" @@ -49,7 +50,9 @@ func listen(path string) (net.Listener, error) { // embedded net.Conn must be a go-winio PipeConn. type WindowsClientConn struct { winioPipeConn - token windows.Token + tokenOnce sync.Once + token windows.Token // or zero, if we couldn't obtain the client's token + tokenErr error } // winioPipeConn is a subset of the interface implemented by the go-winio's @@ -79,12 +82,63 @@ func (conn *WindowsClientConn) ClientPID() (int, error) { return int(pid), nil } -// Token returns the Windows access token of the client user. -func (conn *WindowsClientConn) Token() windows.Token { - return conn.token +// CheckToken returns an error if the client user's access token could not be retrieved, +// for example when the client opens the pipe with an anonymous impersonation level. +// +// Deprecated: use [WindowsClientConn.Token] instead. +func (conn *WindowsClientConn) CheckToken() error { + _, err := conn.getToken() + return err +} + +// getToken returns the Windows access token of the client user, +// or an error if the token could not be retrieved, for example +// when the client opens the pipe with an anonymous impersonation level. +// +// The connection retains ownership of the returned token handle; +// the caller must not close it. +// +// TODO(nickkhyl): Remove this, along with [WindowsClientConn.CheckToken], +// once [ipnauth.ConnIdentity] is removed in favor of [ipnauth.Actor]. +func (conn *WindowsClientConn) getToken() (windows.Token, error) { + conn.tokenOnce.Do(func() { + conn.token, conn.tokenErr = clientUserAccessToken(conn.winioPipeConn) + }) + return conn.token, conn.tokenErr +} + +// Token returns the Windows access token of the client user, +// or an error if the token could not be retrieved, for example +// when the client opens the pipe with an anonymous impersonation level. +// +// The caller is responsible for closing the returned token handle. +func (conn *WindowsClientConn) Token() (windows.Token, error) { + token, err := conn.getToken() + if err != nil { + return 0, err + } + + var dupToken windows.Handle + if err := windows.DuplicateHandle( + windows.CurrentProcess(), + windows.Handle(token), + windows.CurrentProcess(), + &dupToken, + 0, + false, + windows.DUPLICATE_SAME_ACCESS, + ); err != nil { + return 0, err + } + return windows.Token(dupToken), nil } func (conn *WindowsClientConn) Close() error { + // Either wait for any pending [WindowsClientConn.Token] calls to complete, + // or ensure that the token will never be opened. + conn.tokenOnce.Do(func() { + conn.tokenErr = net.ErrClosed + }) if conn.token != 0 { conn.token.Close() conn.token = 0 @@ -110,17 +164,7 @@ func (lw *winIOPipeListener) Accept() (net.Conn, error) { conn.Close() return nil, fmt.Errorf("unexpected type %T from winio.ListenPipe listener (itself a %T)", conn, lw.Listener) } - - token, err := clientUserAccessToken(pipeConn) - if err != nil { - conn.Close() - return nil, err - } - - return &WindowsClientConn{ - winioPipeConn: pipeConn, - token: token, - }, nil + return &WindowsClientConn{winioPipeConn: pipeConn}, nil } func clientUserAccessToken(pc winioPipeConn) (windows.Token, error) { diff --git a/safesocket/pipe_windows_test.go b/safesocket/pipe_windows_test.go index 054781f23..8d9cbd19b 100644 --- a/safesocket/pipe_windows_test.go +++ b/safesocket/pipe_windows_test.go @@ -58,9 +58,14 @@ func TestExpectedWindowsTypes(t *testing.T) { if wcc.winioPipeConn.Fd() == 0 { t.Error("accepted conn had unexpected zero fd") } - if wcc.token == 0 { + tok, err := wcc.Token() + if err != nil { + t.Errorf("failed to retrieve client token: %v", err) + } + if tok == 0 { t.Error("accepted conn had unexpected zero token") } + tok.Close() s.Write([]byte("hello"))