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

E2EE: Introduce EncryptionManager with uploadIdentityKeys and uploadOneTimeKeys API. #329

Merged
merged 13 commits into from Jul 7, 2019

Conversation

Projects
None yet
4 participants
@a-andreyev
Copy link
Member

commented Jun 25, 2019

Contributes to #87 #88 #95

Fxrh and others added some commits Oct 1, 2016

Introduce EncryptionEvent class
This allows to detect if a room has been encrypted (no room state, just
an event as of yet). Closes #84.

@a-andreyev a-andreyev requested a review from KitsuneRal Jun 25, 2019

@a-andreyev a-andreyev force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch from d31a839 to 590a59a Jun 25, 2019

@KitsuneRal
Copy link
Collaborator

left a comment

Thanks for a great start! This is part 1 of the review, only dealing with code organisation but not with the source code itself. You've got some work to do before this can be merged :)

Show resolved Hide resolved .gitmodules Outdated
Show resolved Hide resolved .gitmodules Outdated
Show resolved Hide resolved .travis.yml Outdated
Show resolved Hide resolved .travis.yml Outdated
Show resolved Hide resolved CMakeLists.txt Outdated
Show resolved Hide resolved CMakeLists.txt Outdated

@a-andreyev a-andreyev force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch 3 times, most recently from 45d417b to 2939a05 Jun 26, 2019

@KitsuneRal
Copy link
Collaborator

left a comment

A few comments on the source code. Might bring more, but first let's deal with the base class of EncryptionEvent.

Show resolved Hide resolved lib/events/encryptionevent.h Outdated
Show resolved Hide resolved lib/events/simplestateevents.h Outdated
Show resolved Hide resolved lib/room.cpp Outdated
Show resolved Hide resolved lib/settings.cpp Outdated
Show resolved Hide resolved lib/settings.cpp Outdated
Show resolved Hide resolved lib/settings.cpp Outdated

@a-andreyev a-andreyev force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch 6 times, most recently from a5e444c to ee6281e Jun 27, 2019

@KitsuneRal
Copy link
Collaborator

left a comment

I finally really got to seriously reviewing your code - and you've got more work as a result :)

Show resolved Hide resolved .gitmodules
Show resolved Hide resolved lib/connection.cpp Outdated
Show resolved Hide resolved 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/encryptionmanager.cpp Outdated
Show resolved Hide resolved lib/encryptionmanager.cpp Outdated
Show resolved Hide resolved lib/encryptionmanager.cpp Outdated
Show resolved Hide resolved lib/encryptionmanager.cpp Outdated

@a-andreyev a-andreyev force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch from ee6281e to d49fa06 Jul 2, 2019

@KitsuneRal
Copy link
Collaborator

left a comment

Ok, a few last brushes and it seems we're good to go.

Show resolved Hide resolved lib/connection.cpp Outdated
Show resolved Hide resolved lib/encryptionmanager.cpp
Show resolved Hide resolved lib/encryptionmanager.cpp
Show resolved Hide resolved lib/events/encryptionevent.h Outdated
Show resolved Hide resolved lib/events/encryptionevent.h Outdated

@a-andreyev a-andreyev force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch 2 times, most recently from d02c250 to 14c0437 Jul 3, 2019

@KitsuneRal
Copy link
Collaborator

left a comment

...and a few more brushes below, to satisfy CI.

By the way, Travis fails because https://stackoverflow.com/questions/50061678/cmake-the-following-imported-targets-are-referenced-but-are-missing. cmake/QMatrixClientConfig.cmake is already there, you just have to add the dependency there, as described in the answer.

Show resolved Hide resolved CMakeLists.txt
Show resolved Hide resolved lib/encryptionmanager.cpp
Show resolved Hide resolved .travis.yml

a-andreyev added some commits May 24, 2019

@a-andreyev a-andreyev force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch 3 times, most recently from f13cc71 to a5a8d88 Jul 4, 2019

@a-andreyev a-andreyev force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch from a5a8d88 to abe1d95 Jul 4, 2019

@a-andreyev a-andreyev force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch from abe1d95 to b5f9e1b Jul 4, 2019

@a-andreyev

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

https://stackoverflow.com/questions/50061678/cmake-the-following-imported-targets-are-referenced-but-are-missing

tried to follow the logic (also tried with QtOlmConfig.cmake), no luck yet. I got stuck a bit with the CI, but the local build is working. So I'm focusing at next main tasks (but would be happy to check some more hints and fix the CI too :)

@KitsuneRal

This comment has been minimized.

Copy link
Collaborator

commented Jul 6, 2019

tried to follow the logic (also tried with QtOlmConfig.cmake), no luck yet. I got stuck a bit with the CI, but the local build is working. So I'm focusing at next main tasks (but would be happy to check some more hints and fix the CI too :)

Yep, don't spend more time on this. Looking forward to messages receiving code.

static const auto ed25519Name = QStringLiteral("ed25519");
static const auto Curve25519Name = QStringLiteral("curve25519");
static const auto SignedCurve25519Name = QStringLiteral("signed_curve25519");
static const auto OlmCurve25519AesSha256AlgoName = QStringLiteral("m.olm.curve25519-aes-sha256");

This comment has been minimized.

Copy link
@a-andreyev

a-andreyev Jul 6, 2019

Author Member

Note to myself

found an issue with the naming: matrix-org/matrix-doc#1733 (comment) (point 2)

@KitsuneRal KitsuneRal force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch 2 times, most recently from 1a48753 to 93044c7 Jul 7, 2019

@KitsuneRal KitsuneRal force-pushed the a-andreyev:aa13q-e2ee-enc-mng branch from 93044c7 to d5b4e64 Jul 7, 2019

@KitsuneRal

This comment has been minimized.

Copy link
Collaborator

commented Jul 7, 2019

Congratulations, the first E2EE PR comes in :)

@KitsuneRal KitsuneRal merged commit 6c9d895 into quotient-im:master Jul 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.