Page MenuHomePhabricator

Implement utilities and database query wrapper to create new olm session on eht keyserver side
ClosedPublic

Authored by marcin on Apr 26 2023, 6:41 AM.
Tags
None
Referenced Files
F1657434: D7631.id27209.diff
Wed, Apr 24, 3:54 PM
Unknown Object (File)
Sun, Apr 21, 7:22 AM
Unknown Object (File)
Wed, Apr 17, 11:40 PM
Unknown Object (File)
Wed, Apr 17, 5:56 PM
Unknown Object (File)
Wed, Apr 17, 5:56 PM
Unknown Object (File)
Mon, Apr 15, 3:55 PM
Unknown Object (File)
Mon, Apr 15, 3:47 PM
Unknown Object (File)
Mon, Apr 15, 3:47 PM
Subscribers

Details

Summary

This differential implements relevant olm utils and creator to create olm session for particular cookie id with initial encrypted message provided by the client.

Test Plan

Modify keyserver codebase so that it prints: prekey, one time keys and notifications identity keys to the console. Use provided keys to initialize olm session on the client side and generate initial encrypted message. Call olm-session-creator with hardcoded initial encrypted
message in logInResponder (so that we have valid cookie to use). Examine database content using preferred tool of choice. there should be an entry with pickled olm session.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Throw server error when olm methods fail.

ashoat requested changes to this revision.Apr 27 2023, 12:42 PM

Looks good!! Just a couple comments

keyserver/src/creators/olm-session-creator.js
22 ↗(On Diff #25819)

Do we need to use this function given we're just doing a read? I think the function call is only necessary if we need to lock for writes.

Instead of calling this function and then separately querying for the pickling_key below, I think it would be better to replace both with a single SQL query that fetches both the account and the pickling key

30 ↗(On Diff #25819)

In an earlier diff I requested a name change of this table. I think updating this sooner rather than later will save you some refactoring work

keyserver/src/utils/olm-utils.js
54 ↗(On Diff #25819)

I don't think we need Pickled in the name here

This revision now requires changes to proceed.Apr 27 2023, 12:42 PM
keyserver/src/creators/olm-session-creator.js
22 ↗(On Diff #25819)

Do we need to use this function given we're just doing a read? I think the function call is only necessary if we need to lock for writes.

We need since if someone if performing a write operation we need to wait with reading until they are done.

keyserver/src/creators/olm-session-creator.js
22 ↗(On Diff #25819)

Why do we need to wait for the write to complete? Is there something bad that can happen in this scenario, that wouldn't happen in a scenario where createOlmSession got called right before an update? I assume we (and Olm) need to be resistant to the second scenario, so my assumption is that the first scenario will work as well

marcin added inline comments.
keyserver/src/creators/olm-session-creator.js
22 ↗(On Diff #25819)

We need to answer some questions here:

  1. Does creating a new session using an account changes internal state of an account - does it change its pickled content we keep in the db. If the answer is no, then we can do unblocking read and skip updating database.
  2. If the answer to the first question is yes, then we need to answer another question. Is olm able to recover if we skip persisting updated olm account in the db after creating a session. If the answer to this question is yes, we can do unblocking read and skip updating database.
  3. If the answer to the second question is no, then we have to ensure the sequence read -> create session-> update db executes atomically. Imagine a case when we read olm account with prekey older than 30 days and use it the to create olm session. When we are busy creating olm session the cron generates new prekey and persists to the db an account with new prekey. Once we are done creating new olm session we persist to the db an account with the old prekey and we wait another 24 hours before we create new one. This is just an example but I hope it presents my point well.

cc @anunay

keyserver/src/creators/olm-session-creator.js
22 ↗(On Diff #25819)
  1. I think no. It may be worth confirming this... perhaps the easiest way would be to modify @anunay's unit tests in D7670 to create a new session, and then to check to see if the contents of the pickled account have changed.
  2. N/A
  3. I am not sure I follow your concern about this scenario. It should be okay for the native/web client to create an outgoing session using the keyserver's old prekey, since the keyserver will keep the old prekey for 24 hours. I'm not sure what you mean when you say "Once we are done creating new olm session we persist to the db an account with the old prekey"... the olm-session-creator.js code you introduce here does not seem to save accounts (just sessions). As for the cronjob, it will save the new account, but the old prekey will still be there, so it should not cause any problems.
keyserver/src/creators/olm-session-creator.js
22 ↗(On Diff #25819)

the olm-session-creator.js code you introduce here does not seem to save accounts (just sessions)

It does. fetchCallUpdate saves account after calling lambda.

Scenario with the prekey was just an example. My point is that if creating new olm session alters pickled content of an account then we must ensure atomicity in sequence read->create session -> write. Otherwise changes we made when creating session might be lost when something else writes. If there are no changes when creating a session, or loosing those changes can be handled by olm, then we can bypass fetchCallUpdate function and just read relevant information from the db. However if we can't loose those changes we need atomicity and fetchCallUpdate guarantees that.

keyserver/src/creators/olm-session-creator.js
22 ↗(On Diff #25819)

the olm-session-creator.js code you introduce here does not seem to save accounts (just sessions)

It does. fetchCallUpdate saves account after calling lambda.

My whole point is that fetchCallUpdateOlmAccount does not need to be called. If you take my solution, this will not be a problem.

Scenario with the prekey was just an example. My point is that if creating new olm session alters pickled content of an account then we must ensure atomicity in sequence read->create session -> write. Otherwise changes we made when creating session might be lost when something else writes. If there are no changes when creating a session, or loosing those changes can be handled by olm, then we can bypass fetchCallUpdate function and just read relevant information from the db. However if we can't loose those changes we need atomicity and fetchCallUpdate guarantees that.

I agree with this. I am pretty sure that creating a new Olm session does not alter pickled content of the account. As suggested in my previous comment, you can confirm this by testing.

keyserver/src/creators/olm-session-creator.js
22 ↗(On Diff #25819)

Confirmed by testing that session creation does not alter pickled content of olm account.

Non-blocking fetch of account and pickling key

Nice work!

keyserver/src/creators/olm-session-creator.js
32 ↗(On Diff #26003)

Worth confirming in your test plan that this works correctly for inserting a boolean value

This revision is now accepted and ready to land.May 2 2023, 10:03 AM

Decrypt initial message from the client during session instantiation on the kesyerver

Remove one time keys from the account during session creation

ashoat requested changes to this revision.May 12 2023, 9:30 AM
ashoat added inline comments.
keyserver/src/creators/olm-session-creator.js
1 ↗(On Diff #26408)

Please add a newline here

15–21 ↗(On Diff #26408)

This can be simplified

keyserver/src/utils/olm-utils.js
55 ↗(On Diff #26408)

Let's avoid passing constants as number literals like this. Instead, please define something like eg. threadTypes, so that this code is more "readable" at the callsite (eg. we would see something like olmMessageTypes.TEXT or something)

This revision now requires changes to proceed.May 12 2023, 9:30 AM
  1. Refactor code
  2. Introduce olm encrypted message types and use it durong session creation

(feel free to add me back if there's something specific I can be helpful w/, don't have a ton of context here)

This revision is now accepted and ready to land.May 16 2023, 9:52 AM