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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Rate limiter configurable #3517

Merged
merged 1 commit into from Sep 8, 2018

Conversation

@tu-maurice
Copy link
Contributor

commented Sep 7, 2018

This makes the previously added rate limiter configurable. The change adds messagelimit and messageburst to the configuration file murmur.ini, the ServerDB as well as the ability to set these live. Because setting either of these to a value of 0 would render the Server unusable, both have a minimum of 1. The defaults are kept as before with a messagelimit of 1 and a messageburst of 5.

Though adjusting these live is entirely possible on the serverside, they currently only take effect for new connections. If they should take effect immediately for all present connections, I could add some more changes that allow that if you want.

I tested this change by printing either value on a new connection, as well as rapidly muting and unmuting myself with a messagelimit of 100 and a burstlimit of 500, without being able to get into any rate limit. Also setting either value to 0 in the murmur.ini, showed the expected 1 message per second with a maximum burst of 1 message. However this obviously felt very restricting, so I don't recommend it. 馃槢

Please review the changes and tell me if I forgot something or made a mistake according to the coding style. Alternatively you can edit the change yourself if you prefer.


iMessageLimit=getConf("messagelimit", iMessageLimit).toUInt();
if (iMessageLimit < 1) // Prevent disabling messages entirely
iMessageLimit = 1;

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Sep 7, 2018

Member
// Prevent disabling messages entirely
if (iMessageLimit < 1) {
	iMessageLimit = 1;
}

This comment has been minimized.

Copy link
@tu-maurice

tu-maurice Sep 7, 2018

Author Contributor

With or without a { 馃槢

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Sep 7, 2018

Member

With a {, sorry.

This comment has been minimized.

Copy link
@tu-maurice

tu-maurice Sep 7, 2018

Author Contributor

No problem

else if (key == "messagelimit") {
iMessageLimit = (!v.isNull()) ? v.toUInt() : Meta::mp.iMessageLimit;
if(iMessageLimit < 1)
iMessageLimit = 1;

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Sep 7, 2018

Member
if (iMessageLimit < 1) {
	iMessageLimit = 1;
}
iMessageLimit = 1;
iMessageBurst=getConf("messageburst", iMessageBurst).toUInt();
if (iMessageBurst < 1) // Prevent disabling messages entirely
iMessageBurst = 1;

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Sep 7, 2018

Member
// Prevent disabling messages entirely
if (iMessageBurst < 1) {
	iMessageBurst = 1;
}
} else if (key == "messageburst") {
iMessageBurst = (!v.isNull()) ? v.toUInt() : Meta::mp.iMessageBurst;
if(iMessageBurst < 1)
iMessageBurst = 1;

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Sep 7, 2018

Member
if (iMessageBurst < 1) {
	iMessageBurst = 1;
}
@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

See my comments for the code style.

Aside from that, everything looks good to me.

@tu-maurice tu-maurice force-pushed the tu-maurice:exploitpatch branch 2 times, most recently from 8e2bdfc to dee6353 Sep 7, 2018

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

There we go. I also saw that some of my if didn't have the space after it so I adjusted that too.

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

The two parameters should also be added and commented in murmur.ini: https://github.com/mumble-voip/mumble/blob/master/scripts/murmur.ini

@tu-maurice tu-maurice force-pushed the tu-maurice:exploitpatch branch from dee6353 to 4bd9709 Sep 7, 2018

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

Is the text alright?

scripts/murmur.ini Outdated Show resolved Hide resolved
scripts/murmur.ini Outdated Show resolved Hide resolved
scripts/murmur.ini Outdated Show resolved Hide resolved
scripts/murmur.ini Outdated Show resolved Hide resolved
scripts/murmur.ini Outdated Show resolved Hide resolved
scripts/murmur.ini Outdated Show resolved Hide resolved
@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Reviewed.

@tu-maurice tu-maurice force-pushed the tu-maurice:exploitpatch branch from 4bd9709 to 290ca68 Sep 7, 2018

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

Adjusted.

Make Rate limiter configurable.
This adds messagelimit and messageburst to the configuration file murmur.ini as
well as the ability to set these live.

Though adjusting these live is entirely possible, they only take effect for new connections.

@tu-maurice tu-maurice force-pushed the tu-maurice:exploitpatch branch from 290ca68 to 73a0b2f Sep 7, 2018

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2018

My bad. Had to amend the commit again due to the wrong name in the commit.

@davidebeatrici davidebeatrici merged commit b44b1f2 into mumble-voip:master Sep 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Sep 8, 2018

Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.