Page MenuHomePhabricator

[keyserver] Validate `SIWEMessage` `signature` in `siweAuthResponder`
ClosedPublic

Authored by atul on Dec 25 2022, 1:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 4:18 PM
Unknown Object (File)
Fri, Nov 8, 4:17 PM
Unknown Object (File)
Fri, Nov 8, 4:17 PM
Unknown Object (File)
Fri, Nov 8, 4:17 PM
Unknown Object (File)
Fri, Nov 8, 3:48 PM
Unknown Object (File)
Fri, Nov 8, 3:48 PM
Unknown Object (File)
Tue, Nov 5, 4:22 AM
Unknown Object (File)
Tue, Nov 5, 4:22 AM
Subscribers
None

Details

Summary

Attempt to validate the signature for the SIWEMessage and handle potential errors (unrelated but noticed` siwe` 2.x has much more descriptive error types: https://blob.sh/00a56f.png).


Depends on D6032

Test Plan

Haven't tested just yet but will hardcode some values to try to hit the error cases and make sure things are handled properly.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Dec 25 2022, 1:24 AM
atul added inline comments.
lib/types/siwe-types.js
110 ↗(On Diff #20110)

Based off of the following TypeScript:

de79d6.png (212×772 px, 39 KB)

Will type the parts of siwe we use properly via flow-typed later in the stack.

ashoat added inline comments.
keyserver/src/responders/user-responders.js
345 ↗(On Diff #20110)

We usually do something more like expired_message for ServerError. It's meant to basically be an error code

This revision is now accepted and ready to land.Dec 25 2022, 7:27 PM
keyserver/src/responders/user-responders.js
345 ↗(On Diff #20110)

Gotcha, was using the values of the ErrorTypes enum as the error messages but will use the "keys" instead (eg expired_message, malformed_session, etc)

rebase before addressing feedback

rebase after addressing feedback and before landing

This revision was landed with ongoing or failed builds.Dec 27 2022, 1:08 AM
This revision was automatically updated to reflect the committed changes.
keyserver/src/responders/user-responders.js
336

These { status: 400 } objects don't do anything, do they?

keyserver/src/responders/user-responders.js
336

Not that I know of, I saw that Derek used that pattern so I just went with it... Can remove if they're superfluous which it seems like they are.

Based on the comment:

58058d.png (674×1 px, 129 KB)

it looks like payload field was initially included for updateEntry and deleteEntry.

And looks like we also use it EG for policy acknowledgement stuff.

431385.png (232×930 px, 39 KB)

Doesn't seem like payload is the right place to put the HTTP code nor is it used anywhere. I'll put up a diff removing them.