Page MenuHomePhabricator
Differential D10977

[native] update one time keys logic
ClosedPublic
Actions

Authored by varun on Feb 6 2024, 2:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 25, 4:30 AM
Unknown Object (File)
Wed, Dec 25, 4:30 AM
Unknown Object (File)
Wed, Dec 25, 4:30 AM
Unknown Object (File)
Wed, Dec 25, 4:30 AM
Unknown Object (File)
Fri, Nov 29, 10:02 PM
Unknown Object (File)
Tue, Nov 26, 4:15 PM
Unknown Object (File)
Nov 22 2024, 7:42 PM
Unknown Object (File)
Nov 8 2024, 5:12 PM
View All 52 Files
Subscribers
tomek

Details

Reviewers
ashoat
kamil
marcin
Commits
rCOMM75d0111308c5: [native] update one time keys logic
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

varun created this revision.Feb 6 2024, 2:52 PM
Herald added a subscriber: tomek. · View Herald TranscriptFeb 6 2024, 2:52 PM
varun edited the test plan for this revision. (Show Details)Feb 6 2024, 3:00 PM
varun added inline comments.Feb 6 2024, 3:08 PM
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

Harbormaster completed remote builds in B26595: Diff 36751.Feb 6 2024, 3:10 PM
varun requested review of this revision.Feb 6 2024, 3:10 PM
varun edited the test plan for this revision. (Show Details)Feb 6 2024, 3:25 PM
marcin accepted this revision.Feb 7 2024, 1:24 AM
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 accepted this revision.Feb 7 2024, 7:38 AM
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

ashoat added a comment.Feb 7 2024, 10:03 AM

Should this diff have a stack?

varun added a comment.EditedFeb 7 2024, 10:10 AM
In D10977#317023, @ashoat wrote:

Should this diff have a stack?

no, it's a standalone diff

varun added a child revision: D10927: [native] validateAndGetPrekeys method.Feb 7 2024, 10:11 AM
varun removed a child revision: D10927: [native] validateAndGetPrekeys method.Feb 7 2024, 10:25 AM
ashoat accepted this revision.Feb 7 2024, 10:47 AM

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
ashoat added inline comments.Feb 7 2024, 11:49 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.

varun added inline comments.Feb 7 2024, 2:54 PM
native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
160 ↗(On Diff #36751)

will adjust retrieveAccountKeysSet in https://linear.app/comm/issue/ENG-6688/update-retrieveaccountkeysset-to-only-generate-enough-one-time-keys-to

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)

https://linear.app/comm/issue/ENG-6696/use-jsivaluefromdynamic-in-parseolmonetimekeys

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
Closed by commit rCOMM75d0111308c5: [native] update one time keys logic (authored by varun). · Explain WhyFeb 7 2024, 3:21 PM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMM75d0111308c5: [native] update one time keys logic.
kamil mentioned this in D11119: [crypto] implement getting one-time keys in `OlmAPI`.Feb 20 2024, 5:28 AM
kamil mentioned this in rCOMMb5559ffbc9fc: [crypto] implement getting one-time keys in `OlmAPI`.Feb 26 2024, 5:46 AM

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
CryptoTools/
2 lines
46 lines
NativeModules/
5 lines
60 lines
Notifications/
BackgroundDataStorage/
2 lines
4 lines
_generated/
handlers/
8 lines
identity-service/
8 lines
schema/
12 lines

Diff 36817

View Options

native/cpp/CommonCpp/CryptoTools/CryptoModule.h

Loading...
View Options

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp

Loading...
View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

Loading...
View Options

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.h

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp

Loading...
View Options

native/cpp/CommonCpp/_generated/commJSI-generated.cpp

Loading...
View Options

native/cpp/CommonCpp/_generated/commJSI.h

Loading...
View Options

native/handlers/peer-to-peer-message-handler.js

Loading...
View Options

native/identity-service/identity-service-context-provider.react.js

Loading...
View Options

native/schema/CommCoreModuleSchema.js

Loading...