This differential wraps olm methods for prekey rotation in CryptoModule.
Details
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
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) |
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. |
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 |
Is this ready for review, or are there changes planned? Sending to your queue, but feel free to re-request review
native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp | ||
---|---|---|
133 ↗ | (On Diff #25083) | From my last review:
|
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!
|
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:
|
185–189 ↗ | (On Diff #25208) |
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.
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. |
- getPrekey() always returns std::string now.
- Remove unecessary checks for num_prekeys != 0