-
Notifications
You must be signed in to change notification settings - Fork 200
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
base: main
Are you sure you want to change the base?
Changes from all commits
eee749f
34f1f3e
2c9e53b
29f7d73
9eeb5c8
187cf4e
b641f1a
5a15fb4
ee6403c
cd1befd
53d496b
888909d
61ab47b
41d4180
4493bb9
79bd5ce
aedc9b3
d9940a2
2a10e10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -504,10 +504,13 @@ export class DidExchangeProtocol { | |
message: DidExchangeRequestMessage | DidExchangeResponseMessage, | ||
invitationKeysBase58: string[] = [] | ||
) { | ||
// The only supported case where we expect to receive a did-document attachment is did:peer algo 1 | ||
return isDid(message.did, 'peer') && getNumAlgoFromPeerDid(message.did) === PeerDidNumAlgo.GenesisDoc | ||
? this.extractAttachedDidDocument(agentContext, message, invitationKeysBase58) | ||
: this.extractResolvableDidDocument(agentContext, message, invitationKeysBase58) | ||
// 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) | ||
} | ||
Comment on lines
+507
to
+513
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
I would be curious: in ACA-Py do you use |
||
} | ||
|
||
/** | ||
|
@@ -522,11 +525,11 @@ export class DidExchangeProtocol { | |
// Validate did-rotate attachment in case of DID Exchange response | ||
if (message instanceof DidExchangeResponseMessage) { | ||
const didRotateAttachment = message.didRotate | ||
|
||
if (!didRotateAttachment) { | ||
throw new DidExchangeProblemReportError('DID Rotate attachment is missing.', { | ||
problemCode: DidExchangeProblemReportReason.ResponseNotAccepted, | ||
}) | ||
throw new DidExchangeProblemReportError( | ||
'Either a DID Rotate attachment or a didDoc attachment must be provided to make a secure connection', | ||
{ problemCode: DidExchangeProblemReportReason.ResponseNotAccepted } | ||
) | ||
} | ||
|
||
const jws = didRotateAttachment.data.jws | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}))$' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this change in regex mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This regex is specifically changed for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
) | ||
|
||
export function isValidPeerDid(did: string): boolean { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ import { createWalletKeyPairClass } from '../../../crypto/WalletKeyPair' | |
import { CredoError } from '../../../error' | ||
import { injectable } from '../../../plugins' | ||
import { asArray, JsonTransformer } from '../../../utils' | ||
import { VerificationMethod } from '../../dids' | ||
import { DidsApi, VerificationMethod } from '../../dids' | ||
import { getKeyFromVerificationMethod } from '../../dids/domain/key-type' | ||
import { W3cCredentialsModuleConfig } from '../W3cCredentialsModuleConfig' | ||
import { w3cDate } from '../util' | ||
|
@@ -339,12 +339,23 @@ export class W3cJsonLdCredentialService { | |
agentContext: AgentContext, | ||
verificationMethod: string | ||
): Promise<Key> { | ||
const documentLoader = this.w3cCredentialsModuleConfig.documentLoader(agentContext) | ||
const verificationMethodObject = await documentLoader(verificationMethod) | ||
const verificationMethodClass = JsonTransformer.fromJSON(verificationMethodObject.document, VerificationMethod) | ||
|
||
const key = getKeyFromVerificationMethod(verificationMethodClass) | ||
return key | ||
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}`) | ||
} | ||
Comment on lines
+342
to
+358
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
} | ||
|
||
private getSignatureSuitesForCredential(agentContext: AgentContext, credential: W3cJsonLdVerifiableCredential) { | ||
|
There was a problem hiding this comment.
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