From bfdc8175b131ab3da30800786df384e8fffd862f Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 11 May 2020 20:16:52 +0000 Subject: [PATCH] wgengine/router: add a setting to disable SNAT for subnet routes. Part of #320. Signed-off-by: David Anderson --- cmd/tailscale/tailscale.go | 3 +++ ipn/local.go | 6 ++--- ipn/prefs.go | 14 ++++++++-- ipn/prefs_test.go | 13 +++++++++- wgengine/router/router.go | 1 + wgengine/router/router_linux.go | 46 +++++++++++++++++++++++++++------ wgengine/userspace.go | 11 +++++--- wgengine/watchdog.go | 4 +-- wgengine/wgengine.go | 2 +- 9 files changed, 79 insertions(+), 21 deletions(-) diff --git a/cmd/tailscale/tailscale.go b/cmd/tailscale/tailscale.go index 86b10753f..50d351804 100644 --- a/cmd/tailscale/tailscale.go +++ b/cmd/tailscale/tailscale.go @@ -56,6 +56,7 @@ func main() { upf.BoolVar(&upArgs.shieldsUp, "shields-up", false, "don't allow incoming connections") upf.StringVar(&upArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. 10.0.0.0/8,192.168.0.0/24)") upf.StringVar(&upArgs.advertiseTags, "advertise-tags", "", "ACL tags to request (comma-separated, e.g. eng,montreal,ssh)") + upf.BoolVar(&upArgs.noSNAT, "no-snat", false, "disable SNAT of traffic to local routes advertised with -advertise-routes") upf.StringVar(&upArgs.authKey, "authkey", "", "node authorization key") upCmd := &ffcli.Command{ Name: "up", @@ -105,6 +106,7 @@ var upArgs struct { shieldsUp bool advertiseRoutes string advertiseTags string + noSNAT bool authKey string } @@ -191,6 +193,7 @@ func runUp(ctx context.Context, args []string) error { prefs.ShieldsUp = upArgs.shieldsUp prefs.AdvertiseRoutes = routes prefs.AdvertiseTags = tags + prefs.NoSNAT = upArgs.noSNAT c, bc, ctx, cancel := connect(ctx) defer cancel() diff --git a/ipn/local.go b/ipn/local.go index 277529782..7edd12470 100644 --- a/ipn/local.go +++ b/ipn/local.go @@ -707,7 +707,7 @@ func (b *LocalBackend) authReconfig() { log.Fatalf("WGCfg: %v", err) } - err = b.e.Reconfig(cfg, dom, uc.AdvertiseRoutes) + err = b.e.Reconfig(cfg, dom, uc.AdvertiseRoutes, uc.NoSNAT) if err == wgengine.ErrNoChanges { return } @@ -736,7 +736,7 @@ func (b *LocalBackend) enterState(newState State) { b.blockEngineUpdates(true) fallthrough case Stopped: - err := b.e.Reconfig(&wgcfg.Config{}, nil, nil) + err := b.e.Reconfig(&wgcfg.Config{}, nil, nil, false) if err != nil { b.logf("Reconfig(down): %v", err) } @@ -812,7 +812,7 @@ func (b *LocalBackend) stateMachine() { func (b *LocalBackend) stopEngineAndWait() { b.logf("stopEngineAndWait...") - b.e.Reconfig(&wgcfg.Config{}, nil, nil) + b.e.Reconfig(&wgcfg.Config{}, nil, nil, false) b.requestEngineStatusAndWait() b.logf("stopEngineAndWait: done.") } diff --git a/ipn/prefs.go b/ipn/prefs.go index cdab720eb..2c9fe26e6 100644 --- a/ipn/prefs.go +++ b/ipn/prefs.go @@ -53,6 +53,15 @@ type Prefs struct { // the control server will allow you to take on the rights for that // tag. AdvertiseTags []string + // NoSNAT specifies whether to source NAT traffic going to + // destinations in AdvertiseRoutes. The default is to apply source + // NAT, which makes the traffic appear to come from the router + // machine rather than the peer's Tailscale IP. + // + // Disabling SNAT requires additional manual configuration in your + // network to route Tailscale traffic back to the subnet relay + // machine. + NoSNAT bool // NotepadURLs is a debugging setting that opens OAuth URLs in // notepad.exe on Windows, rather than loading them in a browser. @@ -83,9 +92,9 @@ func (p *Prefs) Pretty() string { } else { pp = "Persist=nil" } - return fmt.Sprintf("Prefs{ra=%v mesh=%v dns=%v want=%v notepad=%v derp=%v shields=%v routes=%v %v}", + return fmt.Sprintf("Prefs{ra=%v mesh=%v dns=%v want=%v notepad=%v derp=%v shields=%v routes=%v snat=%v %v}", p.RouteAll, p.AllowSingleHosts, p.CorpDNS, p.WantRunning, - p.NotepadURLs, !p.DisableDERP, p.ShieldsUp, p.AdvertiseRoutes, pp) + p.NotepadURLs, !p.DisableDERP, p.ShieldsUp, p.AdvertiseRoutes, !p.NoSNAT, pp) } func (p *Prefs) ToBytes() []byte { @@ -113,6 +122,7 @@ func (p *Prefs) Equals(p2 *Prefs) bool { p.NotepadURLs == p2.NotepadURLs && p.DisableDERP == p2.DisableDERP && p.ShieldsUp == p2.ShieldsUp && + p.NoSNAT == p2.NoSNAT && compareIPNets(p.AdvertiseRoutes, p2.AdvertiseRoutes) && compareStrings(p.AdvertiseTags, p2.AdvertiseTags) && p.Persist.Equals(p2.Persist) diff --git a/ipn/prefs_test.go b/ipn/prefs_test.go index a1b0a7913..ff3f69e83 100644 --- a/ipn/prefs_test.go +++ b/ipn/prefs_test.go @@ -20,7 +20,7 @@ func fieldsOf(t reflect.Type) (fields []string) { } func TestPrefsEqual(t *testing.T) { - prefsHandles := []string{"ControlURL", "RouteAll", "AllowSingleHosts", "CorpDNS", "WantRunning", "ShieldsUp", "AdvertiseRoutes", "AdvertiseTags", "NotepadURLs", "DisableDERP", "Persist"} + prefsHandles := []string{"ControlURL", "RouteAll", "AllowSingleHosts", "CorpDNS", "WantRunning", "ShieldsUp", "AdvertiseRoutes", "AdvertiseTags", "NoSNAT", "NotepadURLs", "DisableDERP", "Persist"} if have := fieldsOf(reflect.TypeOf(Prefs{})); !reflect.DeepEqual(have, prefsHandles) { t.Errorf("Prefs.Equal check might be out of sync\nfields: %q\nhandled: %q\n", have, prefsHandles) @@ -111,6 +111,17 @@ func TestPrefsEqual(t *testing.T) { true, }, + { + &Prefs{NoSNAT: true}, + &Prefs{NoSNAT: false}, + false, + }, + { + &Prefs{NoSNAT: true}, + &Prefs{NoSNAT: true}, + true, + }, + { &Prefs{NotepadURLs: true}, &Prefs{NotepadURLs: false}, diff --git a/wgengine/router/router.go b/wgengine/router/router.go index e10d196df..8393482ec 100644 --- a/wgengine/router/router.go +++ b/wgengine/router/router.go @@ -43,4 +43,5 @@ type Settings struct { DNSDomains []string Routes []netaddr.IPPrefix // routes to point into the Tailscale interface SubnetRoutes []netaddr.IPPrefix // subnets being advertised to other Tailscale nodes + NoSNAT bool // don't SNAT traffic to local subnets } diff --git a/wgengine/router/router_linux.go b/wgengine/router/router_linux.go index d4040038c..13a07380f 100644 --- a/wgengine/router/router_linux.go +++ b/wgengine/router/router_linux.go @@ -54,6 +54,7 @@ type linuxRouter struct { addrs map[netaddr.IPPrefix]bool routes map[netaddr.IPPrefix]bool subnetRoutes map[netaddr.IPPrefix]bool + noSNAT bool ipt4 *iptables.IPTables } @@ -72,6 +73,7 @@ func newUserspaceRouter(logf logger.Logf, _ *device.Device, tunDev tun.Device) ( return &linuxRouter{ logf: logf, tunname: tunname, + noSNAT: true, ipt4: ipt4, }, nil } @@ -193,9 +195,23 @@ func (r *linuxRouter) Set(rs Settings) error { errq = err } + switch { + case rs.NoSNAT == r.noSNAT: + // state already correct, nothing to do. + case rs.NoSNAT: + if err := r.delSNATRule(); err != nil && errq == nil { + errq = err + } + default: + if err := r.addSNATRule(); err != nil && errq == nil { + errq = err + } + } + r.addrs = newAddrs r.routes = newRoutes r.subnetRoutes = newSubnetRoutes + r.noSNAT = rs.NoSNAT // TODO: this: if false { @@ -558,13 +574,12 @@ func (r *linuxRouter) addBaseNetfilter4() error { return fmt.Errorf("adding %v in filter/ts-input: %v", args, err) } - // Forward and masquerade packets that have the Tailscale subnet - // route bit set. The bit gets set by rules inserted into - // filter/FORWARD later on. We use packet marks here so both - // filter/FORWARD and nat/POSTROUTING can match on these packets - // of interest. + // Forward and mark packets that have the Tailscale subnet route + // bit set. The bit gets set by rules inserted into filter/FORWARD + // later on. We use packet marks here so both filter/FORWARD and + // nat/POSTROUTING can match on these packets of interest. // - // In particular, we only want to apply masquerading in + // In particular, we only want to apply SNAT rules in // nat/POSTROUTING to packets that originated from the Tailscale // interface, but we can't match on the inbound interface in // POSTROUTING. So instead, we match on the inbound interface and @@ -579,11 +594,26 @@ func (r *linuxRouter) addBaseNetfilter4() error { if err := r.ipt4.Append("filter", "ts-forward", args...); err != nil { return fmt.Errorf("adding %v in filter/ts-forward: %v", args, err) } - // TODO(danderson): this should be optional. - args = []string{"-m", "mark", "--mark", tailscaleSubnetRouteMark, "-j", "MASQUERADE"} + + return nil +} + +// addSNATRule adds a netfilter rule to SNAT traffic destined for +// local subnets. +func (r *linuxRouter) addSNATRule() error { + args := []string{"-m", "mark", "--mark", tailscaleSubnetRouteMark, "-j", "MASQUERADE"} if err := r.ipt4.Append("nat", "ts-postrouting", args...); err != nil { return fmt.Errorf("adding %v in nat/ts-postrouting: %v", args, err) } + return nil +} +// delSNATRule removes the netfilter rule to SNAT traffic destined for +// local subnets. Fails if the rule does not exist. +func (r *linuxRouter) delSNATRule() error { + args := []string{"-m", "mark", "--mark", tailscaleSubnetRouteMark, "-j", "MASQUERADE"} + if err := r.ipt4.Delete("nat", "ts-postrouting", args...); err != nil { + return fmt.Errorf("deleting %v in nat/ts-postrouting: %v", args, err) + } return nil } diff --git a/wgengine/userspace.go b/wgengine/userspace.go index ea9e132ad..d21da466b 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -242,6 +242,8 @@ func newUserspaceEngineAdvanced(logf logger.Logf, tundev tun.Device, routerGen R e.wgdev.Close() return nil, err } + // TODO(danderson): we should delete this. It's pointless to apply + // a no-op settings here. if err := e.router.Set(router.Settings{}); err != nil { e.wgdev.Close() return nil, err @@ -325,14 +327,14 @@ func (e *userspaceEngine) pinger(peerKey wgcfg.Key, ips []wgcfg.IP) { } } -func configSignature(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg.CIDR) (string, error) { +func configSignature(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg.CIDR, noSNAT bool) (string, error) { // TODO(apenwarr): get rid of uapi stuff for in-process comms uapi, err := cfg.ToUAPI() if err != nil { return "", err } - return fmt.Sprintf("%s %v %v", uapi, dnsDomains, localRoutes), nil + return fmt.Sprintf("%s %v %v %v", uapi, dnsDomains, localRoutes, noSNAT), nil } // TODO(apenwarr): dnsDomains really ought to be in wgcfg.Config. @@ -344,7 +346,7 @@ func configSignature(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg // hand. Feels like we either need a wgengine.Config type, or make // router and wgengine siblings of each other that interact via glue // in ipn. -func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg.CIDR) error { +func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg.CIDR, noSNAT bool) error { e.wgLock.Lock() defer e.wgLock.Unlock() @@ -357,7 +359,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, local } e.mu.Unlock() - rc, err := configSignature(cfg, dnsDomains, localRoutes) + rc, err := configSignature(cfg, dnsDomains, localRoutes, noSNAT) if err != nil { return err } @@ -403,6 +405,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, local DNS: wgIPToNetaddr(cfg.DNS), DNSDomains: dnsDomains, SubnetRoutes: wgCIDRToNetaddr(localRoutes), + NoSNAT: noSNAT, } for _, peer := range cfg.Peers { rs.Routes = append(rs.Routes, wgCIDRToNetaddr(peer.AllowedIPs)...) diff --git a/wgengine/watchdog.go b/wgengine/watchdog.go index 1eb210537..9a6f9a12b 100644 --- a/wgengine/watchdog.go +++ b/wgengine/watchdog.go @@ -61,8 +61,8 @@ func (e *watchdogEngine) watchdog(name string, fn func()) { }) } -func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg.CIDR) error { - return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, dnsDomains, localRoutes) }) +func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg.CIDR, noSNAT bool) error { + return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, dnsDomains, localRoutes, noSNAT) }) } func (e *watchdogEngine) GetFilter() *filter.Filter { var x *filter.Filter diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go index dc08ab751..196ecc95c 100644 --- a/wgengine/wgengine.go +++ b/wgengine/wgengine.go @@ -59,7 +59,7 @@ type Engine interface { // sends an updated network map. // // The returned error is ErrNoChanges if no changes were made. - Reconfig(cfg *wgcfg.Config, dnsDomains []string, localSubnets []wgcfg.CIDR) error + Reconfig(cfg *wgcfg.Config, dnsDomains []string, localSubnets []wgcfg.CIDR, noSNAT bool) error // GetFilter returns the current packet filter, if any. GetFilter() *filter.Filter