Page MenuHomePhabricator

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

Authored by atul on Dec 25 2022, 1:23 AM.
Tags
None
Referenced Files
F3558622: D6033.diff
Fri, Dec 27, 4:35 AM
Unknown Object (File)
Wed, Dec 18, 6:17 PM
Unknown Object (File)
Wed, Dec 18, 6:17 PM
Unknown Object (File)
Wed, Dec 18, 6:17 PM
Unknown Object (File)
Wed, Dec 18, 6:17 PM
Unknown Object (File)
Wed, Dec 18, 10:59 AM
Unknown Object (File)
Thu, Dec 5, 4:48 AM
Unknown Object (File)
Sat, Nov 30, 6:52 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
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Dec 25 2022, 1:24 AM
atul added inline comments.
lib/types/siwe-types.js
110

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

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

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

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

keyserver/src/responders/user-responders.js
336 ↗(On Diff #20146)

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.