Page MenuHomePhabricator

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

Authored by atul on Mar 12 2023, 5:59 PM.
Referenced Files
Unknown Object (File)
Fri, Jun 14, 2:51 AM
Unknown Object (File)
Wed, Jun 12, 8:17 AM
Unknown Object (File)
Thu, May 30, 11:38 AM
Unknown Object (File)
Thu, May 30, 11:38 AM
Unknown Object (File)
Thu, May 30, 11:38 AM
Unknown Object (File)
Thu, May 30, 11:38 AM
Unknown Object (File)
Thu, May 30, 11:36 AM
Unknown Object (File)
Sat, May 25, 2:08 PM



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
  1. Shut down keyserver.
  2. 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)
  3. Set breakpoint in handleInitialClientSocketMessage and restart keyserver.
  4. Observe that isCookieMissingSignedIdentityKeysBlob returns true and SIGNED_IDENTITY_KEYS_BLOB request is added to serverRequests.
  5. Set breakpoint in native/web nativeGetClientResponsesSelector/webGetClientResponsesSelector and ensure that signedIdentityKeysBlob is being generated and added to clientResponses as expected
  6. Set breakpoint in processClientResponses and ensure that we're getting expected response.

After next diff:

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

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

atul published this revision for review.Mar 12 2023, 5:59 PM
546–555 ↗(On Diff #23656)
  1. 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.
  1. 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.
  1. I'm awaiting the call to isCookieMissingSignedIdentityKeysBlob separately from other async calls in handleInitialClientSocketMessage. I didn't think it made sense to group with the following:

5eb0cb.png (402×1 px, 62 KB)

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.alls 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.

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

546 ↗(On Diff #23656)

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 ↗(On Diff #23656)

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

This revision is now accepted and ready to land.Mar 13 2023, 12:18 PM
546 ↗(On Diff #23656)

Okay, will remove. Thought it was necessary check because:

b0cf0a-1.png (344×702 px, 53 KB)

548 ↗(On Diff #23656)

Will move

remove viewer.cookieID check

This revision was landed with ongoing or failed builds.Mar 14 2023, 12:51 PM
This revision was automatically updated to reflect the committed changes.