[keyserver] Add new hashing to keyserver state check
ClosedPublic
Actions

Authored by michal on Sep 22 2023, 6:01 AM.

Details

Reviewers

tomek
inka
kamil

Commits

rCOMM7b1601b0741d: [keyserver] Add new hashing to keyserver state check

Summary

Part of ENG-4959
Depends on D9265
Depends on D9266

Modify the hashing on the keyserver (for new clients) so that:

the hashes are generated from data in client-side-schema, so clients don't have to do the conversion
the stores aren't hashed by stringifying the whole store but instead we generate a hash for each info and merge them using xor (there's a code comment explaining why that is correct)

Test Plan

Remove the version checks and wait for state check to trigger (with an old client):

check that there are no errors
check that:
- currentUserInfo hash still matches because it's not a collection and it doesn't have anything to convert
- specific userInfos match because they don't contain anything to convert
- all other contain different hashes than on the client
inside of the getClientResponsesSelector additionally select integrityStore and use it's threadHashes for client side hashes -> check that now specific rawThreadInfos hashes match

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Sep 22 2023, 6:01 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 22 2023, 6:01 AM

Harbormaster completed remote builds in B22800: Diff 31366.Sep 22 2023, 6:20 AM

michal requested review of this revision.Sep 22 2023, 6:20 AM

michal planned changes to this revision.Sep 28 2023, 5:32 AM

michal edited the summary of this revision. (Show Details)Sep 29 2023, 3:33 AM

michal edited the test plan for this revision. (Show Details)

Rebase

Harbormaster completed remote builds in B22910: Diff 31543.Sep 29 2023, 3:52 AM

michal added a child revision: D9327: [lib] Add hash functions to state sync spec.Sep 29 2023, 5:20 AM

michal added inline comments.Sep 29 2023, 5:24 AM

keyserver/src/socket/session-utils.js
392–394 ↗	(On Diff #31543)	I tried creating a separate generic function to force flow to consider only one specific case for each iteration ("monomorphize" the generic) flow playground, but couldn't find a solution.

michal mentioned this in D9328: [lib] Use new hashing functions in state check.Sep 29 2023, 5:35 AM

tomek requested changes to this revision.Oct 3 2023, 3:14 AM

tomek added inline comments.

keyserver/src/shared/state-sync/current-user-state-sync-spec.js
30 ↗	(On Diff #31543)	Why do we use `null`? Is it a good idea?
keyserver/src/socket/session-utils.js
396–407 ↗	(On Diff #31543)	What are the errors when you remove both `FlowFixMe`s?

This revision now requires changes to proceed.Oct 3 2023, 3:14 AM

michal added inline comments.Oct 4 2023, 4:40 AM

keyserver/src/shared/state-sync/current-user-state-sync-spec.js

30 ↗

(On Diff #31543)

It means that we skip the version check for conversion. We will be calling this function only for newer clients only (the version check is added in session-utils.js) so this should always succeed

keyserver/src/socket/session-utils.js

396–407 ↗

(On Diff #31543)

Without the first FlowFixMe:

Cannot call await with spec.fetch(...) bound to p because: [incompatible-call]
 • Either cannot call spec.getServerInfosHash with data bound to infos because Promise [1] is not a subtype of
   UserInfos [2]. Class instances are not subtypes of object types; consider rewriting UserInfos [2] as an interface.

     src/socket/session-utils.js
     393│         // doesn't understand that `data` is of the generic Info type of the
     394│         // spec we are currently iterating on
     395│         (async () => {
     396│           const data = await spec.fetch(viewer);
     397│           if (
     398│             !hasMinCodeVersion(viewer.platformDetails, {
     399│               native: NEXT_CODE_VERSION,

     src/shared/state-sync/state-sync-spec.js
 [1]   9│   +fetch: (viewer: Viewer, ids?: $ReadOnlySet<string>) => Promise<Infos>,

     src/shared/state-sync/users-state-sync-spec.js
 [2]  14│   UserInfos,

Without the second one FlowFixMe there are a lot of errors for different cases because flow types data as (not sure why the Promises are there):

type t =
  | RawEntryInfos
  | RawThreadInfos
  | UserInfos
  | CurrentUserInfo
  | Promise<RawEntryInfos>
  | Promise<RawThreadInfos>
  | Promise<CurrentUserInfo>

and it fails because we are trying to pass a value that can be any of these types to a function getServerInfosHash that takes a specific type. We know that data has this type at runtime but flow doesn't.

michal requested review of this revision.Oct 5 2023, 6:05 AM

tomek accepted this revision.Oct 5 2023, 6:52 AM

tomek added inline comments.

keyserver/src/shared/state-sync/current-user-state-sync-spec.js
30 ↗	(On Diff #31543)	Will it work correctly if the validator changes in some future code version?
keyserver/src/socket/session-utils.js
396–407 ↗	(On Diff #31543)	Can we e.g. create a new function in a spec that calls both `fetch` and `getServerInfosHash` functions? Could this solve the issue?