Make sure all vpnAddrs are hoisted to primary, resolve a few more TODOs (#1319)

cert/cert.go

@@ -149,7 +149,7 @@ func unmarshalCertificateFromHandshake(v Version, b []byte, publicKey []byte, cu
 	case Version2:
 		c, err = unmarshalCertificateV2(b, publicKey, curve)
 	default:
-		//TODO: make a static var
+		//TODO: CERT-V2 make a static var
 		return nil, fmt.Errorf("unknown certificate version %d", v)
 	}
 

cmd/nebula-cert/sign.go

@@ -172,7 +172,6 @@ func signCert(args []string, out io.Writer, errOut io.Writer, pr PasswordReader)
 
 	if *sf.networks != "" {
 		for _, rs := range strings.Split(*sf.networks, ",") {
-			//TODO: error on duplicates? Mainly only addr matters, having two of the same addr in the same or different prefix space is strange
 			rs := strings.Trim(rs, " ")
 			if rs != "" {
 				n, err := netip.ParsePrefix(rs)
@@ -197,7 +196,6 @@ func signCert(args []string, out io.Writer, errOut io.Writer, pr PasswordReader)
 	}
 
 	if *sf.unsafeNetworks != "" {
-		//TODO: error on duplicates?
 		for _, rs := range strings.Split(*sf.unsafeNetworks, ",") {
 			rs := strings.Trim(rs, " ")
 			if rs != "" {

control.go

@@ -234,7 +234,7 @@ func (c *Control) CloseAllTunnels(excludeLighthouses bool) (closed int) {
 		c.f.send(header.CloseTunnel, 0, h.ConnectionState, h, []byte{}, make([]byte, 12, 12), make([]byte, mtu))
 		c.f.closeTunnel(h)
 
-		c.l.WithField("vpnIp", h.vpnAddrs[0]).WithField("udpAddr", h.remote).
+		c.l.WithField("vpnAddrs", h.vpnAddrs).WithField("udpAddr", h.remote).
 			Debug("Sending close tunnel message")
 		closed++
 	}

control_test.go

@@ -13,7 +13,7 @@ import (
 )
 
 func TestControl_GetHostInfoByVpnIp(t *testing.T) {
-	//TODO: with multiple certificate versions we have a problem with this test
+	//TODO: CERT-V2 with multiple certificate versions we have a problem with this test
 	// Some certs versions have different characteristics and each version implements their own Copy() func
 	// which means this is not a good place to test for exposing memory
 	l := test.NewLogger()

e2e/helpers_test.go

@@ -161,7 +161,7 @@ func assertTunnel(t *testing.T, vpnIpA, vpnIpB netip.Addr, controlA, controlB *n
 
 func assertHostInfoPair(t *testing.T, addrA, addrB netip.AddrPort, vpnNetsA, vpnNetsB []netip.Prefix, controlA, controlB *nebula.Control) {
 	// Get both host infos
-	//TODO: we may want to loop over each vpnAddr and assert all the things
+	//TODO: CERT-V2 we may want to loop over each vpnAddr and assert all the things
 	hBinA := controlA.GetHostInfoByVpnAddr(vpnNetsB[0].Addr(), false)
 	assert.NotNil(t, hBinA, "Host B was not found by vpnAddr in controlA")
 

hostmap.go

@@ -353,27 +353,37 @@ func (hm *HostMap) MakePrimary(hostinfo *HostInfo) {
 }
 
 func (hm *HostMap) unlockedMakePrimary(hostinfo *HostInfo) {
-	//TODO: we may need to promote follow on hostinfos from these vpnAddrs as well since their oldHostinfo might not be the same as this one
-	// this really looks like an ideal spot for memory leaks
+	// Get the current primary, if it exists
 	oldHostinfo := hm.Hosts[hostinfo.vpnAddrs[0]]
+
+	// Every address in the hostinfo gets elevated to primary
+	for _, vpnAddr := range hostinfo.vpnAddrs {
+		//NOTE: It is possible that we leave a dangling hostinfo here but connection manager works on
+		// indexes so it should be fine.
+		hm.Hosts[vpnAddr] = hostinfo
+	}
+
+	// If we are already primary then we won't bother re-linking
 	if oldHostinfo == hostinfo {
 		return
 	}
 
+	// Unlink this hostinfo
 	if hostinfo.prev != nil {
 		hostinfo.prev.next = hostinfo.next
 	}
-
 	if hostinfo.next != nil {
 		hostinfo.next.prev = hostinfo.prev
 	}
 
-	hm.Hosts[hostinfo.vpnAddrs[0]] = hostinfo
-
+	// If there wasn't a previous primary then clear out any links
 	if oldHostinfo == nil {
+		hostinfo.next = nil
+		hostinfo.prev = nil
 		return
 	}
 
+	// Relink the hostinfo as primary
 	hostinfo.next = oldHostinfo
 	oldHostinfo.prev = hostinfo
 	hostinfo.prev = nil

iputil/packet.go

@@ -6,8 +6,6 @@ import (
 	"golang.org/x/net/ipv4"
 )
 
-//TODO: IPV6-WORK can probably delete this
-
 const (
 	// Need 96 bytes for the largest reject packet:
 	// - 20 byte ipv4 header

lighthouse.go

@@ -710,7 +710,7 @@ func (lh *LightHouse) IsLighthouseAddr(vpnAddr netip.Addr) bool {
 	return false
 }
 
-// TODO: IsLighthouseAddr should be sufficient, we just need to update the vpnAddrs for lighthouses after a handshake
+// TODO: CERT-V2 IsLighthouseAddr should be sufficient, we just need to update the vpnAddrs for lighthouses after a handshake
 // so that we know all the lighthouse vpnAddrs, not just the ones we were configured to talk to initially
 func (lh *LightHouse) IsAnyLighthouseAddr(vpnAddr []netip.Addr) bool {
 	l := lh.GetLighthouses()
@@ -1123,7 +1123,7 @@ func (lhh *LightHouseHandler) sendHostPunchNotification(n *NebulaMeta, fromVpnAd
 			if ok {
 				whereToPunch = newDest
 			} else {
-				//TODO this means the destination will have no addresses in common with the punch-ee
+				//TODO: CERT-V2 this means the destination will have no addresses in common with the punch-ee
 				//choosing to do nothing for now, but maybe we return an error?
 			}
 		}
@@ -1194,6 +1194,7 @@ func (lhh *LightHouseHandler) coalesceAnswers(v cert.Version, c *cache, n *Nebul
 			}
 
 		} else {
+			//TODO: CERT-V2 don't panic
 			panic("unsupported version")
 		}
 	}
@@ -1257,8 +1258,8 @@ func (lhh *LightHouseHandler) handleHostUpdateNotification(n *NebulaMeta, fromVp
 		return
 	}
 
-	//todo hosts with only v2 certs cannot provide their ipv6 addr when contacting the lighthouse via v4?
-	//todo why do we care about the vpnAddr in the packet? We know where it came from, right?
+	//TODO: CERT-V2 hosts with only v2 certs cannot provide their ipv6 addr when contacting the lighthouse via v4?
+	//TODO: CERT-V2 why do we care about the vpnAddr in the packet? We know where it came from, right?
 	//Simple check that the host sent this not someone else
 	if !slices.Contains(fromVpnAddrs, detailsVpnAddr) {
 		if lhh.l.Level >= logrus.DebugLevel {

lighthouse_test.go

@@ -306,13 +306,16 @@ func TestLighthouse_reload(t *testing.T) {
 }
 
 func newLHHostRequest(fromAddr netip.AddrPort, myVpnIp, queryVpnIp netip.Addr, lhh *LightHouseHandler) testLhReply {
-	//TODO: IPV6-WORK
-	bip := queryVpnIp.As4()
 	req := &NebulaMeta{
-		Type: NebulaMeta_HostQuery,
-		Details: &NebulaMetaDetails{
-			OldVpnAddr: binary.BigEndian.Uint32(bip[:]),
-		},
+		Type:    NebulaMeta_HostQuery,
+		Details: &NebulaMetaDetails{},
+	}
+
+	if queryVpnIp.Is4() {
+		bip := queryVpnIp.As4()
+		req.Details.OldVpnAddr = binary.BigEndian.Uint32(bip[:])
+	} else {
+		req.Details.VpnAddr = netAddrToProtoAddr(queryVpnIp)
 	}
 
 	b, err := req.Marshal()
@@ -329,18 +332,24 @@ func newLHHostRequest(fromAddr netip.AddrPort, myVpnIp, queryVpnIp netip.Addr, l
 }
 
 func newLHHostUpdate(fromAddr netip.AddrPort, vpnIp netip.Addr, addrs []netip.AddrPort, lhh *LightHouseHandler) {
-	//TODO: IPV6-WORK
-	bip := vpnIp.As4()
 	req := &NebulaMeta{
-		Type: NebulaMeta_HostUpdateNotification,
-		Details: &NebulaMetaDetails{
-			OldVpnAddr:  binary.BigEndian.Uint32(bip[:]),
-			V4AddrPorts: make([]*V4AddrPort, len(addrs)),
-		},
+		Type:    NebulaMeta_HostUpdateNotification,
+		Details: &NebulaMetaDetails{},
 	}
 
-	for k, v := range addrs {
-		req.Details.V4AddrPorts[k] = netAddrToProtoV4AddrPort(v.Addr(), v.Port())
+	if vpnIp.Is4() {
+		bip := vpnIp.As4()
+		req.Details.OldVpnAddr = binary.BigEndian.Uint32(bip[:])
+	} else {
+		req.Details.VpnAddr = netAddrToProtoAddr(vpnIp)
+	}
+
+	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()
@@ -352,72 +361,6 @@ func newLHHostUpdate(fromAddr netip.AddrPort, vpnIp netip.Addr, addrs []netip.Ad
 	lhh.HandleRequest(fromAddr, []netip.Addr{vpnIp}, b, w)
 }
 
-//TODO: this is a RemoteList test
-//func Test_lhRemoteAllowList(t *testing.T) {
-//	l := NewLogger()
-//	c := NewConfig(l)
-//	c.Settings["remoteallowlist"] = map[interface{}]interface{}{
-//		"10.20.0.0/12": false,
-//	}
-//	allowList, err := c.GetAllowList("remoteallowlist", false)
-//	assert.Nil(t, err)
-//
-//	lh1 := "10.128.0.2"
-//	lh1IP := net.ParseIP(lh1)
-//
-//	udpServer, _ := NewListener(l, "0.0.0.0", 0, true)
-//
-//	lh := NewLightHouse(l, true, &net.IPNet{IP: net.IP{0, 0, 0, 1}, Mask: net.IPMask{255, 255, 255, 0}}, []uint32{ip2int(lh1IP)}, 10, 10003, udpServer, false, 1, false)
-//	lh.SetRemoteAllowList(allowList)
-//
-//	// A disallowed ip should not enter the cache but we should end up with an empty entry in the addrMap
-//	remote1IP := net.ParseIP("10.20.0.3")
-//	remotes := lh.unlockedGetRemoteList(ip2int(remote1IP))
-//	remotes.unlockedPrependV4(ip2int(remote1IP), NewIp4AndPort(remote1IP, 4242))
-//	assert.NotNil(t, lh.addrMap[ip2int(remote1IP)])
-//	assert.Empty(t, lh.addrMap[ip2int(remote1IP)].CopyAddrs([]*net.IPNet{}))
-//
-//	// Make sure a good ip enters the cache and addrMap
-//	remote2IP := net.ParseIP("10.128.0.3")
-//	remote2UDPAddr := NewUDPAddr(remote2IP, uint16(4242))
-//	lh.addRemoteV4(ip2int(remote2IP), ip2int(remote2IP), NewIp4AndPort(remote2UDPAddr.IP, uint32(remote2UDPAddr.Port)), false, false)
-//	assertUdpAddrInArray(t, lh.addrMap[ip2int(remote2IP)].CopyAddrs([]*net.IPNet{}), remote2UDPAddr)
-//
-//	// Another good ip gets into the cache, ordering is inverted
-//	remote3IP := net.ParseIP("10.128.0.4")
-//	remote3UDPAddr := NewUDPAddr(remote3IP, uint16(4243))
-//	lh.addRemoteV4(ip2int(remote2IP), ip2int(remote2IP), NewIp4AndPort(remote3UDPAddr.IP, uint32(remote3UDPAddr.Port)), false, false)
-//	assertUdpAddrInArray(t, lh.addrMap[ip2int(remote2IP)].CopyAddrs([]*net.IPNet{}), remote2UDPAddr, remote3UDPAddr)
-//
-//	// If we exceed the length limit we should only have the most recent addresses
-//	addedAddrs := []*udpAddr{}
-//	for i := 0; i < 11; i++ {
-//		remoteUDPAddr := NewUDPAddr(net.IP{10, 128, 0, 4}, uint16(4243+i))
-//		lh.addRemoteV4(ip2int(remote2IP), ip2int(remote2IP), NewIp4AndPort(remoteUDPAddr.IP, uint32(remoteUDPAddr.Port)), false, false)
-//		// The first entry here is a duplicate, don't add it to the assert list
-//		if i != 0 {
-//			addedAddrs = append(addedAddrs, remoteUDPAddr)
-//		}
-//	}
-//
-//	// We should only have the last 10 of what we tried to add
-//	assert.True(t, len(addedAddrs) >= 10, "We should have tried to add at least 10 addresses")
-//	assertUdpAddrInArray(
-//		t,
-//		lh.addrMap[ip2int(remote2IP)].CopyAddrs([]*net.IPNet{}),
-//		addedAddrs[0],
-//		addedAddrs[1],
-//		addedAddrs[2],
-//		addedAddrs[3],
-//		addedAddrs[4],
-//		addedAddrs[5],
-//		addedAddrs[6],
-//		addedAddrs[7],
-//		addedAddrs[8],
-//		addedAddrs[9],
-//	)
-//}
-
 type testLhReply struct {
 	nebType    header.MessageType
 	nebSubType header.MessageSubType
@@ -485,7 +428,6 @@ func assertIp4InArray(t *testing.T, have []*V4AddrPort, want ...netip.AddrPort)
 	}
 
 	for k, w := range want {
-		//TODO: IPV6-WORK
 		h := protoV4AddrPortToNetAddrPort(have[k])
 		if !(h == w) {
 			assert.Fail(t, fmt.Sprintf("Response did not contain: %v at %v, found %v", w, k, h))

outside.go

@@ -234,7 +234,7 @@ func (f *Interface) sendCloseTunnel(h *HostInfo) {
 
 func (f *Interface) handleHostRoaming(hostinfo *HostInfo, vpnAddr netip.AddrPort) {
 	if vpnAddr.IsValid() && hostinfo.remote != vpnAddr {
-		//TODO: this is weird now that we can have multiple vpn addrs
+		//TODO: CERT-V2 this is weird now that we can have multiple vpn addrs
 		if !f.lightHouse.GetRemoteAllowList().Allow(hostinfo.vpnAddrs[0], vpnAddr.Addr()) {
 			hostinfo.logger(f.l).WithField("newAddr", vpnAddr).Debug("lighthouse.remote_allow_list denied roaming")
 			return
@@ -301,7 +301,7 @@ func parseV6(data []byte, incoming bool, fp *firewall.Packet) error {
 		fp.RemoteAddr, _ = netip.AddrFromSlice(data[24:40])
 	}
 
-	//TODO: whats a reasonable number of extension headers to attempt to parse?
+	//TODO: CERT-V2 whats a reasonable number of extension headers to attempt to parse?
 	//https://www.ietf.org/archive/id/draft-ietf-6man-eh-limits-00.html
 	protoAt := 6
 	offset := 40
@@ -351,7 +351,7 @@ func parseV6(data []byte, incoming bool, fp *firewall.Packet) error {
 			return nil
 
 		case layers.IPProtocolIPv6Fragment:
-			//TODO: can we determine the protocol?
+			//TODO: CERT-V2 can we determine the protocol?
 			fp.RemotePort = 0
 			fp.LocalPort = 0
 			fp.Fragment = true

overlay/tun_darwin.go

@@ -294,7 +294,7 @@ func (t *tun) activate6(network netip.Prefix) error {
 			Vltime: 0xffffffff,
 			Pltime: 0xffffffff,
 		},
-		//TODO: should we disable DAD (duplicate address detection) and mark this as a secured address?
+		//TODO: CERT-V2 should we disable DAD (duplicate address detection) and mark this as a secured address?
 		Flags: _IN6_IFF_NODAD,
 	}
 

overlay/tun_linux.go

@@ -290,7 +290,7 @@ func (t *tun) addIPs(link netlink.Link) error {
 
 	//add all new addresses
 	for i := range newAddrs {
-		//todo do we want to stack errors and try as many ops as possible?
+		//TODO: CERT-V2 do we want to stack errors and try as many ops as possible?
 		//AddrReplace still adds new IPs, but if their properties change it will change them as well
 		if err := netlink.AddrReplace(link, newAddrs[i]); err != nil {
 			return err

overlay/tun_netbsd.go

@@ -223,7 +223,7 @@ func (t *tun) removeRoutes(routes []Route) error {
 			continue
 		}
 
-		//todo is this right?
+		//TODO: CERT-V2 is this right?
 		cmd := exec.Command("/sbin/route", "-n", "delete", "-net", r.Cidr.String(), t.vpnNetworks[0].Addr().String())
 		t.l.Debug("command: ", cmd.String())
 		if err := cmd.Run(); err != nil {

overlay/tun_openbsd.go

@@ -170,7 +170,7 @@ func (t *tun) addRoutes(logErrors bool) error {
 			// We don't allow route MTUs so only install routes with a via
 			continue
 		}
-		//todo is this right?
+		//TODO: CERT-V2 is this right?
 		cmd := exec.Command("/sbin/route", "-n", "add", "-inet", r.Cidr.String(), t.vpnNetworks[0].Addr().String())
 		t.l.Debug("command: ", cmd.String())
 		if err := cmd.Run(); err != nil {
@@ -191,7 +191,7 @@ func (t *tun) removeRoutes(routes []Route) error {
 		if !r.Install {
 			continue
 		}
-		//todo is this right?
+		//TODO: CERT-V2 is this right?
 		cmd := exec.Command("/sbin/route", "-n", "delete", "-inet", r.Cidr.String(), t.vpnNetworks[0].Addr().String())
 		t.l.Debug("command: ", cmd.String())
 		if err := cmd.Run(); err != nil {

pki.go

@@ -121,7 +121,7 @@ func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError {
 			}
 
 		} else if currentState.v1Cert != nil {
-			//TODO: we should be able to tear this down
+			//TODO: CERT-V2 we should be able to tear this down
 			return util.NewContextualError("v1 certificate was removed, restart required", nil, err)
 		}
 
@@ -173,7 +173,7 @@ func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError {
 
 	p.cs.Store(newState)
 
-	//TODO: newState needs a stringer that does json
+	//TODO: CERT-V2 newState needs a stringer that does json
 	if initial {
 		p.l.WithField("cert", newState).Debug("Client nebula certificate(s)")
 	} else {
@@ -359,14 +359,14 @@ func newCertState(dv cert.Version, v1, v2 cert.Certificate, pkcs11backed bool, p
 			return nil, util.NewContextualError("v1 and v2 curve are not the same, ignoring", nil, nil)
 		}
 
-		//TODO: make sure v2 has v1s address
+		//TODO: CERT-V2 make sure v2 has v1s address
 
 		cs.defaultVersion = dv
 	}
 
 	if v1 != nil {
 		if pkcs11backed {
-			//TODO: We do not currently have a method to verify a public private key pair when the private key is in an hsm
+			//NOTE: We do not currently have a method to verify a public private key pair when the private key is in an hsm
 		} else {
 			if err := v1.VerifyPrivateKey(privateKeyCurve, privateKey); err != nil {
 				return nil, fmt.Errorf("private key is not a pair with public key in nebula cert")
@@ -387,7 +387,7 @@ func newCertState(dv cert.Version, v1, v2 cert.Certificate, pkcs11backed bool, p
 
 	if v2 != nil {
 		if pkcs11backed {
-			//TODO: We do not currently have a method to verify a public private key pair when the private key is in an hsm
+			//NOTE: We do not currently have a method to verify a public private key pair when the private key is in an hsm
 		} else {
 			if err := v2.VerifyPrivateKey(privateKeyCurve, privateKey); err != nil {
 				return nil, fmt.Errorf("private key is not a pair with public key in nebula cert")