Use request time instead of verification time

[Marsup]

Hi,

This resolves an issue we have in production, where a perfectly valid token at the moment of the request becomes invalid when it reaches the verification step. For example, on a super slow upload, by the time the file is fully streamed, the token can become expired.

I think it is safe to assume that the time comparison should be based on the request arrival and not when the token is processed, but should it be considered a breaking change?

[kanongil]

This PR probably makes sense as a breaking change, since auth is generally checked at request time, and then assumed constant over the lifetime of the request.

[Marsup]

To me, timeSkewSec is a different concept. It's there to account for clock synchronization problems, and it's not the case here. If the authentication happened right after the headers processing, there would be no problem at all, but there's the payload.

If for example, and that's admittedly a stupid one, someone uploaded gigabytes of data over let's say 30 minutes right after getting a fresh token, at the end of those 30 minutes once the payload is consumed, the request will still be rejected even if you set 20 minutes of time skew. That looks like an absurd behavior. What counts is the time the request was made, not the time it spent pumping data.

I don't think it changes anything to the other characteristics of the validation, quite the contrary. nbf would suffer the same problem on a token without expiry.

I'm fine with a breaking release, but it really looks like a bug to me.

[devinivy]

For example, on a super slow upload, by the time the file is fully streamed, the token can become expired.

As I understand it, this is one reason that authentication runs before payload processing in the hapi request lifecycle. I can't imagine exactly the cause of the long delay between the request and verification: is it possible that you're using server.auth.verify(request) after payload processing?

Similarly to Gil, I do feel uncertain about the security implications, particularly because this scheme's verify() is dependent on the current time rather than the request time, so we would be introducing an inconsistency there (auth succeeds while server.auth.verify() at the same moment would fail). timeSkewSec seems like a promising workaorund! I think there's an open question about the cause for the long delay that it would be useful to understand too.

[Marsup]

As I understand it, this is one reason that authentication runs before payload processing in the hapi request lifecycle.

Hmm I wrongly assumed it happened after, let me check again what's happening.

[Marsup]

OK, I can't reproduce it with a throttled payload, so it can't be that, but all the other steps prior to auth have a potential to slow down the request, I still think it's a potential problem.

[Marsup]

After further investigation, it appears our problem might be a (not so) transparent modsec that's buffering the payload, hence what I'm seeing but can't reproduce with a bare node. I still think long-ish onRequest and onPreAuth can cause invalid expiry rejections, but I'll let you decide. Deeply sorry for the disturbance, feel free to close the PR if you disagree.

[dominykas]

I'm not sure this does not have security implications.

Suppose I stole a JWT token. It has an expiry time. All I need to do is open a bunch of connections (and deliberately hold them) before that expiry time, and then I can use them. It's a stretch, and the use is still limited, but it's nevertheless an issue - there is a theoretical way to use a token after its expiry.

How long of a gap are we talking between request start and verification step? It feels to me that whoever is issuing the token should be dealing with this, not the verification library, TBH.

[devinivy]

I see how the behavior in this PR is desirable in some ways. But I am of the opinion that the current behavior is good: realistically a token can become invalid during the course of a request, and hapi already accounts for this by ensuring auth occurs very early in the request lifecycle before any significant processing. The user can use server.auth.verify() to check the status of verification over the course of the request. For the result of server.auth.verify() to not necessarily agree with the result of the verification step at the moment of authentication seems like an undesirable inconsistency to me. For whatever it's worth, other popular node JWT implementations behave like this one currently does. As a compromise, I'd support allowing the verification options to be specified as a function of the request, so a user could get this behavior if they desired.

[Marsup]

Let's close then, I don't want to open a potential security issue for the sake of being more accurate. I will agree that making requests that close to expiry with a lengthy onRequest/onPreAuth is such a rare case that there are probably better fixes than this.