From 932e329164736fc63b7fb884dd17dff4113987e0 Mon Sep 17 00:00:00 2001 From: Jack Doan Date: Thu, 4 Sep 2025 14:49:40 -0500 Subject: [PATCH] Don't delete static host mappings for non-primary IPs (#1464) * Don't delete a vpnaddr if it's part of a certificate that contains a vpnaddr that's in the static host map * remove unused arg from ConnectionManager.shouldSwapPrimary() --- connection_manager.go | 4 +- lighthouse.go | 34 ++++++++---- lighthouse_test.go | 120 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 13 deletions(-) diff --git a/connection_manager.go b/connection_manager.go index 1f9b18b..7242c72 100644 --- a/connection_manager.go +++ b/connection_manager.go @@ -356,7 +356,7 @@ func (cm *connectionManager) makeTrafficDecision(localIndex uint32, now time.Tim decision = tryRehandshake } else { - if cm.shouldSwapPrimary(hostinfo, primary) { + if cm.shouldSwapPrimary(hostinfo) { decision = swapPrimary } else { // migrate the relays to the primary, if in use. @@ -447,7 +447,7 @@ func (cm *connectionManager) isInactive(hostinfo *HostInfo, now time.Time) (time return inactiveDuration, true } -func (cm *connectionManager) shouldSwapPrimary(current, primary *HostInfo) bool { +func (cm *connectionManager) shouldSwapPrimary(current *HostInfo) bool { // The primary tunnel is the most recent handshake to complete locally and should work entirely fine. // If we are here then we have multiple tunnels for a host pair and neither side believes the same tunnel is primary. // Let's sort this out. diff --git a/lighthouse.go b/lighthouse.go index 7a679c7..b047689 100644 --- a/lighthouse.go +++ b/lighthouse.go @@ -519,11 +519,15 @@ func (lh *LightHouse) queryAndPrepMessage(vpnAddr netip.Addr, f func(*cache) (in } func (lh *LightHouse) DeleteVpnAddrs(allVpnAddrs []netip.Addr) { - // First we check the static mapping - // and do nothing if it is there - if _, ok := lh.GetStaticHostList()[allVpnAddrs[0]]; ok { - return + // First we check the static host map. If any of the VpnAddrs to be deleted are present, do nothing. + staticList := lh.GetStaticHostList() + for _, addr := range allVpnAddrs { + if _, ok := staticList[addr]; ok { + return + } } + + // None of the VpnAddrs were present. Now we can do the deletes. lh.Lock() rm, ok := lh.addrMap[allVpnAddrs[0]] if ok { @@ -627,16 +631,24 @@ func (lh *LightHouse) addCalculatedRemotes(vpnAddr netip.Addr) bool { return len(calculatedV4) > 0 || len(calculatedV6) > 0 } -// unlockedGetRemoteList -// assumes you have the lh lock +// unlockedGetRemoteList assumes you have the lh lock func (lh *LightHouse) unlockedGetRemoteList(allAddrs []netip.Addr) *RemoteList { - am, ok := lh.addrMap[allAddrs[0]] - if !ok { - am = NewRemoteList(allAddrs, func(a netip.Addr) bool { return lh.shouldAdd(allAddrs[0], a) }) - for _, addr := range allAddrs { - lh.addrMap[addr] = am + // before we go and make a new remotelist, we need to make sure we don't have one for any of this set of vpnaddrs yet + for i, addr := range allAddrs { + am, ok := lh.addrMap[addr] + if ok { + if i != 0 { + lh.addrMap[allAddrs[0]] = am + } + return am } } + + //TODO lighthouse.remote_allow_ranges is almost certainly broken in a multiple-address-per-cert scenario + am := NewRemoteList(allAddrs, func(a netip.Addr) bool { return lh.shouldAdd(allAddrs[0], a) }) + for _, addr := range allAddrs { + lh.addrMap[addr] = am + } return am } diff --git a/lighthouse_test.go b/lighthouse_test.go index eb2d26e..d8a1188 100644 --- a/lighthouse_test.go +++ b/lighthouse_test.go @@ -493,3 +493,123 @@ func Test_findNetworkUnion(t *testing.T) { out, ok = findNetworkUnion([]netip.Prefix{fc00}, []netip.Addr{a1, afe81}) assert.False(t, ok) } + +func TestLighthouse_Dont_Delete_Static_Hosts(t *testing.T) { + l := test.NewLogger() + + myUdpAddr2 := netip.MustParseAddrPort("1.2.3.4:4242") + + testSameHostNotStatic := netip.MustParseAddr("10.128.0.41") + testStaticHost := netip.MustParseAddr("10.128.0.42") + //myVpnIp := netip.MustParseAddr("10.128.0.2") + + c := config.NewC(l) + lh1 := "10.128.0.2" + c.Settings["lighthouse"] = map[string]any{ + "hosts": []any{lh1}, + "interval": "1s", + } + + c.Settings["listen"] = map[string]any{"port": 4242} + c.Settings["static_host_map"] = map[string]any{ + lh1: []any{"1.1.1.1:4242"}, + "10.128.0.42": []any{"1.2.3.4:4242"}, + } + + myVpnNet := netip.MustParsePrefix("10.128.0.1/24") + nt := new(bart.Lite) + nt.Insert(myVpnNet) + cs := &CertState{ + myVpnNetworks: []netip.Prefix{myVpnNet}, + myVpnNetworksTable: nt, + } + lh, err := NewLightHouseFromConfig(context.Background(), l, c, cs, nil, nil) + require.NoError(t, err) + lh.ifce = &mockEncWriter{} + + //test that we actually have the static entry: + out := lh.Query(testStaticHost) + assert.NotNil(t, out) + assert.Equal(t, out.vpnAddrs[0], testStaticHost) + out.Rebuild([]netip.Prefix{}) //why tho + assert.Equal(t, out.addrs[0], myUdpAddr2) + + //bolt on a lower numbered primary IP + am := lh.unlockedGetRemoteList([]netip.Addr{testStaticHost}) + am.vpnAddrs = []netip.Addr{testSameHostNotStatic, testStaticHost} + lh.addrMap[testSameHostNotStatic] = am + out.Rebuild([]netip.Prefix{}) //??? + + //test that we actually have the static entry: + out = lh.Query(testStaticHost) + assert.NotNil(t, out) + assert.Equal(t, out.vpnAddrs[0], testSameHostNotStatic) + assert.Equal(t, out.vpnAddrs[1], testStaticHost) + assert.Equal(t, out.addrs[0], myUdpAddr2) + + //test that we actually have the static entry for BOTH: + out2 := lh.Query(testSameHostNotStatic) + assert.Same(t, out2, out) + + //now do the delete + lh.DeleteVpnAddrs([]netip.Addr{testSameHostNotStatic, testStaticHost}) + //verify + out = lh.Query(testSameHostNotStatic) + assert.NotNil(t, out) + if out == nil { + t.Fatal("expected non-nil query for the static host") + } + assert.Equal(t, out.vpnAddrs[0], testSameHostNotStatic) + assert.Equal(t, out.vpnAddrs[1], testStaticHost) + assert.Equal(t, out.addrs[0], myUdpAddr2) +} + +func TestLighthouse_DeletesWork(t *testing.T) { + l := test.NewLogger() + + myUdpAddr2 := netip.MustParseAddrPort("1.2.3.4:4242") + testHost := netip.MustParseAddr("10.128.0.42") + + c := config.NewC(l) + lh1 := "10.128.0.2" + c.Settings["lighthouse"] = map[string]any{ + "hosts": []any{lh1}, + "interval": "1s", + } + + c.Settings["listen"] = map[string]any{"port": 4242} + c.Settings["static_host_map"] = map[string]any{ + lh1: []any{"1.1.1.1:4242"}, + } + + myVpnNet := netip.MustParsePrefix("10.128.0.1/24") + nt := new(bart.Lite) + nt.Insert(myVpnNet) + cs := &CertState{ + myVpnNetworks: []netip.Prefix{myVpnNet}, + myVpnNetworksTable: nt, + } + lh, err := NewLightHouseFromConfig(context.Background(), l, c, cs, nil, nil) + require.NoError(t, err) + lh.ifce = &mockEncWriter{} + + //insert the host + am := lh.unlockedGetRemoteList([]netip.Addr{testHost}) + am.vpnAddrs = []netip.Addr{testHost} + am.addrs = []netip.AddrPort{myUdpAddr2} + lh.addrMap[testHost] = am + am.Rebuild([]netip.Prefix{}) //??? + + //test that we actually have the entry: + out := lh.Query(testHost) + assert.NotNil(t, out) + assert.Equal(t, out.vpnAddrs[0], testHost) + out.Rebuild([]netip.Prefix{}) //why tho + assert.Equal(t, out.addrs[0], myUdpAddr2) + + //now do the delete + lh.DeleteVpnAddrs([]netip.Addr{testHost}) + //verify + out = lh.Query(testHost) + assert.Nil(t, out) +}