[keyserver] Send `SIGNED_IDENTITY_KEYS_BLOB` request to client if data missing in DB
ClosedPublic
Actions

Authored by atul on Mar 12 2023, 5:59 PM.

Details

Reviewers

ashoat

Commits

rCOMM157e772a80ac: [keyserver] Send `SIGNED_IDENTITY_KEYS_BLOB` request to client if data missing…

Summary

In handleInitialClientSocketMessage we send SIGNED_IDENTITY_KEYS_BLOB request to native or web client if the signed_identity_keys column of the cookies table is missing for an entry with id=cookieID.

These requests will be handled on the client by nativeGetClientResponsesSelector and webGetClientResponsesSelector which will generate functions that return a Promise for an array of ClientClientResponses.

The responses will then be handled by the keyserver with the session-utils:processClientResponses(...) function... which after the next diff will include SignedIdentityKeysBlob from client response in the serverDB cookies table.

Depends on D7050

Test Plan

Shut down keyserver.
Delete contents of signed_identity_keys column for all rows of cookies table. (Deleting contents for all rows wasn't necessary, but just did it anyways)
Set breakpoint in handleInitialClientSocketMessage and restart keyserver.
Observe that isCookieMissingSignedIdentityKeysBlob returns true and SIGNED_IDENTITY_KEYS_BLOB request is added to serverRequests.
Set breakpoint in native/web nativeGetClientResponsesSelector/webGetClientResponsesSelector and ensure that signedIdentityKeysBlob is being generated and added to clientResponses as expected
Set breakpoint in processClientResponses and ensure that we're getting expected response.

After next diff:

Ensure that signedIdentityKeysBlob is valid using same checks in loginResponder and siweAuthResponder.
Ensure that setCookieSignedIdentityKeysBlob query executes correctly and signed_identity_keys column for row with id=cookieID is correctly populated.
Force socket to disconnect/reconnect and ensure that SIGNED_IDENTITY_KEYS_BLOB request is NOT sent from the keyserver.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Mar 12 2023, 5:59 PM

atul published this revision for review.Mar 12 2023, 5:59 PM

Harbormaster completed remote builds in B17268: Diff 23656.Mar 12 2023, 6:14 PM

atul added inline comments.Mar 12 2023, 6:40 PM

keyserver/src/socket/socket.js
546–555	I thought that it made the most sense to handle this check and `serverRequest` "once" in `handleInitialClientSocketMessage`. This is different than the check and request for `MORE_ONE_TIME_KEYS` which is handled within `processClientResponses` and gets "hit" every time the `keyserver` receives client responses. I concluded it was unnecessary to query the `cookies` table to check for `signed_identity_keys` every single time `clientResponses` are received and thought `handleInitialClientSocketMessage` would be a more appropriate place. I concluded that we didn't need any client `codeVersion` checks here because `[native/web]GetClientResponsesSelector` will just ignore `serverRequests` that they're unable to handle (aka no branch to handle a given `serverRequestType`). It doesn't look like the `keyserver` holds onto any state regarding what requests have been sent to clients, so I concluded it was fine if the `SIGNED_IDENTITY_KEYS_BLOB` request was simply ignored by old clients. I'm `await`ing the call to `isCookieMissingSignedIdentityKeysBlob` separately from other `async` calls in `handleInitialClientSocketMessage`. I didn't think it made sense to group with the following: because if, for example, there was a `clientResponse` to a `SIGNED_IDENTITY_KEYS_BLOB` request: there would be a race condition between A. setting the `signed_identity_keys` column B. checking if the `signed_identity_keys` column is `NULL` As a result I decided to sequence the `isCookieMissingSignedIdentityKeysBlob` check AFTER the `processClientResponses` check to ensure that the result from `isCookieMissingSignedIdentityKeysBlob` is current and "up-to-date." It's not the end of the world if `isCookieMissingSignedIdentityKeysBlob` gives us a stale `NULL` since we'll just request it from the client again, but figured this approach was more correct. I also didn't think it made sense to handle within the `Promise.all`s within the `!sessionInitializationResult.sessionContinued` block or the corresponding `else` block because we'd need to write the check out twice. There are definitely ways to pull `promises={}` outside of the conditional branches and "pull the common stuff out," but I thought it was simpler to just sequence `isCookieMissingSignedIdentityKeysBlob` after those blocks and concluded any overhead was minimal. IMPORTANT: This is my first time working with this part of the codebase so I'm less confident in these changes than I might be with other diffs. I tried to read through and understand things as best as I could, but please let me know if any of the assumptions listed above (or otherwise) need to be revisited or reconsidered.

atul attached a referenced file: F424535: 5eb0cb.png. (Show Details)Mar 12 2023, 6:40 PM

atul removed a reviewer: tomek.Mar 13 2023, 7:35 AM

ashoat edited the test plan for this revision. (Show Details)Mar 13 2023, 12:09 PM

Please remove the conditional before landing (it doesn't do anything)

keyserver/src/socket/socket.js
546	This check is not necessary. `viewer.cookieID` always returns a string, so you're just checking if it's the empty string (which isn't possible)
548	One easy way to improve Promise performance without bothering with `Promise.all` and the complexity it forces is to simply start the promise as soon as you can. Eg. `const signedIdentityKeysBlobMissingPromise = isCookieMissingSignedIdentityKeysBlob(viewer.cookieID);` can be moved to eg. line 469