Cert v2 more todos

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.

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