feat: mdoc-support #2054
feat: mdoc-support #2054
Conversation
🦋 Changeset detectedLatest commit: 95eb0db The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Signed-off-by: Martin Auer <[email protected]>
Signed-off-by: Martin Auer <[email protected]>
Signed-off-by: Martin Auer <[email protected]>
| responseUri: | ||
| authorizationRequest.authorizationRequestPayload.response_uri ?? | ||
| authorizationRequest.authorizationRequestPayload.response_uri, |
| @@ -109,6 +110,14 @@ export class OpenId4VcSiopHolderService { | |||
| challenge: nonce, | |||
| domain: clientId, | |||
| presentationSubmissionLocation: DifPresentationExchangeSubmissionLocation.EXTERNAL, | |||
| openid4vp: { | |||
There was a problem hiding this comment.
how are these values mapped to the presentation? Ideally we don't add oid4vp specific properties to the pex service, but i can understand if it's not possible otherwise
There was a problem hiding this comment.
Yeah, I also don't like it, but I don't see an easy better way to propagate the values. There are used to calculate the session transcript
| @@ -109,6 +110,14 @@ export class OpenId4VcSiopHolderService { | |||
| challenge: nonce, | |||
| domain: clientId, | |||
| presentationSubmissionLocation: DifPresentationExchangeSubmissionLocation.EXTERNAL, | |||
| openid4vp: { | |||
| clientId, | |||
| verifierGeneratedNonce: nonce, | |||
There was a problem hiding this comment.
I think this should be extracted from the challenge property in the pex service itself. As it's now redundant, and can lead to inconsistencies.
There was a problem hiding this comment.
I did this intentionally. I don't think we should use a challenge property. openid4vp is very specific. To me it would be completely unclear why the verification of a device response will fail after changing a e.g. domain/challenge property which have no relation at all to Mdoc?
There was a problem hiding this comment.
What do you think?
| openid4vp: { | ||
| clientId, | ||
| verifierGeneratedNonce: nonce, | ||
| mdocGeneratedNonce: await agentContext.wallet.generateNonce(), |
There was a problem hiding this comment.
we need to make sure to reuse this nonce when encrypting the resposne (we generate a new nonce there at the moment, but these should be the same)
| @@ -41,6 +45,8 @@ export function getSphereonVerifiablePresentation( | |||
| return JsonTransformer.toJSON(verifiablePresentation) as SphereonW3cVerifiablePresentation | |||
| } else if (verifiablePresentation instanceof W3cJwtVerifiablePresentation) { | |||
| return verifiablePresentation.serializedJwt | |||
| } else if (verifiablePresentation instanceof MdocVerifiablePresentation) { | |||
| throw new CredoError('Mdoc verifiable credential is not yet supported.') | |||
There was a problem hiding this comment.
| throw new CredoError('Mdoc verifiable credential is not yet supported.') | |
| throw new CredoError('Mdoc verifiable presentation is not yet supported.') |
|
|
||
| const { deviceResponseBase64Url, presentationSubmission } = await MdocDeviceResponse.openId4Vp(agentContext, { | ||
| mdocs: [Mdoc.fromBase64Url(mdocRecord.base64Url)], | ||
| presentationDefinition: presentationDefinition as DifPresentationExchangeDefinitionV2, |
| claimFormat: presentationToCreate.claimFormat, | ||
| }) | ||
|
|
||
| continue |
There was a problem hiding this comment.
I think instead of continue we could use an else clause maybe? I had a hard time understanding this code, until i spotted this continue statement
| .filter((c) => c instanceof MdocRecord === false) | ||
| .map((c) => getSphereonOriginalVerifiableCredential(c as SdJwtVcRecord | W3cCredentialRecord)) |
There was a problem hiding this comment.
| .filter((c) => c instanceof MdocRecord === false) | |
| .map((c) => getSphereonOriginalVerifiableCredential(c as SdJwtVcRecord | W3cCredentialRecord)) | |
| .filter((c): c is Exclude<typeof c, MdocRecord> => c instanceof MdocRecord === false) | |
| .map((c) => getSphereonOriginalVerifiableCredential(c)) |
| ...selectResultsRaw, | ||
| areRequiredCredentialsPresent: | ||
| mdocPresentationDefinition.input_descriptors.length >= 1 | ||
| ? 'warn' // we don't know yet wheater the required credentials are present |
There was a problem hiding this comment.
when will we know this? (i don't see warn changed to error / info?)
| public get deviceSignedNamespaces(): MdocNameSpaces { | ||
| if (this.issuerSignedDocument instanceof DeviceSignedDocument === false) { | ||
| throw new MdocError(`Cannot get 'device-namespaces from a IssuerSignedDocument. Must be a DeviceSignedDocument.`) | ||
| } | ||
|
|
||
| return this.issuerSignedDocument.allDeviceSignedNamespaces | ||
| } |
There was a problem hiding this comment.
isn't device signed the presentation?
There was a problem hiding this comment.
yes. It can also include device-signed namespaces. Essentially self-attested claims.
| document.addIssuerNameSpace(namespace, namespaceRecord) | ||
| } | ||
|
|
||
| const cert = X509Certificate.fromEncodedCertificate(issuerCertificate) |
There was a problem hiding this comment.
i see we're mostly using string certificates here. Do we want to use that as the primary method to pass around certificate in Credo (i haven't made my mind up yet, just thinking 🤔 ).
There was a problem hiding this comment.
I don't mind. We essentially also set trustedCerts as strings. That's why I have chosen this approach
export interface X509ModuleConfigOptions {
/**
*
* Array of trusted base64-encoded certificate strings in the DER-format.
*/
trustedCertificates?: [string, ...string[]]
| } | ||
|
|
||
| const cert = X509Certificate.fromEncodedCertificate(issuerCertificate) | ||
| const issuerPrivateJwk = await getJwkFromKey(options.issuerKey ?? cert.publicKey) |
There was a problem hiding this comment.
Do we check if the issuerKey and cert key match? Can't we always just extract the key from the cert?
There was a problem hiding this comment.
I also think this should work. Not sure why it doesn't. It may be related to the issue I mentioned below with the jwk representation of keys.
| const issuerPrivateJwk = await getJwkFromKey(options.issuerKey ?? cert.publicKey) | ||
| const issuerSignedDocument = await document.sign( | ||
| { | ||
| issuerPrivateKey: issuerPrivateJwk.toJson(), |
There was a problem hiding this comment.
this is the public key jwk, not the private key jwk right? I think the name is confusing. It's important we don't have to pass the private key, so we can support HSM
| const issuerSignedDocument = await document.sign( | ||
| { | ||
| issuerPrivateKey: issuerPrivateJwk.toJson(), | ||
| alg: issuerPrivateJwk.supportedSignatureAlgorithms[0] as 'ES256' | 'ES384' | 'ES512' | 'EdDSA', |
There was a problem hiding this comment.
I don't like the casting here, it can lead to bugs in the future. We should check if index 0 exists, and then also whether it's a supported alg by mdoc (or does it throw if the provided alg is not supported?)
| return new Mdoc(issuerSignedDocument) | ||
| } | ||
|
|
||
| public async verify(agentContext: AgentContext, options?: MdocVerifyOptions): Promise<boolean> { |
There was a problem hiding this comment.
just as an FYI trusted certificaes will need to be aligned with #2052 once merged
| getMdocContext(agentContext) | ||
| ) | ||
|
|
||
| await verifier.verifyData({ mdoc: this.issuerSignedDocument }, mdocContext) |
There was a problem hiding this comment.
does it return a verification object or something? Would be good to return more than true/false, but also something we can add later
| * @param options {MdocCreateOptions} | ||
| * @returns {Promise<Mdoc>} | ||
| */ | ||
| public async create(options: MdocCreateOptions) { |
There was a problem hiding this comment.
does create also sign? I think in SD JWT API we use sign so might be nice to stay consistent?
| const { mac0, jwk, options } = input | ||
| const { data, signature, alg } = mac0.getRawVerificationData(options) | ||
| return await verifyWithJwk({ jwk, signature, data, alg }) |
There was a problem hiding this comment.
does this still use the credo wallet to verify the signature? I can't see the context or something passed around. It is important.
Aslo it's prob ok for now, but we should try to stay away from doing crypto operations outside of the wallet. Also with e.g. the p256 shared secret calcuation etc.. The more we can do in the wallet, the better.
So if you can provide a list of all cryptographic operations that are needed, we can look to move them to the wallet over time
There was a problem hiding this comment.
We just need the key derivation.
| }, | ||
| getPublicKey: async (input) => { | ||
| const certificate = new x509.X509Certificate(input.certificate) | ||
| const key = await importX509({ |
There was a problem hiding this comment.
what do these do? and why are they part of the protokol library?
There was a problem hiding this comment.
Please take a look at my comment. There is an issue with how we represent the key as jwk. It probably should be compressed. Nothing from the protokoll library should remain here. Atleast related to crypto.
There was a problem hiding this comment.
Which comment? I can't find it
There was a problem hiding this comment.
//expected
//{
//kty: 'EC',
//x: 'OFBq4YMKg4w5fTifsytwBuJf_7E7VhRPXiNm52S3q1E',
//y: 'EyIAXV8gyt5FcRsYHhz4ryz97rjL0uogxHO6jMZr3bg',
//crv: 'P-256'
//}
//actual
//{
//kty: 'EC',
//crv: 'P-256',
//x: 'OFBq4YMKg4w5fTifsytwBuJf_7E7VhRPXiNm52S3q1ETIgBdXyDK3kVxGxgeHPiv',
//y: 'LP3uuMvS6iDEc7qMxmvduNeBp_oWscK1x-3_1KKYDayIctdDcpXHi8HcbehAfVIK'
//}
//const comp = X509Certificate.fromRawCertificate(input.certificate)
//const x = getJwkFromKey(comp.publicKey).toJson()
//// eslint-disable-next-line @typescript-eslint/no-unused-vars
//const { use, ...jwk } = x
//return jwk
|
|
||
| const publicDeviceJwk = COSEKey.import(deviceKeyInfo.deviceKey).toJWK() | ||
|
|
||
| deviceKeyInfo.deviceKey |
There was a problem hiding this comment.
| deviceKeyInfo.deviceKey |
| return { | ||
| deviceResponseBase64Url: TypedArrayEncoder.toBase64URL(deviceResponseMdoc.encode()), | ||
| presentationSubmission: MdocDeviceResponse.createPresentationSubmission({ | ||
| id: 'MdocPresentationSubmission ' + agentContext.wallet.generateNonce(), |
There was a problem hiding this comment.
I think we can also just use an uuid?
| id: 'MdocPresentationSubmission ' + agentContext.wallet.generateNonce(), | |
| id: 'MdocPresentationSubmission ' + await agentContext.wallet.generateNonce(), |
| const trustedCerts = | ||
| options?.trustedCertificates ?? agentContext.dependencyManager.resolve(X509ModuleConfig).trustedCertificates | ||
|
|
||
| if (!trustedCerts) { | ||
| throw new MdocError('No trusted certificates found. Cannot verify mdoc.') |
There was a problem hiding this comment.
same with integraiont of pr #2052
Signed-off-by: Martin Auer <[email protected]>
No description provided.