Add demonstrating proof of possession

[dbfr3qs]

This is the second part of splitting up #1361 - hopefully it is easier to review.

Addresses/highlights #1360

As per the issue, this is an implementation of Demonstrating Proof of Possession (DPoP) to constrain access/refresh tokens to devices.

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 have also left out support for Authorization Server supplied nonces, which I would like to address in a separate PR in the future (this is optional in the spec).

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 UserManagerSettings and introduces a new service - the DPoPService. This is off by default.
• It makes use of IndexedDb, which allows storing non extractable key material used to generate the DPoPProofs. This means we can still use the stored keys to generate DPoP proofs, even though the key values themselves can't be accessed by the browser directly. This is critical to this functionality and without it DPoP is pointless in the browser context as the keys can just be exfiltrated by a bad actor alongside any tokens if stored in plain text. Storing key material this way prevents an attack where a bad actor may be able to exfiltrate tokens, because the actor cannot generate the required proof outside of the browser/session context.

Checklist

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

[codecov]

Codecov Report

Attention: Patch coverage is 85.21739% with 17 lines in your changes missing coverage. Please review.

Project coverage is 81.24%. Comparing base (f0ad76e) to head (12ac53f).
Report is 79 commits behind head on main.

❗ Current head 12ac53f differs from pull request most recent head 7764078

Please upload reports for the commit 7764078 to get more accurate results.

Files	Patch %	Lines
src/UserManager.ts	66.66%	6 Missing and 2 partials ⚠️
src/utils/CryptoUtils.ts	78.94%	6 Missing and 2 partials ⚠️
src/OidcClientSettings.ts	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1461      +/-   ##
==========================================
+ Coverage   80.53%   81.24%   +0.71%     
==========================================
  Files          45       46       +1     
  Lines        1731     1850     +119     
  Branches      344      366      +22     
==========================================
+ Hits         1394     1503     +109     
- Misses        299      306       +7     
- Partials       38       41       +3     

Flag	Coverage Δ
unittests	81.24% <85.21%> (+0.71%)	⬆️

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.

[dbfr3qs]

Will work on getting the code coverage up over the next week or so.

[pamapa]

I am thinking about if it makes sense to add an extended userStore for dpop. Within that store we could handle/ isolate stuff like await IndexedDbCryptoKeyPairStore.remove("oidc.dpop");. We might even merge this new userStore class with IndexedDbCryptoKeyPairStore...

