Add OIDC client access token expires in skew

[sberyozkin]

Fixes #46142.

This simple PR adds an OIDC client access token expires in property to let users get the correct token expiry time.

By default, the current time is added to the relative expires_in response property - however, due to the clock skew or the IDP specific configuration (as explained in the recent Zulip dev thread), the actual token issued at time can be earlier (or even later) than the current time. For example, a 1 min difference can be quite significant.

Adding an option to sync it with by configuring this new skew property (Duration can also be negative if needed) will let OIDC client calculate a more precise token expiry time.

I've confirmed that updated test is failing without adding a skew property.

If this PR does not make it to 3.18 right now then we might Guillaume to consider it for the backport to 3.18 later, we don't have to rush it if there are some concerns

[sberyozkin]

Michal, George approved so I added a waiting for ci label for Guillaume @gsmet or someone else to possibly merge tomorrow morning, I just wanted to keep you in the loop re the way the OIDC client configuration was updated (you made it very easy to add it in all the required places).

I guess this is not a critical PR for the immediate backport but it would be worth having it in 3.18 eventually...

I'm signing off now, good night, everyone :-)

[michalvavrik]

@sberyozkin I am happy to read it tomorrow (as I read all your PRs anyway, add me as a reviewer or not) but I just finished long day on QE stuff, so not today, thanks for keeping me in a loop, good night :-)

[github-actions]

🎊 PR Preview 046dc99 has been successfully built and deployed to https://quarkus-pr-main-46222-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[gsmet]

I'm going to ask a stupid question but do we need an additional property?

From what I can see, you would have the same result by increasing expiresIn a bit?

Interested in understanding why this is better!

[sberyozkin]

Hi @gsmet

It is a good question, thanks, that particular test was just easier for me to update to confirm adding the skew works - this test only checks that the calculated token expiry time is correct, indeed, in that particular setup it is not really necessary to configure 2 properties. There was a case reported by the Salesforce user where the standard expires_in property was not even returned, so the test which I updated supports that case - that if the provider does not tell Quarkus when the token expires, users can get a chance to tell Quarkus about it - so that the access token can be refreshed in a timely manner etc.

So usually, the main use case, the relative expires_in property is coming up in the token grant response. Now Quarkus would add it to the now() current time to calculate the absolute expires at time, but as was reported on the dev channel, there could be cases where using now() is not very precise.

We have tests which work with this expires_in property is coming up in the token grant response, but I looked yesterday evening and there were quite complex already, actually waiting for the token to expire, so I just took an easy path with updating the test where adding skew is indeed not really necessary.

Hope it clarifies it

[sberyozkin]

@gsmet, just for your info, #41067, the test which I updated was originally created to address that issue. You are right in this case the skew is unnecessary, but it is just the simplest test out there where I can check easily that configuring a skew impacts the test result :-)

I guess I'll go ahead and merge later today unless yourself or Michal will have more comments

[michalvavrik]

We also have quarkus.oidc.token.refresh-token-time-skew so it makes sense to align this for access token.

As for suggestion that users can use access-token-expires-in, there is a difference because source of expiresAt can be:

1. jwt claim expiration time
2. absolute-expires-in config property
3. granted tokens 'expires-in'

for 1., now is not Quarkus server side now but identity provider now (when they created the tokens), 2. is different completely. I saw a user scenario for this in the Zulip discussion.

@sberyozkin correct ^^ if I am wrong, but changes LGTM, thanks

[sberyozkin]

Michal, @michalvavrik,
We can't know what exactly IDP used as a source of now, this is the problem this skew property is trying to address, at the OAuth2 spec level all we have is a relative expires_in standard response property (this should be at point 1. :-) above) which we need to work with, using the Quarkus now() as the base number of seconds, but indeed, to address some differences between the IDP and Quarkus now(), this skew property is added.

As for checking JWT claims - it is a fallback, only when no standard expires_in response is available - tokens can be opaque as well. You are right the user was not asking about this specific case but I have a feeling that if we restrict adding the configured skew only when expires_in is available, tomorrow someone will say, JWT expiry claim needs to be adjusted a little bit :-).

This is an optional property, if someone thinks it should be configured then they must have a reason for it, and it will be added to the calculated expiry time, irrespectively of what the source for the calculation was, IMHO it is reasonable, and lets us avoid some deep variations there :-)

[sberyozkin]

@michalvavrik Michal, I think I'll rename this property from accessTokenExpiresInSkew toaccessTokenExpirySkew to make less tied to the expiry_in processing, thanks

[michalvavrik]

Thank you @sberyozkin , you explanation is very helpful and it helps me to understand the situation.

[sberyozkin]

Thanks for the feedback, @michalvavrik

[sberyozkin]

Sorry for the noise with pushes, so, Michal, @michalvavrik, I've renamed the property as suggested and simplified the JavaDocs definition to avoid sending a message one should only use it with the token grant response's expires_in, we don't really know if the JWT claim exp claim fallback will always be set to the correct value either... Added an NPE check as well...

Hope it is better now

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 634819a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[michalvavrik]

LGTM

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 634819a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.