Page MenuHomePhabricator

[keyserver] validate prekeys only via cron job
ClosedPublic

Authored by kamil on Oct 9 2023, 7:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 4, 5:01 PM
Unknown Object (File)
Sat, May 4, 5:01 PM
Unknown Object (File)
Sat, May 4, 5:01 PM
Unknown Object (File)
Sat, May 4, 4:58 PM
Unknown Object (File)
Sat, May 4, 4:27 PM
Unknown Object (File)
Wed, May 1, 10:41 PM
Unknown Object (File)
Wed, May 1, 10:37 PM
Unknown Object (File)
Tue, Apr 16, 7:33 AM
Subscribers

Details

Summary

Issue: ENG-3944.

This is part of D8476 (breaking this for smaller diffs because it's complex).

Keys should be validated only via cron job, not on each access to olm account

Depends on D9422

Test Plan

N/A (but tested in previous diff that after actual publishing keys are marked as published).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
This revision is now accepted and ready to land.Oct 10 2023, 3:34 AM
ashoat requested changes to this revision.Oct 10 2023, 7:54 AM

I agree we do not need to call revalidateAccountPrekeys anymore from getOlmSessionInitializationDataResponder. However, I think we still need to do it in registerOrLogin, so that we make sure the prekey that we pass when calling the identity service is initialized (in case it is the first call) and not expired (in case the keyserver has not been touched in some time).

keyserver/src/utils/olm-utils.js
273 ↗(On Diff #31801)

Can we rename this to validateAccountPrekeys now? It seems like it can get called for a brand new OlmAccount, in which case the "re" prefix seems misleading

This revision now requires changes to proceed.Oct 10 2023, 7:54 AM

We discussed this yesterday on 1:1 but today when updating I get confused a bit, so let me respond.

One assumption:
After looking at the Identity code, both login and register cause saving prekeys for a given device, and there is no need to additionally call RefreshUserPrekeys. (@varun could you confirm?).

However, I think we still need to do it in registerOrLogin, so that we make sure the prekey that we pass when calling the identity service is initialized (in case it is the first call)

Maybe I am missing something, but I think prekey is initialized from the beginning. Looking at types prekey() always returns string which means prekey should be defined.
I also verified this by calling createPickledOlmAccount.
So perhaps there is no result in validating on a brand new account because keys are not published yet anyway, but in this case new prekey will be updated to identity while login/register.

Just wondering if this understanding is correct.

and not expired (in case the keyserver has not been touched in some time).

I can imagine a case when we log in to identity, and have very old keys, so validating here makes sense. So I will keep the old validateAccountPrekey which only generates/forget keys and call it here (retrieveAccountKeysSet in login.js). There is no need to upload keys via RefreshUserPrekeys in those keys because it will be done by login/register.
I would also rename the function revalidateAccountPrekeys to validateAndUploadAccountPrekeys or something similar (introduced in D9421, used only for validating via cron job).

Is that okay or I am missing something?

keyserver/src/utils/olm-utils.js
273 ↗(On Diff #31801)

As I wrote in the comment above, how about something like validateAndUploadAccountPrekeys?

Thanks @kamil, your explanation makes sense. Let's keep validateAccountPrekey for log-in/register (but skip the upload step), and rename revalidateAccountPrekey to validateAndUploadAccountPrekeys

This revision is now accepted and ready to land.Oct 11 2023, 6:28 AM