Authenticate recurrent users with memo. #257

gianfra-t · 2024-11-06T20:14:59Z

Closes: #237.

Context and Preliminaries.

This PR adds a way to authenticate users by means of a signature using their wallet (commonly called: Sign in with web3, SIWE). The signer-service will use this signature in a JWT fashion to authorize the signing of the sep-10 operations from the signer-service.

The motivation for these changes is to add a scheme where the application is able to authenticate the users by it's initial Polygon/EVM address and avoid re-do of KYC and storing extra credentials.

Detailed overview

To identify the users based on their authenticated address, the sep-10 operations now correspond to the specification of a memo that is derived uniquely and in a deterministic way only from the EVM address.
The user identifier is then the combination of a master account concatenated with the memo (master:memo) and must be seen by the Anchor as a completely different user than one authenticated with only the master account. (relevant section of the protocol)

Only with a valid signature corresponding to the EVM address in question will the signer-service respond with the corresponding signatures, which verifies: memo, client-domain and master account.

Security guarantees

Without these signatures from the service, the sep-10 will fail when requested to the Anchor. Although it is not stated on the protocol (or I missed it) it is expected that the Anchor would ONLY allow a memo specification when the client-domain is defined, and valid. Apparently, anchors could also whitelist the master account for this purposes.

Flow of operations

The user opens Vortex.
(optional) Upon connect/change of address it is presented with a modal asking to sign a message.
The user attempts an offramp. If needed by Anchor, we search for the user's signature in the storage, check it's expiration time, and potentially ask the user to sign the message again if it doesn't exist or if it's expired. (we don't ask for validation to the service here, to save requests).
The user now sends a request to the signer-service with this signature, and the corresponding sep-10 parameters. The service returns the corresponding stellar signatures.
If the user's signature turns out to be invalid (eg.: the service has lost it's state), then a new sign-in operation will be possible without interrupting the flow.

Session

So far, the concept of session is simply kept by means of the signature stored in the UI (local storage...). Again, drawing parallels to a JWT. The message signed has a date parameter checked by the signer-service.

Alternatively, if the user signs from another browser/machine etc, then the old signature will be invalidated on the signer-service.

Requirements for log-out have not been defined yet.

Security risks, and potential improvements.

This is as far as I would go with this simple/prototypical implementation of using the signature as means of authentication.
There is a potential risk since this will be stored in the localStorage, unlike secure cookies. Therefore, if we are going to aim for a comprehensive session management, I believe we should implement something like iron-session to keep the session, while the initiation would still be by means of this signature verification. This seems to be the common standard practice.

Extra libraries concerns.

This pr adds the library siwe which is useful for creating and parsing the message, and potentially verifying. This is useful yet not critical, and the issue is that it requires ethers dependency. So to be more compatible with our current libraries and have the freedom to remove this one, we use viem to verify on the backend side.

netlify · 2024-11-06T20:15:14Z

✅ Deploy Preview for pendulum-pay ready!

Name	Link
🔨 Latest commit	`e12b1fe`
🔍 Latest deploy log	https://app.netlify.com/sites/pendulum-pay/deploys/6737dd7803cc1400082dd662
😎 Deploy Preview	https://deploy-preview-257--pendulum-pay.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

github-actions · 2024-11-08T22:24:05Z

`yarn.lock` changes

Click to toggle table visibility

Name	Previous	Current
`@spruceid/siwe-parser`	-	2.1.2
`@types/node`	18.16.3	22.7.5
`aes-js`	-	4.0.0-beta.5
`apg-js`	-	4.4.0
`ethers`	-	6.13.4
`siwe`	-	2.3.2
`undici-types`	-	6.19.8
`valid-url`	-	1.0.9

gianfra-t · 2024-11-12T19:38:57Z

signer-service/src/api/controllers/stellar.controller.js

