Demonstrating Proof of Possession

[dbfr3qs]

Addresses/highlights #1360

As per the issue, this is an example of a potential implementation of Demonstrating Proof of Possession (DPoP) to constrain access/refresh tokens to devices. I'm keen on feedback/collaboration if this is a thing that is deemed worth adding to the library...

As mentioned in the issue, this is mostly spec complete - though I have left out some parts that I don't think are appropriate to the oidc-client-ts use case (such as Pushed Authorisation Requests).

I've used the react-odic-client library example app and npm linked it to my local version of this PR. I used a fork of IdentityServer's DPoP examples to develop and test against, if you are interested in trying it out for yourself.

Things of note

• This adds a new set of options specifically for DPoP to the OidcClient and introduces a new service - the DPoPService.
• What's the general feeling in regards to adding dependencies? I ask this because...
  • I've included two dependencies to do this: Jose and idb-keyval. Jose allows for generating JWTs at runtime and idk-keyval wraps the native IndexedDB api with a more intuitive promise based api. Both of these libraries have no external dependencies.
  • I had to get a little bit funky in order to satisfy these dependencies in the tests.
  • It will increase the size of the overall bundle.
• In the case of crypto-js, I see that it is no longer maintained. I've used the browser native SubtleCrypto api to generate key material here in any case, because I wanted to take advantage of the extractable property when generating key pairs.

I'm not super experienced with Typescript, apologies if I've made rookie mistakes. Please give me feedback.

Checklist

• [ ] This PR makes changes to the public API
• [ ] I have included links for closing relevant issue numbers

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 81.81818% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 80.56%. Comparing base (fc1d31c) to head (7085388). Report is 19 commits behind head on main.

:exclamation: Current head 7085388 differs from pull request most recent head 1e019ea. Consider uploading reports for the commit 1e019ea to get more accurate results

Files	Patch %	Lines
src/utils/CryptoUtils.ts	52.94%	7 Missing and 1 partial :warning:
src/DPoPService.ts	81.08%	5 Missing and 2 partials :warning:
src/User.ts	50.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1361      +/-   ##
==========================================
+ Coverage   80.53%   80.56%   +0.03%     
==========================================
  Files          45       46       +1     
  Lines        1731     1816      +85     
  Branches      344      358      +14     
==========================================
+ Hits         1394     1463      +69     
- Misses        299      312      +13     
- Partials       38       41       +3     

Flag	Coverage Δ
unittests	80.56% <81.81%> (+0.03%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[dbfr3qs]

@pamapa I think this is ready for a proper review as it is essentially code complete, apart from optional AS supplied nonce support which I think should be done separately at a later date. Please take a look and let me know what else you'd like to change, or any questions you may have as I know it's quite a large PR.

[pamapa]

Please take a look and let me know what else you'd like to change, or any questions you may have as I know it's quite a large PR.

It is still very invasive and i am not convinced all new code is at the correct place.

• e.g. TokenClient.ts does not need extraHeaders["DPoP"] = ..., but simply support the extraHeaders parameter too

At the end probably (i am not 100% sure) all/most code adaptions, which are DPoP related end up in the UserManager class.

Maybe it makes sense in a first step to only extend this library in a generic way (like you did with extraHeaders), such that you can inherit e.g. the UserManager class yourself and are able to use this feature.

[dbfr3qs]

Please take a look and let me know what else you'd like to change, or any questions you may have as I know it's quite a large PR.

It is still very invasive and i am not convinced all new code is at the correct place.

• e.g. TokenClient.ts does not need extraHeaders["DPoP"] = ..., but simply support the extraHeaders parameter too

At the end probably (i am not 100% sure) all/most code adaptions, which are DPoP related end up in the UserManager class.

Maybe it makes sense in a first step to only extend this library in a generic way (like you did with extraHeaders), such that you can inherit e.g. the UserManager class yourself and are able to use this feature.

No problems - that all makes sense 👍 Will give it a go and see where we end up.

[dbfr3qs]

@pamapa is this more in line with what you were thinking?

I have pulled everything up into the UserManager. DPoP related functionality is then passed through as either an extraHeader (the DPoP header) or as a parameter (dpopJkt which is added as a url param during sign in to bind the DPoP proof to the authorization code).

[pamapa]

The changes go in the right direction, thanks for being patience.

I guess it makes sense to have a separate merge request with stuff you need, but does not contain DPoP, this way you have progress and its easier to review:

• JsonService.ts
• TokenClient.ts
• ResponseValidator.ts
• typos : recommend -> recommended

[dbfr3qs]

The changes go in the right direction, thanks for being patience.

I guess it makes sense to have a separate merge request with stuff you need, but does not contain DPoP, this way you have progress and its easier to review:

• JsonService.ts
• TokenClient.ts
• ResponseValidator.ts
• typos : recommend -> recommended

That's a great idea - I can do that.

[dbfr3qs]

Closing this in favour of #1461