Skip to content

graphql: add query timeout to prevent dos attack #26116

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

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

ahmetavc
Copy link
Contributor

@ahmetavc ahmetavc commented Nov 5, 2022

#26026 as its proposed in this issue, I added the timeout

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, but I wonder if 60s isn't a bit too lenient.

@Shiti
Copy link

Shiti commented Nov 5, 2022

We use graphql queries to fetch block and transaction data in bulk. The range of blocks we are able to fetch is between 100-1000. But the response time varies on the data size. Would it be possible to make this configurable ?

@ahmetavc ahmetavc closed this Nov 5, 2022
@ahmetavc ahmetavc reopened this Nov 5, 2022
@ahmetavc
Copy link
Contributor Author

ahmetavc commented Nov 5, 2022

We use graphql queries to fetch block and transaction data in bulk. The range of blocks we are able to fetch is between 100-1000. But the response time varies on the data size. Would it be possible to make this configurable ?

If the number of connections are not creating any problems, instead of making it configurable and failing fast, setting the limit according to max range would work fine imo.

@orangeagain
Copy link

orangeagain commented Nov 6, 2022

Is it possible make this configurable/ add a start parameter? like geth --querytimeout=2000ms

@fjl
Copy link
Contributor

fjl commented Nov 6, 2022

Scaling the timeout with request size is not possible in the general case. It's also not what this timeout is for. We mainly want to prevent totally stuck queries with this timeout, so it can be long.

@fjl fjl changed the title eth/graphql: add timeout to graphql queries to prevent dos attack graphql: add query timeout to prevent dos attack Nov 6, 2022
Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM. Error example:

{
  "errors": [
    {
      "message": "context deadline exceeded"
    }
  ]
}

@holiman holiman added this to the 1.11.0 milestone Nov 8, 2022
@holiman holiman merged commit ee9ff06 into ethereum:master Nov 8, 2022
@s1na
Copy link
Contributor

s1na commented Nov 8, 2022

Actually this also interacts with the RPC WriteTimeout. The error I posted above will never be returned to the user because connection will be severed at the 30 seconds mark (See #21430 and #25457). I still don't think there's a harm in adding a timeout to graphql, but the original issue was actually a non-issue and the timeout set here is too long.

@ahmetavc
Copy link
Contributor Author

ahmetavc commented Nov 8, 2022

So something between 20-30 sec should be good? @s1na

@s1na
Copy link
Contributor

s1na commented Nov 9, 2022

@ahmetavc Yes let's shoot for a bit under 30 sec.

@fjl
Copy link
Contributor

fjl commented Nov 10, 2022

Wait, so we apply the RPC WriteTimeout to GraphQL connections as well?

shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
This PR adds a 60 second timeout to graphql queries.
@Doc-Pixel
Copy link

Question regarding the timeout: Can I disable this? I'm running large queries and get error messages like this:

requests.exceptions.ChunkedEncodingError: ('Connection broken: IncompleteRead(1197 bytes read, 851 more expected)', IncompleteRead(1197 bytes read, 851 more expected))

I tried several approaches with requests, httpx to talk to the graphql on geth.
Connected straight to the port as well as routing it through nginx. Seems geth is just chopping off data transfer.

Ideally you'd want to run a setup with a proper firewall config, and nginx or another webserver handling and limiting the traffic and timeouts for connections instead of exposing geth RPC / WSS directly to the internet. Hoping there's a parameter, read a through the issues here and the docs. Can't really figure out how to disable this.

@s1na
Copy link
Contributor

s1na commented Jan 15, 2024

@Doc-Pixel Please try adding the following to your config.toml file. This should increase the timeout to 60s.

[Node.HTTPTimeouts]
WriteTimeout = 60000000000

@Doc-Pixel
Copy link

Doc-Pixel commented Jan 17, 2024

Thank you!
:-)
It's still giving the errors, gut feeling is that it is a little bit better.
I'm reducing the amount of blocks per call for now.

pdrobnjak pushed a commit to Tenderly/net-scroll-geth that referenced this pull request Apr 30, 2025
…h#42)

