taildrop: document and cleanup the package (#9699)

Changes made:
* Unexport declarations specific to internal taildrop functionality.
* Document all exported functionality.
* Move TestRedactErr to the taildrop package.
* Rename and invert Handler.DirectFileDoFinalRename as AvoidFinalRename.

Updates tailscale/corp#14772

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

ipn/ipnlocal/local.go

@@ -3547,9 +3547,9 @@ func (b *LocalBackend) initPeerAPIListener() {
 		taildrop: &taildrop.Handler{
 			Logf:             b.logf,
 			Clock:            b.clock,
-			RootDir:                 fileRoot,
+			Dir:              fileRoot,
 			DirectFileMode:   b.directFileRoot != "",
-			DirectFileDoFinalRename: b.directFileDoFinalRename,
+			AvoidFinalRename: !b.directFileDoFinalRename,
 		},
 	}
 	if dm, ok := b.sys.DNSManager.GetOK(); ok {

ipn/ipnlocal/peerapi_test.go

@@ -5,7 +5,6 @@ package ipnlocal
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"io/fs"
@@ -67,7 +66,7 @@ func bodyNotContains(sub string) check {
 
 func fileHasSize(name string, size int) check {
 	return func(t *testing.T, e *peerAPITestEnv) {
-		root := e.ph.ps.taildrop.RootDir
+		root := e.ph.ps.taildrop.Dir
 		if root == "" {
 			t.Errorf("no rootdir; can't check whether %q has size %v", name, size)
 			return
@@ -83,7 +82,7 @@ func fileHasSize(name string, size int) check {
 
 func fileHasContents(name string, want string) check {
 	return func(t *testing.T, e *peerAPITestEnv) {
-		root := e.ph.ps.taildrop.RootDir
+		root := e.ph.ps.taildrop.Dir
 		if root == "" {
 			t.Errorf("no rootdir; can't check contents of %q", name)
 			return
@@ -498,7 +497,7 @@ func TestHandlePeerAPI(t *testing.T) {
 						Clock: &tstest.Clock{},
 					}
 				}
-				e.ph.ps.taildrop.RootDir = rootDir
+				e.ph.ps.taildrop.Dir = rootDir
 			}
 			for _, req := range tt.reqs {
 				e.rr = httptest.NewRecorder()
@@ -540,7 +539,7 @@ func TestFileDeleteRace(t *testing.T) {
 		taildrop: &taildrop.Handler{
 			Logf:  t.Logf,
 			Clock: &tstest.Clock{},
-			RootDir: dir,
+			Dir:   dir,
 		},
 	}
 	ph := &peerAPIHandler{
@@ -635,67 +634,3 @@ func TestPeerAPIReplyToDNSQueries(t *testing.T) {
 		t.Errorf("unexpectedly IPv6 deny; wanted to be a DNS server")
 	}
 }
-
-func TestRedactErr(t *testing.T) {
-	testCases := []struct {
-		name string
-		err  func() error
-		want string
-	}{
-		{
-			name: "PathError",
-			err: func() error {
-				return &os.PathError{
-					Op:   "open",
-					Path: "/tmp/sensitive.txt",
-					Err:  fs.ErrNotExist,
-				}
-			},
-			want: `open redacted.41360718: file does not exist`,
-		},
-		{
-			name: "LinkError",
-			err: func() error {
-				return &os.LinkError{
-					Op:  "symlink",
-					Old: "/tmp/sensitive.txt",
-					New: "/tmp/othersensitive.txt",
-					Err: fs.ErrNotExist,
-				}
-			},
-			want: `symlink redacted.41360718 redacted.6bcf093a: file does not exist`,
-		},
-		{
-			name: "something else",
-			err:  func() error { return errors.New("i am another error type") },
-			want: `i am another error type`,
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			// For debugging
-			var i int
-			for err := tc.err(); err != nil; err = errors.Unwrap(err) {
-				t.Logf("%d: %T @ %p", i, err, err)
-				i++
-			}
-
-			t.Run("Root", func(t *testing.T) {
-				got := taildrop.RedactErr(tc.err()).Error()
-				if got != tc.want {
-					t.Errorf("err = %q; want %q", got, tc.want)
-				}
-			})
-			t.Run("Wrapped", func(t *testing.T) {
-				wrapped := fmt.Errorf("wrapped error: %w", tc.err())
-				want := "wrapped error: " + tc.want
-
-				got := taildrop.RedactErr(wrapped).Error()
-				if got != want {
-					t.Errorf("err = %q; want %q", got, want)
-				}
-			})
-		})
-	}
-}

taildrop/retrieve.go

@@ -19,10 +19,10 @@ import (
 	"tailscale.com/logtail/backoff"
 )
 
-// HasFilesWaiting reports whether any files are buffered in the
+// HasFilesWaiting reports whether any files are buffered in [Handler.Dir].
-// tailscaled daemon storage.
+// This always returns false when [Handler.DirectFileMode] is false.
 func (s *Handler) HasFilesWaiting() bool {
-	if s == nil || s.RootDir == "" || s.DirectFileMode {
+	if s == nil || s.Dir == "" || s.DirectFileMode {
 		return false
 	}
 	if s.knownEmpty.Load() {
@@ -33,7 +33,7 @@ func (s *Handler) HasFilesWaiting() bool {
 		// keep this negative cache.
 		return false
 	}
-	f, err := os.Open(s.RootDir)
+	f, err := os.Open(s.Dir)
 	if err != nil {
 		return false
 	}
@@ -42,7 +42,7 @@ func (s *Handler) HasFilesWaiting() bool {
 		des, err := f.ReadDir(10)
 		for _, de := range des {
 			name := de.Name()
-			if strings.HasSuffix(name, PartialSuffix) {
+			if strings.HasSuffix(name, partialSuffix) {
 				continue
 			}
 			if name, ok := strings.CutSuffix(name, deletedSuffix); ok { // for Windows + tests
@@ -51,16 +51,16 @@ func (s *Handler) HasFilesWaiting() bool {
 				// as the OS may return "foo.jpg.deleted" before "foo.jpg"
 				// and we don't want to delete the ".deleted" file before
 				// enumerating to the "foo.jpg" file.
-				defer tryDeleteAgain(filepath.Join(s.RootDir, name))
+				defer tryDeleteAgain(filepath.Join(s.Dir, name))
 				continue
 			}
 			if de.Type().IsRegular() {
-				_, err := os.Stat(filepath.Join(s.RootDir, name+deletedSuffix))
+				_, err := os.Stat(filepath.Join(s.Dir, name+deletedSuffix))
 				if os.IsNotExist(err) {
 					return true
 				}
 				if err == nil {
-					tryDeleteAgain(filepath.Join(s.RootDir, name))
+					tryDeleteAgain(filepath.Join(s.Dir, name))
 					continue
 				}
 			}
@@ -76,22 +76,19 @@ func (s *Handler) HasFilesWaiting() bool {
 }
 
 // WaitingFiles returns the list of files that have been sent by a
-// peer that are waiting in the buffered "pick up" directory owned by
+// peer that are waiting in [Handler.Dir].
-// the Tailscale daemon.
+// This always returns nil when [Handler.DirectFileMode] is false.
-//
-// As a side effect, it also does any lazy deletion of files as
-// required by Windows.
 func (s *Handler) WaitingFiles() (ret []apitype.WaitingFile, err error) {
 	if s == nil {
 		return nil, errNilHandler
 	}
-	if s.RootDir == "" {
+	if s.Dir == "" {
-		return nil, ErrNoTaildrop
+		return nil, errNoTaildrop
 	}
 	if s.DirectFileMode {
 		return nil, nil
 	}
-	f, err := os.Open(s.RootDir)
+	f, err := os.Open(s.Dir)
 	if err != nil {
 		return nil, err
 	}
@@ -101,7 +98,7 @@ func (s *Handler) WaitingFiles() (ret []apitype.WaitingFile, err error) {
 		des, err := f.ReadDir(10)
 		for _, de := range des {
 			name := de.Name()
-			if strings.HasSuffix(name, PartialSuffix) {
+			if strings.HasSuffix(name, partialSuffix) {
 				continue
 			}
 			if name, ok := strings.CutSuffix(name, deletedSuffix); ok { // for Windows + tests
@@ -143,7 +140,7 @@ func (s *Handler) WaitingFiles() (ret []apitype.WaitingFile, err error) {
 		// Maybe Windows is done virus scanning the file we tried
 		// to delete a long time ago and will let us delete it now.
 		for name := range deleted {
-			tryDeleteAgain(filepath.Join(s.RootDir, name))
+			tryDeleteAgain(filepath.Join(s.Dir, name))
 		}
 	}
 	sort.Slice(ret, func(i, j int) bool { return ret[i].Name < ret[j].Name })
@@ -164,17 +161,19 @@ func tryDeleteAgain(fullPath string) {
 	}
 }
 
+// DeleteFile deletes a file of the given baseName from [Handler.Dir].
+// This method is only allowed when [Handler.DirectFileMode] is false.
 func (s *Handler) DeleteFile(baseName string) error {
 	if s == nil {
 		return errNilHandler
 	}
-	if s.RootDir == "" {
+	if s.Dir == "" {
-		return ErrNoTaildrop
+		return errNoTaildrop
 	}
 	if s.DirectFileMode {
 		return errors.New("deletes not allowed in direct mode")
 	}
-	path, ok := s.DiskPath(baseName)
+	path, ok := s.diskPath(baseName)
 	if !ok {
 		return errors.New("bad filename")
 	}
@@ -184,7 +183,7 @@ func (s *Handler) DeleteFile(baseName string) error {
 	for {
 		err := os.Remove(path)
 		if err != nil && !os.IsNotExist(err) {
-			err = RedactErr(err)
+			err = redactErr(err)
 			// Put a retry loop around deletes on Windows. Windows
 			// file descriptor closes are effectively asynchronous,
 			// as a bunch of hooks run on/after close, and we can't
@@ -203,7 +202,7 @@ func (s *Handler) DeleteFile(baseName string) error {
 					bo.BackOff(context.Background(), err)
 					continue
 				}
-				if err := TouchFile(path + deletedSuffix); err != nil {
+				if err := touchFile(path + deletedSuffix); err != nil {
 					logf("peerapi: failed to leave deleted marker: %v", err)
 				}
 			}
@@ -214,25 +213,27 @@ func (s *Handler) DeleteFile(baseName string) error {
 	}
 }
 
-func TouchFile(path string) error {
+func touchFile(path string) error {
 	f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0666)
 	if err != nil {
-		return RedactErr(err)
+		return redactErr(err)
 	}
 	return f.Close()
 }
 
+// OpenFile opens a file of the given baseName from [Handler.Dir].
+// This method is only allowed when [Handler.DirectFileMode] is false.
 func (s *Handler) OpenFile(baseName string) (rc io.ReadCloser, size int64, err error) {
 	if s == nil {
 		return nil, 0, errNilHandler
 	}
-	if s.RootDir == "" {
+	if s.Dir == "" {
-		return nil, 0, ErrNoTaildrop
+		return nil, 0, errNoTaildrop
 	}
 	if s.DirectFileMode {
 		return nil, 0, errors.New("opens not allowed in direct mode")
 	}
-	path, ok := s.DiskPath(baseName)
+	path, ok := s.diskPath(baseName)
 	if !ok {
 		return nil, 0, errors.New("bad filename")
 	}
@@ -242,12 +243,12 @@ func (s *Handler) OpenFile(baseName string) (rc io.ReadCloser, size int64, err e
 	}
 	f, err := os.Open(path)
 	if err != nil {
-		return nil, 0, RedactErr(err)
+		return nil, 0, redactErr(err)
 	}
 	fi, err := f.Stat()
 	if err != nil {
 		f.Close()
-		return nil, 0, RedactErr(err)
+		return nil, 0, redactErr(err)
 	}
 	return f, fi.Size(), nil
 }

taildrop/send.go

@@ -63,6 +63,8 @@ func (f *incomingFile) Write(p []byte) (n int, err error) {
 }
 
 // HandlePut receives a file.
+// It handles an HTTP PUT request to the "/v0/put/{filename}" endpoint,
+// where {filename} is a base filename.
 // It returns the number of bytes received and whether it was received successfully.
 func (h *Handler) HandlePut(w http.ResponseWriter, r *http.Request) (finalSize int64, success bool) {
 	if !envknob.CanTaildrop() {
@@ -73,8 +75,8 @@ func (h *Handler) HandlePut(w http.ResponseWriter, r *http.Request) (finalSize i
 		http.Error(w, "expected method PUT", http.StatusMethodNotAllowed)
 		return finalSize, success
 	}
-	if h == nil || h.RootDir == "" {
+	if h == nil || h.Dir == "" {
-		http.Error(w, ErrNoTaildrop.Error(), http.StatusInternalServerError)
+		http.Error(w, errNoTaildrop.Error(), http.StatusInternalServerError)
 		return finalSize, success
 	}
 	if distro.Get() == distro.Unraid && !h.DirectFileMode {
@@ -100,9 +102,9 @@ func (h *Handler) HandlePut(w http.ResponseWriter, r *http.Request) (finalSize i
 		http.Error(w, "bad path encoding", http.StatusBadRequest)
 		return finalSize, success
 	}
-	dstFile, ok := h.DiskPath(baseName)
+	dstFile, ok := h.diskPath(baseName)
 	if !ok {
-		http.Error(w, "bad filename", 400)
+		http.Error(w, "bad filename", http.StatusBadRequest)
 		return finalSize, success
 	}
 	// TODO(bradfitz): prevent same filename being sent by two peers at once
@@ -113,10 +115,10 @@ func (h *Handler) HandlePut(w http.ResponseWriter, r *http.Request) (finalSize i
 		return finalSize, success
 	}
 
-	partialFile := dstFile + PartialSuffix
+	partialFile := dstFile + partialSuffix
 	f, err := os.Create(partialFile)
 	if err != nil {
-		h.Logf("put Create error: %v", RedactErr(err))
+		h.Logf("put Create error: %v", redactErr(err))
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return finalSize, success
 	}
@@ -146,7 +148,7 @@ func (h *Handler) HandlePut(w http.ResponseWriter, r *http.Request) (finalSize i
 		defer h.incomingFiles.Delete(inFile)
 		n, err := io.Copy(inFile, r.Body)
 		if err != nil {
-			err = RedactErr(err)
+			err = redactErr(err)
 			f.Close()
 			h.Logf("put Copy error: %v", err)
 			http.Error(w, err.Error(), http.StatusInternalServerError)
@@ -154,18 +156,18 @@ func (h *Handler) HandlePut(w http.ResponseWriter, r *http.Request) (finalSize i
 		}
 		finalSize = n
 	}
-	if err := RedactErr(f.Close()); err != nil {
+	if err := redactErr(f.Close()); err != nil {
 		h.Logf("put Close error: %v", err)
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return finalSize, success
 	}
-	if h.DirectFileMode && !h.DirectFileDoFinalRename {
+	if h.DirectFileMode && h.AvoidFinalRename {
 		if inFile != nil { // non-zero length; TODO: notify even for zero length
 			inFile.markAndNotifyDone()
 		}
 	} else {
 		if err := os.Rename(partialFile, dstFile); err != nil {
-			err = RedactErr(err)
+			err = redactErr(err)
 			h.Logf("put final rename: %v", err)
 			http.Error(w, err.Error(), http.StatusInternalServerError)
 			return finalSize, success

taildrop/taildrop.go

@@ -1,6 +1,12 @@
 // Copyright (c) Tailscale Inc & AUTHORS
 // SPDX-License-Identifier: BSD-3-Clause
 
+// Package taildrop contains the implementation of the Taildrop
+// functionality including sending and retrieving files.
+// This package does not validate permissions, the caller should
+// be responsible for ensuring correct authorization.
+//
+// For related documentation see: http://go/taildrop-how-does-it-work
 package taildrop
 
 import (
@@ -22,30 +28,38 @@ import (
 	"tailscale.com/util/multierr"
 )
 
+// Handler manages the state for receiving and managing taildropped files.
 type Handler struct {
 	Logf  logger.Logf
 	Clock tstime.Clock
 
-	RootDir string // empty means file receiving unavailable
+	// Dir is the directory to store received files.
+	// This main either be the final location for the files
+	// or just a temporary staging directory (see DirectFileMode).
+	Dir string
 
-	// DirectFileMode is whether we're writing files directly to a
+	// DirectFileMode reports whether we are writing files
-	// download directory (as *.partial files), rather than making
+	// directly to a download directory, rather than writing them to
-	// the frontend retrieve it over localapi HTTP and write it
+	// a temporary staging directory.
-	// somewhere itself. This is used on the GUI macOS versions
+	//
-	// and on Synology.
+	// The following methods:
-	// In DirectFileMode, the peerapi doesn't do the final rename
+	//	- HasFilesWaiting
-	// from "foo.jpg.partial" to "foo.jpg" unless
+	//	- WaitingFiles
-	// directFileDoFinalRename is set.
+	//	- DeleteFile
+	//	- OpenFile
+	// have no purpose in DirectFileMode.
+	// They are only used to check whether files are in the staging directory,
+	// copy them out, and then delete them.
 	DirectFileMode bool
 
-	// DirectFileDoFinalRename is whether in directFileMode we
+	// AvoidFinalRename specifies whether in DirectFileMode
-	// additionally move the *.direct file to its final name after
+	// we should avoid renaming "foo.jpg.partial" to "foo.jpg" after reception.
-	// it's received.
+	AvoidFinalRename bool
-	DirectFileDoFinalRename bool
 
 	// SendFileNotify is called periodically while a file is actively
 	// receiving the contents for the file. There is a final call
 	// to the function when reception completes.
+	// It is not called if nil.
 	SendFileNotify func()
 
 	knownEmpty atomic.Bool
@@ -55,13 +69,13 @@ type Handler struct {
 
 var (
 	errNilHandler = errors.New("handler unavailable; not listening")
-	ErrNoTaildrop = errors.New("Taildrop disabled; no storage directory")
+	errNoTaildrop = errors.New("Taildrop disabled; no storage directory")
 )
 
 const (
-	// PartialSuffix is the suffix appended to files while they're
+	// partialSuffix is the suffix appended to files while they're
 	// still in the process of being transferred.
-	PartialSuffix = ".partial"
+	partialSuffix = ".partial"
 
 	// deletedSuffix is the suffix for a deleted marker file
 	// that's placed next to a file (without the suffix) that we
@@ -93,7 +107,7 @@ func validFilenameRune(r rune) bool {
 	return unicode.IsPrint(r)
 }
 
-func (s *Handler) DiskPath(baseName string) (fullPath string, ok bool) {
+func (s *Handler) diskPath(baseName string) (fullPath string, ok bool) {
 	if !utf8.ValidString(baseName) {
 		return "", false
 	}
@@ -108,7 +122,7 @@ func (s *Handler) DiskPath(baseName string) (fullPath string, ok bool) {
 	if clean != baseName ||
 		clean == "." || clean == ".." ||
 		strings.HasSuffix(clean, deletedSuffix) ||
-		strings.HasSuffix(clean, PartialSuffix) {
+		strings.HasSuffix(clean, partialSuffix) {
 		return "", false
 	}
 	for _, r := range baseName {
@@ -119,7 +133,7 @@ func (s *Handler) DiskPath(baseName string) (fullPath string, ok bool) {
 	if !filepath.IsLocal(baseName) {
 		return "", false
 	}
-	return filepath.Join(s.RootDir, baseName), true
+	return filepath.Join(s.Dir, baseName), true
 }
 
 func (s *Handler) IncomingFiles() []ipn.PartialFile {
@@ -166,7 +180,7 @@ func redactString(s string) string {
 	return string(b)
 }
 
-func RedactErr(root error) error {
+func redactErr(root error) error {
 	// redactStrings is a list of sensitive strings that were redacted.
 	// It is not sufficient to just snub out sensitive fields in Go errors
 	// since some wrapper errors like fmt.Errorf pre-cache the error string,

taildrop/taildrop_test.go

@@ -4,6 +4,9 @@
 package taildrop
 
 import (
+	"errors"
+	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -13,7 +16,7 @@ import (
 // Tests "foo.jpg.deleted" marks (for Windows).
 func TestDeletedMarkers(t *testing.T) {
 	dir := t.TempDir()
-	h := &Handler{RootDir: dir}
+	h := &Handler{Dir: dir}
 
 	nothingWaiting := func() {
 		t.Helper()
@@ -24,7 +27,7 @@ func TestDeletedMarkers(t *testing.T) {
 	}
 	touch := func(base string) {
 		t.Helper()
-		if err := TouchFile(filepath.Join(dir, base)); err != nil {
+		if err := touchFile(filepath.Join(dir, base)); err != nil {
 			t.Fatal(err)
 		}
 	}
@@ -86,3 +89,67 @@ func TestDeletedMarkers(t *testing.T) {
 		rc.Close()
 	}
 }
+
+func TestRedactErr(t *testing.T) {
+	testCases := []struct {
+		name string
+		err  func() error
+		want string
+	}{
+		{
+			name: "PathError",
+			err: func() error {
+				return &os.PathError{
+					Op:   "open",
+					Path: "/tmp/sensitive.txt",
+					Err:  fs.ErrNotExist,
+				}
+			},
+			want: `open redacted.41360718: file does not exist`,
+		},
+		{
+			name: "LinkError",
+			err: func() error {
+				return &os.LinkError{
+					Op:  "symlink",
+					Old: "/tmp/sensitive.txt",
+					New: "/tmp/othersensitive.txt",
+					Err: fs.ErrNotExist,
+				}
+			},
+			want: `symlink redacted.41360718 redacted.6bcf093a: file does not exist`,
+		},
+		{
+			name: "something else",
+			err:  func() error { return errors.New("i am another error type") },
+			want: `i am another error type`,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// For debugging
+			var i int
+			for err := tc.err(); err != nil; err = errors.Unwrap(err) {
+				t.Logf("%d: %T @ %p", i, err, err)
+				i++
+			}
+
+			t.Run("Root", func(t *testing.T) {
+				got := redactErr(tc.err()).Error()
+				if got != tc.want {
+					t.Errorf("err = %q; want %q", got, tc.want)
+				}
+			})
+			t.Run("Wrapped", func(t *testing.T) {
+				wrapped := fmt.Errorf("wrapped error: %w", tc.err())
+				want := "wrapped error: " + tc.want
+
+				got := redactErr(wrapped).Error()
+				if got != want {
+					t.Errorf("err = %q; want %q", got, want)
+				}
+			})
+		})
+	}
+}