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

Added conditional logic following the ConditionalProof specification #272

Merged
merged 94 commits into from May 3, 2023

Conversation

theblockstalk
Copy link
Contributor

@theblockstalk theblockstalk commented Mar 7, 2023

Adds the ability to verify JWTs with multi-signatures and delegated signatures and combinations of the two, against DIDs that follow the ConditionalProof specification.

https://github.com/w3c-ccg/verifiable-conditions

See slide deck for overview of changes: https://docs.google.com/presentation/d/13jaeireCRFoI1jFPFkbwBhTMQgUG7zEHRjmq0PWz-CI

theblockstalk and others added 30 commits September 23, 2022 16:33
Copy link
Member

@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.

Apologies for coming back to this review so late.

It seems that verifyJWS functionality is being changed with the expectation that the data is a JWT payload.
This is not a valid assumption and since it is an exported function the changes related to that should be reverted.

src/JWT.ts Outdated Show resolved Hide resolved
src/VerifierAlgorithm.ts Outdated Show resolved Hide resolved
src/JWT.ts Outdated Show resolved Hide resolved
@theblockstalk
Copy link
Contributor Author

I have made changes per your comments.

I have also added test suite ConditionalAlgorithm.test.ts which covers many scenarios. Test suite is now passing all tests.

Copy link
Member

@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.

This looks good.

I think there's a potential error introduced from trying to parse the issuer DID.
Can you clarify the intent there?


let signer: VerificationMethod | null = null

if (did !== didUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

This comparison assumes that a didUrl that does not match the parsed DID refers to one of the verification methods.
The assumption will break if the didUrl contains, for example, query parameters.

Example: did:ethr:0xabcd...?versionId=1234.
This is a valid issuer with a DID document potentially different from did:ethr:0xabcd...

Is the intent here to avoid looping through the authenticators array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the issuer of the DID is a DID URL, here we assume that it is pointing to a specific verification method in the DID Document. This is used in the recursive calling to verifyJWT() for delegation

did:ethr is not a good example, as it cannot express multi-sig conditions or delegations.

Take for example two DIDs which one verification method on each:

  1. did:antelope:jack#delegated which delegates to did:antelope:thomas#base
  2. did:antelope:thomas#base with a ES256K keypair

A VC is issued by did:antelope:jack#delegated. The VC is signed by the did:antelope:thomas#base keypair.

  • verifyJWT() is called which finds the issuer to be did:antelope:jack#delegated
  • it goes and finds the specific verification method, which tells that it is delegating to did:antelope:thomas#base
  • verifyJWT() is then called recursively to check whether the signature matches the did:antelope:thomas#base which it does.

It is important to note that at each of the above steps, a specific verification method is found using the DID URL, required to make the above flow work.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for clarifying the intent.
I realize did:ethr is not relevant to the conditional verification, but the changes I'm asking about are relevant for all verification, not just conditional.

I'll add a test for the case I'm referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I see your concern. If someone issues using a DIDUrl that does not point directly to a verification method, it will now fail. Correct?

Copy link
Member

Choose a reason for hiding this comment

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

exactly

Copy link
Member

Choose a reason for hiding this comment

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

That's one possibility.
Another option would be to just try to find the verification method, which would work for the conditional logic you need, but then to not fail if it's not found, which would continue to do the verification as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JWT.ts verifyJWT()

  const { fragment } = parse(didUrl) as ParsedDID

  let signer: VerificationMethod | null = null

  // If a fragment is supplied, assume it is pointing to a specific Verification Method
  // This is a required feature for Conditional Proof delegated signatures
  if (fragment) {
    const authenticator = authenticators.find((auth) => (parse(auth.id) as ParsedDID).fragment === fragment)

this would avoid this situation (not 100%, e.g. if someone used fragments to refer to a non verification method it would still break)

Copy link
Member

Choose a reason for hiding this comment

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

Your solution with the fragment seems like the right approach, but somehow comparing just the fragment parts seems odd, as it seems close to being able to impersonate some other DID.

How about this pseudocode?

if (didURL contains fragment) {
  find authenticator that matches didUrl
} else {
  try to check the proof against all authenticators
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the original intent, was to treat the DID URL as a relative URL, which are assumed to dereference to a resource in the DID Document

Copy link
Member

@mirceanis mirceanis May 3, 2023

Choose a reason for hiding this comment

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

OK, that's even better, as it would also cover the case where the issuer is a URL but there's also a kid specified in the header, relative to the issuer URL.

I'll merge this PR as is and make the relative kid changes as a separate item ( relates to #267 ).

@mirceanis mirceanis merged commit 9bebe3f into decentralized-identity:master May 3, 2023
1 check passed
uport-automation-bot pushed a commit that referenced this pull request May 3, 2023
# [7.1.0](7.0.0...7.1.0) (2023-05-03)

### Features

* add support for ConditionalProof2022 verificationMethods ([#272](#272)) ([9bebe3f](9bebe3f))
@uport-automation-bot
Copy link
Collaborator

🎉 This PR is included in version 7.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

5 participants