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

Add autobanSuccessfulConnections flag. #4087

Merged
merged 2 commits into from Apr 21, 2020
Merged

Add autobanSuccessfulConnections flag. #4087

merged 2 commits into from Apr 21, 2020

Conversation

seantalts
Copy link
Contributor

@seantalts seantalts commented Apr 20, 2020

The idea here is that sometimes you really do have a lot of folks connecting from a single IP,
and if those connections are successful you don't want to ban any of them.
However, in cases where the server needs to guard against malicious users attempting a DDOS
by reconnecting their valid user account over and over, we need to be able to configure the
server to still ban those successful attempts.

How does this idea sound, and how does this code look for solving it? Thanks!

Changelog

| Added: autobanSuccessfulConnection configuration option

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

Also please prefix your commit message. When I change multiple files in a single commit, I use the common ancestor as the Prefix. In this case this would be src/murmur:.

src/murmur/Meta.cpp Outdated Show resolved Hide resolved
src/murmur/Meta.cpp Outdated Show resolved Hide resolved
src/murmur/Meta.h Show resolved Hide resolved
src/murmur/Meta.cpp Outdated Show resolved Hide resolved
@seantalts
Copy link
Contributor Author

Will fix! What does prefix my commit message mean?

@seantalts
Copy link
Contributor Author

Oh, I think I see what you're talking about from one of your commit messages. I'll try to emulate that!

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Apr 20, 2020

Just add a Prefix to your commit message like so:

src/murmur: <your message>

This makes it easier for us if we need to process commits programmatically as we can then find out which parts of the code was modified.

@Krzmbrzl
Copy link
Member

Looks good now. I'll have to use my real pc tomorrow though to have a deeper look at the current implementation to verify that this fits in nicely :)

@seantalts
Copy link
Contributor Author

Thanks!

@seantalts
Copy link
Contributor Author

For your reference tomorrow, I think the ban logic all lives here: https://github.com/mumble-voip/mumble/blob/master/src/murmur/Meta.cpp#L750. Seems like Meta keeps track of a hash table from client IP to a list of timers representing how long ago they tried to connect.

The idea here is that sometimes you really do have a lot of folks connecting from a single IP,
and if those connections are successful you don't want to ban any of them.
However, in cases where the server needs to guard against malicious users attempting a DDOS
by reconnecting their valid user account over and over, we need to be able to configure the
server to still ban those successful attempts.
Instead, we'll turn off the autoban feature if either are negative.
@Krzmbrzl Krzmbrzl merged commit c84f0a5 into mumble-voip:master Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants