From ee6a48b3dc088b2c4e1580e7b42196d922aa7d61 Mon Sep 17 00:00:00 2001 From: Christian Kamm Date: Tue, 19 Feb 2019 11:38:46 +0100 Subject: [PATCH] Client certs: Store pkcs12 in config, password in keychain It still reads and writes the old format too, but all newly stored client certs will be in the new form. For #6776 because Windows limits credential data to 512 bytes in older versions. --- src/gui/addcertificatedialog.ui | 44 +++++++- src/gui/creds/httpcredentialsgui.h | 9 +- src/gui/wizard/owncloudhttpcredspage.cpp | 2 +- src/gui/wizard/owncloudoauthcredspage.cpp | 2 +- src/gui/wizard/owncloudsetuppage.cpp | 21 ++-- src/gui/wizard/owncloudwizard.h | 6 +- src/libsync/account.cpp | 6 +- src/libsync/creds/httpcredentials.cpp | 122 ++++++++++++++++++++-- src/libsync/creds/httpcredentials.h | 27 ++++- 9 files changed, 204 insertions(+), 35 deletions(-) diff --git a/src/gui/addcertificatedialog.ui b/src/gui/addcertificatedialog.ui index cf1684254..06789745d 100644 --- a/src/gui/addcertificatedialog.ui +++ b/src/gui/addcertificatedialog.ui @@ -9,8 +9,8 @@ 0 0 - 462 - 188 + 478 + 225 @@ -73,11 +73,30 @@ + + + + An encrypted pkcs12 bundle is strongly recommended as a copy will be stored in the configuration file. + + + true + + + + + + + 255 + 0 + 0 + + + @@ -89,6 +108,15 @@ + + + + 255 + 0 + 0 + + + @@ -100,6 +128,15 @@ + + + + 119 + 120 + 120 + + + @@ -112,9 +149,6 @@ - - - true diff --git a/src/gui/creds/httpcredentialsgui.h b/src/gui/creds/httpcredentialsgui.h index 9f4e055a4..fd4161dbb 100644 --- a/src/gui/creds/httpcredentialsgui.h +++ b/src/gui/creds/httpcredentialsgui.h @@ -33,13 +33,14 @@ public: : HttpCredentials() { } - HttpCredentialsGui(const QString &user, const QString &password, const QSslCertificate &certificate, const QSslKey &key) - : HttpCredentials(user, password, certificate, key) + HttpCredentialsGui(const QString &user, const QString &password, + const QByteArray &clientCertBundle, const QByteArray &clientCertPassword) + : HttpCredentials(user, password, clientCertBundle, clientCertPassword) { } HttpCredentialsGui(const QString &user, const QString &password, const QString &refreshToken, - const QSslCertificate &certificate, const QSslKey &key) - : HttpCredentials(user, password, certificate, key) + const QByteArray &clientCertBundle, const QByteArray &clientCertPassword) + : HttpCredentials(user, password, clientCertBundle, clientCertPassword) { _refreshToken = refreshToken; } diff --git a/src/gui/wizard/owncloudhttpcredspage.cpp b/src/gui/wizard/owncloudhttpcredspage.cpp index 02a82c459..cab2a7427 100644 --- a/src/gui/wizard/owncloudhttpcredspage.cpp +++ b/src/gui/wizard/owncloudhttpcredspage.cpp @@ -191,7 +191,7 @@ void OwncloudHttpCredsPage::setErrorString(const QString &err) AbstractCredentials *OwncloudHttpCredsPage::getCredentials() const { - return new HttpCredentialsGui(_ui.leUsername->text(), _ui.lePassword->text(), _ocWizard->_clientSslCertificate, _ocWizard->_clientSslKey); + return new HttpCredentialsGui(_ui.leUsername->text(), _ui.lePassword->text(), _ocWizard->_clientCertBundle, _ocWizard->_clientCertPassword); } void OwncloudHttpCredsPage::slotStyleChanged() diff --git a/src/gui/wizard/owncloudoauthcredspage.cpp b/src/gui/wizard/owncloudoauthcredspage.cpp index 79f36ba36..267830dc1 100644 --- a/src/gui/wizard/owncloudoauthcredspage.cpp +++ b/src/gui/wizard/owncloudoauthcredspage.cpp @@ -112,7 +112,7 @@ AbstractCredentials *OwncloudOAuthCredsPage::getCredentials() const auto *ocWizard = qobject_cast(wizard()); Q_ASSERT(ocWizard); return new HttpCredentialsGui(_user, _token, _refreshToken, - ocWizard->_clientSslCertificate, ocWizard->_clientSslKey); + ocWizard->_clientCertBundle, ocWizard->_clientCertPassword); } bool OwncloudOAuthCredsPage::isComplete() const diff --git a/src/gui/wizard/owncloudsetuppage.cpp b/src/gui/wizard/owncloudsetuppage.cpp index 3fac734d0..13538827a 100644 --- a/src/gui/wizard/owncloudsetuppage.cpp +++ b/src/gui/wizard/owncloudsetuppage.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include "QProgressIndicator.h" @@ -365,14 +366,20 @@ void OwncloudSetupPage::slotCertificateAccepted() { QFile certFile(addCertDial->getCertificatePath()); certFile.open(QFile::ReadOnly); - if (QSslCertificate::importPkcs12( - &certFile, - &_ocWizard->_clientSslKey, - &_ocWizard->_clientSslCertificate, - &_ocWizard->_clientSslCaCertificates, - addCertDial->getCertificatePasswd().toLocal8Bit())) { - // The SSL cert gets added to the QSslConfiguration in checkServer() + QByteArray certData = certFile.readAll(); + QByteArray certPassword = addCertDial->getCertificatePasswd().toLocal8Bit(); + + QBuffer certDataBuffer(&certData); + certDataBuffer.open(QIODevice::ReadOnly); + if (QSslCertificate::importPkcs12(&certDataBuffer, + &_ocWizard->_clientSslKey, &_ocWizard->_clientSslCertificate, + &_ocWizard->_clientSslCaCertificates, certPassword)) { + _ocWizard->_clientCertBundle = certData; + _ocWizard->_clientCertPassword = certPassword; + addCertDial->reinit(); // FIXME: Why not just have this only created on use? + + // The extracted SSL key and cert gets added to the QSslConfiguration in checkServer() validatePage(); } else { addCertDial->showErrorMessage(tr("Could not load certificate. Maybe wrong password?")); diff --git a/src/gui/wizard/owncloudwizard.h b/src/gui/wizard/owncloudwizard.h index 5a61a741f..3dd479d59 100644 --- a/src/gui/wizard/owncloudwizard.h +++ b/src/gui/wizard/owncloudwizard.h @@ -86,8 +86,10 @@ public: // FIXME: Can those be local variables? // Set from the OwncloudSetupPage, later used from OwncloudHttpCredsPage - QSslKey _clientSslKey; - QSslCertificate _clientSslCertificate; + QByteArray _clientCertBundle; // raw, potentially encrypted pkcs12 bundle provided by the user + QByteArray _clientCertPassword; // password for the pkcs12 + QSslKey _clientSslKey; // key extracted from pkcs12 + QSslCertificate _clientSslCertificate; // cert extracted from pkcs12 QList _clientSslCaCertificates; public slots: diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index cde76ceb5..b13fd329d 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -359,9 +359,9 @@ QVariant Account::credentialSetting(const QString &key) const { if (_credentials) { QString prefix = _credentials->authType(); - QString value = _settingsMap.value(prefix + "_" + key).toString(); - if (value.isEmpty()) { - value = _settingsMap.value(key).toString(); + QVariant value = _settingsMap.value(prefix + "_" + key); + if (value.isNull()) { + value = _settingsMap.value(key); } return value; } diff --git a/src/libsync/creds/httpcredentials.cpp b/src/libsync/creds/httpcredentials.cpp index c2c28bcb5..21bb5559c 100644 --- a/src/libsync/creds/httpcredentials.cpp +++ b/src/libsync/creds/httpcredentials.cpp @@ -42,6 +42,8 @@ Q_LOGGING_CATEGORY(lcHttpCredentials, "nextcloud.sync.credentials.http", QtInfoM namespace { const char userC[] = "user"; const char isOAuthC[] = "oauth"; + const char clientCertBundleC[] = "clientCertPkcs12"; + const char clientCertPasswordC[] = "_clientCertPassword"; const char clientCertificatePEMC[] = "_clientCertificatePEM"; const char clientKeyPEMC[] = "_clientKeyPEM"; const char authenticationFailedC[] = "owncloud-authentication-failed"; @@ -114,14 +116,17 @@ static void addSettingsToJob(Account *account, QKeychain::Job *job) HttpCredentials::HttpCredentials() = default; // From wizard -HttpCredentials::HttpCredentials(const QString &user, const QString &password, const QSslCertificate &certificate, const QSslKey &key) +HttpCredentials::HttpCredentials(const QString &user, const QString &password, const QByteArray &clientCertBundle, const QByteArray &clientCertPassword) : _user(user) , _password(password) , _ready(true) - , _clientSslKey(key) - , _clientSslCertificate(certificate) + , _clientCertBundle(clientCertBundle) + , _clientCertPassword(clientCertPassword) , _retryOnKeyChainError(false) { + if (!unpackClientCertBundle()) { + ASSERT(false, "pkcs12 client cert bundle passed to HttpCredentials must be valid"); + } } QString HttpCredentials::authType() const @@ -192,7 +197,20 @@ void HttpCredentials::fetchFromKeychain() void HttpCredentials::fetchFromKeychainHelper() { - // Read client cert from keychain + _clientCertBundle = _account->credentialSetting(QLatin1String(clientCertBundleC)).toByteArray(); + if (!_clientCertBundle.isEmpty()) { + // New case (>=2.6): We have a bundle in the settings and read the password from + // the keychain + ReadPasswordJob *job = new ReadPasswordJob(Theme::instance()->appName()); + addSettingsToJob(_account, job); + job->setInsecureFallback(false); + job->setKey(keychainKey(_account->url().toString(), _user + clientCertPasswordC, _account->id())); + connect(job, &Job::finished, this, &HttpCredentials::slotReadClientCertPasswordJobDone); + job->start(); + return; + } + + // Old case (pre 2.6): Read client cert and then key from keychain const QString kck = keychainKey( _account->url().toString(), _user + clientCertificatePEMC, @@ -221,7 +239,7 @@ void HttpCredentials::deleteOldKeychainEntries() startDeleteJob(_user + clientCertificatePEMC); } -void HttpCredentials::slotReadClientCertPEMJobDone(QKeychain::Job *incoming) +bool HttpCredentials::keychainUnavailableRetryLater(QKeychain::Job *incoming) { #if defined(Q_OS_UNIX) && !defined(Q_OS_MAC) Q_ASSERT(!incoming->insecureFallback()); // If insecureFallback is set, the next test would be pointless @@ -233,10 +251,38 @@ void HttpCredentials::slotReadClientCertPEMJobDone(QKeychain::Job *incoming) qCInfo(lcHttpCredentials) << "Backend unavailable (yet?) Retrying in a few seconds." << incoming->errorString(); QTimer::singleShot(10000, this, &HttpCredentials::fetchFromKeychainHelper); _retryOnKeyChainError = false; - return; + return true; } - _retryOnKeyChainError = false; #endif + _retryOnKeyChainError = false; + return false; +} + +void HttpCredentials::slotReadClientCertPasswordJobDone(QKeychain::Job *job) +{ + if (keychainUnavailableRetryLater(job)) + return; + + ReadPasswordJob *readJob = static_cast(job); + if (readJob->error() == NoError) { + _clientCertPassword = readJob->binaryData(); + } else { + qCWarning(lcHttpCredentials) << "Could not retrieve client cert password from keychain" << job->errorString(); + } + + if (!unpackClientCertBundle()) { + qCWarning(lcHttpCredentials) << "Could not unpack client cert bundle"; + } + _clientCertBundle.clear(); + _clientCertPassword.clear(); + + slotReadPasswordFromKeychain(); +} + +void HttpCredentials::slotReadClientCertPEMJobDone(QKeychain::Job *incoming) +{ + if (keychainUnavailableRetryLater(incoming)) + return; // Store PEM in memory auto *readJob = static_cast(incoming); @@ -282,7 +328,11 @@ void HttpCredentials::slotReadClientKeyPEMJobDone(QKeychain::Job *incoming) } } - // Now fetch the actual server password + slotReadPasswordFromKeychain(); +} + +void HttpCredentials::slotReadPasswordFromKeychain() +{ const QString kck = keychainKey( _account->url().toString(), _user, @@ -468,10 +518,32 @@ void HttpCredentials::persist() _account->setCredentialSetting(QLatin1String(userC), _user); _account->setCredentialSetting(QLatin1String(isOAuthC), isUsingOAuth()); + if (!_clientCertBundle.isEmpty()) { + // Note that the _clientCertBundle will often be cleared after usage, + // it's just written if it gets passed into the constructor. + _account->setCredentialSetting(QLatin1String(clientCertBundleC), _clientCertBundle); + } _account->wantsAccountSaved(_account); - // write cert if there is one - if (!_clientSslCertificate.isNull()) { + // write secrets to the keychain + if (!_clientCertBundle.isEmpty()) { + // Option 1: If we have a pkcs12 bundle, that'll be written to the config file + // and we'll just store the bundle password in the keychain. That's prefered + // since the keychain on older Windows platforms can only store a limited number + // of bytes per entry and key/cert may exceed that. + auto *job = new WritePasswordJob(Theme::instance()->appName()); + addSettingsToJob(_account, job); + job->setInsecureFallback(false); + connect(job, &Job::finished, this, &HttpCredentials::slotWriteClientCertPasswordJobDone); + job->setKey(keychainKey(_account->url().toString(), _user + clientCertPasswordC, _account->id())); + job->setBinaryData(_clientCertPassword); + job->start(); + _clientCertBundle.clear(); + _clientCertPassword.clear(); + } else if (_account->credentialSetting(QLatin1String(clientCertBundleC)).isNull() && !_clientSslCertificate.isNull()) { + // Option 2, pre 2.6 configs: We used to store the raw cert/key in the keychain and + // still do so if no bundle is available. We can't currently migrate to Option 1 + // because we have no functions for creating an encrypted pkcs12 bundle. auto *job = new WritePasswordJob(Theme::instance()->appName()); addSettingsToJob(_account, job); job->setInsecureFallback(false); @@ -480,10 +552,21 @@ void HttpCredentials::persist() job->setBinaryData(_clientSslCertificate.toPem()); job->start(); } else { - slotWriteClientCertPEMJobDone(nullptr); + // Option 3: no client certificate at all (or doesn't need to be written) + slotWritePasswordToKeychain(); } } +void HttpCredentials::slotWriteClientCertPasswordJobDone(Job *finishedJob) +{ + if (finishedJob && finishedJob->error() != QKeychain::NoError) { + qCWarning(lcHttpCredentials) << "Could not write client cert password to credentials" + << finishedJob->error() << finishedJob->errorString(); + } + + slotWritePasswordToKeychain(); +} + void HttpCredentials::slotWriteClientCertPEMJobDone(Job *finishedJob) { if (finishedJob && finishedJob->error() != QKeychain::NoError) { @@ -512,6 +595,11 @@ void HttpCredentials::slotWriteClientKeyPEMJobDone(Job *finishedJob) << finishedJob->error() << finishedJob->errorString(); } + slotWritePasswordToKeychain(); +} + +void HttpCredentials::slotWritePasswordToKeychain() +{ auto *job = new WritePasswordJob(Theme::instance()->appName()); addSettingsToJob(_account, job); job->setInsecureFallback(false); @@ -561,4 +649,16 @@ bool HttpCredentials::retryIfNeeded(AbstractNetworkJob *job) return true; } +bool HttpCredentials::unpackClientCertBundle() +{ + if (_clientCertBundle.isEmpty()) + return true; + + QBuffer certBuffer(&_clientCertBundle); + certBuffer.open(QIODevice::ReadOnly); + QList clientCaCertificates; + return QSslCertificate::importPkcs12( + &certBuffer, &_clientSslKey, &_clientSslCertificate, &clientCaCertificates, _clientCertPassword); +} + } // namespace OCC diff --git a/src/libsync/creds/httpcredentials.h b/src/libsync/creds/httpcredentials.h index 2ac089f09..04c7efc9a 100644 --- a/src/libsync/creds/httpcredentials.h +++ b/src/libsync/creds/httpcredentials.h @@ -80,7 +80,8 @@ public: static constexpr QNetworkRequest::Attribute DontAddCredentialsAttribute = QNetworkRequest::User; HttpCredentials(); - explicit HttpCredentials(const QString &user, const QString &password, const QSslCertificate &certificate = QSslCertificate(), const QSslKey &key = QSslKey()); + explicit HttpCredentials(const QString &user, const QString &password, + const QByteArray &clientCertBundle = QByteArray(), const QByteArray &clientCertPassword = QByteArray()); QString authType() const override; QNetworkAccessManager *createQNAM() const override; @@ -112,12 +113,18 @@ public: private Q_SLOTS: void slotAuthentication(QNetworkReply *, QAuthenticator *); + void slotReadClientCertPasswordJobDone(QKeychain::Job *); void slotReadClientCertPEMJobDone(QKeychain::Job *); void slotReadClientKeyPEMJobDone(QKeychain::Job *); + + void slotReadPasswordFromKeychain(); void slotReadJobDone(QKeychain::Job *); + void slotWriteClientCertPasswordJobDone(QKeychain::Job *); void slotWriteClientCertPEMJobDone(QKeychain::Job *); void slotWriteClientKeyPEMJobDone(QKeychain::Job *); + + void slotWritePasswordToKeychain(); void slotWriteJobDone(QKeychain::Job *); protected: @@ -133,6 +140,22 @@ protected: /// Wipes legacy keychain locations void deleteOldKeychainEntries(); + /** Whether to bow out now because a retry will happen later + * + * Sometimes the keychain needs a while to become available. + * This function should be called on first keychain-read to check + * whether it errored because the keychain wasn't available yet. + * If that happens, this function will schedule another try and + * return true. + */ + bool keychainUnavailableRetryLater(QKeychain::Job *); + + /** Takes client cert pkcs12 and unwraps the key/cert. + * + * Returns false on failure. + */ + bool unpackClientCertBundle(); + QString _user; QString _password; // user's password, or access_token for OAuth QString _refreshToken; // OAuth _refreshToken, set if OAuth is used. @@ -141,6 +164,8 @@ protected: QString _fetchErrorString; bool _ready = false; bool _isRenewingOAuthToken = false; + QByteArray _clientCertBundle; + QByteArray _clientCertPassword; QSslKey _clientSslKey; QSslCertificate _clientSslCertificate; bool _keychainMigration = false; -- 2.30.2