Minor future proofing and cleanup in CertificateOfMembership, improve comments in a few places.

node/CertificateOfMembership.cpp

@@ -17,15 +17,15 @@ namespace ZeroTier {
 
 CertificateOfMembership::CertificateOfMembership(uint64_t timestamp,uint64_t timestampMaxDelta,uint64_t nwid,const Address &issuedTo)
 {
-	_qualifiers[0].id = COM_RESERVED_ID_TIMESTAMP;
-	_qualifiers[0].value = timestamp;
-	_qualifiers[0].maxDelta = timestampMaxDelta;
-	_qualifiers[1].id = COM_RESERVED_ID_NETWORK_ID;
-	_qualifiers[1].value = nwid;
-	_qualifiers[1].maxDelta = 0;
-	_qualifiers[2].id = COM_RESERVED_ID_ISSUED_TO;
-	_qualifiers[2].value = issuedTo.toInt();
-	_qualifiers[2].maxDelta = 0xffffffffffffffffULL;
+	_qualifiers[COM_RESERVED_ID_TIMESTAMP].id = COM_RESERVED_ID_TIMESTAMP;
+	_qualifiers[COM_RESERVED_ID_TIMESTAMP].value = timestamp;
+	_qualifiers[COM_RESERVED_ID_TIMESTAMP].maxDelta = timestampMaxDelta;
+	_qualifiers[COM_RESERVED_ID_NETWORK_ID].id = COM_RESERVED_ID_NETWORK_ID;
+	_qualifiers[COM_RESERVED_ID_NETWORK_ID].value = nwid;
+	_qualifiers[COM_RESERVED_ID_NETWORK_ID].maxDelta = 0;
+	_qualifiers[COM_RESERVED_ID_ISSUED_TO].id = COM_RESERVED_ID_ISSUED_TO;
+	_qualifiers[COM_RESERVED_ID_ISSUED_TO].value = issuedTo.toInt();
+	_qualifiers[COM_RESERVED_ID_ISSUED_TO].maxDelta = 0xffffffffffffffffULL;
 	_qualifierCount = 3;
 	_signatureLength = 0;
 }
@@ -107,7 +107,7 @@ bool CertificateOfMembership::sign(const Identity &with)
 
 int CertificateOfMembership::marshal(uint8_t data[ZT_CERTIFICATEOFMEMBERSHIP_MARSHAL_SIZE_MAX]) const noexcept
 {
-	data[0] = 1;
+	data[0] = 1; // 96-byte signature length; a v2 is supported in unmarshal code where signature is length prefixed
 	Utils::storeBigEndian<uint16_t>(data + 1,(uint16_t)_qualifierCount);
 	int p = 3;
 	for(unsigned int i=0;i<_qualifierCount;++i) {
@@ -117,10 +117,6 @@ int CertificateOfMembership::marshal(uint8_t data[ZT_CERTIFICATEOFMEMBERSHIP_MAR
 	}
 	_signedBy.copyTo(data + p); p += ZT_ADDRESS_LENGTH;
 	if ((_signedBy)&&(_signatureLength == 96)) {
-		// UGLY: Ed25519 signatures in ZT are 96 bytes (64 + 32 bytes of hash).
-		// P-384 signatures are also 96 bytes, praise the horned one. That means
-		// we don't need to include a length. If we ever do we will need a new
-		// serialized object version, but only for those with length != 96.
 		memcpy(data + p,_signature,96); p += 96;
 	}
 	return p;
@@ -128,7 +124,7 @@ int CertificateOfMembership::marshal(uint8_t data[ZT_CERTIFICATEOFMEMBERSHIP_MAR
 
 int CertificateOfMembership::unmarshal(const uint8_t *data,int len) noexcept
 {
-	if ((len < 3)||(data[0] != 1))
+	if ((len < 3)||(data[0] == 0))
 		return -1;
 	unsigned int numq = Utils::loadBigEndian<uint16_t>(data + 1);
 	if (numq > ZT_NETWORK_COM_MAX_QUALIFIERS)
@@ -146,9 +142,19 @@ int CertificateOfMembership::unmarshal(const uint8_t *data,int len) noexcept
 		return -1;
 	_signedBy.setTo(data + p); p += ZT_ADDRESS_LENGTH;
 	if (_signedBy) {
-		if ((p + 96) > len)
-			return -1;
 		_signatureLength = 96;
+		if (data[0] > 1) {
+			// If the version byte is >1, signatures come prefixed by a length. This is the
+			// way it should have been in the first place. Version byte 1 indicates 96 byte
+			// signatures and is backward compatible with <2.x nodes.
+			if ((p + 2) >= len)
+				return -1;
+			_signatureLength = Utils::loadBigEndian<uint16_t>(data + p); p += 2;
+			if (_signatureLength == 0)
+				return -1;
+		}
+		if ((p + _signatureLength) > len)
+			return -1;
 		memcpy(_signature,data + p,96);
 		p += 96;
 	}

node/CertificateOfMembership.hpp

@@ -129,6 +129,8 @@ public:
 	 */
 	ZT_ALWAYS_INLINE int64_t timestamp() const noexcept
 	{
+		if (_qualifiers[COM_RESERVED_ID_TIMESTAMP].id == COM_RESERVED_ID_TIMESTAMP)
+			return (int64_t)_qualifiers[0].value;
 		for(unsigned int i=0;i<_qualifierCount;++i) {
 			if (_qualifiers[i].id == COM_RESERVED_ID_TIMESTAMP)
 				return (int64_t)_qualifiers[i].value;
@@ -141,6 +143,8 @@ public:
 	 */
 	ZT_ALWAYS_INLINE Address issuedTo() const noexcept
 	{
+		if (_qualifiers[COM_RESERVED_ID_ISSUED_TO].id == COM_RESERVED_ID_ISSUED_TO)
+			return Address(_qualifiers[2].value);
 		for(unsigned int i=0;i<_qualifierCount;++i) {
 			if (_qualifiers[i].id == COM_RESERVED_ID_ISSUED_TO)
 				return Address(_qualifiers[i].value);
@@ -153,6 +157,8 @@ public:
 	 */
 	ZT_ALWAYS_INLINE uint64_t networkId() const noexcept
 	{
+		if (_qualifiers[COM_RESERVED_ID_NETWORK_ID].id == COM_RESERVED_ID_NETWORK_ID)
+			return _qualifiers[COM_RESERVED_ID_NETWORK_ID].value;
 		for(unsigned int i=0;i<_qualifierCount;++i) {
 			if (_qualifiers[i].id == COM_RESERVED_ID_NETWORK_ID)
 				return _qualifiers[i].value;

node/FCV.hpp

@@ -242,7 +242,7 @@ public:
 		}
 	}
 
-	ZT_ALWAYS_INLINE bool operator==(const FCV &v) const
+	ZT_ALWAYS_INLINE bool operator==(const FCV &v) const noexcept
 	{
 		if (_s == v._s) {
 			for(unsigned int i=0;i<_s;++i) {
@@ -253,11 +253,11 @@ public:
 		}
 		return false;
 	}
-	ZT_ALWAYS_INLINE bool operator!=(const FCV &v) const { return (!(*this == v)); }
-	ZT_ALWAYS_INLINE bool operator<(const FCV &v) const { return std::lexicographical_compare(begin(),end(),v.begin(),v.end()); }
-	ZT_ALWAYS_INLINE bool operator>(const FCV &v) const { return (v < *this); }
-	ZT_ALWAYS_INLINE bool operator<=(const FCV &v) const { return !(v < *this); }
-	ZT_ALWAYS_INLINE bool operator>=(const FCV &v) const { return !(*this < v); }
+	ZT_ALWAYS_INLINE bool operator!=(const FCV &v) const noexcept { return (!(*this == v)); }
+	ZT_ALWAYS_INLINE bool operator<(const FCV &v) const noexcept { return std::lexicographical_compare(begin(),end(),v.begin(),v.end()); }
+	ZT_ALWAYS_INLINE bool operator>(const FCV &v) const noexcept { return (v < *this); }
+	ZT_ALWAYS_INLINE bool operator<=(const FCV &v) const noexcept { return !(v < *this); }
+	ZT_ALWAYS_INLINE bool operator>=(const FCV &v) const noexcept { return !(*this < v); }
 
 private:
 	unsigned int _s;

node/Node.cpp

@@ -25,7 +25,6 @@
 #include "SelfAwareness.hpp"
 #include "Network.hpp"
 #include "Trace.hpp"
-#include "ScopedPtr.hpp"
 #include "Locator.hpp"
 #include "Protocol.hpp"
 #include "Expect.hpp"
@@ -112,6 +111,8 @@ Node::Node(void *uPtr,void *tPtr,const struct ZT_Node_Callbacks *callbacks,int64
 			stateObjectPut(tPtr,ZT_STATE_OBJECT_IDENTITY_PUBLIC,idtmp,RR->publicIdentityStr,(unsigned int)strlen(RR->publicIdentityStr));
 	}
 
+	RR->identity.hashWithPrivate(RR->localCacheSymmetricKey);
+
 	// This constructs all the components of the ZeroTier core within a single contiguous memory container,
 	// which reduces memory fragmentation and may improve cache locality.
 	_objects = new _NodeObjects(RR,tPtr);

node/Protocol.cpp

@@ -27,6 +27,9 @@
 namespace ZeroTier {
 namespace Protocol {
 
+// The counter used to assign packet IDs / cryptographic nonces.
+std::atomic<uint64_t> _s_packetIdCtr((uint64_t)time(nullptr) << 32U);
+
 uint64_t createProbe(const Identity &sender,const Identity &recipient,const uint8_t key[ZT_PEER_SECRET_KEY_LENGTH]) noexcept
 {
 	uint8_t tmp[ZT_IDENTITY_HASH_SIZE + ZT_IDENTITY_HASH_SIZE];
@@ -37,8 +40,6 @@ uint64_t createProbe(const Identity &sender,const Identity &recipient,const uint
 	return hash[0];
 }
 
-std::atomic<uint64_t> _s_packetIdCtr((uint64_t)time(nullptr) << 32U);
-
 void armor(Buf &pkt,int packetSize,const uint8_t key[ZT_PEER_SECRET_KEY_LENGTH],uint8_t cipherSuite) noexcept
 {
 	Protocol::Header &ph = pkt.as<Protocol::Header>();

node/RuntimeEnvironment.hpp

@@ -30,12 +30,16 @@ class Trace;
 class Expect;
 
 /**
- * Holds global state for an instance of ZeroTier::Node
+ * ZeroTier::Node execution context
+ *
+ * This just holds pointers and various other information used by all the
+ * various moving parts of a node. It's stored or passed as 'RR' to give it
+ * a common name througout the code.
  */
 class RuntimeEnvironment
 {
 public:
-	ZT_ALWAYS_INLINE RuntimeEnvironment(Node *n) :
+	ZT_ALWAYS_INLINE RuntimeEnvironment(Node *n) noexcept :
 		node(n),
 		localNetworkController(nullptr),
 		rtmem(nullptr),
@@ -53,6 +57,7 @@ public:
 	ZT_ALWAYS_INLINE ~RuntimeEnvironment()
 	{
 		Utils::burn(secretIdentityStr,sizeof(secretIdentityStr));
+		Utils::burn(localCacheSymmetricKey,sizeof(localCacheSymmetricKey));
 	}
 
 	// Node instance that owns this RuntimeEnvironment
@@ -64,12 +69,6 @@ public:
 	// Memory actually occupied by Trace, Switch, etc.
 	void *rtmem;
 
-	/* Order matters a bit here. These are constructed in this order
-	 * and then deleted in the opposite order on Node exit. The order ensures
-	 * that things that are needed are there before they're needed.
-	 *
-	 * These are constant and never null after startup unless indicated. */
-
 	Trace *t;
 	Expect *expect;
 	VL2 *vl2;
@@ -81,6 +80,10 @@ public:
 	Identity identity;
 	char publicIdentityStr[ZT_IDENTITY_STRING_BUFFER_LENGTH];
 	char secretIdentityStr[ZT_IDENTITY_STRING_BUFFER_LENGTH];
+
+	// A hash of this node identity's public and private keys that is used as
+	// a secret key to encrypt locally cached sensitive information.
+	uint8_t localCacheSymmetricKey[ZT_IDENTITY_HASH_SIZE];
 };
 
 } // namespace ZeroTier

node/Tests.cpp

@@ -1198,7 +1198,7 @@ int main(int argc,char **argv)
 	ok &= ZTT_crypto() == nullptr;
 	ok &= ZTT_benchmarkCrypto() == nullptr;
 	if (!ok) {
-		ZT_T_PRINTF(ZT_EOL_S "At least one error occurred!" ZT_EOL_S);
+		ZT_T_PRINTF(ZT_EOL_S "*** AT LEAST ONE TEST FAILED! ***" ZT_EOL_S);
 		return 1;
 	}
 	return 0;

node/Tests.h

@@ -18,20 +18,24 @@
  * To build these ensure that ZT_ENABLE_TESTS is defined during build time.
  * Otherwise they are omitted.
  *
+ * The macro ZT_STANDALONE_TESTS will also build a main() function for these
+ * tests for creating a stand-alone test executable. It will return zero if
+ * all tests pass and non-zero if at least one test fails.
+ *
  * These symbols are defined extern "C" so tests can be called from regular
  * C code, which is important for use via CGo or in plain C projects.
  *
  * The ZT_T_PRINTF macro defaults to printf() but if it's defined at compile
- * time (also must be set while building Tests.cpp) it can specify another
+ * time (it must be set while building Tests.cpp) it can specify another
  * function to use for output. Defining it to a no-op can be used to disable
  * output.
  *
  * Each test function returns NULL if the tests succeeds or an error message
  * on test failure.
  *
- * Only the fuzzing test functions are thread-safe. Other test functions
- * should not be called concurrently. It's okay to call different tests from
- * different threads, but not the same test.
+ * Be aware that fuzzing tests can and will crash the program if a serious
+ * error is discovered. This is the point. It's also beneficial to run these
+ * in "valgrind" or a similar tool to detect marginal bad behvaior.
  */
 
 #ifndef ZT_TESTS_HPP
@@ -52,20 +56,24 @@ extern "C" {
 #define ZT_T_PRINTF(fmt,...) printf((fmt),##__VA_ARGS__),fflush(stdout)
 #endif
 
-// Basic self-tests ---------------------------------------------------------------------------------------------------
-
+/**
+ * Test platform, compiler behavior, utility functions, and core classes
+ */
 const char *ZTT_general();
+
+/**
+ * Test crypto using test vectors and simple scenarios
+ *
+ * This is not an absolutely exhaustive test, just a sanity check to make sure
+ * crypto routines are basically working.
+ */
 const char *ZTT_crypto();
 
-// Benchmarks ---------------------------------------------------------------------------------------------------------
-
+/**
+ * Run benchmarks of cryptographic routines and common constructions
+ */
 const char *ZTT_benchmarkCrypto();
 
-// Fuzzing ------------------------------------------------------------------------------------------------------------
-
-
-// --------------------------------------------------------------------------------------------------------------------
-
 #ifdef __cplusplus
 }
 #endif

node/Topology.hpp

@@ -327,6 +327,7 @@ public:
 	void saveAll(void *tPtr);
 
 private:
+	// Load cached peer and set 'peer' to it, if one is found.
 	void _loadCached(void *tPtr,const Address &zta,SharedPtr<Peer> &peer);
 
 	// This is a secure random integer created at startup to salt the calculation of path hash map keys