[keyserver][lib] don't generate one time keys on identity login
ClosedPublic
Actions

Authored by varun on Mar 19 2024, 6:20 PM.

Details

Reviewers

bartek
marcin
kamil
ashoat

Commits

rCOMMd2f01b572f83: [keyserver][lib] don't generate one time keys on identity login

Summary

added a new helper function that only retrieves the identity keys and prekeys
updated login.js to use the new helper function, only getting one-time keys if login fails (to use for identity registration)

Test Plan

logged in from keyserver and confirmed that no new one time keys were added to the one time keys table in ddb

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Mar 19 2024, 6:20 PM

Herald added a subscriber: tomek. · View Herald TranscriptMar 19 2024, 6:20 PM

varun added a child revision: D11361: [lib][web] deviceKeyUpload -> newDeviceKeyUpload.Mar 19 2024, 6:22 PM

varun added inline comments.Mar 19 2024, 6:34 PM

keyserver/src/user/login.js
80–86 ↗	(On Diff #38213)	i don't think this needed to be async since `retrieveAccountKeysSet` / `retrieveIdentityKeysAndPrekeys` are synchronous functions

varun edited reviewers, added: marcin, kamil; removed: ashoat. varun removed 1 blocking reviewer(s): bartek.Mar 19 2024, 6:35 PM

varun added a reviewer: ashoat.

Harbormaster completed remote builds in B27617: Diff 38213.Mar 19 2024, 6:36 PM

varun requested review of this revision.Mar 19 2024, 6:36 PM

marcin added inline comments.Mar 20 2024, 3:58 AM

keyserver/src/user/login.js
138 ↗	(On Diff #38213)	What if the keyserver is registered but `login` fails due to network/database issues? Is it also desirable to attempt to register in such a case? Theoretically attempting to register already registered keyserver will simply result in registration failure. But looking at the code here we will unnecessarily generate new one time keys. I can't see any potential issues with that since those one time keys will not be marked as published (and not persisted in identity service DB?) so they are not wasted - they might be uploaded to the identity in future when it queries the keyserver for one time keys. Nevertheless I might be missing something so I am just leaving this observation as a potential food for thought.
160 ↗	(On Diff #38213)	Is this a desired behaviour to mark one time keys as published after actually publishing them to identity service? On native we adopted opposite behaviour: one time keys are marked as published before they are actually published to the identity service. See here: https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp#L163 and here: https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp#L635. If I remember correctly the goal here was to avoid potential issues in case we successfully upload one time keys to the identity service so that it starts serving them to clients but we fail to mark them as published on the client so the client might send the same one time key multiple times.

ashoat accepted this revision.Mar 20 2024, 11:19 AM

ashoat added inline comments.

keyserver/src/user/login.js
80–86 ↗	(On Diff #38213)	Good catch
138 ↗	(On Diff #38213)	I think your observation is correct. Is it also desirable to attempt to register in such a case? If the login fails due to network/database issues, I don't think registering is desirable. In the network case we would want to retry, and in the database case it would depend on whether retry was possible. However, differentiating the reason for the login failure might be difficult, and in either case it's outside the scope of this diff.
141–146 ↗	(On Diff #38213)	There's a shorthand we could use here
160 ↗	(On Diff #38213)	I think we're intending to change this in April. See ENG-6691