From 975c5f7684574edc7643a008b9e217d94ad0554e Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Mon, 13 Nov 2023 10:20:28 -0800 Subject: [PATCH] taildrop: lazily perform full deletion scan after first taildrop use (#10137) Simply reading the taildrop directory can pop up security dialogs on platforms like macOS. Avoid this by only performing garbage collection of partial and deleted files after the first received taildrop file, which would have prompted the security dialog window. Updates tailscale/corp#14772 Signed-off-by: Joe Tsai --- ipn/ipnlocal/local.go | 1 + ipn/store.go | 5 +++++ taildrop/delete.go | 18 +++++++++++++++--- taildrop/delete_test.go | 20 ++++++++++++++++++-- taildrop/send.go | 12 ++++++++++++ taildrop/taildrop.go | 5 +++-- 6 files changed, 54 insertions(+), 7 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 99dde77f2..d67e2f965 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3728,6 +3728,7 @@ func (b *LocalBackend) initPeerAPIListener() { taildrop: taildrop.ManagerOptions{ Logf: b.logf, Clock: tstime.DefaultClock{Clock: b.clock}, + State: b.store, Dir: fileRoot, DirectFileMode: b.directFileRoot != "", AvoidFinalRename: !b.directFileDoFinalRename, diff --git a/ipn/store.go b/ipn/store.go index 3bef012ba..84ae00dd2 100644 --- a/ipn/store.go +++ b/ipn/store.go @@ -53,6 +53,11 @@ // CurrentProfileStateKey is the key under which we store the current // profile. CurrentProfileStateKey = StateKey("_current-profile") + + // TaildropReceivedKey is the key to indicate whether any taildrop file + // has ever been received (even if partially). + // Any non-empty value indicates that at least one file has been received. + TaildropReceivedKey = StateKey("_taildrop-received") ) // CurrentProfileID returns the StateKey that stores the diff --git a/taildrop/delete.go b/taildrop/delete.go index 12b95c020..aaef34df1 100644 --- a/taildrop/delete.go +++ b/taildrop/delete.go @@ -13,6 +13,7 @@ "sync" "time" + "tailscale.com/ipn" "tailscale.com/syncs" "tailscale.com/tstime" "tailscale.com/types/logger" @@ -52,13 +53,24 @@ func (d *fileDeleter) Init(m *Manager, eventHook func(string)) { d.dir = m.opts.Dir d.event = eventHook - // From a cold-start, load the list of partial and deleted files. d.byName = make(map[string]*list.Element) d.emptySignal = make(chan struct{}) d.shutdownCtx, d.shutdown = context.WithCancel(context.Background()) + + // From a cold-start, load the list of partial and deleted files. + // + // Only run this if we have ever received at least one file + // to avoid ever touching the taildrop directory on systems (e.g., MacOS) + // that pop up a security dialog window upon first access. + if m.opts.State == nil { + return + } + if b, _ := m.opts.State.ReadState(ipn.TaildropReceivedKey); len(b) == 0 { + return + } d.group.Go(func() { - d.event("start init") - defer d.event("end init") + d.event("start full-scan") + defer d.event("end full-scan") rangeDir(d.dir, func(de fs.DirEntry) bool { switch { case d.shutdownCtx.Err() != nil: diff --git a/taildrop/delete_test.go b/taildrop/delete_test.go index 9066c7bc8..5fa4b9c37 100644 --- a/taildrop/delete_test.go +++ b/taildrop/delete_test.go @@ -11,6 +11,8 @@ "time" "github.com/google/go-cmp/cmp" + "tailscale.com/ipn" + "tailscale.com/ipn/store/mem" "tailscale.com/tstest" "tailscale.com/tstime" "tailscale.com/util/must" @@ -72,6 +74,8 @@ func TestDeleter(t *testing.T) { m.opts.Logf = t.Logf m.opts.Clock = tstime.DefaultClock{Clock: clock} m.opts.Dir = dir + m.opts.State = must.Get(mem.New(nil, "")) + must.Do(m.opts.State.WriteState(ipn.TaildropReceivedKey, []byte{1})) fd.Init(&m, eventHook) defer fd.Shutdown() insert := func(name string) { @@ -85,8 +89,8 @@ func TestDeleter(t *testing.T) { fd.Remove(name) } - checkEvents("start init") - checkEvents("end init", "start waitAndDelete") + checkEvents("start full-scan") + checkEvents("end full-scan", "start waitAndDelete") checkDirectory("foo.partial", "bar.partial", "buzz.deleted") advance(deleteDelay / 2) @@ -134,3 +138,15 @@ func TestDeleter(t *testing.T) { remove("wuzz.partial") checkEvents("end waitAndDelete") } + +// Test that the asynchronous full scan of the taildrop directory does not occur +// on a cold start if taildrop has never received any files. +func TestDeleterInitWithoutTaildrop(t *testing.T) { + var m Manager + var fd fileDeleter + m.opts.Logf = t.Logf + m.opts.Dir = t.TempDir() + m.opts.State = must.Get(mem.New(nil, "")) + fd.Init(&m, func(event string) { t.Errorf("unexpected event: %v", event) }) + fd.Shutdown() +} diff --git a/taildrop/send.go b/taildrop/send.go index 42c223737..d96c3542c 100644 --- a/taildrop/send.go +++ b/taildrop/send.go @@ -13,6 +13,7 @@ "time" "tailscale.com/envknob" + "tailscale.com/ipn" "tailscale.com/tstime" "tailscale.com/version/distro" ) @@ -136,6 +137,17 @@ func (m *Manager) PutFile(id ClientID, baseName string, r io.Reader, offset, len }() inFile.w = f + // Record that we have started to receive at least one file. + // This is used by the deleter upon a cold-start to scan the directory + // for any files that need to be deleted. + if m.opts.State != nil { + if b, _ := m.opts.State.ReadState(ipn.TaildropReceivedKey); len(b) == 0 { + if err := m.opts.State.WriteState(ipn.TaildropReceivedKey, []byte{1}); err != nil { + m.opts.Logf("WriteState error: %v", err) // non-fatal error + } + } + } + // A positive offset implies that we are resuming an existing file. // Seek to the appropriate offset and truncate the file. if offset != 0 { diff --git a/taildrop/taildrop.go b/taildrop/taildrop.go index ceda9cd39..39ab5aff5 100644 --- a/taildrop/taildrop.go +++ b/taildrop/taildrop.go @@ -67,8 +67,9 @@ func (id ClientID) partialSuffix() string { // ManagerOptions are options to configure the [Manager]. type ManagerOptions struct { - Logf logger.Logf - Clock tstime.DefaultClock + Logf logger.Logf // may be nil + Clock tstime.DefaultClock // may be nil + State ipn.StateStore // may be nil // Dir is the directory to store received files. // This main either be the final location for the files