-    return res.json({ clientSignature, clientPublic });
+    return res.json({ masterClientSignature, masterClientPublic, clientSignature, clientPublic });
+  } catch (error) {
+    if (error.message.includes('Could not verify signature')) {


We may want to use internal error codes later.

gianfra-t · 2024-11-12T19:48:46Z

signer-service/src/api/services/sep10.service.js

@@ -88,8 +123,15 @@ exports.signSep10Challenge = async (challengeXDR, outToken, clientPublicKey) =>
    clientDomainSignature = transactionSigned.getKeypairSignature(clientDomainStellarKeypair);
  }

+  let masterClientSignature;
+  if (memo !== null && memoEnabled) {


We've verified all operations so reaching here should be safe to append the signature.

gianfra-t · 2024-11-12T19:57:39Z

src/hooks/useSignChallenge.ts

@@ -0,0 +1,123 @@
+import { useEffect, useState, useCallback, useRef } from 'react';


Main hook to get, refresh signature and show the sign modal.

gianfra-t · 2024-11-12T19:59:14Z

src/hooks/useSignChallenge.ts

+      return null;
+    }
+  }, [address, storageKey]);
+


We want to return a promise that will resolve when the signing finishes (or fails). This is so we can use checkAndWaitForSignature and it's force counterpart on the sep-10 process function.

gianfra-t · 2024-11-12T20:00:42Z

src/hooks/useSignChallenge.ts

+      const message = new SiweMessage(siweMessage);
+      const signature = await signMessageAsync({ message: siweMessage });
+
+      const validationResponse = await fetch(`${SIGNING_SERVICE_URL}/v1/siwe/validate`, {


Mostly so that an old session is removed. Otherwise we may not need to validate, and we can save the roundtrip.

gianfra-t · 2024-11-12T20:01:59Z

src/hooks/useSignChallenge.ts

+    return signMessage();
+  }, [storageKey, signMessage]);
+
+  // ask for signature on address change and on init


I found this a bit invasive and annoying. But could be enabled. Perhaps with a timer.

I think we should enable it so that users are immediately requested to log in after connecting the wallet. Makes sense to combine these two events so that the user doesn't need to sign thrice later.

I'll enable it so we can test it. I worry a bit that is invasive in the sense that if the user only wants to see quotes or offramp EURC, it's not needed and may scare some users.

On the other hand it's even more spaced from the other 2 signatures so "feels" better.

gianfra-t · 2024-11-12T20:06:18Z

src/services/anchor/index.ts

