Page MenuHomePhabricator

[crypto] unify one-time keys usage
ClosedPublic

Authored by kamil on Feb 6 2024, 2:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 3:14 AM
Unknown Object (File)
Sat, Nov 23, 2:26 AM
Unknown Object (File)
Thu, Nov 7, 9:23 AM
Unknown Object (File)
Wed, Nov 6, 4:41 AM
Unknown Object (File)
Oct 23 2024, 12:12 AM
Unknown Object (File)
Oct 18 2024, 3:05 PM
Unknown Object (File)
Oct 18 2024, 3:05 PM
Unknown Object (File)
Oct 18 2024, 3:05 PM
Subscribers

Details

Summary

ENG-6577.

  1. Remove getOneTimeKeyArray function.
  2. Updates getOneTimeKeyArray callsites to getOneTimeKeyValues.
  3. Remove parsing by offsets from createSessionAsInitializer.
  4. Remove the keyIndex argument - it's not used, Identity returns only one one-time key.
  5. Rename oneTimeKeys -> oneTimeKey.
  6. Remove calling getOneTimeKeyValuesFromBlob - we now expect short keys in session creators.
  7. Move parsing long-format to getOlmSessionInitializationData. The get_olm_session_initialization_data endpoint returns keys in long format, so parse it immediately after receiving and in the rest of the codebase we assume we operate on short format. I decided to not update get_olm_session_initialization_data to return short keys as it will be deprecated soon so we can avoid handling backward compatibility - old clients still expect long format.

Adding all three reviewers as blocking - all worked on code related to session creation and keys, to make sure we are all on the same page about one-time keys usage.

Test Plan
  1. Creating notif sesssion on native works (not testing notifs itself, just session creation).
  2. Creating notif session on web.
  3. Creating content session on native.
  4. Keyserver during login uploads short keys.
  5. All devices as response to RefreshKeyRequest uploads keys in short format.

Diff Detail

Repository
rCOMM Comm
Branch
land-one-time-keys
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Feb 6 2024, 3:09 AM
kamil edited the summary of this revision. (Show Details)
tomek added inline comments.
lib/actions/user-actions.js
701–703 ↗(On Diff #36671)

Should we check if this value is present?

native/cpp/CommonCpp/CryptoTools/Session.cpp
74–76 ↗(On Diff #36671)

If we're modifying the message, should we also modify a function name to be singular?

This revision is now accepted and ready to land.Feb 7 2024, 6:56 AM

rebase before landing

lib/actions/user-actions.js
701–703 ↗(On Diff #36671)

we didn't in the previous code so I think we can do the same?

native/cpp/CommonCpp/CryptoTools/Session.cpp
74–76 ↗(On Diff #36671)

olm_remove_one_time_keys is olm function so we can't change it - but in our case, we know we are removing only one so it probably makes sense

we need to update native/identity-service/identity-service-context-provider.react.js still, right?

This revision was landed with ongoing or failed builds.Feb 8 2024, 10:16 AM
This revision was automatically updated to reflect the committed changes.

we need to update native/identity-service/identity-service-context-provider.react.js still, right?

Looks like it was done in D10455 - wasn't it?

Here is the note: https://phab.comm.dev/D10455#inline-66286

we need to update native/identity-service/identity-service-context-provider.react.js still, right?

Looks like it was done in D10455 - wasn't it?

Here is the note: https://phab.comm.dev/D10455#inline-66286

ah sorry forgot i did it myself lol