Page MenuHomePhabricator

[keyserver] Introduce `checkAndInvalidateSIWENonceEntry` and consume in `siweAuthResponder`
ClosedPublic

Authored by atul on Dec 24 2022, 10:30 PM.
Tags
None
Referenced Files
F2194812: D6032.id20107.diff
Fri, Jul 5, 8:02 AM
Unknown Object (File)
Fri, Jun 28, 1:52 AM
Unknown Object (File)
Thu, Jun 27, 3:57 AM
Unknown Object (File)
Tue, Jun 25, 9:03 PM
Unknown Object (File)
Tue, Jun 25, 7:26 PM
Unknown Object (File)
Mon, Jun 24, 8:39 PM
Unknown Object (File)
Sat, Jun 22, 1:04 PM
Unknown Object (File)
Sat, Jun 22, 8:42 AM
Subscribers
None

Details

Summary

We use this function to determine if for a given nonce there's an entry in siwe_nonces AND it hasn't expired (creation_time > Date.now() - nonceLifetime). If such a row exists, we delete it so it can't be used again. We determine whether a row existed by looking at QueryResults.affectedRows.

By effectively checking and deleting in the same query we can ensure that even if there is a flood of requests with the same nonce, one and only one will pass this check.

NOTE: I'm working under the assumption that a nonce should only be valid for one request, NOT one successful request. I considered scenarios where a request may be unsuccessful initially due to whatever circumstance (maybe a network outage), but that would succeed if replayed shortly after (maybe after the network recovers), and concluded it's safest to stick to a nonce only being valid for a single request. Please correct me if this understanding is incorrect.

Depends on D6031

Test Plan
  1. Observed siwe_nonces table entry being created on initial call to siwe_nonce endpoint.
  2. Set breakpoints in siweAuthResponder and checkAndInvalidateSIWENonceEntry.
  3. Sent request to siwe_auth endpoint from native and observed the row for the corresponding nonce being validated and deleted.

Also tested some error cases (hard coding an expired nonce from the siwe_nonces table, hard coding a nonce that doesn't exist in the siwe_nonces table, etc).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul requested review of this revision.Dec 24 2022, 10:49 PM
ashoat added inline comments.
keyserver/src/deleters/siwe-nonce-deleters.js
23 ↗(On Diff #20109)

We usually don't indent second lines of SQL queries like this

This revision is now accepted and ready to land.Dec 25 2022, 7:26 PM
keyserver/src/deleters/siwe-nonce-deleters.js
23 ↗(On Diff #20109)

Ah okay, not sure why I did that. I'll outdent the WHERE clause in this query and the one above (deleteStaleSIWENonceEntries)

rebase before addressing feedback

rebase after addressing feedback and before landing

This revision was landed with ongoing or failed builds.Dec 27 2022, 12:58 AM
This revision was automatically updated to reflect the committed changes.