From 955ad12489e371d12c3855c48ad4cb3c8ce133f3 Mon Sep 17 00:00:00 2001
From: Percy Wegmann <percy@tailscale.com>
Date: Tue, 23 Apr 2024 16:11:04 -0500
Subject: [PATCH] ipn/ipnlocal: only show Taildrive peers to which ACLs grant
 us access

This improves convenience and security.

* Convenience - no need to see nodes that can't share anything with you.
* Security - malicious nodes can't expose shares to peers that aren't
             allowed to access their shares.

Updates tailscale/corp#19432

Signed-off-by: Percy Wegmann <percy@tailscale.com>
---
 drive/remote_permissions.go |  2 +-
 ipn/ipnlocal/drive.go       | 51 +++++++++++++++++++++----------------
 tailcfg/tailcfg.go          |  9 +++++--
 3 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/drive/remote_permissions.go b/drive/remote_permissions.go
index 2188ea3c6..d3d41c6ec 100644
--- a/drive/remote_permissions.go
+++ b/drive/remote_permissions.go
@@ -39,7 +39,7 @@ func ParsePermissions(rawGrants [][]byte) (Permissions, error) {
 		var g grant
 		err := json.Unmarshal(rawGrant, &g)
 		if err != nil {
-			return nil, fmt.Errorf("unmarshal raw grants: %v", err)
+			return nil, fmt.Errorf("unmarshal raw grants %s: %v", rawGrant, err)
 		}
 		for _, share := range g.Shares {
 			existingPermission := permissions[share]
diff --git a/ipn/ipnlocal/drive.go b/ipn/ipnlocal/drive.go
index 419058643..d884e4c40 100644
--- a/ipn/ipnlocal/drive.go
+++ b/ipn/ipnlocal/drive.go
@@ -4,10 +4,10 @@
 package ipnlocal
 
 import (
+	"cmp"
 	"fmt"
 	"os"
 	"slices"
-	"strings"
 
 	"tailscale.com/drive"
 	"tailscale.com/ipn"
@@ -318,40 +318,47 @@ func (b *LocalBackend) updateDrivePeersLocked(nm *netmap.NetworkMap) {
 func (b *LocalBackend) driveRemotesFromPeers(nm *netmap.NetworkMap) []*drive.Remote {
 	driveRemotes := make([]*drive.Remote, 0, len(nm.Peers))
 	for _, p := range nm.Peers {
-		// Exclude mullvad exit nodes from list of Taildrive peers
-		// TODO(oxtoacart) - once we have a better mechanism for finding only accessible sharers
-		// (see below) we can remove this logic.
-		if strings.HasSuffix(p.Name(), ".mullvad.ts.net.") {
-			continue
-		}
-
 		peerID := p.ID()
 		url := fmt.Sprintf("%s/%s", peerAPIBase(nm, p), taildrivePrefix[1:])
 		driveRemotes = append(driveRemotes, &drive.Remote{
 			Name: p.DisplayName(false),
 			URL:  url,
 			Available: func() bool {
-				// TODO(oxtoacart): need to figure out a performant and reliable way to only
-				// show the peers that have shares to which we have access
-				// This will require work on the control server to transmit the inverse
-				// of the "tailscale.com/cap/drive" capability.
-				// For now, at least limit it only to nodes that are online.
-				// Note, we have to iterate the latest netmap because the peer we got from the first iteration may not be it
+				// Peers are available to Taildrive if:
+				// - They are online
+				// - They are allowed to share at least one folder with us
 				b.mu.Lock()
 				latestNetMap := b.netMap
 				b.mu.Unlock()
 
-				for _, candidate := range latestNetMap.Peers {
-					if candidate.ID() == peerID {
-						online := candidate.Online()
-						// TODO(oxtoacart): for some reason, this correctly
-						// catches when a node goes from offline to online,
-						// but not the other way around...
-						return online != nil && *online
+				idx, found := slices.BinarySearchFunc(latestNetMap.Peers, peerID, func(candidate tailcfg.NodeView, id tailcfg.NodeID) int {
+					return cmp.Compare(candidate.ID(), id)
+				})
+				if !found {
+					return false
+				}
+
+				peer := latestNetMap.Peers[idx]
+
+				// Exclude offline peers.
+				// TODO(oxtoacart): for some reason, this correctly
+				// catches when a node goes from offline to online,
+				// but not the other way around...
+				online := peer.Online()
+				if online == nil || !*online {
+					return false
+				}
+
+				// Check that the peer is allowed to share with us.
+				addresses := peer.Addresses()
+				for i := range addresses.Len() {
+					addr := addresses.At(i)
+					capsMap := b.PeerCaps(addr.Addr())
+					if capsMap.HasCapability(tailcfg.PeerCapabilityTaildriveSharer) {
+						return true
 					}
 				}
 
-				// peer not found, must not be available
 				return false
 			},
 		})
diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go
index 565c97799..5f4bf7f8b 100644
--- a/tailcfg/tailcfg.go
+++ b/tailcfg/tailcfg.go
@@ -131,7 +131,8 @@
 //   - 88: 2024-03-05: Client understands NodeAttrSuggestExitNode
 //   - 89: 2024-03-23: Client no longer respects deleted PeerChange.Capabilities (use CapMap)
 //   - 90: 2024-04-03: Client understands PeerCapabilityTaildrive.
-const CurrentCapabilityVersion CapabilityVersion = 90
+//   - 91: 2024-04-24: Client understands PeerCapabilityTaildriveSharer.
+const CurrentCapabilityVersion CapabilityVersion = 91
 
 type StableID string
 
@@ -1357,8 +1358,12 @@ type CapGrant struct {
 	// PeerCapabilityWebUI grants the ability for a peer to edit features from the
 	// device Web UI.
 	PeerCapabilityWebUI PeerCapability = "tailscale.com/cap/webui"
-	// PeerCapabilityTaildrive grants the ability for a peer to access Taildrive shares.
+	// PeerCapabilityTaildrive grants the ability for a peer to access Taildrive
+	// shares.
 	PeerCapabilityTaildrive PeerCapability = "tailscale.com/cap/drive"
+	// PeerCapabilityTaildriveSharer indicates that a peer has the ability to
+	// share folders with us.
+	PeerCapabilityTaildriveSharer PeerCapability = "tailscale.com/cap/drive-sharer"
 )
 
 // NodeCapMap is a map of capabilities to their optional values. It is valid for