Page MenuHomePhabricator

[keyserver] add public key/social proof to the cookie record
AbandonedPublic

Authored by ashoat on Nov 15 2022, 1:19 PM.
Tags
None
Referenced Files
F3369177: D5634.diff
Mon, Nov 25, 10:09 PM
F3365578: D5634.diff
Mon, Nov 25, 7:22 AM
Unknown Object (File)
Fri, Nov 22, 4:28 PM
Unknown Object (File)
Fri, Nov 22, 4:24 PM
Unknown Object (File)
Fri, Nov 22, 4:19 PM
Unknown Object (File)
Fri, Nov 22, 4:13 PM
Unknown Object (File)
Fri, Nov 22, 4:01 PM
Unknown Object (File)
Sun, Nov 17, 10:38 PM

Details

Summary

keep the devices' crypto key stored as part of the cookie. linear task depends on D5603

Test Plan

create an account/login & have social proof embedded in the cookie record

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D5634
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 15 2022, 1:22 PM
Harbormaster failed remote builds in B13446: Diff 18444!
Harbormaster failed remote builds in B13447: Diff 18445!
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 16 2022, 9:08 AM
Harbormaster failed remote builds in B13482: Diff 18485!

added social proof & public key to login cookie creation

ashoat requested changes to this revision.Nov 16 2022, 8:07 PM

Please annotate the diff with the correct parent / child relationships with other diffs. I can't see your stack

