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

Request: refresh 'id_token' w/ 'access_token' #53

Open
nwoolls opened this issue Oct 24, 2023 · 5 comments
Open

Request: refresh 'id_token' w/ 'access_token' #53

nwoolls opened this issue Oct 24, 2023 · 5 comments
Labels
area/access-token-management Issues related to Access Token Management impact/non-breaking The fix or change will not be a breaking one
Milestone

Comments

@nwoolls
Copy link

nwoolls commented Oct 24, 2023

Which version of Duende IdentityServer are you using?

I am using only Duende.AccessTokenManagement, version 2.0.3.

Which version of .NET are you using?

.NET 8 RC2.

Describe the bug

This is a feature request. I apologize if this is not the right forum. I tried to find the right path from the here.

I saw this mentioned and closed previously here:

IdentityModel/IdentityModel.AspNetCore#291

We are using a 3rd-party IDP (Ping Identity) and, if we try to sign out using the normal SignOutAsync() after the id_token has expired, we receive an error that the id token is expired.

I can see that the new id_token is returned with the refresh call, but in the current implementation of this library, it is dropped rather than returned with the user token and stored in the cookie.

I've put together a very simple bit of code that unfortunately has to duplicate some of your internal logic to refresh the id_token with the rest of them, e.g. similar to what is seen here:

https://devforum.okta.com/t/get-token-from-asp-net-core-to-pass-to-backend-as-verification/5914/6

And confirmed that this does work.

Given there may be other IDPs with this challenge, is it possible you may revisit refreshing the id_token along with the access_token and refresh_token?

To Reproduce

Output an id_token, access_token, and refresh_token before calling e.g. HttpContext.GetUserAccessTokenAsync(). Note that afterward only the access_token and refresh_token were updated / persisted.

Expected behavior

I'd like to see, either OOTB or optionally, the id_token returned when using a refresh_token returned in the UserToken object and persisted in the store / cookie.

Additional context

You can see the report on this to the IDP (Ping) here: pingidentity/pingone-sample-dotnet#8

@nwoolls nwoolls changed the title Request: refresh Request: refresh 'id_token' w/ 'access_token' Oct 24, 2023
@AndersAbel
Copy link
Member

The logout is described in the OpenID Connect RP-Initiated Logout 1.0 spec. Section 2 states that:

The OP SHOULD accept ID Tokens when the RP identified by the ID Token's aud claim and/or sid claim has a current session or had a recent session at the OP, even when the exp time has passed.

The interpretation of SHOULD is defined in RFC2119 as

This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course.

To me, it looks like an OP should not validate the exp of the id_token_hint, unless there are specific reasons to do it. IdentityServer does accept expired id tokens as id_token_hint, so I don't think this will be anything that is a high priority for us to fix.

@nwoolls
Copy link
Author

nwoolls commented Oct 25, 2023

@AndersAbel thank you for your feedback and for chiming in on the linked post!

@AndersAbel
Copy link
Member

The Duende.AccessTokenManagement library is, as the name implies, all about access token management. We never really considered the id_token.

The only use we see for the id_token after the initial session establishment is to be used at logout. As discussed previously an old id_token should be valid for that. Usually and id_token has a very short lifespan, shorter than the access token, so even if we would save it there's no guarantee that the id_token is valid when logout is tried. The real fix here is for Ping to fix their implementation.

Our conclusion is that we will not make any changes to the library to automatically save the id_token. However, we try to make our libraries flexible enough to allow users to customize behaviour. Apparently, there is no extension point in the existing library that would allow saving the updated id_token. We are looking into possibilities to add such an extension point.

@AndersAbel AndersAbel transferred this issue from DuendeSoftware/Support Nov 30, 2023
@leastprivilege leastprivilege assigned damianh and unassigned AndersAbel Oct 1, 2024
@damianh damianh transferred this issue from DuendeSoftware/Duende.AccessTokenManagement Nov 17, 2024
@damianh damianh added this to the atm-3.1 milestone Nov 17, 2024
@damianh damianh added area/access-token-management Issues related to Access Token Management priority/3 Medium labels Nov 17, 2024
@damianh damianh removed this from the atm-3.1 milestone Nov 18, 2024
@damianh damianh removed their assignment Dec 2, 2024
@damianh
Copy link
Member

damianh commented Dec 2, 2024

https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse

If an ID Token is returned as a result of a token refresh request, the following requirements apply:

We don't currently implement this, I think we should cc @josephdecock

@damianh damianh added this to the atm-future-major milestone Dec 2, 2024
@damianh damianh added impact/non-breaking The fix or change will not be a breaking one and removed priority/3 Medium labels Dec 2, 2024
@damianh damianh modified the milestones: atm-future, atm-3.1 Dec 2, 2024
@AndersAbel
Copy link
Member

https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse

If an ID Token is returned as a result of a token refresh request, the following requirements apply:

We don't currently implement this, I think we should cc @josephdecock

As far as I can see, we do implement this correctly on the IdentityServer side. The question in this issue is about Duende.AccessTokenManagement not making the new id_token available.

This is an original id_token as issued at login time:

{
  "iss": "https://localhost:5001",
  "nbf": 1733303268,
  "iat": 1733303268,
  "exp": 1733303568,
  "aud": "web1",
  "amr": [
    "pwd"
  ],
  "nonce": "638689000648602397.ZmU4OTMwZDgtMDA3Ni00NTA1LWE2OGItZjFhNDdjYmIxNmY3YTk5MzNiMmEtMTljMC00OTA0LWEzYTItY2FkMjNhMWQ4NmVi",
  "at_hash": "Tud6l-X12NgwG7svjJUB9Q",
  "sid": "A446247C7703984E7FE92CB701DADA84",
  "sub": "1",
  "auth_time": 1733303268,
  "idp": "local"
}

And this is the corresponding id_token returned from token endpoint from the refresh token flow:

{
  "iss": "https://localhost:5001",
  "nbf": 1733303603,
  "iat": 1733303603,
  "exp": 1733303903,
  "aud": "web1",
  "amr": [
    "pwd"
  ],
  "at_hash": "vosRtNEH8phSev_zF9WkoQ",
  "sid": "A446247C7703984E7FE92CB701DADA84",
  "sub": "1",
  "auth_time": 1733303268,
  "idp": "local"
}

My suggestion for this issue is to make the refreshed id_token available from the refresh result, but we should not store it automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/access-token-management Issues related to Access Token Management impact/non-breaking The fix or change will not be a breaking one
Projects
None yet
Development

No branches or pull requests

3 participants