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

Provide some leniency in validating time claims #39

Closed
FreekPaans opened this issue Aug 3, 2016 · 15 comments
Closed

Provide some leniency in validating time claims #39

FreekPaans opened this issue Aug 3, 2016 · 15 comments

Comments

@FreekPaans
Copy link
Contributor

I'm currently experiencing problems validating JWTs due to clock skew, the RFC specifies some leeway MAY be provided

Implementers MAY provide for some small leeway, usually no more than a few minutes, to account for clock skew.

https://tools.ietf.org/html/rfc7519#section-4.1.4

Should we implement this? Any ideas what the API might look like?

Currently I can fix this by using a now option that is a few minutes off, but this isn't really explicit.

@niwinz
Copy link
Member

niwinz commented Aug 3, 2016

Hmm, I agree that now is not the best parameter for handle that. I'm open to suggestions!

@camsaul
Copy link
Contributor

camsaul commented Mar 14, 2017

I've ran into the same issue where the iat value from a client library (on the same machine!) is one second in the future from what buddy thinks it is.

It looks like the default impl of now floors the current system time in milliseconds (e.g. 1489534558600 becomes 1489534558) whereas I the client implementation rounds it (1489534558600 becomes 1489534559) meaning any request made in the second half of a second fails.

I've had to override now to deal with the issue (by setting it a bit in the future), but that means exp and nbf are now slightly off (e.g. tokens are rejected slightly sooner than they should be).

I think it would make sense to default to allowing a minute or so of tolerance for iat, exp, and nbf and letting that be overridable with a option called something like :tolerance

@tlrobinson
Copy link

+1 for a :tolerance or :leeway option.

Also note that the RFC doesn't actually say you should validate iat isn't in the future. That's what nbf is for. Here's a discussion on pyjwt's repo jpadilla/pyjwt#190

@niwinz
Copy link
Member

niwinz commented Mar 15, 2017

@camsaul +1, it makes sense, (+1 for :tolerance)

@metametadata
Copy link
Contributor

Following the discussion mentioned above by @tlrobinson I'd vote for removing the iat validation altogether:

RFC 7519 does not specify or even suggest this type of validation on the
'iat' claim (jpadilla/pyjwt@8f3a2a8)

@delitescere
Copy link
Contributor

delitescere commented Jul 14, 2017

Perhaps the simplest thing is to assume/request/require that the system generating the token with an nbf claim should add whatever leeway it deems reasonable? Then the system consuming the token just uses the nbf timestamp as-is.

metametadata added a commit to metametadata/buddy-sign that referenced this issue Jul 17, 2017
@metametadata
Copy link
Contributor

I've just submitted a PR removing iat claim validation (checking if token is from the future).
It cannot close the issue though since it's still desirable to implement leeway/tolerance param for validation of exp and nbf claims as stated in the RFC.

Should leeway also be applied to validation of max-age?

Perhaps the simplest thing is to assume/request/require that the system generating the token with an nbf claim should add whatever leeway it deems reasonable?

@delitescere such workaround is not always possible because token can be provided to the application by the third party.

@delitescere
Copy link
Contributor

G'day @metametadata

such workaround is not always possible because token can be provided to the application by the third party.

Yes, I would expect that in many cases the token is from a 3rd-party. To me, it's not a "workaround". Rather, it is acknowledging that the issuer is authoritative about the time period they want the token to be usable, and also acknowledging that tinkering with clock skew, ultimately, isn't a solution.

Still, I understand the compulsion to add the notion of a leeway to a library as, if you were doing the token validation yourself, you'd add whatever leeway you wanted. However, I don't agree that ignoring iat is good.

@niwinz
Copy link
Member

niwinz commented Jul 18, 2017

I agree with @metametadata. The specs does not mention any specific validation for this claim, and checking for the infuture tokens creation nbf should be used. So i will proceed to merge the #49 PR.

@delitescere
Copy link
Contributor

That is a mistake. And thus the spiral into another insecure implementation of a weak spec begins. OAuth redux.

@metametadata
Copy link
Contributor

metametadata commented Jul 18, 2017

Yes, I would expect that in many cases the token is from a 3rd-party. To me, it's not a "workaround". Rather, it is acknowledging that the issuer is authoritative about the time period they want the token to be usable, and also acknowledging that tinkering with clock skew, ultimately, isn't a solution.

I thought you were suggesting to alter token's nbf / exp on the issuer instead of using leeway on the client. But, according to JWT RFC, the client is responsible to deal with clock skews and thus such functionality should not be expected to be found in existing issuer implementations. That's why the proposed idea looks to me like a non-standard approach which cannot be applied when one has no control over a token issuer.

However, I don't agree that ignoring iat is good.

I've posted a more detailed motivation on why iat must be removed in the PR.

niwinz pushed a commit that referenced this issue Aug 2, 2017
@niwinz
Copy link
Member

niwinz commented Aug 2, 2017

457bfdc

Feedback please. If everything is ok, i will release a new major version with the latest changes on master.

@delitescere
Copy link
Contributor

I provided draft docstring for validate-claims, otherwise LGTM.

@metametadata
Copy link
Contributor

+1, looks good. Thank you for taking time to fix the issue.

@niwinz niwinz closed this as completed Aug 8, 2017
@niwinz
Copy link
Member

niwinz commented Aug 8, 2017

Released 2.0.0

elzibubble added a commit to elzibubble/yada that referenced this issue May 10, 2018
This updates buddy-core 1.1.1 -> 1.4.0,
which updates org.bouncycastle/bcprov-jdk15on 1.55 -> 1.58,
alleviating CVE-2016-1000341.

See https://www.bouncycastle.org/releasenotes.html section 2.4.4, search
for CVE-2016-1000341.

This is a major version upgrade because of the following incompatible
change:
    funcool/buddy-sign#39
malcolmsparks pushed a commit to juxt/yada that referenced this issue May 14, 2018
This updates buddy-core 1.1.1 -> 1.4.0,
which updates org.bouncycastle/bcprov-jdk15on 1.55 -> 1.58,
alleviating CVE-2016-1000341.

See https://www.bouncycastle.org/releasenotes.html section 2.4.4, search
for CVE-2016-1000341.

This is a major version upgrade because of the following incompatible
change:
    funcool/buddy-sign#39
dijonkitchen added a commit to dijonkitchen/buddy-sign that referenced this issue Aug 30, 2018
Since it references Issue funcool#39 and provides better context.
niwinz pushed a commit that referenced this issue Oct 19, 2018
Since it references Issue #39 and provides better context.
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

6 participants