keyserver/src/creators/account-creator.js
74 ↗(On Diff #18496)

I think the plaintext of the signed message should contain the public key, so shouldn't be a need to transmit it separately... we just need to parse it out

106 ↗(On Diff #18496)

Where does this end up getting used again? Is it currently not used? (Fine if not, just need a reminder)

Unrelated – wanted to find the relevant diff, but couldn't because you didn't list your stack, so leaving here – we should make sure a user is only associated with a single ETH address, a single ETH address is only associated with a single user

native/account/siwe-panel.react.js
57 ↗(On Diff #18496)

Why are we sending the same thing up twice?

native/schema/CommCoreModuleSchema.js
21–24 ↗(On Diff #18496)

Thanks for typing this. I think you'll need to rerun codegen now

This revision now requires changes to proceed.Nov 16 2022, 8:07 PM
derek added inline comments.
keyserver/src/creators/account-creator.js
74 ↗(On Diff #18496)

we removed the public key from users/sessions table, i was under the impression every new account was registering with the public key from their device

106 ↗(On Diff #18496)

public key isn't getting used yet:

We need this for SIWE because the "social proof" should have this key, and it's easier for us to make sure it's there from the start then to go back and have to rethink things later

^link

added the diff to the stack (i wrote depends on [link] instead of depends on [diff#]) - regarding single eth address, i'm confident that's covered in D5603, but there's definitely more work to be done in that category (making sure normal usernames don't overlap, ens resolution etc.)

native/account/siwe-panel.react.js
57 ↗(On Diff #18496)

it's really a placeholder pending more discussion on what the social proof actually consists of. i removed it and marked it as optional in the flow types. i'll add a comment to the linear task linking back to this diff

native/schema/CommCoreModuleSchema.js
21–24 ↗(On Diff #18496)

reran codegen but no changes came up - i think it was just the lack of typing on js side

derek marked 2 inline comments as done.

removed duplicate argument

reverted changes applied to wrong diff

ashoat requested changes to this revision.Nov 18 2022, 12:48 PM

I think there's some confusion about what the socialProof needs to contain. See inline comments

keyserver/src/creators/account-creator.js
106–107

we removed the public key from users/sessions table, i was under the impression every new account was registering with the public key from their device

Not sure why you're bringing up the user vs. device distinction. Repeating my comment here:

I think the plaintext of the signed message should contain the public key, so shouldn't be a need to transmit it separately... we just need to parse it out

The socialProof is a signed message that contains the publicKey inside it, right? So instead of sending the publicKey twice, can we just parse it out of the socialProof?

We can't just take the user's "word for it" that the publicKey matches the socialProof... we need to parse the socialProof to make sure that it includes the publicKey. At that point, we'll have the publicKey... so there's no reason to send it up separately.

Let me know over chat if that's unclear!

keyserver/src/session/cookies.js
645–648

Can we make all of these $ReadOnly by prefixing with +?

lib/types/account-types.js
161–165

Why'd you remove the +? Let's make it $ReadOnly again

native/account/siwe-panel.react.js
46

I think we want the signature to contain the publicKey, so this is too late in the process for us to doing this

The point of the social proof is to prove that Ethereum account X corresponds to Comm account Y. To do that, we sign a message containing Y's public key using X's private key. Does that make sense?

This revision now requires changes to proceed.Nov 18 2022, 12:48 PM

added public key to message prior to signing

need flow fix me help

native/account/siwe-panel.react.js
50 ↗(On Diff #18621)

i'm having trouble with flow here. WebViewProps are typescript declarations & don't seem to play nicely with flow

removed unnecessary function call

still planning changes tho

native/account/siwe-panel.react.js
23 ↗(On Diff #18621)

Why'd you remove this space

41 ↗(On Diff #18621)

Prefix with + to make $ReadOnly

50 ↗(On Diff #18621)

Yes you cannot use TypeScript declarations in Flow...

spacing, readonly

native/account/siwe-panel.react.js
50 ↗(On Diff #18621)

Cannot instantiate React.Ref because WebViewProps [1] is incompatible with AbstractComponent [2] in type argument ElementType

can we leave the FlowFixMe(s) here?
if not, how do I typecast WebViewProps as a component?

native/account/siwe-panel.react.js
50 ↗(On Diff #18621)

You have to type WebViewProps in Flow.

if not, how do I typecast WebViewProps as a component?

You shouldn't be "typecasting" it, you should just make sure you're using the types correctly.

Have you tried Googling eg. "how to type a React component in Flow"? The first result for that query (for me, anyways) is Flow's React type reference. You could start by reading React.Ref first (since that's what you're using here), and next try reading React.ComponentType.

What's obviously wrong here from a cursory read through those docs is that you're passing a type representing the props of a component (WebViewProps) to a type (React.Ref) that expects its parameter to be a React component (React.ComponentType).

ashoat requested changes to this revision.Nov 21 2022, 7:31 AM

can we leave the FlowFixMe(s) here?

Definitely not

This revision now requires changes to proceed.Nov 21 2022, 7:31 AM
ashoat requested changes to this revision.Nov 22 2022, 5:14 PM

Two comments from earlier review were not addressed

keyserver/src/creators/account-creator.js
106–107

See here

native/account/siwe-panel.react.js
46

And here

This revision now requires changes to proceed.Nov 22 2022, 5:14 PM

You're most of the way there, though. Point is basically "keyserver needs to verify that the provided public key matches what is contained within the provided message, and that isn't being done currently. To do that, keyserver needs to extract the public key... but at that point, we don't need to ask the client to pass it in separately.'

Does that make sense? keyserver just needs the ETH-signed message. Then it extracts the Comm public key out of the message, verifies that request.address signed it (not currently being done it seems?), and then goes ahead and associated that cookie with that socialProof

keyserver/src/creators/account-creator.js
74 ↗(On Diff #18739)

We need to verify that request.address actually signed the socialProof

landing/siwe.react.js
46 ↗(On Diff #18739)

I checked the whitepaper draft for what to use. Here's what I found:

Comm wants you to sign in with your Ethereum account:
0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2
I accept the Comm Terms of Service: https://comm.app/tos
URI: https://comm.app/login
Version: 1
Chain ID: 1
Nonce: 32891756
Issued At: 2022-07-24T16:25:24Z

There's a lot of questions here:

  1. I think it's missing the public key. I'm rather confident that's an accidental omission, but currently checking with Anunay. You're including that here which is good!
  2. We need a nonce passed in from the keyserver I think, but I seem to remember there's a task for that?
  3. I can't tell what part of this is SIWE boilerplate (presumably that this SiweMessage class will handle?), and it's confusing to see the developer-defined message split by the Ethereum address.
  4. We should probably update the whitepaper to have the real "URI" produced by our messages in production instead of "https://comm.app/login"
  5. "https://comm.app/tos" in the whitepaper should be replaced with "https://comm.app/terms"

We can probably handle all of this stuff in a later diff

atul edited reviewers, added: derek; removed: atul.

I assume we're abandoning this one as well given @atul's comment. We can always come back to it later if we change our mind

ashoat edited reviewers, added: atul; removed: ashoat.
This revision now requires review to proceed.Dec 25 2022, 8:19 AM