Page MenuHomePhabricator

Implement prekey lifecycle in `CryptoModule`.
ClosedPublic

Authored by marcin on Apr 11 2023, 5:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 14, 2:42 AM
Unknown Object (File)
Fri, Mar 8, 1:57 AM
Unknown Object (File)
Fri, Mar 8, 1:56 AM
Unknown Object (File)
Fri, Mar 8, 1:56 AM
Unknown Object (File)
Fri, Mar 8, 1:56 AM
Unknown Object (File)
Fri, Mar 8, 1:56 AM
Unknown Object (File)
Fri, Mar 8, 1:56 AM
Unknown Object (File)
Fri, Mar 8, 1:56 AM
Subscribers

Details

Summary

This differential wraps olm methods for prekey rotation in CryptoModule.

Test Plan

In CommCoreModule::getUserPublicKeys place the following lines and attach XCode debugger to each line (for Android log output of each line and examine logs in logcat):

std::uint8_t numPrekeys = this->cryptoModule->getNumPrekeys(); // should be zero
std::string firstPrekey = this->cryptoModule->generateAndGetPrekey();
std::string secondPrekey = this->cryptoModule->generateAndGetPrekey();
std::uint8_t numPrekeys_1 = this->cryptoModule->getNumPrekeys();//should be 2
std::string prekey_1 = this->cryptoModule->getPrekey();// should be as prekey 2
std::string prekey_2 = this->cryptoModule->getUnpublishedPrekey();// should be as prekey 2
this->cryptoModule->forgetOldPrekey();
std::uint8_t numPrekeys_2 = this->cryptoModule->getNumPrekeys();// should be 1
this->cryptoModule->markPrekeyAsPublished();
std::string secondPrekey_3 = this->cryptoModule->getUnpublishedPrekey();// should crash

Ensure the output of each line matches what is written in the comments.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cc @anunay@marcin already has a diff up with X3DH, one day after we landed it! Exciting

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
118 ↗(On Diff #24965)
133 ↗(On Diff #24965)

Hmm, I wonder if we should wait until the server "acks" that the prekey has been published before calling this

138 ↗(On Diff #24965)

I'm not sure exactly where this function will be called, but I suspect we should probably call unpublished_prekey (I might have that name wrong) instead. That way we are only grabbing the prekey if it has not already been published. Otherwise we will repeatedly send the same prekey to the server

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
118 ↗(On Diff #24965)

I took this pattern from: https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/CryptoTools/CryptoModule.h#L51, where we already use camel case for prekey.

138 ↗(On Diff #24965)

Otherwise we will repeatedly send the same prekey to the server

This function calls olm_account_generate_prekey at the beginning, which will generate new prekey each time it is called so each call to this method will result in new prekey being sent to keyserver.
I plan to call this method in the same place we send signed identity keys to the keyserver to pass notifications prekey to keyserver to initialize e2e session for notifications.

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
118 ↗(On Diff #24965)

Ah we should fix that. In the Olm library we say eg. generate_prekey instead of generate_pre_key... so "prekey" is treated as one word

138 ↗(On Diff #24965)

Ah okay, that makes sense if generate_prekey always generates a new prekey

atul requested changes to this revision.Apr 12 2023, 9:58 AM

Is this ready for review, or are there changes planned? Sending to your queue, but feel free to re-request review

This revision now requires changes to proceed.Apr 12 2023, 9:58 AM
native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
133 ↗(On Diff #25083)

From my last review:

Hmm, I wonder if we should wait until the server "acks" that the prekey has been published before calling this

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
133 ↗(On Diff #25083)

I may be missing something but I am not sure about potential benefits of waiting to mark the prekey as published until keyserver send success response. We agreed on generating new notifications prekey everytime user logs in. Therefore even if the keyserver sends failure response, we will use new prekey when trying again to establish olm session for notifs.

However it may be that your concern is not about whether this method suits notifs case, but whether it suits general case. If that is the case, I would suggest that we add boolean flag as an argument that tells whether to mark the key as published immediately (set it to true in the notifs case) and introduce additional method for CryptoModule to mark the prekey as published (in case someone calls this method in a different context).

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
133 ↗(On Diff #25083)

The boolean flag is an okay interim solution. I worry a little bit about how much we're focused on shipping the immediate project, versus building the long-term solution. By immediately marking the prekey as published, we are making it so it does not get returned when unpublished_prekey is called, which means it would not get published at all in the long-term solution (where we should only look at unpublished_prekey when determining which prekey to publish)

If you want to go with an "interim solution", instead of a boolean flag maybe we just rename this method to generatePrekeyAndMarkAsPublished, and create a follow-up task to address it later.

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
133 ↗(On Diff #25083)

Thanks!
Now that I have got the whole picture I will go with the following approach:

  1. In CryptoModule I will implement more fine-grained methods to get the full control over the prekey lifecycle that will be good long term.
  2. In NotificationsCryptoModule I will keep the current solution, by having one bulk method to generate fresh prekey and mark it as published immediately.

Fine grained methods for prekey rotation

marcin edited the test plan for this revision. (Show Details)
native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
159–162 ↗(On Diff #25208)

I don't think this method should throw if there are no unpublished prekeys...

In the eventual usage, I imagine it will get called every time the client connects with the identity service, to check if there is anything to publish. If it returns "falsey" we will know not to publish anything; otherwise, if it returns "truthy" then we will have something to publish.

Can we instead return folly::Optional<std::string>?

171–172 ↗(On Diff #25208)

Does ::olm_account_generate_prekey need the buffer to be filled with random bytes, or can it be zeros or unitialized?

185–189 ↗(On Diff #25208)

Do we really need to "get" the prekey after generating? Does the buffer we pass to ::olm_account_generate_prekey get filled with the generated prekey?

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
159–162 ↗(On Diff #25208)

I think folly::Optional<std::string> will be better.

171–172 ↗(On Diff #25208)

It definitely needs random bytes. Two reasons:

  1. Each place in CryptoModule that generates some type of key takes a buffer that was randomly initialized in the same way. So I believe original authors of CryptoModule did necessary research.
  2. While olm is platform independent library, generating securely random bytes needs randomness provided by hardware so it is platform dependent. So olm cannot generate random bytes by itself but requires that we provide them ourselves based on some hardware random bytes generator.
185–189 ↗(On Diff #25208)

Do we really need to "get" the prekey after generating?

It is convenient that this method returns already generated prekey. If a caller is only interested in generating prekey they will ignore returned value. But it they are indeed interested in the value of new prekey they won't have to issue a call twice.

Does the buffer we pass to ::olm_account_generate_prekey get filled with the generated prekey?

Each function in olm that generates new key stores it inside olm account. To extract the generated key we need to call another method. This buffer is to provide random bytes olm cannot generate itself and it is not filled with newly generated key.

Defer to @ashoat since he already started giving feedback + has more context

marcin retitled this revision from Add method to CryptoModule to generate new prekey to Implement prekey lifecycle in `CryptoModule`..Apr 17 2023, 8:08 AM
ashoat requested changes to this revision.Apr 17 2023, 10:22 AM

Thanks for explaining @marcin! To your queue for folly::Optional<std::string>

This revision now requires changes to proceed.Apr 17 2023, 10:22 AM

Folly optional instead of throw

This revision is now accepted and ready to land.Apr 21 2023, 6:18 AM
  1. getPrekey() always returns std::string now.
  2. Remove unecessary checks for num_prekeys != 0

Adjust to new olm unpublished_prekey implementation