Demonstrating Proof of Possession #1361

dbfr3qs · 2024-01-25T00:32:26Z

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

…ess token has been issued

…der, UInt8Array and IndexedDB

… nonce support

…de true in signUpRequest

dbfr3qs · 2024-01-25T00:46:40Z

src/JsonService.ts

@@ -176,6 +187,15 @@ export class JsonService {
        if (!response.ok) {
            logger.error("Error from server:", json);
            if (json.error) {
+                if (json.error === "use_dpop_nonce" && dpopEnabled && response.headers.get("Dpop-Nonce")) {


The spec describes an optional nonce requirement whereby if a nonce is not supplied during a request, a bad request is returned by the resource server with a specific response body and the client is expected to generate a nonce and retry the request.

lets keep the throw of that error, if the IDP needs a Dpop-Nonce, we should already provide one in the first place...

Yeah, I'm okay with that but it's worth considering that the spec is a little unclear about when an authorization server should require a dpop_nonce (keep in mind that the nonce is generated server side):

Including a nonce value contributed by the authorization server in the DPoP proof MAY be used by authorization servers to limit the lifetime of DPoP proofs. The server determines when to issue a new DPoP nonce challenge and if it is needed, thereby requiring the use of the nonce value in subsequent DPoP proofs. The logic through which the server makes that determination is out of scope of this document.

but it's quite clear about what to do when no nonce is supplied:

An authorization server MAY supply a nonce value to be included by the client in DPoP proofs sent. In this case, the authorization server responds to requests that do not include a nonce with an HTTP 400 (Bad Request) error response per Section 5.2 of [RFC6749] using use_dpop_nonce as the error code value. The authorization server includes a DPoP-Nonce HTTP header in the response supplying a nonce value to be used when sending the subsequent request. Nonce values MUST be unpredictable. This same error code is used when supplying a new nonce value when there was a nonce mismatch. The client will typically retry the request with the new nonce value supplied upon receiving a use_dpop_nonce error with an accompanying nonce value.

This is how it's implemented in IdentityServer for example - the nonce header isn't returned anywhere in the flow, prior to responding with a 400 bad request if the drop_nonce header doesn't exist. This is also how Okta does it. So, happy to remove it, but I suspect what is here will become the norm if it hasn't already.

However, you did get me thinking and I realised I did miss something out. Once the nonce is received, it should actually be saved by the client and re-used until another error response is returned. I could add this behaviour if you'd like, or just remove it entirely as you've suggested. Thoughts?

Hey @pamapa I have removed the authorization server supplied nonce support for the time being. I think it would be better to do it in a separate PR if that's okay (it is optional in the spec)? This PR is large and it was becoming messy.

src/DPoPService.ts

dbfr3qs · 2024-01-25T00:53:13Z

src/User.ts

@@ -124,4 +125,8 @@ export class User {
        Logger.createStatic("User", "fromStorageString");
        return new User(JSON.parse(storageString));
    }
+
+    public async dpopProof(url: string, httpMethod?: string): Promise<string> {


It feels a bit weird exposing this here: there needs to be a way of allowing consumers to generate a DPoP proof and then attaching that to authenticated requests.

But it seemed like the easiest way from the pov of the react-oidc-client library... for example https://github.com/dbfr3qs/react-oidc-context/blob/217d6d18109fc82526d7abc4015a98ec359fe79b/example/index.tsx#L29C13-L39C16

dbfr3qs · 2024-01-25T00:56:45Z

jest-environment-jsdom.cjs

+    value: true
+});
+
+class JSDOMEnvironment extends $JSDOMEnvironment {


jsdom doesn't support some apis so I had to work around that.
jsdom/jsdom#2524

Also some issues with jest and esm modules...
https://stackoverflow.com/questions/76608600/jest-tests-are-failing-because-of-an-unknown-unexpected-token-export

src/JsonService.ts

src/DPoPService.ts

codecov · 2024-02-14T16:07:48Z

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.

❗ 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 ⚠️
src/DPoPService.ts	81.08%	5 Missing and 2 partials ⚠️
src/User.ts	50.00%	1 Missing ⚠️

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%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pamapa · 2024-02-14T16:13:52Z

src/UserManager.ts

@@ -15,6 +15,7 @@ import type { SignoutResponse } from "./SignoutResponse";
 import type { MetadataService } from "./MetadataService";
 import { RefreshState } from "./RefreshState";
 import type { SigninResponse } from "./SigninResponse";
+import { del } from "idb-keyval";


The idb-keyval must be replaced with the existing store user infrastructure. There where we store the user/session we can store the dpop value too...

No worries, I am working on removing that dependency now.

However, we must use indexedDB to store non extractable crypto key objects. We cannot store these in a plain text storage mechanism, such as local/session storage. If we store key material securely in IndexedDB, it provides some measure of protection against leaking the crypto key material used to generate the DPoP proofs. If we chose to make the key material extractable and stored in plane text, then we largely defeat the purpose of DPoP in the browser as the DPoP keys can be exfiltrated out of the browser alongside any tokens.

See
https://blog.engelke.com/2014/09/19/saving-cryptographic-keys-in-the-browser/
https://stackoverflow.com/a/49479890
https://crypto.stackexchange.com/a/52488
https://developer.mozilla.org/en-US/docs/Web/API/CryptoKey/extractable

And the ietf recommendation https://www.ietf.org/archive/id/draft-ietf-oauth-browser-based-apps-16.html#section-6.3.3.2.2

Instead I will provide a similar interface to the WebStorageStageStore, though wrapping IndexedDB and specifically for storing CryptoKeyPairs. This will remove the dependency on ida-keyval, but the key material will still be stored in IndexedDB.

Does this make sense?

@pamapa I have removed the idv-keyval package as a dependency, and added replacement code to manage indexedDB transactions. As mentioned above, we must use indexedDB to store non extractable crypto keys, otherwise there's not a lot of point in adding a DPoP implementation to pure frontend applications.

…raHeaders

dbfr3qs · 2024-03-06T21:39:12Z

@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 · 2024-03-14T08:06:56Z

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 · 2024-03-14T18:45:56Z

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.

…ettings

dbfr3qs · 2024-03-20T23:01:52Z

@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 · 2024-03-21T10:31:22Z

src/OidcClient.ts

@@ -24,6 +24,7 @@ export interface CreateSigninRequestArgs
    redirect_uri?: string;
    response_type?: string;
    scope?: string;
+    dpopJkt? : string;


i guess this can be part of extra header you add just below or no extra header and this parameter?

I don't think so - it's not sent as a header, it's a url request parameter in the sign up request (to /authorize) so that the AS has a way of knowing that the subsequent /token request comes from the same entity.

The extraHeaders, at this point, are exclusively used to send along the DPoP proof which is used when making said subsequent requests for tokens.

Unless I'm misunderstanding you?

pamapa · 2024-03-21T10:44:50Z

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 · 2024-03-21T19:21:48Z

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 · 2024-04-03T23:00:18Z

Closing this in favour of #1461

Chris Keogh added 21 commits November 30, 2023 20:04

Add initial DPoPService to create proofs when exchanging a secret

dd5adc6

Expose method on User class to access dpop proof after dpop bound acc…

4bc0be5

…ess token has been issued

Persist dpop CryptoKeyPair to indexeddb

460da52

refactor dpop service

d8d9ad6

add DPoPService tests and setup jsdom to handle TextEncoder, TextDeco…

542faa0

…der, UInt8Array and IndexedDB

add optional dpop boolean to OidcClientSettings

5d832fb

add test for ClientSettingsStore.dpop

9c5dd61

Correct DPoP header name

ba16e05

Throw error if dpop keys are not found in the store, add tests

ae148e5

Remove dpop keys from storage on signout if dpop enabled

162ce9f

add test from removing dpop keys when removing user

34a6407

rename dpop setting to dpopEnabled

1f4320a

add test for invalid CryptoKeyPair in indexeddb, add failing test for…

8838369

… nonce support

rename dpop setting to dpopEnabled in oidcClientSettings

1dbb762

Add first cut support for bind DPoP key to auth code

323aa5c

Refactor dpopservice

d8840fa

make DPoPService static

fbe0b32

add DPoPSettings interface

c9c3ee1

Include dpop key thumbprint if dpop enabled and bind_authorization_co…

9e4f76e

…de true in signUpRequest

Minor clean up

7d7c7e8

Fix merge conflicts with main

13cb759

dbfr3qs commented Jan 25, 2024

View reviewed changes

src/DPoPService.ts Show resolved Hide resolved

dbfr3qs commented Jan 25, 2024

View reviewed changes

dbfr3qs mentioned this pull request Jan 25, 2024

Demonstrating Proof Of Possession (DPoP) #1360

Open

pamapa reviewed Jan 30, 2024

View reviewed changes

src/JsonService.ts Show resolved Hide resolved

pamapa reviewed Jan 30, 2024

View reviewed changes

src/DPoPService.ts Outdated Show resolved Hide resolved

Chris Keogh added 2 commits January 31, 2024 20:24

remove jose jwt creation and base64url encode

9b2d100

remove jose entirely for DPoPService

47d7d40

Chris Keogh added 3 commits February 7, 2024 19:46

Move functions into relevant util module and fix jwk serialisation error

9036ad0

Move jose to dev deps for now

50900fd

Move jwkthumbprint code and add test

7085388

pamapa reviewed Feb 14, 2024

View reviewed changes

Chris Keogh added 8 commits February 19, 2024 20:19

Create IndexedDbStore class

4c5d727

remove idb-keyval dep entirely

fe84f3c

add more tests for IndexedDbCryptoKeyPairStore

78891f0

change indexeddb db and store names

45302fb

Move dpopEnabdle out of json service and extend postFormOpts with ext…

d78885f

…raHeaders

Store dpop nonce in session storage when received from authz server

92f5458

remove nonce support for now, will do this separately in another PR

d5067a6

Merge branch 'main' into DPoP

e71ddc5

Chris Keogh added 4 commits March 21, 2024 09:08

Use Opts interface for generateDPoPProof parameters

88af397

pull up DPoP into UserManager

04b1a97

Pull up DPoP settings out of OidcClientSettings and into UserManagerS…

284b6ca

…ettings

refactor JsonService extraHeaders test

1e019ea

pamapa reviewed Mar 21, 2024

View reviewed changes

This was referenced Apr 2, 2024

Add extra headers #1457

Merged

Add demonstrating proof of possession #1461

Merged

dbfr3qs closed this Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demonstrating Proof of Possession #1361

Demonstrating Proof of Possession #1361

dbfr3qs commented Jan 25, 2024 •

edited

Loading

dbfr3qs Jan 25, 2024

pamapa Jan 30, 2024

dbfr3qs Jan 30, 2024 •

edited

Loading

dbfr3qs Mar 6, 2024

dbfr3qs Jan 25, 2024

dbfr3qs Jan 25, 2024

codecov bot commented Feb 14, 2024 •

edited

Loading

pamapa Feb 14, 2024 •

edited

Loading

dbfr3qs Feb 20, 2024 •

edited

Loading

dbfr3qs Mar 6, 2024

dbfr3qs commented Mar 6, 2024

pamapa commented Mar 14, 2024

dbfr3qs commented Mar 14, 2024 •

edited

Loading

dbfr3qs commented Mar 20, 2024

pamapa Mar 21, 2024 •

edited

Loading

dbfr3qs Mar 21, 2024

pamapa commented Mar 21, 2024

dbfr3qs commented Mar 21, 2024

dbfr3qs commented Apr 3, 2024

Demonstrating Proof of Possession #1361

Demonstrating Proof of Possession #1361

Conversation

dbfr3qs commented Jan 25, 2024 • edited Loading

Checklist

dbfr3qs Jan 25, 2024

Choose a reason for hiding this comment

pamapa Jan 30, 2024

Choose a reason for hiding this comment

dbfr3qs Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

dbfr3qs Mar 6, 2024

Choose a reason for hiding this comment

dbfr3qs Jan 25, 2024

Choose a reason for hiding this comment

dbfr3qs Jan 25, 2024

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2024 • edited Loading

Codecov Report

pamapa Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

dbfr3qs Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

dbfr3qs Mar 6, 2024

Choose a reason for hiding this comment

dbfr3qs commented Mar 6, 2024

pamapa commented Mar 14, 2024

dbfr3qs commented Mar 14, 2024 • edited Loading

dbfr3qs commented Mar 20, 2024

pamapa Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

dbfr3qs Mar 21, 2024

Choose a reason for hiding this comment

pamapa commented Mar 21, 2024

dbfr3qs commented Mar 21, 2024

dbfr3qs commented Apr 3, 2024

dbfr3qs commented Jan 25, 2024 •

edited

Loading

dbfr3qs Jan 30, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

pamapa Feb 14, 2024 •

edited

Loading

dbfr3qs Feb 20, 2024 •

edited

Loading

dbfr3qs commented Mar 14, 2024 •

edited

Loading

pamapa Mar 21, 2024 •

edited

Loading