cmd/tailscale/cli, ipn: move exit node IP parsing and validation from cli into prefs.

We need to be able to provide the ability for the GUI clients to resolve and set
the exit node IP from an untrusted string, thus enabling the ability to specify
that information via enterprise policy.

This patch moves the relevant code out of the handler for `tailscale up`,
into a method on `Prefs` that may then be called by GUI clients.

We also update tests accordingly.

Updates https://github.com/tailscale/corp/issues/4239

Signed-off-by: Aaron Klotz <aaron@tailscale.com>

cmd/tailscale/cli/cli_test.go

@@ -19,7 +19,6 @@ import (
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tstest"
-	"tailscale.com/types/key"
 	"tailscale.com/types/persist"
 	"tailscale.com/types/preftype"
 	"tailscale.com/version/distro"
@@ -632,7 +631,7 @@ func TestPrefsFromUpArgs(t *testing.T) {
 			st: &ipnstate.Status{
 				TailscaleIPs: []netaddr.IP{netaddr.MustParseIP("100.105.106.107")},
 			},
-			wantErr: `cannot use 100.105.106.107 as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?`,
+			wantErr: `cannot use 100.105.106.107 as an exit node as it is a local IP address to this machine; did you mean --advertise-exit-node?`,
 		},
 		{
 			name: "warn_linux_netfilter_nodivert",
@@ -897,136 +896,6 @@ var cmpIP = cmp.Comparer(func(a, b netaddr.IP) bool {
 	return a == b
 })
 
-func TestExitNodeIPOfArg(t *testing.T) {
-	mustIP := netaddr.MustParseIP
-	tests := []struct {
-		name    string
-		arg     string
-		st      *ipnstate.Status
-		want    netaddr.IP
-		wantErr string
-	}{
-		{
-			name: "ip_while_stopped_okay",
-			arg:  "1.2.3.4",
-			st: &ipnstate.Status{
-				BackendState: "Stopped",
-			},
-			want: mustIP("1.2.3.4"),
-		},
-		{
-			name: "ip_not_found",
-			arg:  "1.2.3.4",
-			st: &ipnstate.Status{
-				BackendState: "Running",
-			},
-			wantErr: `no node found in netmap with IP 1.2.3.4`,
-		},
-		{
-			name: "ip_not_exit",
-			arg:  "1.2.3.4",
-			st: &ipnstate.Status{
-				BackendState: "Running",
-				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
-					key.NewNode().Public(): {
-						TailscaleIPs: []netaddr.IP{mustIP("1.2.3.4")},
-					},
-				},
-			},
-			wantErr: `node 1.2.3.4 is not advertising an exit node`,
-		},
-		{
-			name: "ip",
-			arg:  "1.2.3.4",
-			st: &ipnstate.Status{
-				BackendState: "Running",
-				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
-					key.NewNode().Public(): {
-						TailscaleIPs:   []netaddr.IP{mustIP("1.2.3.4")},
-						ExitNodeOption: true,
-					},
-				},
-			},
-			want: mustIP("1.2.3.4"),
-		},
-		{
-			name:    "no_match",
-			arg:     "unknown",
-			st:      &ipnstate.Status{MagicDNSSuffix: ".foo"},
-			wantErr: `invalid value "unknown" for --exit-node; must be IP or unique node name`,
-		},
-		{
-			name: "name",
-			arg:  "skippy",
-			st: &ipnstate.Status{
-				MagicDNSSuffix: ".foo",
-				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
-					key.NewNode().Public(): {
-						DNSName:        "skippy.foo.",
-						TailscaleIPs:   []netaddr.IP{mustIP("1.0.0.2")},
-						ExitNodeOption: true,
-					},
-				},
-			},
-			want: mustIP("1.0.0.2"),
-		},
-		{
-			name: "name_not_exit",
-			arg:  "skippy",
-			st: &ipnstate.Status{
-				MagicDNSSuffix: ".foo",
-				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
-					key.NewNode().Public(): {
-						DNSName:      "skippy.foo.",
-						TailscaleIPs: []netaddr.IP{mustIP("1.0.0.2")},
-					},
-				},
-			},
-			wantErr: `node "skippy" is not advertising an exit node`,
-		},
-		{
-			name: "ambiguous",
-			arg:  "skippy",
-			st: &ipnstate.Status{
-				MagicDNSSuffix: ".foo",
-				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
-					key.NewNode().Public(): {
-						DNSName:        "skippy.foo.",
-						TailscaleIPs:   []netaddr.IP{mustIP("1.0.0.2")},
-						ExitNodeOption: true,
-					},
-					key.NewNode().Public(): {
-						DNSName:        "SKIPPY.foo.",
-						TailscaleIPs:   []netaddr.IP{mustIP("1.0.0.2")},
-						ExitNodeOption: true,
-					},
-				},
-			},
-			wantErr: `ambiguous exit node name "skippy"`,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := exitNodeIPOfArg(tt.arg, tt.st)
-			if err != nil {
-				if err.Error() == tt.wantErr {
-					return
-				}
-				if tt.wantErr == "" {
-					t.Fatal(err)
-				}
-				t.Fatalf("error = %#q; want %#q", err, tt.wantErr)
-			}
-			if tt.wantErr != "" {
-				t.Fatalf("got %v; want error %#q", got, tt.wantErr)
-			}
-			if got != tt.want {
-				t.Fatalf("got %v; want %v", got, tt.want)
-			}
-		})
-	}
-}
-
 func TestCleanUpArgs(t *testing.T) {
 	c := qt.New(t)
 	tests := []struct {

cmd/tailscale/cli/up.go

@@ -31,7 +31,6 @@ import (
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/logger"
 	"tailscale.com/types/preftype"
-	"tailscale.com/util/dnsname"
 	"tailscale.com/version"
 	"tailscale.com/version/distro"
 )
@@ -245,65 +244,6 @@ func calcAdvertiseRoutes(advertiseRoutes string, advertiseDefaultRoute bool) ([]
 	return routes, nil
 }
 
-// peerWithTailscaleIP returns the peer in st with the provided
-// Tailscale IP.
-func peerWithTailscaleIP(st *ipnstate.Status, ip netaddr.IP) (ps *ipnstate.PeerStatus, ok bool) {
-	for _, ps := range st.Peer {
-		for _, ip2 := range ps.TailscaleIPs {
-			if ip == ip2 {
-				return ps, true
-			}
-		}
-	}
-	return nil, false
-}
-
-// exitNodeIPOfArg maps from a user-provided CLI flag value to an IP
-// address they want to use as an exit node.
-func exitNodeIPOfArg(arg string, st *ipnstate.Status) (ip netaddr.IP, err error) {
-	if arg == "" {
-		return ip, errors.New("invalid use of exitNodeIPOfArg with empty string")
-	}
-	ip, err = netaddr.ParseIP(arg)
-	if err == nil {
-		// If we're online already and have a netmap, double check that the IP
-		// address specified is valid.
-		if st.BackendState == "Running" {
-			ps, ok := peerWithTailscaleIP(st, ip)
-			if !ok {
-				return ip, fmt.Errorf("no node found in netmap with IP %v", ip)
-			}
-			if !ps.ExitNodeOption {
-				return ip, fmt.Errorf("node %v is not advertising an exit node", ip)
-			}
-		}
-		return ip, err
-	}
-	match := 0
-	for _, ps := range st.Peer {
-		baseName := dnsname.TrimSuffix(ps.DNSName, st.MagicDNSSuffix)
-		if !strings.EqualFold(arg, baseName) {
-			continue
-		}
-		match++
-		if len(ps.TailscaleIPs) == 0 {
-			return ip, fmt.Errorf("node %q has no Tailscale IP?", arg)
-		}
-		if !ps.ExitNodeOption {
-			return ip, fmt.Errorf("node %q is not advertising an exit node", arg)
-		}
-		ip = ps.TailscaleIPs[0]
-	}
-	switch match {
-	case 0:
-		return ip, fmt.Errorf("invalid value %q for --exit-node; must be IP or unique node name", arg)
-	case 1:
-		return ip, nil
-	default:
-		return ip, fmt.Errorf("ambiguous exit node name %q", arg)
-	}
-}
-
 // prefsFromUpArgs returns the ipn.Prefs for the provided args.
 //
 // Note that the parameters upArgs and warnf are named intentionally
@@ -316,25 +256,10 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo
 		return nil, err
 	}
 
-	var exitNodeIP netaddr.IP
+	if upArgs.exitNodeIP == "" && upArgs.exitNodeAllowLANAccess {
-	if upArgs.exitNodeIP != "" {
-		var err error
-		exitNodeIP, err = exitNodeIPOfArg(upArgs.exitNodeIP, st)
-		if err != nil {
-			return nil, err
-		}
-	} else if upArgs.exitNodeAllowLANAccess {
 		return nil, fmt.Errorf("--exit-node-allow-lan-access can only be used with --exit-node")
 	}
 
-	if upArgs.exitNodeIP != "" {
-		for _, ip := range st.TailscaleIPs {
-			if exitNodeIP == ip {
-				return nil, fmt.Errorf("cannot use %s as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?", upArgs.exitNodeIP)
-			}
-		}
-	}
-
 	var tags []string
 	if upArgs.advertiseTags != "" {
 		tags = strings.Split(upArgs.advertiseTags, ",")
@@ -354,7 +279,17 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo
 	prefs.ControlURL = upArgs.server
 	prefs.WantRunning = true
 	prefs.RouteAll = upArgs.acceptRoutes
-	prefs.ExitNodeIP = exitNodeIP
+
+	if upArgs.exitNodeIP != "" {
+		if err := prefs.SetExitNodeIP(upArgs.exitNodeIP, st); err != nil {
+			var e ipn.ExitNodeLocalIPError
+			if errors.As(err, &e) {
+				return nil, fmt.Errorf("%w; did you mean --advertise-exit-node?", err)
+			}
+			return nil, err
+		}
+	}
+
 	prefs.ExitNodeAllowLANAccess = upArgs.exitNodeAllowLANAccess
 	prefs.CorpDNS = upArgs.acceptDNS
 	prefs.AllowSingleHosts = upArgs.singleRoutes

ipn/prefs.go

@@ -7,6 +7,7 @@ package ipn
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"log"
@@ -18,10 +19,12 @@ import (
 
 	"inet.af/netaddr"
 	"tailscale.com/atomicfile"
+	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/net/tsaddr"
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/persist"
 	"tailscale.com/types/preftype"
+	"tailscale.com/util/dnsname"
 )
 
 //go:generate go run tailscale.com/cmd/cloner -type=Prefs -output=prefs_clone.go
@@ -31,6 +34,12 @@ import (
 // The default control plane is the hosted version run by Tailscale.com.
 const DefaultControlURL = "https://controlplane.tailscale.com"
 
+var (
+	// ErrExitNodeIDAlreadySet is returned from (*Prefs).SetExitNodeIP when the
+	// Prefs.ExitNodeID field is already set.
+	ErrExitNodeIDAlreadySet = errors.New("cannot set ExitNodeIP when ExitNodeID is already set")
+)
+
 // IsLoginServerSynonym reports whether a URL is a drop-in replacement
 // for the primary Tailscale login server.
 func IsLoginServerSynonym(val interface{}) bool {
@@ -467,6 +476,109 @@ func (p *Prefs) SetAdvertiseExitNode(runExit bool) {
 		netaddr.IPPrefixFrom(netaddr.IPv6Unspecified(), 0))
 }
 
+// peerWithTailscaleIP returns the peer in st with the provided
+// Tailscale IP.
+func peerWithTailscaleIP(st *ipnstate.Status, ip netaddr.IP) (ps *ipnstate.PeerStatus, ok bool) {
+	for _, ps := range st.Peer {
+		for _, ip2 := range ps.TailscaleIPs {
+			if ip == ip2 {
+				return ps, true
+			}
+		}
+	}
+	return nil, false
+}
+
+func isRemoteIP(st *ipnstate.Status, ip netaddr.IP) bool {
+	for _, selfIP := range st.TailscaleIPs {
+		if ip == selfIP {
+			return false
+		}
+	}
+	return true
+}
+
+// ClearExitNode sets the ExitNodeID and ExitNodeIP to their zero values.
+func (p *Prefs) ClearExitNode() {
+	p.ExitNodeID = ""
+	p.ExitNodeIP = netaddr.IP{}
+}
+
+// ExitNodeLocalIPError is returned when the requested IP address for an exit
+// node belongs to the local machine.
+type ExitNodeLocalIPError struct {
+	hostOrIP string
+}
+
+func (e ExitNodeLocalIPError) Error() string {
+	return fmt.Sprintf("cannot use %s as an exit node as it is a local IP address to this machine", e.hostOrIP)
+}
+
+func exitNodeIPOfArg(s string, st *ipnstate.Status) (ip netaddr.IP, err error) {
+	if s == "" {
+		return ip, os.ErrInvalid
+	}
+	ip, err = netaddr.ParseIP(s)
+	if err == nil {
+		// If we're online already and have a netmap, double check that the IP
+		// address specified is valid.
+		if st.BackendState == "Running" {
+			ps, ok := peerWithTailscaleIP(st, ip)
+			if !ok {
+				return ip, fmt.Errorf("no node found in netmap with IP %v", ip)
+			}
+			if !ps.ExitNodeOption {
+				return ip, fmt.Errorf("node %v is not advertising an exit node", ip)
+			}
+		}
+		if !isRemoteIP(st, ip) {
+			return ip, ExitNodeLocalIPError{s}
+		}
+		return ip, nil
+	}
+	match := 0
+	for _, ps := range st.Peer {
+		baseName := dnsname.TrimSuffix(ps.DNSName, st.MagicDNSSuffix)
+		if !strings.EqualFold(s, baseName) {
+			continue
+		}
+		match++
+		if len(ps.TailscaleIPs) == 0 {
+			return ip, fmt.Errorf("node %q has no Tailscale IP?", s)
+		}
+		if !ps.ExitNodeOption {
+			return ip, fmt.Errorf("node %q is not advertising an exit node", s)
+		}
+		ip = ps.TailscaleIPs[0]
+	}
+	switch match {
+	case 0:
+		return ip, fmt.Errorf("invalid value %q for --exit-node; must be IP or unique node name", s)
+	case 1:
+		if !isRemoteIP(st, ip) {
+			return ip, ExitNodeLocalIPError{s}
+		}
+		return ip, nil
+	default:
+		return ip, fmt.Errorf("ambiguous exit node name %q", s)
+	}
+}
+
+// SetExitNodeIP validates and sets the ExitNodeIP from a user-provided string
+// specifying either an IP address or a MagicDNS base name ("foo", as opposed to
+// "foo.bar.beta.tailscale.net"). This method does not mutate ExitNodeID and
+// will fail if ExitNodeID is already set.
+func (p *Prefs) SetExitNodeIP(s string, st *ipnstate.Status) error {
+	if !p.ExitNodeID.IsZero() {
+		return ErrExitNodeIDAlreadySet
+	}
+	ip, err := exitNodeIPOfArg(s, st)
+	if err == nil {
+		p.ExitNodeIP = ip
+	}
+	return err
+}
+
 // PrefsFromBytes deserializes Prefs from a JSON blob. If
 // enforceDefaults is true, Prefs.RouteAll and Prefs.AllowSingleHosts
 // are forced on.

ipn/prefs_test.go

@@ -17,6 +17,7 @@ import (
 
 	"go4.org/mem"
 	"inet.af/netaddr"
+	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tstest"
 	"tailscale.com/types/key"
@@ -679,3 +680,133 @@ func TestPrefsExitNode(t *testing.T) {
 		t.Errorf("routes = %d; want %d", got, want)
 	}
 }
+
+func TestExitNodeIPOfArg(t *testing.T) {
+	mustIP := netaddr.MustParseIP
+	tests := []struct {
+		name    string
+		arg     string
+		st      *ipnstate.Status
+		want    netaddr.IP
+		wantErr string
+	}{
+		{
+			name: "ip_while_stopped_okay",
+			arg:  "1.2.3.4",
+			st: &ipnstate.Status{
+				BackendState: "Stopped",
+			},
+			want: mustIP("1.2.3.4"),
+		},
+		{
+			name: "ip_not_found",
+			arg:  "1.2.3.4",
+			st: &ipnstate.Status{
+				BackendState: "Running",
+			},
+			wantErr: `no node found in netmap with IP 1.2.3.4`,
+		},
+		{
+			name: "ip_not_exit",
+			arg:  "1.2.3.4",
+			st: &ipnstate.Status{
+				BackendState: "Running",
+				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
+					key.NewNode().Public(): {
+						TailscaleIPs: []netaddr.IP{mustIP("1.2.3.4")},
+					},
+				},
+			},
+			wantErr: `node 1.2.3.4 is not advertising an exit node`,
+		},
+		{
+			name: "ip",
+			arg:  "1.2.3.4",
+			st: &ipnstate.Status{
+				BackendState: "Running",
+				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
+					key.NewNode().Public(): {
+						TailscaleIPs:   []netaddr.IP{mustIP("1.2.3.4")},
+						ExitNodeOption: true,
+					},
+				},
+			},
+			want: mustIP("1.2.3.4"),
+		},
+		{
+			name:    "no_match",
+			arg:     "unknown",
+			st:      &ipnstate.Status{MagicDNSSuffix: ".foo"},
+			wantErr: `invalid value "unknown" for --exit-node; must be IP or unique node name`,
+		},
+		{
+			name: "name",
+			arg:  "skippy",
+			st: &ipnstate.Status{
+				MagicDNSSuffix: ".foo",
+				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
+					key.NewNode().Public(): {
+						DNSName:        "skippy.foo.",
+						TailscaleIPs:   []netaddr.IP{mustIP("1.0.0.2")},
+						ExitNodeOption: true,
+					},
+				},
+			},
+			want: mustIP("1.0.0.2"),
+		},
+		{
+			name: "name_not_exit",
+			arg:  "skippy",
+			st: &ipnstate.Status{
+				MagicDNSSuffix: ".foo",
+				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
+					key.NewNode().Public(): {
+						DNSName:      "skippy.foo.",
+						TailscaleIPs: []netaddr.IP{mustIP("1.0.0.2")},
+					},
+				},
+			},
+			wantErr: `node "skippy" is not advertising an exit node`,
+		},
+		{
+			name: "ambiguous",
+			arg:  "skippy",
+			st: &ipnstate.Status{
+				MagicDNSSuffix: ".foo",
+				Peer: map[key.NodePublic]*ipnstate.PeerStatus{
+					key.NewNode().Public(): {
+						DNSName:        "skippy.foo.",
+						TailscaleIPs:   []netaddr.IP{mustIP("1.0.0.2")},
+						ExitNodeOption: true,
+					},
+					key.NewNode().Public(): {
+						DNSName:        "SKIPPY.foo.",
+						TailscaleIPs:   []netaddr.IP{mustIP("1.0.0.2")},
+						ExitNodeOption: true,
+					},
+				},
+			},
+			wantErr: `ambiguous exit node name "skippy"`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := exitNodeIPOfArg(tt.arg, tt.st)
+			if err != nil {
+				if err.Error() == tt.wantErr {
+					return
+				}
+				if tt.wantErr == "" {
+					t.Fatal(err)
+				}
+				t.Fatalf("error = %#q; want %#q", err, tt.wantErr)
+			}
+			if tt.wantErr != "" {
+				t.Fatalf("got %v; want error %#q", got, tt.wantErr)
+			}
+			if got != tt.want {
+				t.Fatalf("got %v; want %v", got, tt.want)
+			}
+		})
+	}
+}