ceramicnetwork / js-did

A simple interface to interact with DIDs that conform to the DID-provider interface.
Other
95 stars 28 forks source link

The signature problem on Integrating with Lit #173

Closed 0xja-eth closed 8 months ago

0xja-eth commented 9 months ago

Description

I'm trying to approve for Lit Protocol and Ceramic with just one signature. I've managed to do so, but there might be some issues with my solution. I'll describe my problems here and discuss if there's a better approach, or whether it's possible to optimize the code of Lit or Ceramic to better address these issues.

Technical Information

My approach is as follows: first, I call Ceramic's authorization, with the code as below:

const accountId = await getAccountId(ethProvider, address)

const authMethod = await getAuthMethod(ethProvider, accountId)
console.log("[AuthCeramic] Authorize", authMethod)

session = await DIDSession.authorize(authMethod, {resources: ["ceramic://*"]})

After the user signs, they get a session as a login credential. In Lit Protocol, similar to session, there's a data structure called AuthSig as a login credential. The structure of AuthSig is as follows:

interface AuthSig {
    sig: any; // Data after signing
    derivedVia: string; // Generally "web3.eth.personal.sign"
    signedMessage: string; // Data before signing
    address: string; // Signing address
}

Actually, session and AuthSig are somewhat interchangeable to a certain extent. Below is my conversion code:

async function getAuthSig(session, chainId = TargetChainId) {
  const {p, s} = session.cacao;
  const address = p.iss.split(":").pop();

  const message = new SiweMessage({
    domain: p.domain, address, 
    statement: p.statement,
    uri: p.aud,
    version: p.version,
    nonce: p.nonce,
    issuedAt: p.iat,
    expirationTime: p.exp,
    chainId: chainId.toString(),
    resources: p.resources,
  })

  return {
    address, derivedVia: "web3.eth.personal.sign",
    sig: s.s, signedMessage: message.toMessage(),
  } as AuthSig
}

Therefore, we can convert the session obtained after authorizing Ceramic into AuthSig and set it in LocalStorage, to authorize Lit:

const LitAuthSigKey = "lit-auth-signature"
const LitProviderKey = "lit-web3-provider"
const LitDefaultProvider = "metamask"

if (authSig) { // Set the default authSig in advance to avoid reconnecting and re-signing
  setLocalStorage(LitProviderKey, LitDefaultProvider)
  setLocalStorage(LitAuthSigKey, authSig)
}
authSig = await LitJsSdk.checkAndSignAuthMessage({
  chain: Chain
});

// Do some encryption/decryptions ....
const res = await LitJsSdk.decryptToString({
  authSig, chain: Chain, ...
}, lit)

This is my approach.

Solution Problem

There's a problem with the above solution, which is the issue of case sensitivity in the address in the signature information.

In the signature information of Ceramic, the address is converted to lowercase, here's an example:

localhost wants you to sign in with your Ethereum account:
0x66d369e52b1a7ee16.......f84025

Give this application access to some of your data on Ceramic

URI: did:key:z6Mkofcp......pnsDJ26j
Version: 1
Chain ID: 1
Nonce: 1CjSZZjMv2
Issued At: 2023-11-15T08:13:19.690Z
Expiration Time: 2023-11-22T08:13:19.690Z
Resources:
- ceramic://*

However, when we convert this signature into AuthSig and authenticate it in Lit, it says that the address does not conform to the EIP-55 format: a1fbbbbb99c57d8ab7f2f9a94c12187 And this is the request data: image

Therefore, I have to insert the following statement in the first section of the code to ensure that the address in the signature information is in EIP-55 format:

const accountId = await getAccountId(ethProvider, address)
accountId.address = address // Prevent the lowercase address

Only then can I successfully authorize Lit. But after converting to EIP-55, the generated PKH-DID looks like this: did:pkh:eip155:1:0x66d369E52b1A7ee16D65e6a052745561C1f84025, which contains case sensitivity, and that's the problem:

