From 3e4b4a0fc7ff74221f4b464553243c9163ab4c8f Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Mon, 28 Mar 2016 10:03:05 -0700 Subject: [PATCH 1/3] cleanup un-used public keys in ice-server --- ice-server/src/IceServer.cpp | 37 ++++++++++++++------ ice-server/src/IceServer.h | 5 ++- libraries/networking/src/udt/PacketQueue.cpp | 2 +- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/ice-server/src/IceServer.cpp b/ice-server/src/IceServer.cpp index f38923b873..f9d244befa 100644 --- a/ice-server/src/IceServer.cpp +++ b/ice-server/src/IceServer.cpp @@ -11,7 +11,6 @@ #include "IceServer.h" -#include #include #include @@ -161,15 +160,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(it->second.constData()); - - // first load up the public key into an RSA struct - RSA* rsaPublicKey = d2i_RSA_PUBKEY(NULL, &publicKeyData, it->second.size()); + const auto rsaPublicKey = it->second.get(); if (rsaPublicKey) { auto hashedPlaintext = QCryptographicHash::hash(plaintext, QCryptographicHash::Sha256); @@ -180,9 +176,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 +185,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 +233,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(apiPublicKey.constData()); + + RSA* rsaPublicKey = d2i_RSA_PUBKEY(NULL, &publicKeyData, apiPublicKey.size()); + + if (rsaPublicKey) { + _domainPublicKeys[domainID] = { rsaPublicKey, [](RSA* rsa) { RSA_free(rsa); }}; + } 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 +262,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 +284,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 diff --git a/ice-server/src/IceServer.h b/ice-server/src/IceServer.h index 81234b2c3c..ebf1a9ac52 100644 --- a/ice-server/src/IceServer.h +++ b/ice-server/src/IceServer.h @@ -16,6 +16,8 @@ #include #include +#include + #include #include @@ -52,7 +54,8 @@ private: HTTPManager _httpManager; - using DomainPublicKeyHash = std::unordered_map; + using RSAUniquePtr = std::unique_ptr>; + using DomainPublicKeyHash = std::unordered_map; DomainPublicKeyHash _domainPublicKeys; }; diff --git a/libraries/networking/src/udt/PacketQueue.cpp b/libraries/networking/src/udt/PacketQueue.cpp index 8ff2333a20..4b8c0b187c 100644 --- a/libraries/networking/src/udt/PacketQueue.cpp +++ b/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() { From cffeece08e538ad094dc44bc90b430b01481ef69 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Mon, 28 Mar 2016 10:51:15 -0700 Subject: [PATCH 2/3] use time since last packet as short-term status check --- ice-server/src/IceServer.cpp | 13 +++++++++++-- ice-server/src/IceServer.h | 2 ++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/ice-server/src/IceServer.cpp b/ice-server/src/IceServer.cpp index f9d244befa..86b400fe81 100644 --- a/ice-server/src/IceServer.cpp +++ b/ice-server/src/IceServer.cpp @@ -67,7 +67,9 @@ bool IceServer::packetVersionMatch(const udt::Packet& packet) { } void IceServer::processPacket(std::unique_ptr packet) { - + + _lastPacketTimestamp = QDateTime::currentMSecsSinceEpoch(); + auto nlPacket = NLPacket::fromBase(std::move(packet)); // make sure that this packet at least looks like something we can read @@ -303,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; diff --git a/ice-server/src/IceServer.h b/ice-server/src/IceServer.h index ebf1a9ac52..6cc33fd8fc 100644 --- a/ice-server/src/IceServer.h +++ b/ice-server/src/IceServer.h @@ -57,6 +57,8 @@ private: using RSAUniquePtr = std::unique_ptr>; using DomainPublicKeyHash = std::unordered_map; DomainPublicKeyHash _domainPublicKeys; + + quint64 _lastPacketTimestamp; }; #endif // hifi_IceServer_h From 869529a43519ebacbc8c3625e6de70000986a46f Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Mon, 28 Mar 2016 10:51:47 -0700 Subject: [PATCH 3/3] register RSA_free directly as custom deleter --- ice-server/src/IceServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ice-server/src/IceServer.cpp b/ice-server/src/IceServer.cpp index 86b400fe81..d7fba12f26 100644 --- a/ice-server/src/IceServer.cpp +++ b/ice-server/src/IceServer.cpp @@ -245,7 +245,7 @@ void IceServer::publicKeyReplyFinished(QNetworkReply* reply) { RSA* rsaPublicKey = d2i_RSA_PUBKEY(NULL, &publicKeyData, apiPublicKey.size()); if (rsaPublicKey) { - _domainPublicKeys[domainID] = { rsaPublicKey, [](RSA* rsa) { RSA_free(rsa); }}; + _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.";