Issue #191: Allow leeway to handle clock skew #248

m777z · 2018-06-30T23:26:57Z

Not sure if I'm doing this right since it's my first real code contribution to an open-source project. I'm sending the PR to 3.3 per @lcobucci's comment on this issue on May 17.

Note that all the whitespace changes were made by my editor (PHPStorm) when set to the "PSR1/PSR2" predefined style.

Ocramius

Please do revert the code style changes - those are to be avoided in any project contribution (in general, not just here), unless they are sent in as separate dedicated patch :-)

Ocramius · 2018-07-01T13:34:15Z

README.md

@@ -88,6 +88,27 @@ var_dump($token->validate($data)); // true, because current time is between "nbf
 $data->setCurrentTime(time() + 4000); // changing the validation time to future

 var_dump($token->validate($data)); // false, because token is expired since current time is greater than exp
+
+// We can also use leeway to deal with clock skew.


I think most readers of the README.md will unlikely know what leeway or clock skew mean in this context. Also, iat, nbf and exp, while in the spec, need some expansion here.

Is this maybe for a separate documentation page? (README.md should be short and to the point)

Is this maybe for a separate documentation page? (README.md should be short and to the point)

I agree with you @Ocramius but let's do that as a separate PR, we can keep it on the README.md for now and modify it later.

Ocramius · 2018-07-01T13:35:28Z

README.md

+
+var_dump($token->validate($dataWithLeeway)); // false, because token can't be used before now() + 60 and it's now() + 14
+
+$dataWithLeeway->setCurrentTime(time() + 51); // changing the validation time to future


Use $time = time(), assigned only once to make it clear that we're working within the boundaries of fixed time throughout the example

Ocramius · 2018-07-01T13:36:38Z

README.md

@@ -97,6 +118,11 @@ var_dump($token->validate($data)); // false, because token is expired since curr
 configured in ```ValidationData``` they will be ignored by ```Token::validate()```.
 - ```exp```, ```nbf``` and ```iat``` claims are configured by default in ```ValidationData::__construct()```
 with the current UNIX time (```time()```).
+- The values for the keys in the  optional ```$leeway``` array of ```ValidationData``` will add that number of seconds 


Same here about $leeway. A link is required

Ocramius · 2018-07-01T13:37:54Z

src/ValidationData.php

@@ -24,24 +24,28 @@ class ValidationData
     */
    private $items;

+    private $timeFields = ['iat', 'nbf', 'exp'];


Docblock with @var string[] needed

Ocramius · 2018-07-01T13:38:05Z

src/ValidationData.php

@@ -24,24 +24,28 @@ class ValidationData
     */
    private $items;

+    private $timeFields = ['iat', 'nbf', 'exp'];
+
+    private $leeways = [];


Docblock with types needed

Ocramius · 2018-07-01T13:38:20Z

