Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dpop nonce support #1569

Merged
merged 9 commits into from
Jul 9, 2024
Merged

Add dpop nonce support #1569

merged 9 commits into from
Jul 9, 2024

Conversation

dbfr3qs
Copy link
Contributor

@dbfr3qs dbfr3qs commented Jul 2, 2024

This is a first cut aimed at adding DPoP nonce support. I initially removed this feature in the original PR and this is a follow up.

This is very experimental and open to change. @pamapa I would really appreciate your thoughts as to how this could be done more cleanly 🙏🏼

The main functionality is supporting Authorization Server Provided Nonces.

According to the spec, if/when an AS requires the use of a nonce and a token request is made without a nonce claim within the DPoP proof JWT, the AS responds with a specific 400 bad request that includes a response header dpop-nonce which contains the nonce value to be used. The client is then expected to regenerate its DPoP proof again with the included nonce, and then retry the token request.

Upon receiving the nonce, the client is expected to retry its token request using a DPoP proof including the supplied nonce value in the nonce claim of the DPoP proof. An example unencoded JWT payload of such a DPoP proof including a nonce is shown below.

e.g.

HTTP/1.1 400 Bad Request
 DPoP-Nonce: eyJ7S_zG.eyJH0-Z.HX4w-7v

 {
  "error": "use_dpop_nonce",
  "error_description":
    "Authorization server requires nonce in DPoP proof"
 }
