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

Adding support for banning, un-banning TCP clients. #2496

Merged
merged 1 commit into from Jan 7, 2021

Conversation

ferencdg
Copy link
Contributor

This change will help to mitigate simple DOS attacks

foreach (ref ban_manager; virtual_host.settings.banManagers)
{
string remote_address = connection.remoteAddress().toAddressString();
ban_manager.onRequest(remote_address);
Copy link
Contributor Author

@ferencdg ferencdg Nov 20, 2020

Choose a reason for hiding this comment

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

Calling onRequest here will allow implementing more sophisticated BanManagers that could throttle the number of requests per clients.

Calling onFailedRequest/onSuccededRequest remains the responsibility of the app using vibe.d, as only that layer can decide what requests are valid from the application's point of view.

@ferencdg ferencdg force-pushed the ban-manager branch 3 times, most recently from 2a2b068 to bb7309e Compare November 26, 2020 14:59
@Geod24
Copy link
Contributor

Geod24 commented Nov 26, 2020

I think this is way too specific to our use case. What I (originally) had in mind was just the ability to have a callback that returns bool, not to move the BanManager in Vibe.d

@ferencdg
Copy link
Contributor Author

I think this is way too specific to our use case. What I (originally) had in mind was just the ability to have a callback that returns bool, not to move the BanManager in Vibe.d

I was thinking that vibe.d would also need this feature to handle the following scenraio: each IP can call REST endpoints only a certain number of times. This would be implemented by a different kind of BanManager that implements IBanManager and that implementation most probably should be in vibe.d

@Geod24
Copy link
Contributor

Geod24 commented Nov 26, 2020

I think this could be handled by just passing the HTTPServerRequest to the client and let the client decide. It can use whatever is there then.

@ferencdg
Copy link
Contributor Author

I think this could be handled by just passing the HTTPServerRequest to the client and let the client decide. It can use whatever is there then.

that is true, but then every client would need to write its own versioin versus having some useful/common ban logic in vibe.d

so in your suggested solution vibe.d would only receive a callback function called isBanned. This is very similar to having IBanManager with only 1 method: isBanned. In fact currently Rest only calls 2 methods on the IBanManager: isBanned, onRequest, this is quite similar to what you suggest I think.

Maybe I should would narrow the IBanManager interface to those 2(or maybe just to the 1) methods and move out our BanManager implementation from vibe.d(although other projects cannot use that implementation then)

@s-ludwig
Copy link
Member

I feel like this functionality should be fully fleshed out in a separate package before actually integrating it into the core library. It definitely has valid uses outside of the HTTP sub system, too, and I think that more metrics that are useful for the banning decision will emerge. Considering that vibe.http is in the (suspended) process of going to 1.0.0, it would be a bad idea to add APIs that are likely to be changed again.

What would probably make sense though is the isBanned callback in the form of a single bool delegate(in ref NetworkAddress) @safe nothrow, because then at some point the decision could be done before actually establishing the connection, at least on Windows using the lpfnCondition argument to WSAAccept.

@Geod24
Copy link
Contributor

Geod24 commented Nov 27, 2020

What would probably make sense though is the isBanned callback in the form of a single bool delegate(in ref NetworkAddress) @safe nothrow, because then at some point the decision could be done before actually establishing the connection, at least on Windows using the lpfnCondition argument to WSAAccept.

So IIUC, replace L653 (https://github.com/vibe-d/vibe.d/pull/2496/files#diff-982fb1ea1452e69afa1364a57a9ad8ff741507348c9cf29e4fd65da0ea5d7200R653) with a delegate ?
And yes, refusing the connection as early as possible is the use case here.

@s-ludwig
Copy link
Member

@ferencdg ferencdg force-pushed the ban-manager branch 2 times, most recently from 24ebd74 to 5e0ed18 Compare November 30, 2020 12:36
Copy link
Contributor

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Two things:

  • I think this should be handled directly at the TCP connection level, not just the HTTP;
    But then I went and looked at vibe-core and realized there was no way to do this without invasive changes, so let's keep this idea on the shelve;
  • I think the whole "banman" package should go. We only need the change in vibe/http/server.d so far.

If any of the ban managers bans an address, then all incoming requests
from that address will be rejected.
*/
alias IsBannedDlg = bool delegate (ref in NetworkAddress) @safe nothrow;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
alias IsBannedDlg = bool delegate (ref in NetworkAddress) @safe nothrow;
alias IsBannedDg = bool delegate (in NetworkAddress) @safe nothrow;

from that address will be rejected.
*/
alias IsBannedDlg = bool delegate (ref in NetworkAddress) @safe nothrow;
IsBannedDlg[string] isBannedDlgs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an AA ? A simple delegate would do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was to allow multiple different types of BanManagers and if any of them would drop the connection, then it will be dropped. Should I just allow 1 BanManager?

Copy link
Contributor

Choose a reason for hiding this comment

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

One delegate is enough. If a user needs multiple delegates, they can just wrap them in one delegate that do the iteration.

@ferencdg ferencdg force-pushed the ban-manager branch 5 times, most recently from 8a97fbb to d546dfc Compare January 7, 2021 10:57
Comment on lines 647 to 648
*/
alias IsBannedDg = bool delegate (in NetworkAddress) @safe nothrow;
IsBannedDg isBannedDg;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*/
alias IsBannedDg = bool delegate (in NetworkAddress) @safe nothrow;
IsBannedDg isBannedDg;
*/
IsBannedDg isBannedDg;
/// Type of delegate accepted ffor `isBannedDg`
alias IsBannedDg = bool delegate (in NetworkAddress) @safe nothrow;

Otherwise the DDOC will be wrong.

This change will help to mitigate simple DOS attacks
@dlang-bot dlang-bot merged commit 80ddafd into vibe-d:master Jan 7, 2021
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.

None yet

4 participants