Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify ENS name client side #1520

Closed
rachelhamlin opened this issue Jul 9, 2019 · 25 comments
Closed

Verify ENS name client side #1520

rachelhamlin opened this issue Jul 9, 2019 · 25 comments

Comments

@rachelhamlin
Copy link

rachelhamlin commented Jul 9, 2019

Moving from react #8541

User Story

As a user, I want to make sure ENS name displayed in chat and profile has been verified.

Description

Type: Feature

ENS name is sent as part of chat messages and account details. It is displayed as is and could be spoofed.

Proper solution involves checking the ENS validity client side. For performance reason it's been agreed that it should be done status-go side.

@rachelhamlin rachelhamlin changed the title Verify ENS names Verify ENS name client side Jul 9, 2019
@rachelhamlin
Copy link
Author

Is the required change clear @pombeirp?

If it's straightforward enough, perhaps we could get it done in the next week. If so, we can include all of @jeluard's ENS work in the upcoming release (native registration + default display in chat).

@pedropombeiro
Copy link
Contributor

@rachelhamlin I'd just add the details @jeluard mentioned, i.e. that the data that is returned from status-go will omit the ENS name if it couldn't be verified.

@jeluard
Copy link

jeluard commented Jul 10, 2019

@pombeirp Some more details: the message payload contains a :content map, itself optionally containing a :name string. This name is the full ENS name (e.g. julien.stateofus.eth, pombeirp.eth).

At the status-react level we need to know if this name is really controlled by the message sender. So the payload should be modified in such a way that the :name string can be trusted. The simplest would be to just drop it if it has been spoofed.

Checking for name validity is a simple ENS contract call (I can provide guidance if needed).

@rachelhamlin
Copy link
Author

@flexsurfer you mentioned that we need this for the code freeze. Still true based on today's discussion?

@pombeirp could we assign someone to this?

@pedropombeiro
Copy link
Contributor

I think I should be able to take it.

@pedropombeiro pedropombeiro self-assigned this Jul 11, 2019
@flexsurfer
Copy link
Member

i'm not sure about code freeze, but it must be in v1 for sure if we want ens names in chats, but it's blocked by chat api, we can implement only after chat api will be done on go side

@rachelhamlin
Copy link
Author

Noted. Should we consider this a blocker to releasing ENS resolution @flexsurfer @jeluard?

@jeluard
Copy link

jeluard commented Jul 12, 2019

It's definitely a security concern. Not entirely clear if it should prevent ENS being released in the pre-v1 release.
@corpetty WDYT?

Also note that the check could be implemented status-react side, if needed.

@rachelhamlin
Copy link
Author

Note: we can get a contributor to hide the ENS chat resolution feature for beta release v0.14, so this does not need to be done before that. Will come back later on the level of priority for v1.

@pedropombeiro
Copy link
Contributor

@cammellos checked and the name info is only passed in contact request, not regular messages, so we only need to do it for contacts which have added us.

@flexsurfer
Copy link
Member

@flexsurfer
Copy link
Member

btw interesting if we send it only in plain text, we should send with every message

@cammellos
Copy link
Contributor

I checked against messages sent in public chat by @pombeirp and @yenda (I don't have an account myself), and neither of those are sending that field, maybe they are running older versions?

@yenda
Copy link
Contributor

yenda commented Sep 25, 2019

yeah it's bad to send it here, I think they didn't check inside the content key though, in contact request I believe it is separated, which makes more sense.

Anyway here is what is called in clojure to resolve:

(defn resolver
  [registry ens-name cb]
  (json-rpc/eth-call
   {:contract registry
    :method "resolver(bytes32)"
    :params [(namehash ens-name)]
    :outputs ["address"]
    :on-success
    (fn [[address]]
      (cb (when-not (= address default-address) address)))}))

(defn pubkey
  [resolver ens-name cb]
  (json-rpc/eth-call
   {:contract resolver
    :method "pubkey(bytes32)"
    :params [(namehash ens-name)]
    :outputs ["bytes32" "bytes32"]
    :on-success
    (fn [[x y]]
      (let [public-key (uncompressed-public-key x y)]
        (cb public-key)))}))

resolver is used to get the address of the resolver from the ens registry of the chain (so you need the different addresses if you don't have them yet: (maybe we should pass them from react?)

(def ens-registries
  {:mainnet "0x314159265dd8dbb310642f98f50c066173c1259b"
   :testnet "0x112234455c3a32fd11230c42e7bccd4a84e02010"
   :rinkeby "0xe7410170f87102DF0055eB195163A03B7F2Bff4A"})

then on the callback you use the resolver address in pubkey call to get the pubkey associated to the name.

@yenda
Copy link
Contributor

yenda commented Sep 25, 2019

@cammellos yes I'm running desktop from July it might not include it?

@cammellos
Copy link
Contributor

@yenda no idea, status-im/status-mobile@e1bcaeb added the functionality

@yenda
Copy link
Contributor

yenda commented Sep 25, 2019

Sorry I just don't have a ens-name on desktop that's why

@pedropombeiro
Copy link
Contributor

Yeah, I also used an old (working) desktop client, that's probably why

@rachelhamlin
Copy link
Author

Hey @cammellos @pombeirp! What's the status of this issue? Is it still being worked on?

@cammellos
Copy link
Contributor

the code is ready to go, and already merged in status react. Currently the functionality is blocked by geth 1.9 upgrade, one fix is in #9259 , afyert that is merged and the fleet is set to staging, we can update status go and enable it

@rachelhamlin
Copy link
Author

Okay, awesome. I'm mentally filing it as 'basically done.' Thanks @cammellos!

@cammellos
Copy link
Contributor

no probs, maybe is worth getting an handle on this geth upgrade, seems like no one is owning it and a bit in a flux, it also prevents us to deploy new changes to status go.
Happy to do it, although today i am offline most of the day will have to wait until tomorrow

@rachelhamlin
Copy link
Author

Oh, yes please. Let's do that @cammellos. Can you assign yourself on an issue for it? I don't see anything about it in status-go.

@flexsurfer
Copy link
Member

@cammellos can we close this?

@cammellos
Copy link
Contributor

@flexsurfer yes, I'll close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants