feat: Eth wallet support #282

HarryET · 2021-11-19T15:58:12Z

What kind of change does this PR introduce?

Support for eth authentication

Adds new endpoints:

POST /nonce
GET /nonce/{id}
POST /eth

What is the current behavior?

No support for eth login

What is the new behavior?

Adds eth login support

Additional context

100s of reply from the community 😅
fixes Add Ethereum login #242
~~https://goethereumbook.org/signatures/~~
https://gist.github.com/dcb9/385631846097e1f59e3cba3b1d42f3ed#file-eth_sign_verify-go

* This is the first attempt and has flaws will most likely be reverted to try again!

Use crypto secure token function

Also adds them to the example.env file

Rahat-ch · 2021-11-19T16:14:43Z

Looking great so far!

A couple things to keep in mind:

Nonce should regenerate on every auth request
For the user in this case we should look at saving either the wallet address or ens address if it is available. That info will come from the client side implementation but I think is something we should keep in mind

Initial work on getting nonces stored in the database, also did some changes to the api route with a todo to implement saving to the database.

* Validating Signature, Nonce & Wallet Address * Generating Nonce * Starting to save nonce to database * new Audit log type * New deps * Update SQL * Update nonce struct * New migration for nonce

* User signup with Wallet Address * Get nonce by ID * Remove consumed_at

HarryET · 2021-11-20T19:46:51Z

I've now done as much as I can for the API without starting to make changes to https://github.com/Supabase/gotrue-js, I know @Rahat-ch said they wanted to try this. However, I think it could be done faster and with less friction If I was to start the PR and @Rahat-ch can take over and optimise it later on as its inevitable I'll make a few mistakes

HarryET · 2021-11-20T19:47:25Z

Looking great so far!

A couple things to keep in mind:

Nonce should regenerate on every auth request For the user in this case we should look at saving either the wallet address or ens address if it is available. That info will come from the client side implementation but I think is something we should keep in mind

That is how everything is working! It should be even more clear now

HarryET · 2021-11-20T22:31:15Z

The initial JS lib changes are being worked on supabase/auth-js#185, afaik @Rahat-ch was going to take over so here was the start of a testing file I made. Also that PR will needs tons of changed reach out if you need perms to my branch!

import { GoTrueClient } from ".";

const client = new GoTrueClient({
    url: "http://localhost:9999"
});

(async () => {
    const nonce = await client.getNonce();
    // TODO: Sign Nonce
    const {session, error} = await client.signIn({
        web3: {
            wallet_address: "",
            nonce: nonce.data!.nonce,
            signature: ""
        }
    });
    console.log(JSON.stringify(nonce))
})()

Create migration to add the new wallet_address on users table

HarryET · 2021-11-20T22:43:19Z

Unsure as to why tests are failing currently, will review this in more detail when it's time to think about moving the PR out of the draft stage

* Migrate Web3 -> Eth * Change nonce to support ETH standards * Misc Cleanup

* Fixes validation, sends not finished message when logging in as rest of auth needs to be fixed

HarryET · 2021-11-28T15:41:03Z

Ready for review now 🎉. There is a demo in this tweet: https://twitter.com/i/status/1464678182372655110

gwendall · 2021-11-28T17:40:24Z

Awesome job! Do you think we could also customize the message to sign there?

HarryET · 2021-11-28T18:23:34Z

Awesome job! Do you think we could also customize the message to sign there?

No, as the nonce conforms to standards and it would be hard to track if you allowed custom "nonces"

gwendall · 2021-11-28T18:26:56Z

Yup, the server has to know this message too. Would be awesome if it could be somehow integrated in the supabase UI somewhere - ie if we could define the message we want on the app with a placeholder for the nonce like "hey, please sign this message. Nonce: {nonce}".

HarryET · 2021-11-28T18:31:10Z

Yup, the server has to know this message too. Would be awesome if it could be somehow integrated in the supabase UI somewhere - ie if we could define the message we want on the app with a placeholder for the nonce like "hey, please sign this message. Nonce: {nonce}".