src/ValidationData.php

    /**
     * Initializes the object
     *
     * @param int $currentTime
+     * @param array $leeways


Please use type[] (with the correct type) here

This kind of definition of leeways is a bit squishy and hard to keep in mind, as the user needs to know the keys of the $leeways parameters. An explicit parameter for each of the fields supporting $leeways is more explicit and avoids basic mistakes.

Ocramius · 2018-07-01T13:39:39Z

src/ValidationData.php

-        $this->items['nbf'] = (int) $currentTime;
-        $this->items['exp'] = (int) $currentTime;
+        foreach ($this->timeFields as $field) {
+            if (isset($this->leeways[$field]) && is_int($this->leeways[$field])) {


The is_int() is to be avoided here - use an assertion where $this->leeways is being modified instead, and throw if a non-int is passed in.

Ocramius · 2018-07-01T13:42:30Z

test/unit/ValidationDataTest.php

+            'iss' => $iss !== null ? (string)$iss : null,
+            'aud' => $aud !== null ? (string)$aud : null,
+            'sub' => $sub !== null ? (string)$sub : null,
+            'iat' => (int)$iat,


This cast is likely wrong - should never come in as non-integer here

Ocramius · 2018-07-01T13:42:45Z

test/unit/ValidationDataTest.php

@@ -200,7 +268,9 @@ public function claimValues()
     * @param string $sub
     * @param string $iss
     * @param string $aud
-     * @param int $time
+     * @param int $iat
+     * @param int $nbf


int|null, I suppose?

Ocramius · 2018-07-01T13:42:50Z

test/unit/ValidationDataTest.php

-     * @param int $time
+     * @param int $iat
+     * @param int $nbf
+     * @param int $exp


int|null, I suppose?

lcobucci

@m777z thanks for your contribution here :)

On top of what @Ocramius said, I think that allowing people to define a leeway per claim might lead us to accidental complexity...

The problem we're trying to solve here is clock skew, and if the "client's" clock is skewed for one field it will definitely be skewed for the others. So, could you please modify things to simplify it?

lcobucci · 2018-07-02T23:05:20Z

README.md

@@ -88,6 +88,27 @@ var_dump($token->validate($data)); // true, because current time is between "nbf
 $data->setCurrentTime(time() + 4000); // changing the validation time to future

 var_dump($token->validate($data)); // false, because token is expired since current time is greater than exp
+
+// We can also use leeway to deal with clock skew.


Is this maybe for a separate documentation page? (README.md should be short and to the point)

I agree with you @Ocramius but let's do that as a separate PR, we can keep it on the README.md for now and modify it later.

m777z · 2018-07-07T05:02:39Z

Thanks for the feedback, guys! Since some pretty big changes are needed, should I continue to send commits to this pull request or just close it and open a new PR when the code is ready?

Ocramius · 2018-07-07T09:03:38Z

Sending patches here would give us an idea if thr comments are addressed: incremental reviews FTW!

…

On Sat, 7 Jul 2018, 07:02 m777z, ***@***.***> wrote: Thanks for the feedback, guys! Since some pretty big changes are needed, should I continue to send commits to this pull request or just close it and open a new PR when the code is ready? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#248 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakORKzJ6Gn3ws90uQCzWYho1fq4Boks5uEEDvgaJpZM4U-Iw1> .

m777z · 2018-07-22T23:35:13Z

@lcobucci I updated the code to only take one integer parameter for the leeway (which is then applied to each of the time-based checks).

@Ocramius The code style changes were reverted. I made some changes to README.md (used your $time = time() suggestion), though as you and @lcobucci said we may eventually want to link to a good explanation elsewhere since I am having trouble succinctly explaining leeway and clock skew. I believe all your comments on src/ValidationData.php have been indirectly addressed by the simplification I made on @lcobucci's suggestion. I also added the |null in test/unit/ValidationDataTest.php and took out the unneeded cast.

Please let me know what further changes need to be made.

lcobucci · 2018-07-23T06:58:23Z

@m777z awesome, thanks for your work!

This week I'll be releasing the last patch on 3.2.x and focus on 3.3, thus your code will be reviewed ASAP

lcobucci

@m777z I've rebased your branch to get latest changes we have in 3.3 branch (and organise the commits).

I have a small question/suggestion and would like to hear your opinion on this as well =)

Thanks for your contribution!

lcobucci · 2018-08-01T09:37:33Z

src/ValidationData.php

+     * The leeway (in seconds) to use when validating time claims
+     * @var int
+     */
+    private $leeway;


I was wondering if it makes sense to have this as a property... I mean, it's only used for configuring the current time, maybe it would be better to simply pass it to ValidationData#setCurrentTime()... @Ocramius any thoughts?

I'm completely fine with doing it either way. Leaving it as it is makes sense if it's fairly common to use the setCurrentTime() method 1 or more times after initial construction of the object while keeping the same leeway. However, if users would want to keep changing the leeway without constructing a new object, or if they rarely use setCurrentTime() directly at all, then passing leeway as a parameter to setCurrentTime() makes more sense.

@m777z I'm have no clue on how users are using this object, and since it will be deprecated very soon I don't think we should debate this much. Let's go with your suggested approach and if needed we change it. Thanks!

@lcobucci I am assuming you mean it is okay to just leave things as they are for this part. If so, are there other changes I need to make before this pull request is merged?

