Merge PR12964 to not use verification if not keyed

https://github.com/highfidelity/hifi/pull/12964

libraries/networking/src/HMACAuth.cpp

@@ -9,12 +9,14 @@
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
 //
 
+#include "HMACAuth.h"
+
 #include <openssl/opensslv.h>
 #include <openssl/hmac.h>
 
-#include "HMACAuth.h"
-
 #include <QUuid>
+#include "NetworkLogging.h"
+#include <cassert>
 
 #if OPENSSL_VERSION_NUMBER >= 0x10100000
 HMACAuth::HMACAuth(AuthMethod authMethod)
@@ -86,9 +88,17 @@ HMACAuth::HMACHash HMACAuth::result() {
     HMACHash hashValue(EVP_MAX_MD_SIZE);
     unsigned int hashLen;
     QMutexLocker lock(&_lock);
-    if (HMAC_Final(_hmacContext, &hashValue[0], &hashLen)) {
+    
+    auto hmacResult = HMAC_Final(_hmacContext, &hashValue[0], &hashLen);
+    
+    if (hmacResult) {
         hashValue.resize((size_t)hashLen);
+    } else {
+        // the HMAC_FINAL call failed - should not be possible to get into this state
+        qCWarning(networking) << "Error occured calling HMAC_Final";
+        assert(hmacResult);
     }
+
     // Clear state for possible reuse.
     HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr);
     return hashValue;
@@ -97,6 +107,8 @@ HMACAuth::HMACHash HMACAuth::result() {
 bool HMACAuth::calculateHash(HMACHash& hashResult, const char* data, int dataLen) {
     QMutexLocker lock(&_lock);
     if (!HMAC_Update(_hmacContext, reinterpret_cast<const unsigned char*>(data), dataLen)) {
+        qCWarning(networking) << "Error occured calling HMAC_Update";
+        assert(false);
         return false;
     }
 
@@ -108,5 +120,7 @@ bool HMACAuth::calculateHash(HMACHash& hashResult, const char* data, int dataLen
         HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr);
         return true;
     }
+    qCWarning(networking) << "Error occured calling HMAC_Final";
+    assert(false);
     return false;
 }

libraries/networking/src/LimitedNodeList.cpp

@@ -331,10 +331,14 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe
             if (verifiedPacket && !ignoreVerification) {
 
                 QByteArray packetHeaderHash = NLPacket::verificationHashInHeader(packet);
-                QByteArray expectedHash = NLPacket::hashForPacketAndHMAC(packet, sourceNode->getAuthenticateHash());
+                QByteArray expectedHash;
+                auto sourceNodeHMACAuth = sourceNode->getAuthenticateHash();
+                if (sourceNode->getAuthenticateHash()) {
+                    expectedHash = NLPacket::hashForPacketAndHMAC(packet, *sourceNodeHMACAuth);
+                }
 
                 // check if the HMAC-md5 hash in the header matches the hash we would expect
-                if (packetHeaderHash != expectedHash) {
+                if (!sourceNodeHMACAuth || packetHeaderHash != expectedHash) {
                     static QMultiMap<QUuid, PacketType> hashDebugSuppressMap;
 
                     if (!hashDebugSuppressMap.contains(sourceID, headerType)) {
@@ -396,7 +400,7 @@ qint64 LimitedNodeList::sendUnreliablePacket(const NLPacket& packet, const Node&
     emit dataSent(destinationNode.getType(), packet.getDataSize());
     destinationNode.recordBytesSent(packet.getDataSize());
 
-    return sendUnreliablePacket(packet, *destinationNode.getActiveSocket(), &destinationNode.getAuthenticateHash());
+    return sendUnreliablePacket(packet, *destinationNode.getActiveSocket(), destinationNode.getAuthenticateHash());
 }
 
 qint64 LimitedNodeList::sendUnreliablePacket(const NLPacket& packet, const HifiSockAddr& sockAddr,
@@ -419,7 +423,7 @@ qint64 LimitedNodeList::sendPacket(std::unique_ptr<NLPacket> packet, const Node&
         emit dataSent(destinationNode.getType(), packet->getDataSize());
         destinationNode.recordBytesSent(packet->getDataSize());
 
-        return sendPacket(std::move(packet), *activeSocket, &destinationNode.getAuthenticateHash());
+        return sendPacket(std::move(packet), *activeSocket, destinationNode.getAuthenticateHash());
     } else {
         qCDebug(networking) << "LimitedNodeList::sendPacket called without active socket for node" << destinationNode << "- not sending";
         return ERROR_SENDING_PACKET_BYTES;
@@ -447,14 +451,14 @@ qint64 LimitedNodeList::sendUnreliableUnorderedPacketList(NLPacketList& packetLi
 
     if (activeSocket) {
         qint64 bytesSent = 0;
-        auto& connectionHash = destinationNode.getAuthenticateHash();
+        auto connectionHash = destinationNode.getAuthenticateHash();
 
         // close the last packet in the list
         packetList.closeCurrentPacket();
 
         while (!packetList._packets.empty()) {
             bytesSent += sendPacket(packetList.takeFront<NLPacket>(), *activeSocket,
-                &connectionHash);
+                connectionHash);
         }
 
         emit dataSent(destinationNode.getType(), bytesSent);
@@ -502,7 +506,7 @@ qint64 LimitedNodeList::sendPacketList(std::unique_ptr<NLPacketList> packetList,
         for (std::unique_ptr<udt::Packet>& packet : packetList->_packets) {
             NLPacket* nlPacket = static_cast<NLPacket*>(packet.get());
             collectPacketStats(*nlPacket);
-            fillPacketHeader(*nlPacket, &destinationNode.getAuthenticateHash());
+            fillPacketHeader(*nlPacket, destinationNode.getAuthenticateHash());
         }
 
         return _nodeSocket.writePacketList(std::move(packetList), *activeSocket);
@@ -525,7 +529,7 @@ qint64 LimitedNodeList::sendPacket(std::unique_ptr<NLPacket> packet, const Node&
     auto& destinationSockAddr = (overridenSockAddr.isNull()) ? *destinationNode.getActiveSocket()
                                                              : overridenSockAddr;
 
-    return sendPacket(std::move(packet), destinationSockAddr, &destinationNode.getAuthenticateHash());
+    return sendPacket(std::move(packet), destinationSockAddr, destinationNode.getAuthenticateHash());
 }
 
 int LimitedNodeList::updateNodeWithDataFromPacket(QSharedPointer<ReceivedMessage> message, SharedNodePointer sendingNode) {

libraries/networking/src/Node.cpp

@@ -89,7 +89,7 @@ Node::Node(const QUuid& uuid, NodeType_t type, const HifiSockAddr& publicSocket,
     const HifiSockAddr& localSocket, QObject* parent) :
     NetworkPeer(uuid, publicSocket, localSocket, parent),
     _type(type),
-    _authenticateHash(new HMACAuth), _pingMs(-1),  // "Uninitialized"
+    _pingMs(-1),  // "Uninitialized"
     _clockSkewUsec(0),
     _mutex(),
     _clockSkewMovingPercentile(30, 0.8f)   // moving 80th percentile of 30 samples
@@ -201,6 +201,10 @@ void Node::setConnectionSecret(const QUuid& connectionSecret) {
         return;
     }
 
+    if (!_authenticateHash) {
+        _authenticateHash.reset(new HMACAuth());
+    }
+
     _connectionSecret = connectionSecret;
     _authenticateHash->setKey(_connectionSecret);
 }

libraries/networking/src/Node.h

@@ -57,7 +57,7 @@ public:
 
     const QUuid& getConnectionSecret() const { return _connectionSecret; }
     void setConnectionSecret(const QUuid& connectionSecret);
-    HMACAuth& getAuthenticateHash() const { return *_authenticateHash; }
+    HMACAuth* getAuthenticateHash() const { return _authenticateHash.get(); }
 
     NodeData* getLinkedData() const { return _linkedData.get(); }
     void setLinkedData(std::unique_ptr<NodeData> linkedData) { _linkedData = std::move(linkedData); }
@@ -99,7 +99,7 @@ private:
     NodeType_t _type;
 
     QUuid _connectionSecret;
-    std::unique_ptr<HMACAuth> _authenticateHash;
+    std::unique_ptr<HMACAuth> _authenticateHash { nullptr };
     std::unique_ptr<NodeData> _linkedData;
     bool _isReplicated { false };
     int _pingMs;