Add "exp" to mandatory JWT claims

[evavica]

Summary

As I implemented JWT auth for CouchDb, I stumbled upon the "exp" claim being optional and not checked if not configured explicitly.
As long-lived or never expiring JWT tokens are most likely not what you (should) want when working with them, I would expect that claim to be part of standard validation behavior.
Making it optional might lead people to think it's ok to neglect it.

Desired Behaviour

This claim should be mandatory by default, as JWTs should be short lived following the concept.
If someone actually wants to make their tokens last forever, they would have to actively make a bad decision rather than passively making it by not carefully reading the documentation, not testing properly or a lack of conceptual understanding of JWT auth.

Possible Solution

Please consider adding "exp" to the mandatory claims alongside "alg" and "sub".
If it has to stay possible to disable the check for a (good) reason, please add a configuration opportunity to disable the default exp check.

If you do not want to implement this, please emphasize that ignoring "exp" is a bad idea in the documentation.

Additional context

Security is not everybody's favorite topic. "Do my tokens really have to be short lived" is a common question beginners have. As a community, we should help pointing people in the right direction by defining sensible defaults and implementing best practices to promote the proper design of things.

Acknowledgements

Anyways, thank you for your time and work on this great product!

[rnewson]

We used to validate the exp claim if present in the token, regardless of whether it was in required_claims, but this turned out to be a spec violation, so we changed to ignore the non-required claims.

commit 331894a6acb4565c71d800f2e63206101dfbb48c
Author: Jay Doane <jaydoane@apache.org>
Date:   Mon Mar 1 13:48:19 2021 -0800

    Ignore unchecked JWT claims

    Previously, if a JWT claim was present, it was validated regardless of
    whether it was required.

    However, according to the spec [1]:

    "all claims that are not understood by implementations MUST be ignored"

    which we interpret to mean that we should not attempt to validate
    claims we don't require.

    With this change, only claims listed in required checks are validated.

    [1] https://tools.ietf.org/html/rfc7519#section-4

I agree with you that exp is an important field (along with nbf) and there is an argument for it being in the default list of required_claims. The current default is an empty list (alg and sub are required for other reasons, we can't validate without alg and without sub we wouldn't know which user we'd be authenticating).

[evavica]

Thank you for your quick reply, I agree that sticking to the spec here makes sense. And also to differentiate between claims that are required for the fundamental usability of the tokens by the app (alg and sub) and those which are important for a properly defined JWT in general.
Changing the default list to one containing exp and maybe also nbf sounds good to me.
That would greatly reduce the risk of people just forgetting about them while still maintaining flexibility.

[rnewson]

comparing to another JWT library;

https://github.com/G-Corp/jwerl/blob/6e778543ef7470e9e8ae32700f5e5a8cc25e00a4/src/jwerl.erl#L115

check_claim(TokenData, exp, false, fun(ExpireTime) ->
                                            ExpLeeway = proplists:get_value(exp_leeway, Opts, 0),
                                            Now < ExpireTime + ExpLeeway
                                        end, exp),

that false is the Required argument of check_claim. I'm obviously skimming here but it seems jwerl also doesn't check exp by default.

[evavica]

Interesting piece of code, so by default they just skip the checks of some important properties (could have just returned ok instead ). The reason why I stopped by here initially is that while having worked with several applications validating JWTs using different libs for that, none of them required me to configure these checks but implemented them as defaults. That's kind of the expected behavior.
That is probably often done via application logic and not necessarily enforced by the lib used. However, still a good choice to make in my opinion.
While it is nice to have many configuration options as CouchDB users, this particular case would be a nice fit for convention over configuration. And with the solution mentioned above, users could still configure it otherwise so it doesn't hurt having the sensible defaults.