Feature request: JWT claim validation should include checks for expiration claim

[W1M0R]

Currently, the JWT token required by postgres-websockets supports the mode and channel claims. It would be good if the JWT claim validator could also check the standard registered claims, such as exp (see https://tools.ietf.org/html/rfc7519#section-4.1), since it is a good mitigation strategy against JWT replay attacks (https://auth0.com/docs/security/common-threats#preventing-replay-attacks).

The simplest approach is probably to close/terminate/reject the websocket connection if the token has expired. If the token was valid at the time of connection, but has since expired, then the next send/receive should detect the expiration and either close forcibly without notifying the clients of the reason, or close with a reason (e.g. jwt expired) so that clients can reconnect using a new valid JWT.

The documentation for handling websocket termination includes a section about error status codes, that might be useful: https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent#Status_codes

[diogob]

@W1M0R please take a look at #56 I have used a normal 1000 code for closing, it seemed more appropriate than using an error code since the expiration is expected by the client. There is a string sent with the close request JWT expired.

Also note that the older versions of the code should validate on the opening of the connection.

[W1M0R]

Thanks @diogob. I know little about Haskell, but I can understand the unit tests verifying that your implementation works.

Closing the websocket using normal closure makes sense, but I have a few observations. There are client-side (Browser, NodeJS, electron, etc) libraries that handle automatic reconnect of WebSockets that close with an error. I'm not sure whether they attempt reconnects if a WebSocket was closed without an error code. However, this could rather be seen as a limitation of the client side library and not postgres-websockets responsibility. Alternatively, postgres-websockets could improve compatibility by making this configurable, but that introduces complexity.

More importantly, the benefit of using one of the error codes for representing a JWT expired error, is that this library could add support for other types of errors later, and they would be uniquely identifiable by their code, instead of the hard coded string message. If you decide to change the reason message text in the future with the current implementation, then it would be a breaking change for clients, but if they were able to use a dedicated reason code that represents JWT expiry, then they would be fine.

I'm note sure what the cost is of validating token expiry at every message. The use case for WebSockets is typically real-time communication, and whatever the library does should probably not interfere too much with that expectation. The cost is probably negligible, but if not, then a timed check can be implemented, where verification happens after every x seconds that the channel is open, or some other trigger criteria.

Lastly, the expiry time is a JWT and server-side concept that clients usually wouldn't parse or set, because they don't have the JWT secret key. So I think your previous message deliver timestamp was better, since it represents more the format that clients would want to work with. But if you prefer your current implementation using nano seconds, then that is also fine, there is no harm done in doing so. Your previous field name using underscore is also good, if you don't want to make this a breaking change.

Still, this implementation is a good start, and one could always wait and see if clients specifically require the observations I noted, and then implementation them according to actual use cases.

[diogob]

@W1M0R These are good points you bring. I'll roll back the message_delivery_at breaking changes since they were speculative in the first place and breaking compatibility needs a stronger motivation.

Regarding the error code, we could use 1008 since Policy Violation it's pretty vague and does not imply any particular recovery strategy. Let me know if you have any better idea for the particular code to use.

[W1M0R]

I closed this issue by accident while using the GitHub app on my phone (the page looked like a dialog and the close button like it would close the dialog).

[W1M0R]

According to the Mozilla docs on reason codes:

Note that the 1xxx codes are only WebSocket-internal and not for the same meaning by the transported data (like when the application-layer protocol is invalid).

Upon reading some more, one can expect the browser to use the 1xxx codes when closing the websocket. I would say use 3xxx, e.g. 3001.

[diogob]

@W1M0R thanks for all the help designing this feature. I've made some additional changes. Added the code 3001 when the connection is closed due to JWT expired and reverted the changes in the message_delivered_at field.

I'll merge now and will batch in the next release once I get something to solve #55 in place.

[diogob]

Released in 0.7.0.0 to hackage and dockerhub