-async function getUrlParams(ephemeralAccount: string, supportsClientDomain: boolean): Promise<URLSearchParams> {
-  if (supportsClientDomain) {
-    return new URLSearchParams({ account: ephemeralAccount, client_domain: config.applicationClientDomain });
+//A memo derivation. TODO: Secure? how to check for collisions?


Hash the address, and get the base 10 representation of the hash. Apparently the memo needs to be numeric.

gianfra-t · 2024-11-12T20:09:20Z

src/services/anchor/index.ts

 }

+const sep10SignaturesWithLoginRefresh = async (


Helper to retry once if the service returns a signature error, with a force refresh in the middle.

This is for the case the signer service has invalidated the signature, for some reason (it restarted or the user logged in elsewhere and then came back to use the old session)

gianfra-t · 2024-11-12T20:10:12Z

src/services/anchor/index.ts

-    transactionSigned.toXDR(),
-    outputToken,
-    accountId,
+  let signatureData = await checkAndWaitForSignature();


We will fetch the signature, if it's invalid or doesn't exist, will ask the user to sign and only then continue.

gianfra-t · 2024-11-12T20:12:13Z

src/services/anchor/index.ts

    sep24Params = new URLSearchParams({
      asset_code: sessionParams.tokenConfig.stellarAsset.code.stringStellar,
      amount: sessionParams.offrampAmount,
-      account: sep10Account, // THIS is a particularity of Anclap. Should be able to work without it, or with a different one
-      // to that of the sep-10
+      account: sep10Account, // THIS is a particularity of Anclap. Should be able to work just with the epmhemeral account


We are still waiting for Anclap to solve on their side, or tell us if we are sending something wrong.
The commented memo parameter is deprecated... but it could be the case that will be required.

gianfra-t · 2024-11-12T20:16:47Z

@pendulum-chain/devs You can try this one locally, including Anclap until the offramp UI (or use the "testing" credentials to interact)
The signer-service must be ran locally.

ebma

Interesting approach with SIWE. I almost feel like it's too sophisticated for our use case and we can live with something simpler but since you already made it work, I'm happy to keep it as it's better security-wise.

ebma · 2024-11-13T16:35:13Z

signer-service/src/api/services/sep10.service.js

+// we validate a challenge for a given nonce. From it we obtain the address and derive the memo
+// we can then ensure that the memo is the same as the one we expect from the anchor challenge
+const getAndValidateMemo = async (nonce, userChallengeSignature) => {
+  if (!userChallengeSignature || !nonce) {


I think it makes more sense to put this check out of this function and wrap getAndValidateMemo with the check before calling it. Or give the function a different name. But based on the name I would expect to always get a memo for the nonce and userChallengeSignature, and if I don't that's an error.

Well, with the same signature you will get the same memo, but it's true that the nonce may be confusing given the name.
What if we name it validateSignatureAndGetMemo? If you prefer separated, I guess we can validate first and then derive the memo directly on signSep10Challenge.

I'm okay with naming it validateSignatureAndGetMemo.

ebma · 2024-11-13T16:40:44Z

signer-service/src/api/services/siwe.service.js

+    version: '1',
+    chainId: polygon.id,
+    nonce,
+    expirationTime: new Date(Date.now() + 360 * 60 * 1000).toISOString(),


What time are you trying to describe here? A year? If so, it's missing 24. Might also be a good idea to put this into an extra constant or possibly make it configurable with env variables.

Nothing specific, as we still need to define the log out and expiration behavior. I'll add it to constants though.

ebma · 2024-11-13T16:48:50Z

signer-service/src/api/services/siwe.service.js

+  return maybeSiweData.siweMessage;
+};
+
+// TODO we need some sort of session log-out.


So that the map doesn't grow indefinitely? Or why would we need a log out? The log out happens 'automatically' when the token expires and the user tries to use it again. So I don't see why we need an explicit logout.

It will grow indefinitely for new addresses. This simple way of keeping valid sessions is very rudimentary, that's why I would use a session library if we want more functionality.

Regarding if we need it or not, technically no, but I believe it's just a standard. For instance polkassembly has a log out "feature". Again we need to define this.

Yeah maybe we should use sessions. Or at least persist the nonce map so that when the server restarts (due to redeployment or crash) users don't need to always sign in again which would be the case if we only keep an in-memory list.

I think that adding more complexity to this implementation, for instance the persistence after a crash, comes very close to a full session implementation which I would prefer to do with a library already.

I'm even dubious about the logout. Maybe we can skip it for now wdyt? And in the next iteration we use a proper session library.

ebma · 2024-11-13T16:55:44Z

src/hooks/useSignChallenge.ts

+
+  const storageKey = `${storageKeys.SIWE_SIGNATURE_KEY_PREFIX}${address}`;
+
+  const checkStoredSignature = useCallback((): SiweSignatureData | null => {


This is missing a check for different addresses. At the moment, it does not check whether the address passed into useSiweSignature is the same as the one in the localStorage. Thus, we would run into issues when switching between accounts.

The function will refresh to a change in storageKey and address as the hook useSiweSignature itself depends on address.
I just keep address also in the array to filter for undefined values.

ebma · 2024-11-13T16:56:58Z

src/hooks/useSignChallenge.ts

+
+  // Used to wait for the modal interaction and/or return of the
+  // signing promise.
+  const signPromiseRef = useRef<{


Why are we not using useState for this?

This one was interesting. At first I tried that, but since checkAndWaitForSignature and the force counterpart were being used in the sep-10 function which is not a hook, I was experiencing issues and the state was not reflecting the change.

I later changed the implementation (before I was polling for the signature with a setTimeout) so it might be that there is no issue storing the promise in a simple state. I would have to test again.

Update: Okay so with this implementation it works with useState. With previous the issue was that it was checking the state to resolve the promise outside of the hook, but the promise now is being resolved using the values inside the useSiweSignature hook, that must be it.

ebma · 2024-11-13T17:02:31Z

src/hooks/useSignChallenge.ts

+    return signMessage();
+  }, [storageKey, signMessage]);
+
+  // ask for signature on address change and on init


I think we should enable it so that users are immediately requested to log in after connecting the wallet. Makes sense to combine these two events so that the user doesn't need to sign thrice later.

ebma · 2024-11-13T17:03:44Z

src/constants/localStorage.ts

@@ -10,6 +10,7 @@ export const storageKeys = {
  ANCHOR_SESSION_PARAMS: 'ANCHOR_SESSION_PARAMS',
  STELLAR_OPERATIONS: 'STELLAR_OPERATIONS',
  TOKEN_BRIDGED_AMOUNT: 'TOKEN_BRIDGED_AMOUNT',
+  SIWE_SIGNATURE_KEY_PREFIX: 'siwe-signature-',


Is the capitalization format required like this? If it's not, I'd say we make it similar to the others and capitalize to SIWE_SIGNATURE_

ebma · 2024-11-13T17:05:48Z

src/hooks/useMainProcess.ts

 type ExtendedExecutionInput = ExecutionInput & { stellarEphemeralSecret: string };

-export const useMainProcess = () => {
+export const useMainProcess = ({ checkAndWaitForSignature, forceRefreshAndWaitForSignature }: UseMainProcessProps) => {


Since useMainProcess is a hook itself, you can call other hooks in it. Thus, you don't need to pass these two functions as props but you can just call useSignChallenge inside the body of useMainProcess and access the respective functions.

The thing is that we need requiresSign, handleCancel, etc for the modal component, which is on the same level as useMainProcess.

And those 2 functions as a parameter in the sep-10 which is inside useMainProcess. Alternatively we can set a context but seemed like more trouble, or return the modal states from useMainProcess.

I already faced the issue of using useSignChallenge on both levels and doesn't work of course.

Then we might want to move parts of the useSignChallenge hook into a context so that it's shared and every call to useSignChallenge returns the shared values. A similar example would be the useBridgeSettings hook in the portal that uses a shared context for storing the settings a user chose between issue and redeem requests.

Moved to a context, I followed the events one which I found quite clean. This is what you had in mind?

Is this more or less what you had in mind? I don't think we can move only partially the hook into a context.

ebma · 2024-11-13T17:12:23Z

src/services/anchor/index.ts

+  } catch (error: any) {
+    if (error.message === 'Invalid signature') {
+      let { nonce, signature } = await refreshFunction();
+      let regreshedArgs = { ...args, maybeChallengeSignature: signature, maybeNonce: nonce };


Suggested change

let regreshedArgs = { ...args, maybeChallengeSignature: signature, maybeNonce: nonce };

let refreshedArgs = { ...args, maybeChallengeSignature: signature, maybeNonce: nonce };

TorstenStueber · 2024-11-14T09:50:10Z

@gianfra-t as I don't have ARS, I tried to offramp EUR. When I click on the Confirm button, I am presented the sign in popup.

When I click on "Sign Message", nothing happens, the popup just disappears and the form is shown again. If I click on the Confirm button again, the same happens – indefinitely.

Is it even intended that the popup is shown in the first place? The identity verification via signature is not required for Mykobo.

gianfra-t · 2024-11-14T11:36:41Z

If I click on the Confirm button again, the same happens – indefinitely.

This behavior is not intended definitely. What the console shows? We need some more error handling from the UI side, perhaps a toast, I left that open on purpose since there was also no agreement on that.

Nevertheless, for me there is no issue when selecting EURC and it works the same as before. So I imagine there must be a variable missing when you run signer-service.

Also we need to add a condition to NOT ask for signature with EURC or any token that doesn't support memo on config (this, I forgot), although I will not add it now until I understand what happens in your case.

TorstenStueber · 2024-11-14T11:48:04Z

Yes, sorry, should have provided that:

I guess the signer service of the preview deployment does not have the new changes yet and I would need to run locally instead.

gianfra-t · 2024-11-14T12:17:30Z

Yes you need to run it locally. Sadly we don't deploy a service preview so far.

gianfra-t · 2024-11-14T18:48:28Z

src/pages/swap/index.tsx

@@ -177,6 +182,7 @@ export const SwapPage = () => {
          outputTokenType: to as OutputTokenType,
          amountInUnits: fromAmountString,
          offrampAmount: tokenOutAmountData.roundedDownQuotedAmountOut,
+          setInitializeFailed,


I realized we had no action for when handleOnSubmit failed. Probably this doesn't happen to often since sep-10 is quite reliable, but now we add the message sign in the middle which could fail from the s. service.

With this, we will just show the Application initialization fail... message in this case. Same as on the pre-flight check. Let me know what you think.

TorstenStueber · 2024-11-15T09:52:13Z

I didn't review the entire code yet, but when I tested locally I found the following.

Why create the siwe message in the backend? I would not expect that we need this new endpoint and we can simply do it in the frontend as in this example.
Metamask shows the following warning when I want to sign the signing request. Is this only happening because I run on localhost and it would disappear once deployed in the cloud (I tried to change the parameters for siwe.SiweMessage but could not make the warning disappear)? It would be good if this works for any domain, particularly also for the netlify preview domains.

I need to sign in even when I just select EUR, can we avoid signing in if we will not use the memo for the respective anchor (i.e., if usesMemo: false). Let's avoid any conversion rate hurdles.
When I select EUR, then click on "Confirm" so that the button shows "Continue with Partner", then select ARS, then the button still shows "Continue with Partner" and I will start the Mykobo anchor flow. We should reset when switching the output currency.

gianfra-t · 2024-11-15T13:14:26Z

Nice observations @TorstenStueber:

I would not expect that we need this new endpoint

It is true that the message can be created either in the backend, or in the frontend. But even when created on the fronted, we need an endpoint that sends a random nonce or a way to derive the nonce such that we are able to verify it on the service, which seems more complex. The nonce is to prevent replay attacks, more info here.

Now it is true that the message could be generated on the frontend and then validate the values on the backend. See this example from wagmi. Still, the nonce is fetched. So I thought it was better to abstract the message creation to the backend already. I don't see much benefits/cons on either way.

Metamask shows the following warning

Yes, I thought of using vortex main domain, but I will try to find a way to remove the warning for the staging and previews.

I need to sign in even when I just select EUR

We were discussing it here, if we add a signature when the app loads/address changes or just when clicking confirm and the asset requires it. So far it's enabled to test how it "feels".

When I select EUR, then click on "Confirm"

I didn't think about this case! Will add a reset.

TorstenStueber · 2024-11-15T13:49:37Z

As long as the user is not malicious, they will protected from replay attacks if we create the nonce in the frontend. For malicious users – as you point out – they can just create the message anyway and not use the backend.

If there are equal solutions, I would always pick the simpler one – in this case this means to handle this completely in the frontend. Also: this would make it easier to just use the current domain for the message so that Metamask does not show the warning.

We were discussing it #257 (comment),

This is a user facing decision and we should escalate to @pendulum-chain/product. I will move the discussion elsewhere.

gianfra-t · 2024-11-15T14:21:37Z

The issue with replay attacks, as I understand, is to protect from actors picking up the signature of a user and re-using it indefinitely to authenticate as them.

Without nonce checking on the backend, getting a signature would essentially like getting the password of the "wallet" only that this password cannot be changed. I think that especially in this case that we store the signature on the local storage we should implement something like this.

gianfra-t · 2024-11-15T15:45:50Z

src/pages/swap/index.tsx

@@ -310,7 +316,10 @@ export const SwapPage = () => {
      <TermsAndConditions />
      <PoolSelectorModal
        open={!!modalType}
-        onSelect={modalType === 'from' ? onFromChange : onToChange}
+        onSelect={(token) => {


I don't think we were doing this, but we should cancel the offramp both on input and output token selection, if we are in the Continue with partner.. stage.

That, or disable the selection.

gianfra-t added 10 commits November 1, 2024 14:44

merged with WIP sign-client-domain

9778b62

merge with add-ars-offramp-support

72839fb

WIP

3c3ac6a

merged with current staging branch

32a3d5a

add message creation endpoint for testing

db71c33

add simple solution for message signin

d8f2b2c

merged with ars integration, client domain

7d91840

work in progress memo solution

d0202f2

remove comments

780b17b

remove and improve comments

1996e64

gianfra-t linked an issue Nov 6, 2024 that may be closed by this pull request

Change the SEP24 flow to authenticate recurring users #237

Open

1 task

remove master account, either use memo or not

b4eaddf

gianfra-t mentioned this pull request Nov 8, 2024

Add client domain corresponding signature when needed. #236

Merged

improve signature hook

79beb77

gianfra-t added 8 commits November 11, 2024 10:14

merge current client_domain implementation

9fae39e

use viem to validate message on backend, error handling

ec8e798

improve sign in hooks

2b14bbe

invalidate older sessions after log in

c3bcbd1

remove poll from sign in hooks

bf5d521

merged:staging

9e92784

better memo derivation, cleaning

a6be1ed

add validators

9f671b5

gianfra-t commented Nov 12, 2024

View reviewed changes

more cleanup, comments

9f064b9

gianfra-t commented Nov 12, 2024

View reviewed changes

yet more cleanup

4db9ef8

gianfra-t requested a review from a team November 12, 2024 20:13

ebma reviewed Nov 13, 2024

View reviewed changes

lint errors, remove useRef

cd844f1

TorstenStueber mentioned this pull request Nov 14, 2024

Add Asset Hub Flow #283

Open

initialization failed state on handleOnSumbit fail

2b18ed0

gianfra-t commented Nov 14, 2024

View reviewed changes

gianfra-t added 2 commits November 15, 2024 11:47

move siwe to context

22f2339

cancel sep24 first on token selection change

c6d74f2

gianfra-t commented Nov 15, 2024

View reviewed changes

wip store signature as auth cookie

e12b1fe

		@@ -0,0 +1,123 @@
		import { useEffect, useState, useCallback, useRef } from 'react';


		const storageKey = `${storageKeys.SIWE_SIGNATURE_KEY_PREFIX}${address}`;

		const checkStoredSignature = useCallback((): SiweSignatureData \| null => {

	let regreshedArgs = { ...args, maybeChallengeSignature: signature, maybeNonce: nonce };
	let refreshedArgs = { ...args, maybeChallengeSignature: signature, maybeNonce: nonce };

Authenticate recurrent users with memo. #257

Are you sure you want to change the base?

Authenticate recurrent users with memo. #257

Conversation

gianfra-t commented Nov 6, 2024 • edited Loading

Context and Preliminaries.

Detailed overview

Security guarantees

Flow of operations

Session

Security risks, and potential improvements.

Extra libraries concerns.

netlify bot commented Nov 6, 2024 • edited Loading

✅ Deploy Preview for pendulum-pay ready!

github-actions bot commented Nov 8, 2024 • edited Loading

yarn.lock changes

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

gianfra-t Nov 12, 2024 • edited Loading

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

gianfra-t commented Nov 12, 2024

ebma 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

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

TorstenStueber commented Nov 14, 2024

gianfra-t commented Nov 14, 2024 • edited Loading

TorstenStueber commented Nov 14, 2024

gianfra-t commented Nov 14, 2024

Choose a reason for hiding this comment

TorstenStueber commented Nov 15, 2024 • edited Loading

gianfra-t commented Nov 15, 2024

TorstenStueber commented Nov 15, 2024

gianfra-t commented Nov 15, 2024

Choose a reason for hiding this comment

gianfra-t commented Nov 6, 2024 •

edited

Loading

netlify bot commented Nov 6, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

`yarn.lock` changes

gianfra-t Nov 12, 2024 •

edited

Loading

gianfra-t commented Nov 14, 2024 •

edited

Loading

TorstenStueber commented Nov 15, 2024 •

edited

Loading