From e4cb83b18bdf062c9f10e49a645a3b0f7155fa91 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Fri, 6 Oct 2023 15:41:14 -0700 Subject: [PATCH] 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 --- ipn/ipnlocal/local.go | 10 ++--- ipn/ipnlocal/peerapi_test.go | 77 +++--------------------------------- taildrop/retrieve.go | 61 ++++++++++++++-------------- taildrop/send.go | 22 ++++++----- taildrop/taildrop.go | 56 ++++++++++++++++---------- taildrop/taildrop_test.go | 71 ++++++++++++++++++++++++++++++++- 6 files changed, 158 insertions(+), 139 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index b92d45958..6d9d21e5a 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3545,11 +3545,11 @@ func (b *LocalBackend) initPeerAPIListener() { ps := &peerAPIServer{ b: b, taildrop: &taildrop.Handler{ - Logf: b.logf, - Clock: b.clock, - RootDir: fileRoot, - DirectFileMode: b.directFileRoot != "", - DirectFileDoFinalRename: b.directFileDoFinalRename, + Logf: b.logf, + Clock: b.clock, + Dir: fileRoot, + DirectFileMode: b.directFileRoot != "", + AvoidFinalRename: !b.directFileDoFinalRename, }, } if dm, ok := b.sys.DNSManager.GetOK(); ok { diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index 71dfb3386..7996dc62e 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/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() @@ -538,9 +537,9 @@ func TestFileDeleteRace(t *testing.T) { clock: &tstest.Clock{}, }, taildrop: &taildrop.Handler{ - Logf: t.Logf, - Clock: &tstest.Clock{}, - RootDir: dir, + Logf: t.Logf, + Clock: &tstest.Clock{}, + 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) - } - }) - }) - } -} diff --git a/taildrop/retrieve.go b/taildrop/retrieve.go index a472ee270..7a773c950 100644 --- a/taildrop/retrieve.go +++ b/taildrop/retrieve.go @@ -19,10 +19,10 @@ import ( "tailscale.com/logtail/backoff" ) -// HasFilesWaiting reports whether any files are buffered in the -// tailscaled daemon storage. +// HasFilesWaiting reports whether any files are buffered in [Handler.Dir]. +// 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 -// the Tailscale daemon. -// -// As a side effect, it also does any lazy deletion of files as -// required by Windows. +// peer that are waiting in [Handler.Dir]. +// This always returns nil when [Handler.DirectFileMode] is false. func (s *Handler) WaitingFiles() (ret []apitype.WaitingFile, err error) { if s == nil { return nil, errNilHandler } - if s.RootDir == "" { - return nil, ErrNoTaildrop + if s.Dir == "" { + 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 == "" { - return ErrNoTaildrop + if s.Dir == "" { + 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 == "" { - return nil, 0, ErrNoTaildrop + if s.Dir == "" { + 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 } diff --git a/taildrop/send.go b/taildrop/send.go index 7546eeeef..bee750d88 100644 --- a/taildrop/send.go +++ b/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 == "" { - http.Error(w, ErrNoTaildrop.Error(), http.StatusInternalServerError) + if h == nil || h.Dir == "" { + 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 diff --git a/taildrop/taildrop.go b/taildrop/taildrop.go index 46342f1a4..b482655cd 100644 --- a/taildrop/taildrop.go +++ b/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 - - // DirectFileMode is whether we're writing files directly to a - // download directory (as *.partial files), rather than making - // the frontend retrieve it over localapi HTTP and write it - // somewhere itself. This is used on the GUI macOS versions - // and on Synology. - // In DirectFileMode, the peerapi doesn't do the final rename - // from "foo.jpg.partial" to "foo.jpg" unless - // directFileDoFinalRename is set. + // 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 reports whether we are writing files + // directly to a download directory, rather than writing them to + // a temporary staging directory. + // + // The following methods: + // - HasFilesWaiting + // - WaitingFiles + // - 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 - // additionally move the *.direct file to its final name after - // it's received. - DirectFileDoFinalRename bool + // AvoidFinalRename specifies whether in DirectFileMode + // we should avoid renaming "foo.jpg.partial" to "foo.jpg" after reception. + AvoidFinalRename 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, diff --git a/taildrop/taildrop_test.go b/taildrop/taildrop_test.go index 3f37f3582..29c88e8c8 100644 --- a/taildrop/taildrop_test.go +++ b/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) + } + }) + }) + } +}