From kde-commits Thu Aug 13 08:41:35 2015 From: Valentin Rusu Date: Thu, 13 Aug 2015 08:41:35 +0000 To: kde-commits Subject: [ksecrets] /: Backend setup now works Message-Id: X-MARC-Message: https://marc.info/?l=kde-commits&m=143945530618161 Git commit fff18872cc08cb8bb9f8e1a0db61c70ca4b1d29b by Valentin Rusu. Committed on 13/08/2015 at 08:34. Pushed by vrusu into branch 'master'. Backend setup now works M +1 -3 autotests/api/CMakeLists.txt M +4 -1 autotests/ksecrets_store/CMakeLists.txt M +18 -7 autotests/ksecrets_store/ksecrets_store_test.cpp M +6 -2 src/runtime/ksecrets_store/ksecrets_credentials.cpp M +43 -39 src/runtime/ksecrets_store/ksecrets_store.cpp M +31 -22 src/runtime/ksecrets_store/ksecrets_store.h M +6 -4 src/runtime/ksecrets_store/ksecrets_store_p.h http://commits.kde.org/ksecrets/fff18872cc08cb8bb9f8e1a0db61c70ca4b1d29b diff --git a/autotests/api/CMakeLists.txt b/autotests/api/CMakeLists.txt index 6dc2343..9cc791c 100644 --- a/autotests/api/CMakeLists.txt +++ b/autotests/api/CMakeLists.txt @@ -4,8 +4,6 @@ include(ECMAddTests) = find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED Test) = -find_package(LibGcrypt 1.6.0 REQUIRED) - include_directories( ${CMAKE_SOURCE_DIR}/src/runtime/ksecrets_store ${CMAKE_BINARY_DIR}/src/runtime/ksecrets_store @@ -20,6 +18,6 @@ find_package(KF5 REQUIRED CoreAddons) = ecm_add_test( ksecretsservice-test.cpp - LINK_LIBRARIES KF5::Secrets Qt5::Test KF5::Secrets KF5::CoreAddons kse= crets_store ${LIBGCRYPT_LIBRARIES} keyutils + LINK_LIBRARIES Qt5::Test KF5::Secrets KF5::CoreAddons ksecrets_store k= eyutils ) = diff --git a/autotests/ksecrets_store/CMakeLists.txt b/autotests/ksecrets_s= tore/CMakeLists.txt index facb282..b0f916b 100644 --- a/autotests/ksecrets_store/CMakeLists.txt +++ b/autotests/ksecrets_store/CMakeLists.txt @@ -11,8 +11,11 @@ if(NOT Qt5Test_FOUND) return() endif() = +find_package(KF5CoreAddons ${KF5_DEP_VERSION} REQUIRED) +find_package(KF5Config ${KF5_DEP_VERSION} REQUIRED) + ecm_add_test( ksecrets_store_test.cpp - LINK_LIBRARIES Qt5::Test ksecrets_store + LINK_LIBRARIES Qt5::Test KF5::CoreAddons KF5::ConfigCore ksecrets_store ) = diff --git a/autotests/ksecrets_store/ksecrets_store_test.cpp b/autotests/k= secrets_store/ksecrets_store_test.cpp index 353f10a..9465e63 100644 --- a/autotests/ksecrets_store/ksecrets_store_test.cpp +++ b/autotests/ksecrets_store/ksecrets_store_test.cpp @@ -24,10 +24,13 @@ #include #include #include +#include +#include = QTEST_GUILESS_MAIN(KSecretServiceStoreTest); = -const char* test_file_path =3D "~/.qttest/ksecretsbackend-test.dat"; +KSharedConfig::Ptr sharedConfig; +QString secretsFilePath; = KSecretServiceStoreTest::KSecretServiceStoreTest(QObject* parent) : QObject(parent) @@ -36,21 +39,29 @@ KSecretServiceStoreTest::KSecretServiceStoreTest(QObjec= t* parent) = void KSecretServiceStoreTest::initTestCase() { + QStandardPaths::setTestModeEnabled(true); + sharedConfig =3D KSharedConfig::openConfig(QLatin1String("ksecretsrc")= ); + + secretsFilePath =3D QStandardPaths::writableLocation(QStandardPaths::D= ataLocation); + QVERIFY(QDir::home().mkpath(secretsFilePath)); + secretsFilePath +=3D QLatin1Literal("/ksecrets-test.data"); + qDebug() << "secrets store path: " << secretsFilePath; // create a test file here and performe the real open test below KSecretsStore backend; - auto setupfut =3D backend.setup(test_file_path, "test"); + auto setupfut =3D backend.setup(secretsFilePath.toLocal8Bit().constDat= a(), false); + QVERIFY(setupfut.get()); + auto credfut =3D backend.setCredentials("test", "ksecrets-test:crypt",= "ksecrets-test:mac"); + QVERIFY(credfut.get()); } = void KSecretServiceStoreTest::cleanupTestCase() { - QDir::home().remove(QLatin1Literal(test_file_path)); + QDir::home().remove(secretsFilePath); } = void KSecretServiceStoreTest::testOpen() { KSecretsStore backend; - auto setupfut =3D backend.setup(test_file_path, "test"); - auto openfut =3D backend.open(true); - auto openres =3D openfut.get(); - QVERIFY(openres.status_ =3D=3D KSecretsStore::StoreStatus::Good); + auto setupfut =3D backend.setup(secretsFilePath.toLocal8Bit().constDat= a()); + QVERIFY(setupfut.get()); } diff --git a/src/runtime/ksecrets_store/ksecrets_credentials.cpp b/src/runt= ime/ksecrets_store/ksecrets_credentials.cpp index 96e4107..b59352e 100644 --- a/src/runtime/ksecrets_store/ksecrets_credentials.cpp +++ b/src/runtime/ksecrets_store/ksecrets_credentials.cpp @@ -142,9 +142,13 @@ int KSECRETS_STORE_EXPORT kss_set_credentials(const ch= ar* user_name, const char* return TRUE; = KSecretsStore secretsStore; - auto setupres =3D secretsStore.setup(path, password); + auto setupres =3D secretsStore.setup(path); + if (!setupres.get()) { + return FALSE; + } = - return setupres.get() ? 1 : 0; + auto credres =3D secretsStore.setCredentials(password); + return credres.get() ? TRUE: FALSE; } = extern "C" diff --git a/src/runtime/ksecrets_store/ksecrets_store.cpp b/src/runtime/ks= ecrets_store/ksecrets_store.cpp index df43001..8da20a4 100644 --- a/src/runtime/ksecrets_store/ksecrets_store.cpp +++ b/src/runtime/ksecrets_store/ksecrets_store.cpp @@ -52,6 +52,7 @@ KSecretsStorePrivate::KSecretsStorePrivate(KSecretsStore*= b) : b_(b) { status_ =3D KSecretsStore::StoreStatus::JustCreated; + memset(&fileHead_, 0, sizeof(fileHead_)); } = KSecretsStore::KSecretsStore() @@ -61,11 +62,11 @@ KSecretsStore::KSecretsStore() = KSecretsStore::~KSecretsStore() =3D default; = -std::future KSecretsStore::setup(const char* p= ath, const char* password, const char* keyNameEcrypting, const char* keyNam= eMac) +std::future KSecretsStore::setup(const char* p= ath, bool readOnly /* =3D true */) { // sanity checks if (d->status_ !=3D StoreStatus::JustCreated) { - return std::async(std::launch::deferred, []() { return SetupResult= { StoreStatus::IncorrectState, -1 }; }); + return std::async(std::launch::deferred, []() { return SetupResult= ({ StoreStatus::IncorrectState, -1 }); }); } if (path =3D=3D nullptr || strlen(path) =3D=3D 0) { return std::async(std::launch::deferred, []() { return SetupResult= { StoreStatus::NoPathGiven, 0 }; }); @@ -75,7 +76,8 @@ std::future KSecretsStore::se= tup(const char* path, c struct stat buf; if (stat(path, &buf) !=3D 0) { auto err =3D errno; - if (ENOENT =3D=3D err) { + // when asking read only access, creating file is not possible + if (ENOENT =3D=3D err && !readOnly) { shouldCreateFile =3D true; } else { @@ -89,16 +91,12 @@ std::future KSecretsStore::= setup(const char* path, c } } = - ::keyNameEncrypting =3D keyNameEcrypting; - ::keyNameMac =3D keyNameMac; - auto localThis =3D this; std::string filePath =3D path; - std::string pwd =3D password; - return std::async(std::launch::async, [localThis, filePath, shouldCrea= teFile, pwd]() { return localThis->d->setup(filePath, shouldCreateFile, pwd= ); }); + return std::async(std::launch::async, [localThis, filePath, shouldCrea= teFile, readOnly]() { return localThis->d->setup(filePath, shouldCreateFile= , readOnly); }); } = -KSecretsStore::SetupResult KSecretsStorePrivate::setup(const std::string& = path, bool shouldCreateFile, const std::string& password) +KSecretsStore::SetupResult KSecretsStorePrivate::setup(const std::string& = path, bool shouldCreateFile, bool readOnly) { if (shouldCreateFile) { auto createres =3D createFile(path); @@ -106,21 +104,38 @@ KSecretsStore::SetupResult KSecretsStorePrivate::setu= p(const std::string& path, return setStoreStatus(KSecretsStore::SetupResult({ KSecretsSto= re::StoreStatus::SystemError, createres })); } } + secretsFile_.filePath_ =3D path; + secretsFile_.readOnly_ =3D readOnly; + return open(!readOnly); +} + +std::future KSecretsStore::setCredential= s(const char* password, const char* keyNameEncrypting, const char* keyNameM= ac) +{ + ::keyNameEncrypting =3D keyNameEncrypting; + ::keyNameMac =3D keyNameMac; + + std::string pwd =3D password; + auto localThis =3D this; + return std::async(std::launch::async, [localThis, pwd]() { return loca= lThis->d->setCredentials(pwd); }); +} + +KSecretsStore::CredentialsResult KSecretsStorePrivate::setCredentials(cons= t std::string& password) +{ + using Result =3D KSecretsStore::CredentialsResult; if (!kss_init_gcry()) { - return setStoreStatus(KSecretsStore::SetupResult({ KSecretsStore::= StoreStatus::CannotInitGcrypt, -1 })); + return setStoreStatus(Result({ KSecretsStore::StoreStatus::CannotI= nitGcrypt, -1 })); } // FIXME this should be adjusted on platforms where kernel keyring is = not available and store the keys elsewhere char encryption_key[KSECRETS_KEYSIZE]; char mac_key[KSECRETS_KEYSIZE]; if (!kss_derive_keys(salt(), password.c_str(), encryption_key, mac_key= , KSECRETS_KEYSIZE)) { - return setStoreStatus(KSecretsStore::SetupResult({ KSecretsStore::= StoreStatus::CannotDeriveKeys, -1 })); + return setStoreStatus(Result({ KSecretsStore::StoreStatus::CannotD= eriveKeys, -1 })); } = if (!kss_store_keys(encryption_key, mac_key, KSECRETS_KEYSIZE)) { - return setStoreStatus(KSecretsStore::SetupResult({ KSecretsStore::= StoreStatus::CannotStoreKeys, -1 })); + return setStoreStatus(Result({ KSecretsStore::StoreStatus::CannotS= toreKeys, errno })); } - secretsFile_.filePath_ =3D path; - return setStoreStatus(KSecretsStore::SetupResult({ KSecretsStore::Stor= eStatus::SetupDone, 0 })); + return setStoreStatus(Result({ KSecretsStore::StoreStatus::Credentials= Set, 0 })); } = char fileMagic[] =3D { 'k', 's', 'e', 'c', 'r', 'e', 't', 's' }; @@ -139,47 +154,35 @@ int KSecretsStorePrivate::createFile(const std::strin= g& path) gcry_randomize(emptyFileData.iv, IV_SIZE, GCRY_STRONG_RANDOM); = int res =3D 0; - if (fwrite(&emptyFileData, sizeof(emptyFileData), 1, f) !=3D sizeof(em= ptyFileData)) { - res =3D -1; // FIXME is errno available here? + if (fwrite(&emptyFileData, 1, sizeof(emptyFileData), f) !=3D sizeof(em= ptyFileData)) { + res =3D ferror(f); } fclose(f); return res; } = -std::future KSecretsStore::open(bool readonly /= * =3Dtrue */) noexcept +bool KSecretsStore::isGood() const noexcept { return d->status_ =3D=3D Sto= reStatus::Good; } + +const char* KSecretsStore::salt() const { - // check the state first - if (d->status_ =3D=3D StoreStatus::JustCreated) { - return std::async(std::launch::deferred, []() { return OpenResult{= StoreStatus::SetupShouldBeCalledFirst, -1 }; }); - } - if (d->status_ !=3D StoreStatus::SetupDone) { - // open could not be called two times or perhaps the setup call le= ft this store in an invalid state because of some error - return std::async(std::launch::deferred, []() { return OpenResult{= StoreStatus::IncorrectState, -1 }; }); - } - // now we can proceed - auto localThis =3D this; - if (!readonly) { - return std::async([localThis]() { return localThis->d->open(true);= }); - } - else { - return std::async([localThis]() { return localThis->d->open(false)= ; }); - } + if (isGood()) + return d->salt(); + else + return nullptr; } = -const char* KSecretsStore::salt() const { return d->salt(); } - const char* KSecretsStorePrivate::salt() const { return fileHead_.salt; } = -KSecretsStore::OpenResult KSecretsStorePrivate::open(bool lockFile) +KSecretsStore::SetupResult KSecretsStorePrivate::open(bool lockFile) { - using OpenResult =3D KSecretsStore::OpenResult; + using OpenResult =3D KSecretsStore::SetupResult; secretsFile_.file_ =3D ::open(secretsFile_.filePath_.c_str(), O_DSYNC = | O_NOATIME | O_NOFOLLOW); if (secretsFile_.file_ =3D=3D -1) { return setStoreStatus(OpenResult({ KSecretsStore::StoreStatus::Can= notOpenFile, errno })); } if (lockFile) { if (flock(secretsFile_.file_, LOCK_EX) =3D=3D -1) { - return KSecretsStore::OpenResult{ KSecretsStore::StoreStatus::= CannotLockFile, errno }; + return setStoreStatus(OpenResult({ KSecretsStore::StoreStatus:= :CannotLockFile, errno })); } secretsFile_.locked_ =3D true; } @@ -194,7 +197,8 @@ KSecretsStore::OpenResult KSecretsStorePrivate::open(bo= ol lockFile) return setStoreStatus(OpenResult({ KSecretsStore::StoreStatus::Good, 0= })); } = -KSecretsStorePrivate::SecretsFile::~SecretsFile(){ +KSecretsStorePrivate::SecretsFile::~SecretsFile() +{ if (file_ !=3D -1) { auto r =3D close(file_); if (r =3D=3D -1) { diff --git a/src/runtime/ksecrets_store/ksecrets_store.h b/src/runtime/ksec= rets_store/ksecrets_store.h index bb4f394..4d3e5b2 100644 --- a/src/runtime/ksecrets_store/ksecrets_store.h +++ b/src/runtime/ksecrets_store/ksecrets_store.h @@ -190,11 +190,11 @@ public: Good, JustCreated, SetupShouldBeCalledFirst, + CredentialsSet, IncorrectState, CannotInitGcrypt, CannotDeriveKeys, CannotStoreKeys, - SetupDone, SetupError, NoPathGiven, InvalidFile, // the file format was not recognized. Is this a ksec= rets file? @@ -204,38 +204,47 @@ public: SystemError }; = - struct SetupResult { + template + struct OpResult { StoreStatus status_; int errno_; - operator bool() const { return status_ =3D=3D StoreStatus::SetupDo= ne; } + operator bool() const { return status_ =3D=3D G; } }; = + using SetupResult =3D OpResult; + // struct SetupResult { + // StoreStatus status_; + // int errno_; + // operator bool() const { return status_ =3D=3D StoreStatus::Good= ; } + // }; + /* - * Before opening, the store must be setup, that is, it must know the - * file path and the user's password. This method is typically called - * from the pam module but it can also be called from another program - * after prompting user for the password, for example. - * - * This call creates the file if it's not found. + * Before usage, the store must be setup, that is, it must know its fi= le path. + * This call creates the file if it's not found and the readOnly flag = is set to false. + * The file is not created when the readOnly flag is set to false in o= rder to prevent + * unintended side effects. If the file don't exists, the SetupResult.= statu_ is set + * to StoreStatus::SystemError and errno should be 2 * * @return SetupResult whose operator bool could be used to check the = error condition */ - std::future setup(const char* path, const char* password,= const char* keyNameEcrypting =3D "ksecrets:encrypting", const char* keyNam= eMac =3D "ksecrets:mac"); - - struct OpenResult { - StoreStatus status_; - int errno_; - operator bool() const { return status_ =3D=3D StoreStatus::Good; } - }; - + std::future setup(const char* path, bool readOnly =3Dtrue= ); + + using CredentialsResult =3D OpResult; + // struct CredentialsResult { + // StoreStatus status_; + // int errno_; + // operator bool() const { return status_ =3D=3D StoreStatus::Good= ; } + // }; /** - * @parameter path full path to the store file to handle - * @parameter readOnly set it to true if you only intend reading to sp= eed - * things-up and avoid placing a lock on the file + * Set the system-wide credentials for the secrets store * - * @return OpenResult whose operator bool could be used to check the e= rror condition + * This method is typically called from the pam module but it can also= be + * called from another program after prompting user for the password, = for example. + * Client applications only need to call this once per user session. */ - std::future open(bool readOnly =3D true) noexcept; + std::future setCredentials(const char* password =3D= nullptr, const char* keyNameEcrypting =3D "ksecrets:encrypting", const cha= r* keyNameMac =3D "ksecrets:mac"); + + bool isGood() const noexcept; = constexpr static auto SALT_SIZE =3D 56; /** diff --git a/src/runtime/ksecrets_store/ksecrets_store_p.h b/src/runtime/ks= ecrets_store/ksecrets_store_p.h index 4ec75c8..270763d 100644 --- a/src/runtime/ksecrets_store/ksecrets_store_p.h +++ b/src/runtime/ksecrets_store/ksecrets_store_p.h @@ -58,9 +58,9 @@ public: KSecretsStorePrivate() =3D delete; explicit KSecretsStorePrivate(KSecretsStore*); = - KSecretsStore::SetupResult setup(const std::string& path, bool, const = std::string&); - KSecretsStore::OpenResult open(bool lock); - using open_action =3D std::function; + KSecretsStore::SetupResult setup(const std::string& path, bool, bool); + KSecretsStore::CredentialsResult setCredentials(const std::string&); + KSecretsStore::SetupResult open(bool); int createFile(const std::string&); const char* salt() const; = @@ -80,11 +80,13 @@ public: = struct SecretsFile { SecretsFile() - : file_(-1), locked_(false) {}; + : file_(-1) + , locked_(false){}; ~SecretsFile(); std::string filePath_; int file_; bool locked_; + bool readOnly_; }; KSecretsStore* b_; SecretsFile secretsFile_;