This differential implements prekey lifecycle on the keyserver that is compliant with what was established during crypto sync on 20th April 2023.
Details
Details
- Reviewers
ashoat • jon - Commits
- rCOMMcda420d08c5d: Implement prekey lifecycle on the keyserver
Create fresh olm accounts in the database (by changin db version for example and running migrations). Then examine the contents of pickled accounts strings. Change cron to run every minute or every second. Continue examining pickled accounts strings in the database. Make sure they
change once and only once. It means that prekey absence is correctly detected on first cron run, but sbusequents runs do not change anything since it hasn't been enough time since the last run.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/utils/olm-utils.js | ||
|---|---|---|
| 55 ↗ | (On Diff #25548) | This might be confusing so I will explain it. Unfortunately olm library doesn't provide method to check whether prekey was published and an attempt to call unpublished_prekey results in an error in JS if prekey has been already published. We have to rely on last_prekey_publish_time which returns number of seconds that has passed since UTC zero (January 1970) if the prekey has been already published and 0 if it has not been published yet. |
| keyserver/src/utils/olm-utils.js | ||
|---|---|---|
| 55 ↗ | (On Diff #25548) |
@anunay identified the same issue and is working on solving it. Our plan is that it should be possible to call unpublished_prekey and check if the result is falsey. |
| 58 ↗ | (On Diff #25548) | Don't we need to mark the prekey as published somewhere? Where do we do that? |
| 72 ↗ | (On Diff #25548) | What happens if the prekey is published at 23:50, and then this function is called the next day at 0:00. Will this condition evaluate to true? We want to make sure that 24 hours have been passed since when the prekey was published before forgetting the old one. |
| keyserver/src/utils/olm-utils.js | ||
|---|---|---|
| 58 ↗ | (On Diff #25548) | It was stated during crypto sync that marking as published takes place when prekey is sent to the identity service. It is out of the scope of this project to make a decision where, when and how prekey will be sent to the keyserver. |
Comment Actions
It was stated during crypto sync that marking as published takes place when prekey is sent to the identity service. It is out of the scope of this project to make a decision where, when and how prekey will be sent to the keyserver.
That makes sense in theory, but you appear to use the "marked as published" property throughout this diff, despite never setting it.
Won't last_prekey_publish_time always be 0 if you never mark a prekey as published?
| keyserver/src/utils/olm-utils.js | ||
|---|---|---|
| 72 ↗ | (On Diff #25594) | There's no reason here to have this hour-based granularity. Why don't you simplify the condition? const maxOldPrekeyAge = 24 * 60 * 60 * 1000; [...] if ( prekeyPublished && currentDate - lastPrekeyPublishDate >= maxOldPrekeyAge ) { account.forget_old_prekey(); } |
Comment Actions
That makes sense in theory, but you appear to use the "marked as published" property throughout this diff, despite never setting it.
"marked as published" property is not used in this differential.
Won't last_prekey_publish_time always be 0 if you never mark a prekey as published?
Yes it will. Marking prekey as published is expected to take place somewhere else in the code where it is sent to the keyserver. This function is only interested in checking whether it has already been done and taking necessary action.
Apart from simplification of 24-hour condition, which I am going to introduce, I don't have a clear picture of what is wrong with this differential at this point.
Comment Actions
Marking prekey as published is expected to take place somewhere else in the code where it is sent to the keyserver.
I am not sure what you mean. How is the prekey going to be sent to the keyserver? Isn't this the prekey FOR the keyserver, which is generated on the keyserver on line 67 above?
What is your plan for rotating the prekey?
| keyserver/src/utils/olm-utils.js | ||
|---|---|---|
| 63 ↗ | (On Diff #25609) | Please avoid checks like this. Take the feedback I have previously given you and synthesize, then apply it everywhere |
Comment Actions
We still have a problem here which is that we won't be doing any prekey rotation, since it appears that the rotation only happens if the prekey is marked as published. You can either mark the prekey as published here, and @jon will "undo" that once he is actually publishing the prekeys. Or we can change the conditions for the rotation to not look at whether the prekey has been published. Also open to other ideas, but prekey rotation is a must-have here
Also worth syncing with @jon to get a timeline on his work (maybe he will get it done in time, so it's okay to block shipping your work on his?)
EDIT
Ah it appears that the prekey is marked as published in D7586. I'll take a look at this diff again after reviewing that one
Comment Actions
In my personal opinion there are too many reviewers here. If any of you want to be included in the review, feel free to re-add yourself... just figured it would be good to clear out your queue
Comment Actions
I implemented temporary solution to prekey rotation in D7586. It is done only to the notifications prekey, however notifications prekey is the only prekey that is currently used and exchanged with clients, so by not marking primary prekey as published we are not running into security issues. So we can either leave it as it is or exclude primary prekey validation from the cron until primary prekey leaves keyserver ecosystem for the first time. I would prefer the first option.
Comment Actions
Thanks for explaining! We can go with the first option.
- Please delete the unnecessary callback declaration before landing. If it appears that you need it, please re-request review
- If anything changes substantially with the implementation of fetchCallUpdateOlmAccount, it might be a good idea to re-request review
| keyserver/src/cron/cron.js | ||
|---|---|---|
| 101–103 | Why is this callback necessary? Can't we just pass validateAccountPrekey to fetchCallUpdateOlmAccount? | |
| keyserver/src/utils/olm-utils.js | ||
| 60 | Can we move this this declaration outside of the function, so it doesn't need to be recalculated / redeclared on every invocation? | |
| 71 | Can we move this this declaration outside of the function, so it doesn't need to be recalculated / redeclared on every invocation? | |
Comment Actions
- Prekey is always there. Remove hasPrekey condition.
- Add try{}catch(){} to bypass issue with unpublished_prekey