Therefore, I wonder if there is a better solution, or whether it's possible to make some modifications to Ceramic or Lit to solve this problem?

stbrody commented 9 months ago

@oed @ukstv any ideas here?

0xja-eth commented 9 months ago

I have also opened the same issue in Lit: https://github.com/LIT-Protocol/js-sdk/issues/258

oed commented 9 months ago

I'm actually surprised that your proposed solution works. It seems like in the getAuthMethod code, the accountId that get's passed in eventually gets normalized.

Regardless, it seems like we could be able to solve the problem of the mixed case address, by adding a .toLowerCase() later in the code.

0xja-eth commented 9 months ago

I'm actually surprised that your proposed solution works. It seems like in the getAuthMethod code, the accountId that get's passed in eventually gets normalized.

Regardless, it seems like we could be able to solve the problem of the mixed case address, by adding a .toLowerCase() later in the code.

Wait, I forgot something. In fact, in my code, I overrode the createCACAO function to ensure the mixed-case address. Here is my code:

// region Override

export function makeSiweMessage(opts, address, chainId = TargetChainId) {
  const now = new Date();
  const oneWeekLater = new Date(now.getTime() + 7 * 24 * 60 * 60 * 1000);
  return new SiweMessage({
    domain: opts.domain,
    address: address,
    statement: opts.statement ?? 'Give this application access to some of your data on Ceramic',
    uri: opts.uri,
    version: VERSION,
    nonce: opts.nonce ?? randomString(10),
    issuedAt: now.toISOString(),
    expirationTime: opts.expirationTime ?? oneWeekLater.toISOString(),
    chainId: chainId.toString(),
    resources: opts.resources
  });
}

async function createCACAO(opts, ethProvider, account) {
  const siweMessage = makeSiweMessage(opts, account.address, account.chainId.reference)
  siweMessage.signature = await safeSend(
    ethProvider, 'personal_sign', [
      encodeHexStr(siweMessage.signMessage()),
      account.address
    ]);
  console.log('[CACAO] siweMessage', siweMessage, siweMessage.signMessage())
  return fromSiweMessage(siweMessage);
}
function encodeHexStr(str) {
  return `0x${bytesToHex(utf8ToBytes(str))}`;
}
async function getAuthMethod(ethProvider, account) {
  if (typeof window === 'undefined') throw new Error('Web Auth method requires browser environment');
  const domain = window.location.hostname;
  return async (opts)=>{
    opts.domain = domain;
    console.log('[CACAO] opts', opts, ethProvider, account)
    return createCACAO(opts, ethProvider, account);
  };
}

// endregion

I call getAuthMethod defined by myself. If I don't do that, the address will be lowercased and cannot verify by Lit Protocol.

oed commented 9 months ago

Got it. Will work on a PR to fix this 👍

oed commented 9 months ago

After looking close at this, it seems like the best solution to the problem is probably a change to LIT protocol not to be case sensitive about the address.

0xja-eth commented 9 months ago

After looking close at this, it seems like the best solution to the problem is probably a change to LIT protocol not to be case sensitive about the address.

yeh, I think so, so I also opened the same issue in Lit, I'll talk to Lit team. Thank you!

glitch003 commented 9 months ago

Update here: @oed and I are trying to figure out the best path forward. EIP-55 is recommended in the SIWE standard (and Lit just uses the provided SIWE packages that enforce this standard), so this isn't a Lit specific requirement. It's a hard problem because of all the existing data that was stored with non EIP-55 SIWEs, so Ceramic can't just switch to EIP-55. On the other hand, Lit wants to conform to the standard if possible, so that our SIWEs are compatible with the libs that enforce the standard.

I don't think we have an immediate solution but are thinking about options.

skylar745 commented 9 months ago

Interested into solving this too. FYI, I did run into the same issue a couple of months back when Lit changed their auth process to require checksum addresses. I found a message on their discord describing the change from lowercase to checksum. not sure about the reason but probably necessary to them somehow.