From f8a4df66de7cd08dc209ba8af3b07501f9395135 Mon Sep 17 00:00:00 2001 From: Aaron Klotz Date: Fri, 25 Feb 2022 16:36:05 -0700 Subject: [PATCH] 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 --- cmd/tailscale/cli/cli_test.go | 133 +--------------------------------- cmd/tailscale/cli/up.go | 89 +++-------------------- ipn/prefs.go | 112 ++++++++++++++++++++++++++++ ipn/prefs_test.go | 131 +++++++++++++++++++++++++++++++++ 4 files changed, 256 insertions(+), 209 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index d87df2974..639fcc1a2 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/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 { diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 5f7c543db..91436f808 100644 --- a/cmd/tailscale/cli/up.go +++ b/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 != "" { - var err error - exitNodeIP, err = exitNodeIPOfArg(upArgs.exitNodeIP, st) - if err != nil { - return nil, err - } - } else if upArgs.exitNodeAllowLANAccess { + if upArgs.exitNodeIP == "" && 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 diff --git a/ipn/prefs.go b/ipn/prefs.go index 330fd073d..f7d34cf53 100644 --- a/ipn/prefs.go +++ b/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. diff --git a/ipn/prefs_test.go b/ipn/prefs_test.go index d9204b901..043f7e557 100644 --- a/ipn/prefs_test.go +++ b/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) + } + }) + } +}