[Figure 20](https://datatracker.ietf.org/doc/html/rfc9449#figure-20): [HTTP 400 Response to a Token Request without a Nonce](https://datatracker.ietf.org/doc/html/rfc9449#name-http-400-response-to-a-toke)
Other HTTP headers and JSON fields MAY also be included in the error response, but there MUST NOT be more than one DPoP-Nonce header.

Upon receiving the nonce, the client is expected to retry its token request using a DPoP proof including the supplied nonce value in the nonce claim of the DPoP proof. An example unencoded JWT payload of such a DPoP proof including a nonce is shown below.

 {
  "jti": "-BwC3ESc6acc2lTc",
  "htm": "POST",
  "htu": "https://server.example.com/token",
  "iat": 1562262616,
  "nonce": "eyJ7S_zG.eyJH0-Z.HX4w-7v"
 }

In order to accomodate this I have added a new abstraction DPoPState, which is an object that contains both the CryptoKeyPair and the optional nonce value. I have altered the DPoPStore interface to accomodate this as the nonce can be used repeatedly for token requests until the AS decides to invalidate.

Which brings me to my next point, the spec is quite open as to how an AS may provide a new nonce.

The authorization server MAY supply the new nonce in the same way that the initial one was supplied: by using a DPoP-Nonce HTTP header in the response. The DPoP-Nonce HTTP header field uses the nonce syntax defined in Section 8.1. Each time this happens, it requires an extra protocol round trip.

A more efficient manner of supplying a new nonce value is also defined by including a DPoP-Nonce HTTP header in the HTTP 200 (OK) response from the previous request. The client MUST use the new nonce value supplied for the next token request and for all subsequent token requests until the authorization server supplies a new nonce.

I attempted to try and add a way to handle a new nonce upon a 200 response but it was going to be messy and potentially involved altering the SigninResponse class, which didn't feel right. I looked at how IdentityServer issues new nonces and they just re-use the 400 bad request method. I think we should leave this out for the time being.

In regards to supporting Resource Server provided nonces, that is a lot simpler as the client should manage storing and renewing the resource provided nonce as it will be returned on a client initiated fetch request to the RS - it has nothing to do with oidc-client-ts. For now I have opted to provide a way to supply the nonce as an optional parameter when creating the DPoPProof on the user manager class. We can revisit if we want to offer a way for clients to store the RS provided nonce values and/or provide helpers to manage them, but this interaction falls outside the relationship between the client and the AS.

Checklist

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

@psgfaa
Copy link

psgfaa commented Jul 3, 2024

This looks great, I will try to implement and test it today

});
} catch (err) {
if (err instanceof ErrorDPoPNonce) {
extraHeaders!["DPoP"] = await this.getDpopProof(this.settings.dpop!.store, err.nonce);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not doing this before hand? I dislike doing exchangeRefreshToken twice...

Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 81.31%. Comparing base (f0ad76e) to head (7148462).
Report is 99 commits behind head on main.

Files Patch % Lines
src/UserManager.ts 44.44% 4 Missing and 1 partial ⚠️
src/OidcClient.ts 80.95% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1569      +/-   ##
==========================================
+ Coverage   80.53%   81.31%   +0.78%     
==========================================
  Files          45       48       +3     
  Lines        1731     1884     +153     
  Branches      344      371      +27     
==========================================
+ Hits         1394     1532     +138     
- Misses        299      309      +10     
- Partials       38       43       +5     
Flag Coverage Δ
unittests 81.31% <80.00%> (+0.78%) ⬆️

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.

@psgfaa
Copy link

psgfaa commented Jul 8, 2024

I apologize if this is the wrong place to put this comment. Chris I pulled your latest code (14 files changed), rebuilt the npm and put the rebuilt package in my application. Everything builds fine, however I still get the same message "The DPoP proof JWT header is missing.".

I do get a prompt and I do login, then I get that message. Do you have an example project that I can test this with? Thank you.

@dbfr3qs
Copy link
Contributor Author

dbfr3qs commented Jul 8, 2024

I apologize if this is the wrong place to put this comment. Chris I pulled your latest code (14 files changed), rebuilt the npm and put the rebuilt package in my application. Everything builds fine, however I still get the same message "The DPoP proof JWT header is missing.".

I do get a prompt and I do login, then I get that message. Do you have an example project that I can test this with? Thank you.

Yes, I have been using the react-oidc-context example app and the DPoP example from the IdentityServer Samples repo to test. You can try using these branches on my forks here and here. You will need to make sure that the react-oidc-context is referencing a local build of oidc-client-ts (of this branch) in the correct location on your local machine.

What IdP are you using?

@psgfaa
Copy link

psgfaa commented Jul 9, 2024

I apologize if this is the wrong place to put this comment. Chris I pulled your latest code (14 files changed), rebuilt the npm and put the rebuilt package in my application. Everything builds fine, however I still get the same message "The DPoP proof JWT header is missing.".
I do get a prompt and I do login, then I get that message. Do you have an example project that I can test this with? Thank you.

Yes, I have been using the react-oidc-context example app and the DPoP example from the IdentityServer Samples repo to test. You can try using these branches on my forks here and here. You will need to make sure that the react-oidc-context is referencing a local build of oidc-client-ts (of this branch) in the correct location on your local machine.

What IdP are you using?

Thank you. I made a little more progress. I need to review your oidc-client-context project/example further. I am connecting to a US Government Open ID Connect internal auth server. I was able to progress past the DPoP Nonce error message.

They turn the dev servers off around 7PM so I can't get the exact error message I was getting but I will post it in the morning. But it was something along the lines it could not retrieve the publicKey.

@pamapa pamapa merged commit 741d209 into authts:main Jul 9, 2024
3 of 4 checks passed
@pamapa
Copy link
Member

pamapa commented Jul 9, 2024

Thanks for contributing. As I am not using the dpop feature I really appreciate if you can help supporting this feature within this library. Maybe you have time to help supporting other features as well...

@psgfaa
Copy link

psgfaa commented Jul 9, 2024

Here is the error I am receiving (I am connecting to my site using HTTPS://). I tried in Chrome and Edge and it throws the same error. It does work in Firefox though and auth.IsAuthenticated shows TRUE.

I traced it back to line 190 from CryptoUtils.ts.

"Could not retrieve dpop keys from storage: Cannot read properties of undefined (reading 'publicKey')"

I am doing a auth.signinRedirect(), in UserManager.ts I see line 177 is where we do a generateDPoPJkt call

Here is my oidcConfig:

const oidcConfig = {
authority: 'https://mydomain.com/oauth2/default',
client_id: 'myclientid',
redirect_uri: 'https://mydomain/login/',
post_logout_redirect_uri: 'https://mydomain/logout',
onSigninCallback: onSigninCallback,
response_type: 'code',
scope: 'openid profile',
automaticSilentRenew: true,
monitorSession: true,
dpop: { bind_authorization_code: true, store: new IndexedDbDPoPStore() },
};

@dbfr3qs
Copy link
Contributor Author

dbfr3qs commented Jul 9, 2024

Here is the error I am receiving (I am connecting to my site using HTTPS://). I tried in Chrome and Edge and it throws the same error. It does work in Firefox though and auth.IsAuthenticated shows TRUE.

I traced it back to line 190 from CryptoUtils.ts.

"Could not retrieve dpop keys from storage: Cannot read properties of undefined (reading 'publicKey')"

I am doing a auth.signinRedirect(), in UserManager.ts I see line 177 is where we do a generateDPoPJkt call

Here is my oidcConfig:

const oidcConfig = { authority: 'https://mydomain.com/oauth2/default', client_id: 'myclientid', redirect_uri: 'https://mydomain/login/', post_logout_redirect_uri: 'https://mydomain/logout', onSigninCallback: onSigninCallback, response_type: 'code', scope: 'openid profile', automaticSilentRenew: true, monitorSession: true, dpop: { bind_authorization_code: true, store: new IndexedDbDPoPStore() }, };

Ok, can you check what is stored in indexedDb in your browser under oidc.dpop?

Here is an example of what you might see:

image

My first thought is that it's possible that you have an out of date object stored that may need to be cleared. The missing public key indicates that the object being retrieved is not in the shape we expect.

This change introduced a breaking change in that the object that we store for DPoP changed to include storing the nonce.

If the object in indexedDb looks different, delete it and try logging in again.

If this is not the case, would you mind starting an issue and we can work from there? This will allow any findings to be more discoverable for others in the future.

The other possibility may be that for some reason nothing is being written to indexedDb. If that is the case, can you tell me what browser you are using incl the version, any plugins, etc, and perhaps set up some tracing in the debugger to look at what happens when the dpopState is persisted?

Or it could be something else of course 😅 If so, create an issue as mentioned and we will figure it out.

@dbfr3qs
Copy link
Contributor Author

dbfr3qs commented Jul 9, 2024

Thanks for contributing. As I am not using the dpop feature I really appreciate if you can help supporting this feature within this library. Maybe you have time to help supporting other features as well...

Very happy to help. My next PR will document the DPoP feature and I am happy so support it.

@psgfaa
Copy link

psgfaa commented Jul 9, 2024

Thank you! I checked the IndexDB storage option in Chrome and it only shows IndexDB and no database. But oddly enough I tried the website/app and it works now. I tried in Edge and it did show the IndexDb>oidc>dpop and a key. I deleted the oidc database, retried it in Edge and it works correctly.

Not sure what was going on with Chrome regarding the IndexDb database not showing anything but all browsers work now. Thank you very much for your hard work.

If you need me to test anything let me know.

@dbfr3qs
Copy link
Contributor Author

dbfr3qs commented Jul 9, 2024

Thank you! I checked the IndexDB storage option in Chrome and it only shows IndexDB and no database. But oddly enough I tried the website/app and it works now. I tried in Edge and it did show the IndexDb>oidc>dpop and a key. I deleted the oidc database, retried it in Edge and it works correctly.

Not sure what was going on with Chrome regarding the IndexDb database not showing anything but all browsers work now. Thank you very much for your hard work.

If you need me to test anything let me know.

Interesting, but good news. Very odd.

In your environment, do resource servers (e.g. APIs that you call) also require the dpop nonce? I'd be interested in any experiences regarding that because it is a client concern (e.g. handling receiving, storing and using the nonce value). If you look at the react-oidc-context example I linked you to above, you will see a quick and dirty implementation that stores the nonce for requests to the API in the browser sessionStorage.

@psgfaa
Copy link

psgfaa commented Jul 9, 2024

I saw your callApi method., thank you for that example.

Yes I am working on the backend web api as well. I will use C# .NET Core Web API for the backend. I believe it will need to validate the nonce and dpop values with the auth server on all requests. That is my next task. When I get that working I can send you over an example or check it in at github.

@dbfr3qs
Copy link
Contributor Author

dbfr3qs commented Jul 9, 2024

I saw your callApi method., thank you for that example.

Yes I am working on the backend web api as well. I will use C# .NET Core Web API for the backend. I believe it will need to validate the nonce and dpop values with the auth server on all requests. That is my next task. When I get that working I can send you over an example or check it in at github.

No problem. Take a look at the Duende example API DPoP code. This extends .NET's built in JwtBearer validation. Good luck 👍

@psgfaa
Copy link

psgfaa commented Jul 10, 2024

Thank you! I will take a look. I appreciate all the help you have been.

@pamapa pamapa added this to the 3.1.0 milestone Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants