From 2d0c5ff37a8624bc75f7b94a5409ff841c8b1df7 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 25 Jul 2017 11:41:34 -0700 Subject: [PATCH 1/3] remove preloading of allowed user keys since it happens on connect --- domain-server/src/DomainGatekeeper.cpp | 15 ++------------- domain-server/src/DomainGatekeeper.h | 2 -- domain-server/src/DomainServer.cpp | 4 +--- libraries/networking/src/NodeList.cpp | 2 +- 4 files changed, 4 insertions(+), 19 deletions(-) diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index da31fed387..957830bc7b 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -604,19 +604,6 @@ bool DomainGatekeeper::isWithinMaxCapacity() { return true; } - -void DomainGatekeeper::preloadAllowedUserPublicKeys() { - QStringList allowedUsers = _server->_settingsManager.getAllNames(); - - if (allowedUsers.size() > 0) { - // in the future we may need to limit how many requests here - for now assume that lists of allowed users are not - // going to create > 100 requests - foreach(const QString& username, allowedUsers) { - requestUserPublicKey(username); - } - } -} - void DomainGatekeeper::requestUserPublicKey(const QString& username) { // don't request public keys for the standard psuedo-account-names if (NodePermissions::standardNames.contains(username, Qt::CaseInsensitive)) { @@ -667,6 +654,8 @@ void DomainGatekeeper::publicKeyJSONCallback(QNetworkReply& requestReply) { const QString JSON_DATA_KEY = "data"; const QString JSON_PUBLIC_KEY_KEY = "public_key"; + qDebug() << "Extracted public key for" << username.toLower(); + _userPublicKeys[username.toLower()] = QByteArray::fromBase64(jsonObject[JSON_DATA_KEY].toObject()[JSON_PUBLIC_KEY_KEY].toString().toUtf8()); } diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index 163f255411..03a6292c55 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -39,8 +39,6 @@ public: const QUuid& walletUUID, const QString& nodeVersion); QUuid assignmentUUIDForPendingAssignment(const QUuid& tempUUID); - void preloadAllowedUserPublicKeys(); - void removeICEPeer(const QUuid& peerUUID) { _icePeers.remove(peerUUID); } static void sendProtocolMismatchConnectionDenial(const HifiSockAddr& senderSockAddr); diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index c5171620de..163bd48f1b 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -160,9 +160,7 @@ DomainServer::DomainServer(int argc, char* argv[]) : getTemporaryName(); } - _gatekeeper.preloadAllowedUserPublicKeys(); // so they can connect on first request - - //send signal to DomainMetadata when descriptors changed + // send signal to DomainMetadata when descriptors changed _metadata = new DomainMetadata(this); connect(&_settingsManager, &DomainServerSettingsManager::settingsUpdated, _metadata, &DomainMetadata::descriptorsChanged); diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 75c97cc205..48e5c8a62c 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -327,7 +327,7 @@ void NodeList::sendDomainServerCheckIn() { << "but no keypair is present. Waiting for keypair generation to complete."; accountManager->generateNewUserKeypair(); - // don't send the check in packet - wait for the keypair first + // don't send the check in packet - wait for the new public key to be available to the domain-server first return; } From e9845784d5a5a6012b1b10c8729f5e5e6798ae94 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 25 Jul 2017 13:43:30 -0700 Subject: [PATCH 2/3] don't return error decrypting for optimistic public keys --- domain-server/src/DomainGatekeeper.cpp | 34 +++++++++++++++++--------- domain-server/src/DomainGatekeeper.h | 15 +++++++++--- 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index 957830bc7b..fc595be67e 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -385,7 +385,7 @@ SharedNodePointer DomainGatekeeper::processAgentConnectRequest(const NodeConnect // user is attempting to prove their identity to us, but we don't have enough information sendConnectionTokenPacket(username, nodeConnection.senderSockAddr); // ask for their public key right now to make sure we have it - requestUserPublicKey(username); + requestUserPublicKey(username, true); getGroupMemberships(username); // optimistically get started on group memberships #ifdef WANT_DEBUG qDebug() << "stalling login because we have no username-signature:" << username; @@ -521,7 +521,10 @@ bool DomainGatekeeper::verifyUserSignature(const QString& username, const HifiSockAddr& senderSockAddr) { // it's possible this user can be allowed to connect, but we need to check their username signature auto lowerUsername = username.toLower(); - QByteArray publicKeyArray = _userPublicKeys.value(lowerUsername); + KeyFlagPair publicKeyPair = _userPublicKeys.value(lowerUsername); + + QByteArray publicKeyArray = publicKeyPair.first; + bool isOptimisticKey = publicKeyPair.second; const QUuid& connectionToken = _connectionTokenHash.value(lowerUsername); @@ -555,10 +558,16 @@ bool DomainGatekeeper::verifyUserSignature(const QString& username, return true; } else { - if (!senderSockAddr.isNull()) { - qDebug() << "Error decrypting username signature for " << username << "- denying connection."; + // we only send back a LoginError if this wasn't an "optimistic" key + // (a key that we hoped would work but is probably stale) + + if (!senderSockAddr.isNull() && !isOptimisticKey) { + qDebug() << "Error decrypting username signature for" << username << "- denying connection."; sendConnectionDeniedPacket("Error decrypting username signature.", senderSockAddr, DomainHandler::ConnectionRefusedReason::LoginError); + } else if (!senderSockAddr.isNull()) { + qDebug() << "Error decrypting username signature for" << username << "with optimisitic key -" + << "re-requesting public key and delaying connection"; } // free up the public key, we don't need it anymore @@ -604,7 +613,7 @@ bool DomainGatekeeper::isWithinMaxCapacity() { return true; } -void DomainGatekeeper::requestUserPublicKey(const QString& username) { +void DomainGatekeeper::requestUserPublicKey(const QString& username, bool isOptimistic) { // don't request public keys for the standard psuedo-account-names if (NodePermissions::standardNames.contains(username, Qt::CaseInsensitive)) { return; @@ -615,7 +624,7 @@ void DomainGatekeeper::requestUserPublicKey(const QString& username) { // public-key request for this username is already flight, not rerequesting return; } - _inFlightPublicKeyRequests += lowerUsername; + _inFlightPublicKeyRequests.insert(lowerUsername, isOptimistic); // even if we have a public key for them right now, request a new one in case it has just changed JSONCallbackParameters callbackParams; @@ -627,7 +636,7 @@ void DomainGatekeeper::requestUserPublicKey(const QString& username) { const QString USER_PUBLIC_KEY_PATH = "api/v1/users/%1/public_key"; - qDebug() << "Requesting public key for user" << username; + qDebug().nospace() << "Requesting " << (isOptimistic ? "optimistic " : " ") << "public key for user " << username; DependencyManager::get()->sendRequest(USER_PUBLIC_KEY_PATH.arg(username), AccountManagerAuth::None, @@ -649,18 +658,21 @@ void DomainGatekeeper::publicKeyJSONCallback(QNetworkReply& requestReply) { QJsonObject jsonObject = QJsonDocument::fromJson(requestReply.readAll()).object(); QString username = extractUsernameFromPublicKeyRequest(requestReply); + bool isOptimisticKey = _inFlightPublicKeyRequests.take(username); + if (jsonObject["status"].toString() == "success" && !username.isEmpty()) { // pull the public key as a QByteArray from this response const QString JSON_DATA_KEY = "data"; const QString JSON_PUBLIC_KEY_KEY = "public_key"; - qDebug() << "Extracted public key for" << username.toLower(); + qDebug().nospace() << "Extracted " << (isOptimisticKey ? "optimistic " : " ") << "public key for " << username.toLower(); _userPublicKeys[username.toLower()] = - QByteArray::fromBase64(jsonObject[JSON_DATA_KEY].toObject()[JSON_PUBLIC_KEY_KEY].toString().toUtf8()); + { + QByteArray::fromBase64(jsonObject[JSON_DATA_KEY].toObject()[JSON_PUBLIC_KEY_KEY].toString().toUtf8()), + isOptimisticKey + }; } - - _inFlightPublicKeyRequests.remove(username); } void DomainGatekeeper::publicKeyJSONErrorCallback(QNetworkReply& requestReply) { diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index 03a6292c55..36df77815a 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -91,7 +91,7 @@ private: void pingPunchForConnectingPeer(const SharedNetworkPeer& peer); - void requestUserPublicKey(const QString& username); + void requestUserPublicKey(const QString& username, bool isOptimistic = false); DomainServer* _server; @@ -100,8 +100,17 @@ private: QHash _icePeers; QHash _connectionTokenHash; - QHash _userPublicKeys; - QSet _inFlightPublicKeyRequests; // keep track of which we've already asked for + + // the word "optimistic" below is used for keys that we request during user connection before the user has + // had a change to upload a new public key + + // we don't send back user signature decryption errors for those keys so that there isn't a thrasing of key re-generation + // and connection refusal + + using KeyFlagPair = QPair; + + QHash _userPublicKeys; // keep track of keys and flag them as optimistic or not + QHash _inFlightPublicKeyRequests; // keep track of keys we've asked for (and if it was optimistic) QSet _domainOwnerFriends; // keep track of friends of the domain owner QSet _inFlightGroupMembershipsRequests; // keep track of which we've already asked for From dd3755596c48cf66b6f6ea5b80537959c0d56fac Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 25 Jul 2017 15:43:34 -0700 Subject: [PATCH 3/3] fix typo in comment in DomainGatekeeper header --- domain-server/src/DomainGatekeeper.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domain-server/src/DomainGatekeeper.h b/domain-server/src/DomainGatekeeper.h index 36df77815a..09db075e07 100644 --- a/domain-server/src/DomainGatekeeper.h +++ b/domain-server/src/DomainGatekeeper.h @@ -102,7 +102,7 @@ private: QHash _connectionTokenHash; // the word "optimistic" below is used for keys that we request during user connection before the user has - // had a change to upload a new public key + // had a chance to upload a new public key // we don't send back user signature decryption errors for those keys so that there isn't a thrasing of key re-generation // and connection refusal