Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Qt olm binding #443

Closed
wants to merge 45 commits into from
Closed

Conversation

CarlSchwan
Copy link
Contributor

Start working on better Qt olm binding. The api and implementation is a plain port of the rust lib: olm-rs.

@CarlSchwan CarlSchwan marked this pull request as draft January 23, 2021 16:11
lib/olm/e2ee.h Outdated Show resolved Hide resolved
lib/olm/qolmaccount.cpp Outdated Show resolved Hide resolved
lib/olm/qolmoutboundsession.h Outdated Show resolved Hide resolved
@TobiasFella
Copy link
Member

Fails to build when building tests but not E2EE

@CarlSchwan
Copy link
Contributor Author

Fails to build when building tests but not E2EE

This should now be fixed. I just needed to ifdef everything. A good goal would still be at some point to remove all this ifdef once this feature is stable and don't crash :)

@KitsuneRal
Copy link
Member

I feel a bit uneasy about having olm/ directory inside libQuotient tree - it's not immediately clear whether a given header file comes from Olm itself or the wrapper. Perhaps rename it to e2ee/ or qolm?

Also: did I get it right that you plan to eventually get rid of QtOlm dependency, rather than rewrite it?

@CarlSchwan
Copy link
Contributor Author

I feel a bit uneasy about having olm/ directory inside libQuotient tree - it's not immediately clear whether a given header file comes from Olm itself or the wrapper. Perhaps rename it to e2ee/ or qolm?

Yes, it's probably that I will do soon, even more as I'm adding stuff related to encryption but not to olm specific (e.g. key export)

Also: did I get it right that you plan to eventually get rid of QtOlm dependency, rather than rewrite it?

Yes, the goal is to get rid of QtOlm. For the moment, I still have dependencies to QtOlm but it's only to make sure the code still compiles but I think I will be soon at a point where I can replace it with the new API.

lib/converters.cpp Outdated Show resolved Hide resolved
lib/converters.cpp Outdated Show resolved Hide resolved
lib/converters.cpp Outdated Show resolved Hide resolved
QOlmAccount(OlmAccount *account);
OlmAccount *data();
private:
OlmAccount *m_account = nullptr; // owning
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unique_ptr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An unique_ptr with its own custom deleter could work but it's more just add more complexity.

