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

Increase JWT issued-at window to 60s #256

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Conversation

dapplion
Copy link
Contributor

@dapplion dapplion commented Jul 8, 2022

Clock drift can occur in resource constraint machines. During network issues both CLs and ELs resources tend to increase a lot, which can delay message processing and affect the JWT iat window. Having a "short" window of 5 seconds can escalate bad network condition into a full disconnect between CL and EL. If that happens the network stalls. Due to this DOS vectors could be cheaper since you only need to push one of the clients out of the 5s window.

@holiman is there any reasoning for the 5s value? Proposing to increase the value has:
pros:

  • Higher resilience against broken comms between CL-EL during high resource usage
    cons:
  • ??

For this PR I picked 60s as a random value that's > 5s, but don't have any specific argument for it. Would like to hear opinions for

  • 5s
  • 20s
  • 60s
  • 300s, etc

According to the doc:

The type of attacks that this authentication scheme attempts to protect against are the following:

  • RPC port exposed towards the internet, allowing attackers to exchange messages with EL engine API.
  • RPC port exposed towards the browser, allowing malicious webpages to submit messages to the EL engine API.

So, if the token is stolen, what attack can you do in 60s that you can not do in 5s? For a profit driven attacker you probably want to override the fee_recepient, so in that case shorter windows reduce the change of producing a block after stealing. However, if you are able to steal one token, are you likely to be able to steal next token with higher iat?

@MarekM25
Copy link

MarekM25 commented Jul 8, 2022

I agree with @dapplion. I don't see the cons of setting it to 60s. 5s could be a too short period in some edge case situations, for example, the machine is busy.

@LukaszRozmej
Copy link

Engine API specifies 8s timeouts https://github.com/ethereum/execution-apis/blob/main/src/engine/specification.md#timeouts . Is there any sense to wait longer than that? Potentially one is desynced clocks on different machines.

@holiman
Copy link
Contributor

holiman commented Jul 8, 2022

Re the various time-windows. I'd like to avoid time-window on the order of hours. If a user posts an access log or token on a github issue, to debug why CL/EL comms isn't working, then it should effectively be useless by the time an attacker is able to make use of it.
60s fine by me
5 minutes: IMO still pretty ok, but I'd prefer 1 minute.

@lightclient lightclient added the A-engine Area: for future consideration label Jul 11, 2022
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@djrtwo
Copy link
Contributor

djrtwo commented Jul 18, 2022

Planning on merging this in ~24 hours if no other comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-engine Area: for future consideration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants