Merge pull request #7490 from birarda/udt-fixes

cleanup unused public keys in ice-server

ice-server/src/IceServer.cpp

@@ -11,7 +11,6 @@
 
 #include "IceServer.h"
 
-#include <openssl/rsa.h>
 #include <openssl/x509.h>
 
 #include <QtCore/QJsonDocument>
@@ -68,7 +67,9 @@ bool IceServer::packetVersionMatch(const udt::Packet& packet) {
 }
 
 void IceServer::processPacket(std::unique_ptr<udt::Packet> packet) {
-    
+
+    _lastPacketTimestamp = QDateTime::currentMSecsSinceEpoch();
+
     auto nlPacket = NLPacket::fromBase(std::move(packet));
     
     // make sure that this packet at least looks like something we can read
@@ -161,15 +162,12 @@ SharedNetworkPeer IceServer::addOrUpdateHeartbeatingPeer(NLPacket& packet) {
 }
 
 bool IceServer::isVerifiedHeartbeat(const QUuid& domainID, const QByteArray& plaintext, const QByteArray& signature) {
-    // check if we have a private key for this domain ID - if we do not then fire off the request for it
+    // check if we have a public key for this domain ID - if we do not then fire off the request for it
     auto it = _domainPublicKeys.find(domainID);
     if (it != _domainPublicKeys.end()) {
 
         // attempt to verify the signature for this heartbeat
-        const unsigned char* publicKeyData = reinterpret_cast<const unsigned char*>(it->second.constData());
+        const auto rsaPublicKey = it->second.get();
-
-        // first load up the public key into an RSA struct
-        RSA* rsaPublicKey = d2i_RSA_PUBKEY(NULL, &publicKeyData, it->second.size());
 
         if (rsaPublicKey) {
             auto hashedPlaintext = QCryptographicHash::hash(plaintext, QCryptographicHash::Sha256);
@@ -180,9 +178,6 @@ bool IceServer::isVerifiedHeartbeat(const QUuid& domainID, const QByteArray& pla
                                                 signature.size(),
                                                 rsaPublicKey);
 
-            // free up the public key and remove connection token before we return
-            RSA_free(rsaPublicKey);
-
             if (verificationResult == 1) {
                 // this is the only success case - we return true here to indicate that the heartbeat is verified
                 return true;
@@ -192,7 +187,7 @@ bool IceServer::isVerifiedHeartbeat(const QUuid& domainID, const QByteArray& pla
 
         } else {
             // we can't let this user in since we couldn't convert their public key to an RSA key we could use
-            qWarning() << "Could not convert in-memory public key for" << domainID << "to usable RSA public key.";
+            qWarning() << "Public key for" << domainID << "is not a usable RSA* public key.";
             qWarning() << "Re-requesting public key from API";
         }
     }
@@ -240,7 +235,22 @@ void IceServer::publicKeyReplyFinished(QNetworkReply* reply) {
         if (responseObject[STATUS_KEY].toString() == SUCCESS_VALUE) {
             auto dataObject = responseObject[DATA_KEY].toObject();
             if (dataObject.contains(PUBLIC_KEY_KEY)) {
-                _domainPublicKeys[domainID] = QByteArray::fromBase64(dataObject[PUBLIC_KEY_KEY].toString().toUtf8());
+
+                // grab the base 64 public key from the API response
+                auto apiPublicKey = QByteArray::fromBase64(dataObject[PUBLIC_KEY_KEY].toString().toUtf8());
+
+                // convert the downloaded public key to an RSA struct, if possible
+                const unsigned char* publicKeyData = reinterpret_cast<const unsigned char*>(apiPublicKey.constData());
+
+                RSA* rsaPublicKey = d2i_RSA_PUBKEY(NULL, &publicKeyData, apiPublicKey.size());
+
+                if (rsaPublicKey) {
+                    _domainPublicKeys[domainID] = { rsaPublicKey, RSA_free };
+                } else {
+                    qWarning() << "Could not convert in-memory public key for" << domainID << "to usable RSA public key.";
+                    qWarning() << "Public key will be re-requested on next heartbeat.";
+                }
+
             } else {
                 qWarning() << "There was no public key present in response for domain with ID" << domainID;
             }
@@ -254,6 +264,8 @@ void IceServer::publicKeyReplyFinished(QNetworkReply* reply) {
 
         qWarning() << "Error retreiving public key for domain with ID" << domainID << "-" <<  reply->errorString();
     }
+
+    reply->deleteLater();
 }
 
 void IceServer::sendPeerInformationPacket(const NetworkPeer& peer, const HifiSockAddr* destinationSockAddr) {
@@ -274,6 +286,11 @@ void IceServer::clearInactivePeers() {
 
         if ((usecTimestampNow() - peer->getLastHeardMicrostamp()) > (PEER_SILENCE_THRESHOLD_MSECS * 1000)) {
             qDebug() << "Removing peer from memory for inactivity -" << *peer;
+
+            // if we had a public key for this domain, remove it now
+            _domainPublicKeys.erase(peer->getUUID());
+
+            // remove the peer object
             peerItem = _activePeers.erase(peerItem);
         } else {
             // we didn't kill this peer, push the iterator forwards
@@ -288,7 +305,14 @@ bool IceServer::handleHTTPRequest(HTTPConnection* connection, const QUrl& url, b
 
     if (connection->requestOperation() == QNetworkAccessManager::GetOperation) {
         if (url.path() == "/status") {
-            connection->respond(HTTPConnection::StatusCode200, QByteArray::number(_activePeers.size()));
+            // figure out if we respond with 0 (we're good) or 1 (we think we're in trouble)
+
+            const quint64 MAX_PACKET_GAP_MS_FOR_STUCK_SOCKET = 10 * 1000;
+
+            int statusNumber = (QDateTime::currentMSecsSinceEpoch() - _lastPacketTimestamp > MAX_PACKET_GAP_MS_FOR_STUCK_SOCKET)
+                                ? 1 : 0;
+
+            connection->respond(HTTPConnection::StatusCode200, QByteArray::number(statusNumber));
         }
     }
     return true;

ice-server/src/IceServer.h

@@ -16,6 +16,8 @@
 #include <QtCore/QSharedPointer>
 #include <QUdpSocket>
 
+#include <openssl/rsa.h>
+
 #include <UUIDHasher.h>
 
 #include <NetworkPeer.h>
@@ -52,8 +54,11 @@ private:
 
     HTTPManager _httpManager;
 
-    using DomainPublicKeyHash = std::unordered_map<QUuid, QByteArray>;
+    using RSAUniquePtr = std::unique_ptr<RSA, std::function<void(RSA*)>>;
+    using DomainPublicKeyHash = std::unordered_map<QUuid, RSAUniquePtr>;
     DomainPublicKeyHash _domainPublicKeys;
+
+    quint64 _lastPacketTimestamp;
 };
 
 #endif // hifi_IceServer_h

libraries/networking/src/udt/PacketQueue.cpp

@@ -51,7 +51,7 @@ PacketQueue::PacketPointer PacketQueue::takePacket() {
         --_currentIndex;
     }
     
-    return std::move(packet);
+    return packet;
 }
 
 unsigned int PacketQueue::nextIndex() {