export class DPopStorageStateStore extends WebStorageStateStore {
  // ... handles indexeddb stuff and dpop related things needed to be done differently than without dpop
}
```

[dbfr3qs]

I am thinking about if it makes sense to add an extended userStore for dpop. Within that store we could handle/ isolate stuff like await IndexedDbCryptoKeyPairStore.remove("oidc.dpop");. We might even merge this new userStore class with IndexedDbCryptoKeyPairStore...

export class DPopStorageStateStore extends WebStorageStateStore {
  // ... handles indexeddb stuff and dpop related things needed to be done differently than without dpop
}

Okay, I had a Quick Look...

When you say extend the WebStorageStateStore class, what benefit do we get from this?

Because we are dealing with the CryptoKeyPair type when dealing with IndexedDb (this is the type required to be able to store the key material as non extractable and also the type/supertype used by the crypto.subtle library), we can't override the set, get, remove and getAllKeys methods on the class because the StateStore interface specifies the types <string | null>.

So adding additional methods, such as getDPoPKeyPair is fine, but we don't really need the methods set, get, etc that are exposed by WebStorageStateStore because we don't need the session/local storage for anything DPoP currently (we might do if/when we add support for server side nonces though).

We could change the StateStore interface to allow for CyptoKeyPair, e.g. something like:

export interface StateStore<T extends string | null | CryptoKeyPair> {
    set(key: string, value: T): Promise<void>;
    get(key: string): Promise<T>;
    remove(key: string): Promise<T>;
    getAllKeys(): Promise<string[]>;
}

And implement that interface directly e.g. export class DPoPStorageStateStore implements StateStore<CryptoKeyPair | null> but that would mean making slight changes to the classes that already implement it as well I think (to specify the types). If we did this and decided to add the ability to also store DPoP nonces as strings in local/session storage through the same class, we could then widen the accepted types to <string | null | CryptoKeyPair> and add logic to put nonces in local/session storage and key material in IndexedDB.

Anyway, I suppose I'm not fully clear on what you are after as far as extending the WebStorageStateStore class.

[pamapa]

Anyway, I suppose I'm not fully clear on what you are after as far as extending the WebStorageStateStore class.

I was more thinking about this:

your MR:

public async removeUser(): Promise<void> {
        const logger = this._logger.create("removeUser");
        await this.storeUser(null);
        logger.info("user removed from storage");
        if (this.settings.dpopSettings.enabled) {
            await IndexedDbCryptoKeyPairStore.remove("oidc.dpop");
            logger.debug("removed dpop cyptokeys from storage");
        }
        await this._events.unload();
    }

idea:

public async removeUser(): Promise<void> {
        const logger = this._logger.create("removeUser");
        await this.storeUser(null);
        logger.info("user removed from storage");
        /* not needed, as its part of the new `userStore` and done automatically removed when calling "await this.storeUser(null);"
        if (this.settings.dpopSettings.enabled) {
            await IndexedDbCryptoKeyPairStore.remove("oidc.dpop");
            logger.debug("removed dpop cyptokeys from storage");
        }*/
        await this._events.unload();
    }

// untested pseudo code only
export class DPopStorageStateStore extends WebStorageStateStore {
   public async remove(key: string): Promise<string | null> {
     const ret = super.remove(key);
     await IndexedDbCryptoKeyPairStore.remove("oidc.dpop");
     return ret;
   }
   ...
}

[dbfr3qs]

Anyway, I suppose I'm not fully clear on what you are after as far as extending the WebStorageStateStore class.

I was more thinking about this:

your MR:

public async removeUser(): Promise<void> {
        const logger = this._logger.create("removeUser");
        await this.storeUser(null);
        logger.info("user removed from storage");
        if (this.settings.dpopSettings.enabled) {
            await IndexedDbCryptoKeyPairStore.remove("oidc.dpop");
            logger.debug("removed dpop cyptokeys from storage");
        }
        await this._events.unload();
    }

idea:

public async removeUser(): Promise<void> {
        const logger = this._logger.create("removeUser");
        await this.storeUser(null);
        logger.info("user removed from storage");
        /* not needed, as its part of the new `userStore` and done automatically removed when calling "await this.storeUser(null);"
        if (this.settings.dpopSettings.enabled) {
            await IndexedDbCryptoKeyPairStore.remove("oidc.dpop");
            logger.debug("removed dpop cyptokeys from storage");
        }*/
        await this._events.unload();
    }

// untested pseudo code only
export class DPopStorageStateStore extends WebStorageStateStore {
   public async remove(key: string): Promise<string | null> {
     const ret = super.remove(key);
     await IndexedDbCryptoKeyPairStore.remove("oidc.dpop");
     return ret;
   }
   ...
}

Okay, yep - and would you like to try and apply the same idea to all the methods on the WebStorateStateStore class - e.g. set, get, getAllKeys such that it can handle DPoP related data as well as the usual user attributes stored in local/session storage?

[pamapa]

Okay, yep - and would you like to try and apply the same idea to all the methods on the WebStorateStateStore class - e.g. set, get, getAllKeys such that it can handle DPoP related data as well as the usual user attributes stored in local/session storage?

if possible yes. the idea is to simplify the dpop code paths and the not dpop code paths.
Stuff like:

• User.ts (BTW: not used anywhere)

public async dpopProof(url: string, httpMethod?: string): Promise {
return await DPoPService.generateDPoPProof({ url, accessToken: this.access_token, httpMethod: httpMethod } );
}

- it contains directly code of `export class IndexedDbCryptoKeyPairStore` or hold the only reference to that object.

[dbfr3qs]

Okay, yep - and would you like to try and apply the same idea to all the methods on the WebStorateStateStore class - e.g. set, get, getAllKeys such that it can handle DPoP related data as well as the usual user attributes stored in local/session storage?

if possible yes. the idea is to simplify the dpop code paths and the not dpop code paths. Stuff like:

• User.ts (BTW: not used anywhere)

public async dpopProof(url: string, httpMethod?: string): Promise { return await DPoPService.generateDPoPProof({ url, accessToken: this.access_token, httpMethod: httpMethod } ); }

- it contains directly code of `export class IndexedDbCryptoKeyPairStore` or hold the only reference to that object.

So I gave this a go but it's not possible to extend the WebStorageStateStore class with a DPoPStorageStateStore class without changing the types on the WebStorageState class (and the interface) to allow returning a CryptoKeyPair on the get method... which seems a bit gross, e.g.

public async get(key: string): Promise<string | null | CryptoKeyPair> {
        if (key === this.dpop_key) {
            return await IndexedDbCryptoKeyPairStore.get(this.dpop_key);
        } else {
            return await super.get(key);
        }
    }

doesn't work unless I change the signature of the parent WebStorageStateStore class to also return <string | null | CryptoKeyPair>

One option might be to alter the WebStorateStateStore to handle DPoP as well as what it already does (we'd still need to alter the StateStore interface).

What do you think?

[pamapa]

One option might be to alter the WebStorateStateStore to handle DPoP as well as what it already does (we'd still need to alter the StateStore interface).

Maybe by adding a "new/additional" get like getDpop(): Promise<null | CryptoKeyPair>

[dbfr3qs]

One option might be to alter the WebStorateStateStore to handle DPoP as well as what it already does (we'd still need to alter the StateStore interface).

Maybe by adding a "new/additional" get like getDpop(): Promise<null | CryptoKeyPair>

Ok, I have had a rough go at adding a DPoPStorageStateStore for you to look at. I altered the StateStore interface to accept the CryptoKeyPair type, hopefully that makes sense. Also added the additional getDPoP method and inject the instance of the store into the DPoPService now.

Have a look and let me know what you think. If you're happy with the approach, I will move the IndexedDBCryptoKeyPairStore logic directly into the DPoPStorageStateStore and incorporate any other feedback you may have.

I'll be travelling for the next few weeks so may not be able to do too much until May 👍

[pamapa]

Have a look and let me know what you think. If you're happy with the approach, I will move the IndexedDBCryptoKeyPairStore logic directly into the DPoPStorageStateStore and incorporate any other feedback you may have.

I'll be travelling for the next few weeks so may not be able to do too much until May 👍

I will have a deeper look at this in the next days. Thanks for keeping up on this.

[dbfr3qs]

Have a look and let me know what you think. If you're happy with the approach, I will move the IndexedDBCryptoKeyPairStore logic directly into the DPoPStorageStateStore and incorporate any other feedback you may have.
I'll be travelling for the next few weeks so may not be able to do too much until May 👍

I will have a deeper look at this in the next days. Thanks for keeping up on this.

Cool - I am back from travelling so can start working on this again when you have had a look 👍

[dbfr3qs]

Any thoughts on the latest commit @pamapa ? I am keen to progress this.

[pamapa]

Was busy the last weeks, I have checked out your branch and I am now looking at this code and have some thoughts and time in the future to work with you...

General design

• I am afraid of the additional code size for people who do not need DPoP, thus my first idea was to move everything into a sub class of UserManager, like DPoPUserManager, but when looking at the new code this does not seem feasible. The additional code is tight into the existing code.
• By design OidcClient is for the protocol and UserManager is for "user related" stuff. DPoP is mostly protocol stuff except the access token from the user.
• Thus maybe it really makes sense to have all but public async dpopProof( (access token needed), in the OidcClient class like you had in the beginning (sorry about that).

DPoPService:

• i am not so sure it is worth to have this service at all, at the end its a bunch of crypto utils and getting the keys.
• we could split the utils and getting the keys.
• crypto utils code could be added to the CryptoUtils and adding a parameter to pass the keyPair

DPoPStorageStateStore/IndexedDbCryptoKeyPairStore:

• when i look at the current (from me previously proposed idea) implementation of extending the WebStorageStateStore, its really hard to read and seems over complicated...
• at the end is probably simpler if we simply add an additional optional store like dpopStore.
• like this we can merge DPoPStorageStateStore & IndexedDbCryptoKeyPairStore into one and keep its as simple as possible
• if somebody needs a different implementation of dpopStore he can easily do so.

DPoPSettings:

• lets rename dpopSettings to dpop (it is already within the settings)
• drop enable property (if dpop is undefined its like false if not its enabled)
• add store

Something like:

let dpopJkt: string | undefined;
if (this.settings.dpop && this.settings.dpop.bind_authorization_code) {
  const keys = await this.settings.dpop.store.get();
  dpopJkt = await CryptoUtils.generateDPoPJkt({ keys });
}

[dbfr3qs]

Was busy the last weeks, I have checked out your branch and I am now looking at this code and have some thoughts and time in the future to work with you...

General design

• I am afraid of the additional code size for people who do not need DPoP, thus my first idea was to move everything into a sub class of UserManager, like DPoPUserManager, but when looking at the new code this does not seem feasible. The additional code is tight into the existing code.
• By design OidcClient is for the protocol and UserManager is for "user related" stuff. DPoP is mostly protocol stuff except the access token from the user.
• Thus maybe it really makes sense to have all but public async dpopProof( (access token needed), in the OidcClient class like you had in the beginning (sorry about that).

DPoPService:

• i am not so sure it is worth to have this service at all, at the end its a bunch of crypto utils and getting the keys.
• we could split the utils and getting the keys.
• crypto utils code could be added to the CryptoUtils and adding a parameter to pass the keyPair

DPoPStorageStateStore/IndexedDbCryptoKeyPairStore:

• when i look at the current (from me previously proposed idea) implementation of extending the WebStorageStateStore, its really hard to read and seems over complicated...
• at the end is probably simpler if we simply add an additional optional store like dpopStore.
• like this we can merge DPoPStorageStateStore & IndexedDbCryptoKeyPairStore into one and keep its as simple as possible
• if somebody needs a different implementation of dpopStore he can easily do so.

DPoPSettings:

• lets rename dpopSettings to dpop (it is already within the settings)
• drop enable property (if dpop is undefined its like false if not its enabled)
• add store

Something like:

let dpopJkt: string | undefined;
if (this.settings.dpop && this.settings.dpop.bind_authorization_code) {
  const keys = await this.settings.dpop.store.get();
  dpopJkt = await CryptoUtils.generateDPoPJkt({ keys });
}

Thank you for the detailed feedback @pamapa, that is excellent and exactly what I was looking for. At a glance, that all makes sense and I will update this PR accordingly over the next week.

One minor clarifying implementation question for you regarding the new dpopStore: does it matter if it implements the storageStage interface? It's minor, but will require extending the interface as I have already done (to handle the CryptoKeyPair type).

[pamapa]

One minor clarifying implementation question for you regarding the new dpopStore: does it matter if it implements the storageStage interface? It's minor, but will require extending the interface as I have already done (to handle the CryptoKeyPair type).

If we create a new storage type we have some flexibility. I guess when we decouple from StateStore its getting simpler for existing string cases and CryptoKeyPair.
My hope is that when someone does not use dpop, that store is removed when doing tree-shaking in production builds, as its not used/referenced directly. Its only passed via settings.

[pamapa]

@pamapa I believe I have integrated most, if not all, of your most recent suggestions. I have also rebased with main. Please let me know your thoughts 🙏

Looks awesome, its very close to be merged. I have found some little style stuff (see above & below). Thanks a lot for being patient and taking care!

Question:

• in OidcClientSettings.ts i see this.dpop.store = new IndexedDbDPoPStore(); It is done now like the other stores. The difference is that the other stores are essential, where the dpop is not. When we would not create here a new object, for everybody who does not use dpop, the IndexedDbDPoPStore could be tree-shaked (can you verify this?). This would mean when you use dpop you simple must pass that store via config, which is very easy... What do you think?
• The above would also make this if (this.settings.dpop && this.settings.dpop.store) { into this if (this.settings.dpop) { as store is no longer an optional property in dpop...

[dbfr3qs]

@pamapa I believe I have integrated most, if not all, of your most recent suggestions. I have also rebased with main. Please let me know your thoughts 🙏

Looks awesome, its very close to be merged. I have found some little style stuff (see above & below). Thanks a lot for being patient and taking care!

Question:

• in OidcClientSettings.ts i see this.dpop.store = new IndexedDbDPoPStore(); It is done now like the other stores. The difference is that the other stores are essential, where the dpop is not. When we would not create here a new object, for everybody who does not use dpop, the IndexedDbDPoPStore could be tree-shaked (can you verify this?). This would mean when you use dpop you simple must pass that store via config, which is very easy... What do you think?
• The above would also make this if (this.settings.dpop && this.settings.dpop.store) { into this if (this.settings.dpop) { as store is no longer an optional property in dpop...

I see what you are saying: we would just need to be clear that an implementation of the dpopStore must be supplied, and that we provide an implementation that makes use of IndexDb as a default. I don't have a problem with that really, and you are right that in its current form the store is not tree shaked and adds about 1kb:

As far as overall build size goes, there is a 5kb difference between master and this branch when I bundle it
into a sample application using Parcel.

EDIT: I can confirm that changing the dpopStore to a non optional configuration parameter when dpop is enabled does allow the IndexedDbDPoPStore to be tree shaked. Do you think we should add a check that the store is actually configured when drop is enabled, and throw an exception if not?

I've also fixed all the style issues you high lighted. Would like me to add some documentation regarding the feature and how to use it to this PR?

[pamapa]

EDIT: I can confirm that changing the dpopStore to a non optional configuration parameter when dpop is enabled does allow the IndexedDbDPoPStore to be tree shaked. Do you think we should add a check that the store is actually configured when drop is enabled, and throw an exception if not?

We can explicitly throw an exception if the store is undefined, despite the type would enforce to not be undefined. Maybe hidden in the settings class...

I've also fixed all the style issues you high lighted. Would like me to add some documentation regarding the feature and how to use it to this PR?

Would be nice if you could add a new section in the docs/index.md. This can also be done in a next MR when this one has been merged...

[dbfr3qs]

EDIT: I can confirm that changing the dpopStore to a non optional configuration parameter when dpop is enabled does allow the IndexedDbDPoPStore to be tree shaked. Do you think we should add a check that the store is actually configured when drop is enabled, and throw an exception if not?

We can explicitly throw an exception if the store is undefined, despite the type would enforce to not be undefined. Maybe hidden in the settings class...

I've also fixed all the style issues you high lighted. Would like me to add some documentation regarding the feature and how to use it to this PR?

Would be nice if you could add a new section in the docs/index.md. This can also be done in a next MR when this one has been merged...

No problem, happy to do the docs in a different PR. For now I have annotated the IndexedDbDPoPStore as @public with a short description.

I have also updated with main and added a check in OidcSettings to throw an exception if drop is configured without a store.

[pamapa]

Thanks for contributing!

[psgfaa]

Thank you for the contribution. I noticed in your comments you removed the nonce option. We implement it here within our organization. Is it possible you could add the nonce back in? We get past the DPoP Proof but then are warned we need to supply the nonce value.

Can you provide me some help or a workaround? Any help is greatly appreciated and I do thank you for the contribution.

[dbfr3qs]

Thank you for the contribution. I noticed in your comments you removed the nonce option. We implement it here within our organization. Is it possible you could add the nonce back in? We get past the DPoP Proof but then are warned we need to supply the nonce value.

Can you provide me some help or a workaround? Any help is greatly appreciated and I do thank you for the contribution.

Hi there, yes I originally added a rough implementation but opted to remove it in favour of adding it in a separate PR (the PR was getting quite large, there was disagreement regarding the implementation and it is optional in the spec).

I will open another PR next week. Perhaps you would be willing to help test and review it?

[psgfaa]

Thank you for the contribution. I noticed in your comments you removed the nonce option. We implement it here within our organization. Is it possible you could add the nonce back in? We get past the DPoP Proof but then are warned we need to supply the nonce value.
Can you provide me some help or a workaround? Any help is greatly appreciated and I do thank you for the contribution.

Hi there, yes I originally added a rough implementation but opted to remove it in favour of adding it in a separate PR (the PR was getting quite large, there was disagreement regarding the implementation and it is optional in the spec).

I will open another PR next week. Perhaps you would be willing to help test and review it?

Thank you once again, great work! I found your oidc-client-context and was able to get it working in my code. But got stuck on the DPoP nonce requirement.

Yes absolutely I can test it and help anyway I can. Again, appreciate your work and the author of the library - great work !

For reference we are using OIDC+PKCE, with DPoP Proof and Nonce. From the auth0 and okta articles I was reading on the implementation while the Nonce is mentioned as optional I suspect most providers will use it to enhance security.

[dbfr3qs]

@psgfaa please take a look at #1569 🙏