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

Parser throws exception when token is expired #440

Closed
bpappin opened this issue Feb 27, 2019 · 6 comments
Closed

Parser throws exception when token is expired #440

bpappin opened this issue Feb 27, 2019 · 6 comments

Comments

@bpappin
Copy link

bpappin commented Feb 27, 2019

When parsing a token that has expired, the parser should not throw an exception, as was implemented in #6, because then the rest of the data in the token is inaccessible.

This appears to have been brought up before, and ignored in #379.

This exact case is happening to me now. I'm literally parsing the token to check if it has expired, but before I can evaluate it, I get an exception. Ok, so now I know its expired, but I can't get any other data out of it to determine how I should refresh it.

io.jsonwebtoken.ExpiredJwtException: JWT expired at 2019-02-27T15:12:51Z. Current time: 2019-02-27T15:19:33Z, a difference of 402779 milliseconds.  Allowed clock skew: 0 milliseconds.
        at io.jsonwebtoken.impl.DefaultJwtParser.parse(DefaultJwtParser.java:385)
        at io.jsonwebtoken.impl.DefaultJwtParser.parse(DefaultJwtParser.java:481)
        at io.jsonwebtoken.impl.DefaultJwtParser.parseClaimsJwt(DefaultJwtParser.java:514)
@lhazlewood
Copy link
Contributor

This is due to the JWT specification:

https://tools.ietf.org/html/rfc7519#section-4.1.4

Quote:

The "exp" (expiration time) claim identifies the expiration time on
or after which the JWT MUST NOT be accepted for processing.

(Bold emphasis is mine). A parser rejecting the jws (i.e. MUST NOT be accepted) is a pretty clear-cut-and-dry adherence to the RFC specification IMO.

What are you trying to do that requires that you trust an expired JWS? Maybe context will help me understand what's trying to be accomplished and a best practice for that.

@lhazlewood
Copy link
Contributor

Closing as duplicate of #148 . If you want to violate the specification, you'll be able to by getting the JWT from the exception.

Due to the security implications however, we won't be altering the exception-throwing design of JJWT.

@bpappin
Copy link
Author

bpappin commented Mar 11, 2019

Parsing and processing are not the same thing, and not in the spirit of the point of the spec.

I think your interpretation of the spec needs to be peer reviewed, on a more regular basis.

This is not the first time I've noticed this library making assumptions like that, in fact I think it's the 3rd time I have personally noticed that the interpretation of the spec was misapplied in a significantly detrimental way. Which is why you may read into my response, some annoyance.

Please consider enlisting some support, because unilateral anti-pattern decisions like this, cause compatibility issues when they are corrected.

@lhazlewood
Copy link
Contributor

lhazlewood commented Mar 19, 2019

Parsing and processing are not the same thing

If you start using an expired JWT immediately after the parser returned quietly - as you would have it do - what would you call your usage? Once you start reading it and interpreting it and making logic decisions based on it - is that not processing? And consequently in direct violation of the spec?

and not in the spirit of the point of the spec.

According to whom? How do you know we haven't spoken with JWT RFC specification committee members?

I would happily discuss with you or any other contributors on this project the merit of what you claim if you can provide some credible authoritative evidence (e.g. spec committee members or contributors or security experts) to the contrary. I genuinely mean this and I'm happy to oblige.

I think your interpretation of the spec needs to be peer reviewed

It has been - many, many, many times. That is why I'm using 'we' and not 'I'. Most of us agree with JJWT's stance on secure parsing because the pitfalls without it are significant.

The only people I've seen disagree are you and some others in some of the GH issues here (and even some of them have changed their mind in favor of JJWT's current approach). I respectfully submit this is not enough evidence to overturn what we and most others consider a best practice.

This is not the first time I've noticed this library making assumptions like that

It is no assumption, I can assure you. It was a carefully crafted, very calculated behavioral choice based on the exact wording in the RFC specification as well as the significant security attack vectors that can happen with JWTs if library users are not careful.

Believe me - we agonize over this stuff. Every line of code in this project has a very clear intent for what is going on. It is why the entire codebase has 100% line and branch/conditional test coverage. We do our very best to make JJWT insanely easy to use, but sometimes we choose to favor security over usability.

Please consider enlisting some support, because unilateral anti-pattern decisions

Again, not unilateral (peer reviewed and discussed with many), and definitely not an anti-pattern according to those we've spoken with security backgrounds. And we've had great support so far, and more people lend a hand all the time.

Finally, I will end this by saying that we're all human and fallible, and I could be wrong. But so could you. As much as I respect your desire and passion behind what you are advocating, you've presented only conjecture and opinion as to why JJWT's security stance should change. Unfortunately that's just not enough to override what I, @dogeared, and the other significant contributors believe to be the correct direction for JJWT's design.

If you'd like to provide evidence from spec committee members or credible security experts that our design is flawed, I'll be the first person to work with you to design a better approach. Cheers!

@bpappin
Copy link
Author

bpappin commented Mar 20, 2019

Ok, real world software happening here.

Think through the logic of throwing an exception, and preventing any more information from being gleaned from the token, just because it's expired.

Ok, so the token is expired.
It should not be considered valid, and it's the responsibility of the calling code to prevent the client from doing anything that should be protected.
All good. Now what?

In a simple approach, the client would simply ask for a new token.
What happens if the client has several potential token providers (yes, that's a thing where these tokens are used)?
Which one do I ask for a new token from?
Unfortunately, the client can't tell, because an exception was thrown before it could get that data.

My problem with this, and with every other time I have had to raise issues, is that the library doesn't take into account what software really has to do. The library makes far too many assumptions about how it's being used, that only match a very narrow profile.

The library developer can't possibly know what is the right thing to do at the time it's being executed, and tries to impose its will on the calling code, via an exception. To make things worse, it isn't even a checked exception!

You can imagine how frustrating it is, when a library does something contrary to the "principle of least astonishment", and actively prevents normal flow. In this case, the anti-pattern is pretty much control flow by exception.

@hertg
Copy link

hertg commented Sep 28, 2022

While I get the intention behind strictly rejecting expired tokens, an option to manually override that behavior and allow a token to be expired for parsing can be a real-world requirement. There are use-cases where you want to parse JWTs even when they have expired. Although admittedly, those use-cases might be rare.

In my specific use-case, I am working on an OAuth2/OpenID Connect Provider (OP) and am currently implementing the OpenID Connect RP-Initiated Logout 1.0 specification.

I need to parse the id_token_hint parameter here, which is used just as an indicator which Relying Party (RP) is sending the logout request and which user session it's referring to. It serves as a proof that the RP actually has an ID Token which was issued by the OP, but that ID Token MAY have expired. Whether the token has expired or not must not influence the server response. But anything else (eg. the token content and signature) must still be validated.

When an id_token_hint parameter is present, the OP MUST validate that it was the issuer of the ID Token. 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. If the ID Token's sid claim does not correspond to the RP's current session or a recent session at the OP, the OP SHOULD treat the logout request as suspect, and MAY decline to act upon it.
OpenID Connect RP-Initiated Logout 1.0: 2. RP-Initiated Logout

It's an edge-case of course, because I still need to to the signature check but am accepting expired tokens. I will probably need to implement my own parser on this route, that's not that big of a deal. But would have been nice if I could also use the jjwt library for that 😄


Edit: I just realized that my use-case was already mentioned in a comment in #148

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

3 participants