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

Allowed clock skew resolving #815

Closed
laurids opened this issue Sep 7, 2023 · 4 comments
Closed

Allowed clock skew resolving #815

laurids opened this issue Sep 7, 2023 · 4 comments

Comments

@laurids
Copy link

laurids commented Sep 7, 2023

Is your feature request related to a problem? Please describe.
At the time of parsing the JWT, we don't know which authorization server (AS) issued the JWT.
We want to enable configuring "allowed clock skew" per AS. E.g. for one AS, we allow 10 minutes deviation, for another we only allow 30 seconds.
Regarding the signature verification, we can accommodate modelling multiple AS's with the setSigningKeyResolver on the JwtParser, since the resolveSigningKey gets passed the Claims and JwsHeader containing both the Key Identifier and the Issuer, which we can use to identify the AS and lookup the key.
However, for the allowed clock skew, there is only the setAllowedClockSkewSeconds(long seconds), which we would have to set before parsing, when we don't know the specific AS.
We cannot (mis)use the setClock either, because clock.now() is only called once and is used for validating both expiration and "not before".

Describe the solution you'd like
We would like a resolver that sets both the signing key and the allowed clock skew, or a separate resolver that just sets the allowed clock skew.

Describe alternatives you've considered
Alternatively, we could catch and ignore the ExpiredJwtException and PrematureJwtException when parsing, and then verify expiration and "not before" explicitly afterwards. However, in that case, it would be nice to have the time validation as a separate public method so we could call that part of the code again afterwards.

Additional context
N/A

Thank you for creating a good library whether or not you deem this a worthy feature request.

@laurids laurids closed this as completed Sep 7, 2023
@laurids laurids reopened this Sep 7, 2023
@lhazlewood
Copy link
Contributor

lhazlewood commented Sep 7, 2023

@laurids thank you for the issue, especially such a well-written one!

I must admit, I was a little surprised by the variance represented across various servers. A production Authorization Server (AS) - responsible for real security implications of clients, applications and users, has up to a full 10 minute clock skew?

The JWT RFC time-based assertions are expected to be reasonably close to the 'real' time in the world - i.e. synchronized close enough to atomic clocks around the world - such that a minute or two (or three?) should be more than sufficient.

I would find it difficult to undertake this feature request given my bigger concern for an actual security apparatus (the AS) that is that far out of date, when so many security implications derive from an AS. In other words, that level of difference is a real security risk, and I feel it'd be better to get that AS on a reasonable system time than invest design, implementation and test time in JJWT for a feature that, at least IMO, helps circumvent the 'real' risk of a misconfigured AS.

Also, for what it's worth this is the first such request in nearly 9 years we've had for a per-issuer clock skew, so at least on the surface, feels a little to me like JJWT modifications might not be the right place to address the issue. To be clear however, I'm not saying we shouldn't undertake the work, just expressing my doubts. If others requested such a feature, I'd be much more open to doing the work.

At the very least, if we would undertake this kind of request, it'll have to wait until after probably the 1.0 release is made in the coming months since we have a lot of people waiting on that 😅

@laurids
Copy link
Author

laurids commented Sep 8, 2023

Hi @lhazlewood, thanks for the response!
To be honest, I just picked the values out of thin air to state an example. Fair point though, e.g. 10 seconds to 2 minutes would perhaps be a more reasonable range.
In our project, the users (admins) will be able to configure reasonable values themselves.
It is very understandable that you want to focus on 1.0.
If you are not completely opposed to implement any of my suggestions, we can perhaps discuss this further after 1.0?

PS: I made a mistake when describing our possible workaround. We cannot just catch and ignore those exceptions, since the whole parsing method would obviously fail then.
However, we can set the allowed clock skew to some "max" value, and then do our own validation using the configured allowed clock skew after the parsing.

@lhazlewood
Copy link
Contributor

I was thinking about this more today, and I'm pretty sure this will be resolved via #474. You would be able to specify a custom nbf or exp validator that can adjust clock skew as desired. If you feel otherwise, please let me know! I'd be happy to reopen this issue otherwise.

@laurids
Copy link
Author

laurids commented Sep 11, 2023

Thanks for the update! Without having seen the code, this indeed sounds like it could solve our issue.
I'll join the group of people looking forward to 1.0 ;)

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

No branches or pull requests

2 participants