This differential implements relevant olm utils and creator to create olm session for particular cookie id with initial encrypted message provided by the client.
Details
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
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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 |
keyserver/src/creators/olm-session-creator.js | ||
---|---|---|
22 ↗ | (On Diff #25819) |
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 |
keyserver/src/creators/olm-session-creator.js | ||
---|---|---|
22 ↗ | (On Diff #25819) | We need to answer some questions here:
cc @anunay |
keyserver/src/creators/olm-session-creator.js | ||
---|---|---|
22 ↗ | (On Diff #25819) |
|
keyserver/src/creators/olm-session-creator.js | ||
---|---|---|
22 ↗ | (On Diff #25819) |
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) |
My whole point is that fetchCallUpdateOlmAccount does not need to be called. If you take my solution, this will not be a problem.
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. |
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 |
Decrypt initial message from the client during session instantiation on the kesyerver
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) |
- Refactor code
- 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)