* rpc: improve error codes for internal server errors (ethereum#25678)

This changes the error code returned by the RPC server in certain situations:

- handler panic: code -32603
- result marshaling error: code -32603
- attempt to subscribe via HTTP: code -32001

In all of the above cases, the server previously returned the default error
code -32000.

Co-authored-by: Nicholas Zhao <[email protected]>
Co-authored-by: Felix Lange <[email protected]>

* rpc: add PeerInfo (ethereum#24255)

This replaces the sketchy and undocumented string context keys for HTTP requests
with a defined interface. Using string keys with context is discouraged because
they may clash with keys created by other packages.

We added these keys to make connection metadata available in the signer, so this
change also updates signer/core to use the new PeerInfo API.

* graphql: add query timeout (ethereum#26116)

This PR adds a 60 second timeout to graphql queries.

* graphql, node, rpc: improve HTTP write timeout handling (ethereum#25457)

Here we add special handling for sending an error response when the write timeout of the
HTTP server is just about to expire. This is surprisingly difficult to get right, since is
must be ensured that all output is fully flushed in time, which needs support from
multiple levels of the RPC handler stack:

The timeout response can't use chunked transfer-encoding because there is no way to write
the final terminating chunk. net/http writes it when the topmost handler returns, but the
timeout will already be over by the time that happens. We decided to disable chunked
encoding by setting content-length explicitly.

Gzip compression must also be disabled for timeout responses because we don't know the
true content-length before compressing all output, i.e. compression would reintroduce
chunked transfer-encoding.

* eth/filters, eth/tracers: add request cancellation checks (ethereum#26320)

This ensures that RPC method handlers will react to a timeout or
cancelled request soon after the event occurs.

Co-authored-by: Sina Mahmoodi <[email protected]>

* rpc: add limit for batch request items and response size (ethereum#26681)

This PR adds server-side limits for JSON-RPC batch requests. Before this change, batches
were limited only by processing time. The server would pick calls from the batch and
answer them until the response timeout occurred, then stop processing the remaining batch
items.

Here, we are adding two additional limits which can be configured:

- the 'item limit': batches can have at most N items
- the 'response size limit': batches can contain at most X response bytes

These limits are optional in package rpc. In Geth, we set a default limit of 1000 items
and 25MB response size.

When a batch goes over the limit, an error response is returned to the client. However,
doing this correctly isn't always possible. In JSON-RPC, only method calls with a valid
`id` can be responded to. Since batches may also contain non-call messages or
notifications, the best effort thing we can do to report an error with the batch itself is
reporting the limit violation as an error for the first method call in the batch. If a batch is
too large, but contains only notifications and responses, the error will be reported with
a null `id`.

The RPC client was also changed so it can deal with errors resulting from too large
batches. An older client connected to the server code in this PR could get stuck
until the request timeout occurred when the batch is too large. **Upgrading to a version
of the RPC client containing this change is strongly recommended to avoid timeout issues.**

For some weird reason, when writing the original client implementation, @fjl worked off of
the assumption that responses could be distributed across batches arbitrarily. So for a
batch request containing requests `[A B C]`, the server could respond with `[A B C]` but
also with `[A B] [C]` or even `[A] [B] [C]` and it wouldn't make a difference to the
client.

So in the implementation of BatchCallContext, the client waited for all requests in the
batch individually. If the server didn't respond to some of the requests in the batch, the
client would eventually just time out (if a context was used).

With the addition of batch limits into the server, we anticipate that people will hit this
kind of error way more often. To handle this properly, the client now waits for a single
response batch and expects it to contain all responses to the requests.

---------

Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>

* format

* ethereum, ethclient: add FeeHistory support (ethereum#25403)

Co-authored-by: Felix Lange <[email protected]>

* internal/ethapi: return error when requesting invalid trie key (ethereum#25762)

This change makes eth_getProof and eth_getStorageAt return an error when
the argument contains invalid hex in storage keys.

Co-authored-by: Felix Lange <[email protected]>

* internal/ethapi: handle odd length hex in decodeHash (ethereum#25883)

This change adds zero-padding (prefix) of odd nibbles in the decodeHash function. 

Co-authored-by: ty <[email protected]>

* eth/filters, ethclient/gethclient: add fullTx option to pending tx filter (ethereum#25186)

This PR adds a way to subscribe to the _full_ pending transactions, as opposed to just being notified about hashes.

In use cases where client subscribes to newPendingTransactions and gets txhashes only to then request the actual transaction, the caller can now shortcut that flow and obtain the transactions directly.

Co-authored-by: Felix Lange <[email protected]>

* graphql: check header first in blocks query (ethereum#24190)

Fixes ethereum#24167

New behaviour is that the endpoint returns results only for available
blocks without returning an error when it doesn't find a block. Note we
skip any block after a non-existent block.

This adds a header fetch for every block in range (even if header
is not needed). Alternatively, we could do the check in every field's
resolver method to avoid this overhead.

* graphql: embed *Resolver instead of backend interface (ethereum#25468)

This creates some infrastructure to share resources between graphql
API objects.

* eth/filters: fix getLogs for pending block (ethereum#24949)

* eth/filters: fix pending for getLogs

* add pending method to test backend

* fix block range validation

* eth/filters: add global block logs cache (ethereum#25459)

This adds a cache for block logs which is shared by all filters. The cache
size of is configurable using the `--cache.blocklogs` flag.

Co-authored-by: Felix Lange <[email protected]>

* eth/filters: send rpctransactions in pending-subscription (ethereum#26126)

This PR changes the pending tx subscription to return RPCTransaction types instead of normal Transaction objects. This will fix the inconsistencies with other tx returning API methods (i.e. getTransactionByHash), and also fill in the sender value for the tx.

co-authored by @s1na

* rpc: fix unmarshaling of null result in CallContext (ethereum#26723)

The change fixes unmarshaling of JSON null results into json.RawMessage.

---------

Co-authored-by: Jason Yuan <[email protected]>
Co-authored-by: Jason Yuan <[email protected]>

* eth/filters: fix a breaking change and return rpctransaction (ethereum#26757)

* eth/filters: fix a breaking change and return rpctransaction

* eth/filters: fix test cases

---------

Co-authored-by: Catror <[email protected]>

---------

Co-authored-by: Nicholas <[email protected]>
Co-authored-by: Nicholas Zhao <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Ahmet Avci <[email protected]>
Co-authored-by: Sina Mahmoodi <[email protected]>
Co-authored-by: Sina Mahmoodi <[email protected]>
Co-authored-by: mmsqe <[email protected]>
Co-authored-by: Martin Holst Swende <[email protected]>
Co-authored-by: lightclient <[email protected]>
Co-authored-by: TY <[email protected]>
Co-authored-by: ty <[email protected]>
Co-authored-by: lmittmann <[email protected]>
Co-authored-by: Jason Yuan <[email protected]>
Co-authored-by: Jason Yuan <[email protected]>
Co-authored-by: Yier <[email protected]>
Co-authored-by: Catror <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants