feat: dpop support #1966

auer-martin · 2024-07-25T13:27:25Z

blocked by for now Sphereon-Opensource/OID4VC#131

Signed-off-by: Martin Auer <[email protected]>

changeset-bot · 2024-07-25T13:27:28Z

🦋 Changeset detected

Latest commit: 33ed706

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages

Name	Type
@credo-ts/core	Patch
@credo-ts/openid4vc	Patch
@credo-ts/action-menu	Patch
@credo-ts/anoncreds	Patch
@credo-ts/askar	Patch
@credo-ts/bbs-signatures	Patch
@credo-ts/cheqd	Patch
@credo-ts/drpc	Patch
@credo-ts/indy-sdk-to-askar-migration	Patch
@credo-ts/indy-vdr	Patch
@credo-ts/node	Patch
@credo-ts/question-answer	Patch
@credo-ts/react-native	Patch
@credo-ts/tenants	Patch

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

TimoGlastra

Nice very good stuff martin!!'

Only thinkg i'm not sure about is the retuen value of requestAccesToken. Bit unfortunate we already released the previous pr as now it will be a breaking change...

TimoGlastra · 2024-07-25T14:11:25Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

+    if (!dPoPSigningAlgValuesSupported) return undefined
+
+    const alg = dPoPSigningAlgValuesSupported
+      .flatMap((alg) => alg)


Why is this needed? It's already string array right?

TimoGlastra · 2024-07-25T14:13:31Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

@@ -271,6 +319,11 @@ export class OpenId4VciHolderService {
    let accessTokenResponse: OpenIDResponse<AccessTokenResponse>

    const accessTokenClient = new AccessTokenClient()
+
+    const createDPoPOptions = await this.getCreateDPoPOptions(agentContext, metadata)


In credo we usually remove the capitalization if it's being used aa a word.

So createDpopOptions. Then you always know when a word starts and stops.

TimoGlastra · 2024-07-25T14:13:49Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

+    const createDPoPOptions = await this.getCreateDPoPOptions(agentContext, metadata)
+    const dPoPJwk = createDPoPOptions ? getJwkFromJson(createDPoPOptions.jwtIssuer.jwk) : undefined
+
+    resolvedCredentialOffer.metadata.credentialIssuerMetadata


Suggested change

resolvedCredentialOffer.metadata.credentialIssuerMetadata

TimoGlastra · 2024-07-25T14:17:18Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

@@ -297,7 +352,7 @@ export class OpenId4VciHolderService {

    this.logger.debug('Requested OpenId4VCI Access Token.')

-    return accessTokenResponse.successBody
+    return { ...accessTokenResponse.successBody, dPoPJwk }


This is infortunate, but i don't think we should mix it. Ideally we return a new object with keys: {response: {body,headers,code}, dpop: {jwk}}

Then It's more extendable.

Maybe deprecate and add a new one? Or we keep it like this for now and upgrade when moving to 0.6

But still would like to have dpop.jwk so we can extend dpop values as well.

I would leave it as is for now. Maybe change with 0.6 then.

But still would like to have dpop.jwk so we can extend dpop values as well.
On the client side you only receive the dpop not really things that can be extended IMO

Until we maybe want to return the dpop that was sent, or the htu, or anything.. I think we've had a few times now where not adding an extra layer of nesting resulted in using having to make breaking changes. Rather take that into account now

TimoGlastra · 2024-07-25T14:17:29Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

@@ -308,6 +363,7 @@ export class OpenId4VciHolderService {
      resolvedAuthorizationRequestWithCode?: OpenId4VciResolvedAuthorizationRequestWithCode
      accessToken?: string
      cNonce?: string
+      dPoPJwk?: Jwk


TimoGlastra · 2024-07-25T14:24:06Z

packages/openid4vc/src/openid4vc-issuer/router/accessTokenEndpoint.ts

@@ -120,6 +122,30 @@ export function handleTokenRequest(config: OpenId4VciAccessTokenEndpointConfig)
    const issuerMetadata = openId4VcIssuerService.getIssuerMetadata(agentContext, issuer)
    const accessTokenSigningKey = Key.fromFingerprint(issuer.accessTokenPublicKeyFingerprint)

+    let dPoPJwk: JWK | undefined
+    if (request.headers.dpop) {


Can we require dpop?

Well, we could implement it. But in theory, the client can opt into it for more security.
Also, supporting dpop without having clients use it is perfectly viable.
If we implement it we should probably create a config field allowing users to opt into that strict behaviour

For the funke pid issuer it is a requirement. There's going to be cases where the issuer wants to require it (fine to keep it out for now, but something we can add later. Maybe on the issuer record, or per credential offer)

TimoGlastra · 2024-07-25T14:25:51Z

packages/openid4vc/src/openid4vc-issuer/router/verifyResourceRequest.ts

@@ -27,6 +36,15 @@ export async function verifyAccessToken(
    },
  })

+  const fullUrl = request.protocol + '://' + request.get('host') + request.originalUrl


Same here . Need to use baseUrl from configuration

TimoGlastra · 2024-07-25T14:26:25Z

packages/openid4vc/src/openid4vc-issuer/router/verifyResourceRequest.ts

@@ -27,6 +36,15 @@ export async function verifyAccessToken(
    },
  })

