This differential implements an endpoint on the keyserver to get data necessary to initialize olm session with the client.
Details
Details
Call commCoreModule.initializeCryptoAccount() (it is called in log-in-panel via nativeLogInExtraInfoSelector). This ensures notifications session is initialized. Then call this endpoint to get necessary data to initialize sesion for notifications. Pass obtained data to
commCoreModule.initializeNotificationsSession().
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I think we'll need to find a way to synchronize any updates to Olm accounts / sessions so that we only do one such update at a time
| keyserver/src/responders/keys-responders.js | ||
|---|---|---|
| 57 ↗ | (On Diff #25607) | There is a serious issue here, which is that we are not returning a signed prekey. I created ENG-3754 to track |
| 66 ↗ | (On Diff #25607) | There appears to be a serious risk of a data race here causing one update to overwrite another |
| keyserver/src/responders/keys-responders.js | ||
|---|---|---|
| 37 ↗ | (On Diff #25607) | Let's name this getOlmNotifSessionInitializationDataResponder and only support notifications |
| 57 ↗ | (On Diff #25607) | Clarification: this should be signed by the notif identity's signing key. The client will trust that, because it is inside the signed identity keys blob, which is itself signed by the primary identity's signing key |
| 63 ↗ | (On Diff #25607) | Let's avoid sending these identity keys... instead, let's return the signed identity keys blob |
| keyserver/src/responders/keys-responders.js | ||
|---|---|---|
| 37 ↗ | (On Diff #25607) | Done renaming |
| 57 ↗ | (On Diff #25607) | Will be monitoring: https://linear.app/comm/issue/ENG-3754/signing-the-prekey. If it is not done by the time I want to land this diff, I will sign prekey here and send signature with the response to the client, where it will be verified with crypto::CryptoModule::verifySignature(...); |
| 66 ↗ | (On Diff #25607) | Novel approach should mitigate the risk. |
Comment Actions
- Use transactional olm account updater.
- Rename to getOlm{Notifs}SessionInitializationData...`.
- Respond with SignedIdentityKeysBlob instead of plain identity keys.
| keyserver/src/responders/keys-responders.js | ||
|---|---|---|
| 38 ↗ | (On Diff #25818) | We can remove this and assume the client will always want just a single one-time key |
| 54 ↗ | (On Diff #25818) | We can call account.prekey_signature() here to get the prekey signature |
| 62 ↗ | (On Diff #25818) | We'll need to do this for the content account as well |
| 58 ↗ | (On Diff #25761) | Did you mean this? |
| lib/types/request-types.js | ||
| 202 ↗ | (On Diff #25818) | Since this endpoint will only be used for a client to connect with the keyserver, I think we will always ask for a single one-time key, and we can remove GetOlmNotifsSessionInitializationDataRequest altogether |
| 205–209 ↗ | (On Diff #25818) | Following the discussion in today's encryption sync, I think this type should look like this: type OlmAccountSessionInitializationInfo = { +prekey: string, +prekeySignature: string, +oneTimeKey: string, }; export type GetOlmSessionInitializationDataResponse = { +signedIdentityKeysBlob: SignedIdentityKeysBlob, +contentInitializationInfo: OlmAccountSessionInitializationInfo, +notifInitializationInfo: OlmAccountSessionInitializationInfo, }; |
| 207 ↗ | (On Diff #25761) | Based on this comment on Linear, it appears that we will need to also include a prekey signature here. (Worth confirming with @anunay) |
Comment Actions
(Removing most reviewers to clear out their queues, feel free to re-add yourself if you're interested)
| keyserver/src/responders/keys-responders.js | ||
|---|---|---|
| 41 ↗ | (On Diff #25818) | We should rename this (and all related names) to not mention "Notifs" anymore, since it will have to initialize both OlmAccounts now |
| lib/types/request-types.js | ||
|---|---|---|
| 205–209 ↗ | (On Diff #25818) | Looking at the Linear issues it looks like we don't need to send prekey signature anymore |
| lib/types/request-types.js | ||
|---|---|---|
| 205–209 ↗ | (On Diff #25818) | Please ignore the comment above. |
Comment Actions
Great work here! Mostly requesting changes to get rid of the any type. You'll also potentially need to refactor the use of fetchCallUpdateOlmAccount following my feedback in D7568
| keyserver/src/responders/keys-responders.js | ||
|---|---|---|
| 50 ↗ | (On Diff #26002) | My feedback here was not addressed |
| 81 ↗ | (On Diff #26002) | You should never submit a diff with any. If you do, you should always annotate and attempt to justify it. Please find a way to remove this any |
| 117 ↗ | (On Diff #26002) | Get rid of any |
| keyserver/src/responders/keys-responders.js | ||
|---|---|---|
| 114 ↗ | (On Diff #26098) | My feedback about any was not addressed here |