Page MenuHomePhabricator

[native] update one time keys logic
ClosedPublic

Authored by varun on Feb 6 2024, 2:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 5:12 PM
Unknown Object (File)
Fri, Nov 8, 11:10 AM
Unknown Object (File)
Fri, Oct 18, 3:05 PM
Unknown Object (File)
Fri, Oct 18, 3:05 PM
Unknown Object (File)
Fri, Oct 18, 3:04 PM
Unknown Object (File)
Fri, Oct 18, 7:10 AM
Unknown Object (File)
Sep 4 2024, 5:09 AM
Unknown Object (File)
Aug 14 2024, 6:48 AM
Subscribers

Details

Summary

A lot of changes here... will annotate all of them inline. Here are the big things:

  • consolidated getPrimaryOneTimeKeys and getNotificationsOneTimeKeys into a single API, getOneTimeKeys
  • make sure we only generate one-time keys if we don't have enough unpublished one-time keys
  • rename CryptoModule::getOneTimeKeys to CryptoModule::getOneTimeKeysForPublishing so that it's clear to the caller that the returned keys should be published immediately
Test Plan

test patch

  1. Modified getOneTimeKeysForPublishing to generate one unpublished one-time key
  2. called `commCoreModule.getOneTimeKeys(10) twice from JS
  3. confirmed that the one unpublished key appeared in otks but not otks2 (all the keys in otks get marked as published so they are no longer accessible on the second call)
  4. confirmed that otks2 was a brand new set of 10 keys
  5. (not in patch) called commCoreModule.getOneTimeKeys(101) and got an error: {"message": "error generateKeys => invalid amount of one-time keys published. Expected 101, got 100"}. this is expected since the account can only hold 100 one-time keys.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
72–74 ↗(On Diff #36751)

this check didn't make sense to me and looked incorrect. we aren't reading from or writing to this->keys.oneTimeKeys in this method.

157 ↗(On Diff #36751)

unfortunately there is no function to get the number of unpublished keys in the olm library, so we have to use folly to convert the buffer to JSON and check the size of parsedUnpublishedKeys["curve25519"]

160 ↗(On Diff #36751)

only generate enough one-time keys to reach oneTimeKeysAmount

native/cpp/CommonCpp/CryptoTools/CryptoModule.h
46 ↗(On Diff #36751)

50 was arbitrary based on discussion in D1843 and didn't match the default of 10 that we use on web and keyserver

varun requested review of this revision.Feb 6 2024, 3:10 PM
marcin added inline comments.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
591 ↗(On Diff #36751)

As I pointed in D10927 - please early reject and return here so that we don't have to wrap try ... catch with else and indentation is reduced.

kamil added inline comments.
native/handlers/peer-to-peer-message-handler.js
67–68 ↗(On Diff #36751)

same comment as below

native/identity-service/identity-service-context-provider.react.js
220–221 ↗(On Diff #36751)

we should update this here or in D10964 - depending on who lands diff first

Should this diff have a stack?

Should this diff have a stack?

no, it's a standalone diff

Please rename NotificationsCryptoModule::getNotificationsOneTimeKeys before landing

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
550–566 ↗(On Diff #36751)

Can this be replaced with jsi::valueFromDynamic?

folly::dynamic parsedOneTimeKeys = folly::parseJson(oneTimeKeysBlob);
jsi::Value jsiOneTimeKeys = jsi::valueFromDynamic(rt, parsedOneTimeKeys);
return jsiOneTimeKeys.asObject(rt);

You'll need to import this header to use it. This should probably be handled as a separate diff.

591 ↗(On Diff #36751)

Agree with this

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
239 ↗(On Diff #36751)

Shouldn't this be renamed to include the ForPublishing suffix as well?

This revision is now accepted and ready to land.Feb 7 2024, 10:47 AM
native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
160 ↗(On Diff #36751)

This is inconsistent with retrieveAccountKeysSet. We should adjust either one or the other to be consistent

164–170 ↗(On Diff #36751)

It's not clear to me why we need this check, and I suspect it will make it harder to deprecate this->keys, since publishOneTimeKeys will need to return both this number, as well as the actual one-time keys that need to be published to identity.

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
160 ↗(On Diff #36751)
164–170 ↗(On Diff #36751)

removing this check. the only case where publishedOneTimeKeys should be different from oneTimeKeysAmount is when oneTimeKeysAmount is greater than 100 (MAX_ONE_TIME_KEYS), but we will add better error handling for that case in https://linear.app/comm/issue/ENG-6689/add-a-check-in-commcoremodule-and-cryptomodule-to-make-sure-that

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
550–566 ↗(On Diff #36751)
591 ↗(On Diff #36751)

makes sense

native/identity-service/identity-service-context-provider.react.js
220–221 ↗(On Diff #36751)

landing this now so will probably have to update in D10964

varun marked 8 inline comments as done.Feb 7 2024, 3:18 PM
This revision was automatically updated to reflect the committed changes.