lib/room.cpp Outdated Show resolved Hide resolved
@@ -15,7 +15,12 @@ class NetworkAccessManager::Private {

NetworkAccessManager::NetworkAccessManager(QObject* parent)
: QNetworkAccessManager(parent), d(std::make_unique<Private>())
{}
{
connect(this, &QNetworkAccessManager::sslErrors, this, [](QNetworkReply *reply, const QList<QSslError> &errors)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's for the test but need a better solution because we can't ignore the SSL errors in production...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch that should fix it but doesn't work. Probably because of a stupid error somewhere.

diff --git a/autotests/testolmaccount.cpp b/autotests/testolmaccount.cpp
index 9134224..eb44791 100644
--- a/autotests/testolmaccount.cpp
+++ b/autotests/testolmaccount.cpp
@@ -165,6 +165,7 @@ void TestOlmAccount::encryptedFile()
 
 #define CREATE_CONNECTION(VAR, USERNAME, SECRET, DEVICE_NAME) \
     auto VAR = std::make_shared<Connection>(); \
+    VAR->ignoreSslErrors(true); \
     (VAR) ->resolveServer("@alice:localhost:" + QString::number(443)); \
     connect( (VAR) .get(), &Connection::loginFlowsChanged, this, [this, VAR ] () { \
         (VAR) ->loginWithPassword( (USERNAME) , SECRET , DEVICE_NAME , ""); \
diff --git a/lib/connection.cpp b/lib/connection.cpp
index 070aac0..d01aab3 100644
--- a/lib/connection.cpp
+++ b/lib/connection.cpp
@@ -573,6 +573,11 @@ void Connection::sync(int timeout)
     });
 }
 
+void Connection::ignoreSslErrors(bool ignore)
+{
+    connectionData()->ignoreSslErrors(ignore);
+}
+
 void Connection::syncLoop(int timeout)
 {
     if (d->syncLoopConnection && d->syncTimeout == timeout) {
diff --git a/lib/connection.h b/lib/connection.h
index 9a952e0..f3a6b8c 100644
--- a/lib/connection.h
+++ b/lib/connection.h
@@ -481,6 +481,11 @@ public:
         setUserFactory(defaultUserFactory<T>());
     }
 
+    /// Ignore ssl errors (usefull for automated testing with local synapse
+    /// instance).
+    /// \internal
+    void ignoreSslErrors(bool ignore);
+
 public Q_SLOTS:
     /** Set the homeserver base URL */
     void setHomeserver(const QUrl& baseUrl);
diff --git a/lib/connectiondata.cpp b/lib/connectiondata.cpp
index e54d909..123febe 100644
--- a/lib/connectiondata.cpp
+++ b/lib/connectiondata.cpp
@@ -140,6 +140,12 @@ bool ConnectionData::needsToken(const QString& requestName) const
            != d->needToken.cend();
 }
 
+void ConnectionData::ignoreSslErrors(bool ignore) const
+{
+    auto quotientNam = static_cast<NetworkAccessManager>(nam());
+    quotientNam.ignoreSslErrors(ignore);
+}
+
 void ConnectionData::setDeviceId(const QString& deviceId)
 {
     d->deviceId = deviceId;
diff --git a/lib/connectiondata.h b/lib/connectiondata.h
index 7dd96f2..00d056e 100644
--- a/lib/connectiondata.h
+++ b/lib/connectiondata.h
@@ -29,6 +29,7 @@ public:
     bool needsToken(const QString& requestName) const;
     QNetworkAccessManager* nam() const;
 
+    void ignoreSslErrors(bool ignore = true) const;
     void setBaseUrl(QUrl baseUrl);
     void setToken(QByteArray accessToken);
     [[deprecated("Use setBaseUrl() instead")]]
diff --git a/lib/networkaccessmanager.cpp b/lib/networkaccessmanager.cpp
index c6e761e..1276305 100644
--- a/lib/networkaccessmanager.cpp
+++ b/lib/networkaccessmanager.cpp
@@ -16,10 +16,6 @@ public:
 NetworkAccessManager::NetworkAccessManager(QObject* parent)
     : QNetworkAccessManager(parent), d(std::make_unique<Private>())
 {
-    connect(this, &QNetworkAccessManager::sslErrors, this, [](QNetworkReply *reply, const QList<QSslError> &errors)
-            {
-                reply->ignoreSslErrors();
-            });
 }
 
 QList<QSslError> NetworkAccessManager::ignoredSslErrors() const
@@ -27,6 +23,19 @@ QList<QSslError> NetworkAccessManager::ignoredSslErrors() const
     return d->ignoredSslErrors;
 }
 
+void NetworkAccessManager::ignoreSslErrors(bool ignore) const
+{
+    if (ignore) {
+        connect(this, &QNetworkAccessManager::sslErrors, this, [](QNetworkReply *reply, const QList<QSslError> &errors)
+                {
+    qDebug() << 24 / 0;
+                    reply->ignoreSslErrors();
+                });
+    } else {
+        disconnect(this, &QNetworkAccessManager::sslErrors, this, nullptr);
+    }
+}
+
 void NetworkAccessManager::addIgnoredSslError(const QSslError& error)
 {
     d->ignoredSslErrors << error;
diff --git a/lib/networkaccessmanager.h b/lib/networkaccessmanager.h
index 47729a1..22280e0 100644
--- a/lib/networkaccessmanager.h
+++ b/lib/networkaccessmanager.h
@@ -17,6 +17,7 @@ public:
     QList<QSslError> ignoredSslErrors() const;
     void addIgnoredSslError(const QSslError& error);
     void clearIgnoredSslErrors();
+    void ignoreSslErrors(bool ignore = true) const;
 
     /** Get a pointer to the singleton */
     static NetworkAccessManager* instance();

lib/encryptionmanager.cpp Outdated Show resolved Hide resolved
lib/encryptionmanager.cpp Outdated Show resolved Hide resolved
lib/encryptionmanager.cpp Outdated Show resolved Hide resolved
lib/crypto/qolmutils.cpp Outdated Show resolved Hide resolved
autotests/testolmaccount.cpp Outdated Show resolved Hide resolved
autotests/testolmaccount.cpp Outdated Show resolved Hide resolved
lib/crypto/qolmaccount.cpp Outdated Show resolved Hide resolved

bool operator==(const IdentityKeys& lhs, const IdentityKeys& rhs)
{
return lhs.curve25519 == rhs.curve25519 &&& lhs.ed25519 == rhs.ed25519;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of '&' - is that something in C++ i don't know or an error?

}

// Convert olm error to enum
QOlmError lastError(OlmAccount *account) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that be a member function of QOlmAccount instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an internal function, that could be a private method too but I don't really see an advantage.

QByteArray randomData = getRandom(randomSize);
const auto error = olm_create_account(m_account, randomData.data(), randomSize);
if (error == olm_error()) {
throw lastError(m_account);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not throw things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I tried to only throw for errors that can't and shouldn't be recovered. In that case, it should only throw when there was a problem when allocating the memory.

lib/crypto/qolmaccount.cpp Outdated Show resolved Hide resolved
lib/crypto/qolmaccount.cpp Outdated Show resolved Hide resolved
lib/crypto/qolmaccount.cpp Outdated Show resolved Hide resolved
lib/crypto/qolmaccount.cpp Outdated Show resolved Hide resolved
lib/crypto/qolmaccount.h Outdated Show resolved Hide resolved
lib/crypto/qolmaccount.h Outdated Show resolved Hide resolved
autotests/testgroupsession.cpp Outdated Show resolved Hide resolved
autotests/testgroupsession.h Outdated Show resolved Hide resolved
const auto ed25519 = identityKeys.ed25519;
// verify encoded keys length
QCOMPARE(curve25519.size(), 43);
QCOMPARE(ed25519.size(), 43);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that a safe assumption?

QOlmAccount olmAccount(QStringLiteral("@foo:bar.com"), QStringLiteral("QuotientTestDevice"));
olmAccount.createNewAccount();
const auto maxNumberOfOneTimeKeys = olmAccount.maxNumberOfOneTimeKeys();
QCOMPARE(100, maxNumberOfOneTimeKeys);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that defined as the maximum somewhere or is it just what olm outputs currently?

Comment on lines +194 to +197
connect(request, &BaseJob::result, this, [request, conn](BaseJob *job) {
auto job2 = static_cast<UploadKeysJob *>(job);
QCOMPARE(job2->oneTimeKeyCounts().size(), 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
connect(request, &BaseJob::result, this, [request, conn](BaseJob *job) {
auto job2 = static_cast<UploadKeysJob *>(job);
QCOMPARE(job2->oneTimeKeyCounts().size(), 0);
connect(request, &BaseJob::result, this, [request]() {
QCOMPARE(request->oneTimeKeyCounts().size(), 0);

should be the same

Comment on lines +222 to +226
connect(request, &BaseJob::result, this, [request, conn](BaseJob *job) {
auto job2 = static_cast<UploadKeysJob *>(job);
QCOMPARE(job2->oneTimeKeyCounts().size(), 1);
QCOMPARE(job2->oneTimeKeyCounts()["curve25519"], 5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
connect(request, &BaseJob::result, this, [request, conn](BaseJob *job) {
auto job2 = static_cast<UploadKeysJob *>(job);
QCOMPARE(job2->oneTimeKeyCounts().size(), 1);
QCOMPARE(job2->oneTimeKeyCounts()["curve25519"], 5);
connect(request, &BaseJob::result, this, [request]() {
QCOMPARE(request->oneTimeKeyCounts().size(), 1);
QCOMPARE(request->oneTimeKeyCounts()["curve25519"], 5);

Comment on lines +246 to +249
QVariant var;
var.setValue(key);
oneTimeKeysHash[keyId] = var;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
QVariant var;
var.setValue(key);
oneTimeKeysHash[keyId] = var;
oneTimeKeysHash[keyId] = QVariant::fromValue(key);

Comment on lines +251 to +255
connect(request, &BaseJob::result, this, [request, nKeys, conn](BaseJob *job) {
auto job2 = static_cast<UploadKeysJob *>(job);
QCOMPARE(job2->oneTimeKeyCounts().size(), 1);
QCOMPARE(job2->oneTimeKeyCounts()["signed_curve25519"], nKeys);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
connect(request, &BaseJob::result, this, [request, nKeys, conn](BaseJob *job) {
auto job2 = static_cast<UploadKeysJob *>(job);
QCOMPARE(job2->oneTimeKeyCounts().size(), 1);
QCOMPARE(job2->oneTimeKeyCounts()["signed_curve25519"], nKeys);
connect(request, &BaseJob::result, this, [request, nKeys]() {
QCOMPARE(request->oneTimeKeyCounts().size(), 1);
QCOMPARE(request->oneTimeKeyCounts()["signed_curve25519"], nKeys);

Comment on lines +272 to +276
connect(request, &BaseJob::result, this, [request, conn](BaseJob *job) {
auto job2 = static_cast<UploadKeysJob *>(job);
QCOMPARE(job2->oneTimeKeyCounts().size(), 1);
QCOMPARE(job2->oneTimeKeyCounts()["signed_curve25519"], 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
connect(request, &BaseJob::result, this, [request, conn](BaseJob *job) {
auto job2 = static_cast<UploadKeysJob *>(job);
QCOMPARE(job2->oneTimeKeyCounts().size(), 1);
QCOMPARE(job2->oneTimeKeyCounts()["signed_curve25519"], 1);
connect(request, &BaseJob::result, this, [request]() {
QCOMPARE(request->oneTimeKeyCounts().size(), 1);
QCOMPARE(request->oneTimeKeyCounts()["signed_curve25519"], 1);

void TestOlmAccount::claimKeys()
{
CREATE_CONNECTION(alice, "alice", "secret", "AlicePhone")
CREATE_CONNECTION(bob, "alice", "secret", "AlicePhone")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's important, but should bob maybe log in as "bob" instead of "alice"?

auto job = bob->callApi<ClaimKeysJob>(oneTimeKeys);
connect(job, &BaseJob::result, this, [bob, job] {
const auto userId = bob->userId();
const auto deviceId = bob->deviceId();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const auto deviceId = bob->deviceId();

autotests/testolmaccount.h Show resolved Hide resolved
autotests/testolmaccount.h Outdated Show resolved Hide resolved
autotests/testolmsession.cpp Show resolved Hide resolved
autotests/testolmsession.h Show resolved Hide resolved
auto utility = olm_utility(utilityBuf);


QByteArray signatureBuf1(sig.length(), '0');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this copy needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly olm_ed25519_verify will modify sig and we still need the original sig later.

QCOMPARE(res2, true);
}

void TestOlmUtility::validUploadKeysRequest()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content doesn't really fit to the method name

autotests/testolmutility.h Outdated Show resolved Hide resolved
Comment on lines +13 to +15
if (v.canConvert<SignedOneTimeKey>()) {
return toJson(v.value<SignedOneTimeKey>());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it shouldn't be necessary. is it just some hack or is there a deeper reason for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a hack, unfortunately, the JSON /C++ bridge can't really handle the case where something is either an object or a string.

qCDebug(E2EE) << "Found key" << ourKey
<< "instead of ours own ed25519 key"
<< encryptionManager->account()->ed25519IdentityKey()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this is not new code, but is it a good idea to log keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is just debugging code, for the moment I completely disabled this code since it was crashy

lib/crypto/qolmutility.h Outdated Show resolved Hide resolved
const auto ret = olm_ed25519_verify(m_utility, key.data(), key.size(),
message.data(), message.size(), (void *)signatureBuf.data(), signatureBuf.size());

const auto error = ret;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that stuff should be simplified (same with the if condition underneath)

lib/crypto/qolmoutboundsession.h Show resolved Hide resolved
lib/crypto/qolmoutboundsession.h Outdated Show resolved Hide resolved
lib/crypto/qolmoutboundsession.h Show resolved Hide resolved
@TobiasFella
Copy link
Member

Since there will probably be other E2EE encryption systems in matrix sooner or later, should the olm stuff be moved to crypto/qolm or something?

const QByteArray key = toKey(mode);
const auto error = olm_unpickle_account(m_account, key.data(), key.length(), pickled.data(), pickled.size());
if (error == olm_error()) {
throw lastError(m_account);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some of the errors thrown by olm_unpickle_account (invalid base64, bad account key) are probably recoverable

reinterpret_cast<const uint8_t *>(temp.data()), temp.size());

if (error == olm_error()) {
throw lastError(olmInboundGroupSession);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably shouldn't fail completely for these errors

const auto error = olm_import_inbound_group_session(olmInboundGroupSession,
reinterpret_cast<const uint8_t *>(keyBuf.data()), keyBuf.size());
if (error == olm_error()) {
throw lastError(olmInboundGroupSession);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

{
if (preKeyMessage.type() != QOlmMessage::PreKey) {
qCDebug(E2EE) << "The message is not a pre-key";
throw BadMessageFormat;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assume that this will never happen if the client uses this correctly?

{
const auto messageTypeResult = olm_encrypt_message_type(m_session);
if (messageTypeResult == olm_error()) {
throw lastError(m_session);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The olm documentation does not specify which errors might happen here. we should look into the implementation to figure out if we should handle them or not

@TobiasFella
Copy link
Member

we could also assert the unrecoverable errors instead of throwing them, since they should not happen if the program is reasonably correct

@CarlSchwan CarlSchwan changed the title Draft: Start implementing Qt olm binding Start implementing Qt olm binding May 6, 2021
@CarlSchwan CarlSchwan changed the title Start implementing Qt olm binding Add Qt olm binding May 6, 2021
@KitsuneRal KitsuneRal added the enhancement A feature or change request for the library label May 15, 2021
@KitsuneRal
Copy link
Member

Upon a quick look-through, without really analysing the code:

  • Please double-check the code is formatted according to .clang-format.
  • Please indicate the Qt module of the header: #include <QtCore/QDebug>, not #include <QDebug>.
  • I understand the idea of throwing when things are utterly grim and irrecoverable; but in some cases throw is combined with the error object, which is very confusing from the API standpoint. Please let's really try to go without exceptions, as long as there's QOlmError.

I will look closer into the code; it would help if you could clearly declare the scope of the work accomplished by now (what is supposed to work, namely).

@CarlSchwan
Copy link
Contributor Author

I pushed this to the work/olm branch. I will close this MR and new E2EE related MR should be done against the work/olm branch. Once everything is working we can merge the work/olm branch inside master. :)

@hugohn
Copy link

hugohn commented Apr 4, 2022

Hi guys, is this still ongoing work? was there an MR that replaced this MR?

@CarlSchwan
Copy link
Contributor Author

Hi guys, is this still ongoing work? was there an MR that replaced this MR?

This was merged in another mr. Message decryption should now work if the compile flag is set

@hugohn
Copy link

hugohn commented Apr 4, 2022

Hi guys, is this still ongoing work? was there an MR that replaced this MR?

This was merged in another mr. Message decryption should now work if the compile flag is set

Awesome! Since 0.7 is not out yet, I assume message decryption is currently available only in the dev branch, correct?

Also do you know when will 0.7 be released?

@KitsuneRal
Copy link
Member

Hopefully sometime later in the summer, contingent on me having at least some reasonable time for the project (and in any case it likely won't have complete E2EE, rather an early beta of sorts).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants