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

fix(credential-ld): fix Ed25519Signature2020 verification #1166

Merged
merged 8 commits into from
Apr 14, 2023

Conversation

mirceanis
Copy link
Member

What issue is this PR fixing

This fixes the Ed25519Signature2020 signature verification.

What is being changed

This PR fixes several issues with the suite:

  • The signature encoding was broken (it was interpreted as utf-8 instead of base64url)
  • The underlying LD-suite was expecting the document loader to also dereference DID URLs
  • The other suites would still expect full DID documents to be returned by the documentLoader
  • The underlying signature suite expected a particular @context and publicKeyMultibase to be set on the verification method used for verification

This PR also cleans the default contexts and adds a schema.org @context as well.

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.

closes #1146

nickreynolds and others added 6 commits April 14, 2023 15:25
* fix(utils): fix typo in key comparison

* fix(credential-ld): pre-process DID doc for Ed25519Signature2020

* feat(credential-ld): add more granularity to the documentLoader for DID docs

* feat(utils): test multibase/multicoded for ed25519 keys

* fix(credential-ld): return correct signature bytes for Ed25519-2020 suite
@mirceanis mirceanis requested a review from nickreynolds April 14, 2023 13:34
@mirceanis mirceanis marked this pull request as ready for review April 14, 2023 13:34
Copy link
Member Author

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

some comments to help the review

})
return u8a.fromString(signature)
return base64ToBytes(signature)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the culprit of signature verification failures

}

// this signature suite requires the document loader to dereference the DID URL
if (didUrl.includes('#') && didUrl !== document.id) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the workaround for the very strict documentLoader requirements.

@mirceanis mirceanis force-pushed the nickreynolds/multibase-key-fixes branch from 972cf74 to d8e0530 Compare April 14, 2023 13:49
@mirceanis mirceanis merged commit c965fd5 into next Apr 14, 2023
@mirceanis mirceanis deleted the nickreynolds/multibase-key-fixes branch April 14, 2023 22:38
@Eengineer1
Copy link
Contributor

Nice work getting to the bottom of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants