diff --git a/handshake_manager.go b/handshake_manager.go index fa9ae154..2d04f0fc 100644 --- a/handshake_manager.go +++ b/handshake_manager.go @@ -807,7 +807,7 @@ func (hm *HandshakeManager) beginHandshake(via ViaSender, packet []byte, h *head } hm.sendHandshakeResponse(via, response, hostinfo, false) - hostinfo.remotes.RefreshFromHandshake(vpnAddrs) + hostinfo.remotes.RefreshFromHandshake(vpnAddrs, remoteCert.Certificate.Version()) // Don't wait for UpdateWorker if f.lightHouse.IsAnyLighthouseAddr(vpnAddrs) { @@ -995,7 +995,7 @@ func (hm *HandshakeManager) continueHandshake(via ViaSender, hh *HandshakeHostIn f.cachedPacketMetrics.sent.Inc(int64(len(hh.packetStore))) } - hostinfo.remotes.RefreshFromHandshake(vpnAddrs) + hostinfo.remotes.RefreshFromHandshake(vpnAddrs, remoteCert.Certificate.Version()) f.metricHandshakes.Update(duration) // Don't wait for UpdateWorker diff --git a/lighthouse.go b/lighthouse.go index 9cece233..54572ad7 100644 --- a/lighthouse.go +++ b/lighthouse.go @@ -513,17 +513,15 @@ func (lh *LightHouse) QueryServer(vpnAddr netip.Addr) { } func (lh *LightHouse) QueryCache(vpnAddrs []netip.Addr) *RemoteList { - lh.RLock() - if v, ok := lh.addrMap[vpnAddrs[0]]; ok { - lh.RUnlock() - return v + rl, ok := lh.findRemoteList(vpnAddrs) + if ok { + return rl } - lh.RUnlock() lh.Lock() defer lh.Unlock() // Add an entry if we don't already have one - return lh.unlockedGetRemoteList(vpnAddrs) //todo CERT-V2 this contains addrmap lookups we could potentially skip + return lh.unlockedGetRemoteList(vpnAddrs) //todo this re-calls unlockedFindRemoteList } // queryAndPrepMessage is a lock helper on RemoteList, assisting the caller to build a lighthouse message containing @@ -669,23 +667,58 @@ func (lh *LightHouse) addCalculatedRemotes(vpnAddr netip.Addr) bool { return len(calculatedV4) > 0 || len(calculatedV6) > 0 } +func (lh *LightHouse) findRemoteList(vpnAddrs []netip.Addr) (*RemoteList, bool) { + lh.RLock() + defer lh.RUnlock() + return lh.unlockedFindRemoteList(vpnAddrs) +} + +// unlockedFindRemoteList checks addrMap for each of vpnAddrs. It returns the first RemoteList found, +// and true if that RemoteList is present for all vpnAddrs. +// If false, it means the addrMap, and possibly the RemoteList, need to be corrected. +func (lh *LightHouse) unlockedFindRemoteList(vpnAddrs []netip.Addr) (*RemoteList, bool) { + var am *RemoteList + //todo: if a host with addresses A and B is "split", so it only has address A and a new host has only address B + //todo: we don't handly that correctly, I'm pretty sure. + missingOrDifferent := false + for _, addr := range vpnAddrs { + found, ok := lh.addrMap[addr] + if !ok { + missingOrDifferent = true + } else if am == nil { + am = found //the first list we find wins + } else if am != found { + missingOrDifferent = true + } + } + return am, !missingOrDifferent +} + // unlockedGetRemoteList assumes you have the lh lock func (lh *LightHouse) unlockedGetRemoteList(allAddrs []netip.Addr) *RemoteList { // 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 + am, ok := lh.unlockedFindRemoteList(allAddrs) + + // we failed to find any RemoteLists: make a new one, fill out the addrMap + if am == nil { + am = NewRemoteList(allAddrs, lh.shouldAdd) + for _, addr := range allAddrs { + lh.addrMap[addr] = am + } + return am + } + + // we found one! Do we need to fix it? + if !ok { + am.Lock() + am.vpnAddrs = make([]netip.Addr, len(allAddrs)) + copy(am.vpnAddrs, allAddrs) + am.Unlock() + for _, addr := range allAddrs { + lh.addrMap[addr] = am } } - am := NewRemoteList(allAddrs, lh.shouldAdd) - for _, addr := range allAddrs { - lh.addrMap[addr] = am - } return am } diff --git a/lighthouse_test.go b/lighthouse_test.go index 7a81e5d2..29d33345 100644 --- a/lighthouse_test.go +++ b/lighthouse_test.go @@ -738,3 +738,123 @@ func TestLighthouse_DeletesWork(t *testing.T) { out = lh.Query(testHost) assert.Nil(t, out) } + +// newLHHostUpdateV2 sends a v2-style HostUpdateNotification where the sending tunnel carries +// multiple vpn addrs (a dual-stack v2 cert). Details.VpnAddr is left blank like SendUpdate does. +func newLHHostUpdateV2(fromAddr netip.AddrPort, vpnAddrs []netip.Addr, addrs []netip.AddrPort, lhh *LightHouseHandler) { + req := &NebulaMeta{ + Type: NebulaMeta_HostUpdateNotification, + Details: &NebulaMetaDetails{}, + } + for _, v := range addrs { + if v.Addr().Is4() { + req.Details.V4AddrPorts = append(req.Details.V4AddrPorts, netAddrToProtoV4AddrPort(v.Addr(), v.Port())) + } else { + req.Details.V6AddrPorts = append(req.Details.V6AddrPorts, netAddrToProtoV6AddrPort(v.Addr(), v.Port())) + } + } + b, err := req.Marshal() + if err != nil { + panic(err) + } + lhh.HandleRequest(fromAddr, vpnAddrs, b, &testEncWriter{}) +} + +func newIssue1868Lighthouse(t *testing.T) (*LightHouse, *LightHouseHandler) { + l := test.NewLogger() + c := config.NewC(l) + c.Settings["lighthouse"] = map[string]any{"am_lighthouse": true} + c.Settings["listen"] = map[string]any{"port": 4242} + + myVpnNet4 := netip.MustParsePrefix("10.128.0.1/24") + myVpnNet6 := netip.MustParsePrefix("fd00::1/64") + nt := new(bart.Lite) + nt.Insert(myVpnNet4) + nt.Insert(myVpnNet6) + cs := &CertState{ + myVpnNetworks: []netip.Prefix{myVpnNet4, myVpnNet6}, + myVpnNetworksTable: nt, + } + lh, err := NewLightHouseFromConfig(t.Context(), l, c, cs, nil, nil) + require.NoError(t, err) + lh.ifce = &mockEncWriter{} + return lh, lh.NewRequestHandler() +} + +// Scenario A: host registers via v2 (both addrs), then a v1 handshake with the same host completes on +// the lighthouse (rehandshake after cert renewal, relay-initiated handshake, traffic to the LH's v4 +// addr...). handshake_manager does QueryCache(vpnAddrs) + RefreshFromHandshake(vpnAddrs) with the v1 +// cert's single address, which truncates RemoteList.vpnAddrs. +func TestLighthouse_Issue1868_V1HandshakeTruncatesVpnAddrs(t *testing.T) { + lh, lhh := newIssue1868Lighthouse(t) + + hostV4 := netip.MustParseAddr("10.128.0.3") + hostV6 := netip.MustParseAddr("fd00::3") + hostUdp := netip.MustParseAddrPort("192.0.2.3:4242") + hostLan := netip.MustParseAddrPort("10.0.0.3:4242") + + askerV4 := netip.MustParseAddr("10.128.0.2") + askerUdp := netip.MustParseAddrPort("192.0.2.2:4242") + + // Boot: host handshakes with the LH using its v2 cert and sends an update + newLHHostUpdateV2(hostUdp, []netip.Addr{hostV4, hostV6}, []netip.AddrPort{hostLan}, lhh) + + // Both addresses resolve + r := newLHHostRequest(askerUdp, askerV4, hostV4, lhh) + require.NotNil(t, r.msg, "v4 query should be answered") + assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan) + + r = newLHHostRequest(askerUdp, askerV4, hostV6, lhh) + require.NotNil(t, r.msg, "v6 query should be answered before the v1 handshake") + assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan) + + // Later: a v1 handshake with the same host completes on the LH. This is exactly what + // handshake_manager.go does on completion, with the v1 cert's single vpn addr. + rl := lh.QueryCache([]netip.Addr{hostV4}) + rl.RefreshFromHandshake([]netip.Addr{hostV4}, cert.Version1) + + // The host keeps sending v2 updates over its v2 tunnel too + newLHHostUpdateV2(hostUdp, []netip.Addr{hostV4, hostV6}, []netip.AddrPort{hostLan}, lhh) + + r = newLHHostRequest(askerUdp, askerV4, hostV4, lhh) + require.NotNil(t, r.msg, "v4 query should still be answered") + assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan) + + r = newLHHostRequest(askerUdp, askerV4, hostV6, lhh) + if assert.NotNil(t, r.msg, "BUG: v6 query is silently dropped after a v1 handshake truncated RemoteList.vpnAddrs") { + assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan) + } +} + +// Scenario B: the LH first creates a RemoteList for the host keyed only by its v4 addr (a pending +// LH-initiated v1 handshake does QueryCache([v4]) in handleOutbound), then the host arrives with v2. +// unlockedGetRemoteList/QueryCache hit on allAddrs[0] and never add the v6 key to addrMap. +func TestLighthouse_Issue1868_V4OnlyListNeverGainsV6Key(t *testing.T) { + lh, lhh := newIssue1868Lighthouse(t) + + hostV4 := netip.MustParseAddr("10.128.0.3") + hostV6 := netip.MustParseAddr("fd00::3") + hostUdp := netip.MustParseAddrPort("192.0.2.3:4242") + hostLan := netip.MustParseAddrPort("10.0.0.3:4242") + + askerV4 := netip.MustParseAddr("10.128.0.2") + askerUdp := netip.MustParseAddrPort("192.0.2.2:4242") + + // LH is a relay and someone asked it to relay to hostV4 while the host was offline: + // StartHandshake(hostV4) -> handleOutbound -> QueryCache([hostV4]) creates a v4-only list. + _ = lh.QueryCache([]netip.Addr{hostV4}) + + // Host boots and handshakes v2 with the LH (responder path does QueryCache + RefreshFromHandshake) + rl := lh.QueryCache([]netip.Addr{hostV4, hostV6}) + rl.RefreshFromHandshake([]netip.Addr{hostV4, hostV6}, cert.Version2) + newLHHostUpdateV2(hostUdp, []netip.Addr{hostV4, hostV6}, []netip.AddrPort{hostLan}, lhh) + + r := newLHHostRequest(askerUdp, askerV4, hostV4, lhh) + require.NotNil(t, r.msg, "v4 query should be answered") + assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan) + + r = newLHHostRequest(askerUdp, askerV4, hostV6, lhh) + if assert.NotNil(t, r.msg, "BUG: v6 query is silently dropped, addrMap never got the v6 key") { + assertIp4InArray(t, r.msg.Details.V4AddrPorts, hostLan) + } +} diff --git a/remote_list.go b/remote_list.go index 9d1b387e..835febc4 100644 --- a/remote_list.go +++ b/remote_list.go @@ -11,6 +11,8 @@ import ( "sync" "sync/atomic" "time" + + "github.com/slackhq/nebula/cert" ) // forEachFunc is used to benefit folks that want to do work inside the lock @@ -408,11 +410,15 @@ func (r *RemoteList) CopyBlockedRemotes() []netip.AddrPort { } // RefreshFromHandshake locks and updates the RemoteList to account for data learned upon a completed handshake -func (r *RemoteList) RefreshFromHandshake(vpnAddrs []netip.Addr) { +func (r *RemoteList) RefreshFromHandshake(vpnAddrs []netip.Addr, v cert.Version) { r.Lock() r.badRemotes = nil - r.vpnAddrs = make([]netip.Addr, len(vpnAddrs)) - copy(r.vpnAddrs, vpnAddrs) + if v != cert.Version1 { + // a handshake from a v1 cert can never expand our knowledge of the number of addresses a host has, + // and, because v2 certs exist, it can also never contract it. So, only update this for non-v1 certs. + r.vpnAddrs = make([]netip.Addr, len(vpnAddrs)) + copy(r.vpnAddrs, vpnAddrs) + } r.Unlock() }