Validation of iat breaks on systems with a clock skew.

[viviivanov]

Hi, i am using the packagist 3.2.1 version of the library and noticed a problem which occurs by validating 'iat' by default.

Scenario:
Issuing server has a skew of +1 second
Communication between server and client is sub-second speed.
Resulting in example timestamps.: $jwt['iat'] = 1001, $clientNow = 1000.
Which results in.: $token->validate() === false

I did not find a good solution to disable the validation. I don't have control over server and i would rather not adjust reference timestamp in ValidationData::__construct as it also affects nbf and exp checks. There is also no guaranty that the skew will be not more then 5 or 10 seconds and i have no time limit besides the timeframe of exp-nbf but this seems absolutely hacky to start such adjustments with variable time frames based on expand nbf. To stay compatible the change of course should not break existing behavior but a flag which disables 'iat' validation explicitly in ValidationData would suffice imho. Would you accept a PR or be willing to extend the ValidationData::_construct yourself with something like this?

    public function __construct($currentTime = null, $ignoreIAT = false)
    {
        $currentTime = $currentTime ?: time();

        $this->items = [
            'jti' => null,
            'iss' => null,
            'aud' => null,
            'sub' => null,
            'iat' => $currentTime,
            'nbf' => $currentTime,
            'exp' => $currentTime
        ];

    if ($ignoreIAT)
        $this->items['iat'] = null;
    }

I am open for any clean solution.
Regards.

[Ocramius]

@viviivanov the 4.x branch uses a Clock object to handle time issues - can a test be written against that, and then we try backporting it to 3.x?

[viviivanov]

Referencing the function from: https://github.com/lcobucci/jwt/blob/master/src/Validation/Constraint/ValidAt.php#L37

    public function assert(Token $token): void
    {
        $now = $this->clock->now();
        $this->assertIssueTime($token, $now);
        $this->assertMinimumTime($token, $now);
        $this->assertExpiration($token, $now);
    }

Changing the clock would result in the same undesired behavior of changing the validation of all three (iat, nbf, exp) parameters. What i actually think would be cleaner is disabling 'iat' validation completely and leave everything else as is.

A test case would test if a switch will turn of 'iat' validation but if there is no switch for it, what should it test :) ?

A testcase would be create a token with 'iat' in the future.

[lcobucci]

@viviivanov to disable iat constraint you on v4.0 you can create your own constraint that doesn't validate iat, and so I wouldn't change anything on the lib side.

For v3, you can extend ValidationData and override the constructor to add the behaviour you want (on only your project).

If you want to add this as a new feature I'd rather go with this direction (but it's only applicable for v3):

class ValidationData
{
    /* ... */
    public function remove($name)
    {
        unset($this->items[$name]);
    }
}

I'm not really sure if this is something other people would like to have, however I'd rather have that on the library instead of changing the constructor 😉

[telomir]

@lcobucci Something we have been discussing is the reasoning behind this check. I mean, time drift is nothing too unusual unfortunately and this is a rather strict, thus impractical check. Considering the fact that the JWT spec adds nbf and exp to define a pretty exact time frame for the lifetime of a token adds to the question marks around the necessity of the check at all.

Taking the spec at RFC 7519 into consideration as well, it does not mention any checks for that field, see https://tools.ietf.org/html/rfc7519#section-4.1.6:

"iat" (Issued At) Claim

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.

For nbf and exp, the spec is requesting validation, see https://tools.ietf.org/html/rfc7519#section-4.1.4:

4.1.4. "exp" (Expiration Time) Claim

The "exp" (expiration time) claim identifies the expiration time on
or after which the JWT MUST NOT be accepted for processing. The
processing of the "exp" claim requires that the current date/time
MUST be before the expiration date/time listed in the "exp" claim.
Implementers MAY provide for some small leeway, usually no more than
a few minutes, to account for clock skew. Its value MUST be a number
containing a NumericDate value. Use of this claim is OPTIONAL.

4.1.5. "nbf" (Not Before) Claim

The "nbf" (not before) claim identifies the time before which the JWT
MUST NOT be accepted for processing. The processing of the "nbf"
claim requires that the current date/time MUST be after or equal to
the not-before date/time listed in the "nbf" claim. Implementers MAY
provide for some small leeway, usually no more than a few minutes, to
account for clock skew. Its value MUST be a number containing a
NumericDate value. Use of this claim is OPTIONAL.

