From 1297090af37a06b9f0415b249291fdc167ceacd2 Mon Sep 17 00:00:00 2001 From: Ryan Huber Date: Fri, 27 Mar 2020 11:26:39 -0700 Subject: [PATCH] add configurable punching delay because of race-condition-y conntracks (#210) * add configurable punching delay because of race-condition-y conntracks * add changelog * fix tests * only do one punch per query * Coalesce punchy config * It is not is not set * Add tests Co-authored-by: Nate Brown --- CHANGELOG.md | 15 +++++++++++++ config.go | 4 ++++ connection_manager_test.go | 4 ++-- examples/config.yml | 16 +++++++++----- lighthouse.go | 10 ++++----- lighthouse_test.go | 4 ++-- main.go | 8 +++---- punchy.go | 30 ++++++++++++++++++++++++++ punchy_test.go | 43 ++++++++++++++++++++++++++++++++++++++ 9 files changed, 116 insertions(+), 18 deletions(-) create mode 100644 punchy.go create mode 100644 punchy_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dbd580..dab6360 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Added a delay to punching via lighthouse signal to deal with race conditions in some linux conntrack implementations. + + See deprecated, this also adds a new `punchy.delay` option that defaults to `1s` + + +### Deprecated + +- `punchy`, `punch_back` configuration options have been collapsed under the now top level `punchy` config directive. + + `punchy.punch` - This is the old `punchy` option. Should we perform NAT hole punching (default false)? + + `punchy.respond` - This is the old `punch_back` option, Should we respond to hole punching by hole punching back (default false)? + ## [1.1.0] - 2020-01-17 ### Added diff --git a/config.go b/config.go index 7cf8037..8c88f0c 100644 --- a/config.go +++ b/config.go @@ -217,6 +217,10 @@ func (c *Config) Get(k string) interface{} { return c.get(k, c.Settings) } +func (c *Config) IsSet(k string) bool { + return c.get(k, c.Settings) != nil +} + func (c *Config) get(k string, v interface{}) interface{} { parts := strings.Split(k, ".") for _, p := range parts { diff --git a/connection_manager_test.go b/connection_manager_test.go index e561b77..a59747d 100644 --- a/connection_manager_test.go +++ b/connection_manager_test.go @@ -28,7 +28,7 @@ func Test_NewConnectionManagerTest(t *testing.T) { rawCertificateNoKey: []byte{}, } - lh := NewLightHouse(false, 0, []uint32{}, 1000, 0, &udpConn{}, false) + lh := NewLightHouse(false, 0, []uint32{}, 1000, 0, &udpConn{}, false, 1) ifce := &Interface{ hostMap: hostMap, inside: &Tun{}, @@ -91,7 +91,7 @@ func Test_NewConnectionManagerTest2(t *testing.T) { rawCertificateNoKey: []byte{}, } - lh := NewLightHouse(false, 0, []uint32{}, 1000, 0, &udpConn{}, false) + lh := NewLightHouse(false, 0, []uint32{}, 1000, 0, &udpConn{}, false, 1) ifce := &Interface{ hostMap: hostMap, inside: &Tun{}, diff --git a/examples/config.yml b/examples/config.yml index a87c454..2a6aa0d 100644 --- a/examples/config.yml +++ b/examples/config.yml @@ -55,11 +55,17 @@ listen: #read_buffer: 10485760 #write_buffer: 10485760 -# Punchy continues to punch inbound/outbound at a regular interval to avoid expiration of firewall nat mappings -punchy: true -# punch_back means that a node you are trying to reach will connect back out to you if your hole punching fails -# this is extremely useful if one node is behind a difficult nat, such as symmetric -#punch_back: true +punchy: + # Continues to punch inbound/outbound at a regular interval to avoid expiration of firewall nat mappings + punch: true + + # respond means that a node you are trying to reach will connect back out to you if your hole punching fails + # this is extremely useful if one node is behind a difficult nat, such as a symmetric NAT + # Default is false + #respond: true + + # delays a punch response for misbehaving NATs, default is 1 second, respond must be true to take effect + #delay: 1s # Cipher allows you to choose between the available ciphers for your network. # IMPORTANT: this value must be identical on ALL NODES/LIGHTHOUSES. We do not/will not support use of different ciphers simultaneously! diff --git a/lighthouse.go b/lighthouse.go index 8aeaced..ad49196 100644 --- a/lighthouse.go +++ b/lighthouse.go @@ -26,6 +26,7 @@ type LightHouse struct { interval int nebulaPort int punchBack bool + punchDelay time.Duration } type EncWriter interface { @@ -33,7 +34,7 @@ type EncWriter interface { SendMessageToAll(t NebulaMessageType, st NebulaMessageSubType, vpnIp uint32, p, nb, out []byte) } -func NewLightHouse(amLighthouse bool, myIp uint32, ips []uint32, interval int, nebulaPort int, pc *udpConn, punchBack bool) *LightHouse { +func NewLightHouse(amLighthouse bool, myIp uint32, ips []uint32, interval int, nebulaPort int, pc *udpConn, punchBack bool, punchDelay time.Duration) *LightHouse { h := LightHouse{ amLighthouse: amLighthouse, myIp: myIp, @@ -44,6 +45,7 @@ func NewLightHouse(amLighthouse bool, myIp uint32, ips []uint32, interval int, n interval: interval, punchConn: pc, punchBack: punchBack, + punchDelay: punchDelay, } for _, ip := range ips { @@ -328,10 +330,8 @@ func (lh *LightHouse) HandleRequest(rAddr *udpAddr, vpnIp uint32, p []byte, c *c for _, a := range n.Details.IpAndPorts { vpnPeer := NewUDPAddr(a.Ip, uint16(a.Port)) go func() { - for i := 0; i < 5; i++ { - lh.punchConn.WriteTo(empty, vpnPeer) - time.Sleep(time.Second * 1) - } + time.Sleep(lh.punchDelay) + lh.punchConn.WriteTo(empty, vpnPeer) }() l.Debugf("Punching %s on %d for %s", IntIp(a.Ip), a.Port, IntIp(n.Details.VpnIp)) diff --git a/lighthouse_test.go b/lighthouse_test.go index 96d77bd..02b8ca7 100644 --- a/lighthouse_test.go +++ b/lighthouse_test.go @@ -52,7 +52,7 @@ func Test_lhStaticMapping(t *testing.T) { udpServer, _ := NewListener("0.0.0.0", 0, true) - meh := NewLightHouse(true, 1, []uint32{ip2int(lh1IP)}, 10, 10003, udpServer, false) + meh := NewLightHouse(true, 1, []uint32{ip2int(lh1IP)}, 10, 10003, udpServer, false, 1) meh.AddRemote(ip2int(lh1IP), NewUDPAddr(ip2int(lh1IP), uint16(4242)), true) err := meh.ValidateLHStaticEntries() assert.Nil(t, err) @@ -60,7 +60,7 @@ func Test_lhStaticMapping(t *testing.T) { lh2 := "10.128.0.3" lh2IP := net.ParseIP(lh2) - meh = NewLightHouse(true, 1, []uint32{ip2int(lh1IP), ip2int(lh2IP)}, 10, 10003, udpServer, false) + meh = NewLightHouse(true, 1, []uint32{ip2int(lh1IP), ip2int(lh2IP)}, 10, 10003, udpServer, false, 1) meh.AddRemote(ip2int(lh1IP), NewUDPAddr(ip2int(lh1IP), uint16(4242)), true) err = meh.ValidateLHStaticEntries() assert.EqualError(t, err, "Lighthouse 10.128.0.3 does not have a static_host_map entry") diff --git a/main.go b/main.go index 2d60492..266cca8 100644 --- a/main.go +++ b/main.go @@ -177,8 +177,8 @@ func Main(configPath string, configTest bool, buildVersion string) { go hostMap.Promoter(config.GetInt("promoter.interval")) */ - punchy := config.GetBool("punchy", false) - if punchy == true { + punchy := NewPunchyFromConfig(config) + if punchy.Punch { l.Info("UDP hole punching enabled") go hostMap.Punchy(udpServer) } @@ -193,7 +193,6 @@ func Main(configPath string, configTest bool, buildVersion string) { port = int(uPort.Port) } - punchBack := config.GetBool("punch_back", false) amLighthouse := config.GetBool("lighthouse.am_lighthouse", false) // warn if am_lighthouse is enabled but upstream lighthouses exists @@ -222,7 +221,8 @@ func Main(configPath string, configTest bool, buildVersion string) { config.GetInt("lighthouse.interval", 10), port, udpServer, - punchBack, + punchy.Respond, + punchy.Delay, ) //TODO: Move all of this inside functions in lighthouse.go diff --git a/punchy.go b/punchy.go new file mode 100644 index 0000000..153d0ac --- /dev/null +++ b/punchy.go @@ -0,0 +1,30 @@ +package nebula + +import "time" + +type Punchy struct { + Punch bool + Respond bool + Delay time.Duration +} + +func NewPunchyFromConfig(c *Config) *Punchy { + p := &Punchy{} + + if c.IsSet("punchy.punch") { + p.Punch = c.GetBool("punchy.punch", false) + } else { + // Deprecated fallback + p.Punch = c.GetBool("punchy", false) + } + + if c.IsSet("punchy.respond") { + p.Respond = c.GetBool("punchy.respond", false) + } else { + // Deprecated fallback + p.Respond = c.GetBool("punch_back", false) + } + + p.Delay = c.GetDuration("punchy.delay", time.Second) + return p +} diff --git a/punchy_test.go b/punchy_test.go new file mode 100644 index 0000000..98ed608 --- /dev/null +++ b/punchy_test.go @@ -0,0 +1,43 @@ +package nebula + +import ( + "github.com/stretchr/testify/assert" + "testing" + "time" +) + +func TestNewPunchyFromConfig(t *testing.T) { + c := NewConfig() + + // Test defaults + p := NewPunchyFromConfig(c) + assert.Equal(t, false, p.Punch) + assert.Equal(t, false, p.Respond) + assert.Equal(t, time.Second, p.Delay) + + // punchy deprecation + c.Settings["punchy"] = true + p = NewPunchyFromConfig(c) + assert.Equal(t, true, p.Punch) + + // punchy.punch + c.Settings["punchy"] = map[interface{}]interface{}{"punch": true} + p = NewPunchyFromConfig(c) + assert.Equal(t, true, p.Punch) + + // punch_back deprecation + c.Settings["punch_back"] = true + p = NewPunchyFromConfig(c) + assert.Equal(t, true, p.Respond) + + // punchy.respond + c.Settings["punchy"] = map[interface{}]interface{}{"respond": true} + c.Settings["punch_back"] = false + p = NewPunchyFromConfig(c) + assert.Equal(t, true, p.Respond) + + // punchy.delay + c.Settings["punchy"] = map[interface{}]interface{}{"delay": "1m"} + p = NewPunchyFromConfig(c) + assert.Equal(t, time.Minute, p.Delay) +}