From eab6e9ea4e457b36d55f4bca064dfb6a0e9abc91 Mon Sep 17 00:00:00 2001 From: Elias Naur Date: Fri, 30 Oct 2020 13:30:13 +0100 Subject: [PATCH 01/15] ipn: don't temporarilySetMachineKeyInPersist for Android clients Without this change, newly installed Android clients crash on startup with panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9881b9f8] goroutine 29 [running]: tailscale.com/ipn.(*LocalBackend).initMachineKeyLocked.func1(0x50cb1b9c, 0x503c9a00) /home/elias/proj/tailscale/ipn/local.go:711 +0x2c tailscale.com/ipn.(*LocalBackend).initMachineKeyLocked(0x503c9a00, 0x0, 0x0) /home/elias/proj/tailscale/ipn/local.go:736 +0x728 tailscale.com/ipn.(*LocalBackend).loadStateLocked(0x503c9a00, 0x988be40e, 0xb, 0x0, 0x0, 0x0, 0x0, 0x0) /home/elias/proj/tailscale/ipn/local.go:817 +0x1e8 tailscale.com/ipn.(*LocalBackend).Start(0x503c9a00, 0x0, 0x0, 0x988be40e, 0xb, 0x0, 0x0, 0x0, 0x0, 0x0, ...) /home/elias/proj/tailscale/ipn/local.go:412 +0x200 main.(*backend).Start(...) /home/elias/proj/tailscale-android/cmd/tailscale/backend.go:116 main.(*App).runBackend.func3(0x50106340, 0x5000c060, 0x50d9a280) /home/elias/proj/tailscale-android/cmd/tailscale/main.go:169 +0x90 created by main.(*App).runBackend /home/elias/proj/tailscale-android/cmd/tailscale/main.go:168 +0x27c Signed-off-by: Elias Naur --- ipn/local.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipn/local.go b/ipn/local.go index 02dc84feb..b7066e1aa 100644 --- a/ipn/local.go +++ b/ipn/local.go @@ -1540,8 +1540,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 From e98f2c57d637c4bf46e08ef39454c09b8a9525f8 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Thu, 29 Oct 2020 13:55:42 -0700 Subject: [PATCH 02/15] tsweb: add StdHandlerOpts that accepts an options struct I'm about to add yet another StdHandler option. Time to refactor. Signed-off-by: Josh Bleecher Snyder --- tsweb/tsweb.go | 44 ++++++++++++++++++++++++++++---------------- tsweb/tsweb_test.go | 2 +- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/tsweb/tsweb.go b/tsweb/tsweb.go index 649abdfa0..8675fae0c 100644 --- a/tsweb/tsweb.go +++ b/tsweb/tsweb.go @@ -157,11 +157,17 @@ 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 +} + // 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 +184,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 +220,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 +230,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 +256,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 +275,9 @@ 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) + } } } 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 { From 3b46655dbb0dc077530a9dd601fc65a0d8d7b16e Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Thu, 29 Oct 2020 15:44:46 -0700 Subject: [PATCH 03/15] tsweb: add StatusCodeCounters to HandlerOptions This lets servers using tsweb register expvars that will track the number of requests ending in 200s/300s/400s/500s. Signed-off-by: Josh Bleecher Snyder --- tsweb/tsweb.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tsweb/tsweb.go b/tsweb/tsweb.go index 8675fae0c..134f034cf 100644 --- a/tsweb/tsweb.go +++ b/tsweb/tsweb.go @@ -161,6 +161,11 @@ 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. @@ -278,6 +283,10 @@ func (h retHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { 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) } } From 037daad47a030b1e3cc77660bae104a444267666 Mon Sep 17 00:00:00 2001 From: Alex Brainman Date: Mon, 2 Nov 2020 18:51:28 +1100 Subject: [PATCH 04/15] =?UTF-8?q?.github/workflows:=20use=20cache=20to=20s?= =?UTF-8?q?peed=20up=20Windows=C2=A0tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #872 Signed-off-by: Alex Brainman --- .github/workflows/windows.yml | 8 ++++++++ 1 file changed, 8 insertions(+) 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 ./... From 01ee638ccac0d537c841fc830732b58c5627fe73 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 2 Nov 2020 08:33:34 -0800 Subject: [PATCH 05/15] Change some os.IsNotExist to errors.Is(err, os.ErrNotExist) for non-os errors. os.IsNotExist doesn't unwrap errors. errors.Is does. The ioutil.ReadFile ones happened to be fine but I changed them so we're consistent with the rule: if the error comes from os, you can use os.IsNotExist, but from any other package, use errors.Is. (errors.Is always would also work, but not worth updating all the code) The motivation here was that we were logging about failure to migrate legacy relay node prefs file on startup, even though the code tried to avoid that. See golang/go#41122 --- cmd/derper/derper.go | 2 +- ipn/local.go | 4 ++-- ipn/prefs_test.go | 15 +++++++++++++++ wgengine/router/dns/direct.go | 3 ++- 4 files changed, 20 insertions(+), 4 deletions(-) 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/local.go b/ipn/local.go index b7066e1aa..f41f47d98 100644 --- a/ipn/local.go +++ b/ipn/local.go @@ -803,8 +803,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 { 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/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 @@ 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 { From cc3259f8d9ff32994cdbeba6045abe3e73986e06 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 2 Nov 2020 08:51:12 -0800 Subject: [PATCH 06/15] ipn: fix crash generating machine key on new installs Regression from d6ad41dceaf20f (for #732). Probably also means eab6e9ea4e45 was unnecessary, but it's fine. Fixes #887 --- ipn/local.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ipn/local.go b/ipn/local.go index f41f47d98..6a8ff8406 100644 --- a/ipn/local.go +++ b/ipn/local.go @@ -708,7 +708,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 + } }() } From f3aa08de7696bad8f17e847752a0d8ef96dda84b Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 2 Nov 2020 09:22:21 -0800 Subject: [PATCH 07/15] ipn/ipnserver: remove "Server mode" from a user-visible error message That's an internal nickname. --- ipn/ipnserver/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index cb76ba5a4..f572fcd22 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -285,7 +285,7 @@ func (s *server) addConn(c net.Conn, isHTTP bool) (ci connIdentity, err error) { } 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) + return ci, fmt.Errorf("Tailscale running as %s. Access denied.", su.Username) } if !isHTTP { From 710b105f383db7c92a8195a1d31bfd3b02f1e87d Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 30 Oct 2020 13:03:46 -0700 Subject: [PATCH 08/15] version: use -g as the "other" suffix, so that `git show` works. Fixes #880. Signed-off-by: David Anderson --- version/mkversion.sh | 2 +- version/mkversion_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/version/mkversion.sh b/version/mkversion.sh index a5768c040..84772bc20 100755 --- a/version/mkversion.sh +++ b/version/mkversion.sh @@ -72,7 +72,7 @@ fi # used and Go toolchain version). if [ -n "$other" ]; then other="$(echo $other | cut -c1-9)" - other="-o${other}" + other="-g${other}" fi # Validate that the version data makes sense. Rules: diff --git a/version/mkversion_test.go b/version/mkversion_test.go index 00560bbf1..e209b4756 100644 --- a/version/mkversion_test.go +++ b/version/mkversion_test.go @@ -47,7 +47,7 @@ func TestMkversion(t *testing.T) { {"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")}, + {"v1.1.0-129-gabcdef", "0123456789abcdef0123456789abcdef", true, "1.1.1129-tabcdef-g012345678", "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 From 437142daa5856c76b7587bef0d5e64e2e39ae814 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Thu, 29 Oct 2020 00:17:36 -0700 Subject: [PATCH 09/15] version: calculate version info without using git tags. This makes it easier to integrate this version math into a submodule-ful world. We'll continue to have regular git tags that parallel the information in VERSION, so that builds out of this repository behave the same. Signed-off-by: David Anderson --- VERSION | 1 + build_dist.sh | 6 ++---- version/describe.sh | 26 ++++++++++++++++++++++++++ version/describe.txt.do | 3 +-- 4 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 VERSION create mode 100755 version/describe.sh diff --git a/VERSION b/VERSION new file mode 100644 index 000000000..4c902a289 --- /dev/null +++ b/VERSION @@ -0,0 +1 @@ +1.1.0 f81233524fddeec450940af8dc1a0dd8841bf28c diff --git a/build_dist.sh b/build_dist.sh index 3818a6207..07d82a4c1 100755 --- a/build_dist.sh +++ b/build_dist.sh @@ -11,10 +11,8 @@ set -euo pipefail -describe=$(git describe --long --abbrev=9) -# --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) +describe=$(./version/describe.sh) +commit=$(git rev-parse --verify --quiet HEAD) long=$(./version/mkversion.sh long "$describe" "") short=$(./version/mkversion.sh short "$describe" "") diff --git a/version/describe.sh b/version/describe.sh new file mode 100755 index 000000000..5e12aa266 --- /dev/null +++ b/version/describe.sh @@ -0,0 +1,26 @@ +#!/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/describe.txt.do b/version/describe.txt.do index 0bdf6651a..9fb1095d6 100644 --- a/version/describe.txt.do +++ b/version/describe.txt.do @@ -1,4 +1,3 @@ -describe=$(git describe --long --abbrev=9) -echo "$describe" >$3 +./describe.sh >$3 redo-always redo-stamp <$3 From 20a357b38618c296272e857772d75d836452e7d8 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 2 Nov 2020 09:52:59 -0800 Subject: [PATCH 10/15] ipn, ipn/ipnserver: add IPN state for server in use, handle explicitly On Windows, we were previously treating a server used by different users as a fatal error, which meant the second user (upon starting Tailscale, explicitly or via Start Up programs) got an invasive error message dialog. Instead, give it its own IPN state and change the Notify.ErrMessage to be details in that state. Then the Windows GUI can be less aggresive about that happening. Also, * wait to close the IPN connection until the server ownership state changes so the GUI doesn't need to repeatedly reconnect to discover changes. * fix a bug discovered during testing: on system reboot, the ipnserver's serverModeUser was getting cleared while the state transitioned from Unknown to Running. Instead, track 'inServerMode' explicitly and remove the old accessor method which was error prone. * fix a rare bug where the client could start up and set the server mode prefs in its Start call and we wouldn't persist that to the StateStore storage's prefs start key. (Previously it was only via a prefs toggle at runtime) --- ipn/backend.go | 13 +++- ipn/ipnserver/server.go | 137 +++++++++++++++++++++++++++++++++------- ipn/local.go | 61 ++++++++++-------- ipn/message.go | 11 ++++ 4 files changed, 170 insertions(+), 52 deletions(-) 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 @@ const ( NoState = State(iota) + InUseOtherUser NeedsLogin NeedsMachineAuth Stopped @@ -33,8 +34,14 @@ 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 f572fcd22..43acd9ef1 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -10,6 +10,7 @@ "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 running as %s. Access denied.", 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 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 6a8ff8406..8dabe1b79 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,6 +415,7 @@ func (b *LocalBackend) Start(opts Options) error { return fmt.Errorf("loading requested state: %v", err) } + b.inServerMode = b.stateKey != "" b.serverURL = b.prefs.ControlURL hostinfo.RoutableIPs = append(hostinfo.RoutableIPs, b.prefs.AdvertiseRoutes...) hostinfo.RequestTags = append(hostinfo.RequestTags, b.prefs.AdvertiseTags...) @@ -766,6 +768,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. @@ -786,6 +817,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 } @@ -843,13 +875,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. @@ -1001,6 +1030,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() @@ -1019,26 +1049,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()) 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 { From 65bad9a8bd35b5691f4ccfcfca637844a0892ed3 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 2 Nov 2020 18:08:47 -0800 Subject: [PATCH 11/15] version: greatly simplify redo nonsense, now that we use VERSION. Signed-off-by: David Anderson --- version/.gitignore | 1 + version/describe.sh | 26 ---- version/extragitcommit.txt.do | 6 - version/gitcommit.txt.do | 6 - version/long.txt.do | 5 - version/mkversion.sh | 139 ------------------ version/mkversion_test.go | 118 +++++++++------ version/short.txt.do | 5 - version/ver.go.do | 15 +- .../{describe.txt.do => version-info.sh.do} | 2 +- version/version.h.do | 22 +-- version/version.sh | 109 ++++++++++++++ version/version.xcconfig.do | 19 ++- 13 files changed, 208 insertions(+), 265 deletions(-) delete mode 100755 version/describe.sh delete mode 100644 version/extragitcommit.txt.do delete mode 100644 version/gitcommit.txt.do delete mode 100644 version/long.txt.do delete mode 100755 version/mkversion.sh delete mode 100644 version/short.txt.do rename version/{describe.txt.do => version-info.sh.do} (54%) create mode 100755 version/version.sh 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/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 84772bc20..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="-g${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 e209b4756..f4ac1fe69 100644 --- a/version/mkversion_test.go +++ b/version/mkversion_test.go @@ -8,22 +8,21 @@ "fmt" "os/exec" "runtime" + "strconv" "strings" "testing" + + "github.com/google/go-cmp/cmp" ) -func xcode(short, long string) string { - return fmt.Sprintf("VERSION_NAME = %s\nVERSION_ID = %s", short, long) -} - -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-g012345678", "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) + 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 } - 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) - } - 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" + 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" From de5da37a22ba8a47db43124fcb8a067f61b5549e Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 2 Nov 2020 20:38:52 -0800 Subject: [PATCH 12/15] VERSION: rename to version.txt to work around macOS limitations. Signed-off-by: David Anderson --- VERSION => VERSION.txt | 0 version/version.sh | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename VERSION => VERSION.txt (100%) diff --git a/VERSION b/VERSION.txt similarity index 100% rename from VERSION rename to VERSION.txt diff --git a/version/version.sh b/version/version.sh index fe85339fe..69582bb2f 100755 --- a/version/version.sh +++ b/version/version.sh @@ -22,7 +22,7 @@ case $# in # 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" + 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) From 07b6ffd55c006706c06e8bb16eecb169730786fb Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 2 Nov 2020 21:11:20 -0800 Subject: [PATCH 13/15] ipn: only use Prefs, not computed stateKey, to determine server mode When the service was running without a client (e.g. after a reboot) and then the owner logs in and the GUI attaches, the computed state key changed to "" (driven by frontend prefs), and then it was falling out of server mode, despite the GUI-provided prefs still saying it wanted server mode. Also add some logging. And remove a scary "Access denied" from a user-visible error, making the two possible already-in-use error messages consistent with each other. --- ipn/ipnserver/server.go | 2 +- ipn/local.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 43acd9ef1..6df546bae 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -314,7 +314,7 @@ func (s *server) checkConnIdentityLocked(ci connIdentity) error { } if su := s.serverModeUser; su != nil && ci.UserID != su.Uid { //lint:ignore ST1005 we want to capitalize Tailscale here - return inUseOtherUserError{fmt.Errorf("Tailscale running as %s. Access denied.", su.Username)} + return inUseOtherUserError{fmt.Errorf("Tailscale already in use by %s", su.Username)} } return nil } diff --git a/ipn/local.go b/ipn/local.go index 8dabe1b79..0b5d1cf82 100644 --- a/ipn/local.go +++ b/ipn/local.go @@ -415,10 +415,11 @@ func (b *LocalBackend) Start(opts Options) error { return fmt.Errorf("loading requested state: %v", err) } - b.inServerMode = b.stateKey != "" + 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 From 1036f51a56458a1fa44d4657a214d0fc9e88a8b0 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 3 Nov 2020 08:53:01 -0800 Subject: [PATCH 14/15] net/tshttpproxy: aggressively rate-limit error logs in Transport.Proxy path Otherwise log upload HTTP requests generate proxy errrors which generate logs which generate HTTP requests which generate proxy errors which generate more logs, etc. Fixes #879 --- net/tshttpproxy/tshttpproxy_windows.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) 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 @@ "github.com/alexbrainman/sspi/negotiate" "golang.org/x/sys/windows" + "tailscale.com/types/logger" ) var ( @@ -38,6 +39,13 @@ func init() { 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 @@ type result struct { 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 @@ type result struct { 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 @@ type result struct { 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() From 28f6552646b5c80634f9970ac9c1e286cdf77d78 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 3 Nov 2020 09:59:50 -0800 Subject: [PATCH 15/15] wgengine/router/dns: run ipconfig /registerdns async, log timing Signed-off-by: Brad Fitzpatrick --- wgengine/router/dns/manager_windows.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) 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 @@ "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 }