diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index c2b55814e..de0cdbe60 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -24,6 +24,14 @@ jobs: - name: Checkout code uses: actions/checkout@v2 + - name: Restore Cache + uses: actions/cache@v2 + with: + path: ~/go/pkg/mod + key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ runner.os }}-go- + - name: Test run: go test ./... diff --git a/VERSION b/VERSION deleted file mode 100644 index 9a3c499a6..000000000 --- a/VERSION +++ /dev/null @@ -1 +0,0 @@ -1.2.0 bb058703ee682490124a8a9f93919c4b3a3c991d diff --git a/VERSION.txt b/VERSION.txt new file mode 100644 index 000000000..4c902a289 --- /dev/null +++ b/VERSION.txt @@ -0,0 +1 @@ +1.1.0 f81233524fddeec450940af8dc1a0dd8841bf28c diff --git a/cmd/derper/derper.go b/cmd/derper/derper.go index 9fafe210c..f328932a2 100644 --- a/cmd/derper/derper.go +++ b/cmd/derper/derper.go @@ -63,7 +63,7 @@ func loadConfig() config { } b, err := ioutil.ReadFile(*configPath) switch { - case os.IsNotExist(err): + case errors.Is(err, os.ErrNotExist): return writeNewConfig() case err != nil: log.Fatal(err) diff --git a/ipn/backend.go b/ipn/backend.go index fe591a0ab..8042b6625 100644 --- a/ipn/backend.go +++ b/ipn/backend.go @@ -21,6 +21,7 @@ type State int const ( NoState = State(iota) + InUseOtherUser NeedsLogin NeedsMachineAuth Stopped @@ -33,8 +34,14 @@ const ( const GoogleIDTokenType = "ts_android_google_login" func (s State) String() string { - return [...]string{"NoState", "NeedsLogin", "NeedsMachineAuth", - "Stopped", "Starting", "Running"}[s] + return [...]string{ + "NoState", + "InUseOtherUser", + "NeedsLogin", + "NeedsMachineAuth", + "Stopped", + "Starting", + "Running"}[s] } // EngineStatus contains WireGuard engine stats. @@ -53,7 +60,7 @@ type EngineStatus struct { type Notify struct { _ structs.Incomparable Version string // version number of IPN backend - ErrMessage *string // critical error message, if any + ErrMessage *string // critical error message, if any; for InUseOtherUser, the details LoginFinished *empty.Message // event: non-nil when login process succeeded State *State // current IPN state has changed Prefs *Prefs // preferences were changed diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index cb76ba5a4..6df546bae 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "io" + "io/ioutil" "log" "net" "net/http" @@ -102,10 +103,11 @@ type server struct { bs *ipn.BackendServer mu sync.Mutex - serverModeUser *user.User // or nil if not in server mode - lastUserID string // tracks last userid; on change, Reset state for paranoia - allClients map[net.Conn]connIdentity // HTTP or IPN - clients map[net.Conn]bool // subset of allClients; only IPN protocol + serverModeUser *user.User // or nil if not in server mode + lastUserID string // tracks last userid; on change, Reset state for paranoia + allClients map[net.Conn]connIdentity // HTTP or IPN + clients map[net.Conn]bool // subset of allClients; only IPN protocol + disconnectSub map[chan<- struct{}]struct{} // keys are subscribers of disconnects } // connIdentity represents the owner of a localhost TCP connection. @@ -177,6 +179,42 @@ func (s *server) lookupUserFromID(uid string) (*user.User, error) { return u, err } +// blockWhileInUse blocks while until either a Read from conn fails +// (i.e. it's closed) or until the server is able to accept ci as a +// user. +func (s *server) blockWhileInUse(conn io.Reader, ci connIdentity) { + s.logf("blocking client while server in use; connIdentity=%v", ci) + connDone := make(chan struct{}) + go func() { + io.Copy(ioutil.Discard, conn) + close(connDone) + }() + ch := make(chan struct{}, 1) + s.registerDisconnectSub(ch, true) + defer s.registerDisconnectSub(ch, false) + for { + select { + case <-connDone: + s.logf("blocked client Read completed; connIdentity=%v", ci) + return + case <-ch: + s.mu.Lock() + err := s.checkConnIdentityLocked(ci) + s.mu.Unlock() + if err == nil { + s.logf("unblocking client, server is free; connIdentity=%v", ci) + // Server is now available again for a new user. + // TODO(bradfitz): keep this connection alive. But for + // now just return and have our caller close the connection + // (which unblocks the io.Copy goroutine we started above) + // and then the client (e.g. Windows) will reconnect and + // discover that it works. + return + } + } + } +} + func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) { // First see if it's an HTTP request. br := bufio.NewReader(c) @@ -195,8 +233,14 @@ func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) { defer c.Close() serverToClient := func(b []byte) { ipn.WriteMsg(c, b) } bs := ipn.NewBackendServer(logf, nil, serverToClient) - bs.SendErrorMessage(err.Error()) - time.Sleep(time.Second) + _, occupied := err.(inUseOtherUserError) + if occupied { + bs.SendInUseOtherUserErrorMessage(err.Error()) + s.blockWhileInUse(c, ci) + } else { + bs.SendErrorMessage(err.Error()) + time.Sleep(time.Second) + } return } @@ -243,6 +287,58 @@ func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) { } } +// inUseOtherUserError is the error type for when the server is in use +// by a different local user. +type inUseOtherUserError struct{ error } + +func (e inUseOtherUserError) Unwrap() error { return e.error } + +// checkConnIdentityLocked checks whether the provided identity is +// allowed to connect to the server. +// +// The returned error, when non-nil, will be of type inUseOtherUserError. +// +// s.mu must be held. +func (s *server) checkConnIdentityLocked(ci connIdentity) error { + // If clients are already connected, verify they're the same user. + // This mostly matters on Windows at the moment. + if len(s.allClients) > 0 { + var active connIdentity + for _, active = range s.allClients { + break + } + if ci.UserID != active.UserID { + //lint:ignore ST1005 we want to capitalize Tailscale here + return inUseOtherUserError{fmt.Errorf("Tailscale already in use by %s, pid %d", active.User.Username, active.Pid)} + } + } + if su := s.serverModeUser; su != nil && ci.UserID != su.Uid { + //lint:ignore ST1005 we want to capitalize Tailscale here + return inUseOtherUserError{fmt.Errorf("Tailscale already in use by %s", su.Username)} + } + return nil +} + +// registerDisconnectSub adds ch as a subscribe to connection disconnect +// events. If add is false, the subscriber is removed. +func (s *server) registerDisconnectSub(ch chan<- struct{}, add bool) { + s.mu.Lock() + defer s.mu.Unlock() + if add { + if s.disconnectSub == nil { + s.disconnectSub = make(map[chan<- struct{}]struct{}) + } + s.disconnectSub[ch] = struct{}{} + } else { + delete(s.disconnectSub, ch) + } + +} + +// addConn adds c to the server's list of clients. +// +// If the returned error is of type inUseOtherUserError then the +// returned connIdentity is also valid. func (s *server) addConn(c net.Conn, isHTTP bool) (ci connIdentity, err error) { ci, err = s.getConnIdentity(c) if err != nil { @@ -271,21 +367,8 @@ func (s *server) addConn(c net.Conn, isHTTP bool) (ci connIdentity, err error) { s.allClients = map[net.Conn]connIdentity{} } - // If clients are already connected, verify they're the same user. - // This mostly matters on Windows at the moment. - if len(s.allClients) > 0 { - var active connIdentity - for _, active = range s.allClients { - break - } - if ci.UserID != active.UserID { - //lint:ignore ST1005 we want to capitalize Tailscale here - return ci, fmt.Errorf("Tailscale already in use by %s, pid %d", active.User.Username, active.Pid) - } - } - if su := s.serverModeUser; su != nil && ci.UserID != su.Uid { - //lint:ignore ST1005 we want to capitalize Tailscale here - return ci, fmt.Errorf("Tailscale running in server mode as %s. Access denied.", su.Username) + if err := s.checkConnIdentityLocked(ci); err != nil { + return ci, err } if !isHTTP { @@ -307,10 +390,16 @@ func (s *server) removeAndCloseConn(c net.Conn) { delete(s.clients, c) delete(s.allClients, c) remain := len(s.allClients) + for sub := range s.disconnectSub { + select { + case sub <- struct{}{}: + default: + } + } s.mu.Unlock() if remain == 0 && s.resetOnZero { - if s.b.RunningAndDaemonForced() { + if s.b.InServerMode() { s.logf("client disconnected; staying alive in server mode") } else { s.logf("client disconnected; stopping server") @@ -358,7 +447,7 @@ func (s *server) setServerModeUserLocked() { } func (s *server) writeToClients(b []byte) { - inServerMode := s.b.RunningAndDaemonForced() + inServerMode := s.b.InServerMode() s.mu.Lock() defer s.mu.Unlock() @@ -456,7 +545,7 @@ func Run(ctx context.Context, logf logger.Logf, logid string, getEngine func() ( key := string(autoStartKey) if strings.HasPrefix(key, "user-") { uid := strings.TrimPrefix(key, "user-") - u, err := user.LookupId(uid) + u, err := server.lookupUserFromID(uid) if err != nil { logf("ipnserver: found server mode auto-start key %q; failed to load user: %v", key, err) } else { diff --git a/ipn/local.go b/ipn/local.go index 02dc84feb..0b5d1cf82 100644 --- a/ipn/local.go +++ b/ipn/local.go @@ -81,6 +81,7 @@ type LocalBackend struct { stateKey StateKey // computed in part from user-provided value userID string // current controlling user ID (for Windows, primarily) prefs *Prefs + inServerMode bool machinePrivKey wgcfg.PrivateKey state State // hostinfo is mutated in-place while mu is held. @@ -414,9 +415,11 @@ func (b *LocalBackend) Start(opts Options) error { return fmt.Errorf("loading requested state: %v", err) } + b.inServerMode = b.prefs.ForceDaemon b.serverURL = b.prefs.ControlURL hostinfo.RoutableIPs = append(hostinfo.RoutableIPs, b.prefs.AdvertiseRoutes...) hostinfo.RequestTags = append(hostinfo.RequestTags, b.prefs.AdvertiseTags...) + b.logf("Start: serverMode=%v; stateKey=%q; tags=%q; routes=%v; url=%v", b.inServerMode, b.stateKey, b.prefs.AdvertiseTags, b.prefs.AdvertiseRoutes, b.prefs.ControlURL) applyPrefsToHostinfo(hostinfo, b.prefs) b.notify = opts.Notify @@ -708,7 +711,9 @@ func (b *LocalBackend) initMachineKeyLocked() (err error) { if err != nil { return } - b.prefs.Persist.LegacyFrontendPrivateMachineKey = b.machinePrivKey + if b.prefs != nil && b.prefs.Persist != nil { + b.prefs.Persist.LegacyFrontendPrivateMachineKey = b.machinePrivKey + } }() } @@ -764,6 +769,35 @@ func (b *LocalBackend) initMachineKeyLocked() (err error) { return nil } +// writeServerModeStartState stores the ServerModeStartKey value based on the current +// user and prefs. If userID is blank or prefs is blank, no work is done. +// +// b.mu may either be held or not. +func (b *LocalBackend) writeServerModeStartState(userID string, prefs *Prefs) { + if userID == "" || prefs == nil { + return + } + + if prefs.ForceDaemon { + stateKey := StateKey("user-" + userID) + if err := b.store.WriteState(ServerModeStartKey, []byte(stateKey)); err != nil { + b.logf("WriteState error: %v", err) + } + // It's important we do this here too, even if it looks + // redundant with the one in the 'if stateKey != ""' + // check block above. That one won't fire in the case + // where the Windows client started up in client mode. + // This happens when we transition into server mode: + if err := b.store.WriteState(stateKey, prefs.ToBytes()); err != nil { + b.logf("WriteState error: %v", err) + } + } else { + if err := b.store.WriteState(ServerModeStartKey, nil); err != nil { + b.logf("WriteState error: %v", err) + } + } +} + // loadStateLocked sets b.prefs and b.stateKey based on a complex // combination of key, prefs, and legacyPath. b.mu must be held when // calling. @@ -784,6 +818,7 @@ func (b *LocalBackend) loadStateLocked(key StateKey, prefs *Prefs, legacyPath st return fmt.Errorf("initMachineKeyLocked: %w", err) } b.stateKey = "" + b.writeServerModeStartState(b.userID, b.prefs) return nil } @@ -803,8 +838,8 @@ func (b *LocalBackend) loadStateLocked(key StateKey, prefs *Prefs, legacyPath st if legacyPath != "" { b.prefs, err = LoadPrefs(legacyPath) if err != nil { - if !os.IsNotExist(err) { - b.logf("Failed to load legacy prefs: %v", err) + if !errors.Is(err, os.ErrNotExist) { + b.logf("failed to load legacy prefs: %v", err) } b.prefs = NewPrefs() } else { @@ -841,13 +876,10 @@ func (b *LocalBackend) State() State { return b.state } -// RunningAndDaemonForced reports whether the backend is currently -// running and the preferences say that Tailscale should run in -// "server mode" (ForceDaemon). -func (b *LocalBackend) RunningAndDaemonForced() bool { +func (b *LocalBackend) InServerMode() bool { b.mu.Lock() defer b.mu.Unlock() - return b.state == Running && b.prefs != nil && b.prefs.ForceDaemon + return b.inServerMode } // getEngineStatus returns a copy of b.engineStatus. @@ -999,6 +1031,7 @@ func (b *LocalBackend) SetPrefs(newp *Prefs) { oldp := b.prefs newp.Persist = oldp.Persist // caller isn't allowed to override this b.prefs = newp + b.inServerMode = newp.ForceDaemon // We do this to avoid holding the lock while doing everything else. newp = b.prefs.Clone() @@ -1017,26 +1050,7 @@ func (b *LocalBackend) SetPrefs(newp *Prefs) { b.logf("Failed to save new controlclient state: %v", err) } } - if userID != "" { // e.g. on Windows - if newp.ForceDaemon { - stateKey := StateKey("user-" + userID) - if err := b.store.WriteState(ServerModeStartKey, []byte(stateKey)); err != nil { - b.logf("WriteState error: %v", err) - } - // It's important we do this here too, even if it looks - // redundant with the one in the 'if stateKey != ""' - // check block above. That one won't fire in the case - // where the Windows client started up in client mode. - // This happens when we transition into server mode: - if err := b.store.WriteState(stateKey, newp.ToBytes()); err != nil { - b.logf("WriteState error: %v", err) - } - } else { - if err := b.store.WriteState(ServerModeStartKey, nil); err != nil { - b.logf("WriteState error: %v", err) - } - } - } + b.writeServerModeStartState(userID, newp) // [GRINDER STATS LINE] - please don't remove (used for log parsing) b.logf("SetPrefs: %v", newp.Pretty()) @@ -1540,8 +1554,8 @@ func (b *LocalBackend) TestOnlyPublicKeys() (machineKey tailcfg.MachineKey, node // clients. We can't do that until 1.0.x is no longer supported. func temporarilySetMachineKeyInPersist() bool { //lint:ignore S1008 for comments - if runtime.GOOS == "darwin" { - // iOS and macOS users can't downgrade anyway. + if runtime.GOOS == "darwin" || runtime.GOOS == "android" { + // iOS, macOS, Android users can't downgrade anyway. return false } return true diff --git a/ipn/message.go b/ipn/message.go index 77bb9deaa..2ff7f7432 100644 --- a/ipn/message.go +++ b/ipn/message.go @@ -92,6 +92,17 @@ func (bs *BackendServer) SendErrorMessage(msg string) { bs.send(Notify{ErrMessage: &msg}) } +// SendInUseOtherUserErrorMessage sends a Notify message to the client that +// both sets the state to 'InUseOtherUser' and sets the associated reason +// to msg. +func (bs *BackendServer) SendInUseOtherUserErrorMessage(msg string) { + inUse := InUseOtherUser + bs.send(Notify{ + State: &inUse, + ErrMessage: &msg, + }) +} + // GotCommandMsg parses the incoming message b as a JSON Command and // calls GotCommand with it. func (bs *BackendServer) GotCommandMsg(b []byte) error { diff --git a/ipn/prefs_test.go b/ipn/prefs_test.go index 767686cdf..21e8e7b44 100644 --- a/ipn/prefs_test.go +++ b/ipn/prefs_test.go @@ -5,8 +5,12 @@ package ipn import ( + "errors" + "fmt" + "os" "reflect" "testing" + "time" "github.com/tailscale/wireguard-go/wgcfg" "tailscale.com/control/controlclient" @@ -330,3 +334,14 @@ func TestPrefsPretty(t *testing.T) { } } } + +func TestLoadPrefsNotExist(t *testing.T) { + bogusFile := fmt.Sprintf("/tmp/not-exist-%d", time.Now().UnixNano()) + + p, err := LoadPrefs(bogusFile) + if errors.Is(err, os.ErrNotExist) { + // expected. + return + } + t.Fatalf("unexpected prefs=%#v, err=%v", p, err) +} diff --git a/net/tshttpproxy/tshttpproxy_windows.go b/net/tshttpproxy/tshttpproxy_windows.go index 90f2b3f95..bd85d2104 100644 --- a/net/tshttpproxy/tshttpproxy_windows.go +++ b/net/tshttpproxy/tshttpproxy_windows.go @@ -19,6 +19,7 @@ import ( "github.com/alexbrainman/sspi/negotiate" "golang.org/x/sys/windows" + "tailscale.com/types/logger" ) var ( @@ -38,6 +39,13 @@ var cachedProxy struct { val *url.URL } +// proxyErrorf is a rate-limited logger specifically for errors asking +// WinHTTP for the proxy information. We don't want to log about +// errors often, otherwise the log message itself will generate a new +// HTTP request which ultimately will call back into us to log again, +// forever. So for errors, we only log a bit. +var proxyErrorf = logger.RateLimitedFn(log.Printf, 10*time.Minute, 2 /* burst*/, 10 /* maxCache */) + func proxyFromWinHTTPOrCache(req *http.Request) (*url.URL, error) { if req.URL == nil { return nil, nil @@ -79,7 +87,14 @@ func proxyFromWinHTTPOrCache(req *http.Request) (*url.URL, error) { setNoProxyUntil(10 * time.Second) return nil, nil } - log.Printf("tshttpproxy: winhttp: GetProxyForURL(%q): %v/%#v", urlStr, err, err) + if err == windows.ERROR_INVALID_PARAMETER { + // Seen on Windows 8.1. (https://github.com/tailscale/tailscale/issues/879) + // TODO(bradfitz): figure this out. + setNoProxyUntil(time.Hour) + proxyErrorf("tshttpproxy: winhttp: GetProxyForURL(%q): ERROR_INVALID_PARAMETER [unexpected]", urlStr) + return nil, nil + } + proxyErrorf("tshttpproxy: winhttp: GetProxyForURL(%q): %v/%#v", urlStr, err, err) if err == syscall.Errno(ERROR_WINHTTP_UNABLE_TO_DOWNLOAD_SCRIPT) { setNoProxyUntil(10 * time.Second) return nil, nil @@ -88,7 +103,7 @@ func proxyFromWinHTTPOrCache(req *http.Request) (*url.URL, error) { case <-ctx.Done(): cachedProxy.Lock() defer cachedProxy.Unlock() - log.Printf("tshttpproxy: winhttp: GetProxyForURL(%q): timeout; using cached proxy %v", urlStr, cachedProxy.val) + proxyErrorf("tshttpproxy: winhttp: GetProxyForURL(%q): timeout; using cached proxy %v", urlStr, cachedProxy.val) return cachedProxy.val, nil } } @@ -96,7 +111,7 @@ func proxyFromWinHTTPOrCache(req *http.Request) (*url.URL, error) { func proxyFromWinHTTP(ctx context.Context, urlStr string) (proxy *url.URL, err error) { whi, err := winHTTPOpen() if err != nil { - log.Printf("winhttp: Open: %v", err) + proxyErrorf("winhttp: Open: %v", err) return nil, err } defer whi.Close() diff --git a/tsweb/tsweb.go b/tsweb/tsweb.go index 649abdfa0..134f034cf 100644 --- a/tsweb/tsweb.go +++ b/tsweb/tsweb.go @@ -157,11 +157,22 @@ type ReturnHandler interface { ServeHTTPReturn(http.ResponseWriter, *http.Request) error } +type HandlerOptions struct { + Quiet200s bool // if set, do not log successfully handled HTTP requests + Logf logger.Logf + Now func() time.Time // if nil, defaults to time.Now + + // If non-nil, StatusCodeCounters maintains counters + // of status codes for handled responses. + // The keys are "1xx", "2xx", "3xx", "4xx", and "5xx". + StatusCodeCounters *expvar.Map +} + // StdHandler converts a ReturnHandler into a standard http.Handler. // Handled requests are logged using logf, as are any errors. Errors // are handled as specified by the Handler interface. func StdHandler(h ReturnHandler, logf logger.Logf) http.Handler { - return stdHandler(h, logf, time.Now, true) + return StdHandlerOpts(h, HandlerOptions{Logf: logf, Now: time.Now}) } // ReturnHandlerFunc is an adapter to allow the use of ordinary @@ -178,27 +189,32 @@ func (f ReturnHandlerFunc) ServeHTTPReturn(w http.ResponseWriter, r *http.Reques // StdHandlerNo200s is like StdHandler, but successfully handled HTTP // requests don't write an access log entry to logf. // -// TODO(danderson): quick stopgap, probably want ...Options on StdHandler instead? +// TODO(josharian): eliminate this and StdHandler in favor of StdHandlerOpts, +// rename StdHandlerOpts to StdHandler. Will be a breaking API change. func StdHandlerNo200s(h ReturnHandler, logf logger.Logf) http.Handler { - return stdHandler(h, logf, time.Now, false) + return StdHandlerOpts(h, HandlerOptions{Logf: logf, Now: time.Now, Quiet200s: true}) } -func stdHandler(h ReturnHandler, logf logger.Logf, now func() time.Time, log200s bool) http.Handler { - return retHandler{h, logf, now, log200s} +// StdHandlerOpts converts a ReturnHandler into a standard http.Handler. +// Handled requests are logged using opts.Logf, as are any errors. +// Errors are handled as specified by the Handler interface. +func StdHandlerOpts(h ReturnHandler, opts HandlerOptions) http.Handler { + if opts.Now == nil { + opts.Now = time.Now + } + return retHandler{h, opts} } // retHandler is an http.Handler that wraps a Handler and handles errors. type retHandler struct { - rh ReturnHandler - logf logger.Logf - timeNow func() time.Time - log200s bool + rh ReturnHandler + opts HandlerOptions } // ServeHTTP implements the http.Handler interface. func (h retHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { msg := AccessLogRecord{ - When: h.timeNow(), + When: h.opts.Now(), RemoteAddr: r.RemoteAddr, Proto: r.Proto, TLS: r.TLS != nil, @@ -209,7 +225,7 @@ func (h retHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { Referer: r.Referer(), } - lw := &loggingResponseWriter{ResponseWriter: w, logf: h.logf} + lw := &loggingResponseWriter{ResponseWriter: w, logf: h.opts.Logf} err := h.rh.ServeHTTPReturn(lw, r) hErr, hErrOK := err.(HTTPError) @@ -219,7 +235,7 @@ func (h retHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { lw.code = 200 } - msg.Seconds = h.timeNow().Sub(msg.When).Seconds() + msg.Seconds = h.opts.Now().Sub(msg.When).Seconds() msg.Code = lw.code msg.Bytes = lw.bytes @@ -245,12 +261,12 @@ func (h retHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } if lw.code != 0 { - h.logf("[unexpected] handler returned HTTPError %v, but already sent a response with code %d", hErr, lw.code) + h.opts.Logf("[unexpected] handler returned HTTPError %v, but already sent a response with code %d", hErr, lw.code) break } msg.Code = hErr.Code if msg.Code == 0 { - h.logf("[unexpected] HTTPError %v did not contain an HTTP status code, sending internal server error", hErr) + h.opts.Logf("[unexpected] HTTPError %v did not contain an HTTP status code, sending internal server error", hErr) msg.Code = http.StatusInternalServerError } http.Error(lw, hErr.Msg, msg.Code) @@ -264,8 +280,13 @@ func (h retHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } - if msg.Code != 200 || h.log200s { - h.logf("%s", msg) + if msg.Code != 200 || !h.opts.Quiet200s { + h.opts.Logf("%s", msg) + } + + if h.opts.StatusCodeCounters != nil { + key := fmt.Sprintf("%dxx", msg.Code/100) + h.opts.StatusCodeCounters.Add(key, 1) } } diff --git a/tsweb/tsweb_test.go b/tsweb/tsweb_test.go index 2e94ef752..4d4651e5c 100644 --- a/tsweb/tsweb_test.go +++ b/tsweb/tsweb_test.go @@ -248,7 +248,7 @@ func TestStdHandler(t *testing.T) { clock.Reset() rec := noopHijacker{httptest.NewRecorder(), false} - h := stdHandler(test.rh, logf, clock.Now, true) + h := StdHandlerOpts(test.rh, HandlerOptions{Logf: logf, Now: clock.Now}) h.ServeHTTP(&rec, test.r) res := rec.Result() if res.StatusCode != test.wantCode { diff --git a/version/.gitignore b/version/.gitignore index 60df6e2d6..58d19bfc2 100644 --- a/version/.gitignore +++ b/version/.gitignore @@ -3,6 +3,7 @@ long.txt short.txt gitcommit.txt extragitcommit.txt +version-info.sh version.h version.xcconfig ver.go diff --git a/version/all.do b/version/all.do index 2c0f8ce8b..d0bdd8ead 100644 --- a/version/all.do +++ b/version/all.do @@ -1,3 +1,2 @@ - redo-ifchange ver.go version.xcconfig version.h diff --git a/version/describe.sh b/version/describe.sh deleted file mode 100755 index 5e12aa266..000000000 --- a/version/describe.sh +++ /dev/null @@ -1,26 +0,0 @@ -#!/bin/sh -# -# Constructs a "git describe" compatible version number by using the -# information in the VERSION file, rather than git tags. - -set -eu - -dir="$(dirname $0)" -verfile="$dir/../VERSION" - -read -r version hash <"$verfile" - -if [ -z "$hash" ]; then - # If no explicit hash was given, use the last time the version - # file changed as the "origin" hash for this version. - hash="$(git rev-list --max-count=1 HEAD -- $verfile)" -fi - -if [ -z "$hash" ]; then - echo "Couldn't find base git hash for version '$version'" >2 - exit 1 -fi - -head="$(git rev-parse --short=9 HEAD)" -changecount="$(git rev-list ${hash}..HEAD | wc -l)" -echo "v${version}-${changecount}-g${head}" diff --git a/version/extragitcommit.txt.do b/version/extragitcommit.txt.do deleted file mode 100644 index d494fd04f..000000000 --- a/version/extragitcommit.txt.do +++ /dev/null @@ -1,6 +0,0 @@ -# --abbrev=200 is an arbitrary large number to capture the entire git -# hash without trying to compact it. -commit=$(cd ../.. && git describe --dirty --exclude "*" --always --abbrev=200) -echo "$commit" >$3 -redo-always -redo-stamp <$3 diff --git a/version/gitcommit.txt.do b/version/gitcommit.txt.do deleted file mode 100644 index f943fa8bf..000000000 --- a/version/gitcommit.txt.do +++ /dev/null @@ -1,6 +0,0 @@ -# --abbrev=200 is an arbitrary large number to capture the entire git -# hash without trying to compact it. -commit=$(git describe --dirty --exclude "*" --always --abbrev=200) -echo "$commit" >$3 -redo-always -redo-stamp <$3 diff --git a/version/long.txt.do b/version/long.txt.do deleted file mode 100644 index 96bd18377..000000000 --- a/version/long.txt.do +++ /dev/null @@ -1,5 +0,0 @@ -redo-ifchange mkversion.sh describe.txt extragitcommit.txt -read -r describe $3 diff --git a/version/mkversion.sh b/version/mkversion.sh deleted file mode 100755 index a5768c040..000000000 --- a/version/mkversion.sh +++ /dev/null @@ -1,139 +0,0 @@ -#!/bin/sh - -set -eu - -mode=$1 -describe=$2 -other=$3 - -# Git describe output overall looks like -# MAJOR.MINOR.PATCH-NUMCOMMITS-GITHASH. Depending on the tag being -# described and the state of the repo, ver can be missing PATCH, -# NUMCOMMITS, or both. -# -# Valid values look like: 1.2.3-1234-abcdef, 0.98-1234-abcdef, -# 1.0.0-abcdef, 0.99-abcdef. -ver="${describe#v}" -stem="${ver%%-*}" # Just the semver-ish bit e.g. 1.2.3, 0.98 -suffix="${ver#$stem}" # The rest e.g. -23-abcdef, -abcdef - -# Normalize the stem into a full major.minor.patch semver. We might -# not use all those pieces depending on what kind of version we're -# making, but it's good to have them all on hand. -case "$stem" in - *.*.*) - # Full SemVer, nothing to do - stem="$stem" - ;; - *.*) - # Old style major.minor, add a .0 - stem="${stem}.0" - ;; - *) - echo "Unparseable version $stem" >&2 - exit 1 - ;; -esac -major=$(echo "$stem" | cut -f1 -d.) -minor=$(echo "$stem" | cut -f2 -d.) -patch=$(echo "$stem" | cut -f3 -d.) - -# Extract the change count and git ID from the suffix. -case "$suffix" in - -*-*) - # Has both a change count and a commit hash. - changecount=$(echo "$suffix" | cut -f2 -d-) - githash=$(echo "$suffix" | cut -f3 -d-) - ;; - -*) - # Git hash only, change count is zero. - changecount="0" - githash=$(echo "$suffix" | cut -f2 -d-) - ;; - *) - echo "Unparseable version suffix $suffix" >&2 - exit 1 - ;; -esac - -# The git hash is of the form "gCOMMITHASH". We want to replace the -# 'g' with a 't', for "tailscale", to convey that it's specifically -# the commit hash of the tailscale repo. -if [ -n "$githash" ]; then - # POSIX shell doesn't understand ${foo:1:9} syntax, gaaah. - githash="$(echo $githash | cut -c2-10)" - githash="t${githash}" -fi - -# "other" is a second git commit hash for another repository used to -# build the Tailscale code. In practice it's either the commit hash in -# the Android repository, or the commit hash of Tailscale's -# proprietary repository (which pins a bunch things like build scripts -# used and Go toolchain version). -if [ -n "$other" ]; then - other="$(echo $other | cut -c1-9)" - other="-o${other}" -fi - -# Validate that the version data makes sense. Rules: -# - Odd number minors are unstable. Patch must be 0, and gets -# replaced by changecount. -# - Even number minors are stable. Changecount must be 0, and -# gets removed. -# -# After this section, we only use major/minor/patch, which have been -# tweaked as needed. -if expr "$minor" : "[0-9]*[13579]$" >/dev/null; then - # Unstable - if [ "$patch" != "0" ]; then - # This is a fatal error, because a non-zero patch number - # indicates that we created an unstable git tag in violation - # of our versioning policy, and we want to blow up loudly to - # get that fixed. - echo "Unstable release $describe has a non-zero patch number, which is not allowed" >&2 - exit 1 - fi - patch="$changecount" -else - # Stable - if [ "$changecount" != "0" ]; then - # This is a commit that's sitting between two stable - # releases. We never want to release such a commit to the - # pbulic, but it's useful to be able to build it for - # debugging. Just force the version to 0.0.0, so that we're - # forced to rely on the git commit hash. - major="0" - minor="0" - patch="0" - fi -fi - -if [ "$minor" -eq 1 ]; then - # Hack for 1.1: add 1000 to the patch number, so that builds that - # use the OSS change count order after the builds that used the - # proprietary repo's changecount. Otherwise, the version numbers - # would go backwards and things would be unhappy. - patch=$((patch + 1000)) -fi - -case "$1" in - long) - echo "${major}.${minor}.${patch}-${githash}${other}" - ;; - short) - echo "${major}.${minor}.${patch}" - ;; - xcode) - # CFBundleShortVersionString: the "short name" used in the App - # Store. eg. 0.92.98 - echo "VERSION_NAME = ${major}.${minor}.${patch}" - # CFBundleVersion: the build number. Needs to be 3 numeric - # sections that increment for each release according to SemVer - # rules. - # - # We start counting at 100 because we submitted using raw - # build numbers before, and Apple doesn't let you start over. - # e.g. 0.98.3 -> 100.98.3 - echo "VERSION_ID = $((major + 100)).${minor}.${patch}" - ;; -esac diff --git a/version/mkversion_test.go b/version/mkversion_test.go index 00560bbf1..f4ac1fe69 100644 --- a/version/mkversion_test.go +++ b/version/mkversion_test.go @@ -8,22 +8,21 @@ import ( "fmt" "os/exec" "runtime" + "strconv" "strings" "testing" -) -func xcode(short, long string) string { - return fmt.Sprintf("VERSION_NAME = %s\nVERSION_ID = %s", short, long) -} + "github.com/google/go-cmp/cmp" +) -func mkversion(t *testing.T, mode, describe, other string) (string, bool) { +func mkversion(t *testing.T, gitHash, otherHash string, major, minor, patch, changeCount int) (string, bool) { t.Helper() - bs, err := exec.Command("./mkversion.sh", mode, describe, other).CombinedOutput() + bs, err := exec.Command("./version.sh", gitHash, otherHash, strconv.Itoa(major), strconv.Itoa(minor), strconv.Itoa(patch), strconv.Itoa(changeCount)).CombinedOutput() + out := strings.TrimSpace(string(bs)) if err != nil { - t.Logf("mkversion.sh output: %s", string(bs)) - return "", false + return out, false } - return strings.TrimSpace(string(bs)), true + return out, true } func TestMkversion(t *testing.T) { @@ -31,50 +30,73 @@ func TestMkversion(t *testing.T) { t.Skip("skip test on Windows, because there is no shell to execute mkversion.sh.") } tests := []struct { - describe string - other string - ok bool - long string - short string - xcode string + gitHash, otherHash string + major, minor, patch, changeCount int + want string }{ - {"v0.98-gabcdef", "", true, "0.98.0-tabcdef", "0.98.0", xcode("0.98.0", "100.98.0")}, - {"v0.98.1-gabcdef", "", true, "0.98.1-tabcdef", "0.98.1", xcode("0.98.1", "100.98.1")}, - {"v1.1.0-37-gabcdef", "", true, "1.1.1037-tabcdef", "1.1.1037", xcode("1.1.1037", "101.1.1037")}, - {"v1.2.9-gabcdef", "", true, "1.2.9-tabcdef", "1.2.9", xcode("1.2.9", "101.2.9")}, - {"v1.2.9-0-gabcdef", "", true, "1.2.9-tabcdef", "1.2.9", xcode("1.2.9", "101.2.9")}, - {"v1.15.0-129-gabcdef", "", true, "1.15.129-tabcdef", "1.15.129", xcode("1.15.129", "101.15.129")}, - - {"v0.98-123-gabcdef", "", true, "0.0.0-tabcdef", "0.0.0", xcode("0.0.0", "100.0.0")}, - {"v1.0.0-37-gabcdef", "", true, "0.0.0-tabcdef", "0.0.0", xcode("0.0.0", "100.0.0")}, - {"v1.1.0-129-gabcdef", "0123456789abcdef0123456789abcdef", true, "1.1.1129-tabcdef-o012345678", "1.1.1129", xcode("1.1.1129", "101.1.1129")}, - {"v0.99.5-0-gabcdef", "", false, "", "", ""}, // unstable, patch not allowed - {"v0.99.5-123-gabcdef", "", false, "", "", ""}, // unstable, patch not allowed - {"v1-gabcdef", "", false, "", "", ""}, // bad semver - {"v1.0", "", false, "", "", ""}, // missing suffix + {"abcdef", "", 0, 98, 0, 0, ` + VERSION_SHORT="0.98.0" + VERSION_LONG="0.98.0-tabcdef" + VERSION_GIT_HASH="abcdef" + VERSION_EXTRA_HASH="" + VERSION_XCODE="100.98.0" + VERSION_WINRES="0,98,0,0"`}, + {"abcdef", "", 0, 98, 1, 0, ` + VERSION_SHORT="0.98.1" + VERSION_LONG="0.98.1-tabcdef" + VERSION_GIT_HASH="abcdef" + VERSION_EXTRA_HASH="" + VERSION_XCODE="100.98.1" + VERSION_WINRES="0,98,1,0"`}, + {"abcdef", "", 1, 1, 0, 37, ` + VERSION_SHORT="1.1.1037" + VERSION_LONG="1.1.1037-tabcdef" + VERSION_GIT_HASH="abcdef" + VERSION_EXTRA_HASH="" + VERSION_XCODE="101.1.1037" + VERSION_WINRES="1,1,1037,0"`}, + {"abcdef", "", 1, 2, 9, 0, ` + VERSION_SHORT="1.2.9" + VERSION_LONG="1.2.9-tabcdef" + VERSION_GIT_HASH="abcdef" + VERSION_EXTRA_HASH="" + VERSION_XCODE="101.2.9" + VERSION_WINRES="1,2,9,0"`}, + {"abcdef", "", 1, 15, 0, 129, ` + VERSION_SHORT="1.15.129" + VERSION_LONG="1.15.129-tabcdef" + VERSION_GIT_HASH="abcdef" + VERSION_EXTRA_HASH="" + VERSION_XCODE="101.15.129" + VERSION_WINRES="1,15,129,0"`}, + {"abcdef", "", 1, 2, 0, 17, ` + VERSION_SHORT="0.0.0" + VERSION_LONG="0.0.0-tabcdef" + VERSION_GIT_HASH="abcdef" + VERSION_EXTRA_HASH="" + VERSION_XCODE="100.0.0" + VERSION_WINRES="0,0,0,0"`}, + {"abcdef", "defghi", 1, 15, 0, 129, ` + VERSION_SHORT="1.15.129" + VERSION_LONG="1.15.129-tabcdef-gdefghi" + VERSION_GIT_HASH="abcdef" + VERSION_EXTRA_HASH="defghi" + VERSION_XCODE="101.15.129" + VERSION_WINRES="1,15,129,0"`}, + {"abcdef", "", 0, 99, 5, 0, ""}, // unstable, patch number not allowed + {"abcdef", "", 0, 99, 5, 123, ""}, // unstable, patch number not allowed } for _, test := range tests { - gotlong, longOK := mkversion(t, "long", test.describe, test.other) - if longOK != test.ok { - t.Errorf("mkversion.sh long %q ok=%v, want %v", test.describe, longOK, test.ok) - } - gotshort, shortOK := mkversion(t, "short", test.describe, test.other) - if shortOK != test.ok { - t.Errorf("mkversion.sh short %q ok=%v, want %v", test.describe, shortOK, test.ok) - } - gotxcode, xcodeOK := mkversion(t, "xcode", test.describe, test.other) - if xcodeOK != test.ok { - t.Errorf("mkversion.sh xcode %q ok=%v, want %v", test.describe, xcodeOK, test.ok) - } - if longOK && gotlong != test.long { - t.Errorf("mkversion.sh long %q: got %q, want %q", test.describe, gotlong, test.long) - } - if shortOK && gotshort != test.short { - t.Errorf("mkversion.sh short %q: got %q, want %q", test.describe, gotshort, test.short) + want := strings.ReplaceAll(strings.TrimSpace(test.want), " ", "") + got, ok := mkversion(t, test.gitHash, test.otherHash, test.major, test.minor, test.patch, test.changeCount) + invoc := fmt.Sprintf("version.sh %s %s %d %d %d %d", test.gitHash, test.otherHash, test.major, test.minor, test.patch, test.changeCount) + if want == "" && ok { + t.Errorf("%s ok=true, want false", invoc) + continue } - if xcodeOK && gotxcode != test.xcode { - t.Errorf("mkversion.sh xcode %q: got %q, want %q", test.describe, gotxcode, test.xcode) + if diff := cmp.Diff(got, want); want != "" && diff != "" { + t.Errorf("%s wrong output (-got+want):\n%s", invoc, diff) } } } diff --git a/version/short.txt.do b/version/short.txt.do deleted file mode 100644 index ca9426b60..000000000 --- a/version/short.txt.do +++ /dev/null @@ -1,5 +0,0 @@ -redo-ifchange mkversion.sh describe.txt extragitcommit.txt -read -r describe $3 diff --git a/version/ver.go.do b/version/ver.go.do index 28a1bab79..6883fbdfc 100644 --- a/version/ver.go.do +++ b/version/ver.go.do @@ -1,12 +1,9 @@ -redo-ifchange long.txt short.txt gitcommit.txt extragitcommit.txt ver.go.in +redo-ifchange version-info.sh ver.go.in -read -r LONGVER $3 diff --git a/version/describe.txt.do b/version/version-info.sh.do similarity index 54% rename from version/describe.txt.do rename to version/version-info.sh.do index 9fb1095d6..f6e3554e2 100644 --- a/version/describe.txt.do +++ b/version/version-info.sh.do @@ -1,3 +1,3 @@ -./describe.sh >$3 +./version.sh ../.. >$3 redo-always redo-stamp <$3 diff --git a/version/version.h.do b/version/version.h.do index 7d1542b3e..7068f08e3 100644 --- a/version/version.h.do +++ b/version/version.h.do @@ -1,17 +1,9 @@ -redo-ifchange long.txt short.txt -read -r long $3 +cat >$3 <&2 + exit 1 + fi + + # Load the base version and optional corresponding git hash + # from the VERSION file. If there is no git hash in the file, + # we use the hash of the last change to the VERSION file. + version_file="$(dirname $0)/../VERSION.txt" + IFS=".$IFS" read -r major minor patch base_git_hash <"$version_file" + if [ -z "$base_git_hash" ]; then + base_git_hash=$(git rev-list --max-count=1 HEAD -- $version_file) + fi + + # The full git has we're currently building at. --abbrev=200 is an + # arbitrary large number larger than all currently-known hashes, so + # that git displays the full commit hash. + git_hash=$(git describe --always --dirty --exclude '*' --abbrev=200) + # The number of extra commits between the release base to git_hash. + change_count=$(git rev-list ${base_git_hash}..HEAD | wc -l) + ;; + 6) + # Test mode: rather than run git commands and whatnot, take in + # all the version pieces as arguments. + git_hash=$1 + extra_hash=$2 + major=$3 + minor=$4 + patch=$5 + change_count=$6 + ;; + *) + echo "Usage: $0 [extra-git-hash-or-checkout]" + exit 1 +esac + +# Shortened versions of git hashes, so that they fit neatly into an +# "elongated" but still human-readable version number. +short_git_hash=$(echo $git_hash | cut -c-9) +short_extra_hash=$(echo $extra_hash | cut -c-9) + +# Convert major/minor/patch/change_count into an adjusted +# major/minor/patch. This block is where all our policies on +# versioning are. +if expr "$minor" : "[0-9]*[13579]$" >/dev/null; then + # Odd minor numbers are unstable builds. + if [ "$patch" != "0" ]; then + # This is a fatal error, because a non-zero patch number + # indicates that we created an unstable git tag in violation + # of our versioning policy, and we want to blow up loudly to + # get that fixed. + echo "Unstable release $major.$minor.$patch has a non-zero patch number, which is not allowed" >&2 + exit 1 + fi + patch="$change_count" +elif [ "$change_count" != "0" ]; then + # Even minor numbers are stable builds, but stable builds are + # supposed to have a zero change count. Therefore, we're currently + # describing a commit that's on a release branch, but hasn't been + # tagged as a patch release yet. We allow these commits to build + # for testing purposes, but force their version number to 0.0.0, + # to reflect that they're an unreleasable build. The git hashes + # still completely describe the build commit, so we can still + # figure out what this build is if it escapes into the wild. + major="0" + minor="0" + patch="0" +fi + +# Hack for 1.1: add 1000 to the patch number. We switched from using +# the proprietary repo's change_count over to using the OSS repo's +# change_count, and this was necessary to avoid a backwards jump in +# release numbers. +if [ "$major.$minor" = "1.1" ]; then + patch="$((patch + 1000))" +fi + +# At this point, the version number correctly reflects our +# policies. All that remains is to output the various vars that other +# code can use to embed version data. +if [ -z "$extra_hash" ]; then + long_version_suffix="-t$short_git_hash" +else + long_version_suffix="-t${short_git_hash}-g${short_extra_hash}" +fi +cat <$3 +redo-ifchange version-info.sh + +. ./version-info.sh + +# CFBundleShortVersionString: the "short name" used in the App Store. +# eg. 0.92.98 +echo "VERSION_NAME = $VERSION_SHORT" +# CFBundleVersion: the build number. Needs to be 3 numeric sections +# that increment for each release according to SemVer rules. +# +# We start counting at 100 because we submitted using raw build +# numbers before, and Apple doesn't let you start over. e.g. 0.98.3 +# -> 100.98.3 +echo "VERSION_ID = $VERSION_XCODE" diff --git a/wgengine/router/dns/direct.go b/wgengine/router/dns/direct.go index 62814c5f6..bd1c03b9d 100644 --- a/wgengine/router/dns/direct.go +++ b/wgengine/router/dns/direct.go @@ -9,6 +9,7 @@ package dns import ( "bufio" "bytes" + "errors" "fmt" "io" "io/ioutil" @@ -130,7 +131,7 @@ func (m directManager) Up(config Config) error { contents, err := ioutil.ReadFile(resolvConf) // If the original did not exist, still back up an empty file. // The presence of a backup file is the way we know that Up ran. - if err != nil && !os.IsNotExist(err) { + if err != nil && !errors.Is(err, os.ErrNotExist) { return err } if err := atomicfile.WriteFile(backupConf, contents, 0644); err != nil { diff --git a/wgengine/router/dns/manager_windows.go b/wgengine/router/dns/manager_windows.go index 1de16b5c2..d1e7a2157 100644 --- a/wgengine/router/dns/manager_windows.go +++ b/wgengine/router/dns/manager_windows.go @@ -9,6 +9,7 @@ import ( "os/exec" "strings" "syscall" + "time" "github.com/tailscale/wireguard-go/tun" "golang.org/x/sys/windows/registry" @@ -96,12 +97,19 @@ func (m windowsManager) Up(config Config) error { // have changed, which makes the DNS settings actually take // effect. // - // This command can take a few seconds to run. - cmd := exec.Command("ipconfig", "/registerdns") - cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} - if err := cmd.Run(); err != nil { - return fmt.Errorf("running ipconfig /registerdns: %w", err) - } + // This command can take a few seconds to run, so run it async, best effort. + go func() { + t0 := time.Now() + m.logf("running ipconfig /registerdns ...") + cmd := exec.Command("ipconfig", "/registerdns") + cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} + d := time.Since(t0).Round(time.Millisecond) + if err := cmd.Run(); err != nil { + m.logf("error running ipconfig /registerdns after %v: %v", d, err) + } else { + m.logf("ran ipconfig /registerdns in %v", d) + } + }() return nil }