Can you elaborate on what you mean? The client dose receive the nonce btw!

Also this was made with help from the wallet connect team. Originally the nonce was just a random value but after recommendation from them it was moved to how it is now and it follows this: https://github.com/ethereum/EIPs/blob/75e2bd7f6afc6b6ed3f5a064e3ddf8c60a562e00/EIPS/eip-4361.md

You can see the reference implementation here: https://github.com/HarryET/walletconnect-supabase-example
The specific auth logic is here: https://github.com/HarryET/walletconnect-supabase-example/blob/master/src/App.tsx#L46-L86

HarryET · 2022-03-12T11:25:19Z

This can now be tested on the demo, the only thing missing in this PR are tests. Any other comments are appreciated in regards to parts of the PR that are done incorrectly or are missing.

Flagged as issue by SpruceID as not conformant to spec

Xanewok · 2022-04-26T16:43:19Z

Should this be marked as ready for review? Thanks for your work btw!

HarryET · 2022-04-26T16:58:29Z

Should this be marked as ready for review? Thanks for your work btw!

No @Xanewok, this will stay as draft till the Supabase team are happy with the security side. There open internal channels of communication to keep this PR moving. Refer too @kiwicopple's reason for closing this issue here for an explanation from the Supabase team: supabase/supabase#6474 (comment)

ghost · 2022-05-02T22:12:16Z

Really need this. What can we do to merge this? @kiwicopple

Even though the eip is still in draft, it is already widely used.

I need the magic.link wallet though this also means that I cannot use magic.link to authenticate with Supabase while having access to the wallet. This pr would allow me to use the magic.link wallet while using this wallet to sign the user into Supabase.

HarryET · 2022-05-03T10:17:38Z

Hello @byterose, I see you have also commented on the demo repo. This PR changes the core of GoTrue by adding a brand new method of auth that could have added vulnerabilities to GoTrue; as such this PR may take some time. Having people keep replying here with some version of "I need this" will not help. Also pinging the Supabase team will not help speed this up. As I have previously started, there are internal communication tracks between myself, Supabase and other parties. And updates will be posted here as it is applicable for people to know.

kiwicopple · 2022-06-06T09:34:01Z

Hey all, dropping a note here so that everyone knows where we're at. At this stage we are putting this on hold for the following reasons:

Prioritisation: we are prioritising SAML/SSO before web3. There are more users asking for this. We also plan to make this server multi-tenant, which is a big task and ideally happens before we introduce more complexity.
Backwards compatibility: this PR is a significant change to GoTrue, and we will need to do a full security audit to ensure that it doesn't introduce security concerns for any existing auth methods.
Forwards compatibility - there are a few things that came up from an initial investigation:

EIP-4361 is still in draft (specifically the EIP calls out "This EIP is not recommended for general use or implementation as it is likely to change.")
siwe-go (used in this PR) hasn't had a formal security review yet (see Disclaimer), and perhaps doesn't follow the EIP spec, which would make it hard to add other providers

We'll revisit this once we have completed 1!

…base-master

Fix audit entries

Migrate the implementation to support multiple crypto providers

hf

Hey @HarryET, here's my feedback (I'm back from holiday!). Let me know on Slack or here if you want to discuss more about it.

hf · 2022-12-21T15:35:57Z

api/crypto.go

+	"bytes"
+	"encoding/json"
+	"github.com/gofrs/uuid"
+	"github.com/netlify/gotrue/api/crypto_provider"


This style of naming packages is not common in Go code. Can you rename it to crypto-provider or something else?

hf · 2022-12-21T15:37:05Z

api/crypto.go

+	body, err := ioutil.ReadAll(r.Body)
+	jsonDecoder := json.NewDecoder(bytes.NewReader(body))
+	if err = jsonDecoder.Decode(params); err != nil {
+		return badRequestError("Could not read verification params: %v", err)
+	}


