lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

Return `InvalidToken` only for jwt token errors

Open pawanjay176 opened this issue 2 years ago • 4 comments

Description

Currently, we return the InvalidToken execution endpoint error if the HTTP error code is 401 or 403. However, execution clients can return jwt errors for other types of errors as well (e.g. incorrectly set vhosts on geth) which might be potentially confusing to debug.

Also, different ELs return different errors codes/responses for jwt errors:

  1. geth returns 403 with a a response "missing token" "invalid signature"
  2. nethermind returns a 200 with a valid json rpc response like {"jsonrpc":"2.0","error":{"code":-32700,"message":"Authentication error"},"id":null}
  3. besu returns a 403 with valid json rpc response like { "jsonrpc": "2.0", "id": null, "error": { "code": -40100, "message": "Unauthorized" } }

Expected behaviour

Fix the error response in lighthouse to return InvalidToken only for actual jwt errors.

pawanjay176 avatar Aug 30 '22 20:08 pawanjay176

Fix the error response in lighthouse to return InvalidToken only for actual jwt errors.

What do you mean by an actual JWT error? Ideally I think we should log the error message if we get a 401/403 which is what I was trying to do in https://github.com/sigp/lighthouse/pull/3454, https://github.com/sigp/lighthouse/issues/3435. For Nethermind I guess we should look for that non-null error value and translate that to an error as well (while preserving the error).

michaelsproul avatar Aug 31 '22 07:08 michaelsproul

What do you mean by an actual JWT error?

I meant the error returned by the the EL for invalid jwt. The fact that all ELs respond in different formats and error codes makes this a bit annoying, but I think we can change the InvalidToken error to something like AuthenticationError to keep it more general and just log whatever the EL returns in the error field if 403/401. Made an issue in nethermind to return the correct response code https://github.com/NethermindEth/nethermind/issues/4508

pawanjay176 avatar Aug 31 '22 19:08 pawanjay176

However, execution clients can return jwt errors for other types of errors as well (e.g. incorrectly set vhosts on geth) which might be potentially confusing to debug.

The fact that all ELs respond in different formats and error codes makes this a bit annoying

I think the true solution would be for all (current and future) EL's to offer streamlined, predictable behaviour on failure, ideally including an error message in the JSON response that we can re-use on top of the error code (e.g.: 401 for JWT or other auth issues, 403 for mismatching host headers, disallowed sources, etc ?) Is this not defined in the OpenRPC base? If not, can we get it included in the EE API spec?

Having to catch specific clients' behaviour would be a maintenance nightmare: nothing worse than a specific but wrong error (this is what happens today)!

In the meantime, we could just make the current error message more ambiguous (Request rejected by EE: potential JWT error or host mismatch ?), as to not misdirect any troubleshooting effort. It basically boils down to this; if we can already jump one place ahead with very little effort, we should go for it, I think:

Correct, concise logging > ambiguous logging > wrong or misleading logging

antondlr avatar Sep 03 '22 12:09 antondlr

latest version awalys print Auth(InvalidToken) :(

baguettex avatar Sep 15 '22 14:09 baguettex