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

routing/http/server: limit response sizes #15

Closed
guseggert opened this issue Dec 7, 2022 · 7 comments
Closed

routing/http/server: limit response sizes #15

guseggert opened this issue Dec 7, 2022 · 7 comments
Assignees
Labels
dif/easy Someone with a little familiarity can pick up kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked

Comments

@guseggert
Copy link
Contributor

Complementary to #9

By default we should limit response sizes of the server to whatever the client can accept by default (which is current 1 MiB).

@guseggert guseggert added need/triage Needs initial labeling and prioritization P1 High: Likely tackled by core team if no one steps up dif/easy Someone with a little familiarity can pick up kind/enhancement A net-new feature or improvement to an existing feature status/ready Ready to be worked and removed need/triage Needs initial labeling and prioritization labels Dec 7, 2022
@ipfs ipfs deleted a comment from welcome bot Dec 7, 2022
@guseggert guseggert moved this to 🥞 Todo in IPFS Shipyard Team Dec 7, 2022
Jorropo pushed a commit that referenced this issue Mar 27, 2023
* disable Travis

* add version.json file

* add .github/workflows/automerge.yml

* add .github/workflows/go-test.yml

* add .github/workflows/go-check.yml

* add .github/workflows/releaser.yml

* add .github/workflows/release-check.yml

* add .github/workflows/tagpush.yml

* fix: field cid is unused (U1000)

https://github.com/ipfs/go-pinning-service-http-client/runs/5594929290?check_suite_focus=true#step:10:31

Co-authored-by: web3-bot <web3-bot@users.noreply.github.com>
Co-authored-by: Marcin Rataj <lidel@lidel.org>

This commit was moved from ipfs/go-pinning-service-http-client@014aba0
@ethanbanez
Copy link

hi @guseggert I'm new to contributing. I saw the recommendation in #9 to use the standard libraries provided functionality for the server but I'm confused at where the default server option should be implemented. Should it be in the handler?

@BigLep
Copy link
Contributor

BigLep commented May 11, 2023

@guseggert : can you provide pointers here please?

@lidel
Copy link
Member

lidel commented Oct 5, 2023

It feels important to have this, towards productizing Gateway.ExposeRoutingAPI in Kubo 0.24+.

The routing/http/server library could have configuration for setting /routing/v1 response limits:

@hacdias thoughts on making the above configurable and exposing in Kubo in Routing.DelegatedServer (I think we want to make it a map with related flags, similar to RelayService)?

@hacdias
Copy link
Member

hacdias commented Oct 10, 2023

@lidel I think it is a good idea to let this options be configurable in Kubo. However, I don't need any additional changes need to be made to Boxo. Let's see:

  1. Max number of results: already possible via server.WithRecordsLimit and server.WithStreamingRecordsLimit.
  2. Max byte budget: the PR you link is for client. I don't understand how that would be useful for server.
  3. Max time budget per request: server implementers provide a server.ContentRouter implementation. They can define their timeouts there, no need to add more options to Boxo, unless we want some default IMHO.

What do you think?

@hacdias hacdias self-assigned this Oct 10, 2023
@lidel
Copy link
Member

lidel commented Oct 10, 2023

(1) and (3) sgtm, for (2) I think the problem is real for server too: someone could publish malicious provider or peer records and then ask routing server to resolve and return them and response could have very long multiaddrs or additional metadata in binary blobs returned for specific protocol (that could act as DoS vector )

@BigLep BigLep mentioned this issue Nov 9, 2023
11 tasks
@hacdias
Copy link
Member

hacdias commented Nov 29, 2023

@lidel still regarding 2: shouldn't that be included in the implementation of server.ContentRouter itself?

@lidel
Copy link
Member

lidel commented Jan 8, 2024

We've discussed this today, and there nothing left to do on the boxo:

  • in case of someguy or kubo, we have middleware-proxy type of situation, so limit will be applied on the "client" side
  • additional timeouts and limits can be set on reverse proxy like nginx

@lidel lidel closed this as completed Jan 8, 2024
@github-project-automation github-project-automation bot moved this from 🥞 Todo to 🎉 Done in IPFS Shipyard Team Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dif/easy Someone with a little familiarity can pick up kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked
Projects
No open projects
Archived in project
Development

No branches or pull requests

5 participants