Page MenuHomePhabricator

[CommCoreModule] implement creating inbound `olm` session
ClosedPublic

Authored by kamil on Dec 18 2023, 4:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 20, 7:22 PM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:09 AM
Unknown Object (File)
Fri, Oct 18, 1:08 AM
Unknown Object (File)
Oct 5 2024, 8:40 AM
Subscribers

Details

Summary

Implemented method to creating olm session.

Depends on D10375

Test Plan

This code is really hard to test as it's impossible to verify partial results.
I generated some keys and values, verified them with olm-utils.test.js, and checked if they succeeded/failed when calling the method introduced here in the same cases as JS tests.
Full functionality tested later in the stack.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Dec 18 2023, 6:51 AM
kamil added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
796–797 ↗(On Diff #34790)

wondering if we should add default 0 type

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
796–797 ↗(On Diff #34790)

Not sure if this is what you're asking about, but I think all messages we send via Olm should be a structured JSON blob

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
796–797 ↗(On Diff #34790)

While decrypting, we need to manually add message type like here which I believe it's always 1, only when decrypting for the very first time is 0, and was wondering if we need to do it explicitly because it work the way it is right now

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
796–797 ↗(On Diff #34790)

Ah, I understand the question now. I'm not sure, but I'd be worried that default initialization of the size_t messageType parameter of struct EncryptedData might not always initialize to 0. My vague memory of C / C++ default initialization is that if you don't explicitly initialize to 0, it might be a "garbage value".

Separately, my comment above was actually about the contents of the Olm-encrypted message. I think we should avoid encrypted raw text data, and instead make sure all of the text we're encrypted is "structured". This will help future-proof our system, for when we might have multiple categories of encrypted message.

marcin added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
796–797 ↗(On Diff #34790)

It would be best if messageType field of EncryptedData structure was an enum with two possible values. If you think it is out of the scope of this diff, then please add sth like const prekeyOlmMessageType = 0; similarly as you did with the content of initial encrypted message returned from the counterpart method. Relying on C++ default initialization here is brittle and unreadable

This revision is now accepted and ready to land.Dec 21 2023, 3:07 AM
marcin requested changes to this revision.Dec 21 2023, 3:21 AM
marcin added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
800 ↗(On Diff #34790)

This comment is relevant here as well.

This revision now requires changes to proceed.Dec 21 2023, 3:21 AM
marcin requested changes to this revision.Jan 2 2024, 11:50 PM
marcin added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
796 ↗(On Diff #35142)

This diff is almost LGTM, but there is one last change we need to introduce. We need to remove one time key from the account when creating inbound session just like we do on the keyserver.

However after a quick glance at CryptoModule.cpp I realized that we don't even have an API in this class to remove a one time key after indound session creation!
@kamil do you think you could modify CryptoModule::initializeInboundForReceivingSession method so that after session creation one time key is removed from account by calling appropriate olm function? Ideally we should create a Linear task for that since it is indeed a bug and a separate diff but If it is too much work then I think it is fine as well to amend bug fix into this diff.

This revision now requires changes to proceed.Jan 2 2024, 11:50 PM

implement removing one-time keys

This revision is now accepted and ready to land.Jan 8 2024, 2:14 AM