diff --git a/README.md b/README.md index 7cd2e14..68580c8 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,5 @@ Some todo items include, - QR code scanning - Backup and Restore of accounts - - Encrypted storage of the secret token This code is largely based on the [authenticator-ng](https://github.com/dobey/authenticator-ng) application by the Rodney Dawes and Michael Zanetti for the Ubuntu Touch. diff --git a/autotests/account/compute-jobs/compute-hotp.cpp b/autotests/account/compute-jobs/compute-hotp.cpp index 134961b..aafaddb 100644 --- a/autotests/account/compute-jobs/compute-hotp.cpp +++ b/autotests/account/compute-jobs/compute-hotp.cpp @@ -4,6 +4,7 @@ */ #include "account/actions_p.h" +#include "../test-utils/secret.h" #include "../test-utils/spy.h" #include @@ -13,24 +14,32 @@ class ComputeHotpTest: public QObject { Q_OBJECT private Q_SLOTS: + void initTestCase(void); void testDefaults(void); void testDefaults_data(void); +private: + accounts::AccountSecret m_secret; }; -/* - * RFC test vector uses the key: 12345678901234567890 - * The secret value below is the bas32 encoded version of that - */ -static QLatin1String secret("GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ"); +// RFC test vector uses the key: 12345678901234567890 +static QByteArray rfcSecret("12345678901234567890"); // the RFC test vector consists of 6-character tokens static int tokenLength = 6; +void ComputeHotpTest::initTestCase(void) +{ + QVERIFY2(test::useDummyPassword(&m_secret), "should be able to set up the master key"); +} + void ComputeHotpTest::testDefaults(void) { QFETCH(quint64, counter); - accounts::ComputeHotp uut(secret, counter, tokenLength); + std::optional tokenSecret = test::encrypt(&m_secret, rfcSecret); + QVERIFY2(tokenSecret, "should be able to encrypt the token secret"); + + accounts::ComputeHotp uut(&m_secret, *tokenSecret, counter, tokenLength); QSignalSpy tokenGenerated(&uut, &accounts::ComputeHotp::otp); QSignalSpy jobFinished(&uut, &accounts::ComputeHotp::finished); diff --git a/autotests/account/compute-jobs/compute-totp.cpp b/autotests/account/compute-jobs/compute-totp.cpp index 6b00a88..4fe9df1 100644 --- a/autotests/account/compute-jobs/compute-totp.cpp +++ b/autotests/account/compute-jobs/compute-totp.cpp @@ -4,6 +4,7 @@ */ #include "account/actions_p.h" +#include "../test-utils/secret.h" #include "../test-utils/spy.h" #include @@ -13,15 +14,15 @@ class ComputeTotpTest: public QObject { Q_OBJECT private Q_SLOTS: + void initTestCase(void); void testDefaults(void); void testDefaults_data(void); +private: + accounts::AccountSecret m_secret; }; -/* - * RFC test vector uses the key: 12345678901234567890 - * The secret value below is the bas32 encoded version of that - */ -static QLatin1String secret("GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ"); +// RFC test vector uses the key: 12345678901234567890 +static QByteArray rfcSecret("12345678901234567890"); // the RFC test vector consists of 6-character tokens static int tokenLength = 6; @@ -32,6 +33,11 @@ static uint timeStep = 30; // the default TOTP epoch is the Unix epoch static QDateTime epoch = QDateTime::fromMSecsSinceEpoch(0); +void ComputeTotpTest::initTestCase(void) +{ + QVERIFY2(test::useDummyPassword(&m_secret), "should be able to set up the master key"); +} + void ComputeTotpTest::testDefaults(void) { QFETCH(qint64, counter); @@ -39,7 +45,10 @@ void ComputeTotpTest::testDefaults(void) return counter * timeStep * 1000; }); - accounts::ComputeTotp uut(secret, epoch, timeStep, tokenLength, accounts::Account::Hash::Default, clock); + std::optional tokenSecret = test::encrypt(&m_secret, rfcSecret); + QVERIFY2(tokenSecret, "should be able to encrypt the token secret"); + + accounts::ComputeTotp uut(&m_secret, *tokenSecret, epoch, timeStep, tokenLength, accounts::Account::Hash::Default, clock); QSignalSpy tokenGenerated(&uut, &accounts::ComputeTotp::otp); QSignalSpy jobFinished(&uut, &accounts::ComputeTotp::finished); diff --git a/autotests/account/file-jobs/CMakeLists.txt b/autotests/account/file-jobs/CMakeLists.txt index 38707d8..b22804b 100644 --- a/autotests/account/file-jobs/CMakeLists.txt +++ b/autotests/account/file-jobs/CMakeLists.txt @@ -3,7 +3,7 @@ # SPDX-FileCopyrightText: 2020 Johan Ouwerkerk # -set(Test_DEP_LIBS Qt5::Core Qt5::Test account_lib account_test_lib) +set(Test_DEP_LIBS Qt5::Core Qt5::Test account_lib account_test_lib secrets_test_lib) qt5_add_resources(RCC_SOURCES resources/resources.qrc) ecm_add_test(load-accounts.cpp ${RCC_SOURCES} LINK_LIBRARIES ${Test_DEP_LIBS} TEST_NAME load-accounts NAME_PREFIX account-jobs-) diff --git a/autotests/account/file-jobs/load-accounts.cpp b/autotests/account/file-jobs/load-accounts.cpp index 0a3d6eb..501c1a3 100644 --- a/autotests/account/file-jobs/load-accounts.cpp +++ b/autotests/account/file-jobs/load-accounts.cpp @@ -5,8 +5,11 @@ #include "account/actions_p.h" #include "../test-utils/output.h" +#include "../test-utils/secret.h" #include "../test-utils/spy.h" +#include "../../secrets/test-utils/random.h" + #include #include #include @@ -23,23 +26,33 @@ private Q_SLOTS: void initTestCase(void); void emptyAccountsFile(void); void sampleAccountsFile(void); +private: + accounts::AccountSecret m_secret {&test::fakeRandom}; }; +static QByteArray rawSecret = QByteArray::fromBase64(QByteArray("8juE9gJFLp3OgL4CxJ5v5q8sw+h7Vbn06+NY4uc="), QByteArray::Base64Encoding); +static QByteArray rawNonce = QByteArray::fromBase64(QByteArray("QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB"), QByteArray::Base64Encoding); + void LoadAccountsTest::initTestCase(void) { QVERIFY2(test::ensureOutputDirectory(), "output directory should be available"); QVERIFY2(test::copyResource(":/load-accounts/empty-accounts.ini", emptyIniResource), "empty INI resource should be available as file"); QVERIFY2(test::copyResource(":/load-accounts/sample-accounts.ini", corpusIniResource), "test corpus INI resource should be available as file"); + QVERIFY2(test::useDummyPassword(&m_secret), "should be able to set up the master key"); } void LoadAccountsTest::emptyAccountsFile(void) { - accounts::LoadAccounts uut([](const accounts::PersistenceAction &action) -> void + bool actionRun = false; + const accounts::SettingsProvider settings([&actionRun](const accounts::PersistenceAction &action) -> void { QSettings data(test::path(emptyIniResource), QSettings::IniFormat); + actionRun = true; action(data); }); + accounts::LoadAccounts uut(settings, &m_secret); + QSignalSpy hotpFound(&uut, &accounts::LoadAccounts::foundHotp); QSignalSpy totpFound(&uut, &accounts::LoadAccounts::foundTotp); QSignalSpy jobFinished(&uut, &accounts::LoadAccounts::finished); @@ -47,18 +60,23 @@ void LoadAccountsTest::emptyAccountsFile(void) uut.run(); QVERIFY2(test::signal_eventually_emitted_once(jobFinished), "job should be finished"); + QVERIFY2(actionRun, "accounts action should have run"); QCOMPARE(hotpFound.count(), 0); QCOMPARE(totpFound.count(), 0); } void LoadAccountsTest::sampleAccountsFile(void) { - accounts::LoadAccounts uut([](const accounts::PersistenceAction &action) -> void + bool actionRun = false; + const accounts::SettingsProvider settings([&actionRun](const accounts::PersistenceAction &action) -> void { QSettings data(test::path(corpusIniResource), QSettings::IniFormat); + actionRun = true; action(data); }); + accounts::LoadAccounts uut(settings, &m_secret); + QSignalSpy hotpFound(&uut, &accounts::LoadAccounts::foundHotp); QSignalSpy totpFound(&uut, &accounts::LoadAccounts::foundTotp); QSignalSpy jobFinished(&uut, &accounts::LoadAccounts::finished); @@ -66,22 +84,25 @@ void LoadAccountsTest::sampleAccountsFile(void) uut.run(); QVERIFY2(test::signal_eventually_emitted_once(jobFinished), "job should be finished"); + QVERIFY2(actionRun, "accounts action should have run"); QCOMPARE(hotpFound.count(), 1); QCOMPARE(totpFound.count(), 1); const auto firstHotp = hotpFound.at(0); QCOMPARE(firstHotp.at(0).toUuid(), QUuid(QLatin1String("072a645d-6c26-57cc-81eb-d9ef3b9b39e2"))); QCOMPARE(firstHotp.at(1).toString(), QLatin1String("valid-hotp-sample-1")); - QCOMPARE(firstHotp.at(2).toString(), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ=")); - QCOMPARE(firstHotp.at(3).toULongLong(), 0ULL); - QCOMPARE(firstHotp.at(4).toInt(), 6); + QCOMPARE(firstHotp.at(2).toByteArray(), rawSecret); + QCOMPARE(firstHotp.at(3).toByteArray(), rawNonce); + QCOMPARE(firstHotp.at(4).toULongLong(), 0ULL); + QCOMPARE(firstHotp.at(5).toInt(), 6); const auto firstTotp = totpFound.at(0); QCOMPARE(firstTotp.at(0).toUuid(), QUuid(QLatin1String("534cc72e-e9ec-5e39-a1ff-9f017c9be8cc"))); QCOMPARE(firstTotp.at(1).toString(), QLatin1String("valid-totp-sample-1")); - QCOMPARE(firstTotp.at(2).toString(), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ=")); - QCOMPARE(firstTotp.at(3).toUInt(), 30); - QCOMPARE(firstTotp.at(4).toInt(), 6); + QCOMPARE(firstHotp.at(2).toByteArray(), rawSecret); + QCOMPARE(firstHotp.at(3).toByteArray(), rawNonce); + QCOMPARE(firstTotp.at(4).toUInt(), 30); + QCOMPARE(firstTotp.at(5).toInt(), 6); } QTEST_MAIN(LoadAccountsTest) diff --git a/autotests/account/file-jobs/resources/load-accounts/sample-accounts.ini b/autotests/account/file-jobs/resources/load-accounts/sample-accounts.ini index 7dc3509..c500cf4 100644 --- a/autotests/account/file-jobs/resources/load-accounts/sample-accounts.ini +++ b/autotests/account/file-jobs/resources/load-accounts/sample-accounts.ini @@ -1,13 +1,15 @@ [%7B072a645d-6c26-57cc-81eb-d9ef3b9b39e2%7D] account=valid-hotp-sample-1 counter=0 +nonce=QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB pinLength=6 +secret="8juE9gJFLp3OgL4CxJ5v5q8sw+h7Vbn06+NY4uc=" type=hotp -secret="NBSWY3DPFQQHO33SNRSCCCQ=" [%7B534cc72e-e9ec-5e39-a1ff-9f017c9be8cc%7D] account=valid-totp-sample-1 -timeStep=30 +nonce=QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB pinLength=6 +secret="8juE9gJFLp3OgL4CxJ5v5q8sw+h7Vbn06+NY4uc=" +timeStep=30 type=totp -secret="NBSWY3DPFQQHO33SNRSCCCQ=" diff --git a/autotests/account/file-jobs/resources/save-hotp/expected-accounts.ini b/autotests/account/file-jobs/resources/save-hotp/expected-accounts.ini index dd25b39..782f619 100644 --- a/autotests/account/file-jobs/resources/save-hotp/expected-accounts.ini +++ b/autotests/account/file-jobs/resources/save-hotp/expected-accounts.ini @@ -1,6 +1,7 @@ [%7B072a645d-6c26-57cc-81eb-d9ef3b9b39e2%7D] account=valid-hotp-sample-1 counter=0 +nonce=QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB pinLength=6 -secret="NBSWY3DPFQQHO33SNRSCCCQ=" +secret="8juE9gJFLp3OgL4CxJ5v5q8sw+h7Vbn06+NY4uc=" type=hotp diff --git a/autotests/account/file-jobs/resources/save-totp/expected-accounts.ini b/autotests/account/file-jobs/resources/save-totp/expected-accounts.ini index 6f16127..6e7bf95 100644 --- a/autotests/account/file-jobs/resources/save-totp/expected-accounts.ini +++ b/autotests/account/file-jobs/resources/save-totp/expected-accounts.ini @@ -1,6 +1,7 @@ [%7B534cc72e-e9ec-5e39-a1ff-9f017c9be8cc%7D] account=valid-totp-sample-1 +nonce=QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB pinLength=6 -secret="NBSWY3DPFQQHO33SNRSCCCQ=" +secret="8juE9gJFLp3OgL4CxJ5v5q8sw+h7Vbn06+NY4uc=" timeStep=30 type=totp diff --git a/autotests/account/file-jobs/save-hotp.cpp b/autotests/account/file-jobs/save-hotp.cpp index 1f6fdaa..c477772 100644 --- a/autotests/account/file-jobs/save-hotp.cpp +++ b/autotests/account/file-jobs/save-hotp.cpp @@ -5,8 +5,11 @@ #include "account/actions_p.h" #include "../test-utils/output.h" +#include "../test-utils/secret.h" #include "../test-utils/spy.h" +#include "../../secrets/test-utils/random.h" + #include #include #include @@ -23,27 +26,27 @@ private Q_SLOTS: void validHotp_data(void); void invalidHotp(void); void invalidHotp_data(void); +private: + accounts::AccountSecret m_secret {&test::fakeRandom}; }; static void define_test_data(void) { QTest::addColumn("id"); QTest::addColumn("name"); - QTest::addColumn("secret"); QTest::addColumn("counter"); QTest::addColumn("tokenLength"); } -static void define_test_case(const char * label, const QUuid &id, const QString &accountName, const QString &secret, quint64 counter, int tokenLength) +static void define_test_case(const char * label, const QUuid &id, const QString &accountName, quint64 counter, int tokenLength) { - QTest::newRow(label) << id << accountName << secret << counter << tokenLength; + QTest::newRow(label) << id << accountName << counter << tokenLength; } void SaveHotpTest::validHotp(void) { QFETCH(QUuid, id); QFETCH(QString, name); - QFETCH(QString, secret); QFETCH(quint64, counter); QFETCH(int, tokenLength); @@ -58,7 +61,10 @@ void SaveHotpTest::validHotp(void) action(data); }); - accounts::SaveHotp uut(settings, id, name, secret, counter, tokenLength); + std::optional tokenSecret = test::encrypt(&m_secret, QByteArray("Hello, world!")); + QVERIFY2(tokenSecret, "should be able to encrypt the token secret"); + + accounts::SaveHotp uut(settings, id, name, *tokenSecret, counter, tokenLength); QSignalSpy invalidAccount(&uut, &accounts::SaveHotp::invalid); QSignalSpy savedAccount(&uut, &accounts::SaveHotp::saved); QSignalSpy jobFinished(&uut, &accounts::SaveHotp::finished); @@ -84,7 +90,6 @@ void SaveHotpTest::invalidHotp(void) { QFETCH(QUuid, id); QFETCH(QString, name); - QFETCH(QString, secret); QFETCH(quint64, counter); QFETCH(int, tokenLength); @@ -99,7 +104,10 @@ void SaveHotpTest::invalidHotp(void) action(data); }); - accounts::SaveHotp uut(settings, id, name, secret, counter, tokenLength); + std::optional tokenSecret = test::encrypt(&m_secret, QByteArray("Hello, world!")); + QVERIFY2(tokenSecret, "should be able to encrypt the token secret"); + + accounts::SaveHotp uut(settings, id, name, *tokenSecret, counter, tokenLength); QSignalSpy invalidAccount(&uut, &accounts::SaveHotp::invalid); QSignalSpy savedAccount(&uut, &accounts::SaveHotp::saved); QSignalSpy jobFinished(&uut, &accounts::SaveHotp::finished); @@ -121,24 +129,23 @@ void SaveHotpTest::invalidHotp(void) void SaveHotpTest::validHotp_data(void) { define_test_data(); - define_test_case("valid-hotp-sample-1", QUuid("072a645d-6c26-57cc-81eb-d9ef3b9b39e2"), QLatin1String("valid-hotp-sample-1"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 0, 6); + define_test_case("valid-hotp-sample-1", QUuid("072a645d-6c26-57cc-81eb-d9ef3b9b39e2"), QLatin1String("valid-hotp-sample-1"), 0, 6); } void SaveHotpTest::invalidHotp_data(void) { define_test_data(); - define_test_case("null UUID", QUuid(), QLatin1String("null UUID"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 0, 6); - define_test_case("null account name", QUuid("00611bbf-5e0b-5c6a-9847-ad865315ce86"), QString(), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 0, 6); - define_test_case("empty account name", QUuid("1e42b907-99d8-5da3-a59b-89b257e49c83"), QLatin1String(""), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 0, 6); - define_test_case("null secret", QUuid("6e5ba95c-984d-538c-844e-f9edc1341bd2"), QLatin1String("null secret"), QString(), 0, 6); - define_test_case("empty secret", QUuid("fe68a65e-287e-5dcd-909b-1837d7ab94ee"), QLatin1String("empty secret"), QLatin1String(""), 0, 6); - define_test_case("tokenLength too small", QUuid("bca12e13-4b5b-5e4e-b162-3b86a6284dea"), QLatin1String("tokenLength too small"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 0, 5); - define_test_case("tokenLength too large", QUuid("5c10d530-fb22-5438-848d-3d4d1f738610"), QLatin1String("tokenLength too large"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 0, 11); + define_test_case("null UUID", QUuid(), QLatin1String("null UUID"), 0, 6); + define_test_case("null account name", QUuid("00611bbf-5e0b-5c6a-9847-ad865315ce86"), QString(), 0, 6); + define_test_case("empty account name", QUuid("1e42b907-99d8-5da3-a59b-89b257e49c83"), QLatin1String(""), 0, 6); + define_test_case("tokenLength too small", QUuid("bca12e13-4b5b-5e4e-b162-3b86a6284dea"), QLatin1String("tokenLength too small"), 0, 5); + define_test_case("tokenLength too large", QUuid("5c10d530-fb22-5438-848d-3d4d1f738610"), QLatin1String("tokenLength too large"), 0, 11); } void SaveHotpTest::initTestCase(void) { QVERIFY2(test::ensureOutputDirectory(), "output directory should be available"); + QVERIFY2(test::useDummyPassword(&m_secret), "should be able to set up the master key"); } QTEST_MAIN(SaveHotpTest) diff --git a/autotests/account/file-jobs/save-totp.cpp b/autotests/account/file-jobs/save-totp.cpp index 1f8b95f..f8eb125 100644 --- a/autotests/account/file-jobs/save-totp.cpp +++ b/autotests/account/file-jobs/save-totp.cpp @@ -5,8 +5,11 @@ #include "account/actions_p.h" #include "../test-utils/output.h" +#include "../test-utils/secret.h" #include "../test-utils/spy.h" +#include "../../secrets/test-utils/random.h" + #include #include #include @@ -23,27 +26,27 @@ private Q_SLOTS: void validHotp_data(void); void invalidHotp(void); void invalidHotp_data(void); +private: + accounts::AccountSecret m_secret {&test::fakeRandom}; }; static void define_test_data(void) { QTest::addColumn("id"); QTest::addColumn("name"); - QTest::addColumn("secret"); QTest::addColumn("timeStep"); QTest::addColumn("tokenLength"); } -static void define_test_case(const char * label, const QUuid &id, const QString &accountName, const QString &secret, uint timeStep, int tokenLength) +static void define_test_case(const char * label, const QUuid &id, const QString &accountName, uint timeStep, int tokenLength) { - QTest::newRow(label) << id << accountName << secret << timeStep << tokenLength; + QTest::newRow(label) << id << accountName << timeStep << tokenLength; } void SaveTotpTest::validHotp(void) { QFETCH(QUuid, id); QFETCH(QString, name); - QFETCH(QString, secret); QFETCH(uint, timeStep); QFETCH(int, tokenLength); @@ -58,7 +61,10 @@ void SaveTotpTest::validHotp(void) action(data); }); - accounts::SaveTotp uut(settings, id, name, secret, timeStep, tokenLength); + std::optional tokenSecret = test::encrypt(&m_secret, QByteArray("Hello, world!")); + QVERIFY2(tokenSecret, "should be able to encrypt the token secret"); + + accounts::SaveTotp uut(settings, id, name, *tokenSecret, timeStep, tokenLength); QSignalSpy invalidAccount(&uut, &accounts::SaveTotp::invalid); QSignalSpy savedAccount(&uut, &accounts::SaveTotp::saved); QSignalSpy jobFinished(&uut, &accounts::SaveTotp::finished); @@ -84,7 +90,6 @@ void SaveTotpTest::invalidHotp(void) { QFETCH(QUuid, id); QFETCH(QString, name); - QFETCH(QString, secret); QFETCH(uint, timeStep); QFETCH(int, tokenLength); @@ -99,7 +104,10 @@ void SaveTotpTest::invalidHotp(void) action(data); }); - accounts::SaveTotp uut(settings, id, name, secret, timeStep, tokenLength); + std::optional tokenSecret = test::encrypt(&m_secret, QByteArray("Hello, world!")); + QVERIFY2(tokenSecret, "should be able to encrypt the token secret"); + + accounts::SaveTotp uut(settings, id, name, *tokenSecret, timeStep, tokenLength); QSignalSpy invalidAccount(&uut, &accounts::SaveTotp::invalid); QSignalSpy savedAccount(&uut, &accounts::SaveTotp::saved); QSignalSpy jobFinished(&uut, &accounts::SaveTotp::finished); @@ -121,25 +129,24 @@ void SaveTotpTest::invalidHotp(void) void SaveTotpTest::validHotp_data(void) { define_test_data(); - define_test_case("valid-totp-sample-1", QUuid("534cc72e-e9ec-5e39-a1ff-9f017c9be8cc"), QLatin1String("valid-totp-sample-1"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 30, 6); + define_test_case("valid-totp-sample-1", QUuid("534cc72e-e9ec-5e39-a1ff-9f017c9be8cc"), QLatin1String("valid-totp-sample-1"), 30, 6); } void SaveTotpTest::invalidHotp_data(void) { define_test_data(); - define_test_case("null UUID", QUuid(), QLatin1String("null UUID"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 30, 6); - define_test_case("null account name", QUuid("00611bbf-5e0b-5c6a-9847-ad865315ce86"), QString(), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 30, 6); - define_test_case("empty account name", QUuid("1e42b907-99d8-5da3-a59b-89b257e49c83"), QLatin1String(""), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 30, 6); - define_test_case("null secret", QUuid("6e5ba95c-984d-538c-844e-f9edc1341bd2"), QLatin1String("null secret"), QString(), 30, 6); - define_test_case("empty secret", QUuid("fe68a65e-287e-5dcd-909b-1837d7ab94ee"), QLatin1String("empty secret"), QLatin1String(""), 30, 6); - define_test_case("timeStep too small", QUuid("5ab8749b-f973-5f48-a70e-c261ebd0521a"), QLatin1String("timeStep too small"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 0, 6); - define_test_case("tokenLength too small", QUuid("bca12e13-4b5b-5e4e-b162-3b86a6284dea"), QLatin1String("tokenLength too small"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 30, 5); - define_test_case("tokenLength too large", QUuid("5c10d530-fb22-5438-848d-3d4d1f738610"), QLatin1String("tokenLength too large"), QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 30, 11); + define_test_case("null UUID", QUuid(), QLatin1String("null UUID"), 30, 6); + define_test_case("null account name", QUuid("00611bbf-5e0b-5c6a-9847-ad865315ce86"), QString(), 30, 6); + define_test_case("empty account name", QUuid("1e42b907-99d8-5da3-a59b-89b257e49c83"), QLatin1String(""), 30, 6); + define_test_case("timeStep too small", QUuid("5ab8749b-f973-5f48-a70e-c261ebd0521a"), QLatin1String("timeStep too small"), 0, 6); + define_test_case("tokenLength too small", QUuid("bca12e13-4b5b-5e4e-b162-3b86a6284dea"), QLatin1String("tokenLength too small"), 30, 5); + define_test_case("tokenLength too large", QUuid("5c10d530-fb22-5438-848d-3d4d1f738610"), QLatin1String("tokenLength too large"), 30, 11); } void SaveTotpTest::initTestCase(void) { QVERIFY2(test::ensureOutputDirectory(), "output directory should be available"); + QVERIFY2(test::useDummyPassword(&m_secret), "should be able to set up the master key"); } QTEST_MAIN(SaveTotpTest) diff --git a/autotests/account/storage/CMakeLists.txt b/autotests/account/storage/CMakeLists.txt index ccc0b07..c15b012 100644 --- a/autotests/account/storage/CMakeLists.txt +++ b/autotests/account/storage/CMakeLists.txt @@ -3,7 +3,7 @@ # SPDX-FileCopyrightText: 2020 Johan Ouwerkerk # -set(Test_DEP_LIBS Qt5::Core Qt5::Test account_lib account_test_lib) +set(Test_DEP_LIBS Qt5::Core Qt5::Test account_lib account_test_lib secrets_test_lib) qt5_add_resources(RCC_SOURCES resources/resources.qrc) ecm_add_test(storage-object-lifecycles.cpp ${RCC_SOURCES} LINK_LIBRARIES ${Test_DEP_LIBS} TEST_NAME storage-object-lifecycles NAME_PREFIX account-) diff --git a/autotests/account/storage/hotp-counter-update.cpp b/autotests/account/storage/hotp-counter-update.cpp index e93ab70..e63f484 100644 --- a/autotests/account/storage/hotp-counter-update.cpp +++ b/autotests/account/storage/hotp-counter-update.cpp @@ -7,6 +7,8 @@ #include "../test-utils/output.h" #include "../test-utils/spy.h" +#include "../../secrets/test-utils/random.h" + #include #include #include @@ -53,13 +55,7 @@ void HotpCounterUpdateTest::testCounterUpdate(void) thread->start(); QVERIFY2(test::signal_eventually_emitted_once(threadStarted), "worker thread should be running by now"); - accounts::AccountStorage *uut = new accounts::AccountStorage(settings, thread); - QSignalSpy accountAdded(uut, &accounts::AccountStorage::added); - QSignalSpy accountRemoved(uut, &accounts::AccountStorage::removed); - QSignalSpy storageDisposed(uut, &accounts::AccountStorage::disposed); - QSignalSpy storageCleaned(uut, &accounts::AccountStorage::destroyed); - - accounts::AccountSecret *secret = uut->secret(); + accounts::AccountSecret *secret = new accounts::AccountSecret(&test::fakeRandom); QSignalSpy existingPasswordNeeded(secret, &accounts::AccountSecret::existingPasswordNeeded); QSignalSpy newPasswordNeeded(secret, &accounts::AccountSecret::newPasswordNeeded); QSignalSpy passwordAvailable(secret, &accounts::AccountSecret::passwordAvailable); @@ -67,6 +63,12 @@ void HotpCounterUpdateTest::testCounterUpdate(void) QSignalSpy passwordRequestsCancelled(secret, &accounts::AccountSecret::requestsCancelled); QSignalSpy secretCleaned(secret, &accounts::AccountSecret::destroyed); + accounts::AccountStorage *uut = new accounts::AccountStorage(settings, thread, secret); + QSignalSpy accountAdded(uut, &accounts::AccountStorage::added); + QSignalSpy accountRemoved(uut, &accounts::AccountStorage::removed); + QSignalSpy storageDisposed(uut, &accounts::AccountStorage::disposed); + QSignalSpy storageCleaned(uut, &accounts::AccountStorage::destroyed); + // first phase: check that account objects can be loaded from storage // expect that unlocking is scheduled automatically, so advancing the event loop should trigger the signal diff --git a/autotests/account/storage/resources/counter-update/after-updating-counter.ini b/autotests/account/storage/resources/counter-update/after-updating-counter.ini index c49ca32..22d4100 100644 --- a/autotests/account/storage/resources/counter-update/after-updating-counter.ini +++ b/autotests/account/storage/resources/counter-update/after-updating-counter.ini @@ -8,6 +8,7 @@ salt="MDEyMzQ1Njc4OUFCQ0RFRg==" [%7B072a645d-6c26-57cc-81eb-d9ef3b9b39e2%7D] account=valid-hotp-sample-1 counter=1 +nonce=QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB pinLength=6 -secret=GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ +secret=/8zRRlAF80eHEtU/ZJEFbR4nIeuMVs4YBRHvxRSod8iQculp type=hotp diff --git a/autotests/account/storage/resources/counter-update/starting.ini b/autotests/account/storage/resources/counter-update/starting.ini index 4bf97fb..c219f8a 100644 --- a/autotests/account/storage/resources/counter-update/starting.ini +++ b/autotests/account/storage/resources/counter-update/starting.ini @@ -8,6 +8,7 @@ salt="MDEyMzQ1Njc4OUFCQ0RFRg==" [%7B072a645d-6c26-57cc-81eb-d9ef3b9b39e2%7D] account=valid-hotp-sample-1 counter=0 +nonce=QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB pinLength=6 -secret=GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ +secret=/8zRRlAF80eHEtU/ZJEFbR4nIeuMVs4YBRHvxRSod8iQculp type=hotp diff --git a/autotests/account/storage/resources/storage-lifecycles/after-adding.ini b/autotests/account/storage/resources/storage-lifecycles/after-adding.ini index 5f46c1d..2cfa145 100644 --- a/autotests/account/storage/resources/storage-lifecycles/after-adding.ini +++ b/autotests/account/storage/resources/storage-lifecycles/after-adding.ini @@ -7,7 +7,8 @@ salt="MDEyMzQ1Njc4OUFCQ0RFRg==" [%7B534cc72e-e9ec-5e39-a1ff-9f017c9be8cc%7D] account=valid-totp-sample-1 +nonce=QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB pinLength=8 -secret="NBSWY3DPFQQHO33SNRSCCCQ=" +secret="LXM8veM3T1qY/gAYsTGZNEdwfrPWTNlXU1OykwY=" timeStep=42 type=totp diff --git a/autotests/account/storage/resources/storage-lifecycles/starting.ini b/autotests/account/storage/resources/storage-lifecycles/starting.ini index 5691a4e..54777e9 100644 --- a/autotests/account/storage/resources/storage-lifecycles/starting.ini +++ b/autotests/account/storage/resources/storage-lifecycles/starting.ini @@ -8,6 +8,7 @@ salt="MDEyMzQ1Njc4OUFCQ0RFRg==" [%7B072a645d-6c26-57cc-81eb-d9ef3b9b39e2%7D] account=valid-hotp-sample-1 counter=42 +nonce=QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFB pinLength=7 -secret="NBSWY3DPFQQHO33SNRSCCCQ=" +secret="LXM8veM3T1qY/gAYsTGZNEdwfrPWTNlXU1OykwY=" type=hotp diff --git a/autotests/account/storage/storage-object-lifecycles.cpp b/autotests/account/storage/storage-object-lifecycles.cpp index 788e03d..9890085 100644 --- a/autotests/account/storage/storage-object-lifecycles.cpp +++ b/autotests/account/storage/storage-object-lifecycles.cpp @@ -7,6 +7,8 @@ #include "../test-utils/output.h" #include "../test-utils/spy.h" +#include "../../secrets/test-utils/random.h" + #include #include #include @@ -15,6 +17,8 @@ #include #include +#include + static QString testIniResource(QLatin1String("test.ini")); static QString testIniLockFile(QLatin1String("test.ini.lock")); @@ -51,13 +55,7 @@ void StorageLifeCyclesTest::testLifecycle(void) thread->start(); QVERIFY2(test::signal_eventually_emitted_once(threadStarted), "worker thread should be running by now"); - accounts::AccountStorage *uut = new accounts::AccountStorage(settings, thread); - QSignalSpy accountAdded(uut, &accounts::AccountStorage::added); - QSignalSpy accountRemoved(uut, &accounts::AccountStorage::removed); - QSignalSpy storageDisposed(uut, &accounts::AccountStorage::disposed); - QSignalSpy storageCleaned(uut, &accounts::AccountStorage::destroyed); - - accounts::AccountSecret *secret = uut->secret(); + accounts::AccountSecret *secret = new accounts::AccountSecret(&test::fakeRandom); QSignalSpy existingPasswordNeeded(secret, &accounts::AccountSecret::existingPasswordNeeded); QSignalSpy newPasswordNeeded(secret, &accounts::AccountSecret::newPasswordNeeded); QSignalSpy passwordAvailable(secret, &accounts::AccountSecret::passwordAvailable); @@ -65,6 +63,12 @@ void StorageLifeCyclesTest::testLifecycle(void) QSignalSpy passwordRequestsCancelled(secret, &accounts::AccountSecret::requestsCancelled); QSignalSpy secretCleaned(secret, &accounts::AccountSecret::destroyed); + accounts::AccountStorage *uut = new accounts::AccountStorage(settings, thread, secret); + QSignalSpy accountAdded(uut, &accounts::AccountStorage::added); + QSignalSpy accountRemoved(uut, &accounts::AccountStorage::removed); + QSignalSpy storageDisposed(uut, &accounts::AccountStorage::disposed); + QSignalSpy storageCleaned(uut, &accounts::AccountStorage::destroyed); + // first phase: check that account objects can be loaded from storage QCOMPARE(accountAdded.count(), 0); QVERIFY2(uut->isNameStillAvailable(initialAccountName), "sample account name should still be available"); @@ -141,7 +145,7 @@ void StorageLifeCyclesTest::testLifecycle(void) QVERIFY2(test::signal_eventually_emitted_once(initialAccountCleaned), "sample account should be cleaned up by now"); // third phase: check that new account objects can be added to storage - uut->addTotp(addedAccountName, QLatin1String("NBSWY3DPFQQHO33SNRSCCCQ="), 42, 8); + uut->addTotp(addedAccountName, QLatin1String("NBSWY3DPFQQHO33SNRSCC==="), 42, 8); QVERIFY2(test::signal_eventually_emitted_twice(accountAdded), "new account should be added to storage by now"); QCOMPARE(accountAdded.at(1).at(0), addedAccountName); diff --git a/autotests/account/test-utils/CMakeLists.txt b/autotests/account/test-utils/CMakeLists.txt index fddff83..d9adf74 100644 --- a/autotests/account/test-utils/CMakeLists.txt +++ b/autotests/account/test-utils/CMakeLists.txt @@ -6,8 +6,9 @@ set(account_test_lib_SRCS job.cpp output.cpp + secret.cpp spy.cpp ) add_library(account_test_lib STATIC ${account_test_lib_SRCS}) -target_link_libraries(account_test_lib Qt5::Core Qt5::Test) +target_link_libraries(account_test_lib Qt5::Core Qt5::Test account_lib) diff --git a/autotests/account/test-utils/secret.cpp b/autotests/account/test-utils/secret.cpp new file mode 100644 index 0000000..fca41a8 --- /dev/null +++ b/autotests/account/test-utils/secret.cpp @@ -0,0 +1,72 @@ +/* + * SPDX-License-Identifier: GPL-3.0-or-later + * SPDX-FileCopyrightText: 2020 Johan Ouwerkerk + */ + +#include "secret.h" + +#include +#include + +#include + +namespace test +{ + secrets::SecureMasterKey * useDummyPassword(accounts::AccountSecret *secret) + { + QByteArray salt; + salt.resize(crypto_pwhash_SALTBYTES); + salt.fill('\x0', -1); + QString password(QLatin1String("password")); + return useDummyPassword(secret, password, salt); + } + + secrets::SecureMasterKey * useDummyPassword(accounts::AccountSecret *secret, QString &password, QByteArray &salt) + { + if (!secret) { + qDebug () << "No account secret provided..."; + return nullptr; + } + + std::optional keyParams = secrets::KeyDerivationParameters::create( + crypto_secretbox_KEYBYTES, crypto_pwhash_ALG_DEFAULT, crypto_pwhash_MEMLIMIT_MIN, crypto_pwhash_OPSLIMIT_MIN + ); + if (!keyParams) { + qDebug () << "Failed to construct key derivation parameters"; + return nullptr; + } + + if (!secret->requestExistingPassword(salt, *keyParams)) { + qDebug() << "Failed to simulate password request"; + return nullptr; + } + if (!secret->answerExistingPassword(password)) { + qDebug() << "Failed to supply the password"; + return nullptr; + } + + secrets::SecureMasterKey * k = secret->deriveKey(); + if (!k) { + qDebug() << "Failed to derive the master key"; + return nullptr; + } + return k; + } + + std::optional encrypt(const accounts::AccountSecret *secret, const QByteArray &tokenSecret) + { + QScopedPointer memory(secrets::SecureMemory::allocate((size_t) tokenSecret.size())); + if (!memory) { + qDebug () << "Failed to set up secure memory region for token secret"; + return std::nullopt; + } + + memcpy(memory->data(), tokenSecret.constData(), memory->size()); + std::optional s = secret->encrypt(memory.data()); + if (!s) { + qDebug () << "Failed to encrypt token secret"; + return std::nullopt; + } + return s; + } +} diff --git a/autotests/account/test-utils/secret.h b/autotests/account/test-utils/secret.h new file mode 100644 index 0000000..cb0463b --- /dev/null +++ b/autotests/account/test-utils/secret.h @@ -0,0 +1,24 @@ +/* + * SPDX-License-Identifier: GPL-3.0-or-later + * SPDX-FileCopyrightText: 2020 Johan Ouwerkerk + */ +#ifndef ACCOUNTS_TEST_UTIL_ACCOUNT_SECRET_H +#define ACCOUNTS_TEST_UTIL_ACCOUNT_SECRET_H + +#include "account/keys.h" +#include "secrets/secrets.h" + +#include +#include + +#include + +namespace test +{ + secrets::SecureMasterKey * useDummyPassword(accounts::AccountSecret *secret); + secrets::SecureMasterKey * useDummyPassword(accounts::AccountSecret *secret, QString &password, QByteArray &salt); + + std::optional encrypt(const accounts::AccountSecret *secret, const QByteArray &tokenSecret); +} + +#endif diff --git a/src/account/account.cpp b/src/account/account.cpp index abc64b1..aeb6c62 100644 --- a/src/account/account.cpp +++ b/src/account/account.cpp @@ -206,7 +206,7 @@ namespace accounts return d->activeAccounts(); } - void AccountStorage::handleHotp(const QUuid id, const QString name, const QString secret, quint64 counter, int tokenLength) + void AccountStorage::handleHotp(const QUuid id, const QString name, const QByteArray secret, const QByteArray nonce, quint64 counter, int tokenLength) { Q_D(AccountStorage); if (!d->isStillOpen()) { @@ -223,13 +223,21 @@ namespace accounts return; } - Account *accepted = d->acceptHotpAccount(id, name, secret, counter, tokenLength); + std::optional encryptedSecret = secrets::EncryptedSecret::from(secret, nonce); + if (!encryptedSecret) { + qCDebug(logger) + << "Not handling HOTP account:" << id + << "Invalid encrypted secret/nonce"; + return; + } + + Account *accepted = d->acceptHotpAccount(id, name, *encryptedSecret, counter, tokenLength); QObject::connect(accepted, &Account::removed, this, &AccountStorage::accountRemoved); Q_EMIT added(name); } - void AccountStorage::handleTotp(const QUuid id, const QString name, const QString secret, uint timeStep, int tokenLength) + void AccountStorage::handleTotp(const QUuid id, const QString name, const QByteArray secret, const QByteArray nonce, uint timeStep, int tokenLength) { Q_D(AccountStorage); if (!d->isStillOpen()) { @@ -246,7 +254,15 @@ namespace accounts return; } - Account *accepted = d->acceptTotpAccount(id, name, secret, timeStep, tokenLength); + std::optional encryptedSecret = secrets::EncryptedSecret::from(secret, nonce); + if (!encryptedSecret) { + qCDebug(logger) + << "Not handling TOTP account:" << id + << "Invalid encrypted secret/nonce"; + return; + } + + Account *accepted = d->acceptTotpAccount(id, name, *encryptedSecret, timeStep, tokenLength); QObject::connect(accepted, &Account::removed, this, &AccountStorage::accountRemoved); Q_EMIT added(name); diff --git a/src/account/account.h b/src/account/account.h index b89fa42..182bd37 100644 --- a/src/account/account.h +++ b/src/account/account.h @@ -96,8 +96,8 @@ namespace accounts void load(void); void accountRemoved(void); void handleDisposal(void); - void handleHotp(const QUuid id, const QString name, const QString secret, quint64 counter, int tokenLength); - void handleTotp(const QUuid id, const QString name, const QString secret, uint timeStep, int tokenLength); + void handleHotp(const QUuid id, const QString name, const QByteArray secret, const QByteArray nonce, quint64 counter, int tokenLength); + void handleTotp(const QUuid id, const QString name, const QByteArray secret, const QByteArray nonce, uint timeStep, int tokenLength); private: QScopedPointer m_dptr; Q_DECLARE_PRIVATE_D(m_dptr, AccountStorage) diff --git a/src/account/account_p.cpp b/src/account/account_p.cpp index 5b977e2..6dce53b 100644 --- a/src/account/account_p.cpp +++ b/src/account/account_p.cpp @@ -174,13 +174,13 @@ namespace accounts switch (m_algorithm) { case Account::Algorithm::Hotp: - hotpJob = new ComputeHotp(m_secret, m_counter, m_tokenLength, m_offset, m_checksum); + hotpJob = new ComputeHotp(m_storage->secret(), m_secret, m_counter, m_tokenLength, m_offset, m_checksum); m_actions->queueAndProceed(hotpJob, [hotpJob, q, this](void) -> void { new HandleTokenUpdate(this, hotpJob, q); }); break; case Account::Algorithm::Totp: - totpJob = new ComputeTotp(m_secret, m_epoch, m_timeStep, m_tokenLength, m_hash); + totpJob = new ComputeTotp(m_storage->secret(), m_secret, m_epoch, m_timeStep, m_tokenLength, m_hash); m_actions->queueAndProceed(totpJob, [totpJob, q, this](void) -> void { new HandleTokenUpdate(this, totpJob, q); @@ -224,7 +224,7 @@ namespace accounts } AccountPrivate::AccountPrivate(const std::function &account, AccountStoragePrivate *storage, Dispatcher *dispatcher, const QUuid &id, - const QString &name, const QString &secret, quint64 counter, int tokenLength, int offset, bool addChecksum) : + const QString &name, const secrets::EncryptedSecret &secret, quint64 counter, int tokenLength, int offset, bool addChecksum) : q_ptr(account(this)), m_storage(storage), m_actions(dispatcher), m_is_still_alive(true), m_algorithm(Account::Algorithm::Hotp), m_id(id), m_token(QString()), m_name(name), m_secret(secret), m_tokenLength(tokenLength), m_counter(counter), m_offset(offset), m_checksum(addChecksum), @@ -233,7 +233,7 @@ namespace accounts } AccountPrivate::AccountPrivate(const std::function &account, AccountStoragePrivate *storage, Dispatcher *dispatcher, const QUuid &id, - const QString &name, const QString &secret, const QDateTime &epoch, uint timeStep, int tokenLength, Account::Hash hash) : + const QString &name, const secrets::EncryptedSecret &secret, const QDateTime &epoch, uint timeStep, int tokenLength, Account::Hash hash) : q_ptr(account(this)), m_storage(storage), m_actions(dispatcher), m_is_still_alive(true), m_algorithm(Account::Algorithm::Totp), m_id(id), m_token(QString()), m_name(name), m_secret(secret), m_tokenLength(tokenLength), m_counter(0), m_offset(-1), m_checksum(false), // not a hotp token so these values don't really matter @@ -427,6 +427,32 @@ namespace accounts return attempt; } + std::optional AccountStoragePrivate::encrypt(const QString &secret) const + { + if (!m_is_still_open) { + qCDebug(logger) << "Will not encrypt account secret: storage no longer open"; + return std::nullopt; + } + + if (!m_secret || !m_secret->key()) { + qCDebug(logger) << "Will not encrypt account secret: encryption key not available"; + return std::nullopt; + } + + QScopedPointer decoded(secrets::decodeBase32(secret)); + if (!decoded) { + qCDebug(logger) << "Will not encrypt account secret: failed to decode base32"; + return std::nullopt; + } + + return m_secret->encrypt(decoded.data()); + } + + bool AccountStoragePrivate::validateGenericNewToken(const QString &name, const QString &secret, int tokenLength) const + { + return checkTokenLength(tokenLength) && checkName(name) && isNameStillAvailable(name) && checkSecret(secret); + } + void AccountStoragePrivate::addHotp(const std::function &handler, const QString &name, const QString &secret, quint64 counter, int tokenLength, int offset, bool addChecksum) { Q_UNUSED(offset); @@ -435,21 +461,23 @@ namespace accounts qCDebug(logger) << "Will not add new HOTP account: storage no longer open"; return; } - if (!isNameStillAvailable(name)) { - qCDebug(logger) << "Will not add new HOTP account: account name not available"; - return; - } - QUuid id = generateId(name); - if (!checkHotpAccount(id, name, secret, tokenLength)) { + if (!validateGenericNewToken(name, secret, tokenLength)) { qCDebug(logger) << "Will not add new HOTP account: invalid account details"; return; } + std::optional encryptedSecret = encrypt(secret); + if (!encryptedSecret) { + qCDebug(logger) << "Will not add new HOTP account: failed to encrypt secret"; + return; + } + + QUuid id = generateId(name); qCDebug(logger) << "Requesting to store details for new HOTP account:" << id; m_ids.insert(id); - SaveHotp *job = new SaveHotp(m_settings, id, name, secret, counter, tokenLength); + SaveHotp *job = new SaveHotp(m_settings, id, name, *encryptedSecret, counter, tokenLength); m_actions->queueAndProceed(job, [job, &handler](void) -> void { handler(job); @@ -464,21 +492,23 @@ namespace accounts qCDebug(logger) << "Will not add new TOTP account: storage no longer open"; return; } - if (!isNameStillAvailable(name)) { - qCDebug(logger) << "Will not add new TOTP account: account name not available"; - return; - } - QUuid id = generateId(name); - if (!checkTotpAccount(id, name, secret, tokenLength, timeStep)) { + if (!validateGenericNewToken(name, secret, tokenLength) || !checkTimeStep(timeStep)) { qCDebug(logger) << "Will not add new TOTP account: invalid account details"; return; } + std::optional encryptedSecret = encrypt(secret); + if (!encryptedSecret) { + qCDebug(logger) << "Will not add new TOTP account: failed to encrypt secret"; + return; + } + + QUuid id = generateId(name); qCDebug(logger) << "Requesting to store details for new TOTP account:" << id; m_ids.insert(id); - SaveTotp *job = new SaveTotp(m_settings, id, name, secret, timeStep, tokenLength); + SaveTotp *job = new SaveTotp(m_settings, id, name, *encryptedSecret, timeStep, tokenLength); m_actions->queueAndProceed(job, [job, &handler](void) -> void { handler(job); @@ -507,14 +537,14 @@ namespace accounts return; } - LoadAccounts *job = new LoadAccounts(m_settings); + LoadAccounts *job = new LoadAccounts(m_settings, m_secret); m_actions->queueAndProceed(job, [job, &handler](void) -> void { handler(job); }); } - Account * AccountStoragePrivate::acceptHotpAccount(const QUuid &id, const QString &name, const QString &secret, quint64 counter, int tokenLength, int offset, bool addChecksum) + Account * AccountStoragePrivate::acceptHotpAccount(const QUuid &id, const QString &name, const secrets::EncryptedSecret &secret, quint64 counter, int tokenLength, int offset, bool addChecksum) { Q_Q(AccountStorage); qCDebug(logger) << "Registering HOTP account:" << id; @@ -532,7 +562,7 @@ namespace accounts return m_accounts[id]; } - Account * AccountStoragePrivate::acceptTotpAccount(const QUuid &id, const QString &name, const QString &secret, uint timeStep, int tokenLength, const QDateTime &epoch, Account::Hash hash) + Account * AccountStoragePrivate::acceptTotpAccount(const QUuid &id, const QString &name, const secrets::EncryptedSecret &secret, uint timeStep, int tokenLength, const QDateTime &epoch, Account::Hash hash) { Q_Q(AccountStorage); qCDebug(logger) << "Registering TOTP account:" << id; diff --git a/src/account/account_p.h b/src/account/account_p.h index 4553b26..25ef2a3 100644 --- a/src/account/account_p.h +++ b/src/account/account_p.h @@ -39,11 +39,11 @@ namespace accounts public: explicit AccountPrivate(const std::function &account, AccountStoragePrivate *storage, Dispatcher *dispatcher, const QUuid &id, - const QString &name, const QString &secret, + const QString &name, const secrets::EncryptedSecret &secret, quint64 counter, int tokenLength, int offset, bool addChecksum); explicit AccountPrivate(const std::function &account, AccountStoragePrivate *storage, Dispatcher *dispatcher, const QUuid &id, - const QString &name, const QString &secret, + const QString &name, const secrets::EncryptedSecret &secret, const QDateTime &epoch, uint timeStep, int tokenLength, Account::Hash hash); void recompute(void); void setCounter(quint64 counter); @@ -65,7 +65,7 @@ namespace accounts private: QString m_token; const QString m_name; - const QString m_secret; + const secrets::EncryptedSecret m_secret; const int m_tokenLength; quint64 m_counter; const int m_offset; @@ -94,14 +94,14 @@ namespace accounts void acceptAccountRemoval(const QString &accountName); Account * acceptHotpAccount(const QUuid &id, const QString &name, - const QString &secret, + const secrets::EncryptedSecret &secret, quint64 counter = 0ULL, int tokenLength = 6, int offset = -1, bool addChecksum = false); Account * acceptTotpAccount(const QUuid &id, const QString &name, - const QString &secret, + const secrets::EncryptedSecret &secret, uint timeStep = 30, int tokenLength = 6, const QDateTime &epoch = QDateTime::fromMSecsSinceEpoch(0), @@ -121,6 +121,8 @@ namespace accounts const QDateTime &epoch = QDateTime::fromMSecsSinceEpoch(0), Account::Hash hash = Account::Hash::Default); private: + bool validateGenericNewToken(const QString &name, const QString &secret, int tokenLength) const; + std::optional encrypt(const QString &secret) const; QUuid generateId(const QString &name) const; private: Q_DECLARE_PUBLIC(AccountStorage); diff --git a/src/account/actions_p.cpp b/src/account/actions_p.cpp index 44d6c12..c53d500 100644 --- a/src/account/actions_p.cpp +++ b/src/account/actions_p.cpp @@ -9,6 +9,7 @@ #include "../logging_p.h" #include "../oath/oath.h" +#include #include KEYSMITH_LOGGER(logger, ".accounts.actions") @@ -42,7 +43,7 @@ namespace accounts { } - LoadAccounts::LoadAccounts(const SettingsProvider &settings) : AccountJob(), m_settings(settings) + LoadAccounts::LoadAccounts(const SettingsProvider &settings, const AccountSecret *secret) : AccountJob(), m_settings(settings), m_secret(secret) { } @@ -50,19 +51,19 @@ namespace accounts { } - SaveHotp::SaveHotp(const SettingsProvider &settings, const QUuid &id, const QString &accountName, const QString &secret, quint64 counter, int tokenLength) : + SaveHotp::SaveHotp(const SettingsProvider &settings, const QUuid &id, const QString &accountName, const secrets::EncryptedSecret &secret, quint64 counter, int tokenLength) : AccountJob(), m_settings(settings), m_id(id), m_accountName(accountName), m_secret(secret), m_counter(counter), m_tokenLength(tokenLength) { } - SaveTotp::SaveTotp(const SettingsProvider &settings, const QUuid &id, const QString &accountName, const QString &secret, uint timeStep, int tokenLength) : + SaveTotp::SaveTotp(const SettingsProvider &settings, const QUuid &id, const QString &accountName, const secrets::EncryptedSecret &secret, uint timeStep, int tokenLength) : AccountJob(), m_settings(settings), m_id(id), m_accountName(accountName), m_secret(secret), m_timeStep(timeStep), m_tokenLength(tokenLength) { } void SaveHotp::run(void) { - if (!checkHotpAccount(m_id, m_accountName, m_secret, m_tokenLength)) { + if (!checkId(m_id) || !checkName(m_accountName) || !checkTokenLength(m_tokenLength)) { qCDebug(logger) << "Unable to save HOTP account:" << m_id << "Invalid account details"; @@ -88,7 +89,10 @@ namespace accounts settings.beginGroup(group); settings.setValue("account", m_accountName); settings.setValue("type", "hotp"); - settings.setValue("secret", m_secret); + QString encodedNonce = QString::fromUtf8(m_secret.nonce().toBase64(QByteArray::Base64Encoding)); + QString encodedSecret = QString::fromUtf8(m_secret.cryptText().toBase64(QByteArray::Base64Encoding)); + settings.setValue("secret", encodedSecret); + settings.setValue("nonce", encodedNonce); settings.setValue("counter", m_counter); settings.setValue("pinLength", m_tokenLength); settings.endGroup(); @@ -96,7 +100,7 @@ namespace accounts // Try to guarantee that data will have been written before claiming the account was actually saved settings.sync(); - Q_EMIT saved(m_id, m_accountName, m_secret, m_counter, m_tokenLength); + Q_EMIT saved(m_id, m_accountName, m_secret.cryptText(), m_secret.nonce(), m_counter, m_tokenLength); }); m_settings(act); @@ -105,7 +109,7 @@ namespace accounts void SaveTotp::run(void) { - if (!checkTotpAccount(m_id, m_accountName, m_secret, m_tokenLength, m_timeStep)) { + if (!checkId(m_id) || !checkName(m_accountName) || !checkTokenLength(m_tokenLength) || !checkTimeStep(m_timeStep)) { qCDebug(logger) << "Unable to save TOTP account:" << m_id << "Invalid account details"; @@ -131,7 +135,10 @@ namespace accounts settings.beginGroup(group); settings.setValue("account", m_accountName); settings.setValue("type", "totp"); - settings.setValue("secret", m_secret); + QString encodedNonce = QString::fromUtf8(m_secret.nonce().toBase64(QByteArray::Base64Encoding)); + QString encodedSecret = QString::fromUtf8(m_secret.cryptText().toBase64(QByteArray::Base64Encoding)); + settings.setValue("secret", encodedSecret); + settings.setValue("nonce", encodedNonce); settings.setValue("timeStep", m_timeStep); settings.setValue("pinLength", m_tokenLength); settings.endGroup(); @@ -139,7 +146,7 @@ namespace accounts // Try to guarantee that data will have been written before claiming the account was actually saved settings.sync(); - Q_EMIT saved(m_id, m_accountName, m_secret, m_timeStep, m_tokenLength); + Q_EMIT saved(m_id, m_accountName, m_secret.cryptText(), m_secret.nonce(), m_timeStep, m_tokenLength); }); m_settings(act); @@ -309,13 +316,22 @@ namespace accounts void LoadAccounts::run(void) { + if (!m_secret || !m_secret->key()) { + qCDebug(logger) << "Unable to load accounts: secret decryption key not available"; + Q_EMIT finished(); + return; + } + const PersistenceAction act([this](QSettings &settings) -> void { qCInfo(logger, "Loading accounts from storage"); const QStringList entries = settings.childGroups(); for (const QString &group : entries) { - const QUuid id(group); + if (group == QLatin1String("master-key")) { + continue; + } + const QUuid id(group); if (id.isNull()) { qCDebug(logger) << "Ignoring:" << group @@ -323,47 +339,87 @@ namespace accounts continue; } - bool ok = false; settings.beginGroup(group); - const QString secret = settings.value("secret").toString(); const QString accountName = settings.value("account").toString(); - const QString type = settings.value("type", "hotp").toString(); - const int tokenLength = settings.value("pinLength").toInt(&ok); - - if (!ok || (type != "hotp" && type != "totp")) { - qCWarning(logger) << "Skipping invalid account:" << id; + if (!checkName(accountName)) { + qCWarning(logger) + << "Skipping invalid account:" << id + << "Invalid account name"; settings.endGroup(); continue; } - if (type == "totp") { + const QString type = settings.value("type", "hotp").toString(); + if (type != QLatin1String("hotp") && type != QLatin1String("totp")) { + qCWarning(logger) + << "Skipping invalid account:" << id + << "Invalid account type"; + settings.endGroup(); + continue; + } + + bool ok = false; + const int tokenLength = settings.value("pinLength").toInt(&ok); + if (!ok || !checkTokenLength(tokenLength)) { + qCWarning(logger) + << "Skipping invalid account:" << id + << "Invalid token length"; + settings.endGroup(); + continue; + } + + const QByteArray encodedNonce = settings.value("nonce").toString().toUtf8(); + const QByteArray encodedSecret = settings.value("secret").toString().toUtf8(); + const QByteArray nonce = QByteArray::fromBase64(encodedNonce, QByteArray::Base64Encoding); + const QByteArray secret = QByteArray::fromBase64(encodedSecret, QByteArray::Base64Encoding); + + std::optional encryptedSecret = secrets::EncryptedSecret::from(secret, nonce); + if (!encryptedSecret) { + qCWarning(logger) + << "Skipping invalid account:" << id + << "Invalid token secret"; + settings.endGroup(); + continue; + } + + QScopedPointer decrypted(m_secret->decrypt(*encryptedSecret)); + if (!decrypted) { + qCWarning(logger) + << "Skipping invalid account:" << id + << "Unable to decrypt token secret"; + settings.endGroup(); + continue; + } + + if (type == QLatin1String("totp")) { ok = false; const uint timeStep = settings.value("timeStep").toUInt(&ok); - if (ok && checkTotpAccount(id, accountName, secret, tokenLength, timeStep)) { - qCInfo(logger) << "Found valid TOTP account:" << id; - Q_EMIT foundTotp( - id, - accountName, - secret, - timeStep, - tokenLength - ); + if (!ok || !checkTimeStep(timeStep)) { + qCWarning(logger) + << "Skipping invalid account:" << id + << "Invalid time step"; + settings.endGroup(); + continue; } + + qCInfo(logger) << "Found valid TOTP account:" << id; + Q_EMIT foundTotp(id, accountName, secret, nonce, timeStep, tokenLength); } - if (type == "hotp") { + + if (type == QLatin1String("hotp")) { ok = false; const quint64 counter = settings.value("counter").toULongLong(&ok); - if (ok && checkHotpAccount(id, accountName, secret, tokenLength)) { - qCInfo(logger) << "Found valid HOTP account:" << id; - Q_EMIT foundHotp( - id, - accountName, - secret, - counter, - tokenLength - ); + if (!ok) { + qCWarning(logger) + << "Skipping invalid account:" << id + << "Invalid counter"; + settings.endGroup(); + continue; } + + qCInfo(logger) << "Found valid HOTP account:" << id; + Q_EMIT foundHotp(id, accountName, secret, nonce, counter, tokenLength); } settings.endGroup(); @@ -374,22 +430,27 @@ namespace accounts Q_EMIT finished(); } - ComputeTotp::ComputeTotp(const QString &secret, const QDateTime &epoch, uint timeStep, int tokenLength, const Account::Hash &hash, const std::function &clock) : - AccountJob(), m_secret(secret), m_epoch(epoch), m_timeStep(timeStep), m_tokenLength(tokenLength), m_hash(hash), m_clock(clock) + ComputeTotp::ComputeTotp(const AccountSecret *secret, const secrets::EncryptedSecret &tokenSecret, const QDateTime &epoch, uint timeStep, int tokenLength, const Account::Hash &hash, const std::function &clock) : + AccountJob(), m_secret(secret), m_tokenSecret(tokenSecret), m_epoch(epoch), m_timeStep(timeStep), m_tokenLength(tokenLength), m_hash(hash), m_clock(clock) { } void ComputeTotp::run(void) { - if (!checkTotp(m_secret, m_tokenLength, m_timeStep)) { - qCDebug(logger) << "Unable to compute TOTP token: invalid token details"; + if (!m_secret || !m_secret->key()) { + qCDebug(logger) << "Unable to compute TOTP token: secret decryption key not available"; Q_EMIT finished(); return; } - std::optional secret = base32::decode(m_secret); - if (!secret.has_value()) { - qCDebug(logger) << "Unable to compute TOTP token: unable to decode secret"; + if (!checkTokenLength(m_tokenLength)) { + qCDebug(logger) << "Unable to compute THOTP token: invalid token length:" << m_tokenLength; + Q_EMIT finished(); + return; + } + + if (!checkTimeStep(m_timeStep)) { + qCDebug(logger) << "Unable to compute THOTP token: invalid time step:" << m_timeStep; Q_EMIT finished(); return; } @@ -415,19 +476,26 @@ namespace accounts const std::optional algorithm = oath::Algorithm::usingDynamicTruncation(hash, m_tokenLength); if (!algorithm) { - qCDebug(logger) << "Unable to compute TOTP token: unable to set up truncation for token length:" << m_tokenLength; + qCDebug(logger) << "Unable to compute TOTP token: failed to construct algorithm"; Q_EMIT finished(); return; } const std::optional counter = oath::count(m_epoch, m_timeStep, m_clock); if (!counter) { - qCDebug(logger) << "Unable to compute TOTP token: unable to count time steps"; + qCDebug(logger) << "Unable to compute TOTP token: failed to count time steps"; Q_EMIT finished(); return; } - const std::optional token = algorithm->compute(*counter, secret->data(), secret->size()); + QScopedPointer secret(m_secret->decrypt(m_tokenSecret)); + if (!secret) { + qCDebug(logger) << "Unable to compute TOTP token: failed to decrypt secret"; + Q_EMIT finished(); + return; + } + + const std::optional token = algorithm->compute(*counter, reinterpret_cast(secret->data()), secret->size()); if (token) { Q_EMIT otp(*token); } else { @@ -437,22 +505,21 @@ namespace accounts Q_EMIT finished(); } - ComputeHotp::ComputeHotp(const QString &secret, quint64 counter, int tokenLength, int offset, bool checksum) : - AccountJob(), m_secret(secret), m_counter(counter), m_tokenLength(tokenLength), m_offset(offset), m_checksum(checksum) + ComputeHotp::ComputeHotp(const AccountSecret *secret, const secrets::EncryptedSecret &tokenSecret, quint64 counter, int tokenLength, int offset, bool checksum) : + AccountJob(), m_secret(secret), m_tokenSecret(tokenSecret), m_counter(counter), m_tokenLength(tokenLength), m_offset(offset), m_checksum(checksum) { } void ComputeHotp::run(void) { - if (!checkHotp(m_secret, m_tokenLength)) { - qCDebug(logger) << "Unable to compute HOTP token: invalid token details"; + if (!m_secret || !m_secret->key()) { + qCDebug(logger) << "Unable to compute HOTP token: secret decryption key not available"; Q_EMIT finished(); return; } - std::optional secret = base32::decode(m_secret); - if (!secret.has_value()) { - qCDebug(logger) << "Unable to compute HOTP token: unable to decode secret"; + if (!checkTokenLength(m_tokenLength)) { + qCDebug(logger) << "Unable to compute HOTP token: invalid token length:" << m_tokenLength; Q_EMIT finished(); return; } @@ -462,12 +529,19 @@ namespace accounts ? oath::Algorithm::usingTruncationOffset(QCryptographicHash::Sha1, (uint) m_offset, encoder) : oath::Algorithm::usingDynamicTruncation(QCryptographicHash::Sha1, encoder); if (!algorithm) { - qCDebug(logger) << "Unable to compute HOTP token: unable to set up truncation for token length:" << m_tokenLength; + qCDebug(logger) << "Unable to compute HOTP token: failed to construct algorithm"; Q_EMIT finished(); return; } - const std::optional token = algorithm->compute(m_counter, secret->data(), secret->size()); + QScopedPointer secret(m_secret->decrypt(m_tokenSecret)); + if (!secret) { + qCDebug(logger) << "Unable to compute HOTP token: failed to decrypt secret"; + Q_EMIT finished(); + return; + } + + const std::optional token = algorithm->compute(m_counter, reinterpret_cast(secret->data()), secret->size()); if (token) { Q_EMIT otp(*token); } else { diff --git a/src/account/actions_p.h b/src/account/actions_p.h index 4ebb30d..358f48c 100644 --- a/src/account/actions_p.h +++ b/src/account/actions_p.h @@ -17,6 +17,7 @@ #include +#include "../secrets/secrets.h" #include "keys.h" namespace accounts @@ -65,13 +66,14 @@ namespace accounts { Q_OBJECT public: - explicit LoadAccounts(const SettingsProvider &settings); + explicit LoadAccounts(const SettingsProvider &settings, const AccountSecret *secret); void run(void) override; Q_SIGNALS: - void foundHotp(const QUuid id, const QString name, const QString secret, quint64 counter, int tokenLength); - void foundTotp(const QUuid id, const QString name, const QString secret, uint timeStep, int tokenLength); + void foundHotp(const QUuid id, const QString name, const QByteArray secret, const QByteArray nonce, quint64 counter, int tokenLength); + void foundTotp(const QUuid id, const QString name, const QByteArray secret, const QByteArray nonce, uint timeStep, int tokenLength); private: const SettingsProvider m_settings; + const AccountSecret * m_secret; }; class DeleteAccounts: public AccountJob @@ -91,16 +93,16 @@ namespace accounts { Q_OBJECT public: - explicit SaveHotp(const SettingsProvider &settings, const QUuid &id, const QString &accountName, const QString &secret, quint64 counter, int tokenLength); + explicit SaveHotp(const SettingsProvider &settings, const QUuid &id, const QString &accountName, const secrets::EncryptedSecret &secret, quint64 counter, int tokenLength); void run(void) override; Q_SIGNALS: void invalid(void); - void saved(const QUuid id, const QString accountName, const QString secret, quint64 counter, int tokenLength); + void saved(const QUuid id, const QString accountName, const QByteArray secret, const QByteArray nonce, quint64 counter, int tokenLength); private: const SettingsProvider m_settings; const QUuid m_id; const QString m_accountName; - const QString m_secret; + const secrets::EncryptedSecret m_secret; const quint64 m_counter; const int m_tokenLength; }; @@ -109,16 +111,16 @@ namespace accounts { Q_OBJECT public: - explicit SaveTotp(const SettingsProvider &settings, const QUuid &id, const QString &accountName, const QString &secret, uint timeStep, int tokenLength); + explicit SaveTotp(const SettingsProvider &settings, const QUuid &id, const QString &accountName, const secrets::EncryptedSecret &secret, uint timeStep, int tokenLength); void run(void) override; Q_SIGNALS: void invalid(void); - void saved(const QUuid id, const QString accountName, const QString secret, uint timeStep, int tokenLength); + void saved(const QUuid id, const QString accountName, const QByteArray secret, const QByteArray nonce, uint timeStep, int tokenLength); private: const SettingsProvider m_settings; const QUuid m_id; const QString m_accountName; - const QString m_secret; + const secrets::EncryptedSecret m_secret; const uint m_timeStep; const int m_tokenLength; }; @@ -127,12 +129,13 @@ namespace accounts { Q_OBJECT public: - explicit ComputeTotp(const QString &secret, const QDateTime &epoch, uint timeStep, int tokenLength, const Account::Hash &hash = Account::Hash::Default, const std::function &clock = &QDateTime::currentMSecsSinceEpoch); + explicit ComputeTotp(const AccountSecret *secret, const secrets::EncryptedSecret &tokenSecret, const QDateTime &epoch, uint timeStep, int tokenLength, const Account::Hash &hash = Account::Hash::Default, const std::function &clock = &QDateTime::currentMSecsSinceEpoch); void run(void) override; Q_SIGNALS: void otp(const QString otp); private: - const QString m_secret; + const AccountSecret * m_secret; + const secrets::EncryptedSecret m_tokenSecret; const QDateTime m_epoch; const uint m_timeStep; const int m_tokenLength; @@ -144,12 +147,13 @@ namespace accounts { Q_OBJECT public: - explicit ComputeHotp(const QString &secret, quint64 counter, int tokenLength, int offset = -1, bool checksum = false); + explicit ComputeHotp(const AccountSecret *secret, const secrets::EncryptedSecret &tokenSecret, quint64 counter, int tokenLength, int offset = -1, bool checksum = false); void run(void) override; Q_SIGNALS: void otp(const QString otp); private: - const QString m_secret; + const AccountSecret * m_secret; + const secrets::EncryptedSecret m_tokenSecret; const quint64 m_counter; const int m_tokenLength; const int m_offset; diff --git a/src/account/keys.cpp b/src/account/keys.cpp index 660a809..e6ada76 100644 --- a/src/account/keys.cpp +++ b/src/account/keys.cpp @@ -220,5 +220,25 @@ namespace accounts return m_stillAlive && m_key ? m_key.data() : nullptr; } + std::optional AccountSecret::encrypt(const secrets::SecureMemory *secret) const + { + secrets::SecureMasterKey *k = key(); + if (!k) { + qCDebug(logger) << "Unable to encrypt secret: encryption key not available"; + return std::nullopt; + } + return k->encrypt(secret); + } + + secrets::SecureMemory * AccountSecret::decrypt(const secrets::EncryptedSecret &secret) const + { + secrets::SecureMasterKey *k = key(); + if (!k) { + qCDebug(logger) << "Unable to decrypt secret: decryption key not available"; + return nullptr; + } + + return k->decrypt(secret); + } } diff --git a/src/account/keys.h b/src/account/keys.h index 83aeed3..41e1ba1 100644 --- a/src/account/keys.h +++ b/src/account/keys.h @@ -33,6 +33,8 @@ namespace accounts secrets::SecureMasterKey * deriveKey(void); secrets::SecureMasterKey * key(void) const; + std::optional encrypt(const secrets::SecureMemory *secret) const; + secrets::SecureMemory * decrypt(const secrets::EncryptedSecret &secret) const; bool isStillAlive(void) const; bool isNewPasswordRequested(void) const; bool isExistingPasswordRequested(void) const; diff --git a/src/account/validation.cpp b/src/account/validation.cpp index e03ce6d..0994cad 100644 --- a/src/account/validation.cpp +++ b/src/account/validation.cpp @@ -33,24 +33,4 @@ namespace accounts { return timeStep > 0; } - - bool checkHotp(const QString &secret, const int tokenLength) - { - return checkSecret(secret) && checkTokenLength(tokenLength); - } - - bool checkHotpAccount(const QUuid &id, const QString &name, const QString &secret, const int tokenLength) - { - return checkId(id) && checkName(name) && checkHotp(secret, tokenLength); - } - - bool checkTotp(const QString &secret, const int tokenLength, const uint timeStep) - { - return checkSecret(secret) && checkTokenLength(tokenLength) && checkTimeStep(timeStep); - } - - bool checkTotpAccount(const QUuid &id, const QString &name, const QString &secret, const int tokenLength, const uint timeStep) - { - return checkId(id) && checkName(name) && checkTotp(secret, tokenLength, timeStep); - } } diff --git a/src/account/validation.h b/src/account/validation.h index c3275ef..6a0e6b2 100644 --- a/src/account/validation.h +++ b/src/account/validation.h @@ -15,12 +15,6 @@ namespace accounts bool checkName(const QString &name); bool checkTokenLength(int tokenLength); bool checkTimeStep(uint timeStep); - - bool checkHotp(const QString &secret, const int tokenLength); - bool checkTotp(const QString &secret, const int tokenLength, const uint timeStep); - - bool checkHotpAccount(const QUuid &id, const QString &name, const QString &secret, const int tokenLength); - bool checkTotpAccount(const QUuid &id, const QString &name, const QString &secret, const int tokenLength, const uint timeStep); } #endif