From a7e7f7fdd2a9d38921476d57f305c9cd4459a556 Mon Sep 17 00:00:00 2001 From: Christian Mollekopf Date: Mon, 18 Sep 2017 11:40:41 +0200 Subject: Avoid storing the password in the configuration The password (or any other secret), is now cached in the client process (in-memory only), and delivered to the resource via command. The resource avoids doing any operations against the source until the secret is available. --- common/CMakeLists.txt | 3 ++ common/commands.h | 1 + common/commands/secret.fbs | 7 ++++ common/domain/applicationdomaintype.h | 1 + common/genericresource.cpp | 7 ++++ common/genericresource.h | 2 ++ common/listener.cpp | 12 +++++++ common/resource.cpp | 5 +++ common/resource.h | 2 ++ common/resourceaccess.cpp | 22 ++++++++++++ common/resourceaccess.h | 6 ++++ common/secretstore.cpp | 51 ++++++++++++++++++++++++++++ common/secretstore.h | 50 +++++++++++++++++++++++++++ common/synchronizer.cpp | 18 ++++++++++ common/synchronizer.h | 5 +++ examples/davresource/davresource.cpp | 30 ++++++++++------ examples/davresource/davresource.h | 5 --- examples/dummyresource/resourcefactory.cpp | 2 +- examples/imapresource/imapresource.cpp | 16 ++++----- examples/imapresource/imapserverproxy.cpp | 3 ++ examples/imapresource/imapserverproxy.h | 1 + examples/maildirresource/maildirresource.cpp | 2 +- 22 files changed, 226 insertions(+), 25 deletions(-) create mode 100644 common/commands/secret.fbs create mode 100644 common/secretstore.cpp create mode 100644 common/secretstore.h diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt index 8421fc2..b0e0d04 100644 --- a/common/CMakeLists.txt +++ b/common/CMakeLists.txt @@ -33,6 +33,7 @@ set(storage_LIBS lmdb) set(command_SRCS store.cpp + secretstore.cpp notifier.cpp resourcecontrol.cpp modelresult.cpp @@ -97,6 +98,7 @@ generate_flatbuffers( commands/revisionreplayed commands/inspection commands/flush + commands/secret domain/contact domain/addressbook domain/event @@ -144,6 +146,7 @@ install(FILES test.h log.h flush.h + secretstore.h ${CMAKE_CURRENT_BINARY_DIR}/sink_export.h DESTINATION ${INCLUDE_INSTALL_DIR}/${PROJECT_NAME} COMPONENT Devel ) diff --git a/common/commands.h b/common/commands.h index 1548eac..8ced268 100644 --- a/common/commands.h +++ b/common/commands.h @@ -48,6 +48,7 @@ enum CommandIds InspectionCommand, RemoveFromDiskCommand, FlushCommand, + SecretCommand, CustomCommand = 0xffff }; diff --git a/common/commands/secret.fbs b/common/commands/secret.fbs new file mode 100644 index 0000000..5bb2246 --- /dev/null +++ b/common/commands/secret.fbs @@ -0,0 +1,7 @@ +namespace Sink.Commands; + +table Secret { + secret: string; +} + +root_type Secret; diff --git a/common/domain/applicationdomaintype.h b/common/domain/applicationdomaintype.h index f7fd07e..de2b5c9 100644 --- a/common/domain/applicationdomaintype.h +++ b/common/domain/applicationdomaintype.h @@ -102,6 +102,7 @@ enum SINK_EXPORT ErrorCode { ConfigurationError, TransmissionError, ConnectionLostError, + MissingCredentialsError }; enum SINK_EXPORT SuccessCode { diff --git a/common/genericresource.cpp b/common/genericresource.cpp index 7178b3d..b6a8f62 100644 --- a/common/genericresource.cpp +++ b/common/genericresource.cpp @@ -45,6 +45,13 @@ GenericResource::~GenericResource() { } +void GenericResource::setSecret(const QString &s) +{ + if (mSynchronizer) { + mSynchronizer->setSecret(s); + } +} + void GenericResource::setupPreprocessors(const QByteArray &type, const QVector &preprocessors) { mPipeline->setPreprocessors(type, preprocessors); diff --git a/common/genericresource.h b/common/genericresource.h index bffc697..4e86b75 100644 --- a/common/genericresource.h +++ b/common/genericresource.h @@ -47,6 +47,8 @@ public: static void removeFromDisk(const QByteArray &instanceIdentifier); static qint64 diskUsage(const QByteArray &instanceIdentifier); + virtual void setSecret(const QString &s) Q_DECL_OVERRIDE; + //TODO Remove this API, it's only used in tests KAsync::Job synchronizeWithSource(const Sink::QueryBase &query); //TODO Remove this API, it's only used in tests diff --git a/common/listener.cpp b/common/listener.cpp index c9fd9d3..c6747cd 100644 --- a/common/listener.cpp +++ b/common/listener.cpp @@ -33,6 +33,7 @@ #include "common/revisionupdate_generated.h" #include "common/notification_generated.h" #include "common/revisionreplayed_generated.h" +#include "common/secret_generated.h" #include #include @@ -240,6 +241,17 @@ void Listener::processCommand(int commandId, uint messageId, const QByteArray &c } break; } + case Sink::Commands::SecretCommand: { + flatbuffers::Verifier verifier((const uint8_t *)commandBuffer.constData(), commandBuffer.size()); + if (Sink::Commands::VerifySecretBuffer(verifier)) { + auto buffer = Sink::Commands::GetSecret(commandBuffer.constData()); + loadResource().setSecret(QString{buffer->secret()->c_str()}); + } else { + SinkWarning() << "received invalid command"; + } + break; + } + case Sink::Commands::SynchronizeCommand: case Sink::Commands::InspectionCommand: case Sink::Commands::DeleteEntityCommand: diff --git a/common/resource.cpp b/common/resource.cpp index 804f0bf..78cc51b 100644 --- a/common/resource.cpp +++ b/common/resource.cpp @@ -51,6 +51,11 @@ void Resource::setLowerBoundRevision(qint64 revision) Q_UNUSED(revision) } +void Resource::setSecret(const QString &s) +{ + Q_UNUSED(s) +} + class ResourceFactory::Private { diff --git a/common/resource.h b/common/resource.h index a50ffb5..938f267 100644 --- a/common/resource.h +++ b/common/resource.h @@ -47,6 +47,8 @@ public: */ virtual void setLowerBoundRevision(qint64 revision); + virtual void setSecret(const QString &s); + signals: void revisionUpdated(qint64); void notify(Notification); diff --git a/common/resourceaccess.cpp b/common/resourceaccess.cpp index 35fa46c..5ed3609 100644 --- a/common/resourceaccess.cpp +++ b/common/resourceaccess.cpp @@ -32,9 +32,11 @@ #include "common/revisionreplayed_generated.h" #include "common/inspection_generated.h" #include "common/flush_generated.h" +#include "common/secret_generated.h" #include "common/entitybuffer.h" #include "common/bufferutils.h" #include "common/test.h" +#include "common/secretstore.h" #include "log.h" #include @@ -234,6 +236,12 @@ ResourceAccess::ResourceAccess(const QByteArray &resourceInstanceIdentifier, con { mResourceStatus = Sink::ApplicationDomain::NoStatus; SinkTrace() << "Starting access"; + + QObject::connect(&SecretStore::instance(), &SecretStore::secretAvailable, this, [this] (const QByteArray &resourceId) { + if (resourceId == d->resourceInstanceIdentifier) { + sendSecret(SecretStore::instance().resourceSecret(d->resourceInstanceIdentifier)).exec(); + } + }); } ResourceAccess::~ResourceAccess() @@ -387,6 +395,15 @@ KAsync::Job ResourceAccess::sendFlushCommand(int flushType, const QByteArr return sendCommand(Sink::Commands::FlushCommand, fbb); } +KAsync::Job ResourceAccess::sendSecret(const QString &secret) +{ + flatbuffers::FlatBufferBuilder fbb; + auto s = fbb.CreateString(secret.toStdString()); + auto location = Sink::Commands::CreateSecret(fbb, s); + Sink::Commands::FinishSecretBuffer(fbb, location); + return sendCommand(Sink::Commands::SecretCommand, fbb); +} + void ResourceAccess::open() { if (d->socket && d->socket->isValid()) { @@ -483,6 +500,11 @@ void ResourceAccess::connected() Commands::write(d->socket.data(), ++d->messageId, Commands::HandshakeCommand, fbb); } + auto secret = SecretStore::instance().resourceSecret(d->resourceInstanceIdentifier); + if (!secret.isEmpty()) { + sendSecret(secret).exec(); + } + // Reenqueue pending commands, we failed to send them processPendingCommandQueue(); // Send queued commands diff --git a/common/resourceaccess.h b/common/resourceaccess.h index b6a0b34..890cc6d 100644 --- a/common/resourceaccess.h +++ b/common/resourceaccess.h @@ -80,6 +80,11 @@ public: return KAsync::null(); } + virtual KAsync::Job sendSecret(const QString &secret) + { + return KAsync::null(); + } + int getResourceStatus() const { return mResourceStatus; @@ -121,6 +126,7 @@ public: KAsync::Job sendInspectionCommand(int inspectionType,const QByteArray &inspectionId, const QByteArray &domainType, const QByteArray &entityId, const QByteArray &property, const QVariant &expecedValue) Q_DECL_OVERRIDE; KAsync::Job sendFlushCommand(int flushType, const QByteArray &flushId) Q_DECL_OVERRIDE; + KAsync::Job sendSecret(const QString &secret) Q_DECL_OVERRIDE; /** * Tries to connect to server, and returns a connected socket on success. */ diff --git a/common/secretstore.cpp b/common/secretstore.cpp new file mode 100644 index 0000000..39d5206 --- /dev/null +++ b/common/secretstore.cpp @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2017 Christian Mollekopf + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) version 3, or any + * later version accepted by the membership of KDE e.V. (or its + * successor approved by the membership of KDE e.V.), which shall + * act as a proxy defined in Section 6 of version 3 of the license. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see . + */ + +#include "secretstore.h" + +#include +#include +#include +#include + +using namespace Sink; + +QMutex SecretStore::sMutex; + +SecretStore &SecretStore::instance() +{ + static SecretStore s; + return s; +} + +void SecretStore::insert(const QByteArray &resourceId, const QString &secret) +{ + QMutexLocker locker{&sMutex}; + mCache.insert(resourceId, secret); + locker.unlock(); + emit secretAvailable(resourceId); +} + +QString SecretStore::resourceSecret(const QByteArray &resourceId) +{ + QMutexLocker locker{&sMutex}; + return mCache.value(resourceId); +} + diff --git a/common/secretstore.h b/common/secretstore.h new file mode 100644 index 0000000..04fdaba --- /dev/null +++ b/common/secretstore.h @@ -0,0 +1,50 @@ +/* + * Copyright (C) 2017 Christian Mollekopf + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) version 3, or any + * later version accepted by the membership of KDE e.V. (or its + * successor approved by the membership of KDE e.V.), which shall + * act as a proxy defined in Section 6 of version 3 of the license. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see . + */ + +#pragma once + +#include "sink_export.h" + +#include +#include +#include +#include + +namespace Sink { + +class SINK_EXPORT SecretStore : public QObject +{ + Q_OBJECT +public: + static SecretStore &instance(); + + void insert(const QByteArray &resourceId, const QString &secret); + QString resourceSecret(const QByteArray &resourceId); + +Q_SIGNALS: + void secretAvailable(const QByteArray &resourceId); + +private: + QMap mCache; + static QMutex sMutex; +}; + +} + diff --git a/common/synchronizer.cpp b/common/synchronizer.cpp index 959cf48..4a0ac46 100644 --- a/common/synchronizer.cpp +++ b/common/synchronizer.cpp @@ -49,6 +49,20 @@ Synchronizer::~Synchronizer() } +void Synchronizer::setSecret(const QString &s) +{ + mSecret = s; + + if (!mSyncRequestQueue.isEmpty()) { + processSyncQueue().exec(); + } +} + +QString Synchronizer::secret() const +{ + return mSecret; +} + void Synchronizer::setup(const std::function &enqueueCommandCallback, MessageQueue &mq) { mEnqueue = enqueueCommandCallback; @@ -474,6 +488,10 @@ void Synchronizer::setBusy(bool busy, const QString &reason, const QByteArray re KAsync::Job Synchronizer::processSyncQueue() { + if (secret().isEmpty()) { + SinkLogCtx(mLogCtx) << "Secret not available but required."; + return KAsync::null(); + } if (mSyncRequestQueue.isEmpty()) { SinkLogCtx(mLogCtx) << "All requests processed."; return KAsync::null(); diff --git a/common/synchronizer.h b/common/synchronizer.h index cc082be..bd03ba3 100644 --- a/common/synchronizer.h +++ b/common/synchronizer.h @@ -60,6 +60,8 @@ public: bool allChangesReplayed() Q_DECL_OVERRIDE; void flushComplete(const QByteArray &flushId); + void setSecret(const QString &s); + signals: void notify(Notification); @@ -78,6 +80,8 @@ protected: virtual KAsync::Job replay(const Sink::ApplicationDomain::Mail &, Sink::Operation, const QByteArray &oldRemoteId, const QList &); virtual KAsync::Job replay(const Sink::ApplicationDomain::Folder &, Sink::Operation, const QByteArray &oldRemoteId, const QList &); protected: + QString secret() const; + ///Calls the callback to enqueue the command void enqueueCommand(int commandId, const QByteArray &data); @@ -224,6 +228,7 @@ private: MessageQueue *mMessageQueue; bool mSyncInProgress; QMultiHash mPendingSyncRequests; + QString mSecret; }; } diff --git a/examples/davresource/davresource.cpp b/examples/davresource/davresource.cpp index 22c502f..fa5e612 100644 --- a/examples/davresource/davresource.cpp +++ b/examples/davresource/davresource.cpp @@ -132,8 +132,8 @@ public: KAsync::Job synchronizeWithSource(const Sink::QueryBase &query) Q_DECL_OVERRIDE { if (query.type() == ApplicationDomain::getTypeName()) { - SinkLogCtx(mLogCtx) << "Synchronizing addressbooks:" << mResourceUrl.url(); - auto collectionsFetchJob = new KDAV2::DavCollectionsFetchJob(mResourceUrl); + SinkLogCtx(mLogCtx) << "Synchronizing addressbooks:" << resourceUrl().url(); + auto collectionsFetchJob = new KDAV2::DavCollectionsFetchJob(resourceUrl()); auto job = runJob(collectionsFetchJob).then([this, collectionsFetchJob] (const KAsync::Error &error) { if (error) { SinkWarningCtx(mLogCtx) << "Failed to synchronize addressbooks." << collectionsFetchJob->errorString(); @@ -147,7 +147,7 @@ public: auto ridList = QSharedPointer::create(); auto total = QSharedPointer::create(0); auto progress = QSharedPointer::create(0); - auto collectionsFetchJob = new KDAV2::DavCollectionsFetchJob(mResourceUrl); + auto collectionsFetchJob = new KDAV2::DavCollectionsFetchJob(resourceUrl()); auto job = runJob(collectionsFetchJob).then([this, collectionsFetchJob] { synchronizeAddressbooks(collectionsFetchJob ->collections()); return collectionsFetchJob->collections(); @@ -233,8 +233,20 @@ KAsync::Job replay(const ApplicationDomain::Contact &contact, Sink:: return KAsync::null(); } + KDAV2::DavUrl resourceUrl() const + { + if (secret().isEmpty()) { + return {}; + } + auto resourceUrl = mServer; + resourceUrl.setUserName(mUsername); + resourceUrl.setPassword(secret()); + return KDAV2::DavUrl{resourceUrl, KDAV2::CardDav}; + } + public: - KDAV2::DavUrl mResourceUrl; + QUrl mServer; + QString mUsername; }; @@ -242,14 +254,12 @@ DavResource::DavResource(const Sink::ResourceContext &resourceContext) : Sink::GenericResource(resourceContext) { auto config = ResourceConfig::getConfiguration(resourceContext.instanceId()); - auto resourceUrl = QUrl::fromUserInput(config.value("server").toString()); - resourceUrl.setUserName(config.value("username").toString()); - resourceUrl.setPassword(config.value("password").toString()); - - mResourceUrl = KDAV2::DavUrl(resourceUrl, KDAV2::CardDav); + auto server = QUrl::fromUserInput(config.value("server").toString()); + auto username = config.value("username").toString(); auto synchronizer = QSharedPointer::create(resourceContext); - synchronizer->mResourceUrl = mResourceUrl; + synchronizer->mServer = server; + synchronizer->mUsername = username; setupSynchronizer(synchronizer); setupPreprocessors(ENTITY_TYPE_CONTACT, QVector() << new ContactPropertyExtractor); diff --git a/examples/davresource/davresource.h b/examples/davresource/davresource.h index db175a4..b4f9e5a 100644 --- a/examples/davresource/davresource.h +++ b/examples/davresource/davresource.h @@ -44,11 +44,6 @@ class DavResource : public Sink::GenericResource { public: DavResource(const Sink::ResourceContext &resourceContext); - -private: - QStringList listAvailableFolders(); - - KDAV2::DavUrl mResourceUrl; }; class DavResourceFactory : public Sink::ResourceFactory diff --git a/examples/dummyresource/resourcefactory.cpp b/examples/dummyresource/resourcefactory.cpp index dffdfc9..f3c8be2 100644 --- a/examples/dummyresource/resourcefactory.cpp +++ b/examples/dummyresource/resourcefactory.cpp @@ -50,7 +50,7 @@ class DummySynchronizer : public Sink::Synchronizer { DummySynchronizer(const Sink::ResourceContext &context) : Sink::Synchronizer(context) { - + setSecret("dummy"); } Sink::ApplicationDomain::Event::Ptr createEvent(const QByteArray &ridBuffer, const QMap &data) diff --git a/examples/imapresource/imapresource.cpp b/examples/imapresource/imapresource.cpp index 3ae7fd7..2aba6b0 100644 --- a/examples/imapresource/imapresource.cpp +++ b/examples/imapresource/imapresource.cpp @@ -470,7 +470,7 @@ public: { SinkTrace() << "Connecting to:" << mServer << mPort; SinkTrace() << "as:" << mUser; - return imap->login(mUser, mPassword) + return imap->login(mUser, secret()) .addToContext(imap); } @@ -513,6 +513,8 @@ public: return {ApplicationDomain::NoServerError, error.errorMessage}; case Imap::ConnectionLost: return {ApplicationDomain::ConnectionLostError, error.errorMessage}; + case Imap::MissingCredentialsError: + return {ApplicationDomain::MissingCredentialsError, error.errorMessage}; default: return {ApplicationDomain::UnknownError, error.errorMessage}; } @@ -631,7 +633,7 @@ public: } } auto imap = QSharedPointer::create(mServer, mPort, &mSessionCache); - auto login = imap->login(mUser, mPassword); + auto login = imap->login(mUser, secret()); KAsync::Job job = KAsync::null(); if (operation == Sink::Operation_Creation) { const QString mailbox = syncStore().resolveLocalId(ENTITY_TYPE_FOLDER, mail.getFolder()); @@ -716,7 +718,7 @@ public: } } auto imap = QSharedPointer::create(mServer, mPort, &mSessionCache); - auto login = imap->login(mUser, mPassword); + auto login = imap->login(mUser, secret()); if (operation == Sink::Operation_Creation) { QString parentFolder; if (!folder.getParent().isEmpty()) { @@ -736,7 +738,7 @@ public: }); } else { //We try to merge special purpose folders first auto specialPurposeFolders = QSharedPointer>::create(); - auto mergeJob = imap->login(mUser, mPassword) + auto mergeJob = imap->login(mUser, secret()) .then(imap->fetchFolders([=](const Imap::Folder &folder) { if (SpecialPurpose::isSpecialPurposeFolderName(folder.name())) { specialPurposeFolders->insert(SpecialPurpose::getSpecialPurposeType(folder.name()), folder.path()); @@ -790,7 +792,6 @@ public: QString mServer; int mPort; QString mUser; - QString mPassword; int mDaysToSync = 0; QByteArray mResourceInstanceIdentifier; Imap::SessionCache mSessionCache; @@ -959,7 +960,6 @@ ImapResource::ImapResource(const ResourceContext &resourceContext) auto server = config.value("server").toString(); auto port = config.value("port").toInt(); auto user = config.value("username").toString(); - auto password = config.value("password").toString(); if (server.startsWith("imap")) { server.remove("imap://"); server.remove("imaps://"); @@ -974,7 +974,6 @@ ImapResource::ImapResource(const ResourceContext &resourceContext) synchronizer->mServer = server; synchronizer->mPort = port; synchronizer->mUser = user; - synchronizer->mPassword = password; synchronizer->mDaysToSync = 14; setupSynchronizer(synchronizer); @@ -982,7 +981,8 @@ ImapResource::ImapResource(const ResourceContext &resourceContext) inspector->mServer = server; inspector->mPort = port; inspector->mUser = user; - inspector->mPassword = password; + //TODO + // inspector->mPassword = password; setupInspector(inspector); setupPreprocessors(ENTITY_TYPE_MAIL, QVector() << new SpecialPurposeProcessor << new MailPropertyExtractor); diff --git a/examples/imapresource/imapserverproxy.cpp b/examples/imapresource/imapserverproxy.cpp index 16887b1..317fbdc 100644 --- a/examples/imapresource/imapserverproxy.cpp +++ b/examples/imapresource/imapserverproxy.cpp @@ -139,6 +139,9 @@ ImapServerProxy::ImapServerProxy(const QString &serverUrl, int port, SessionCach KAsync::Job ImapServerProxy::login(const QString &username, const QString &password) { + if (password.isEmpty()) { + return KAsync::error(Imap::MissingCredentialsError); + } if (mSessionCache) { auto session = mSessionCache->getSession(); if (session.isValid()) { diff --git a/examples/imapresource/imapserverproxy.h b/examples/imapresource/imapserverproxy.h index 86e3378..9e73f68 100644 --- a/examples/imapresource/imapserverproxy.h +++ b/examples/imapresource/imapserverproxy.h @@ -35,6 +35,7 @@ enum ErrorCode { CouldNotConnectError, SslHandshakeError, ConnectionLost, + MissingCredentialsError, UnknownError }; diff --git a/examples/maildirresource/maildirresource.cpp b/examples/maildirresource/maildirresource.cpp index b406f63..41f2433 100644 --- a/examples/maildirresource/maildirresource.cpp +++ b/examples/maildirresource/maildirresource.cpp @@ -215,7 +215,7 @@ public: MaildirSynchronizer(const Sink::ResourceContext &resourceContext) : Sink::Synchronizer(resourceContext) { - + setSecret("dummy"); } static QStringList listRecursive( const QString &root, const KPIM::Maildir &dir ) -- cgit v1.2.3