-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
The nonstandard "no future iat" check is disruptive #475
Comments
For future readers, here's my current workaround (I chose to use the leeway since that seems to be the supported way of doing this).
|
I will compliment you that i was able to immediately find the code in question and understand what it was trying to do and diagnose what was going on, so nice job on the code cleanliness/readability-for-newcomers |
I just wanted to chime in here that I have run inte the same problem a few times and always set a small leeway to avoid it. However 5 minutes seems a bit extreme, and might be insecure (since it's used for expiration checks as well). Something like 10 or 30 seconds should be more reasonable. |
I agree that the I am not sure that fixing this issue is something we'd want to do in a minor version, as it would change behavior in an unexpected way that could be considered breaking. So I think this would be an appropriate fix for the next major version of this library. |
I think this is all reasonable. @Krisell I think tuning the leeway is absolutely a trade off for security vs risk of this functional issue and the cost vs benefit depends on what the JWT is authorizing the user to do. This is a personal website of mine that doesn't really protect sensitive information or let you do much more once authenticated other than edit some recipes. For sensitive stuff like financial information I totally agree but for my use case I just want to forget about the problem. :) It is worth noting that if someone copy/pastes my workaround, careful attention to the tuning should be given and if someone is creating a php library to handle this generically, the consumer of your library should be able to configure a leeway with a conservative default like Krisell noted. |
FYI I've submitted an errata request to RFC 7519 to update the spec to explicitly prohibit rejecting tokens with Feel free to follow along here: Discussion: JWT tokens containing |
The JWT.php decode() has a nonstandard check to verify that "the time according to the JWT on the issuing server" is not later than "the time on the machine that is verifying the JWT", w/in some apparently statically configured leeway. This presents a problem when the server that signs the JWT has a clock that is ahead of the machine calling decode(), where the code calling decode() is punished by getting an exception thrown, saying a perfectly valid JWT is invalid. The code calling decode() is expected to configure some arbitrary $leeway variable that is likely to break again or sacrifice security.
More details:
The firebase php-jwt JWT.php has this line:
// Check that this token has been created before 'now'. This prevents
// using tokens that have been created for later use (and haven't
// correctly used the nbf claim).
if (isset($payload->iat) && $payload->iat > ($timestamp + static::$leeway)) {
throw new BeforeValidException(
'Cannot handle token prior to ' . \date(DateTime::ISO8601, $payload->iat)
);
}
According to the RFC:
The "iat" (issued at) claim identifies the time at which the JWT was
issued. This claim can be used to determine the age of the JWT. Its
value MUST be a number containing a NumericDate value. Use of this
claim is OPTIONAL.
Notice that it says nothing about validating that this timestamp is not "in the future according to the validating machine's time". Enforcing this in the JWT.php code seems like an issue, as I've already seen (it looks like the Sign In With Google server is off by about 2 seconds with my machine). It's unreasonable to assume that it's a server maintainer's fault for getting out of sync with google's server's timestamp. It seems like it would be better, if the 'iat' is in the future, to use this to offset the timestamp, instead of throwing an exception.
The current workarounds I'm pondering are if I want to set the $timestamp static variable to this offset, to sleep() until 'iat' if it isn't too far in the future, or if I want to just bite the bullet and set the $leeway and cross my fingers that I guessed a good arbitrary leeway value.
The text was updated successfully, but these errors were encountered: