From a887ca7efe123afd711b17a1c2b6c9d28ca99a14 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 6 Dec 2022 15:05:51 -0500 Subject: [PATCH] ipn/ipnlocal: improve redactErr to handle more cases This handles the case where the inner *os.PathError is wrapped in another error type, and additionally will redact errors of type *os.LinkError. Finally, add tests to verify that redaction works. Signed-off-by: Andrew Dunham Change-Id: Ie83424ff6c85cdb29fb48b641330c495107aab7c --- cmd/tailscaled/depaware.txt | 1 + ipn/ipnlocal/peerapi.go | 77 ++++++++++++++++++++++++++++++++++-- ipn/ipnlocal/peerapi_test.go | 65 ++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 3 deletions(-) diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index edf3b331c..28af0a41b 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -407,6 +407,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de flag from tailscale.com/control/controlclient+ fmt from compress/flate+ hash from crypto+ + hash/adler32 from tailscale.com/ipn/ipnlocal hash/crc32 from compress/gzip+ hash/fnv from tailscale.com/wgengine/magicsock+ hash/maphash from go4.org/mem diff --git a/ipn/ipnlocal/peerapi.go b/ipn/ipnlocal/peerapi.go index 7fd2ed443..7433e0821 100644 --- a/ipn/ipnlocal/peerapi.go +++ b/ipn/ipnlocal/peerapi.go @@ -10,6 +10,7 @@ import ( "encoding/json" "errors" "fmt" + "hash/adler32" "hash/crc32" "html" "io" @@ -341,11 +342,81 @@ func (s *peerAPIServer) DeleteFile(baseName string) error { // accidentally logging actual filenames anywhere. const redacted = "redacted" +type redactedErr struct { + msg string + inner error +} + +func (re *redactedErr) Error() string { + return re.msg +} + +func (re *redactedErr) Unwrap() error { + return re.inner +} + +func redactString(s string) string { + hash := adler32.Checksum([]byte(s)) + + var buf [len(redacted) + len(".12345678")]byte + b := append(buf[:0], []byte(redacted)...) + b = append(b, '.') + b = strconv.AppendUint(b, uint64(hash), 16) + return string(b) +} + func redactErr(err error) error { - if pe, ok := err.(*os.PathError); ok { - pe.Path = redacted + var redactStrings []string + + var pe *os.PathError + if errors.As(err, &pe) { + // If this is the root error, then we can redact it directly. + if err == pe { + pe.Path = redactString(pe.Path) + return pe + } + + // Otherwise, we have a *PathError somewhere in the error + // chain, and we can't redact it because something later in the + // chain may have cached the Error() return already (as + // fmt.Errorf does). + // + // Add this path to the set of paths that we will redact, below. + redactStrings = append(redactStrings, pe.Path) + + // Also redact the Path value so that anything that calls + // Unwrap in the future gets the redacted value. + pe.Path = redactString(pe.Path) + } + + var le *os.LinkError + if errors.As(err, &le) { + // If this is the root error, then we can redact it directly. + if err == le { + le.New = redactString(le.New) + le.Old = redactString(le.Old) + return le + } + + // As above + redactStrings = append(redactStrings, le.New, le.Old) + le.New = redactString(le.New) + le.Old = redactString(le.Old) } - return err + + if len(redactStrings) == 0 { + return err + } + + s := err.Error() + for _, toRedact := range redactStrings { + s = strings.Replace(s, toRedact, redactString(toRedact), -1) + } + + // Stringify and and replace any paths that we found above, then return + // the error wrapped in a type that uses the newly-redacted string + // while also allowing Unwrap()-ing to the inner error type(s). + return &redactedErr{msg: s, inner: err} } func touchFile(path string) error { diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index 2da8d7a95..4566e0b9f 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/ipn/ipnlocal/peerapi_test.go @@ -6,6 +6,7 @@ package ipnlocal import ( "bytes" + "errors" "fmt" "io" "io/fs" @@ -685,3 +686,67 @@ 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 := 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) + } + }) + }) + } +}