Revert some bad docs in Packet -- I think we will still use that. Also rename addMembershipCertificate to more security-descriptive validateAndAddMembershipCertificate, give it a return value, and drop unused force parameter.

4 files changed, 34 insertions, 38 deletions

diff --git a/node/IncomingPacket.cpp b/node/IncomingPacket.cpp
index ab771b7e..80159194 100644
--- a/node/IncomingPacket.cpp
+++ b/node/IncomingPacket.cpp
@@ -429,7 +429,7 @@ bool IncomingPacket::_doOK(const RuntimeEnvironment *RR,const SharedPtr<Peer> &p
 					offset += com.deserialize(*this,ZT_PROTO_VERB_MULTICAST_FRAME__OK__IDX_COM_AND_GATHER_RESULTS);
 					SharedPtr<Network> network(RR->node->network(nwid));
 					if ((network)&&(com.hasRequiredFields()))
-						network->addMembershipCertificate(com,false);
+						network->validateAndAddMembershipCertificate(com);
 				}
 
 				if ((flags & 0x02) != 0) {
@@ -558,7 +558,7 @@ bool IncomingPacket::_doEXT_FRAME(const RuntimeEnvironment *RR,const SharedPtr<P
 					CertificateOfMembership com;
 					comLen = com.deserialize(*this,ZT_PROTO_VERB_EXT_FRAME_IDX_COM);
 					if (com.hasRequiredFields())
-						network->addMembershipCertificate(com,false);
+						network->validateAndAddMembershipCertificate(com);
 				}
 
 				if (!network->isAllowed(peer->address())) {
@@ -648,7 +648,7 @@ bool IncomingPacket::_doNETWORK_MEMBERSHIP_CERTIFICATE(const RuntimeEnvironment
 			if (com.hasRequiredFields()) {
 				SharedPtr<Network> network(RR->node->network(com.networkId()));
 				if (network)
-					network->addMembershipCertificate(com,false);
+					network->validateAndAddMembershipCertificate(com);
 			}
 		}
 
@@ -806,7 +806,7 @@ bool IncomingPacket::_doMULTICAST_FRAME(const RuntimeEnvironment *RR,const Share
 				CertificateOfMembership com;
 				offset += com.deserialize(*this,ZT_PROTO_VERB_MULTICAST_FRAME_IDX_COM);
 				if (com.hasRequiredFields())
-					network->addMembershipCertificate(com,false);
+					network->validateAndAddMembershipCertificate(com);
 			}
 
 			// Check membership after we've read any included COM, since
diff --git a/node/Network.cpp b/node/Network.cpp
index 4414e4d1..84b8d320 100644
--- a/node/Network.cpp
+++ b/node/Network.cpp
@@ -267,53 +267,55 @@ void Network::requestConfiguration()
 	RR->sw->send(outp,true,_id);
 }
 
-void Network::addMembershipCertificate(const CertificateOfMembership &cert,bool forceAccept)
+bool Network::validateAndAddMembershipCertificate(const CertificateOfMembership &cert)
 {
 	if (!cert) // sanity check
-		return;
+		return false;
 
 	Mutex::Lock _l(_lock);
 	CertificateOfMembership &old = _membershipCertificates[cert.issuedTo()];
 
 	// Nothing to do if the cert hasn't changed -- we get duplicates due to zealous cert pushing
 	if (old == cert)
-		return;
+		return true; // but if it's a duplicate of one we already accepted, return is 'true'
 
 	// Check signature, log and return if cert is invalid
-	if (!forceAccept) {
-		if (cert.signedBy() != controller()) {
-			TRACE("rejected network membership certificate for %.16llx signed by %s: signer not a controller of this network",(unsigned long long)_id,cert.signedBy().toString().c_str());
-			return;
+	if (cert.signedBy() != controller()) {
+		TRACE("rejected network membership certificate for %.16llx signed by %s: signer not a controller of this network",(unsigned long long)_id,cert.signedBy().toString().c_str());
+		return false; // invalid signer
+	}
+
+	if (cert.signedBy() == RR->identity.address()) {
+
+		// We are the controller: RR->identity.address() == controller() == cert.signedBy()
+		// So, verify that we signed th cert ourself
+		if (!cert.verify(RR->identity)) {
+			TRACE("rejected network membership certificate for %.16llx self signed by %s: signature check failed",(unsigned long long)_id,cert.signedBy().toString().c_str());
+			return false; // invalid signature
 		}
 
-		if (cert.signedBy() == RR->identity.address()) {
-			// We are the controller: RR->identity.address() == controller() == cert.signedBy()
-			// So, verify that we signed th cert ourself
-			if (!cert.verify(RR->identity)) {
-				TRACE("rejected network membership certificate for %.16llx self signed by %s: signature check failed",(unsigned long long)_id,cert.signedBy().toString().c_str());
-				return;
-			}
-		} else {
+	} else {
 
-			SharedPtr<Peer> signer(RR->topology->getPeer(cert.signedBy()));
+		SharedPtr<Peer> signer(RR->topology->getPeer(cert.signedBy()));
 
-			if (!signer) {
-				// This would be rather odd, since this is our controller... could happen
-				// if we get packets before we've gotten config.
-				RR->sw->requestWhois(cert.signedBy());
-				return;
-			}
+		if (!signer) {
+			// This would be rather odd, since this is our controller... could happen
+			// if we get packets before we've gotten config.
+			RR->sw->requestWhois(cert.signedBy());
+			return false; // signer unknown
+		}
 
-			if (!cert.verify(signer->identity())) {
-				TRACE("rejected network membership certificate for %.16llx signed by %s: signature check failed",(unsigned long long)_id,cert.signedBy().toString().c_str());
-				return;
-			}
+		if (!cert.verify(signer->identity())) {
+			TRACE("rejected network membership certificate for %.16llx signed by %s: signature check failed",(unsigned long long)_id,cert.signedBy().toString().c_str());
+			return false; // invalid signature
 		}
 	}
 
 	// If we made it past authentication, update cert
 	if (cert.revision() != old.revision())
 		old = cert;
+
+	return true;
 }
 
 bool Network::peerNeedsOurMembershipCertificate(const Address &to,uint64_t now)
diff --git a/node/Network.hpp b/node/Network.hpp
index daa4554e..53a22dcd 100644
--- a/node/Network.hpp
+++ b/node/Network.hpp
@@ -179,9 +179,9 @@ public:
 	 * Add or update a membership certificate
 	 *
 	 * @param cert Certificate of membership
-	 * @param forceAccept If true, accept without validating signature
+	 * @return True if certificate was accepted as valid
 	 */
-	void addMembershipCertificate(const CertificateOfMembership &cert,bool forceAccept);
+	bool validateAndAddMembershipCertificate(const CertificateOfMembership &cert);
 
 	/**
 	 * Check if we should push membership certificate to a peer, and update last pushed
diff --git a/node/Packet.hpp b/node/Packet.hpp
index 50471b06..51a241ba 100644
--- a/node/Packet.hpp
+++ b/node/Packet.hpp
@@ -680,12 +680,6 @@ public:
 		 *
 		 * Certificate contains network ID, peer it was issued for, etc.
 		 *
-		 * Note that in the current code base, separate COM pushes are not done.
-		 * Instead the "bundled COM" options are utilized in EXT_FRAME and
-		 * MULTICAST_FRAME to push the COM along with frames. This is slightly
-		 * more efficient. But we'll keep this simple message around in case we
-		 * want to use it in the future.
-		 *
 		 * OK/ERROR are not generated.
 		 */
 		VERB_NETWORK_MEMBERSHIP_CERTIFICATE = 10,