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

credential-eip712 is not compatible with every DID method #991

Closed
andyv09 opened this issue Aug 12, 2022 · 2 comments · Fixed by #1011
Closed

credential-eip712 is not compatible with every DID method #991

andyv09 opened this issue Aug 12, 2022 · 2 comments · Fixed by #1011
Labels
bug Something isn't working pinned don't close this just for being stale triage

Comments

@andyv09
Copy link
Contributor

andyv09 commented Aug 12, 2022

Bug severity
3

Describe the bug
Credential EIP712 plugin is not compatible with every DID method. It works fine with did:ethr, but when tried to use did:key it did not work. Same error happens during verification of VPs where holder is did:key.

To Reproduce
Steps to reproduce the behaviour
Generating VP:

  1. Import a did:key did using agent.didManagerImport(...) (Provider of keys is MetaMask)
  2. createVerifiablePresentation (EIP712) with said did
  3. "Key not found" error is thrown.

Verifying VP:

  1. agent.verifyVerifiableCredentialEIP712({presentation: ...})
  2. "Recovered address does not match issuer" error is thrown.

Observed behaviour
I think the error comes from the getEthereumAddress function. This function is trying to find blockchainAccountId, which is missing from the verificationMethods of did:key did document (and potentially documents generated by other did methods).

Another issue might come from the fact that compareBlockchainAccountId function from did-utils has hardcoded EcdsaSecp256k1RecoveryMethod2020, while did:key resolver uses EcdsaSecp256k1VerificationMethod2019 and from my understanding, those two methods should be interchangeable/compatible.

I was able to bypass the issue when generating a VP, by generating a custom DID document for did:key, that adds an additional property blockchainAccountId: + address + '@eip155:4', to the verification method.

getEthereumAddress function 'could' be fixed by adding various methods that transform publicKeyHex/publicKeyBase58 into blockchainAddress.

Expected behaviour
Credential EIP712 should work with every DID method capable of expressing secp256k1/Ethereum address

Additional context
This error was observed in a MetaMask snaps application, where a very lightweight implementation of did-key-resolver and did-key-provider was used instead of @veramo/did-provider-key. Resulting did document is identical to the one generated with @veramo/did-provider-key.

Versions (please complete the following information):

  • Veramo: next
  • Node Version: 16
@andyv09 andyv09 added the bug Something isn't working label Aug 12, 2022
@mirceanis mirceanis added triage pinned don't close this just for being stale labels Aug 12, 2022
@andyv09
Copy link
Contributor Author

andyv09 commented Sep 27, 2022

@mirceanis is there any ETA on this issue?

@mirceanis
Copy link
Member

@mirceanis is there any ETA on this issue?

I don't have an ETA yet, but you seem to have identified the problem precisely.
There is a bit of relevant code in did-jwt for extracting the public key material from other verification method types: https://github.com/decentralized-identity/did-jwt/blob/86010a6f403619b85259b1790d9d21799716bd4b/src/VerifierAlgorithm.ts#L32
This then needs to be converted to an ethereumaddress/blockchainAccountId

We could either export that method from did-jwt and bubble up a new release from there or duplicate the relevant bits of it here until it becomes available from there.

Would you like to contribute a PR with a fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pinned don't close this just for being stale triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants