feat: dpop support #131

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

This PR adds support for dPoP.
DPoP support itself did not require many changes. Most of the changes made in this PR are refactorings, e.g., JwtIssuer and JwtVerifier to common so they can be reused ....sorry for that.
Changes made for actual dpop support can be found in this commit.
I open the PR as a draft for now, as I plan to go over the spec one last time in the next couple of days and add some tests.

… develop

nklomp

Overall looking very good. Some small suggestions, comments.

I do think we probably need to create a real common module. Because with the changes proposed (which are good) now the SIOP package will start depending on the vci-common package. The latter really is about VCI ATM, so probably better to start a real common package, which will also server as a nice module for the refactor happening soon

nklomp · 2024-07-25T13:34:18Z

packages/client/lib/AccessTokenClient.ts

@@ -87,10 +92,17 @@ export class AccessTokenClient {
          : undefined,
    });

-    return this.sendAuthCode(requestTokenURL, accessTokenRequest);
+    let dPoP: string | undefined;
+    if (createDPoPOptions?.dPoPSigningAlgValuesSupported && createDPoPOptions?.dPoPSigningAlgValuesSupported.length > 0) {


Suggested change

if (createDPoPOptions?.dPoPSigningAlgValuesSupported && createDPoPOptions?.dPoPSigningAlgValuesSupported.length > 0) {

if (createDPoPOptions?.dPoPSigningAlgValuesSupported && createDPoPOptions.dPoPSigningAlgValuesSupported.length > 0) {

packages/client/lib/AccessTokenClient.ts

packages/client/lib/AccessTokenClientV1_0_11.ts

nklomp · 2024-07-25T13:49:39Z

packages/common/lib/functions/DPoP.ts

+  }
+
+  // The DPoP HTTP request header field value is a single and well-formed JWT.
+  const dPoPHeader = jwtDecode<JwtHeader & Partial<DPoPJwtHeaderProps>>(dpop, { header: true });


We have a parseJWT that does both. I guess we could change that function to accept some generics for header and payload, like the original functions do, with defaults for JwtHeader and JwtPayload

nklomp · 2024-07-25T13:53:46Z

packages/common/lib/functions/DPoP.ts

+  }
+
+  // Validate iat claim
+  const now = epochTime();


It is probably wise to add an small negative offset option to the function. You will typically use this in time based properties in JWTs, like for instance iat. Since there could be small offsets in time between your computer and the remote computer, we typically subtract a few seconds from the iat, because otherwise you might run into a JWT not valid yet type of situation if the remote machine is slightly lagging behind

nklomp · 2024-07-25T13:56:27Z

packages/common/lib/types/ServerMetadata.ts

@@ -27,7 +27,8 @@ export interface AuthorizationServerMetadata {
  // Note that the presence of pushed_authorization_request_endpoint is sufficient for a client to determine that it may use the PAR flow. A request_uri value obtained from the PAR endpoint is usable at the authorization endpoint regardless of other authorization server metadata such as request_uri_parameter_supported or require_request_uri_registration
  require_pushed_authorization_requests?: boolean; // Boolean parameter indicating whether Indicates whether the client is required to use PAR to initiate authorization. If omitted, the default value is false.
  'pre-authorized_grant_anonymous_access_supported': boolean; // OPTIONAL. A JSON Boolean indicating whether the issuer accepts a Token Request with a Pre-Authorized Code but without a client id. The default is false
-
+  // A JSON array containing a list of the JWS alg values (from the [IANA.JOSE.ALGS] registry) supported by the authorization server for DPoP proof JWTs.
+  dpop_signing_alg_values_supported?: string[];


Could we create a type for this and if exhaustive use the type, if not use the union of the type and string?

packages/issuer-rest/lib/IssuerTokenEndpoint.ts

packages/siop-oid4vp/lib/request-object/Payload.ts

TimoGlastra

Very nice martin!

TimoGlastra · 2024-07-25T13:49:45Z

packages/client/lib/AccessTokenClientV1_0_11.ts

-    return this.sendAuthCode(requestTokenURL, accessTokenRequest);
+    let dPoP: string | undefined;
+    if (createDPoPOptions?.dPoPSigningAlgValuesSupported && createDPoPOptions.dPoPSigningAlgValuesSupported.length > 0) {
+      const htu = requestTokenURL.split('?')[0].split('#')[0];


Can we extract this into an util?

TimoGlastra · 2024-07-25T13:50:59Z

packages/client/lib/AccessTokenClientV1_0_11.ts

+  private async sendAuthCode(
+    requestTokenURL: string,
+    accessTokenRequest: AccessTokenRequest,
+    options?: { dPoP?: string },


Should we allow passing customHeaders here as well? So if other extensions are needed we can add it over time without needing to add them? Not sure just thinking here

TimoGlastra · 2024-07-25T13:52:15Z

packages/client/lib/CredentialRequestClient.ts

@@ -89,6 +91,7 @@ export class CredentialRequestClient {
    context?: string[];
    format?: CredentialFormat | OID4VCICredentialFormat;
    subjectIssuance?: ExperimentalSubjectIssuance;
+    createDPoPOptions?: CreateDPoPClientOptions;


As this is inside an opts object, would just dpop as a key make more sense here / simpler?

IMO not the dpop to me is the proof itself not to options for creating it.

TimoGlastra · 2024-07-25T13:54:00Z

packages/client/lib/CredentialRequestClient.ts

+    let dPoP: string | undefined;
+    if (createDPoPOptions) {
+      const htu = credentialEndpoint.split('?')[0].split('#')[0];
+      dPoP = createDPoPOptions
+        ? await createDPoP({
+            ...createDPoPOptions,
+            jwtPayloadProps: { ...createDPoPOptions.jwtPayloadProps, htu, htm: 'POST', accessToken: requestToken },
+          })
+        : undefined;
+    }


I see this repeated quite often. Maybe we can do the htu extracting in the createDpop method. That way we can do:

const dpop = createDPoPOptions ? createDPoP() : undefined

TimoGlastra · 2024-07-25T13:55:22Z

packages/common/lib/functions/DPoP.ts

+/**
+ * Returns the current unix timestamp in seconds.
+ */
+function epochTime() {


This is not dpop specific

TimoGlastra · 2024-07-25T13:59:03Z

packages/common/lib/functions/JwtIssuer.ts

+}
+
+export interface JwtIssuerCustom extends JwtIssuerBase {
+  method: 'custom';


Is alg now optional in the jwt sign callback? As with custom it will still not be provided right?

no custom will not do anything

TimoGlastra · 2024-07-25T14:03:34Z

packages/issuer-rest/lib/IssuerTokenEndpoint.ts

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


Host is not always defined or the same url. Host can be 0.0.0.0 while you use a public url. I think this needs to be configured somewhere (i've bad issues with this in credo when deploying to a https server url)

Hmmm, I am not sure about it. I don't mind adding it as a parameter to the handleTokenRequest, but it looks weird to have it pass as a parameter.

@nklomp You also had a question about this.
Please doublecheck whether request.protocol isn't already including the trailing :
expressjs/express#4697 (comment)
https://stackoverflow.com/questions/10183291/how-to-get-the-full-url-in-express/10185427#10185427

For now, I changed the implementation so that the access token endpoint URL can be passed; if passed as an argument, we use it. Otherwise, we fall back to the above behavior. We can also enforce passing it. What do you think?

packages/issuer-rest/lib/IssuerTokenEndpoint.ts

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

packages/callback-example/package.json

nklomp

Thanks @auer-martin . This probably became a way bigger PR than imagined. 😂

A few small remarks. My only bigger issue remaining is the retryDPoP result object. I think we should create a small type for it and I am also not sure about the current handling of these methods when the dpop create options are set and the dpop methods return ok: false.

nklomp · 2024-07-30T21:07:54Z

packages/client/lib/AccessTokenClientV1_0_11.ts

+      nextDPoPNonce = successDPoPNonce ?? retryWithNonce.dpopNonce;
+    }
+
+    if (response.successBody && createDPoPOpts && createDPoPOpts && response.successBody.token_type !== 'DPoP') {


This for sure will ensures the DPoPOpts is present :D

nklomp · 2024-07-30T21:08:14Z

packages/client/lib/AccessTokenClientV1_0_11.ts

+      nextDPoPNonce = successDPoPNonce ?? retryWithNonce.dpopNonce;
+    }
+
+    if (response.successBody && createDPoPOpts && createDPoPOpts && response.successBody.token_type !== 'DPoP') {


Suggested change

if (response.successBody && createDPoPOpts && createDPoPOpts && response.successBody.token_type !== 'DPoP') {

if (response.successBody && createDPoPOpts && response.successBody.token_type !== 'DPoP') {

nklomp · 2024-07-30T21:19:19Z

packages/client/lib/functions/dpopUtil.ts

+      throw new Error('The DPoP nonce was not returned');
+    }
+
+    return { ok: true, dpopNonce: dPoPNonce } as const;


Can we create a small type for this? The ok is not immediately clear. I think having the http status code instead of ok would be nicer. The 401 is because we handle it nicely I guess, but it could also happen with a 400. So probably just using the http status is quite useful

I created a type. Hmmmmm, Not sure about the HTTP status return. Because then, at a higher level, you need to know which status codes trigger the retry.

The 401 is required for Resource Requests. https://www.rfc-editor.org/rfc/rfc9449.html#name-resource-server-provided-no

packages/client/lib/CredentialRequestClientV1_0_11.ts

packages/client/lib/AccessTokenClient.ts

nklomp · 2024-07-30T21:33:38Z

packages/common/lib/dpop/DPoP.ts

+  const { nowSkewedPast, nowSkewedFuture } = getNowSkewed(options.now);
+  if (
+    // iat claim is to far in the future
+    nowSkewedPast - (options.maxIatAgeInSeconds ?? 300) > dPoPPayload.iat ||


I would make the defaults a bit smaller. Systems being of 5 minutes would be very bad. Suggest to use 60

packages/common/lib/index.ts

nklomp · 2024-07-30T21:37:37Z

packages/common/lib/jwt/jwtUtils.ts

+ *
+ * See https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.5
+ */
+const DEFAULT_SKEW_TIME = 300;


See other remark. Suggest to lower this to 60

packages/issuer-rest/lib/IssuerTokenEndpoint.ts

auer-martin · 2024-07-31T07:25:37Z

Thanks @auer-martin . This probably became a way bigger PR than imagined. 😂

It was indeed a lot more work and took much longer than originally expected 🤣.
I still need to test the integration with the PID issuer with these changes. Probably some small nits still need to be fixed than we can merge the PR.

… develop

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

TimoGlastra

Tested the PR in our wallet and succesfully received credentials from the Funke PID issuer.

TimoGlastra · 2024-08-01T09:58:20Z

@nklomp leaving final review / approval to you

nklomp

LGTM. Will merge

auer-martin added 3 commits July 24, 2024 15:26

refactor: uuid and others to core

162f3e9

Merge branch 'develop' of github.com:Sphereon-Opensource/OID4VCI into…

981e262

… develop

feat: dpop support

9202667

auer-martin mentioned this pull request Jul 25, 2024

feat: dpop support openwallet-foundation/credo-ts#1966

Merged

auer-martin force-pushed the dpop branch from bda030e to a38e04f Compare July 25, 2024 13:37

fix: some imports

5034468

auer-martin force-pushed the dpop branch from a38e04f to 5034468 Compare July 25, 2024 13:43

nklomp requested changes Jul 25, 2024

View reviewed changes

TimoGlastra reviewed Jul 25, 2024

View reviewed changes

auer-martin added 8 commits July 29, 2024 12:03

feat: incorporate feedback part1

f30475a

feat: rename common to oid4vci-common

9efbf32

feat: create common package

d5b4b75

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

feat: address feedback part 2

01f6d4d

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

fix: prettier + eslint

57c7592

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

fix: some last nits

3c71599

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

fix: nits

1a54e69

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

fix: add some dpop unit tests

c24a898

auer-martin force-pushed the dpop branch from 9d37e5d to c24a898 Compare July 30, 2024 09:28

nklomp reviewed Jul 30, 2024

View reviewed changes

packages/callback-example/package.json Outdated Show resolved Hide resolved

fix: rename common to oid4vc-common

d89ac4f

auer-martin marked this pull request as ready for review July 30, 2024 09:44

auer-martin added 4 commits July 30, 2024 13:14

fix: varname

f92b2b9

fix: ensure correct token_type in response

668c53f

feat: add additional dpop retry mechanisms

a102854

fix: header casing

b696dba

nklomp requested changes Jul 30, 2024

View reviewed changes

auer-martin added 2 commits July 31, 2024 09:25

feat: incorporate feedback and fix tests

c7c6af4

Merge branch 'develop' of github.com:Sphereon-Opensource/OID4VCI into…

86d0423

… develop

TimoGlastra mentioned this pull request Jul 31, 2024

fix: jwk thumprint using crypto.subtle #134

Closed

TimoGlastra mentioned this pull request Jul 31, 2024

fix: jwk thumprint using crypto.subtle #135

Merged

TimoGlastra added 2 commits August 1, 2024 11:52

fix: headers for error response

4c8319e

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

fix: incorrect usage of errorBody

f25b7d6

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

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

nklomp approved these changes Aug 2, 2024

View reviewed changes

nklomp merged commit 130b3db into Sphereon-Opensource:develop Aug 2, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dpop support #131

feat: dpop support #131

auer-martin commented Jul 25, 2024

nklomp left a comment •

edited

Loading

nklomp Jul 25, 2024

nklomp Jul 25, 2024

nklomp Jul 25, 2024

nklomp Jul 25, 2024

TimoGlastra left a comment

TimoGlastra Jul 25, 2024

TimoGlastra Jul 25, 2024

TimoGlastra Jul 25, 2024

auer-martin Jul 29, 2024

TimoGlastra Jul 25, 2024

TimoGlastra Jul 25, 2024

TimoGlastra Jul 25, 2024

auer-martin Jul 29, 2024

TimoGlastra Jul 25, 2024

auer-martin Jul 29, 2024

nklomp left a comment

nklomp Jul 30, 2024

nklomp Jul 30, 2024

nklomp Jul 30, 2024

auer-martin Jul 31, 2024

nklomp Jul 30, 2024

nklomp Jul 30, 2024

auer-martin commented Jul 31, 2024 •

edited

Loading

TimoGlastra left a comment

TimoGlastra commented Aug 1, 2024

nklomp left a comment

	if (createDPoPOptions?.dPoPSigningAlgValuesSupported && createDPoPOptions?.dPoPSigningAlgValuesSupported.length > 0) {
	if (createDPoPOptions?.dPoPSigningAlgValuesSupported && createDPoPOptions.dPoPSigningAlgValuesSupported.length > 0) {

	if (response.successBody && createDPoPOpts && createDPoPOpts && response.successBody.token_type !== 'DPoP') {
	if (response.successBody && createDPoPOpts && response.successBody.token_type !== 'DPoP') {

feat: dpop support #131

feat: dpop support #131

Conversation

auer-martin commented Jul 25, 2024

nklomp left a comment • edited Loading

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 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

nklomp 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

auer-martin commented Jul 31, 2024 • edited Loading

TimoGlastra left a comment

Choose a reason for hiding this comment

TimoGlastra commented Aug 1, 2024

nklomp left a comment

Choose a reason for hiding this comment

nklomp left a comment •

edited

Loading

auer-martin commented Jul 31, 2024 •

edited

Loading