Skip to content

Commit

Permalink
Client certs: Store pkcs12 in config, password in keychain
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ckamm
ckamm committed Feb 19, 2019
1 parent a627f37 commit 1063f8c
Show file tree
Hide file tree
Showing 9 changed files with 202 additions and 32 deletions.
44 changes: 39 additions & 5 deletions src/gui/addcertificatedialog.ui
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>462</width>
<height>188</height>
<width>478</width>
<height>225</height>
</rect>
</property>
<property name="windowTitle">
Expand Down Expand Up @@ -73,11 +73,30 @@
</item>
</layout>
</item>
<item>
<widget class="QLabel" name="label">
<property name="text">
<string>An encrypted pkcs12 bundle is strongly recommended as a copy will be stored in the configuration file.</string>
</property>
<property name="wordWrap">
<bool>true</bool>
</property>
</widget>
</item>
<item>
<widget class="QLabel" name="labelErrorCertif">
<property name="palette">
<palette>
<active>
<colorrole role="WindowText">
<brush brushstyle="SolidPattern">
<color alpha="255">
<red>255</red>
<green>0</green>
<blue>0</blue>
</color>
</brush>
</colorrole>
<colorrole role="Text">
<brush brushstyle="SolidPattern">
<color alpha="255">
Expand All @@ -89,6 +108,15 @@
</colorrole>
</active>
<inactive>
<colorrole role="WindowText">
<brush brushstyle="SolidPattern">
<color alpha="255">
<red>255</red>
<green>0</green>
<blue>0</blue>
</color>
</brush>
</colorrole>
<colorrole role="Text">
<brush brushstyle="SolidPattern">
<color alpha="255">
Expand All @@ -100,6 +128,15 @@
</colorrole>
</inactive>
<disabled>
<colorrole role="WindowText">
<brush brushstyle="SolidPattern">
<color alpha="255">
<red>119</red>
<green>120</green>
<blue>120</blue>
</color>
</brush>
</colorrole>
<colorrole role="Text">
<brush brushstyle="SolidPattern">
<color alpha="255">
Expand All @@ -112,9 +149,6 @@
</disabled>
</palette>
</property>
<property name="text">
<string/>
</property>
<property name="wordWrap">
<bool>true</bool>
</property>
Expand Down
9 changes: 5 additions & 4 deletions src/gui/creds/httpcredentialsgui.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,14 @@ class HttpCredentialsGui : public HttpCredentials
: 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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/gui/wizard/owncloudhttpcredspage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}


Expand Down
2 changes: 1 addition & 1 deletion src/gui/wizard/owncloudoauthcredspage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ AbstractCredentials *OwncloudOAuthCredsPage::getCredentials() const
OwncloudWizard *ocWizard = qobject_cast<OwncloudWizard *>(wizard());
Q_ASSERT(ocWizard);
return new HttpCredentialsGui(_user, _token, _refreshToken,
ocWizard->_clientSslCertificate, ocWizard->_clientSslKey);
ocWizard->_clientCertBundle, ocWizard->_clientCertPassword);
}

bool OwncloudOAuthCredsPage::isComplete() const
Expand Down
17 changes: 13 additions & 4 deletions src/gui/wizard/owncloudsetuppage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <QSsl>
#include <QSslCertificate>
#include <QNetworkAccessManager>
#include <QBuffer>

#include "QProgressIndicator.h"

Expand Down Expand Up @@ -295,12 +296,20 @@ void OwncloudSetupPage::slotCertificateAccepted()
QList<QSslCertificate> clientCaCertificates;
QFile certFile(addCertDial->getCertificatePath());
certFile.open(QFile::ReadOnly);
if (QSslCertificate::importPkcs12(&certFile,
QByteArray certData = certFile.readAll();
QByteArray certPassword = addCertDial->getCertificatePasswd().toLocal8Bit();

QBuffer certDataBuffer(&certData);
certDataBuffer.open(QIODevice::ReadOnly);
if (QSslCertificate::importPkcs12(&certDataBuffer,
&_ocWizard->_clientSslKey, &_ocWizard->_clientSslCertificate,
&clientCaCertificates,
addCertDial->getCertificatePasswd().toLocal8Bit())) {
// The SSL cert gets added to the QSslConfiguration in checkServer()
&clientCaCertificates, 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?"));
Expand Down
6 changes: 4 additions & 2 deletions src/gui/wizard/owncloudwizard.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,10 @@ class OwncloudWizard : public QWizard

// 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

public slots:
void setAuthType(DetermineAuthTypeJob::AuthType type);
Expand Down
6 changes: 3 additions & 3 deletions src/libsync/account.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,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;
}
Expand Down
121 changes: 110 additions & 11 deletions src/libsync/creds/httpcredentials.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ Q_LOGGING_CATEGORY(lcHttpCredentials, "sync.credentials.http", QtInfoMsg)
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";
Expand Down Expand Up @@ -112,15 +114,18 @@ static void addSettingsToJob(Account *account, QKeychain::Job *job)
}

// 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)
, _keychainMigration(false)
, _retryOnKeyChainError(false)
{
if (!unpackClientCertBundle()) {
ASSERT(false, "pkcs12 client cert bundle passed to HttpCredentials must be valid");
}
}

QString HttpCredentials::authType() const
Expand Down Expand Up @@ -191,7 +196,20 @@ void HttpCredentials::fetchFromKeychain()

void HttpCredentials::fetchFromKeychainHelper()
{
// Read client cert from keychain
_clientCertBundle = _account->credentialSetting(QLatin1String(clientCertBundleC)).toByteArray();
if (!_clientCertBundle.isEmpty()) {
// New case: 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: Read client cert and then key from keychain
const QString kck = keychainKey(
_account->url().toString(),
_user + clientCertificatePEMC,
Expand Down Expand Up @@ -220,7 +238,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
Expand All @@ -232,10 +250,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<ReadPasswordJob *>(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
ReadPasswordJob *readJob = static_cast<ReadPasswordJob *>(incoming);
Expand Down Expand Up @@ -281,7 +327,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,
Expand Down Expand Up @@ -467,10 +517,31 @@ 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.
WritePasswordJob *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, older configs: We used to store the raw cert/key in the keychain and
// still do so if no bundle is available.
WritePasswordJob *job = new WritePasswordJob(Theme::instance()->appName());
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
Expand All @@ -479,10 +550,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) {
Expand Down Expand Up @@ -511,6 +593,11 @@ void HttpCredentials::slotWriteClientKeyPEMJobDone(Job *finishedJob)
<< finishedJob->error() << finishedJob->errorString();
}

slotWritePasswordToKeychain();
}

void HttpCredentials::slotWritePasswordToKeychain()
{
WritePasswordJob *job = new WritePasswordJob(Theme::instance()->appName());
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
Expand Down Expand Up @@ -560,4 +647,16 @@ bool HttpCredentials::retryIfNeeded(AbstractNetworkJob *job)
return true;
}

bool HttpCredentials::unpackClientCertBundle()
{
if (_clientCertBundle.isEmpty())
return true;

QBuffer certBuffer(&_clientCertBundle);
certBuffer.open(QIODevice::ReadOnly);
QList<QSslCertificate> clientCaCertificates;
return QSslCertificate::importPkcs12(
&certBuffer, &_clientSslKey, &_clientSslCertificate, &clientCaCertificates, _clientCertPassword);
}

} // namespace OCC
Loading

0 comments on commit 1063f8c

Please sign in to comment.