From e4daed356370d1cad03f751bccffa487af494201 Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Thu, 30 Jan 2025 13:33:19 -0600 Subject: [PATCH] Make sure all vpnAddrs are hoisted to primary, resolve a few more TODOs (#1319) --- cert/cert.go | 2 +- cmd/nebula-cert/sign.go | 2 - control.go | 2 +- control_test.go | 2 +- e2e/helpers_test.go | 2 +- hostmap.go | 20 ++++++-- iputil/packet.go | 2 - lighthouse.go | 9 ++-- lighthouse_test.go | 106 +++++++++------------------------------- outside.go | 6 +-- overlay/tun_darwin.go | 2 +- overlay/tun_linux.go | 2 +- overlay/tun_netbsd.go | 2 +- overlay/tun_openbsd.go | 4 +- pki.go | 10 ++-- 15 files changed, 61 insertions(+), 112 deletions(-) diff --git a/cert/cert.go b/cert/cert.go index 73c88cd..4246571 100644 --- a/cert/cert.go +++ b/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) } diff --git a/cmd/nebula-cert/sign.go b/cmd/nebula-cert/sign.go index 253ef86..ebcb592 100644 --- a/cmd/nebula-cert/sign.go +++ b/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 != "" { diff --git a/control.go b/control.go index 80a095e..20dd7fe 100644 --- a/control.go +++ b/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++ } diff --git a/control_test.go b/control_test.go index 0f21ed9..6ce7083 100644 --- a/control_test.go +++ b/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() diff --git a/e2e/helpers_test.go b/e2e/helpers_test.go index e71c42b..e1b7ac2 100644 --- a/e2e/helpers_test.go +++ b/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") diff --git a/hostmap.go b/hostmap.go index eca9dd7..f9e3c4e 100644 --- a/hostmap.go +++ b/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 diff --git a/iputil/packet.go b/iputil/packet.go index 719e034..b18e524 100644 --- a/iputil/packet.go +++ b/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 diff --git a/lighthouse.go b/lighthouse.go index 5c3b92f..ce37023 100644 --- a/lighthouse.go +++ b/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 { diff --git a/lighthouse_test.go b/lighthouse_test.go index 611d9f6..d5947aa 100644 --- a/lighthouse_test.go +++ b/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)) diff --git a/outside.go b/outside.go index f011c87..1efe50d 100644 --- a/outside.go +++ b/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 diff --git a/overlay/tun_darwin.go b/overlay/tun_darwin.go index 09af173..1a02b49 100644 --- a/overlay/tun_darwin.go +++ b/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, } diff --git a/overlay/tun_linux.go b/overlay/tun_linux.go index d850172..993bd4a 100644 --- a/overlay/tun_linux.go +++ b/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 diff --git a/overlay/tun_netbsd.go b/overlay/tun_netbsd.go index 5498491..f7586cb 100644 --- a/overlay/tun_netbsd.go +++ b/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 { diff --git a/overlay/tun_openbsd.go b/overlay/tun_openbsd.go index dcba054..a2fd184 100644 --- a/overlay/tun_openbsd.go +++ b/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 { diff --git a/pki.go b/pki.go index c7c93ac..888da7c 100644 --- a/pki.go +++ b/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")