@m777z that's correct. There're some small CS changes (which I'll handle ASAP), I just didn't manage to work on this. Plan to work on it tomorrow

lcobucci · 2018-08-01T09:48:16Z

test/unit/ValidationDataTest.php

-            'nbf' => $time,
-            'exp' => $time
+            'iat' => $iat,
+            'nbf' => $nbf !== null ? $nbf: $iat,


It's sad that we can't use ?? to simplify this due to PHP 5.x compatibility 😭

lcobucci · 2018-08-23T14:32:46Z

@m777z thanks for your contribution (and sorry about the delay!) 😄

To address clock skew issues. More info: - #191 - #248

To address clock skew issues. More info: - lcobucci/jwt#191 - lcobucci/jwt#248

Ocramius requested changes Jul 1, 2018

View reviewed changes

lcobucci requested changes Jul 2, 2018

View reviewed changes

lcobucci force-pushed the 3.3 branch from 2c2c77f to a2a4039 Compare July 29, 2018 15:19

lcobucci force-pushed the allow-jwt-leeway branch from 85e72f3 to 895f95d Compare July 29, 2018 15:27

lcobucci reviewed Aug 1, 2018

View reviewed changes

lcobucci force-pushed the 3.3 branch from fbc5e4f to 9a137a2 Compare August 23, 2018 13:53

lcobucci force-pushed the allow-jwt-leeway branch from 895f95d to 38b75a0 Compare August 23, 2018 14:06

Issue lcobucci#191: Allow leeway to handle clock skew

78a4b22

lcobucci force-pushed the allow-jwt-leeway branch from 38b75a0 to 78a4b22 Compare August 23, 2018 14:23

lcobucci approved these changes Aug 23, 2018

View reviewed changes

lcobucci merged commit cf54151 into lcobucci:3.3 Aug 23, 2018

lcobucci self-assigned this Aug 23, 2018

lcobucci added the Improvement label Aug 23, 2018

lcobucci added this to the 3.3.0 milestone Aug 23, 2018

lcobucci mentioned this pull request Oct 14, 2018

Validation of iat breaks on systems with a clock skew. #191

Closed

lcobucci added a commit that referenced this pull request Oct 14, 2018

Add leeway to ValidAt constraint

222811d

To address clock skew issues. More info: - #191 - #248

lcobucci mentioned this pull request Oct 14, 2018

Add leeway to ValidAt constraint #271

Merged

Horikawaer added a commit to Horikawaer/jwt that referenced this pull request Nov 3, 2022

Add leeway to ValidAt constraint

7b1ca75

To address clock skew issues. More info: - lcobucci/jwt#191 - lcobucci/jwt#248

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #191: Allow leeway to handle clock skew #248

Issue #191: Allow leeway to handle clock skew #248

m777z commented Jun 30, 2018

Ocramius left a comment

Ocramius Jul 1, 2018

lcobucci Jul 2, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

Ocramius Jul 1, 2018

lcobucci left a comment

lcobucci Jul 2, 2018

m777z commented Jul 7, 2018

Ocramius commented Jul 7, 2018 via email

m777z commented Jul 22, 2018

lcobucci commented Jul 23, 2018

lcobucci left a comment

lcobucci Aug 1, 2018

m777z Aug 2, 2018

lcobucci Aug 2, 2018

m777z Aug 11, 2018

lcobucci Aug 11, 2018

lcobucci Aug 1, 2018

lcobucci commented Aug 23, 2018


		var_dump($token->validate($dataWithLeeway)); // false, because token can't be used before now() + 60 and it's now() + 14

		$dataWithLeeway->setCurrentTime(time() + 51); // changing the validation time to future

Issue #191: Allow leeway to handle clock skew #248

Issue #191: Allow leeway to handle clock skew #248

Conversation

m777z commented Jun 30, 2018

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m777z commented Jul 7, 2018

Ocramius commented Jul 7, 2018 via email

m777z commented Jul 22, 2018

lcobucci commented Jul 23, 2018

lcobucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci commented Aug 23, 2018