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.

This commit is contained in:
Adam Ierymenko 2015-07-07 08:14:41 -07:00
parent 56285ec0d4
commit f398952a6c
4 changed files with 34 additions and 38 deletions

View File

@ -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

View File

@ -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)

View File

@ -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

View File

@ -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,