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

Limit concurrent offers that can be received from each peer in Fluffy #2874

Closed
bhartnett opened this issue Nov 26, 2024 · 8 comments
Closed
Assignees
Labels
Fluffy optimization Security Security vulnerability or security related

Comments

@bhartnett
Copy link
Contributor

In order to reduce the impact of certain DOS attacks we should put limits in place to reduce the impact on Fluffy nodes.

We should enforce the following:

  • For each content key/content id we can only receive one concurrent offer from each peer/enr for that content
  • Limit the total number of offers that can be received concurrently from each peer/enr. This limit will be configurable as a debug parameter.

Nodes that hit the total number of offers limit will be added to the temporary ban list.

@bhartnett bhartnett added Fluffy Security Security vulnerability or security related optimization labels Nov 26, 2024
@kdeme
Copy link
Contributor

kdeme commented Nov 26, 2024

Yes, I agree that these would be two good limits to have enabled.

@bhartnett bhartnett self-assigned this Nov 28, 2024
@bhartnett
Copy link
Contributor Author

@kdeme Do you think that content lookups should be included in the limits?

In other words, when a node asks a peer for content to download it should be limited to one concurrent download/upload per contentId and limited to n concurrent downloads/uploads in total. When I say download I mean requesting content from a peer and when I say upload I mean offering content to a peer.

@bhartnett
Copy link
Contributor Author

Should we also limit the number of concurrent pings and findNodes requests which we respond to?

@kdeme
Copy link
Contributor

kdeme commented Nov 28, 2024

Should we also limit the number of concurrent pings and findNodes requests which we respond to?

I think this would be a different type of limit that is more of a rate limiter.

@kdeme
Copy link
Contributor

kdeme commented Nov 28, 2024

Do you think that content lookups should be included in the limits?

Not fully sure on this one. It seems more like the responsibility of the application not to do such things. I mean, it's in the hands of the local node and not its peers for this to occur. But maybe there are situations where this can anyhow happen, although probably rare.

@bhartnett
Copy link
Contributor Author

Not fully sure on this one. It seems more like the responsibility of the application not to do such things. I mean, it's in the hands of the local node and not its peers for this to occur. But maybe there are situations where this can anyhow happen, although probably rare.

What I meant is limiting the number of content lookups which we handle/respond to. My reasoning for applying the limit here as well is that we need to open a utp stream and so it seems reasonable to prevent a single peer requesting too much content. I've implemented this already but happy to revert if we don't want to go this way. See here: a96a134

@kdeme
Copy link
Contributor

kdeme commented Nov 28, 2024

Oh, yeah. I misunderstood as I already noticed that feature in your PR.

So yes, that definitely sounds good as a limit protection to have considering it is incoming from other peers.

@bhartnett
Copy link
Contributor Author

The changes for this task were implemented in this PR: #2885

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fluffy optimization Security Security vulnerability or security related
Projects
None yet
Development

No branches or pull requests

2 participants