From 86a8672c5a7a230ece2a79341299cb2dab186334 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Thu, 17 May 2018 10:17:22 -0700 Subject: [PATCH 1/6] Initialize Qt/openssl before RSA keygen to prevent collision --- libraries/networking/src/RSAKeypairGenerator.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/RSAKeypairGenerator.cpp b/libraries/networking/src/RSAKeypairGenerator.cpp index 8ca8b81ea3..dcd4e7fd3c 100644 --- a/libraries/networking/src/RSAKeypairGenerator.cpp +++ b/libraries/networking/src/RSAKeypairGenerator.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include "NetworkLogging.h" @@ -25,7 +26,8 @@ RSAKeypairGenerator::RSAKeypairGenerator(QObject* parent) : QObject(parent) { - + // Ensure openssl/Qt config is set up. + QSslConfiguration::defaultConfiguration(); } void RSAKeypairGenerator::generateKeypair() { From fe92cf0c4788bc2942253d9ade93849d344cf7b2 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Thu, 17 May 2018 13:34:38 -0700 Subject: [PATCH 2/6] Move fix up a level to bit a little more general --- libraries/networking/src/AccountManager.cpp | 3 +++ libraries/networking/src/RSAKeypairGenerator.cpp | 4 +--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/libraries/networking/src/AccountManager.cpp b/libraries/networking/src/AccountManager.cpp index 049129b2ba..1f6932094c 100644 --- a/libraries/networking/src/AccountManager.cpp +++ b/libraries/networking/src/AccountManager.cpp @@ -729,6 +729,9 @@ void AccountManager::generateNewKeypair(bool isUserKeypair, const QUuid& domainI return; } + // Ensure openssl/Qt config is set up. + QSslConfiguration::defaultConfiguration(); + // make sure we don't already have an outbound keypair generation request if (!_isWaitingForKeypairResponse) { _isWaitingForKeypairResponse = true; diff --git a/libraries/networking/src/RSAKeypairGenerator.cpp b/libraries/networking/src/RSAKeypairGenerator.cpp index dcd4e7fd3c..222b04b47c 100644 --- a/libraries/networking/src/RSAKeypairGenerator.cpp +++ b/libraries/networking/src/RSAKeypairGenerator.cpp @@ -15,7 +15,6 @@ #include #include -#include #include #include "NetworkLogging.h" @@ -26,8 +25,7 @@ RSAKeypairGenerator::RSAKeypairGenerator(QObject* parent) : QObject(parent) { - // Ensure openssl/Qt config is set up. - QSslConfiguration::defaultConfiguration(); + } void RSAKeypairGenerator::generateKeypair() { From 545ada0abb608a577362a36d0550bce1260dc83f Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 18 May 2018 10:28:52 -0700 Subject: [PATCH 3/6] Connect signal to keyGen thread for it to quit when owner destructed --- libraries/networking/src/AccountManager.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/networking/src/AccountManager.cpp b/libraries/networking/src/AccountManager.cpp index 1f6932094c..a975a86ada 100644 --- a/libraries/networking/src/AccountManager.cpp +++ b/libraries/networking/src/AccountManager.cpp @@ -761,6 +761,7 @@ void AccountManager::generateNewKeypair(bool isUserKeypair, const QUuid& domainI this, &AccountManager::handleKeypairGenerationError); connect(keypairGenerator, &QObject::destroyed, generateThread, &QThread::quit); + connect(this, &QObject::destroyed, generateThread, &QThread::quit); connect(generateThread, &QThread::finished, generateThread, &QThread::deleteLater); keypairGenerator->moveToThread(generateThread); From 66d39667e1587983129ab8b84132b756650e1c6d Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 29 May 2018 17:04:37 -0700 Subject: [PATCH 4/6] AccountManager destructor waits for any RSA key thread --- libraries/networking/src/AccountManager.cpp | 16 +++++++++++++++- libraries/networking/src/AccountManager.h | 7 +++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/AccountManager.cpp b/libraries/networking/src/AccountManager.cpp index a975a86ada..ffc73f0701 100644 --- a/libraries/networking/src/AccountManager.cpp +++ b/libraries/networking/src/AccountManager.cpp @@ -93,6 +93,13 @@ AccountManager::AccountManager(UserAgentGetter userAgentGetter) : const QString DOUBLE_SLASH_SUBSTITUTE = "slashslash"; const QString ACCOUNT_MANAGER_REQUESTED_SCOPE = "owner"; +AccountManager::~AccountManager() { + QMutexLocker lock(&_rsaKeygenLock); + while (_rsaKeygenThread) { + _rsaKeygenWait.wait(&_rsaKeygenLock); + } +} + void AccountManager::logout() { // a logout means we want to delete the DataServerAccountInfo we currently have for this URL, in-memory and in file _accountInfo = DataServerAccountInfo(); @@ -761,8 +768,9 @@ void AccountManager::generateNewKeypair(bool isUserKeypair, const QUuid& domainI this, &AccountManager::handleKeypairGenerationError); connect(keypairGenerator, &QObject::destroyed, generateThread, &QThread::quit); - connect(this, &QObject::destroyed, generateThread, &QThread::quit); + connect(generateThread, &QThread::finished, this, &AccountManager::rsaKeygenThreadFinished); connect(generateThread, &QThread::finished, generateThread, &QThread::deleteLater); + _rsaKeygenThread = generateThread; keypairGenerator->moveToThread(generateThread); @@ -870,3 +878,9 @@ void AccountManager::handleKeypairGenerationError() { sender()->deleteLater(); } + +void AccountManager::rsaKeygenThreadFinished() { + QMutexLocker lock(&_rsaKeygenLock); + _rsaKeygenThread = nullptr; + _rsaKeygenWait.wakeAll(); +} diff --git a/libraries/networking/src/AccountManager.h b/libraries/networking/src/AccountManager.h index 87b17d00d5..af89b67bdc 100644 --- a/libraries/networking/src/AccountManager.h +++ b/libraries/networking/src/AccountManager.h @@ -15,6 +15,8 @@ #include #include #include +#include +#include #include #include @@ -63,6 +65,7 @@ class AccountManager : public QObject, public Dependency { Q_OBJECT public: AccountManager(UserAgentGetter userAgentGetter = DEFAULT_USER_AGENT_GETTER); + ~AccountManager(); Q_INVOKABLE void sendRequest(const QString& path, AccountManagerAuth::Type authType, @@ -131,6 +134,7 @@ private slots: void publicKeyUploadSucceeded(QNetworkReply& reply); void publicKeyUploadFailed(QNetworkReply& reply); void generateNewKeypair(bool isUserKeypair = true, const QUuid& domainID = QUuid()); + void rsaKeygenThreadFinished(); private: AccountManager(AccountManager const& other) = delete; @@ -156,6 +160,9 @@ private: QByteArray _pendingPrivateKey; QUuid _sessionID { QUuid::createUuid() }; + QMutex _rsaKeygenLock; + QWaitCondition _rsaKeygenWait; + QThread* _rsaKeygenThread { nullptr }; }; #endif // hifi_AccountManager_h From 8fb5d1b92d8f1b1fde8319a82113c7a723396de3 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 30 May 2018 13:41:08 -0700 Subject: [PATCH 5/6] Make RSA key-pair class a Runnable that's owned by a threadpool --- libraries/networking/src/AccountManager.cpp | 134 +++++++----------- libraries/networking/src/AccountManager.h | 8 +- .../networking/src/RSAKeypairGenerator.cpp | 5 +- .../networking/src/RSAKeypairGenerator.h | 15 +- 4 files changed, 61 insertions(+), 101 deletions(-) diff --git a/libraries/networking/src/AccountManager.cpp b/libraries/networking/src/AccountManager.cpp index ffc73f0701..516484780c 100644 --- a/libraries/networking/src/AccountManager.cpp +++ b/libraries/networking/src/AccountManager.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -94,10 +95,6 @@ const QString DOUBLE_SLASH_SUBSTITUTE = "slashslash"; const QString ACCOUNT_MANAGER_REQUESTED_SCOPE = "owner"; AccountManager::~AccountManager() { - QMutexLocker lock(&_rsaKeygenLock); - while (_rsaKeygenThread) { - _rsaKeygenWait.wait(&_rsaKeygenLock); - } } void AccountManager::logout() { @@ -747,96 +744,75 @@ void AccountManager::generateNewKeypair(bool isUserKeypair, const QUuid& domainI qCDebug(networking) << "Clearing current private key in DataServerAccountInfo"; _accountInfo.setPrivateKey(QByteArray()); - // setup a new QThread to generate the keypair on, in case it takes a while - QThread* generateThread = new QThread(this); - generateThread->setObjectName("Account Manager Generator Thread"); - - // setup a keypair generator + // Create a runnable keypair generated to create an RSA pair and exit. RSAKeypairGenerator* keypairGenerator = new RSAKeypairGenerator; if (!isUserKeypair) { - keypairGenerator->setDomainID(domainID); _accountInfo.setDomainID(domainID); } - // start keypair generation when the thread starts - connect(generateThread, &QThread::started, keypairGenerator, &RSAKeypairGenerator::generateKeypair); - // handle success or failure of keypair generation - connect(keypairGenerator, &RSAKeypairGenerator::generatedKeypair, this, &AccountManager::processGeneratedKeypair); - connect(keypairGenerator, &RSAKeypairGenerator::errorGeneratingKeypair, - this, &AccountManager::handleKeypairGenerationError); - - connect(keypairGenerator, &QObject::destroyed, generateThread, &QThread::quit); - connect(generateThread, &QThread::finished, this, &AccountManager::rsaKeygenThreadFinished); - connect(generateThread, &QThread::finished, generateThread, &QThread::deleteLater); - _rsaKeygenThread = generateThread; - - keypairGenerator->moveToThread(generateThread); + connect(keypairGenerator, &RSAKeypairGenerator::generatedKeypair, this, + &AccountManager::processGeneratedKeypair); + connect(keypairGenerator, &RSAKeypairGenerator::errorGeneratingKeypair, this, + &AccountManager::handleKeypairGenerationError); qCDebug(networking) << "Starting worker thread to generate 2048-bit RSA keypair."; - generateThread->start(); + // Start on Qt's global thread pool. + QThreadPool::globalInstance()->start(keypairGenerator); } } -void AccountManager::processGeneratedKeypair() { +void AccountManager::processGeneratedKeypair(QByteArray publicKey, QByteArray privateKey) { qCDebug(networking) << "Generated 2048-bit RSA keypair. Uploading public key now."; - RSAKeypairGenerator* keypairGenerator = qobject_cast(sender()); + // hold the private key to later set our metaverse API account info if upload succeeds + _pendingPrivateKey = privateKey; - if (keypairGenerator) { - // hold the private key to later set our metaverse API account info if upload succeeds - _pendingPrivateKey = keypairGenerator->getPrivateKey(); + // upload the public key so data-web has an up-to-date key + const QString USER_PUBLIC_KEY_UPDATE_PATH = "api/v1/user/public_key"; + const QString DOMAIN_PUBLIC_KEY_UPDATE_PATH = "api/v1/domains/%1/public_key"; - // upload the public key so data-web has an up-to-date key - const QString USER_PUBLIC_KEY_UPDATE_PATH = "api/v1/user/public_key"; - const QString DOMAIN_PUBLIC_KEY_UPDATE_PATH = "api/v1/domains/%1/public_key"; - - QString uploadPath; - const auto& domainID = keypairGenerator->getDomainID(); - if (domainID.isNull()) { - uploadPath = USER_PUBLIC_KEY_UPDATE_PATH; - } else { - uploadPath = DOMAIN_PUBLIC_KEY_UPDATE_PATH.arg(uuidStringWithoutCurlyBraces(domainID)); - } - - // setup a multipart upload to send up the public key - QHttpMultiPart* requestMultiPart = new QHttpMultiPart(QHttpMultiPart::FormDataType); - - QHttpPart publicKeyPart; - publicKeyPart.setHeader(QNetworkRequest::ContentTypeHeader, QVariant("application/octet-stream")); - - publicKeyPart.setHeader(QNetworkRequest::ContentDispositionHeader, - QVariant("form-data; name=\"public_key\"; filename=\"public_key\"")); - publicKeyPart.setBody(keypairGenerator->getPublicKey()); - requestMultiPart->append(publicKeyPart); - - if (!domainID.isNull()) { - const auto& key = getTemporaryDomainKey(domainID); - QHttpPart apiKeyPart; - publicKeyPart.setHeader(QNetworkRequest::ContentTypeHeader, QVariant("application/octet-stream")); - apiKeyPart.setHeader(QNetworkRequest::ContentDispositionHeader, - QVariant("form-data; name=\"api_key\"")); - apiKeyPart.setBody(key.toUtf8()); - requestMultiPart->append(apiKeyPart); - } - - // setup callback parameters so we know once the keypair upload has succeeded or failed - JSONCallbackParameters callbackParameters; - callbackParameters.jsonCallbackReceiver = this; - callbackParameters.jsonCallbackMethod = "publicKeyUploadSucceeded"; - callbackParameters.errorCallbackReceiver = this; - callbackParameters.errorCallbackMethod = "publicKeyUploadFailed"; - - sendRequest(uploadPath, AccountManagerAuth::Optional, QNetworkAccessManager::PutOperation, - callbackParameters, QByteArray(), requestMultiPart); - - keypairGenerator->deleteLater(); + QString uploadPath; + const auto& domainID = _accountInfo.getDomainID(); + if (domainID.isNull()) { + uploadPath = USER_PUBLIC_KEY_UPDATE_PATH; } else { - qCWarning(networking) << "Expected processGeneratedKeypair to be called by a live RSAKeypairGenerator" - << "but the casted sender is NULL. Will not process generated keypair."; + uploadPath = DOMAIN_PUBLIC_KEY_UPDATE_PATH.arg(uuidStringWithoutCurlyBraces(domainID)); } + + // setup a multipart upload to send up the public key + QHttpMultiPart* requestMultiPart = new QHttpMultiPart(QHttpMultiPart::FormDataType); + + QHttpPart publicKeyPart; + publicKeyPart.setHeader(QNetworkRequest::ContentTypeHeader, QVariant("application/octet-stream")); + + publicKeyPart.setHeader(QNetworkRequest::ContentDispositionHeader, + QVariant("form-data; name=\"public_key\"; filename=\"public_key\"")); + publicKeyPart.setBody(publicKey); + requestMultiPart->append(publicKeyPart); + + // Currently broken? We don't have the temporary domain key. + if (!domainID.isNull()) { + const auto& key = getTemporaryDomainKey(domainID); + QHttpPart apiKeyPart; + publicKeyPart.setHeader(QNetworkRequest::ContentTypeHeader, QVariant("application/octet-stream")); + apiKeyPart.setHeader(QNetworkRequest::ContentDispositionHeader, + QVariant("form-data; name=\"api_key\"")); + apiKeyPart.setBody(key.toUtf8()); + requestMultiPart->append(apiKeyPart); + } + + // setup callback parameters so we know once the keypair upload has succeeded or failed + JSONCallbackParameters callbackParameters; + callbackParameters.jsonCallbackReceiver = this; + callbackParameters.jsonCallbackMethod = "publicKeyUploadSucceeded"; + callbackParameters.errorCallbackReceiver = this; + callbackParameters.errorCallbackMethod = "publicKeyUploadFailed"; + + sendRequest(uploadPath, AccountManagerAuth::Optional, QNetworkAccessManager::PutOperation, + callbackParameters, QByteArray(), requestMultiPart); } void AccountManager::publicKeyUploadSucceeded(QNetworkReply& reply) { @@ -875,12 +851,4 @@ void AccountManager::handleKeypairGenerationError() { // reset our waiting state for keypair response _isWaitingForKeypairResponse = false; - - sender()->deleteLater(); -} - -void AccountManager::rsaKeygenThreadFinished() { - QMutexLocker lock(&_rsaKeygenLock); - _rsaKeygenThread = nullptr; - _rsaKeygenWait.wakeAll(); } diff --git a/libraries/networking/src/AccountManager.h b/libraries/networking/src/AccountManager.h index af89b67bdc..a24acb35f5 100644 --- a/libraries/networking/src/AccountManager.h +++ b/libraries/networking/src/AccountManager.h @@ -15,8 +15,6 @@ #include #include #include -#include -#include #include #include @@ -130,11 +128,10 @@ signals: private slots: void processReply(); void handleKeypairGenerationError(); - void processGeneratedKeypair(); + void processGeneratedKeypair(QByteArray publicKey, QByteArray privateKey); void publicKeyUploadSucceeded(QNetworkReply& reply); void publicKeyUploadFailed(QNetworkReply& reply); void generateNewKeypair(bool isUserKeypair = true, const QUuid& domainID = QUuid()); - void rsaKeygenThreadFinished(); private: AccountManager(AccountManager const& other) = delete; @@ -160,9 +157,6 @@ private: QByteArray _pendingPrivateKey; QUuid _sessionID { QUuid::createUuid() }; - QMutex _rsaKeygenLock; - QWaitCondition _rsaKeygenWait; - QThread* _rsaKeygenThread { nullptr }; }; #endif // hifi_AccountManager_h diff --git a/libraries/networking/src/RSAKeypairGenerator.cpp b/libraries/networking/src/RSAKeypairGenerator.cpp index 222b04b47c..e83615e3df 100644 --- a/libraries/networking/src/RSAKeypairGenerator.cpp +++ b/libraries/networking/src/RSAKeypairGenerator.cpp @@ -25,7 +25,10 @@ RSAKeypairGenerator::RSAKeypairGenerator(QObject* parent) : QObject(parent) { +} +void RSAKeypairGenerator::run() { + generateKeypair(); } void RSAKeypairGenerator::generateKeypair() { @@ -92,5 +95,5 @@ void RSAKeypairGenerator::generateKeypair() { OPENSSL_free(publicKeyDER); OPENSSL_free(privateKeyDER); - emit generatedKeypair(); + emit generatedKeypair(_publicKey, _privateKey); } diff --git a/libraries/networking/src/RSAKeypairGenerator.h b/libraries/networking/src/RSAKeypairGenerator.h index 36f4a9550b..552f12395b 100644 --- a/libraries/networking/src/RSAKeypairGenerator.h +++ b/libraries/networking/src/RSAKeypairGenerator.h @@ -13,25 +13,20 @@ #define hifi_RSAKeypairGenerator_h #include +#include #include -class RSAKeypairGenerator : public QObject { +class RSAKeypairGenerator : public QObject, public QRunnable { Q_OBJECT public: - RSAKeypairGenerator(QObject* parent = 0); + RSAKeypairGenerator(QObject* parent = nullptr); - void setDomainID(const QUuid& domainID) { _domainID = domainID; } - const QUuid& getDomainID() const { return _domainID; } - - const QByteArray& getPublicKey() const { return _publicKey; } - const QByteArray& getPrivateKey() const { return _privateKey; } - -public slots: + virtual void run() override; void generateKeypair(); signals: void errorGeneratingKeypair(); - void generatedKeypair(); + void generatedKeypair(QByteArray publicKey, QByteArray privateKey); private: QUuid _domainID; From a3324c5616268d9b6fccae47b228d47d0f9b1257 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 30 May 2018 13:52:26 -0700 Subject: [PATCH 6/6] Remove empty destructor --- libraries/networking/src/AccountManager.cpp | 3 --- libraries/networking/src/AccountManager.h | 1 - 2 files changed, 4 deletions(-) diff --git a/libraries/networking/src/AccountManager.cpp b/libraries/networking/src/AccountManager.cpp index 516484780c..1830560130 100644 --- a/libraries/networking/src/AccountManager.cpp +++ b/libraries/networking/src/AccountManager.cpp @@ -94,9 +94,6 @@ AccountManager::AccountManager(UserAgentGetter userAgentGetter) : const QString DOUBLE_SLASH_SUBSTITUTE = "slashslash"; const QString ACCOUNT_MANAGER_REQUESTED_SCOPE = "owner"; -AccountManager::~AccountManager() { -} - void AccountManager::logout() { // a logout means we want to delete the DataServerAccountInfo we currently have for this URL, in-memory and in file _accountInfo = DataServerAccountInfo(); diff --git a/libraries/networking/src/AccountManager.h b/libraries/networking/src/AccountManager.h index a24acb35f5..3ff657b72a 100644 --- a/libraries/networking/src/AccountManager.h +++ b/libraries/networking/src/AccountManager.h @@ -63,7 +63,6 @@ class AccountManager : public QObject, public Dependency { Q_OBJECT public: AccountManager(UserAgentGetter userAgentGetter = DEFAULT_USER_AGENT_GETTER); - ~AccountManager(); Q_INVOKABLE void sendRequest(const QString& path, AccountManagerAuth::Type authType,