Cert v2 more todos (#1316)

allow_list.go

@@ -128,7 +128,6 @@ func newAllowList(k string, raw interface{}, handleKey func(key string, value in
 
 		ipNet = netip.PrefixFrom(ipNet.Addr().Unmap(), ipNet.Bits())
 
-		// TODO: should we error on duplicate CIDRs in the config?
 		tree.Insert(ipNet, value)
 
 		maskBits := ipNet.Bits()

cmd/nebula-cert/ca_test.go

@@ -16,8 +16,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-//TODO: test file permissions
-
 func Test_caSummary(t *testing.T) {
 	assert.Equal(t, "ca <flags>: create a self signed certificate authority", caSummary())
 }

cmd/nebula-cert/keygen_test.go

@@ -9,8 +9,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-//TODO: test file permissions
-
 func Test_keygenSummary(t *testing.T) {
 	assert.Equal(t, "keygen <flags>: create a public/private key pair. the public key can be passed to `nebula-cert sign`", keygenSummary())
 }

cmd/nebula-cert/main_test.go

@@ -11,8 +11,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-//TODO: all flag parsing continueOnError will print to stderr on its own currently
-
 func Test_help(t *testing.T) {
 	expected := "Usage of " + os.Args[0] + " <global flags> <mode>:\n" +
 		"  Global flags:\n" +

cmd/nebula-cert/sign_test.go

@@ -16,8 +16,6 @@ import (
 	"golang.org/x/crypto/ed25519"
 )
 
-//TODO: test file permissions
-
 func Test_signSummary(t *testing.T) {
 	assert.Equal(t, "sign <flags>: create and sign a certificate", signSummary())
 }

config/config_test.go

@@ -38,9 +38,6 @@ func TestConfig_Load(t *testing.T) {
 		"new": "hi",
 	}
 	assert.Equal(t, expected, c.Settings)
-
-	//TODO: test symlinked file
-	//TODO: test symlinked directory
 }
 
 func TestConfig_Get(t *testing.T) {

control.go

@@ -227,7 +227,6 @@ func (c *Control) CloseTunnel(vpnIp netip.Addr, localOnly bool) bool {
 // CloseAllTunnels is just like CloseTunnel except it goes through and shuts them all down, optionally you can avoid shutting down lighthouse tunnels
 // the int returned is a count of tunnels closed
 func (c *Control) CloseAllTunnels(excludeLighthouses bool) (closed int) {
-	//TODO: this is probably better as a function in ConnectionManager or HostMap directly
 	shutdown := func(h *HostInfo) {
 		if excludeLighthouses && c.f.lightHouse.IsAnyLighthouseAddr(h.vpnAddrs) {
 			return

e2e/handshakes_test.go

@@ -94,7 +94,6 @@ func TestGoodHandshake(t *testing.T) {
 	r.RenderHostmaps("Final hostmaps", myControl, theirControl)
 	myControl.Stop()
 	theirControl.Stop()
-	//TODO: assert hostmaps
 }
 
 func TestWrongResponderHandshake(t *testing.T) {
@@ -152,8 +151,6 @@ func TestWrongResponderHandshake(t *testing.T) {
 		return router.KeepRouting
 	})
 
-	//TODO: Assert pending hostmap - I should have a correct hostinfo for them now
-
 	t.Log("My cached packet should be received by them")
 	myCachedPacket := theirControl.GetFromTun(true)
 	assertUdpPacket(t, []byte("Hi from me"), myCachedPacket, myVpnIpNet[0].Addr(), theirVpnIpNet[0].Addr(), 80, 80)
@@ -169,7 +166,6 @@ func TestWrongResponderHandshake(t *testing.T) {
 	assert.Nil(t, myControl.GetHostInfoByVpnAddr(evilVpnIp[0].Addr(), true), "My pending hostmap should not contain evil")
 	assert.Nil(t, myControl.GetHostInfoByVpnAddr(evilVpnIp[0].Addr(), false), "My main hostmap should not contain evil")
 
-	//TODO: assert hostmaps for everyone
 	r.RenderHostmaps("Final hostmaps", myControl, theirControl, evilControl)
 	t.Log("Success!")
 	myControl.Stop()
@@ -236,8 +232,6 @@ func TestWrongResponderHandshakeStaticHostMap(t *testing.T) {
 		return router.KeepRouting
 	})
 
-	//TODO: Assert pending hostmap - I should have a correct hostinfo for them now
-
 	t.Log("My cached packet should be received by them")
 	myCachedPacket := theirControl.GetFromTun(true)
 	assertUdpPacket(t, []byte("Hi from me"), myCachedPacket, myVpnIpNet[0].Addr(), theirVpnIpNet[0].Addr(), 80, 80)
@@ -254,7 +248,6 @@ func TestWrongResponderHandshakeStaticHostMap(t *testing.T) {
 	assert.Nil(t, myControl.GetHostInfoByVpnAddr(evilVpnIp[0].Addr(), false), "My main hostmap should not contain evil")
 	//NOTE: if evil lost the handshake race it may still have a tunnel since me would reject the handshake since the tunnel is complete
 
-	//TODO: assert hostmaps for everyone
 	r.RenderHostmaps("Final hostmaps", myControl, theirControl, evilControl)
 	t.Log("Success!")
 	myControl.Stop()
@@ -468,7 +461,6 @@ func TestRelays(t *testing.T) {
 	r.Log("Assert the tunnel works")
 	assertUdpPacket(t, []byte("Hi from me"), p, myVpnIpNet[0].Addr(), theirVpnIpNet[0].Addr(), 80, 80)
 	r.RenderHostmaps("Final hostmaps", myControl, relayControl, theirControl)
-	//TODO: assert we actually used the relay even though it should be impossible for a tunnel to have occurred without it
 }
 
 func TestReestablishRelays(t *testing.T) {
@@ -647,8 +639,6 @@ func TestStage1RaceRelays(t *testing.T) {
 	myControl.Stop()
 	theirControl.Stop()
 	relayControl.Stop()
-	//
-	////TODO: assert hostmaps
 }
 
 func TestStage1RaceRelays2(t *testing.T) {
@@ -735,9 +725,6 @@ func TestStage1RaceRelays2(t *testing.T) {
 	myControl.Stop()
 	theirControl.Stop()
 	relayControl.Stop()
-
-	//
-	////TODO: assert hostmaps
 }
 
 func TestRehandshakingRelays(t *testing.T) {
@@ -1173,7 +1160,6 @@ func TestRaceRegression(t *testing.T) {
 	myControl.InjectUDPPacket(theirStage1ForMe)
 	theirControl.InjectUDPPacket(myStage1ForThem)
 
-	//TODO: ensure stage 2
 	t.Log("Get both stage 2")
 	myStage2ForThem := myControl.GetFromUDP(true)
 	theirStage2ForMe := theirControl.GetFromUDP(true)
@@ -1237,9 +1223,3 @@ func TestV2NonPrimaryWithLighthouse(t *testing.T) {
 	myControl.Stop()
 	theirControl.Stop()
 }
-
-//TODO: test
-// Race winner renews and handshakes
-// Race loser renews and handshakes
-// Does race winner repin the cert to old?
-//TODO: add a test with many lies

e2e/helpers_test.go

@@ -178,22 +178,6 @@ func assertHostInfoPair(t *testing.T, addrA, addrB netip.AddrPort, vpnNetsA, vpn
 	// Check that our indexes match
 	assert.Equal(t, hBinA.LocalIndex, hAinB.RemoteIndex, "Host B local index does not match host A remote index")
 	assert.Equal(t, hBinA.RemoteIndex, hAinB.LocalIndex, "Host B remote index does not match host A local index")
-
-	//TODO: Would be nice to assert this memory
-	//checkIndexes := func(name string, hm *HostMap, hi *HostInfo) {
-	//	hBbyIndex := hmA.Indexes[hBinA.localIndexId]
-	//	assert.NotNil(t, hBbyIndex, "Could not host info by local index in %s", name)
-	//	assert.Equal(t, &hBbyIndex, &hBinA, "%s Indexes map did not point to the right host info", name)
-	//
-	//	//TODO: remote indexes are susceptible to collision
-	//	hBbyRemoteIndex := hmA.RemoteIndexes[hBinA.remoteIndexId]
-	//	assert.NotNil(t, hBbyIndex, "Could not host info by remote index in %s", name)
-	//	assert.Equal(t, &hBbyRemoteIndex, &hBinA, "%s RemoteIndexes did not point to the right host info", name)
-	//}
-	//
-	//// Check hostmap indexes too
-	//checkIndexes("hmA", hmA, hBinA)
-	//checkIndexes("hmB", hmB, hAinB)
 }
 
 func assertUdpPacket(t *testing.T, expected, b []byte, fromIp, toIp netip.Addr, fromPort, toPort uint16) {

examples/config.yml

@@ -250,7 +250,6 @@ tun:
   # in nebula configuration files. Default false, not reloadable.
   #use_system_route_table: false
 
-# TODO
 # Configure logging level
 logging:
   # panic, fatal, error, warning, info, or debug. Default is info and is reloadable.
@@ -342,9 +341,8 @@ firewall:
   #   host: `any` or a literal hostname, ie `test-host`
   #   group: `any` or a literal group name, ie `default-group`
   #   groups: Same as group but accepts a list of values. Multiple values are AND'd together and a certificate would have to contain all groups to pass
-  #   cidr: a remote CIDR, `0.0.0.0/0` is any ipv4 and `::/0` is any ipv6. //TODO: we have a problem, firewall needs to understand this and should probably allow `any` for both
+  #   cidr: a remote CIDR, `0.0.0.0/0` is any ipv4 and `::/0` is any ipv6.
   #   local_cidr: a local CIDR, `0.0.0.0/0` is any ipv4 and `::/0` is any ipv6. This could be used to filter destinations when using unsafe_routes.
-  #   //TODO: probably should have an `any` that covers both ip versions
   #     If no unsafe networks are present in the certificate(s) or `default_local_cidr_any` is true then the default is any ipv4 or ipv6 network.
   #     Otherwise the default is any vpn network assigned to via the certificate.
   #     `default_local_cidr_any` defaults to false and is deprecated, it will be removed in a future release.

firewall_test.go

@@ -575,8 +575,6 @@ func BenchmarkLookup(b *testing.B) {
 			ml(m, a)
 		}
 	})
-
-	//TODO: only way array lookup in array will help is if both are sorted, then maybe it's faster
 }
 
 func Test_parsePort(t *testing.T) {

handshake_ix.go

@@ -533,7 +533,6 @@ func ixHandshakeStage2(f *Interface, addr netip.AddrPort, via *ViaSender, hh *Ha
 
 		// Create a new hostinfo/handshake for the intended vpn ip
 		f.handshakeManager.StartHandshake(hostinfo.vpnAddrs[0], func(newHH *HandshakeHostInfo) {
-			//TODO: this doesnt know if its being added or is being used for caching a packet
 			// Block the current used address
 			newHH.hostinfo.remotes = hostinfo.remotes
 			newHH.hostinfo.remotes.BlockRemote(addr)

handshake_manager.go

@@ -224,7 +224,7 @@ func (hm *HandshakeManager) handleOutbound(vpnIp netip.Addr, lighthouseTriggered
 
 	hh.lastRemotes = remotes
 
-	// TODO: this will generate a load of queries for hosts with only 1 ip
+	// This will generate a load of queries for hosts with only 1 ip
 	// (such as ones registered to the lighthouse with only a private IP)
 	// So we only do it one time after attempting 5 handshakes already.
 	if len(remotes) <= 1 && hh.counter == 5 {

hostmap.go

@@ -780,8 +780,6 @@ func localAddrs(l *logrus.Logger, allowList *LocalAllowList) []netip.Addr {
 			}
 			addr = addr.Unmap()
 
-			//TODO: Filtering out link local for now, this is probably the most correct thing
-			//TODO: Would be nice to filter out SLAAC MAC based ips as well
 			if addr.IsLoopback() == false && addr.IsLinkLocalUnicast() == false {
 				isAllowed := allowList.Allow(addr)
 				if l.Level >= logrus.TraceLevel {

inside.go

@@ -27,7 +27,6 @@ func (f *Interface) consumeInsidePacket(packet []byte, fwPacket *firewall.Packet
 		}
 	}
 
-	//TODO: seems like a huge bummer
 	_, found := f.myVpnAddrsTable.Lookup(fwPacket.RemoteAddr)
 	if found {
 		// Immediately forward packets from self to self.
@@ -264,7 +263,6 @@ func (f *Interface) SendVia(via *HostInfo,
 
 func (f *Interface) sendNoMetrics(t header.MessageType, st header.MessageSubType, ci *ConnectionState, hostinfo *HostInfo, remote netip.AddrPort, p, nb, out []byte, q int) {
 	if ci.eKey == nil {
-		//TODO: log warning
 		return
 	}
 	useRelay := !remote.IsValid() && !hostinfo.remote.IsValid()

interface.go

@@ -410,6 +410,8 @@ func (f *Interface) emitStats(ctx context.Context, i time.Duration) {
 	udpStats := udp.NewUDPStatsEmitter(f.writers)
 
 	certExpirationGauge := metrics.GetOrRegisterGauge("certificate.ttl_seconds", nil)
+	certDefaultVersion := metrics.GetOrRegisterGauge("certificate.default_version", nil)
+	certMaxVersion := metrics.GetOrRegisterGauge("certificate.max_version", nil)
 
 	for {
 		select {
@@ -419,8 +421,18 @@ func (f *Interface) emitStats(ctx context.Context, i time.Duration) {
 			f.firewall.EmitStats()
 			f.handshakeManager.EmitStats()
 			udpStats()
-			certExpirationGauge.Update(int64(f.pki.getCertState().GetDefaultCertificate().NotAfter().Sub(time.Now()) / time.Second))
+
-			//TODO: we should also report the default certificate version
+			certState := f.pki.getCertState()
+			defaultCrt := certState.GetDefaultCertificate()
+			certExpirationGauge.Update(int64(defaultCrt.NotAfter().Sub(time.Now()) / time.Second))
+			certDefaultVersion.Update(int64(defaultCrt.Version()))
+
+			// Report the max certificate version we are capable of using
+			if certState.v2Cert != nil {
+				certMaxVersion.Update(int64(certState.v2Cert.Version()))
+			} else {
+				certMaxVersion.Update(int64(certState.v1Cert.Version()))
+			}
 		}
 	}
 }

lighthouse.go

@@ -241,7 +241,6 @@ func (lh *LightHouse) reload(c *config.C, initial bool) error {
 
 		lh.remoteAllowList.Store(ral)
 		if !initial {
-			//TODO: a diff will be annoyingly difficult
 			lh.l.Info("lighthouse.remote_allow_list and/or lighthouse.remote_allow_ranges has changed")
 		}
 	}
@@ -254,7 +253,6 @@ func (lh *LightHouse) reload(c *config.C, initial bool) error {
 
 		lh.localAllowList.Store(lal)
 		if !initial {
-			//TODO: a diff will be annoyingly difficult
 			lh.l.Info("lighthouse.local_allow_list has changed")
 		}
 	}
@@ -267,7 +265,6 @@ func (lh *LightHouse) reload(c *config.C, initial bool) error {
 
 		lh.calculatedRemotes.Store(cr)
 		if !initial {
-			//TODO: a diff will be annoyingly difficult
 			lh.l.Info("lighthouse.calculated_remotes has changed")
 		}
 	}
@@ -294,7 +291,6 @@ func (lh *LightHouse) reload(c *config.C, initial bool) error {
 
 		lh.staticList.Store(&staticList)
 		if !initial {
-			//TODO: we should remove any remote list entries for static hosts that were removed/modified?
 			if c.HasChanged("static_host_map") {
 				lh.l.Info("static_host_map has changed")
 			}
@@ -1007,7 +1003,6 @@ func (lhh *LightHouseHandler) resetMeta() *NebulaMeta {
 	details.V6AddrPorts = details.V6AddrPorts[:0]
 	details.RelayVpnAddrs = details.RelayVpnAddrs[:0]
 	details.OldRelayVpnAddrs = details.OldRelayVpnAddrs[:0]
-	//TODO: these are unfortunate
 	details.OldVpnAddr = 0
 	details.VpnAddr = nil
 	lhh.meta.Details = details
@@ -1077,7 +1072,6 @@ func (lhh *LightHouseHandler) handleHostQuery(n *NebulaMeta, fromVpnAddrs []neti
 		return
 	}
 
-	//TODO: Maybe instead of marshalling into n we marshal into a new `r` to not nuke our current request data
 	found, ln, err := lhh.lh.queryAndPrepMessage(queryVpnAddr, func(c *cache) (int, error) {
 		n = lhh.resetMeta()
 		n.Type = NebulaMeta_HostQueryReply

lighthouse_test.go

@@ -16,8 +16,6 @@ import (
 	"gopkg.in/yaml.v2"
 )
 
-//TODO: Add a test to ensure udpAddr is copied and not reused
-
 func TestOldIPv4Only(t *testing.T) {
 	// This test ensures our new ipv6 enabled LH protobuf IpAndPorts works with the old style to enable backwards compatibility
 	b := []byte{8, 129, 130, 132, 80, 16, 10}

main.go

@@ -256,8 +256,6 @@ func Main(c *config.C, configTest bool, buildVersion string, logger *logrus.Logg
 			return nil, fmt.Errorf("failed to initialize interface: %s", err)
 		}
 
-		// TODO: Better way to attach these, probably want a new interface in InterfaceConfig
-		// I don't want to make this initial commit too far-reaching though
 		ifce.writers = udpConns
 		lightHouse.ifce = ifce
 
@@ -269,8 +267,6 @@ func Main(c *config.C, configTest bool, buildVersion string, logger *logrus.Logg
 		go handshakeManager.Run(ctx)
 	}
 
-	// TODO - stats third-party modules start uncancellable goroutines. Update those libs to accept
-	// a context so that they can exit when the context is Done.
 	statsStart, err := startStats(l, c, buildVersion, configTest)
 	if err != nil {
 		return nil, util.ContextualizeIfNeeded("Failed to start stats emitter", err)
@@ -280,7 +276,6 @@ func Main(c *config.C, configTest bool, buildVersion string, logger *logrus.Logg
 		return nil, nil
 	}
 
-	//TODO: check if we _should_ be emitting stats
 	go ifce.emitStats(ctx, c.GetDuration("stats.interval", time.Second*10))
 
 	attachCommands(l, c, ssh, ifce)

message_metrics.go

@@ -7,8 +7,6 @@ import (
 	"github.com/slackhq/nebula/header"
 )
 
-//TODO: this can probably move into the header package
-
 type MessageMetrics struct {
 	rx [][]metrics.Counter
 	tx [][]metrics.Counter

outside.go

@@ -23,8 +23,6 @@ const (
 func (f *Interface) readOutsidePackets(ip netip.AddrPort, via *ViaSender, out []byte, packet []byte, h *header.H, fwPacket *firewall.Packet, lhf *LightHouseHandler, nb []byte, q int, localCache firewall.ConntrackCache) {
 	err := h.Parse(packet)
 	if err != nil {
-		// TODO: best if we return this and let caller log
-		// TODO: Might be better to send the literal []byte("holepunch") packet and ignore that?
 		// Hole punch packets are 0 or 1 byte big, so lets ignore printing those errors
 		if len(packet) > 1 {
 			f.l.WithField("packet", packet).Infof("Error while parsing inbound packet from %s: %s", ip, err)
@@ -139,9 +137,6 @@ func (f *Interface) readOutsidePackets(ip netip.AddrPort, via *ViaSender, out []
 			hostinfo.logger(f.l).WithError(err).WithField("udpAddr", ip).
 				WithField("packet", packet).
 				Error("Failed to decrypt lighthouse packet")
-
-			//TODO: maybe after build 64 is out? 06/14/2018 - NB
-			//f.sendRecvError(net.Addr(addr), header.RemoteIndex)
 			return
 		}
 
@@ -160,9 +155,6 @@ func (f *Interface) readOutsidePackets(ip netip.AddrPort, via *ViaSender, out []
 			hostinfo.logger(f.l).WithError(err).WithField("udpAddr", ip).
 				WithField("packet", packet).
 				Error("Failed to decrypt test packet")
-
-			//TODO: maybe after build 64 is out? 06/14/2018 - NB
-			//f.sendRecvError(net.Addr(addr), header.RemoteIndex)
 			return
 		}
 
@@ -322,7 +314,6 @@ func parseV6(data []byte, incoming bool, fp *firewall.Packet) error {
 		//fmt.Println(proto, protoAt)
 		switch proto {
 		case layers.IPProtocolICMPv6:
-			//TODO: we need a new protocol in config language "icmpv6"
 			fp.Protocol = uint8(proto)
 			fp.RemotePort = 0
 			fp.LocalPort = 0
@@ -463,8 +454,6 @@ func (f *Interface) decryptToTun(hostinfo *HostInfo, messageCounter uint64, out
 	out, err = hostinfo.ConnectionState.dKey.DecryptDanger(out, packet[:header.Len], packet[header.Len:], messageCounter, nb)
 	if err != nil {
 		hostinfo.logger(f.l).WithError(err).Error("Failed to decrypt packet")
-		//TODO: maybe after build 64 is out? 06/14/2018 - NB
-		//f.sendRecvError(hostinfo.remote, header.RemoteIndex)
 		return false
 	}
 
@@ -511,7 +500,6 @@ func (f *Interface) maybeSendRecvError(endpoint netip.AddrPort, index uint32) {
 func (f *Interface) sendRecvError(endpoint netip.AddrPort, index uint32) {
 	f.messageMetrics.Tx(header.RecvError, 0, 1)
 
-	//TODO: this should be a signed message so we can trust that we should drop the index
 	b := header.Encode(make([]byte, header.Len), header.Version, header.RecvError, 0, index, 0)
 	f.outside.WriteTo(b, endpoint)
 	if f.l.Level >= logrus.DebugLevel {
@@ -547,25 +535,3 @@ func (f *Interface) handleRecvError(addr netip.AddrPort, h *header.H) {
 	// We also delete it from pending hostmap to allow for fast reconnect.
 	f.handshakeManager.DeleteHostInfo(hostinfo)
 }
-
-/*
-func (f *Interface) sendMeta(ci *ConnectionState, endpoint *net.UDPAddr, meta *NebulaMeta) {
-	if ci.eKey != nil {
-		//TODO: log error?
-		return
-	}
-
-	msg, err := proto.Marshal(meta)
-	if err != nil {
-		l.Debugln("failed to encode header")
-	}
-
-	c := ci.messageCounter
-	b := HeaderEncode(nil, Version, uint8(metadata), 0, hostinfo.remoteIndexId, c)
-	ci.messageCounter++
-
-	msg := ci.eKey.EncryptDanger(b, nil, msg, c)
-	//msg := ci.eKey.EncryptDanger(b, nil, []byte(fmt.Sprintf("%d", counter)), c)
-	f.outside.WriteTo(msg, endpoint)
-}
-*/

overlay/tun_darwin.go

@@ -351,7 +351,7 @@ func (t *tun) RouteFor(ip netip.Addr) netip.Addr {
 }
 
 // Get the LinkAddr for the interface of the given name
-// TODO: Is there an easier way to fetch this when we create the interface?
+// Is there an easier way to fetch this when we create the interface?
 // Maybe SIOCGIFINDEX? but this doesn't appear to exist in the darwin headers.
 func getLinkAddr(name string) (*netroute.LinkAddr, error) {
 	rib, err := netroute.FetchRIB(unix.AF_UNSPEC, unix.NET_RT_IFLIST, 0)

relay_manager.go

@@ -92,11 +92,24 @@ func AddRelay(l *logrus.Logger, relayHostInfo *HostInfo, hm *HostMap, vpnIp neti
 func (rm *relayManager) EstablishRelay(relayHostInfo *HostInfo, m *NebulaControl) (*Relay, error) {
 	relay, ok := relayHostInfo.relayState.CompleteRelayByIdx(m.InitiatorRelayIndex, m.ResponderRelayIndex)
 	if !ok {
-		//TODO: we need to handle possibly logging deprecated fields as well
+		fields := logrus.Fields{
-		rm.l.WithFields(logrus.Fields{"relay": relayHostInfo.vpnAddrs[0],
+			"relay":               relayHostInfo.vpnAddrs[0],
 			"initiatorRelayIndex": m.InitiatorRelayIndex,
-			"relayFrom":           m.RelayFromAddr,
+		}
-			"relayTo":             m.RelayToAddr}).Info("relayManager failed to update relay")
+
+		if m.RelayFromAddr == nil {
+			fields["relayFrom"] = m.OldRelayFromAddr
+		} else {
+			fields["relayFrom"] = m.RelayFromAddr
+		}
+
+		if m.RelayToAddr == nil {
+			fields["relayTo"] = m.OldRelayToAddr
+		} else {
+			fields["relayTo"] = m.RelayToAddr
+		}
+
+		rm.l.WithFields(fields).Info("relayManager failed to update relay")
 		return nil, fmt.Errorf("unknown relay")
 	}
 
@@ -115,7 +128,6 @@ func (rm *relayManager) HandleControlMsg(h *HostInfo, d []byte, f *Interface) {
 	if msg.OldRelayFromAddr > 0 || msg.OldRelayToAddr > 0 {
 		v = cert.Version1
 
-		//TODO: yeah this is junk but maybe its less junky than the other options
 		b := [4]byte{}
 		binary.BigEndian.PutUint32(b[:], msg.OldRelayFromAddr)
 		msg.RelayFromAddr = netAddrToProtoAddr(netip.AddrFrom4(b))
@@ -181,7 +193,12 @@ func (rm *relayManager) handleCreateRelayResponse(v cert.Version, h *HostInfo, f
 		if v == cert.Version1 {
 			peer := peerHostInfo.vpnAddrs[0]
 			if !peer.Is4() {
-				//TODO: log cant do it
+				rm.l.WithField("relayFrom", peer).
+					WithField("relayTo", target).
+					WithField("initiatorRelayIndex", resp.InitiatorRelayIndex).
+					WithField("responderRelayIndex", resp.ResponderRelayIndex).
+					WithField("vpnAddrs", peerHostInfo.vpnAddrs).
+					Error("Refusing to CreateRelayResponse for a v1 relay with an ipv6 address")
 				return
 			}
 
@@ -303,7 +320,6 @@ func (rm *relayManager) handleCreateRelayRequest(v cert.Version, h *HostInfo, f
 		} else {
 			f.SendMessageToHostInfo(header.Control, 0, h, msg, make([]byte, 12), make([]byte, mtu))
 			rm.l.WithFields(logrus.Fields{
-				//TODO: IPV6-WORK, this used to use the resp object but I am getting lazy now
 				"relayFrom":           from,
 				"relayTo":             target,
 				"initiatorRelayIndex": resp.InitiatorRelayIndex,
@@ -349,7 +365,12 @@ func (rm *relayManager) handleCreateRelayRequest(v cert.Version, h *HostInfo, f
 
 		if v == cert.Version1 {
 			if !h.vpnAddrs[0].Is4() {
-				//TODO: log it
+				rm.l.WithField("relayFrom", h.vpnAddrs[0]).
+					WithField("relayTo", target).
+					WithField("initiatorRelayIndex", req.InitiatorRelayIndex).
+					WithField("responderRelayIndex", req.ResponderRelayIndex).
+					WithField("vpnAddr", target).
+					Error("Refusing to CreateRelayRequest for a v1 relay with an ipv6 address")
 				return
 			}
 
@@ -369,7 +390,6 @@ func (rm *relayManager) handleCreateRelayRequest(v cert.Version, h *HostInfo, f
 		} else {
 			f.SendMessageToHostInfo(header.Control, 0, peer, msg, make([]byte, 12), make([]byte, mtu))
 			rm.l.WithFields(logrus.Fields{
-				//TODO: IPV6-WORK another lazy used to use the req object
 				"relayFrom":           h.vpnAddrs[0],
 				"relayTo":             target,
 				"initiatorRelayIndex": req.InitiatorRelayIndex,

remote_list.go

@@ -33,9 +33,6 @@ type Cache struct {
 	Relay    []netip.Addr     `json:"relay"`
 }
 
-//TODO: Seems like we should plop static host entries in here too since the are protected by the lighthouse from deletion
-// We will never clean learned/reported information for them as it stands today
-
 // cache is an internal struct that splits v4 and v6 addresses inside the cache map
 type cache struct {
 	v4    *cacheV4
@@ -275,7 +272,6 @@ func (r *RemoteList) CopyAddrs(preferredRanges []netip.Prefix) []netip.AddrPort
 // LearnRemote locks and sets the learned slot for the owner vpn ip to the provided addr
 // Currently this is only needed when HostInfo.SetRemote is called as that should cover both handshaking and roaming.
 // It will mark the deduplicated address list as dirty, so do not call it unless new information is available
-// TODO: this needs to support the allow list list
 func (r *RemoteList) LearnRemote(ownerVpnIp netip.Addr, remote netip.AddrPort) {
 	r.Lock()
 	defer r.Unlock()
@@ -386,7 +382,6 @@ func (r *RemoteList) Rebuild(preferredRanges []netip.Prefix) {
 	defer r.Unlock()
 
 	// Only rebuild if the cache changed
-	//TODO: shouldRebuild is probably pointless as we don't check for actual change when lighthouse updates come in
 	if r.shouldRebuild {
 		r.unlockedCollect()
 		r.shouldRebuild = false
@@ -709,7 +704,6 @@ func minInt(a, b int) int {
 
 // isPreferred returns true of the ip is contained in the preferredRanges list
 func isPreferred(ip netip.Addr, preferredRanges []netip.Prefix) bool {
-	//TODO: this would be better in a CIDR6Tree
 	for _, p := range preferredRanges {
 		if p.Contains(ip) {
 			return true

ssh.go

@@ -77,9 +77,6 @@ func wireSSHReload(l *logrus.Logger, ssh *sshd.SSHServer, c *config.C) {
 // that callers may invoke to run the configured ssh server. On
 // failure, it returns nil, error.
 func configSSH(l *logrus.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error) {
-	//TODO conntrack list
-	//TODO print firewall rules or hash?
-
 	listen := c.GetString("sshd.listen", "")
 	if listen == "" {
 		return nil, fmt.Errorf("sshd.listen must be provided")
@@ -93,7 +90,6 @@ func configSSH(l *logrus.Logger, ssh *sshd.SSHServer, c *config.C) (func(), erro
 		return nil, fmt.Errorf("sshd.listen can not use port 22")
 	}
 
-	//TODO: no good way to reload this right now
 	hostKeyPathOrKey := c.GetString("sshd.host_key", "")
 	if hostKeyPathOrKey == "" {
 		return nil, fmt.Errorf("sshd.host_key must be provided")
@@ -418,7 +414,6 @@ func attachCommands(l *logrus.Logger, c *config.C, ssh *sshd.SSHServer, f *Inter
 func sshListHostMap(hl controlHostLister, a interface{}, w sshd.StringWriter) error {
 	fs, ok := a.(*sshListHostMapFlags)
 	if !ok {
-		//TODO: error
 		return nil
 	}
 
@@ -441,7 +436,6 @@ func sshListHostMap(hl controlHostLister, a interface{}, w sshd.StringWriter) er
 
 		err := js.Encode(hm)
 		if err != nil {
-			//TODO
 			return nil
 		}
 
@@ -460,7 +454,6 @@ func sshListHostMap(hl controlHostLister, a interface{}, w sshd.StringWriter) er
 func sshListLighthouseMap(lightHouse *LightHouse, a interface{}, w sshd.StringWriter) error {
 	fs, ok := a.(*sshListHostMapFlags)
 	if !ok {
-		//TODO: error
 		return nil
 	}
 
@@ -493,7 +486,6 @@ func sshListLighthouseMap(lightHouse *LightHouse, a interface{}, w sshd.StringWr
 
 		err := js.Encode(addrMap)
 		if err != nil {
-			//TODO
 			return nil
 		}
 
@@ -564,7 +556,6 @@ func sshQueryLighthouse(ifce *Interface, fs interface{}, a []string, w sshd.Stri
 func sshCloseTunnel(ifce *Interface, fs interface{}, a []string, w sshd.StringWriter) error {
 	flags, ok := fs.(*sshCloseTunnelFlags)
 	if !ok {
-		//TODO: error
 		return nil
 	}
 
@@ -605,7 +596,6 @@ func sshCloseTunnel(ifce *Interface, fs interface{}, a []string, w sshd.StringWr
 func sshCreateTunnel(ifce *Interface, fs interface{}, a []string, w sshd.StringWriter) error {
 	flags, ok := fs.(*sshCreateTunnelFlags)
 	if !ok {
-		//TODO: error
 		return nil
 	}
 
@@ -651,7 +641,6 @@ func sshCreateTunnel(ifce *Interface, fs interface{}, a []string, w sshd.StringW
 func sshChangeRemote(ifce *Interface, fs interface{}, a []string, w sshd.StringWriter) error {
 	flags, ok := fs.(*sshChangeRemoteFlags)
 	if !ok {
-		//TODO: error
 		return nil
 	}
 
@@ -781,7 +770,6 @@ func sshLogFormat(l *logrus.Logger, fs interface{}, a []string, w sshd.StringWri
 func sshPrintCert(ifce *Interface, fs interface{}, a []string, w sshd.StringWriter) error {
 	args, ok := fs.(*sshPrintCertFlags)
 	if !ok {
-		//TODO: error
 		return nil
 	}
 
@@ -807,7 +795,6 @@ func sshPrintCert(ifce *Interface, fs interface{}, a []string, w sshd.StringWrit
 	if args.Json || args.Pretty {
 		b, err := cert.MarshalJSON()
 		if err != nil {
-			//TODO: handle it
 			return nil
 		}
 
@@ -816,7 +803,6 @@ func sshPrintCert(ifce *Interface, fs interface{}, a []string, w sshd.StringWrit
 			err := json.Indent(buf, b, "", "    ")
 			b = buf.Bytes()
 			if err != nil {
-				//TODO: handle it
 				return nil
 			}
 		}
@@ -827,7 +813,6 @@ func sshPrintCert(ifce *Interface, fs interface{}, a []string, w sshd.StringWrit
 	if args.Raw {
 		b, err := cert.MarshalPEM()
 		if err != nil {
-			//TODO: handle it
 			return nil
 		}
 
@@ -840,7 +825,6 @@ func sshPrintCert(ifce *Interface, fs interface{}, a []string, w sshd.StringWrit
 func sshPrintRelays(ifce *Interface, fs interface{}, a []string, w sshd.StringWriter) error {
 	args, ok := fs.(*sshPrintTunnelFlags)
 	if !ok {
-		//TODO: error
 		w.WriteLine(fmt.Sprintf("sshPrintRelays failed to convert args type"))
 		return nil
 	}
@@ -938,7 +922,6 @@ func sshPrintRelays(ifce *Interface, fs interface{}, a []string, w sshd.StringWr
 func sshPrintTunnel(ifce *Interface, fs interface{}, a []string, w sshd.StringWriter) error {
 	args, ok := fs.(*sshPrintTunnelFlags)
 	if !ok {
-		//TODO: error
 		return nil
 	}
 

sshd/command.go

@@ -57,7 +57,6 @@ func execCommand(c *Command, args []string, w StringWriter) error {
 func dumpCommands(c *radix.Tree, w StringWriter) {
 	err := w.WriteLine("Available commands:")
 	if err != nil {
-		//TODO: log
 		return
 	}
 
@@ -67,10 +66,7 @@ func dumpCommands(c *radix.Tree, w StringWriter) {
 	}
 
 	sort.Strings(cmds)
-	err = w.Write(strings.Join(cmds, "\n") + "\n\n")
+	_ = w.Write(strings.Join(cmds, "\n") + "\n\n")
-	if err != nil {
-		//TODO: log
-	}
 }
 
 func lookupCommand(c *radix.Tree, sCmd string) (*Command, error) {
@@ -119,8 +115,6 @@ func helpCallback(commands *radix.Tree, a []string, w StringWriter) (err error)
 	// We are printing a specific commands help text
 	cmd, err := lookupCommand(commands, a[0])
 	if err != nil {
-		//TODO: handle error
-		//TODO: message the user
 		return
 	}
 

sshd/server.go

@@ -80,9 +80,7 @@ func NewSSHServer(l *logrus.Entry) (*SSHServer, error) {
 
 	s.config = &ssh.ServerConfig{
 		PublicKeyCallback: cc.Authenticate,
-		//TODO: AuthLogCallback: s.authAttempt,
+		ServerVersion:     fmt.Sprintf("SSH-2.0-Nebula???"),
-		//TODO: version string
-		ServerVersion: fmt.Sprintf("SSH-2.0-Nebula???"),
 	}
 
 	s.RegisterCommand(&Command{

sshd/session.go

@@ -62,7 +62,6 @@ func (s *session) handleChannels(chans <-chan ssh.NewChannel) {
 func (s *session) handleRequests(in <-chan *ssh.Request, channel ssh.Channel) {
 	for req := range in {
 		var err error
-		//TODO: maybe support window sizing?
 		switch req.Type {
 		case "shell":
 			if s.term == nil {
@@ -89,9 +88,7 @@ func (s *session) handleRequests(in <-chan *ssh.Request, channel ssh.Channel) {
 			req.Reply(true, nil)
 			s.dispatchCommand(payload.Value, &stringWriter{channel})
 
-			//TODO: Fix error handling and report the proper status back
 			status := struct{ Status uint32 }{uint32(0)}
-			//TODO: I think this is how we shut down a shell as well?
 			channel.SendRequest("exit-status", false, ssh.Marshal(status))
 			channel.Close()
 			return
@@ -110,7 +107,6 @@ func (s *session) handleRequests(in <-chan *ssh.Request, channel ssh.Channel) {
 }
 
 func (s *session) createTerm(channel ssh.Channel) *terminal.Terminal {
-	//TODO: PS1 with nebula cert name
 	term := terminal.NewTerminal(channel, s.c.User()+"@nebula > ")
 	term.AutoCompleteCallback = func(line string, pos int, key rune) (newLine string, newPos int, ok bool) {
 		// key 9 is tab
@@ -137,7 +133,6 @@ func (s *session) handleInput(channel ssh.Channel) {
 	for {
 		line, err := s.term.ReadLine()
 		if err != nil {
-			//TODO: log
 			break
 		}
 
@@ -148,7 +143,6 @@ func (s *session) handleInput(channel ssh.Channel) {
 func (s *session) dispatchCommand(line string, w StringWriter) {
 	args, err := shlex.Split(line, true)
 	if err != nil {
-		//todo: LOG IT
 		return
 	}
 
@@ -159,13 +153,11 @@ func (s *session) dispatchCommand(line string, w StringWriter) {
 
 	c, err := lookupCommand(s.commands, args[0])
 	if err != nil {
-		//TODO: handle the error
 		return
 	}
 
 	if c == nil {
 		err := w.WriteLine(fmt.Sprintf("did not understand: %s", line))
-		//TODO: log error
 		_ = err
 
 		dumpCommands(s.commands, w)
@@ -177,10 +169,7 @@ func (s *session) dispatchCommand(line string, w StringWriter) {
 		return
 	}
 
-	err = execCommand(c, args[1:], w)
+	_ = execCommand(c, args[1:], w)
-	if err != nil {
-		//TODO: log the error
-	}
 	return
 }
 

udp/udp_generic.go

@@ -58,7 +58,7 @@ func (u *GenericConn) LocalAddr() (netip.AddrPort, error) {
 }
 
 func (u *GenericConn) ReloadConfig(c *config.C) {
-	// TODO
+
 }
 
 func NewUDPStatsEmitter(udpConns []Conn) func() {

udp/udp_linux.go

@@ -17,8 +17,6 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-//TODO: make it support reload as best you can!
-
 type StdConn struct {
 	sysFd int
 	isV4  bool
@@ -57,7 +55,6 @@ func NewListener(l *logrus.Logger, ip netip.Addr, port int, multi bool, batch in
 		}
 	}
 
-	//TODO: support multiple listening IPs (for limiting ipv6)
 	var sa unix.Sockaddr
 	if ip.Is4() {
 		sa4 := &unix.SockaddrInet4{Port: port}
@@ -72,11 +69,6 @@ func NewListener(l *logrus.Logger, ip netip.Addr, port int, multi bool, batch in
 		return nil, fmt.Errorf("unable to bind to socket: %s", err)
 	}
 
-	//TODO: this may be useful for forcing threads into specific cores
-	//unix.SetsockoptInt(fd, unix.SOL_SOCKET, unix.SO_INCOMING_CPU, x)
-	//v, err := unix.GetsockoptInt(fd, unix.SOL_SOCKET, unix.SO_INCOMING_CPU)
-	//l.Println(v, err)
-
 	return &StdConn{sysFd: fd, isV4: ip.Is4(), l: l, batch: batch}, err
 }
 
@@ -215,8 +207,6 @@ func (u *StdConn) writeTo6(b []byte, ip netip.AddrPort) error {
 			return &net.OpError{Op: "sendto", Err: err}
 		}
 
-		//TODO: handle incomplete writes
-
 		return nil
 	}
 }
@@ -246,8 +236,6 @@ func (u *StdConn) writeTo4(b []byte, ip netip.AddrPort) error {
 			return &net.OpError{Op: "sendto", Err: err}
 		}
 
-		//TODO: handle incomplete writes
-
 		return nil
 	}
 }
@@ -294,7 +282,6 @@ func (u *StdConn) getMemInfo(meminfo *[unix.SK_MEMINFO_VARS]uint32) error {
 }
 
 func (u *StdConn) Close() error {
-	//TODO: this will not interrupt the read loop
 	return syscall.Close(u.sysFd)
 }
 

udp/udp_linux_32.go

@@ -39,7 +39,6 @@ func (u *StdConn) PrepareRawMessages(n int) ([]rawMessage, [][]byte, [][]byte) {
 		buffers[i] = make([]byte, MTU)
 		names[i] = make([]byte, unix.SizeofSockaddrInet6)
 
-		//TODO: this is still silly, no need for an array
 		vs := []iovec{
 			{Base: &buffers[i][0], Len: uint32(len(buffers[i]))},
 		}

udp/udp_linux_64.go

@@ -42,7 +42,6 @@ func (u *StdConn) PrepareRawMessages(n int) ([]rawMessage, [][]byte, [][]byte) {
 		buffers[i] = make([]byte, MTU)
 		names[i] = make([]byte, unix.SizeofSockaddrInet6)
 
-		//TODO: this is still silly, no need for an array
 		vs := []iovec{
 			{Base: &buffers[i][0], Len: uint64(len(buffers[i]))},
 		}