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: json ld fixes and aca-py interop fixes #1865

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

KolbyRKunz
Copy link
Contributor

This PR fixes some minor typos and issues with JSON-LD between aca-py and credo. It additionally updates did:peer:2 to ignore unknown service objects, and allow multiple services to be on a did:peer:2 did. These fixes overall allows interop with aca-py's implementation of JSON-LD over didCOMM.

@KolbyRKunz KolbyRKunz changed the title Feat: json ld fixes and aca-py interop fixes feat: json ld fixes and aca-py interop fixes May 14, 2024
@KolbyRKunz KolbyRKunz changed the title feat: json ld fixes and aca-py interop fixes fix: json ld fixes and aca-py interop fixes May 14, 2024
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

Thanks @KolbyRKunz ! Good fixes here. I left some minor comments and also doubts on DID Exchange message handling but overall looks good.

didDocument.addService(JsonTransformer.fromJSON(service, DidDocumentService))
} catch (e) {
//Ignore a service if we do not recognize it
serviceIndex--
Copy link
Contributor

Choose a reason for hiding this comment

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

We should nog skip the service index i think, as then the index references will be messed up. I think we can skip adding it to the document, but also skip the index

Copy link
Contributor

Choose a reason for hiding this comment

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

Also furious when does this happen? If a json transform fails? I don't think we should just swallow such an error. Can you explain why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this I realize this was a bit of a hold over from implementing this functionality in another library that had much stricter serialization rules than typescript does, specifically with unknown additional keys appearing causing errors. With the regex adjustment this is redundant for Credo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this logic as it is not needed in Credo because serialization is more fault tolerant than I thought at development time.

Copy link
Contributor

Choose a reason for hiding this comment

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

The code is still here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With my comment below about did:peer:2 adding multiple services this was added because some of the agents and mediator I tested against included didCommV2 services that were intended to be ignored by the agent if they do not use didCommV2. Have this try catch here successfully ignores it and keeps the indexes of the services we do recognize consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

But by doing this the references between implementations will mess up. We would skip didcomm v2 and others would not skip it and thus the same id could point to different services. I think we should not do this

Copy link
Contributor

Choose a reason for hiding this comment

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

AFter looking at this in more depth -- we don't handle the DIDComm v2 serviceEndpoint being an object. So instead of doing this, we need to fix that parsing of serviceEndpoint instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have example did:peer:2 document with DIDComm v2 service for test? I have a PR locally that correctly fixes #1789, and thus should also fix this issue

… resolution of verification methods, and updated JSON-LD key resolution to not be specific to did:peer

Signed-off-by: KolbyRKunz <[email protected]>
Signed-off-by: KolbyRKunz <[email protected]>
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Hey @KolbyRKunz, left another review. I think it would be great to get it merged. if you can:

  • remove the changes not related to JSON-LD (anoncreds / indy-did) etc.. I think these have been solved by other PRs
  • update the getKeyFromVerificationMethod to support multi-key and update all places that use the specific getKeyFromXXXX methods to use getKeyFromVerificationMethod instead.
  • revert the change to the peerDidNumalgo2.ts file
  • revert the change to the getPublicKeyFromVerificationMethod or explain specifically why this change is needed (as the did resolving is handled by the document loader)

packages/anoncreds/src/utils/w3cAnonCredsUtils.ts Outdated Show resolved Hide resolved
@@ -3,7 +3,7 @@ import { CredoError } from '../../../../error'
import { getAlternativeDidsForNumAlgo4Did } from './peerDidNumAlgo4'

const PEER_DID_REGEX = new RegExp(
'^did:peer:(([01](z)([1-9a-km-zA-HJ-NP-Z]{5,200}))|(2((.[AEVID](z)([1-9a-km-zA-HJ-NP-Z]{5,200}))+(.(S)[0-9a-zA-Z=]*)?))|([4](z[1-9a-km-zA-HJ-NP-Z]{46})(:z[1-9a-km-zA-HJ-NP-Z]{6,}){0,1}))$'
'^did:peer:(([01](z)([1-9a-km-zA-HJ-NP-Z]{5,200}))|(2((.[AEVID](z)([1-9a-km-zA-HJ-NP-Z]{5,200}))+(.(S)[0-9a-zA-Z=]*)*))|([4](z[1-9a-km-zA-HJ-NP-Z]{46})(:z[1-9a-km-zA-HJ-NP-Z]{6,}){0,1}))$'
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change in regex mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This regex is specifically changed for did:peer:2. With the updates to the protocol of did:peer:2 the services on the encoded version are not always a list and can be multiple service appended instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an example did:peer so I can add a test covering this?

didDocument.addService(JsonTransformer.fromJSON(service, DidDocumentService))
} catch (e) {
//Ignore a service if we do not recognize it
serviceIndex--
Copy link
Contributor

Choose a reason for hiding this comment

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

The code is still here?

Comment on lines +342 to +358
if (!verificationMethod.startsWith('did:')) {
const documentLoader = this.w3cCredentialsModuleConfig.documentLoader(agentContext)
const verificationMethodObject = await documentLoader(verificationMethod)
const verificationMethodClass = JsonTransformer.fromJSON(verificationMethodObject.document, VerificationMethod)

const key = getKeyFromVerificationMethod(verificationMethodClass)
return key
} else {
const [did, keyid] = verificationMethod.split('#')
const didsApi = agentContext.dependencyManager.resolve(DidsApi)
const doc = await didsApi.resolve(did)
if (doc.didDocument) {
const verificationMethodClass = doc.didDocument.dereferenceKey(keyid)
return getKeyFromVerificationMethod(verificationMethodClass)
}
throw new CredoError(`Could not resolve verification method with id ${verificationMethod}`)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? The document loader should be able to load did documents, so I'm not sure if we want to bypass it here

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 document loader would resolve the did but not return the full did Document with the verification information applied. This might have changed since my testing because I added this while testing with 0.5.1

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, could you elaborate a bit further? What do you mean with "not return the full did Document with the verification information applied"? What does it return?

Comment on lines 574 to 576
} else {
this.logger.warn(`Document does not contain didRotate`)
}
Copy link
Contributor

@TimoGlastra TimoGlastra Jun 4, 2024

Choose a reason for hiding this comment

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

I don't think this change is correct. extractResolvableDidDocument is called when the did document is not did:peer:1. If we resolve the did, instead of using an diddocument attachment, we REQUIRE the rotate attachment to be present. Could you explain a bit more why this change is needed?

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 test case I ran into was agents or mediators using did:peer:2 without the intention of doing a did rotate or alsoKnownAs. It also is a part of the didExchange RFC that the value is optional for any reason so for interoperability sake I think this should remain.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the did is not rotated from the invitation we can indeed ignore it. But if the did is rotated, not doing this check means the connection can be hijacked. I could send a response even though you created the invitation, because we don't require a signature from the invitation did. If the did is the same, then we don't have that issue and i'm ok with not requiring did rotate. But the current code doesn't take this into consideration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be confused but the warn only happens if the attachment is not present, the else block is for the if statement on line 526 where it checks if the didRotateAttachment is defined. The signature is always checked if the rotate attachment is present. The else block with the warn was a suggestion made by @genaris when this PR was initially made.

Copy link
Contributor

@TimoGlastra TimoGlastra Jun 15, 2024

Choose a reason for hiding this comment

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

The check that is now in place always allows didRotate to not be present, meaning we could just remove the check. So we need to be more specific in checking when didRotate needs to be present: when you rotate your did from the invitation. Even though the Aries RFC mentions it's optional, it's super important for security to do this check. did_rotate was added specifically for this reason, and it's only optional to not introduce a breaking change in the RFC. Before did rotate was introduced, it was basically not possible to rotate a did from the invitation if you were not using diddoc~attach and sign it.

If the did is rotated from the invitation, either a signed diddoc~attach must be present OR a signed did_rotate~attach must be present

@KolbyRKunz
Copy link
Contributor Author

KolbyRKunz commented Jun 14, 2024

@TimoGlastra Sorry for the long time between reviews. I have made most of the recommended changes and commented on the ones that I did not. Hopefully that can get this PR in. Not sure if/when I will be available to update this PR again just so you are aware. Hopefully this helps :)

Signed-off-by: KolbyRKunz <[email protected]>
@TimoGlastra
Copy link
Contributor

TimoGlastra commented Jun 16, 2024

Okay with #1903 and #1902, the following features from this PR remain:

  • multi key: ok
  • support array in did:peer:2? -> Need an example did so we can add a test
  • did dodcument loader with JSON-LD not using the JSON-LD loader -> need more input on what the issue here is exactly. As there could be different behaviour in not using the JSON-LD loader
  • did rotate handling. We should check that if the did is rotated from the invitation, the did_rotate is present and valid. Only if the did is not rotated, is it OK to skip the did_rotate attachment.

All other changes should be removed (please look at changed files tab and undo all changes not needed)

@KolbyRKunz
Copy link
Contributor Author

An example did:peer:2 is this one did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9

It has the multiple service endpoints that require the regex change and it also has didCommV2 endpints

Copy link

changeset-bot bot commented Jun 17, 2024

⚠️ No Changeset found

Latest commit: 2a10e10

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@KolbyRKunz
Copy link
Contributor Author

KolbyRKunz commented Jun 17, 2024

The document loader issue I was running into was that it would resolve a didDoc but the verification arrays would come back as empty services, they would only contain ids. Something like { id: "did:peer:2:someRandomDid#key-1"}, the actual keys would not be included making it impossible to sign the payload correctly.

@KolbyRKunz
Copy link
Contributor Author

I changed the logic on the didExchange issue so that if we are getting a response message we now require that either a didRotate or didDoc attachment is added to the message so that we have a signature to validate against. I have also removed the changes that are made redundant by the other PR's that you mentioned @TimoGlastra. I also want to point out the addition of the buffer import in the indy-vdr package, without that import it is not possible to resolve an abbreviated verkey on a did:sov in React Native. Thanks for working through these fixes with me.

@TimoGlastra
Copy link
Contributor

Could you confirm whether this looks OK as resolved variant of the DID you provided?

{
      "@context": [
        "https://w3id.org/did/v1"
      ],
      "id": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9",
      "service": [
        {
          "type": "DIDCommMessaging",
          "serviceEndpoint": {
            "uri": "https://us-east.proven.mediator.indiciotech.io/message",
            "accept": [
              "didcomm/v2",
              "didcomm/aip2;env=rfc19"
            ]
          },
          "id": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9#didcommmessaging-0"
        },
        {
          "type": "DIDCommMessaging",
          "serviceEndpoint": {
            "uri": "wss://ws.us-east.proven.mediator.indiciotech.io/ws",
            "accept": [
              "didcomm/v2",
              "didcomm/aip2;env=rfc19"
            ]
          },
          "id": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9#didcommmessaging-1"
        },
        {
          "serviceEndpoint": "https://us-east.proven.mediator.indiciotech.io/message",
          "accept": [
            "didcomm/aip1",
            "didcomm/aip2;env=rfc19"
          ],
          "recipientKeys": [
            "#key-2"
          ],
          "type": "did-communication",
          "id": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9#did-communication-2"
        },
        {
          "serviceEndpoint": "wss://ws.us-east.proven.mediator.indiciotech.io/ws",
          "accept": [
            "didcomm/aip1",
            "didcomm/aip2;env=rfc19"
          ],
          "recipientKeys": [
            "#key-2"
          ],
          "type": "did-communication",
          "id": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9#did-communication-3"
        }
      ],
      "authentication": [
        {
          "id": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9#key-2",
          "type": "Ed25519VerificationKey2018",
          "controller": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9",
          "publicKeyBase58": "GtfgyFMiPxp2kdWM9oboanEXhyzQL7N4cCNE6efHSTVo"
        }
      ],
      "keyAgreement": [
        {
          "id": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9#key-1",
          "type": "X25519KeyAgreementKey2019",
          "controller": "did:peer:2.Ez6LSe3YyteKQVXSgGfZyCzbLq9K34zjAfap4EuaWJjbo5Gwk.Vz6MkvLvjZVc9jWJVs8M3qNZeRsnXXZGFjzcRJDH9vvdJMgHB.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6Imh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsImEiOlsiZGlkY29tbS92MiIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXX19.SeyJ0IjoiZG0iLCJzIjp7InVyaSI6IndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwiYSI6WyJkaWRjb21tL3YyIiwiZGlkY29tbS9haXAyO2Vudj1yZmMxOSJdfX0.SeyJzIjogImh0dHBzOi8vdXMtZWFzdC5wcm92ZW4ubWVkaWF0b3IuaW5kaWNpb3RlY2guaW8vbWVzc2FnZSIsICJhIjogWyJkaWRjb21tL2FpcDEiLCJkaWRjb21tL2FpcDI7ZW52PXJmYzE5Il0sICJyZWNpcGllbnRLZXlzIjogWyIja2V5LTIiXSwgInQiOiAiZGlkLWNvbW11bmljYXRpb24ifQ.SeyJzIjogIndzczovL3dzLnVzLWVhc3QucHJvdmVuLm1lZGlhdG9yLmluZGljaW90ZWNoLmlvL3dzIiwgImEiOiBbImRpZGNvbW0vYWlwMSIsImRpZGNvbW0vYWlwMjtlbnY9cmZjMTkiXSwgInJlY2lwaWVudEtleXMiOiBbIiNrZXktMiJdLCAidCI6ICJkaWQtY29tbXVuaWNhdGlvbiJ9",
          "publicKeyBase58": "3NNpNLWYQ4iwBHCCgM5PWZ6ZDrC3xyduMvrppGxGMuAz"
        }
      ]
    }

@KolbyRKunz
Copy link
Contributor Author

@TimoGlastra That didDoc looks correct to me

@@ -222,7 +222,7 @@ export function getW3cRecordAnonCredsTags(options: {
...((isIndyDid(issuerId) || isUnqualifiedIndyDid(issuerId)) && {
anonCredsUnqualifiedIssuerId: getUnQualifiedDidIndyDid(issuerId),
anonCredsUnqualifiedCredentialDefinitionId: getUnQualifiedDidIndyDid(credentialDefinitionId),
anonCredsUnqualifiedSchemaId: getUnQualifiedDidIndyDid(schemaId),
anonCredsUnqualifiedSchemaId: getUnQualifiedDidIndyDid(issuerId),
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be the schemaId, not the issuer id

Suggested change
anonCredsUnqualifiedSchemaId: getUnQualifiedDidIndyDid(issuerId),
anonCredsUnqualifiedSchemaId: getUnQualifiedDidIndyDid(schemaId),

Comment on lines +507 to +513
// Not all agents use didRotate yet, some may still send a didDoc attach with various did types
// we should check if the didDoc attach is there and if not require that the didRotate be present
if (message.didDoc) {
return this.extractAttachedDidDocument(agentContext, message, invitationKeysBase58)
} else {
return this.extractResolvableDidDocument(agentContext, message, invitationKeysBase58)
}
Copy link
Contributor

@TimoGlastra TimoGlastra Jun 24, 2024

Choose a reason for hiding this comment

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

Sorry to be so nitpicky and pedantic, but I want to be careful with this, as it can have a lot of impact on security.

If we allow any did method to be passed in did_doc~attach, it means that e.g. for a did:web document, I could provide it as attachment here, and how you currently implemented the code, it will store the document as is. That means I could impersonate a did:web document from someone else, as we don't actually resolve the did document. We trust that the document the connection provides is the actual did document, and we bypass the ledger security layer most did methods provide.

So I think we should implement the logic as follows:

  • check if the did is resolvable using a did resolver (all dids except for did:peer:1)
    • if it is resolvable, and the did is the same as in the invitation we will resolve the DID, without requireing did_rotate~attach or did_doc~attach.
    • if it is resolvable, and the did is different from the did in the invitation, verify the did_rotate~attach has been signed with the invitation did (as we do now) and resolve the did
    • if it is not resolvable (so did:peer:1), we will extract the did from the did_doc~attach and verify the signature

I would be curious: in ACA-Py do you use did_doc~attach also with did:peer:4?

@TimoGlastra
Copy link
Contributor

The document loader issue I was running into was that it would resolve a didDoc but the verification arrays would come back as empty services, they would only contain ids. Something like { id: "did:peer:2:someRandomDid#key-1"}, the actual keys would not be included making it impossible to sign the payload correctly.

Can you provide me with a sample input that would cause this? I think it is because of missing @context values. Would like to fix it with the JSON-LD loader and add some tests for it.

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