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 <nbrown.us@gmail.com>

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

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 {

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{},

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:
-punchy: true
+  # Continues to punch inbound/outbound at a regular interval to avoid expiration of firewall nat mappings
-# punch_back means that a node you are trying to reach will connect back out to you if your hole punching fails
+  punch: true
-# this is extremely useful if one node is behind a difficult nat, such as symmetric
+
-#punch_back: 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!

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++ {
+				time.Sleep(lh.punchDelay)
-					lh.punchConn.WriteTo(empty, vpnPeer)
+				lh.punchConn.WriteTo(empty, vpnPeer)
-					time.Sleep(time.Second * 1)
-				}
 
 			}()
 			l.Debugf("Punching %s on %d for %s", IntIp(a.Ip), a.Port, IntIp(n.Details.VpnIp))

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")

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)
+	punchy := NewPunchyFromConfig(config)
-	if punchy == true {
+	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

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
+}

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)
+}