So, the check might actually be questioned for the default behavior of the library as well.

[viviivanov]

@lcobucci I am cool with the remove method actually as it would already solve the problem. And by the way I don't think this "feature" is something uncommon as i see other libs either not including 'iat' in the default set or having a discussion about exactly the same issues. eg.: jpadilla/pyjwt#190. Or introducing some other mechanism to account for clock skew. eg.: firebase/php-jwt

/**
 * You can add a leeway to account for when there is a clock skew times between
 * the signing and verifying servers. It is recommended that this leeway should
 * not be bigger than a few minutes.
 *
 * Source: http://self-issued.info/docs/draft-ietf-oauth-json-web-token.html#nbfDef
 */
JWT::$leeway = 60; // $leeway in seconds
$decoded = JWT::decode($jwt, $key, array('HS256'));

?>

But for my case the remove would be an optimal solution.
I can create a PR for 3.2 branch with the remove method if needed.

[lcobucci]

@viviivanov @telomir I've issued so many tokens where iat = X, nbf = X + Y and ex = X + Z, so I do think that this validation is important (and there are many other implementations that still have it).

I'd say we should add a leeway to the constraint (on master) and backport it to v3.3-dev.

[soundsgoodsofar]

Upvote fixing this as hard as I can. Ran into this problem in production again today and it's extremely frustrating.

I opened a ticket on a downstream consumer of this package months ago and didn't get any real traction. Would love to see this fixed here.

thephpleague/oauth2-server#798

I would also strongly suggest we add leeway to the "not valid before" (nbf) value in addition to 'iat' (or omit it). It's just not relevant to the majority of users.

Here is the patch I'm applying to this package during my composer post-install to fix the problem.

--- a/vendor/lcobucci/jwt/src/ValidationData.php
+++ b/vendor/lcobucci/jwt/src/ValidationData.php
@@ -36,8 +36,8 @@ class ValidationData
             'iss' => null,
             'aud' => null,
             'sub' => null,
-            'iat' => $currentTime,
-            'nbf' => $currentTime,
+            'iat' => null,
+            'nbf' => null,
             'exp' => $currentTime
         ];
     }
@@ -89,8 +89,6 @@ class ValidationData
      */
     public function setCurrentTime($currentTime)
     {
-        $this->items['iat'] = (int) $currentTime;
-        $this->items['nbf'] = (int) $currentTime;
         $this->items['exp'] = (int) $currentTime;
     }

[lcobucci]

@soundsgoodsofar I completely get your frustration, I'm mostly focused on getting v4 to a stable state and I couldn't get to implement this yet. You can also send a PR to branch 3.3 to help on this

[viviivanov]

@soundsgoodsofar I've been in the same position and missed the forest for the trees. There is actually a solution without updating the library even though not 100% perfect. Just offset the timestamp to ValidationData by a value before validation. Btw the 5 sec default works for me for couple of months now without any problems.

		$leeway = 5 // compensate skew with 5 seconds 
		// parse text
		$token = (new Parser())->parse((string) $tokenData);
		// verify signature
		$signer = new Sha256();
		/*
			...
		*/
		// validate claims
		$vData = new ValidationData(time() +$leeway); // iat, nbf and exp are validated by default
		if (!$token->validate($vData)) {
			//	...
		}

@lcobucci I think this issue can be closed as is. Thx for all the input.

[soundsgoodsofar]

Hey thanks for the responses guys. I'd be happy to send a PR if we can agree on a fix. Would adding a default of 5-10 seconds to IAT/NBF values be considered? Adding a bit of leeway seems to be recommended even by the official ietf spec referenced above.

@viviivanov that may be, but I'm using this as an upstream dependency from another library. True, I could try to get that project to do what you're saying, but I would argue that this should really be the default behavior. It has the potential to break things in very hard to diagnose ways for anyone using the library in a distributed environment (and that's really where oauth is meant to be used).

[lcobucci]

Hey thanks for the responses guys. I'd be happy to send a PR if we can agree on a fix. Would adding a default of 5-10 seconds to IAT/NBF values be considered? Adding a bit of leeway seems to be recommended even by the official ietf spec referenced above.

@soundsgoodsofar I would add an argument to define the leeway for the validation data, with 0 as default value to not modify the current behaviour. We can modify the default value for v4, if necessary.

[lcobucci]

With #248 it's now possible to set a leeway for the validation.