Please use the getBody function instead.

hf · 2022-12-21T15:37:24Z

api/crypto.go

+
+func (a *API) Crypto(w http.ResponseWriter, r *http.Request) error {
+	ctx := r.Context()
+	config := a.getConfig(ctx)


AFAIK this method is no longer there.

hf · 2022-12-21T15:41:48Z

api/crypto.go

+			user, uerr = a.signupNewUser(ctx, tx, &SignupParams{
+				CryptoAddress:  accountInfo.Address,
+				Provider:       "crypto",
+				CryptoProvider: params.Provider,
+				Aud:            aud,
+			})
+			didUserExist = false
+
+			identity, terr := a.createNewIdentity(tx, user, params.Provider, map[string]interface{}{"sub": accountInfo.Address, "address": accountInfo.Address})
+			if terr != nil {
+				return terr
+			}
+
+			user.Identities = []models.Identity{*identity}
+
+			if uerr = user.Confirm(tx); uerr != nil {
+				return uerr
+			}
+
+			if uerr = models.NewAuditLogEntry(r, tx, instanceID, user, models.UserSignedUpAction, "", nil); uerr != nil {
+				return uerr
+			}
+			if uerr = triggerEventHooks(ctx, tx, SignupEvent, user, instanceID, config); uerr != nil {
+				return uerr
+			}
+
+			return uerr


This account creation logic does not take many edge cases into account which we're trying to resolve systematically in GoTrue. Accounts may need to be linked -- assigned a linking domain and linking rules -- or be identified separately (which today is only possible with SSO accounts).

hf · 2022-12-21T15:42:51Z

api/crypto_provider/eth.go

+}
+
+func (e *EthProvider) GenerateNonce(_ *http.Request, instanceId uuid.UUID, options CryptoNonceOptions) (*models.Nonce, error) {
+	uri, err := url.Parse(options.Url)


Probably ParseRequestURI is required here?

hf · 2022-12-21T15:47:01Z

api/signup.go

+	case "crypto":
+		user, err = models.NewUser(instanceID, "", "", params.Password, params.Aud, params.Data)
+		cryptoAddress, err = models.NewCryptoAddress(instanceID, user.ID, params.CryptoProvider, params.CryptoAddress)


I would not put crypto "sign up" in the sign up method as it's getting complex. Can you pull this out in another API that can be easily turned on / off instead? Does that make sense?

Are you sure we want users with no email and phone address to be added here? Sounds like a dangerous thing if someone can just pile data in the database without any identifying information?

hf · 2022-12-21T15:47:39Z

migrations/20210909172000_create_identities_table.up.sql

-CREATE TABLE IF NOT EXISTS auth.identities (
-    id text NOT NULL,
-    user_id uuid NOT NULL,
-    identity_data JSONB NOT NULL,
-    provider text NOT NULL,
+CREATE TABLE IF NOT EXISTS auth.identities
+(
+    id              text        NOT NULL,
+    user_id         uuid        NOT NULL,
+    identity_data   JSONB       NOT NULL,
+    provider        text        NOT NULL,
    last_sign_in_at timestamptz NULL,
-    created_at timestamptz NULL,
-    updated_at timestamptz NULL,
+    created_at      timestamptz NULL,
+    updated_at      timestamptz NULL,
    CONSTRAINT identities_pkey PRIMARY KEY (provider, id),
-    CONSTRAINT identities_user_id_fkey FOREIGN KEY (user_id) REFERENCES auth.users(id) ON DELETE CASCADE
+    CONSTRAINT identities_user_id_fkey FOREIGN KEY (user_id) REFERENCES auth.users (id) ON DELETE CASCADE
 );
 COMMENT ON TABLE auth.identities is 'Auth: Stores identities associated to a user.';


Existing migration files must not have any changes.

hf · 2022-12-21T15:49:19Z

migrations/20220220193004_create_crypto_addresses.up.sql

+CREATE TABLE auth.crypto_addresses
+(
+    instance_id uuid NULL,
+    id          uuid         NOT NULL,
+
+    account_id  uuid         NOT NULL REFERENCES auth.users (id),
+    address     varchar(255) NOT NULL UNIQUE,
+    provider    varchar(255) NOT NULL,
+
+    created_at  timestamptz NULL DEFAULT now()
+);
+
+CREATE INDEX IF NOT EXISTS crypto_addresses_instance_id_idx ON auth.crypto_addresses USING btree (instance_id);
+comment on table auth.crypto_addresses is 'Auth: Stored Cryptocurrency addresses for web3 authentication.';


To me, these seem to belong better inside auth.identities as a (id, provider) pair. For example provider could be web3:<chain> and ID could be the 0x... address.

hf · 2022-12-21T15:49:52Z

models/crypto_address.go

+type CryptoAddress struct {
+	InstanceID uuid.UUID `json:"-" db:"instance_id"`
+	ID         uuid.UUID `json:"id" db:"id"`
+
+	AccountId uuid.UUID `json:"account_id" db:"account_id"`
+	Address   string    `json:"address" db:"address"`
+	Provider  string    `json:"provider" db:"provider"`
+
+	CreatedAt time.Time `json:"created_at" db:"created_at"`
+}


Ditto. This whole logic seems to be a specialization of the auth.identities table.

hf · 2022-12-21T15:50:27Z

models/nonce.go

+type Nonce struct {
+	InstanceID uuid.UUID `json:"-" db:"instance_id"`
+	ID         uuid.UUID `json:"id" db:"id"`
+	Provider   string    `json:"provider" db:"provider"`
+
+	Url      string `json:"url" db:"uri"`
+	Hostname string `json:"hostname" db:"hostname"`
+
+	ChainId   int    `json:"chain_id" db:"chain_id"`
+	Address   string `json:"address" db:"address"`
+	Namespace string `json:"namespace" db:"namespace"`
+	Nonce     string `json:"nonce" db:"nonce"`
+
+	CreatedAt time.Time `json:"created_at" db:"created_at"`
+	UpdatedAt time.Time `json:"updated_at" db:"updated_at"`
+	ExpiresAt time.Time `json:"expires_at" db:"expires_at"`
+}


Why are nonces stored in the database? Can't they be cryptographically signed and verified without storing anything in the database?

HarryET added 4 commits November 19, 2021 07:48

First Try at Web3

af59c7a

* This is the first attempt and has flaws will most likely be reverted to try again!

Update go.mod

3ed3025

Make /nonce more secure

e2a64b4

Use crypto secure token function

Fix new environment variables

c08fce6

Also adds them to the example.env file

HarryET mentioned this pull request Nov 19, 2021

Web3 Wallet Auth supabase/supabase#3936

Closed

HarryET added 7 commits November 19, 2021 17:49

Remove Unnecessary ENV

f8997b8

Generate of nonces

8fe0e9a

Initial work on getting nonces stored in the database, also did some changes to the api route with a todo to implement saving to the database.

Update init_postgres.sql

25a0e1c

Misc Changes

66246ba

* Validating Signature, Nonce & Wallet Address * Generating Nonce * Starting to save nonce to database * new Audit log type * New deps * Update SQL * Update nonce struct * New migration for nonce

Disable endpoints when disabled in config

30a0b1f

Fix nonce insertion

3685c8c

Misc Changes

9fe34cf

* User signup with Wallet Address * Get nonce by ID * Remove consumed_at

HarryET mentioned this pull request Nov 20, 2021

feat: Eth supabase/auth-js#185

Closed

Create new users migration

2a4efea

Create migration to add the new wallet_address on users table

HarryET added 2 commits November 23, 2021 19:12

Misc Changes

c1f88b7

* Migrate Web3 -> Eth * Change nonce to support ETH standards * Misc Cleanup

Fix Eth Validation

a7e68fa

* Fixes validation, sends not finished message when logging in as rest of auth needs to be fixed

HarryET changed the title ~~feat: web3/eth wallet support~~ feat: Eth wallet support Nov 27, 2021

HarryET added 2 commits November 27, 2021 17:27

Misc Changes

3c1f18a

Finish Signup

645a642

HarryET marked this pull request as ready for review November 27, 2021 19:29

Tidy go.mod

7967abc

Fix issue where chain id can be hex

1f3945c

Flagged as issue by SpruceID as not conformant to spec

kiwicopple mentioned this pull request Apr 19, 2022

MetaMask for authentication supabase/supabase#6474

Closed

supabase locked and limited conversation to collaborators May 3, 2022

HarryET and others added 18 commits June 29, 2022 16:48

Merge branch 'master' of https://github.com/supabase/gotrue into supa…

b625e60

…base-master

Merge branch 'supabase-master' into feat/web3

653da97

Fix incorrect table for comment

e423dcd

Fix API changes

b123739

Update signup.go

ec1c086

Confirm eth accounts automatically

17b6316

Merge branch 'supabase:master' into feat/web3

844bf6b

Update eth.go

9185e33

Fix audit entries

Support Multiple Providers

1cd8993

Migrate the implementation to support multiple crypto providers

Create identity on crypto account creation

a04462d

Fix queries

8d3180c

Update api.go

816a647

Remove TODO

0fa4b5d

Switch chainId to int

fbd0466

Bump siwe-go version

8c3115e

Merge branch 'supabase:master' into feat/web3

67a0c08

Fix identity id

32accf8

misc

977d8d9

hf reviewed Jan 16, 2023

View reviewed changes

kiwicopple closed this Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Eth wallet support #282

feat: Eth wallet support #282

HarryET commented Nov 19, 2021 •

edited

Loading

Rahat-ch commented Nov 19, 2021

HarryET commented Nov 20, 2021

HarryET commented Nov 20, 2021

HarryET commented Nov 20, 2021

HarryET commented Nov 20, 2021

HarryET commented Nov 28, 2021

gwendall commented Nov 28, 2021

HarryET commented Nov 28, 2021

gwendall commented Nov 28, 2021

HarryET commented Nov 28, 2021 •

edited

Loading

HarryET commented Mar 12, 2022

Xanewok commented Apr 26, 2022

HarryET commented Apr 26, 2022

ghost commented May 2, 2022 •

edited by ghost

Loading

HarryET commented May 3, 2022

kiwicopple commented Jun 6, 2022

hf left a comment

hf Dec 21, 2022

hf Dec 21, 2022

hf Dec 21, 2022

hf Dec 21, 2022

hf Dec 21, 2022

hf Dec 21, 2022

hf Dec 21, 2022

hf Dec 21, 2022

hf Dec 21, 2022

hf Dec 21, 2022

feat: Eth wallet support #282

feat: Eth wallet support #282

Conversation

HarryET commented Nov 19, 2021 • edited Loading

What kind of change does this PR introduce?

Adds new endpoints:

What is the current behavior?

What is the new behavior?

Additional context

Rahat-ch commented Nov 19, 2021

HarryET commented Nov 20, 2021

HarryET commented Nov 20, 2021

HarryET commented Nov 20, 2021

HarryET commented Nov 20, 2021

HarryET commented Nov 28, 2021

gwendall commented Nov 28, 2021

HarryET commented Nov 28, 2021

gwendall commented Nov 28, 2021

HarryET commented Nov 28, 2021 • edited Loading

HarryET commented Mar 12, 2022

Xanewok commented Apr 26, 2022

HarryET commented Apr 26, 2022

ghost commented May 2, 2022 • edited by ghost Loading

HarryET commented May 3, 2022

kiwicopple commented Jun 6, 2022

hf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarryET commented Nov 19, 2021 •

edited

Loading

HarryET commented Nov 28, 2021 •

edited

Loading

ghost commented May 2, 2022 •

edited by ghost

Loading