+  const fullUrl = request.protocol + '://' + request.get('host') + request.originalUrl
+  await verifyResourceDPoP(


I know it looks a bit weird, but consistency is king imo

Suggested change

await verifyResourceDPoP(

await verifyResourceDpop(

TimoGlastra · 2024-07-25T14:27:44Z

packages/openid4vc/src/shared/utils.ts

    if (!issuer.startsWith('https://')) {
      throw new CredoError('The X509 certificate issuer must be a HTTPS URI.')
    }

    if (leafCertificate.sanUriNames?.includes(issuer)) {
      return {
        ...openId4VcTokenIssuer,
+        alg: alg as unknown as SigningAlgo,


I think the lib should just support string. As you can use any algorithm and this is a bit annoying to cast everytime

TimoGlastra · 2024-07-25T14:30:42Z

packages/openid4vc/tests/openid4vc.e2e.test.ts

@@ -218,6 +223,7 @@ describe('OpenId4Vc', () => {
      },
    })
    const openIdIssuerTenant2 = await issuerTenant2.modules.openId4VcIssuer.createIssuer({
+      dPoPSigningAlgValuesSupported: ['EdDSA'],


Thinking in general whether this is something you want to configure per issuer? It will make uodates in multi-tenancy quite cumbersome. But i also see the case for having different values per issuer

TimoGlastra · 2024-08-01T10:08:04Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

+      if (!createDPoPOpts.jwtIssuer.jwk.kty) {
+        throw new CredoError('Missing required key type (kty) in the jwk.')
+      }


why is this needed? In theory the jwk should have this property, so I'm not sure why we check for this specific property

TimoGlastra · 2024-08-01T10:08:50Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

+      if (!createDPoPOpts.jwtIssuer.jwk.kty) {
+        throw new CredoError('Missing required key type (kty) in the jwk.')
+      }
+      dpopJwk = getJwkFromJson(createDPoPOpts.jwtIssuer.jwk as JwkJson)


Why first transform from JWK class to json and then back to jwk, shouldn't the createDpopOptions just return the instance?

Hmmm. I don't think so because we create the options which are passed to sphereons library and there we don't have the jwk class

TimoGlastra · 2024-08-01T10:10:41Z

packages/openid4vc/src/openid4vc-holder/OpenId4VcHolderApi.ts

+    const {
+      access_token: accessToken,
+      c_nonce: cNonce,
+      dpop,
+    } = await this.openId4VciHolderService.requestAccessToken(this.agentContext, options)


We can change the service interface just fine, so let's change the service return value to:

{ accessToken, cNonce, dpop, response // should be cloned response (using .clone) }

? what should the response contain? the whole response?

Yes it would be a start at solving #1965, which was requested. Returning the response means better extensibility as you can extract the headers, etc..

In the PR where I implemented the tokenRequestfunctionality, I had that behaviour, and you made me change it.

Okay then i misunderstood the previous implementation. What i don't want is requiring the whole access token response as input to the request credentials method.

I want to have separation between the required fields for credo to work, and additional params you may need for extension. And as I said in the previous PR, i'm not a fan of mixing the successBody with extra params we extracted from e.g. the header.

TimoGlastra · 2024-08-01T10:11:56Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

@@ -311,6 +371,7 @@ export class OpenId4VciHolderService {
      resolvedAuthorizationRequestWithCode?: OpenId4VciResolvedAuthorizationRequestWithCode
      accessToken?: string
      cNonce?: string
+      dpop?: { dpopJwk: Jwk; dpopNonce?: string }


just thinking: is it needed to prefix dpop if we already are in dpop object?

TimoGlastra · 2024-08-01T10:12:48Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

+      ? {
+          access_token: options.accessToken,
+          c_nonce: options.cNonce,
+          ...(options.dpop && { dpop: { dpopJwk: options.dpop.dpopJwk, dpopNonce: options.dpop?.dpopNonce } }),


this won't work?

Suggested change

...(options.dpop && { dpop: { dpopJwk: options.dpop.dpopJwk, dpopNonce: options.dpop?.dpopNonce } }),

...(options.dpop && { dpop: options.dpop }),

TimoGlastra · 2024-08-01T10:15:31Z

packages/openid4vc/src/openid4vc-holder/OpenId4VciHolderService.ts

+      let createDpopOpts: CreateDPoPClientOpts | undefined
+      if (tokenResponse.dpop) {
+        const jwk = tokenResponse.dpop.dpopJwk
+        const alg = jwk.supportedSignatureAlgorithms[0]
+
+        createDpopOpts = {
+          jwtIssuer: { alg: alg as unknown as SigningAlgo, jwk: jwk.toJson() },
+          jwtPayloadProps: { accessToken: tokenResponse.access_token, nonce: tokenResponse.dpop?.dpopNonce },
+          createJwtCallback: getCreateJwtCallback(agentContext),
+        }
+      }


Why not reuse this.getCreateDPoPOptions here? Also this could give an error when multiple alg values are supported for a jwk type, but the server only supports a specific one. So reusing the dpop alg values supported logic from existing method makes sense?

Also, accessing index [0] is unsafe again

Also should we check if dpop is required? Can we see that using the access token? Or how do we know it is required?

if the tokenresponse contains dpop it is required

But token response is passed by the user

Hmm, IMO, the user should not modify the object unless he knows what he is doing.
Also in theory it is valid to remove it since an authorization server can protect multiple credential issuers,
for some of them use of dpop may be optional even if the authorization server requires dpop.

TimoGlastra · 2024-08-01T10:18:30Z

packages/openid4vc/src/openid4vc-issuer/OpenId4VcIssuerApi.ts


-    return this.openId4VcIssuerService.updateIssuer(this.agentContext, issuer)
+    return this.openId4VcIssuerService.updateIssuer(this.agentContext, Object.assign(issuer, issuerOptions))


I'd rather manually assign the props that can be updated instead of doing object.assign. Especially since if you're not using TypeScript it will just assign any property to the record that you pass here. You could provide e.g. type: 'Random' and it will overwrite the type of the record.

TimoGlastra · 2024-08-01T10:21:22Z

packages/openid4vc/src/shared/utils.ts

+      if (!jwtIssuer.jwk.kty) {
+        throw new CredoError('Missing required key type (kty) in the jwk.')
+      }


again, why this check?

can be undefined in sphereon

But in this case we have a jwk class instance right? So it won't happen?

TimoGlastra · 2024-08-03T10:07:03Z

Made a small mistake in my PR on jwk thumbprint, so we need to wait for Sphereon-Opensource/OID4VC#140

Signed-off-by: Timo Glastra <[email protected]>

Signed-off-by: Martin Auer <[email protected]>

Signed-off-by: Timo Glastra <[email protected]>

auer-martin added 3 commits July 22, 2024 11:29

fix: openid4vc draft 13 support (credential-response format removed)

a3fe108

Signed-off-by: Martin Auer <[email protected]>

Merge branch 'main' of github.com:openwallet-foundation/credo-ts

88e2ee5

feat: integrate dpop functionality

8765c0b

Signed-off-by: Martin Auer <[email protected]>

TimoGlastra reviewed Jul 25, 2024

View reviewed changes

auer-martin and others added 6 commits July 26, 2024 08:14

fix: incorporate parts of the feedback

cb3e749

fix: feedback

c49466b

fix: types

c599fa9

fix: update lib

7a656c8

feat: dpop retry mechanisms

f250d57

Merge remote-tracking branch 'upstream/main' into dpop

7b13e97

TimoGlastra reviewed Aug 1, 2024

View reviewed changes

TimoGlastra approved these changes Aug 1, 2024

View reviewed changes

TimoGlastra mentioned this pull request Aug 1, 2024

feat: authenticated screens and pid receiving animo/paradym-wallet#130

Merged

auer-martin mentioned this pull request Aug 1, 2024

test: openid4vp x509 #1984

Merged

auer-martin and others added 2 commits August 1, 2024 20:17

feat: incorporate feedback

e55bb2f

Merge remote-tracking branch 'upstream/main' into dpop

004eb51

TimoGlastra marked this pull request as ready for review August 3, 2024 09:27

TimoGlastra and others added 4 commits August 3, 2024 12:07

small type improvement

bc92691

Signed-off-by: Timo Glastra <[email protected]>

feat: update version

7273ca1

Signed-off-by: Martin Auer <[email protected]>

pin version

f2d73af

Signed-off-by: Timo Glastra <[email protected]>

Create sharp-crews-sniff.md

33ed706

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra merged commit fa62b74 into openwallet-foundation:main Aug 5, 2024
11 of 12 checks passed

	...(options.dpop && { dpop: { dpopJwk: options.dpop.dpopJwk, dpopNonce: options.dpop?.dpopNonce } }),
	...(options.dpop && { dpop: options.dpop }),


		return this.openId4VcIssuerService.updateIssuer(this.agentContext, issuer)
		return this.openId4VcIssuerService.updateIssuer(this.agentContext, Object.assign(issuer, issuerOptions))

feat: dpop support #1966

feat: dpop support #1966

Conversation

auer-martin commented Jul 25, 2024

changeset-bot bot commented Jul 25, 2024 • edited Loading

🦋 Changeset detected

TimoGlastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra commented Aug 3, 2024

changeset-bot bot commented Jul 25, 2024 •

edited

Loading