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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { fetchCredentialDefinition } from '../../utils/anonCredsObjects'
import {
getIndyNamespaceFromIndyDid,
getQualifiedDidIndyDid,
getUnQualifiedDidIndyDid,
getUnqualifiedRevocationRegistryDefinitionId,
isIndyDid,
isUnqualifiedCredentialDefinitionId,
Expand Down
2 changes: 1 addition & 1 deletion packages/anoncreds/src/utils/w3cAnonCredsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),

anonCredsUnqualifiedSchemaIssuerId: getUnQualifiedDidIndyDid(schema.issuerId),
anonCredsUnqualifiedRevocationRegistryId: revocationRegistryId
? getUnQualifiedDidIndyDid(revocationRegistryId)
Expand Down
19 changes: 11 additions & 8 deletions packages/core/src/modules/connections/DidExchangeProtocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
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?

}

/**
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export class V2OfferCredentialMessage extends AgentMessage {

@Expose({ name: 'credential_preview' })
@Type(() => V2CredentialPreview)
@IsOptional()
@ValidateNested()
@IsInstance(V2CredentialPreview)
public credentialPreview?: V2CredentialPreview
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ export function isBls12381G1Key2020(verificationMethod: VerificationMethod): ver
* Get a key from a Bls12381G1Key2020 verification method.
*/
export function getKeyFromBls12381G1Key2020(verificationMethod: Bls12381G1Key2020) {
if (!verificationMethod.publicKeyBase58) {
throw new CredoError('verification method is missing publicKeyBase58')
if (verificationMethod.publicKeyBase58) {
return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.Bls12381g1)
}

return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.Bls12381g1)
throw new CredoError('verification method is missing publicKeyBase58')
KolbyRKunz marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ export function isBls12381G2Key2020(verificationMethod: VerificationMethod): ver
* Get a key from a Bls12381G2Key2020 verification method.
*/
export function getKeyFromBls12381G2Key2020(verificationMethod: Bls12381G2Key2020) {
if (!verificationMethod.publicKeyBase58) {
throw new CredoError('verification method is missing publicKeyBase58')
if (verificationMethod.publicKeyBase58) {
return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.Bls12381g2)
}

return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.Bls12381g2)
throw new CredoError('verification method is missing publicKeyBase58')
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ export function isEcdsaSecp256k1VerificationKey2019(
* Get a key from a EcdsaSecp256k1VerificationKey2019 verification method.
*/
export function getKeyFromEcdsaSecp256k1VerificationKey2019(verificationMethod: EcdsaSecp256k1VerificationKey2019) {
if (!verificationMethod.publicKeyBase58) {
throw new CredoError('verification method is missing publicKeyBase58')
if (verificationMethod.publicKeyBase58) {
return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.K256)
}

return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.K256)
throw new CredoError('verification method is missing publicKeyBase58')
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ export function isEd25519VerificationKey2018(
* Get a key from a Ed25519VerificationKey2018 verification method.
*/
export function getKeyFromEd25519VerificationKey2018(verificationMethod: Ed25519VerificationKey2018) {
if (!verificationMethod.publicKeyBase58) {
throw new CredoError('verification method is missing publicKeyBase58')
if (verificationMethod.publicKeyBase58) {
return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.Ed25519)
}

return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.Ed25519)
throw new CredoError('verification method is missing publicKeyBase58')
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,20 @@ export function getMultikey({ did, key, verificationMethodId }: GetMultikeyOptio
*/
export function isMultikey(
verificationMethod: VerificationMethod
): verificationMethod is VerificationMethod & { type: 'Multikey' } {
): verificationMethod is
| (VerificationMethod & { type: 'Multikey' })
| (VerificationMethod & { publicKeyMultibase: string }) {
return verificationMethod.type === VERIFICATION_METHOD_TYPE_MULTIKEY
}

/**
* Get a key from a Multikey verification method.
*/
export function getKeyFromMultikey(verificationMethod: VerificationMethod & { type: 'Multikey' }) {
export function getKeyFromMultikey(
verificationMethod:
| (VerificationMethod & { type: 'Multikey' })
| (VerificationMethod & { publicKeyMultibase: string })
) {
if (!verificationMethod.publicKeyMultibase) {
throw new CredoError(
`Missing publicKeyMultibase on verification method with type ${VERIFICATION_METHOD_TYPE_MULTIKEY}`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,17 @@ export function isX25519KeyAgreementKey2019(
* Get a key from a X25519KeyAgreementKey2019 verification method.
*/
export function getKeyFromX25519KeyAgreementKey2019(verificationMethod: X25519KeyAgreementKey2019) {
if (!verificationMethod.publicKeyBase58) {
throw new CredoError('verification method is missing publicKeyBase58')
if (verificationMethod.publicKeyBase58) {
return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.X25519)
}
if (verificationMethod.publicKeyMultibase) {
const key = Key.fromFingerprint(verificationMethod.publicKeyMultibase)
if (key.keyType === KeyType.X25519) return key
else
throw new CredoError(
`Unexpected key type from resolving multibase encoding, key type was ${key.keyType} but expected ${KeyType.X25519}`
)
}

return Key.fromPublicKeyBase58(verificationMethod.publicKeyBase58, KeyType.X25519)
throw new CredoError('verification method is missing publicKeyBase58 or publicKeyMultibase')
}
2 changes: 1 addition & 1 deletion packages/core/src/modules/dids/methods/peer/didPeer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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?

)

export function isValidPeerDid(did: string): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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
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?

}

private getSignatureSuitesForCredential(agentContext: AgentContext, credential: W3cJsonLdVerifiableCredential) {
Expand Down
1 change: 1 addition & 0 deletions packages/indy-vdr/src/dids/didSovUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
DidCommV2Service,
convertPublicKeyToX25519,
CredoError,
Buffer,
} from '@credo-ts/core'

export type CommEndpointType = 'endpoint' | 'did-communication' | 'DIDComm'
Expand Down
Loading