From 69cdc30c6d41aab287e6d262827a8f92eafb2ee7 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Fri, 2 Apr 2021 18:04:39 -0700 Subject: [PATCH] wgengine/wgcfg: remove Config.ListenPort We don't use the port that wireguard-go passes to us (via magicsock.connBind.Open). We ignore it entirely and use the port we selected. When we tell wireguard-go that we're changing the listen_port, it calls connBind.Close and then connBind.Open. And in the meantime, it stops calling the receive functions, which means that we stop receiving and processing UDP and DERP packets. And that is Very Bad. That was never a problem prior to b3ceca1dd7d7a1a6f9ddab136a4e12900e976333, because we passed the SkipBindUpdate flag to our wireguard-go fork, which told wireguard-go not to re-bind on listen_port changes. That commit eliminated the SkipBindUpdate flag. We could write a bunch of code to work around the gap. We could add background readers that process UDP and DERP packets when wireguard-go isn't. But it's simpler to never create the conditions in which wireguard-go rebinds. The other scenario in which wireguard-go re-binds is device.Down. Conveniently, we never call device.Down. We go from device.Up to device.Close, and the latter only when we're shutting down a magicsock.Conn completely. Rubber-ducked-by: Avery Pennarun Signed-off-by: Josh Bleecher Snyder --- internal/deepprint/deepprint_test.go | 5 ++--- wgengine/magicsock/magicsock_test.go | 3 +-- wgengine/wgcfg/config.go | 1 - wgengine/wgcfg/nmcfg/nmcfg.go | 1 - wgengine/wgcfg/parser.go | 6 +----- wgengine/wgcfg/writer.go | 3 --- 6 files changed, 4 insertions(+), 15 deletions(-) diff --git a/internal/deepprint/deepprint_test.go b/internal/deepprint/deepprint_test.go index 33847e27f..01ecb43ad 100644 --- a/internal/deepprint/deepprint_test.go +++ b/internal/deepprint/deepprint_test.go @@ -36,9 +36,8 @@ func TestDeepPrint(t *testing.T) { func getVal() []interface{} { return []interface{}{ &wgcfg.Config{ - Name: "foo", - Addresses: []netaddr.IPPrefix{{Bits: 5, IP: netaddr.IPFrom16([16]byte{3: 3})}}, - ListenPort: 5, + Name: "foo", + Addresses: []netaddr.IPPrefix{{Bits: 5, IP: netaddr.IPFrom16([16]byte{3: 3})}}, Peers: []wgcfg.Peer{ { Endpoints: "foo:5", diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 8da97779b..33a571029 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -460,12 +460,11 @@ func makeConfigs(t *testing.T, addrs []netaddr.IPPort) []wgcfg.Config { } var cfgs []wgcfg.Config - for i, addr := range addrs { + for i := range addrs { cfg := wgcfg.Config{ Name: fmt.Sprintf("peer%d", i+1), PrivateKey: privKeys[i], Addresses: addresses[i], - ListenPort: addr.Port, } for peerNum, addr := range addrs { if peerNum == i { diff --git a/wgengine/wgcfg/config.go b/wgengine/wgcfg/config.go index 2928e47d2..08328bcea 100644 --- a/wgengine/wgcfg/config.go +++ b/wgengine/wgcfg/config.go @@ -20,7 +20,6 @@ type Config struct { Name string PrivateKey PrivateKey Addresses []netaddr.IPPrefix - ListenPort uint16 MTU uint16 DNS []netaddr.IP Peers []Peer diff --git a/wgengine/wgcfg/nmcfg/nmcfg.go b/wgengine/wgcfg/nmcfg/nmcfg.go index fb75fb923..d481066fb 100644 --- a/wgengine/wgcfg/nmcfg/nmcfg.go +++ b/wgengine/wgcfg/nmcfg/nmcfg.go @@ -57,7 +57,6 @@ func WGCfg(nm *netmap.NetworkMap, logf logger.Logf, flags netmap.WGConfigFlags, Name: "tailscale", PrivateKey: wgcfg.PrivateKey(nm.PrivateKey), Addresses: nm.Addresses, - ListenPort: nm.LocalPort, Peers: make([]wgcfg.Peer, 0, len(nm.Peers)), } diff --git a/wgengine/wgcfg/parser.go b/wgengine/wgcfg/parser.go index bf0b45835..ec66eb6bc 100644 --- a/wgengine/wgcfg/parser.go +++ b/wgengine/wgcfg/parser.go @@ -144,11 +144,7 @@ func (cfg *Config) handleDeviceLine(key, value string) error { // wireguard-go guarantees not to send zero value; private keys are already clamped. cfg.PrivateKey = PrivateKey(*k) case "listen_port": - port, err := strconv.ParseUint(value, 10, 16) - if err != nil { - return fmt.Errorf("failed to parse listen_port: %w", err) - } - cfg.ListenPort = uint16(port) + // ignore case "fwmark": // ignore default: diff --git a/wgengine/wgcfg/writer.go b/wgengine/wgcfg/writer.go index 079c1eb5e..f64b71bf3 100644 --- a/wgengine/wgcfg/writer.go +++ b/wgengine/wgcfg/writer.go @@ -40,9 +40,6 @@ func (cfg *Config) ToUAPI(w io.Writer, prev *Config) error { if prev.PrivateKey != cfg.PrivateKey { set("private_key", cfg.PrivateKey.HexString()) } - if prev.ListenPort != cfg.ListenPort { - setUint16("listen_port", cfg.ListenPort) - } old := make(map[Key]Peer) for _, p := range prev.Peers {