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

Add support for negative leeway values #365

Closed
ploftness-tesla opened this issue Jan 18, 2024 · 8 comments
Closed

Add support for negative leeway values #365

ploftness-tesla opened this issue Jan 18, 2024 · 8 comments

Comments

@ploftness-tesla
Copy link
Contributor

ploftness-tesla commented Jan 18, 2024

I use jsonwebtoken as part of a client application. During the course of development, I have encountered two issues with how token expiration is handled:

  1. The Validation::get_current_timestamp() method truncates the current time to the previous second. This behaves differently from a library in a different language used by a server application that the client interacts with. This library considers a token to be expired if the current time is 0.001 seconds past the expiration in the token, rather than 1.000 seconds past.
  2. The Validation::validate() method does not provide a way to judge if a token is about to expire shortly. This becomes a problem if a token will expire in 0.001 seconds and the code is preparing to send it over a network socket. It may expire in-transit.

In order to allow library users to work around both these problems, I propose the following solution.

  1. Split the leeway option into separate settings for validating token exp and nbf claims. These settings can be named exp_leeway and nbf_leeway.
  2. Allow exp_leeway values to be negative numbers. This allows library users to specify that a token needs to be replaced at some time interval before expiration to account for inconsistent handling of fractional time values by other software or delays imposed by network latency.
  3. Allow nbf_leeway values to be negative numbers for consistency.

Update: Here is a PR implementing the proposed solution.

@Keats
Copy link
Owner

Keats commented Jan 18, 2024

  1. I don't see why we would go to ms resolution when we only have seconds in the api/sec
  2. Not an issue with a decent leeway, that's what it's for

@ploftness-tesla
Copy link
Contributor Author

ploftness-tesla commented Jan 18, 2024

@Keats thanks for the quick response!

  1. I'm not proposing that we go to ms resolution. My proposed fix is in the attached PR.

  2. I have not been able to identify a leeway value that fixes the following issue. Here is a drawing representing a token which is about to expire, but has not expired yet.

    positive-leeway (1)

    I would like to make the current code fail for this case since the token may expire in transit over the network.

    if matches!(claims.exp, TryParse::Parsed(exp) if options.validate_exp && exp < now - options.leeway)
    {
       return Err(new_error(ErrorKind::ExpiredSignature));
    }
    

    It looks like it will succeed for all unsigned integers.

    positive-leeway-series

    Can you suggest a leeway value that will allow me to return ErrorKind::ExpiredSignature when the token is about to expire? As a specific example, maybe you could suggest a value that would work when time now=1 and time exp=2.

@Keats
Copy link
Owner

Keats commented Jan 21, 2024

It seems that what you want, rather than negative leeway which is imo a bit confusing is another option to reject tokens that are x seconds or less from expiration?

@ploftness-tesla
Copy link
Contributor Author

ploftness-tesla commented Jan 22, 2024

It seems that what you want, rather than negative leeway which is imo a bit confusing is another option to reject tokens that are x seconds or less from expiration?

@Keats yes, rejecting tokens that are x seconds or less prior to expiration is our goal.

Configuring this as a separate setting rather than as a negative leeway value should work fine.

@Keats
Copy link
Owner

Keats commented Jan 25, 2024

Something like Validation::reject_tokens_expiring_in_less_than(seconds: int)?

@ploftness-tesla
Copy link
Contributor Author

ploftness-tesla commented Jan 25, 2024

Something like Validation::reject_tokens_expiring_in_less_than(seconds: int)?

Yes, that is a clear name in my opinion.

Would you like me to update the existing PR to reflect?

@ploftness-tesla
Copy link
Contributor Author

ploftness-tesla commented Feb 17, 2024

@Keats Here is a new PR implementing Validation::reject_tokens_expiring_in_less_than. Can you review and approve?

@ploftness-tesla
Copy link
Contributor Author

Fix released as part of version 9.3.0; closing this issue

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