add: jwt-allowed-skew-seconds config

[steve-chavez]

For mitigating #5196 when it happens.

Was previously discussed it on #4035 as a way to lower the skew for testing purposes, but this was dangerous as users could induce errors.

To avoid the above, we don't allow lowering the value past our default of 30 seconds.

[wolfgangwalther]

If 30 seconds is not enough skew, the problem must be fixed on the various machines: sync their clocks properly!

I don't think it makes sense to add this.

[taimoorzaeem]

If 30 seconds is not enough skew, the problem must be fixed on the various machines: sync their clocks properly!

Agreed. Also, without any upper bound, this is very unsafe.

[steve-chavez]

If 30 seconds is not enough skew, the problem must be fixed on the various machines: sync their clocks properly!

Right but #5196 (comment) applies.

without any upper bound, this is very unsafe.

RFC says:

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

One interesting thing is that the RFC only mentions that for the exp and nbf claims and not iat but we've been applying the skew for iat for a while; I don't think it hurts.

So what about two minutes maximum?

usually no more than a few minutes

We could interpret that as 3 to 5 minutes.

[steve-chavez]

Found that Envoy has a skew of 60 seconds by default and doesn't have a max imposed (ref).

[steve-chavez]

Added a 300 seconds (5 minutes) max skew. Open to discuss if it should be lower.

[steve-chavez]

Also found this standard that does consider iat:

NOTE 3: Clock skew is a cause of many interoperability issues. Even a few hundred milliseconds of clock skew can cause JWTs to be rejected for being "issued in the future". The DPoP specification [RFC9449] suggests that JWTs are accepted in the reasonably near future (on the order of seconds or minutes). This document goes further by requiring authorization servers to accept JWTs that have timestamps up to 10 seconds in the future. 10 seconds was chosen as a value that does not affect security while greatly increasing interoperability. Implementers are free to accept JWTs with a timestamp of up to 60 seconds in the future. Some ecosystems have found that the value of 30 seconds is needed to fully eliminate clock skew issues. To prevent implementations switching off iat and nbf checks completely this document imposes a maximum timestamp in the future of 60 seconds.
https://openid.net/specs/fapi-security-profile-2_0-final.html

We could decide on a later change whether to change the default to 60 seconds.

Or perhaps we can make this a fixand not have a skew config?

[steve-chavez]

One interesting thing is that the RFC only mentions that for the exp and nbf claims and not iat but we've been applying the skew for iat for a while

Actually the above makes sense since the RFC doesn't really mandate validation against iat, it's only informational unlike nbf. So is only our implementation policy that we validate iat.

The RFC does conflict with https://openid.net/specs/fapi-security-profile-2_0-final.html, IIUC that does mention iat validation.

So I guess another option for a fix would be to stop validating iat?

[wolfgangwalther]

None of what is discussed here makes any sense to me. The detailed report in #5196 (comment) allows two interpretations:

• either the clocks are just not synced - the proper fix is to sync these clocks, not to increase the allowed skew!
• or it's a postgrest thing when initializing the get time cache, in this case the difference that we'll be looking at will be much higher than any possible allowed skew, it would probably compare a unix timestamp to 0 or so.

There is no point in adding such a feature without any indication of its usefulness.

[taimoorzaeem]

or it's a postgrest thing when initializing the get time cache, in this case the difference that we'll be looking at will be much higher than any possible allowed skew, it would probably compare a unix timestamp to 0 or so.

There is no point in adding such a feature without any indication of its usefulness.

Yeah, it could possibly turn out that this config doesn't mitigate the timing related problem — what if the timing is way off?

I think before progressing with this PR, we should actually try to debug the core issue first i.e merge and release #5197.

[steve-chavez]

I think before progressing with this PR, we should actually try to debug the core issue first i.e merge and release #5197.

I can agree with that, we need #5197 on v14 though as mentioned on #5189 (comment). Putting this on draft for now.

[steve-chavez]

I'm having a hard time reproducing #5196 (comment) again.

Under synced clocks, perhaps the 30 clock skew is too big to get the iat failure.

So I'm thinking, what's enough to prove #5196 is only that there's a skew > 1s in the cached time because:

postgrest/src/library/PostgREST/AppState.hs

Line 81 in a2c1b40

<*> mkAutoUpdate defaultUpdateSettings { updateAction = getCurrentTime }

defaultUpdateSettings :: UpdateSettings ()
defaultUpdateSettings =
    UpdateSettings
        { updateFreq = 1000000 -- 1 second here
        , updateSpawnThreshold = 3
        , updateAction = return ()
        , updateThreadName = "AutoUpdate"
        }

The cached time is only supposed to lag 1 second.

[steve-chavez]

So to do the above, we could have one of those PGRST_INTERNAL configs. But I feel like this issue has gotten so much attention that it cannot be a hidden test-only functionality.

Also I should be able to configure it to be lower than 30s.

[steve-chavez]

While #5208 was solved. I believe this should be added for testing/debugging purposes. Without this, it was impossible to confirm the previous fixes were ineffective #5196 (comment).

I'm not seeing how #5196 would happen again, but I've said that in the past. So it'd be handy to have this to config to quickly override the skew to 0. Maybe that's something required in more security sensitive contexts too.