From e496451928d941d1012d696db67ae412d3984682 Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Fri, 8 Mar 2024 10:43:32 -0600 Subject: [PATCH] ipn,cmd/tailscale,client/tailscale: add support for renaming TailFS shares - Updates API to support renaming TailFS shares. - Adds a CLI rename subcommand for renaming a share. - Renames the CLI subcommand 'add' to 'set' to make it clear that this is an add or update. - Adds a unit test for TailFS in ipnlocal Updates tailscale/corp#16827 Signed-off-by: Percy Wegmann --- client/tailscale/localclient.go | 23 +++- cmd/tailscale/cli/share.go | 52 +++++-- ipn/ipnlocal/local_test.go | 231 ++++++++++++++++++++++++++++++++ ipn/ipnlocal/tailfs.go | 96 +++++++++++-- ipn/ipnlocal/tailfs_test.go | 4 +- ipn/localapi/localapi.go | 43 +++++- 6 files changed, 411 insertions(+), 38 deletions(-) diff --git a/client/tailscale/localclient.go b/client/tailscale/localclient.go index 5473d5e07..644d5d87e 100644 --- a/client/tailscale/localclient.go +++ b/client/tailscale/localclient.go @@ -1426,10 +1426,10 @@ func (lc *LocalClient) TailFSSetFileServerAddr(ctx context.Context, addr string) return err } -// TailFSShareAdd adds the given share to the list of shares that TailFS will -// serve to remote nodes. If a share with the same name already exists, the -// existing share is replaced/updated. -func (lc *LocalClient) TailFSShareAdd(ctx context.Context, share *tailfs.Share) error { +// TailFSShareSet adds or updates the given share in the list of shares that +// TailFS will serve to remote nodes. If a share with the same name already +// exists, the existing share is replaced/updated. +func (lc *LocalClient) TailFSShareSet(ctx context.Context, share *tailfs.Share) error { _, err := lc.send(ctx, "PUT", "/localapi/v0/tailfs/shares", http.StatusCreated, jsonBody(share)) return err } @@ -1442,9 +1442,18 @@ func (lc *LocalClient) TailFSShareRemove(ctx context.Context, name string) error "DELETE", "/localapi/v0/tailfs/shares", http.StatusNoContent, - jsonBody(&tailfs.Share{ - Name: name, - })) + strings.NewReader(name)) + return err +} + +// TailFSShareRename renames the share from old to new name. +func (lc *LocalClient) TailFSShareRename(ctx context.Context, oldName, newName string) error { + _, err := lc.send( + ctx, + "POST", + "/localapi/v0/tailfs/shares", + http.StatusNoContent, + jsonBody([2]string{oldName, newName})) return err } diff --git a/cmd/tailscale/cli/share.go b/cmd/tailscale/cli/share.go index 6a530b7c5..b9481e2eb 100644 --- a/cmd/tailscale/cli/share.go +++ b/cmd/tailscale/cli/share.go @@ -14,7 +14,8 @@ import ( ) const ( - shareAddUsage = "share add " + shareSetUsage = "share set " + shareRenameUsage = "share rename " shareRemoveUsage = "share remove " shareListUsage = "share list" ) @@ -23,7 +24,7 @@ var shareCmd = &ffcli.Command{ Name: "share", ShortHelp: "Share a directory with your tailnet", ShortUsage: strings.Join([]string{ - shareAddUsage, + shareSetUsage, shareRemoveUsage, shareListUsage, }, "\n "), @@ -31,9 +32,15 @@ var shareCmd = &ffcli.Command{ UsageFunc: usageFuncNoDefaultValues, Subcommands: []*ffcli.Command{ { - Name: "add", - Exec: runShareAdd, - ShortHelp: "[ALPHA] add a share", + Name: "set", + Exec: runShareSet, + ShortHelp: "[ALPHA] set a share", + UsageFunc: usageFunc, + }, + { + Name: "rename", + ShortHelp: "[ALPHA] rename a share", + Exec: runShareRename, UsageFunc: usageFunc, }, { @@ -54,20 +61,20 @@ var shareCmd = &ffcli.Command{ }, } -// runShareAdd is the entry point for the "tailscale share add" command. -func runShareAdd(ctx context.Context, args []string) error { +// runShareSet is the entry point for the "tailscale share set" command. +func runShareSet(ctx context.Context, args []string) error { if len(args) != 2 { - return fmt.Errorf("usage: tailscale %v", shareAddUsage) + return fmt.Errorf("usage: tailscale %v", shareSetUsage) } name, path := args[0], args[1] - err := localClient.TailFSShareAdd(ctx, &tailfs.Share{ + err := localClient.TailFSShareSet(ctx, &tailfs.Share{ Name: name, Path: path, }) if err == nil { - fmt.Printf("Added share %q at %q\n", name, path) + fmt.Printf("Set share %q at %q\n", name, path) } return err } @@ -86,6 +93,21 @@ func runShareRemove(ctx context.Context, args []string) error { return err } +// runShareRename is the entry point for the "tailscale share rename" command. +func runShareRename(ctx context.Context, args []string) error { + if len(args) != 2 { + return fmt.Errorf("usage: tailscale %v", shareRenameUsage) + } + oldName := args[0] + newName := args[1] + + err := localClient.TailFSShareRename(ctx, oldName, newName) + if err == nil { + fmt.Printf("Renamed share %q to %q\n", oldName, newName) + } + return err +} + // runShareList is the entry point for the "tailscale share list" command. func runShareList(ctx context.Context, args []string) error { if len(args) != 0 { @@ -148,7 +170,7 @@ For example, to enable sharing and accessing shares for all member nodes: Each share is identified by a name and points to a directory at a specific path. For example, to share the path /Users/me/Documents under the name "docs", you would run: - $ tailscale share add docs /Users/me/Documents + $ tailscale share set docs /Users/me/Documents Note that the system forces share names to lowercase to avoid problems with clients that don't support case-sensitive filenames. @@ -202,9 +224,13 @@ To categorically give yourself access to all your shares, you can use the below Whenever either you or anyone in the group "home" connects to the share, they connect as if they are using your local machine user. They'll be able to read the same files as your user and if they create files, those files will be owned by your user.%s +You can rename shares, for example you could rename the above share by running: + + $ tailscale share rename docs newdocs + You can remove shares by name, for example you could remove the above share by running: - $ tailscale share remove docs + $ tailscale share remove newdocs You can get a list of currently published shares by running: @@ -214,4 +240,4 @@ var shareLongHelpAs = ` If you want a share to be accessed as a different user, you can use sudo to accomplish this. For example, to create the aforementioned share as "theuser", you could run: - $ sudo -u theuser tailscale share add docs /Users/theuser/Documents` + $ sudo -u theuser tailscale share set docs /Users/theuser/Documents` diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index c2e6f1cbd..d37020ce4 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -5,16 +5,20 @@ package ipnlocal import ( "context" + "encoding/json" "errors" "fmt" "net" "net/http" "net/netip" + "os" "reflect" "slices" + "sync" "testing" "time" + "github.com/google/go-cmp/cmp" "go4.org/netipx" "golang.org/x/net/dns/dnsmessage" "tailscale.com/appc" @@ -25,6 +29,8 @@ import ( "tailscale.com/net/interfaces" "tailscale.com/net/tsaddr" "tailscale.com/tailcfg" + "tailscale.com/tailfs" + "tailscale.com/tailfs/tailfsimpl" "tailscale.com/tsd" "tailscale.com/tstest" "tailscale.com/types/dnstype" @@ -34,6 +40,7 @@ import ( "tailscale.com/types/netmap" "tailscale.com/types/opt" "tailscale.com/types/ptr" + "tailscale.com/types/views" "tailscale.com/util/dnsname" "tailscale.com/util/mak" "tailscale.com/util/must" @@ -2238,3 +2245,227 @@ func TestTCPHandlerForDst(t *testing.T) { }) } } + +func TestTailFSManageShares(t *testing.T) { + tests := []struct { + name string + disabled bool + existing []*tailfs.Share + add *tailfs.Share + remove string + rename [2]string + expect any + }{ + { + name: "append", + existing: []*tailfs.Share{ + {Name: "b"}, + {Name: "d"}, + }, + add: &tailfs.Share{Name: " E "}, + expect: []*tailfs.Share{ + {Name: "b"}, + {Name: "d"}, + {Name: "e"}, + }, + }, + { + name: "prepend", + existing: []*tailfs.Share{ + {Name: "b"}, + {Name: "d"}, + }, + add: &tailfs.Share{Name: " A "}, + expect: []*tailfs.Share{ + {Name: "a"}, + {Name: "b"}, + {Name: "d"}, + }, + }, + { + name: "insert", + existing: []*tailfs.Share{ + {Name: "b"}, + {Name: "d"}, + }, + add: &tailfs.Share{Name: " C "}, + expect: []*tailfs.Share{ + {Name: "b"}, + {Name: "c"}, + {Name: "d"}, + }, + }, + { + name: "replace", + existing: []*tailfs.Share{ + {Name: "b", Path: "i"}, + {Name: "d"}, + }, + add: &tailfs.Share{Name: " B ", Path: "ii"}, + expect: []*tailfs.Share{ + {Name: "b", Path: "ii"}, + {Name: "d"}, + }, + }, + { + name: "add_bad_name", + add: &tailfs.Share{Name: "$"}, + expect: ErrInvalidShareName, + }, + { + name: "add_disabled", + disabled: true, + add: &tailfs.Share{Name: "a"}, + expect: ErrTailFSNotEnabled, + }, + { + name: "remove", + existing: []*tailfs.Share{ + {Name: "a"}, + {Name: "b"}, + {Name: "c"}, + }, + remove: "b", + expect: []*tailfs.Share{ + {Name: "a"}, + {Name: "c"}, + }, + }, + { + name: "remove_non_existing", + existing: []*tailfs.Share{ + {Name: "a"}, + {Name: "b"}, + {Name: "c"}, + }, + remove: "D", + expect: os.ErrNotExist, + }, + { + name: "remove_disabled", + disabled: true, + remove: "b", + expect: ErrTailFSNotEnabled, + }, + { + name: "rename", + existing: []*tailfs.Share{ + {Name: "a"}, + {Name: "b"}, + }, + rename: [2]string{"a", " C "}, + expect: []*tailfs.Share{ + {Name: "b"}, + {Name: "c"}, + }, + }, + { + name: "rename_not_exist", + existing: []*tailfs.Share{ + {Name: "a"}, + {Name: "b"}, + }, + rename: [2]string{"d", "c"}, + expect: os.ErrNotExist, + }, + { + name: "rename_exists", + existing: []*tailfs.Share{ + {Name: "a"}, + {Name: "b"}, + }, + rename: [2]string{"a", "b"}, + expect: os.ErrExist, + }, + { + name: "rename_bad_name", + rename: [2]string{"a", "$"}, + expect: ErrInvalidShareName, + }, + { + name: "rename_disabled", + disabled: true, + rename: [2]string{"a", "c"}, + expect: ErrTailFSNotEnabled, + }, + } + + tailfs.DisallowShareAs = true + t.Cleanup(func() { + tailfs.DisallowShareAs = false + }) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := newTestBackend(t) + b.mu.Lock() + if tt.existing != nil { + b.tailFSSetSharesLocked(tt.existing) + } + if !tt.disabled { + self := b.netMap.SelfNode.AsStruct() + self.CapMap = tailcfg.NodeCapMap{tailcfg.NodeAttrsTailFSShare: nil} + b.netMap.SelfNode = self.View() + b.sys.Set(tailfsimpl.NewFileSystemForRemote(b.logf)) + } + b.mu.Unlock() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + t.Cleanup(cancel) + + result := make(chan views.SliceView[*tailfs.Share, tailfs.ShareView], 1) + + var wg sync.WaitGroup + wg.Add(1) + go b.WatchNotifications( + ctx, + 0, + func() { wg.Done() }, + func(n *ipn.Notify) bool { + select { + case result <- n.TailFSShares: + default: + // + } + return false + }, + ) + wg.Wait() + + var err error + switch { + case tt.add != nil: + err = b.TailFSSetShare(tt.add) + case tt.remove != "": + err = b.TailFSRemoveShare(tt.remove) + default: + err = b.TailFSRenameShare(tt.rename[0], tt.rename[1]) + } + + switch e := tt.expect.(type) { + case error: + if !errors.Is(err, e) { + t.Errorf("expected error, want: %v got: %v", e, err) + } + case []*tailfs.Share: + if err != nil { + t.Errorf("unexpected error: %v", err) + } else { + r := <-result + + got, err := json.MarshalIndent(r, "", " ") + if err != nil { + t.Fatalf("can't marshal got: %v", err) + } + want, err := json.MarshalIndent(e, "", " ") + if err != nil { + t.Fatalf("can't marshal want: %v", err) + } + if diff := cmp.Diff(string(got), string(want)); diff != "" { + t.Errorf("wrong shares; (-got+want):%v", diff) + } + } + } + }) + } +} diff --git a/ipn/ipnlocal/tailfs.go b/ipn/ipnlocal/tailfs.go index 19d6202ab..6f11b1178 100644 --- a/ipn/ipnlocal/tailfs.go +++ b/ipn/ipnlocal/tailfs.go @@ -6,7 +6,9 @@ package ipnlocal import ( "errors" "fmt" + "os" "regexp" + "slices" "strings" "tailscale.com/ipn" @@ -24,7 +26,8 @@ const ( var ( shareNameRegex = regexp.MustCompile(`^[a-z0-9_\(\) ]+$`) - errInvalidShareName = errors.New("Share names may only contain the letters a-z, underscore _, parentheses (), or spaces") + ErrTailFSNotEnabled = errors.New("TailFS not enabled") + ErrInvalidShareName = errors.New("Share names may only contain the letters a-z, underscore _, parentheses (), or spaces") ) // TailFSSharingEnabled reports whether sharing to remote nodes via tailfs is @@ -59,18 +62,18 @@ func (b *LocalBackend) tailFSAccessEnabledLocked() bool { func (b *LocalBackend) TailFSSetFileServerAddr(addr string) error { fs, ok := b.sys.TailFSForRemote.GetOK() if !ok { - return errors.New("tailfs not enabled") + return ErrTailFSNotEnabled } fs.SetFileServerAddr(addr) return nil } -// TailFSAddShare adds the given share if no share with that name exists, or -// replaces the existing share if one with the same name already exists. -// To avoid potential incompatibilities across file systems, share names are +// TailFSSetShare adds the given share if no share with that name exists, or +// replaces the existing share if one with the same name already exists. To +// avoid potential incompatibilities across file systems, share names are // limited to alphanumeric characters and the underscore _. -func (b *LocalBackend) TailFSAddShare(share *tailfs.Share) error { +func (b *LocalBackend) TailFSSetShare(share *tailfs.Share) error { var err error share.Name, err = normalizeShareName(share.Name) if err != nil { @@ -78,7 +81,7 @@ func (b *LocalBackend) TailFSAddShare(share *tailfs.Share) error { } b.mu.Lock() - shares, err := b.tailFSAddShareLocked(share) + shares, err := b.tailFSSetShareLocked(share) b.mu.Unlock() if err != nil { return err @@ -99,18 +102,18 @@ func normalizeShareName(name string) (string, error) { name = strings.TrimSpace(name) if !shareNameRegex.MatchString(name) { - return "", errInvalidShareName + return "", ErrInvalidShareName } return name, nil } -func (b *LocalBackend) tailFSAddShareLocked(share *tailfs.Share) (views.SliceView[*tailfs.Share, tailfs.ShareView], error) { +func (b *LocalBackend) tailFSSetShareLocked(share *tailfs.Share) (views.SliceView[*tailfs.Share, tailfs.ShareView], error) { existingShares := b.pm.prefs.TailFSShares() fs, ok := b.sys.TailFSForRemote.GetOK() if !ok { - return existingShares, errors.New("tailfs not enabled") + return existingShares, ErrTailFSNotEnabled } addedShare := false @@ -139,6 +142,70 @@ func (b *LocalBackend) tailFSAddShareLocked(share *tailfs.Share) (views.SliceVie return b.pm.prefs.TailFSShares(), nil } +// TailFSRenameShare renames the share at old name to new name. To avoid +// potential incompatibilities across file systems, the new share name is +// limited to alphanumeric characters and the underscore _. +// Any of the following will result in an error. +// - no share found under old name +// - new share name contains disallowed characters +// - share already exists under new name +func (b *LocalBackend) TailFSRenameShare(oldName, newName string) error { + var err error + newName, err = normalizeShareName(newName) + if err != nil { + return err + } + + b.mu.Lock() + shares, err := b.tailFSRenameShareLocked(oldName, newName) + b.mu.Unlock() + if err != nil { + return err + } + + b.tailFSNotifyShares(shares) + return nil +} + +func (b *LocalBackend) tailFSRenameShareLocked(oldName, newName string) (views.SliceView[*tailfs.Share, tailfs.ShareView], error) { + existingShares := b.pm.prefs.TailFSShares() + + fs, ok := b.sys.TailFSForRemote.GetOK() + if !ok { + return existingShares, ErrTailFSNotEnabled + } + + found := false + var shares []*tailfs.Share + for i := 0; i < existingShares.Len(); i++ { + existing := existingShares.At(i) + if existing.Name() == newName { + return existingShares, os.ErrExist + } + if existing.Name() == oldName { + share := existing.AsStruct() + share.Name = newName + shares = append(shares, share) + found = true + } else { + shares = append(shares, existing.AsStruct()) + } + } + + if !found { + return existingShares, os.ErrNotExist + } + + slices.SortFunc(shares, tailfs.CompareShares) + err := b.tailFSSetSharesLocked(shares) + if err != nil { + return existingShares, err + } + fs.SetShares(shares) + + return b.pm.prefs.TailFSShares(), nil +} + // TailFSRemoveShare removes the named share. Share names are forced to // lowercase. func (b *LocalBackend) TailFSRemoveShare(name string) error { @@ -166,17 +233,24 @@ func (b *LocalBackend) tailFSRemoveShareLocked(name string) (views.SliceView[*ta fs, ok := b.sys.TailFSForRemote.GetOK() if !ok { - return existingShares, errors.New("tailfs not enabled") + return existingShares, ErrTailFSNotEnabled } + found := false var shares []*tailfs.Share for i := 0; i < existingShares.Len(); i++ { existing := existingShares.At(i) if existing.Name() != name { shares = append(shares, existing.AsStruct()) + } else { + found = true } } + if !found { + return existingShares, os.ErrNotExist + } + err := b.tailFSSetSharesLocked(shares) if err != nil { return existingShares, err diff --git a/ipn/ipnlocal/tailfs_test.go b/ipn/ipnlocal/tailfs_test.go index 1462c40bf..dade9583f 100644 --- a/ipn/ipnlocal/tailfs_test.go +++ b/ipn/ipnlocal/tailfs_test.go @@ -20,11 +20,11 @@ func TestNormalizeShareName(t *testing.T) { }, { name: "", - err: errInvalidShareName, + err: ErrInvalidShareName, }, { name: "generally good except for .", - err: errInvalidShareName, + err: ErrInvalidShareName, }, } for _, tt := range tests { diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index f6f142cfb..a361d8030 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -2571,9 +2571,14 @@ func (h *Handler) serveTailFSFileServerAddr(w http.ResponseWriter, r *http.Reque } // serveShares handles the management of tailfs shares. +// +// PUT - adds or updates an existing share +// DELETE - removes a share +// GET - gets a list of all shares, sorted by name +// POST - renames an existing share func (h *Handler) serveShares(w http.ResponseWriter, r *http.Request) { if !h.b.TailFSSharingEnabled() { - http.Error(w, `tailfs sharing not enabled, please add the attribute "tailfs:share" to this node in your ACLs' "nodeAttrs" section`, http.StatusInternalServerError) + http.Error(w, `tailfs sharing not enabled, please add the attribute "tailfs:share" to this node in your ACLs' "nodeAttrs" section`, http.StatusForbidden) return } switch r.Method { @@ -2603,20 +2608,23 @@ func (h *Handler) serveShares(w http.ResponseWriter, r *http.Request) { } share.As = username } - err = h.b.TailFSAddShare(&share) + err = h.b.TailFSSetShare(&share) if err != nil { + if errors.Is(err, ipnlocal.ErrInvalidShareName) { + http.Error(w, "invalid share name", http.StatusBadRequest) + return + } http.Error(w, err.Error(), http.StatusInternalServerError) return } w.WriteHeader(http.StatusCreated) case "DELETE": - var share tailfs.Share - err := json.NewDecoder(r.Body).Decode(&share) + b, err := io.ReadAll(r.Body) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - err = h.b.TailFSRemoveShare(share.Name) + err = h.b.TailFSRemoveShare(string(b)) if err != nil { if os.IsNotExist(err) { http.Error(w, "share not found", http.StatusNotFound) @@ -2626,6 +2634,31 @@ func (h *Handler) serveShares(w http.ResponseWriter, r *http.Request) { return } w.WriteHeader(http.StatusNoContent) + case "POST": + var names [2]string + err := json.NewDecoder(r.Body).Decode(&names) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + err = h.b.TailFSRenameShare(names[0], names[1]) + if err != nil { + if os.IsNotExist(err) { + http.Error(w, "share not found", http.StatusNotFound) + return + } + if os.IsExist(err) { + http.Error(w, "share name already used", http.StatusBadRequest) + return + } + if errors.Is(err, ipnlocal.ErrInvalidShareName) { + http.Error(w, "invalid share name", http.StatusBadRequest) + return + } + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusNoContent) case "GET": shares := h.b.TailFSGetShares() err := json.NewEncoder(w).Encode(shares)