feat: openid4vp-mdoc #2080
feat: openid4vp-mdoc #2080
Conversation
Signed-off-by: Martin Auer <[email protected]>
🦋 Changeset detectedLatest commit: 2bcedab 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 |
packages/core/package.json
Outdated
| @@ -35,16 +35,17 @@ | |||
| "@peculiar/asn1-schema": "^2.3.8", | |||
| "@peculiar/asn1-x509": "^2.3.8", | |||
| "@peculiar/x509": "^1.11.0", | |||
| "@protokoll/mdoc-client": "0.2.35", | |||
| "@protokoll/mdoc-client": "0.2.36", | |||
| "@sd-jwt/core": "^0.7.0", | |||
| "@sd-jwt/decode": "^0.7.0", | |||
| "@sd-jwt/jwt-status-list": "^0.7.0", | |||
| "@sd-jwt/sd-jwt-vc": "^0.7.0", | |||
| "@sd-jwt/types": "^0.7.0", | |||
| "@sd-jwt/utils": "^0.7.0", | |||
| "@sphereon/pex": "5.0.0-unstable.18", | |||
There was a problem hiding this comment.
can you update this to unstable 5.0.0-unstable.25
packages/core/package.json
Outdated
| "@sd-jwt/core": "^0.7.0", | ||
| "@sd-jwt/decode": "^0.7.0", | ||
| "@sd-jwt/jwt-status-list": "^0.7.0", | ||
| "@sd-jwt/sd-jwt-vc": "^0.7.0", | ||
| "@sd-jwt/types": "^0.7.0", | ||
| "@sd-jwt/utils": "^0.7.0", | ||
| "@sphereon/pex": "5.0.0-unstable.18", | ||
| "@sphereon/kmp-mdl-mdoc": "0.2.0-SNAPSHOT.22", |
There was a problem hiding this comment.
We don't need this right?
packages/core/package.json
Outdated
| "@sphereon/pex-models": "^2.3.1", | ||
| "@sphereon/ssi-types": "^0.30.1", | ||
| "@sphereon/ssi-types": "0.30.2-next.129", |
There was a problem hiding this comment.
And this one to 0.30.2-next.135 (for latest mdoc features)
| nonMdocPresentationDefinition.input_descriptors.length === 0 && | ||
| mdocPresentationDefinition.input_descriptors.length > 0 | ||
| ? Status.INFO | ||
| : selectResultsRaw.areRequiredCredentialsPresent, |
There was a problem hiding this comment.
i don't understand this? mdoc also handle mdoc selecting now right? And what if the mdoc is not available, it should'nt be info the nright?
There was a problem hiding this comment.
we set it to error below when we also check if all the mdoc credentials are available.
| return com.sphereon.mdoc.data.device.DeviceResponseCbor.Static.cborDecode( | ||
| new Int8Array(TypedArrayEncoder.fromBase64(verifiablePresentation.base64Url)) | ||
| ) |
There was a problem hiding this comment.
original can just be a string for PEX / ssi-types now. That way we do'nt have to depend on / import the sphereon mdl lib
| try { | ||
| const parsed = parseDeviceResponse(TypedArrayEncoder.fromBase64(base64Url)) | ||
| if (parsed.status === MDocStatus.OK) return true | ||
| return false | ||
| } catch (error) { | ||
| // no-op | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
| try { | |
| const parsed = parseDeviceResponse(TypedArrayEncoder.fromBase64(base64Url)) | |
| if (parsed.status === MDocStatus.OK) return true | |
| return false | |
| } catch (error) { | |
| // no-op | |
| } | |
| return false | |
| try { | |
| const parsed = parseDeviceResponse(TypedArrayEncoder.fromBase64(base64Url)) | |
| return parsed.status === MDocStatus.OK | |
| } catch (error) { | |
| return false | |
| } |
There was a problem hiding this comment.
Also seems quite expensive. That we try to parse. But maybe the best way. Are we doing in places:
if (isBase64DeviceReponse(data)) {
parseBase64DeviceResponse()
}
Because in that case a tryParseBase64DeviceResponse that returns null on error might be better?
|
|
||
| public static fromBase64Url(base64Url: string) { | ||
| const parsed = parseDeviceResponse(TypedArrayEncoder.fromBase64(base64Url)) | ||
| if (parsed.status !== MDocStatus.OK) { |
There was a problem hiding this comment.
no extra error provided in parsed?
There was a problem hiding this comment.
No, not really atm
| const issuerSigned = cborEncode(prepared.get('issuerSigned')) | ||
| const deviceSigned = cborEncode(prepared.get('deviceSigned')) | ||
|
|
||
| return Mdoc.fromIssuerSignedDocument( |
There was a problem hiding this comment.
fromIssuerSigned and then pasing deviceSigned is a bit weird maybe?
| @@ -113,7 +148,18 @@ export class MdocDeviceResponse { | |||
|
|
|||
| const inputDescriptor = this.assertMdocInputDescriptor(options.inputDescriptor) | |||
| const _mdoc = parseIssuerSigned(TypedArrayEncoder.fromBase64(mdoc.base64Url), mdoc.docType) | |||
| return mdocLimitDisclosureToId({ mdoc: _mdoc, inputDescriptor }) | |||
|
|
|||
| const disclosure = mdocLimitDisclosureToId(_mdoc, inputDescriptor) | |||
There was a problem hiding this comment.
Id is maybe a bit offputting as inputDescriptor ( i thought which ID?). So mabye just mdocLimitDisclosureToInputDescriptor?
| const x509ModuleConfig = agentContext.dependencyManager.isRegistered(X509ModuleConfig) | ||
| ? agentContext.dependencyManager.resolve(X509ModuleConfig) | ||
| : undefined |
There was a problem hiding this comment.
this should always be configured, are you getting issues when you just reslve it directly?
packages/openid4vc/package.json
Outdated
| "@sphereon/oid4vci-issuer": "0.16.1-next.168", | ||
| "@sphereon/ssi-types": "^0.30.1", | ||
| "@sphereon/did-auth-siop": "0.16.1-fix.173", | ||
| "@sphereon/kmp-mdl-mdoc": "0.2.0-SNAPSHOT.22", |
There was a problem hiding this comment.
again, would be good to not depend on this.
packages/openid4vc/package.json
Outdated
| "@sphereon/oid4vci-client": "0.16.1-fix.173", | ||
| "@sphereon/oid4vci-common": "0.16.1-fix.173", | ||
| "@sphereon/oid4vci-issuer": "0.16.1-fix.173", | ||
| "@sphereon/ssi-types": "0.30.2-next.129", |
| iss: 'did:key:z6MkrzQPBr4pyqC776KKtrz13SchM5ePPbssuPuQZb5t4uKQ', | ||
| vct: 'OpenBadgeCredential2', | ||
| degree: 'bachelor2', | ||
| }, |
There was a problem hiding this comment.
we should spit this up at some point 😅 but ok for now
| @@ -78,9 +76,12 @@ export function getVerifiablePresentationFromSphereonWrapped( | |||
| } satisfies SdJwtVc | |||
| } else if (wrappedVerifiablePresentation.format === 'mso_mdoc') { | |||
| if (typeof wrappedVerifiablePresentation.original !== 'string') { | |||
There was a problem hiding this comment.
i think the original will ALWAYS be string if we also change the other code to always provide a string from the start. I think it's ok as we don't need to import the library here though. Just want to be careful in depending on two mdl librs. Ideally we jus tprovide string and get back string from PEX. For the rest it should be a black box (so we're also not prone to changes to their library)
| @@ -608,9 +624,28 @@ export class OpenId4VcSiopVerifierService { | |||
|
|
|||
| let isValid: boolean | |||
|
|
|||
| // TODO: it might be better here to look at the presentation submission to know | |||
| // If presentation includes a ~, we assume it's an SD-JWT-VC | |||
| if (typeof encodedPresentation === 'string' && encodedPresentation.includes('~')) { | |||
There was a problem hiding this comment.
let's keep sd-jwt first, as it's cheaper to check for inclusion of ~ than to parse the mdoc.
Also this is what i meant in other comment with check if isBase64DeviceResponse and then MdocDeviceResponse.fromBase64Url. Might as well save the result from the first call and then using that to not do the whole cbor decoding twice.
There was a problem hiding this comment.
Also the last one is JWT -- i think that one is also cheaoer than to do the cbor encoding. So maybe we could also do there if is string and then check for JWT regex to take priority over the mdoc parsing (we do that as the try )
| if (typeof encodedPresentation === 'string' && encodedPresentation.includes('~')) { | ||
| if (typeof encodedPresentation === 'string' && MdocDeviceResponse.isBase64DeviceResponse(encodedPresentation)) { | ||
| if (!options.responseUri || !options.mdocGeneratedNonce) { | ||
| isValid = false |
There was a problem hiding this comment.
let's add a reason to the return value. as this is very hard to debug that you didn't provide these values: https://github.com/Sphereon-Opensource/OID4VC/blob/3b516bef69484cf7af370ea0213e486f4f701d28/packages/siop-oid4vp/lib/authorization-response/types.ts#L94
| @@ -553,9 +563,9 @@ export class OpenId4VcSiopVerifierService { | |||
| // to fix that in Sphereon lib | |||
| client_id: clientId, | |||
| passBy: PassBy.VALUE, | |||
There was a problem hiding this comment.
This one is also included in the final metadata. I think we can jsut remove it?
There was a problem hiding this comment.
that does not work. Many failing tests.
There was a problem hiding this comment.
Hmm okay, this should be fixed in sphereon's lib then.
| @@ -229,6 +232,7 @@ export class OpenId4VcSiopVerifierService { | |||
| const requestClientId = await authorizationRequest.getMergedProperty<string>('client_id') | |||
| const requestNonce = await authorizationRequest.getMergedProperty<string>('nonce') | |||
| const requestState = await authorizationRequest.getMergedProperty<string>('state') | |||
| const responseUri = await authorizationRequest.getMergedProperty<string>('response_uri') | |||
There was a problem hiding this comment.
in other code you tried both response_uri and redirect_uri is it ok to jsut take the response uri here?
There was a problem hiding this comment.
mdoc requires using response_uri. So I think we should be fine
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Martin Auer <[email protected]>
Signed-off-by: Martin Auer <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
| const mdocDeviceResponse = MdocDeviceResponse.fromBase64Url(encodedPresentation) | ||
| await mdocDeviceResponse.verify(agentContext, { | ||
| sessionTranscriptOptions: { | ||
| clientId: options.audience, | ||
| mdocGeneratedNonce: options.mdocGeneratedNonce, | ||
| responseUri: options.responseUri, | ||
| verifierGeneratedNonce: options.nonce, | ||
| }, | ||
| verificationContext: { | ||
| openId4VcVerificationSessionId: options.correlationId, | ||
| }, | ||
| }) | ||
| isValid = true |
There was a problem hiding this comment.
By the way, does thi throw if it fails, or should we set the isValid based on the verify result?
No description provided.