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.

Sign up for GitHub

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

fix: make SIWE default to EIP55 #193

Merged
merged 1 commit into from
Jul 15, 2024
Merged

fix: make SIWE default to EIP55 #193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/did-session/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@
"uint8arrays": "^5.0.1"
},
"devDependencies": {
"@ceramicnetwork/common": "^3.2.0",
"@ceramicnetwork/stream-model": "^2.2.0",
"@ceramicnetwork/stream-model-instance": "^2.2.0",
"@ceramicnetwork/stream-tile": "^3.2.0",
"@ceramicnetwork/streamid": "^3.2.0",
"@ceramicnetwork/common": "^5.15.0",
"@ceramicnetwork/stream-model": "^4.15.0",
"@ceramicnetwork/stream-model-instance": "^4.16.0",
"@ceramicnetwork/stream-tile": "^5.15.0",
"@ceramicnetwork/streamid": "^5.4.0",
"@didtools/cacao": "workspace:^",
"@didtools/pkh-ethereum": "workspace:^",
"@didtools/pkh-solana": "workspace:^",
Expand Down
8 changes: 7 additions & 1 deletion packages/did-session/test/lib.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* @jest-environment ceramic
*/
import type { CeramicApi } from '@ceramicnetwork/common'
import type { StreamWriter } from '@ceramicnetwork/common'
import { TileDocument } from '@ceramicnetwork/stream-tile'
import { EventEmitter } from 'events'
import { Wallet as EthereumWallet, Wallet } from '@ethersproject/wallet'
Expand Down Expand Up @@ -97,6 +97,10 @@ const testResources = [
'`ceramic://*?model=k2t6wyfsu4pgz0ftx664veuaf2qib95zj8je2x7pf89v6g5p7xa7n9eo45g64a',
]

interface CeramicApi extends StreamWriter {
did: DID
}

declare global {
const ceramic: CeramicApi
}
Expand All @@ -113,6 +117,7 @@ describe('did-session', () => {
resolver: getResolver(),
provider: new Ed25519Provider(seed),
})
await did.authenticate()
ceramic.did = did
const authResult = await createEthereumAuthMethod()
authMethod = authResult.authMethod
Expand Down Expand Up @@ -466,6 +471,7 @@ describe('did-session Solana Authmethod', () => {
resolver: getResolver(),
provider: new Ed25519Provider(seed),
})
await did.authenticate()
ceramic.did = did
authMethod = await createSolanaAuthMethod()
model = await Model.create(ceramic, MODEL_DEFINITION)
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-environment-ceramic/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"lint": "eslint index.js --fix"
},
"dependencies": {
"@ceramicnetwork/core": "^3.2.0",
"@ceramicnetwork/core": "^5.16.0",
"ipfs-core": "^0.18.1",
"jest-environment-node": "^29.7.0",
"tmp-promise": "^3.0.3"
Expand Down
2 changes: 1 addition & 1 deletion packages/pkh-ethereum/src/authmethod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
* Get a configured authMethod for an Ethereum account in a web based environment
*/
// eslint-disable-next-line @typescript-eslint/require-await
export async function getAuthMethod(ethProvider: any, account: AccountId): Promise<AuthMethod> {

Check warning on line 25 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 21.x and ubuntu-latest 

Unexpected any. Specify a different type

Check warning on line 25 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and macOS-latest 

Unexpected any. Specify a different type

Check warning on line 25 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 21.x and macOS-latest 

Unexpected any. Specify a different type

Check warning on line 25 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and ubuntu-latest 

Unexpected any. Specify a different type
if (typeof window === 'undefined')
throw new Error('Web Auth method requires browser environment')
const domain = (window as Window).location.hostname
Expand All @@ -40,7 +40,7 @@
*/
// eslint-disable-next-line @typescript-eslint/require-await
export async function getAuthMethod(
ethProvider: any,

Check warning on line 43 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 21.x and ubuntu-latest 

Unexpected any. Specify a different type

Check warning on line 43 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and macOS-latest 

Unexpected any. Specify a different type

Check warning on line 43 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 21.x and macOS-latest 

Unexpected any. Specify a different type

Check warning on line 43 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and ubuntu-latest 

Unexpected any. Specify a different type
account: AccountId,
appName: string,
): Promise<AuthMethod> {
Expand All @@ -58,7 +58,7 @@

async function createCACAO(
opts: AuthMethodOpts,
ethProvider: any,

Check warning on line 61 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 21.x and ubuntu-latest 

Unexpected any. Specify a different type

Check warning on line 61 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and macOS-latest 

Unexpected any. Specify a different type

Check warning on line 61 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and ubuntu-latest 

Unexpected any. Specify a different type
account: AccountId,
): Promise<Cacao> {
const now = new Date()
Expand All @@ -78,14 +78,14 @@
resources: opts.resources,
})
const signature = await safeSend(ethProvider, 'personal_sign', [
encodeHexStr(siweMessage.signMessage()),
encodeHexStr(siweMessage.signMessage({ eip55: true })),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought line https://github.com/ceramicnetwork/js-did/pull/193/files#diff-d2c60f04e1dcb4772ca82cf036d7f3826d992a107c54b8ff1e844128fb091189R99 was the issue with the casing of the address?

What you are signing might be modified to be eip55 but the message itself doesn't seem to have the casing.

Or am I mistaken?

Copy link

@derekpierre derekpierre Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this -

js-did/packages/pkh-ethereum/src/authmethod.ts

Line 66 in f771e4a

const normAccount = normalizeAccountId(account)
...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this get's a bit complicated because Ceramic needs the DID in lowercase format for the controller of any given stream.

The .signMessage is badly named. It really just returns the messaged to be sent to the wallet and signed. You can see the implementation accepts the eip55 parameter here:

js-did/packages/cacao/src/siwx/siwe.ts

Lines 28 to 43 in 3a8ad6d

signMessage({ eip55 }: { eip55?: boolean } = {}): string {
// TODO - switch to eip55 by default when the verification change has been roled out
let message: string
switch (this.type) {
case SignatureType.PERSONAL_SIGNATURE: {
message = eip55 ? this.toMessageEip55() : this.toMessage()
break
}
default: {
message = eip55 ? this.toMessageEip55() : this.toMessage()
break
}
}
return message
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. So I would use toMessageEip55() to get the string with the correct casing.

The signature associated with the SiweMessage object used for the session, did that use lower case for the address when signing the string, or does it use the EIP-55 format?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and toMessageEip55() in turn uses checksumAddress from the viem package.

The string that gets sent to the wallet for signing should be a valid SIWE message, i.e. use eip55 encoding for the address.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to using toMessageEip55() but now there is a signature mismatch with the signature on the SIWE message object.

Here is the code I use to get the information from the DID session:

const session = await DIDSession.fromSession(sessionStr);
const message = SiweMessage.fromCacao(session.cacao);
const messageStr = message.toMessageEip55();
const signature = message.signature;

Later on when validating the signature with the message, the validation fails:

error:  SiweError
expected: "0xf9B4Fbf41C540ba28491a5B9EFD9BbcaBaaa95d3"
received: "Resolved address to be 0x2d1F436Ce1A7Ca3959f23D90981B2e65f46629e6"
type: "Signature does not match address of the message."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derekpierre Why are you calling .toMessageEip55() separately? Maybe if you want to share more of your code I can help to see what's going wrong?

Copy link

@derekpierre derekpierre Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it was an old dependency. It works now!

I call .toMessageEip55() because I'm trying to reuse the same SIWE message string and signature that Ceramic stores in the DIDSession. This message/signature combination is passed to TACo nodes as proof of ownership of a wallet - so instead of generating a separate one, we just reuse Ceramic's as a single sign-on type of functionality.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, glad you got it working. Let us know if you run into any other issues!

normAccount.address,
])
siweMessage.signature = signature
return Cacao.fromSiweMessage(siweMessage)
}

async function requestChainId(provider: any): Promise<number> {

Check warning on line 88 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 21.x and ubuntu-latest 

Unexpected any. Specify a different type

Check warning on line 88 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and macOS-latest 

Unexpected any. Specify a different type

Check warning on line 88 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and ubuntu-latest 

Unexpected any. Specify a different type
const chainIdHex = (await safeSend(provider, 'eth_chainId', [])) as string
return parseInt(chainIdHex, 16)
}
Expand All @@ -93,7 +93,7 @@
/**
* Helper function to get an accountId (CAIP10) for an Ethereum account, uses ethProvider to get chainId/network
*/
export async function getAccountId(ethProvider: any, address: string): Promise<AccountId> {

Check warning on line 96 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 21.x and ubuntu-latest 

Unexpected any. Specify a different type

Check warning on line 96 in packages/pkh-ethereum/src/authmethod.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test on Node 20.x and macOS-latest 

Unexpected any. Specify a different type
const ethChainId = await requestChainId(ethProvider)
const chainId = `${CHAIN_NAMESPACE}:${ethChainId}`
return new AccountId({ address: address.toLowerCase(), chainId })
Expand Down
Loading
Loading